From d19dfeb56c91f47d93b7f5e1ab7087524c8817df Mon Sep 17 00:00:00 2001 From: Lee Clissett Date: Tue, 1 Sep 2026 14:59:34 +0200 Subject: [PATCH] feat: generate readable default newsletter slugs --- backend/app/core/slug.py | 16 ++ backend/app/crud/newsletters.py | 87 +++++++-- backend/app/tests/test_email_processor.py | 28 +++ backend/app/tests/test_slugs.py | 167 +++++++++++++++++- .../letterfeed/NewsletterDialog.tsx | 9 +- 5 files changed, 284 insertions(+), 23 deletions(-) diff --git a/backend/app/core/slug.py b/backend/app/core/slug.py index eb285e9..c3010b1 100644 --- a/backend/app/core/slug.py +++ b/backend/app/core/slug.py @@ -1,4 +1,5 @@ import re +import unicodedata def sanitize_slug(slug: str | None) -> str | None: @@ -16,3 +17,18 @@ def sanitize_slug(slug: str | None) -> str | None: slug = re.sub(r"[^a-z0-9-]", "", slug) slug = slug.strip("-") return slug or None + + +def slugify_name(name: str) -> str | None: + """Create a readable, URL-safe slug from a newsletter name.""" + normalized_name = unicodedata.normalize("NFKD", name.casefold()) + normalized_name = "".join( + character + for character in normalized_name + if not unicodedata.combining(character) + ) + normalized_name = normalized_name.replace("'", "").replace("’", "") + normalized_name = re.sub(r"[\W_]+", "-", normalized_name) + normalized_name = normalized_name.encode("ascii", "ignore").decode("ascii") + slug = re.sub(r"[^a-z0-9]+", "-", normalized_name).strip("-") + return slug or None diff --git a/backend/app/crud/newsletters.py b/backend/app/crud/newsletters.py index bbd1844..c46c714 100644 --- a/backend/app/crud/newsletters.py +++ b/backend/app/crud/newsletters.py @@ -1,13 +1,16 @@ from nanoid import generate from sqlalchemy import func, or_ +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from app.core.logging import get_logger +from app.core.slug import slugify_name from app.models.entries import Entry from app.models.newsletters import Newsletter, Sender from app.schemas.newsletters import NewsletterCreate, NewsletterUpdate logger = get_logger(__name__) +_RESERVED_FEED_SLUGS = {"all"} def get_newsletter_by_identifier(db: Session, identifier: str): @@ -32,6 +35,37 @@ def get_newsletter_by_slug(db: Session, slug: str): return db.query(Newsletter).filter(Newsletter.slug == slug).first() +def _slug_is_unavailable( + db: Session, slug: str, exclude_newsletter_id: str | None = None +) -> bool: + """Check whether a slug would conflict with a feed route or identifier.""" + if slug in _RESERVED_FEED_SLUGS: + return True + + query = db.query(Newsletter.id).filter( + or_(Newsletter.slug == slug, Newsletter.id == slug) + ) + if exclude_newsletter_id: + query = query.filter(Newsletter.id != exclude_newsletter_id) + + return query.first() is not None + + +def _generate_unique_slug(db: Session, name: str) -> str | None: + """Generate an available slug from a newsletter name.""" + base_slug = slugify_name(name) + if not base_slug: + return None + + candidate = base_slug + suffix = 2 + while _slug_is_unavailable(db, candidate): + candidate = f"{base_slug}-{suffix}" + suffix += 1 + + return candidate + + def get_newsletters(db: Session, skip: int = 0, limit: int = 100): """Retrieve a list of newsletters.""" logger.debug(f"Querying for newsletters with skip={skip}, limit={limit}") @@ -57,19 +91,38 @@ def create_newsletter(db: Session, newsletter: NewsletterCreate): """Create a new newsletter.""" logger.info(f"Creating new newsletter with name '{newsletter.name}'") - if newsletter.slug and get_newsletter_by_slug(db, newsletter.slug): - return None # Indicates a conflict + if newsletter.slug: + if _slug_is_unavailable(db, newsletter.slug): + return None # Indicates a conflict + slug = newsletter.slug + else: + slug = _generate_unique_slug(db, newsletter.name) + + while True: + db_newsletter = Newsletter( + id=generate(size=10), + name=newsletter.name, + slug=slug, + search_folder=newsletter.search_folder, + extract_content=newsletter.extract_content, + move_to_folder=newsletter.move_to_folder, + ) + db.add(db_newsletter) + try: + db.commit() + break + except IntegrityError: + db.rollback() + if newsletter.slug: + if _slug_is_unavailable(db, newsletter.slug): + return None # Indicates a conflict + raise + + next_slug = _generate_unique_slug(db, newsletter.name) + if not next_slug or next_slug == slug: + raise + slug = next_slug - db_newsletter = Newsletter( - id=generate(size=10), - name=newsletter.name, - slug=newsletter.slug, - search_folder=newsletter.search_folder, - extract_content=newsletter.extract_content, - move_to_folder=newsletter.move_to_folder, - ) - db.add(db_newsletter) - db.commit() db.refresh(db_newsletter) for email in newsletter.sender_emails: @@ -93,10 +146,12 @@ def update_newsletter( if not db_newsletter: return None - if newsletter_update.slug: - existing_newsletter = get_newsletter_by_slug(db, newsletter_update.slug) - if existing_newsletter and existing_newsletter.id != newsletter_id: - return "conflict" # Indicates a conflict + if ( + newsletter_update.slug + and newsletter_update.slug != db_newsletter.slug + and _slug_is_unavailable(db, newsletter_update.slug, newsletter_id) + ): + return "conflict" # Indicates a conflict update_data = newsletter_update.model_dump(exclude_unset=True) for key, value in update_data.items(): diff --git a/backend/app/tests/test_email_processor.py b/backend/app/tests/test_email_processor.py index d1fd1de..4d61b09 100644 --- a/backend/app/tests/test_email_processor.py +++ b/backend/app/tests/test_email_processor.py @@ -225,6 +225,34 @@ def test_process_single_email_with_encoded_from_header(db_session: Session): assert newsletters[0].senders[0].email == "test@example.com" +def test_process_single_email_generates_slug_for_auto_added_newsletter( + db_session: Session, +): + """Test deriving a slug from an auto-detected newsletter name.""" + settings_data = SettingsCreate( + imap_server="test.com", + imap_username="test", + imap_password="password", + auto_add_new_senders=True, + ) + settings = create_or_update_settings(db_session, settings_data) + + mock_mail = MagicMock(spec=imaplib.IMAP4_SSL) + msg = Message() + msg["From"] = "The Daily & Weekly " + msg["Subject"] = "Test Email" + msg["Message-ID"] = "" + msg.set_payload("

Body

", "utf-8") + mock_mail.fetch.return_value = ("OK", [(b"1 (RFC822)", msg.as_bytes())]) + + sender_map = {} + _process_single_email("1", mock_mail, db_session, sender_map, settings) + + newsletter = sender_map["daily-weekly@example.com"] + assert newsletter.name == "The Daily & Weekly" + assert newsletter.slug == "the-daily-weekly" + + def test_process_single_email_with_null_bytes_in_body(db_session: Session): """Test that an email with NULL bytes in its body is handled gracefully. diff --git a/backend/app/tests/test_slugs.py b/backend/app/tests/test_slugs.py index 5b8b0fd..699e51c 100644 --- a/backend/app/tests/test_slugs.py +++ b/backend/app/tests/test_slugs.py @@ -1,8 +1,13 @@ +from unittest.mock import MagicMock, patch + import pytest from fastapi.testclient import TestClient +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session -from app.core.slug import sanitize_slug +from app.core.slug import sanitize_slug, slugify_name +from app.crud.newsletters import create_newsletter +from app.schemas.newsletters import NewsletterCreate @pytest.mark.parametrize( @@ -22,6 +27,26 @@ def test_sanitize_slug(input_slug, expected_slug): assert sanitize_slug(input_slug) == expected_slug +@pytest.mark.parametrize( + "name, expected_slug", + [ + ("Foo & Bar", "foo-bar"), + ("Caf\u00e9 D\u00e9j\u00e0 Vu", "cafe-deja-vu"), + ("What's New?", "whats-new"), + ("Morning\u2014Brief", "morning-brief"), + ("\u041a\u0438\u0440\u0438\u043b\u043b", None), + ], +) +def test_slugify_name(name, expected_slug): + """Test creating pretty, URL-safe slugs from newsletter names.""" + assert slugify_name(name) == expected_slug + + +def test_sanitize_slug_preserves_legacy_repeated_hyphens(): + """Test keeping existing custom feed URLs stable when they are revalidated.""" + assert sanitize_slug("daily--brief") == "daily--brief" + + def test_create_newsletter_with_slug(client: TestClient, db_session: Session): """Test creating a newsletter with a custom slug.""" newsletter_data = { @@ -53,7 +78,7 @@ def test_create_newsletter_with_sanitization(client: TestClient, db_session: Ses def test_create_newsletter_without_slug(client: TestClient, db_session: Session): - """Test creating a newsletter without a slug, expecting it to be None.""" + """Test deriving a slug from the name when one is not supplied.""" newsletter_data = { "name": "No Slug Newsletter", "sender_emails": ["no-slug@example.com"], @@ -61,13 +86,103 @@ def test_create_newsletter_without_slug(client: TestClient, db_session: Session) response = client.post("/newsletters", json=newsletter_data) assert response.status_code == 200 data = response.json() - assert data["slug"] is None + assert data["slug"] == "no-slug-newsletter" - # Verify the feed URL uses the ID - feed_response = client.get(f"/feeds/{data['id']}") + # Verify the feed URL uses the generated slug + feed_response = client.get(f"/feeds/{data['slug']}") assert feed_response.status_code == 200 +def test_create_newsletter_with_blank_slug(client: TestClient, db_session: Session): + """Test the blank slug sent by the manual add form.""" + response = client.post( + "/newsletters", + json={ + "name": "The Daily & Weekly", + "slug": "", + "sender_emails": ["daily-weekly@example.com"], + }, + ) + + assert response.status_code == 200 + assert response.json()["slug"] == "the-daily-weekly" + + +def test_create_newsletters_with_duplicate_generated_slugs( + client: TestClient, db_session: Session +): + """Test suffixing generated slugs when newsletter names match.""" + first_response = client.post( + "/newsletters", + json={ + "name": "Daily Brief", + "sender_emails": ["first-daily@example.com"], + }, + ) + second_response = client.post( + "/newsletters", + json={ + "name": "Daily Brief", + "sender_emails": ["second-daily@example.com"], + }, + ) + + assert first_response.status_code == 200 + assert first_response.json()["slug"] == "daily-brief" + assert second_response.status_code == 200 + assert second_response.json()["slug"] == "daily-brief-2" + + +def test_generated_slug_retries_after_unique_constraint_race(): + """Test retrying when a generated slug is taken during insertion.""" + db = MagicMock(spec=Session) + db.commit.side_effect = [ + IntegrityError("INSERT", {}, Exception("unique constraint")), + None, + None, + ] + newsletter_data = NewsletterCreate( + name="Daily Brief", sender_emails=["race@example.com"] + ) + + with patch( + "app.crud.newsletters._generate_unique_slug", + side_effect=["daily-brief", "daily-brief-2"], + ): + newsletter = create_newsletter(db, newsletter_data) + + assert newsletter.slug == "daily-brief-2" + db.rollback.assert_called_once() + + +def test_generated_slug_avoids_reserved_feed_path( + client: TestClient, db_session: Session +): + """Test avoiding the master feed's reserved `all` path.""" + response = client.post( + "/newsletters", + json={"name": "All", "sender_emails": ["all@example.com"]}, + ) + + assert response.status_code == 200 + assert response.json()["slug"] == "all-2" + + +def test_non_slugifiable_name_falls_back_to_id( + client: TestClient, db_session: Session +): + """Test retaining the ID fallback when a safe ASCII slug cannot be made.""" + response = client.post( + "/newsletters", + json={"name": "\u041a\u0438\u0440\u0438\u043b\u043b", "sender_emails": ["cyrillic@example.com"]}, + ) + + assert response.status_code == 200 + data = response.json() + assert data["slug"] is None + assert client.get(f"/feeds/{data['id']}").status_code == 200 + + def test_create_newsletter_with_conflicting_slug( client: TestClient, db_session: Session ): @@ -120,3 +235,45 @@ def test_update_newsletter_with_conflicting_slug( response = client.put(f"/newsletters/{newsletter1_id}", json=update_data) assert response.status_code == 409 assert response.json()["detail"] == "Slug already in use" + + +def test_update_newsletter_keeps_generated_slug_when_slug_is_omitted( + client: TestClient, db_session: Session +): + """Test preserving a generated feed URL when only the name changes.""" + create_response = client.post( + "/newsletters", + json={"name": "Original Name", "sender_emails": ["rename@example.com"]}, + ) + newsletter_id = create_response.json()["id"] + + update_response = client.put( + f"/newsletters/{newsletter_id}", + json={"name": "Renamed", "sender_emails": ["rename@example.com"]}, + ) + + assert update_response.status_code == 200 + assert update_response.json()["slug"] == "original-name" + + +def test_update_newsletter_rejects_reserved_slug( + client: TestClient, db_session: Session +): + """Test preventing updates from shadowing the master feed route.""" + create_response = client.post( + "/newsletters", + json={"name": "Original", "sender_emails": ["reserved@example.com"]}, + ) + newsletter_id = create_response.json()["id"] + + update_response = client.put( + f"/newsletters/{newsletter_id}", + json={ + "name": "Original", + "slug": "all", + "sender_emails": ["reserved@example.com"], + }, + ) + + assert update_response.status_code == 409 + assert update_response.json()["detail"] == "Slug already in use" diff --git a/frontend/src/components/letterfeed/NewsletterDialog.tsx b/frontend/src/components/letterfeed/NewsletterDialog.tsx index 94e4ad1..4c8f87d 100644 --- a/frontend/src/components/letterfeed/NewsletterDialog.tsx +++ b/frontend/src/components/letterfeed/NewsletterDialog.tsx @@ -142,13 +142,18 @@ export function NewsletterDialog({ newsletter, isOpen, folderOptions, onOpenChan
- + setFormData((prev) => ({ ...prev, slug: e.target.value }))} - placeholder="my-custom-url" + placeholder={isEditMode ? "my-custom-url" : "Generated from newsletter name"} /> + {!isEditMode && ( +

+ Leave blank to generate a URL from the newsletter name. +

+ )}