Auto slug/summary/SEO and deterministic tag colours #65
Reference in New Issue
Block a user
Delete Branch "feature/auto-slug-summary-and-tag-colours"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Closes #61 and #63.
ArticlePage.save()now auto-generates slug from title and populatessearch_descriptionfrom summary when empty, reducing manual SEO work. The admin form also auto-fillssearch_description.TagMetadatasnippet colour assignment with a deterministic hash-based system. Tags automatically get a consistent colour from a 12-entry palette (cyan, pink, amber, emerald, violet, rose, sky, lime, orange, fuchsia, teal, indigo).TagMetadatastill works as an explicit override.Test plan
🤖 Generated with Claude Code
QA Review — PR #65: Auto slug/summary/SEO and deterministic tag colours
Overall
Good implementation of both #61 and #63. The deterministic hash-based tag colour system is well-designed (case-insensitive, good palette distribution), the auto-slug/summary/SEO features work correctly through the admin form path, the
_resolve_tag_cssrefactoring eliminates duplication in template tags, and the Tailwind content config correctly includesmodels.pyso all palette classes are scanned at build time. Test coverage for the colour system is solid.Two issues need fixing before merge — both are small, targeted changes.
Issues
1. Eager default evaluation in
TagMetadata.get_css_classes()(performance bug)mapping.get(self.colour, get_auto_tag_colour_css(self.tag.name))— Python evaluates thedefaultargument eagerly before callingdict.get(). This meansget_auto_tag_colour_css(self.tag.name)runs on every call, even whenself.colouris a valid key ("cyan", "pink", "neutral") and the computed result is silently discarded. The impact:self.tag.nameis accessed every time, which can trigger an extra DB query whenmetawas fetched viaTagMetadata.objects.filter(tag=tag).first()rather than prefetched viatag.metadataFix: replace with explicit
if/else:2.
test_article_save_auto_generates_slug_from_titledoesn't test auto-generationThe test explicitly provides
slug="my-great-article"and asserts the slug remains"my-great-article". Sincesave()only auto-generates when slug is falsy (not getattr(self, "slug", "")), this test never exercises the auto-generation path — it's actually testing "preserve explicit slug". The model-level auto-slug feature is effectively untested.Fix: use
slug=""and assert the slug is derived from the title, e.g.:Minor observation (non-blocking)
_auto_slug_from_titlereturnsbase_slugwithout sibling collision checking whenself.pk is None(new page). The admin form's_build_unique_page_slughandles this correctly since it hasparent_pagefrom the form context, so this only affects programmatic creation. Worth a comment in the code noting this limitation, or a note in the test for completeness.@@ -204,3 +276,3 @@}return mapping.get(self.colour, self.get_fallback_css())return mapping.get(self.colour, get_auto_tag_colour_css(self.tag.name))Performance bug — eager default evaluation. Python evaluates
get_auto_tag_colour_css(self.tag.name)before callingdict.get(), so it runs on every call even whenself.colourmatches a valid key. This means an MD5 hash is computed and potentially an extra DB query is fired (to loadself.tag) — then the result is silently discarded.Fix:
@@ -95,0 +115,4 @@def test_auto_tag_colour_is_case_insensitive():"""Tag colour assignment is case-insensitive."""assert get_auto_tag_colour_css("Python") == get_auto_tag_colour_css("python")This test doesn't exercise auto-generation.
slug="my-great-article"is truthy, so thesave()conditionnot getattr(self, "slug", "")isFalseand the auto-slug path is never entered. This test verifies "preserve explicit slug", not "auto-generate slug from title".To test actual auto-generation, pass
slug=""and assert the result:Both issues from the previous review are resolved cleanly:
get_css_classes()now uses explicitif/else, avoiding the unnecessaryself.tag.nameaccess and MD5 computation on every call.slug=""and usesrefresh_from_db(), properly exercising the auto-generation path.Tests pass (0 failures, 93.58% coverage). Approving.