diff --git a/MPCAutofill/cardpicker/admin.py b/MPCAutofill/cardpicker/admin.py index 5df2ca559..cee320d20 100644 --- a/MPCAutofill/cardpicker/admin.py +++ b/MPCAutofill/cardpicker/admin.py @@ -31,8 +31,8 @@ # Register your models here. @admin.register(Tag) class AdminTag(admin.ModelAdmin[Tag]): - list_display = ("name",) - search_fields = ("name",) + list_display = ("name", "display_name") + search_fields = ("name", "display_name") @admin.register(Card) diff --git a/MPCAutofill/cardpicker/default_tags.py b/MPCAutofill/cardpicker/default_tags.py index 99281037d..f5c47679f 100644 --- a/MPCAutofill/cardpicker/default_tags.py +++ b/MPCAutofill/cardpicker/default_tags.py @@ -6,43 +6,61 @@ `cardpicker.printing_candidates`'s `expansion_hint` handling for those instead. """ +from typing import Optional + from cardpicker.models import Tag -DEFAULT_TAGS: list[tuple[str, list[str]]] = [ - ("Borderless", ["Borderless Art"]), - ("Popout", []), - ("Extended", []), - ("Showcase", []), - ("Retro", []), - ("Classic", []), - ("Anime", []), - ("Full Art", ["Fullart"]), - ("Custom", []), - ("Upscaled", ["Upscaled Scan"]), - ("Placeholder", []), - ("Token", []), - ("AI-Generated", ["Midjourney"]), +# (name, aliases, display_name). display_name is None for almost every entry here - these +# names are already nice human-readable Title Case, so they fall back to rendering as `name` +# with no further seeding needed (see frontend useTagDisplayName). The two entries that do +# specify one ("Full Art", "Borderless") aren't renamed or given a *different* display_name - +# they're just given an explicit row so the taxonomy has no silent gaps, matching every other +# actively-displayed tag having a real display_name value rather than relying on fallback. +DEFAULT_TAGS: list[tuple[str, list[str], Optional[str]]] = [ + ("Borderless", ["Borderless Art"], "Borderless"), + ("Popout", [], None), + ("Extended", [], None), + ("Showcase", [], None), + ("Retro", [], None), + ("Classic", [], None), + ("Anime", [], None), + ("Full Art", ["Fullart"], "Full Art"), + ("Custom", [], None), + ("Upscaled", ["Upscaled Scan"], None), + ("Placeholder", [], None), + ("Token", [], None), + ("AI-Generated", ["Midjourney"], None), ] def seed_default_tags() -> dict[str, int]: """ - Idempotent - safe to re-run. Creates any tag that doesn't exist yet, and adds any - alias from `DEFAULT_TAGS` that's missing from an already-existing tag (e.g. if it - was previously created by hand, or by an earlier version of this list). + Idempotent - safe to re-run. Creates any tag that doesn't exist yet, adds any alias from + `DEFAULT_TAGS` that's missing from an already-existing tag, and backfills display_name for + the (few) entries above that specify one - only when it's still null, never overwriting a + manually-edited display_name (see Tag.display_name's help_text: "freely editable"). """ created = 0 updated = 0 - for name, aliases in DEFAULT_TAGS: - tag, was_created = Tag.objects.get_or_create(name=name, defaults={"aliases": aliases}) + for name, aliases, display_name in DEFAULT_TAGS: + defaults: dict[str, object] = {"aliases": aliases} + if display_name is not None: + defaults["display_name"] = display_name + tag, was_created = Tag.objects.get_or_create(name=name, defaults=defaults) if was_created: created += 1 continue + changed_fields = [] missing_aliases = [alias for alias in aliases if alias not in tag.aliases] if missing_aliases: tag.aliases = [*tag.aliases, *missing_aliases] - tag.save(update_fields=["aliases"]) + changed_fields.append("aliases") + if display_name is not None and tag.display_name is None: + tag.display_name = display_name + changed_fields.append("display_name") + if changed_fields: + tag.save(update_fields=changed_fields) updated += 1 return {"created": created, "updated": updated} diff --git a/MPCAutofill/cardpicker/management/commands/seed_no_match_reason_tags.py b/MPCAutofill/cardpicker/management/commands/seed_no_match_reason_tags.py index 838a1056a..1d958c2bd 100644 --- a/MPCAutofill/cardpicker/management/commands/seed_no_match_reason_tags.py +++ b/MPCAutofill/cardpicker/management/commands/seed_no_match_reason_tags.py @@ -10,4 +10,4 @@ class Command(BaseCommand): def handle(self, *args: Any, **kwargs: Any) -> None: stats = seed_no_match_reason_tags() - print(f"No-match reason tags: {stats['created']} created.") + print(f"No-match reason tags: {stats['created']} created, {stats['updated']} display_name backfilled.") diff --git a/MPCAutofill/cardpicker/migrations/0056_tag_display_name.py b/MPCAutofill/cardpicker/migrations/0056_tag_display_name.py new file mode 100644 index 000000000..b853f7a5a --- /dev/null +++ b/MPCAutofill/cardpicker/migrations/0056_tag_display_name.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.30 on 2026-07-14 10:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("cardpicker", "0055_card_tag_vote_statuses_alter_card_artist_vote_status"), + ] + + operations = [ + migrations.AddField( + model_name="tag", + name="display_name", + field=models.CharField( + blank=True, + help_text="Presentation only — freely editable. `name` is the immutable machine key used by votes, tag_vote_statuses, Card.tags, and federation; NEVER rename a Tag after creation.", + max_length=200, + null=True, + ), + ), + ] diff --git a/MPCAutofill/cardpicker/models.py b/MPCAutofill/cardpicker/models.py index a37cd2dee..193c9cbe9 100755 --- a/MPCAutofill/cardpicker/models.py +++ b/MPCAutofill/cardpicker/models.py @@ -627,6 +627,14 @@ def __str__(self) -> str: class Tag(models.Model): name = models.CharField(unique=True) + display_name = models.CharField( + max_length=200, + null=True, + blank=True, + help_text="Presentation only — freely editable. `name` is the immutable machine key " + "used by votes, tag_vote_statuses, Card.tags, and federation; NEVER rename a Tag " + "after creation.", + ) # null=True is just for admin panel aliases = ArrayField(models.CharField(max_length=200), default=list, blank=True) is_enabled_by_default = models.BooleanField(default=True) @@ -638,6 +646,7 @@ def __str__(self) -> str: def serialise(self) -> SerialisedTag: return SerialisedTag( name=self.name, + displayName=self.display_name, aliases=self.aliases, isEnabledByDefault=self.is_enabled_by_default, parent=(self.parent.name if self.parent else None), diff --git a/MPCAutofill/cardpicker/reason_tags.py b/MPCAutofill/cardpicker/reason_tags.py index 303e2caed..0600b758e 100644 --- a/MPCAutofill/cardpicker/reason_tags.py +++ b/MPCAutofill/cardpicker/reason_tags.py @@ -25,29 +25,40 @@ from cardpicker.models import Tag -NO_MATCH_REASON_TAGS: list[tuple[str, str]] = [ - ("custom-art", "Original or alternate artwork - does not depict a real printing"), - ("altered-frame", "Real printing's art in a modified frame"), - ("upscaled", "AI-upscaled version of an official image"), - ("ai-art", "AI-generated artwork"), - ("no-collector-line", "No legible collector line on the card face"), - ("non-english", "Non-English printing"), +# (name, description, display_name). `description` is documentation only (no DB column for +# it - see Tag.display_name's help_text); `display_name` is real, seeded presentation text +# the frontend looks up dynamically (useTagDisplayName) rather than hardcoding, so this list +# is the single source of truth for both the machine key and its human label. +NO_MATCH_REASON_TAGS: list[tuple[str, str, str]] = [ + ("custom-art", "Original or alternate artwork - does not depict a real printing", "Custom art"), + ("altered-frame", "Real printing's art in a modified frame", "Altered frame"), + ("upscaled", "AI-upscaled version of an official image", "Upscaled"), + ("ai-art", "AI-generated artwork", "AI art"), + ("no-collector-line", "No legible collector line on the card face", "No collector line"), + ("non-english", "Non-English printing", "Non-English"), ] def seed_no_match_reason_tags() -> dict[str, int]: """ - Idempotent - safe to re-run. Creates any tag that doesn't exist yet. `Tag` has no - description field (see cardpicker.models.Tag) - the descriptions above are documentation - only, mirrored as display copy in the frontend's NoMatchReasonStrip.tsx. + Idempotent - safe to re-run. Creates any tag that doesn't exist yet (display_name set at + creation), and backfills display_name on an already-existing tag only if it's still null + - never overwrites a manually-edited display_name (see Tag.display_name's help_text: + "freely editable"). """ created = 0 - for name, _description in NO_MATCH_REASON_TAGS: - _tag, was_created = Tag.objects.get_or_create(name=name, defaults={"aliases": []}) + updated = 0 + for name, _description, display_name in NO_MATCH_REASON_TAGS: + tag, was_created = Tag.objects.get_or_create(name=name, defaults={"aliases": [], "display_name": display_name}) if was_created: created += 1 - return {"created": created} + continue + if tag.display_name is None: + tag.display_name = display_name + tag.save(update_fields=["display_name"]) + updated += 1 + return {"created": created, "updated": updated} __all__ = ["seed_no_match_reason_tags", "NO_MATCH_REASON_TAGS"] diff --git a/MPCAutofill/cardpicker/schema_types.py b/MPCAutofill/cardpicker/schema_types.py index 3574f9e85..363ec96a9 100644 --- a/MPCAutofill/cardpicker/schema_types.py +++ b/MPCAutofill/cardpicker/schema_types.py @@ -1551,6 +1551,7 @@ class ChildElement(BaseModel): children: List["ChildElement"] name: str aliases: Optional[List[str]] = None + displayName: Optional[str] = None isEnabledByDefault: Optional[bool] = None parent: Optional[str] = None @@ -1560,9 +1561,10 @@ def from_dict(obj: Any) -> "ChildElement": children = from_list(ChildElement.from_dict, obj.get("children")) name = from_str(obj.get("name")) aliases = from_union([lambda x: from_list(from_str, x), from_none], obj.get("aliases")) + displayName = from_union([from_none, from_str], obj.get("displayName")) isEnabledByDefault = from_union([from_bool, from_none], obj.get("isEnabledByDefault")) parent = from_union([from_none, from_str], obj.get("parent")) - return ChildElement(children, name, aliases, isEnabledByDefault, parent) + return ChildElement(children, name, aliases, displayName, isEnabledByDefault, parent) def to_dict(self) -> dict: result: dict = {} @@ -1570,6 +1572,8 @@ def to_dict(self) -> dict: result["name"] = from_str(self.name) if self.aliases is not None: result["aliases"] = from_union([lambda x: from_list(from_str, x), from_none], self.aliases) + if self.displayName is not None: + result["displayName"] = from_union([from_none, from_str], self.displayName) if self.isEnabledByDefault is not None: result["isEnabledByDefault"] = from_union([from_bool, from_none], self.isEnabledByDefault) result["parent"] = from_union([from_none, from_str], self.parent) @@ -1580,6 +1584,7 @@ class Tag(BaseModel): children: List[ChildElement] name: str aliases: Optional[List[str]] = None + displayName: Optional[str] = None isEnabledByDefault: Optional[bool] = None parent: Optional[str] = None @@ -1589,9 +1594,10 @@ def from_dict(obj: Any) -> "Tag": children = from_list(ChildElement.from_dict, obj.get("children")) name = from_str(obj.get("name")) aliases = from_union([lambda x: from_list(from_str, x), from_none], obj.get("aliases")) + displayName = from_union([from_none, from_str], obj.get("displayName")) isEnabledByDefault = from_union([from_bool, from_none], obj.get("isEnabledByDefault")) parent = from_union([from_none, from_str], obj.get("parent")) - return Tag(children, name, aliases, isEnabledByDefault, parent) + return Tag(children, name, aliases, displayName, isEnabledByDefault, parent) def to_dict(self) -> dict: result: dict = {} @@ -1599,6 +1605,8 @@ def to_dict(self) -> dict: result["name"] = from_str(self.name) if self.aliases is not None: result["aliases"] = from_union([lambda x: from_list(from_str, x), from_none], self.aliases) + if self.displayName is not None: + result["displayName"] = from_union([from_none, from_str], self.displayName) if self.isEnabledByDefault is not None: result["isEnabledByDefault"] = from_union([from_bool, from_none], self.isEnabledByDefault) result["parent"] = from_union([from_none, from_str], self.parent) diff --git a/MPCAutofill/cardpicker/tests/test_default_tags.py b/MPCAutofill/cardpicker/tests/test_default_tags.py new file mode 100644 index 000000000..31704831c --- /dev/null +++ b/MPCAutofill/cardpicker/tests/test_default_tags.py @@ -0,0 +1,38 @@ +from cardpicker.default_tags import DEFAULT_TAGS, seed_default_tags +from cardpicker.models import Tag + + +class TestSeedDefaultTags: + def test_creates_every_tag(self, db): + stats = seed_default_tags() + assert stats["created"] == len(DEFAULT_TAGS) + assert Tag.objects.count() == len(DEFAULT_TAGS) + + def test_display_name_only_set_for_full_art_and_borderless(self, db): + seed_default_tags() + assert Tag.objects.get(name="Full Art").display_name == "Full Art" + assert Tag.objects.get(name="Borderless").display_name == "Borderless" + # every other seeded tag has no display_name - it's already a nice Title Case + # `name`, so frontend fallback (displayName ?? name) covers it without a seeded row + others = Tag.objects.exclude(name__in=["Full Art", "Borderless"]) + assert all(tag.display_name is None for tag in others) + + def test_rerunning_does_not_duplicate_or_reclobber(self, db): + seed_default_tags() + stats = seed_default_tags() + assert stats["created"] == 0 + assert stats["updated"] == 0 + assert Tag.objects.count() == len(DEFAULT_TAGS) + + def test_backfills_display_name_only_when_null_never_overwrites_manual_edit(self, db): + Tag.objects.create(name="Full Art", aliases=[], display_name=None) + + stats = seed_default_tags() + + assert stats["created"] == len(DEFAULT_TAGS) - 1 + assert stats["updated"] == 1 + assert Tag.objects.get(name="Full Art").display_name == "Full Art" + + Tag.objects.filter(name="Full Art").update(display_name="Admin's custom label") + seed_default_tags() + assert Tag.objects.get(name="Full Art").display_name == "Admin's custom label" diff --git a/MPCAutofill/cardpicker/tests/test_reason_tags.py b/MPCAutofill/cardpicker/tests/test_reason_tags.py index c0e303a1c..2cae00ea5 100644 --- a/MPCAutofill/cardpicker/tests/test_reason_tags.py +++ b/MPCAutofill/cardpicker/tests/test_reason_tags.py @@ -9,31 +9,52 @@ def test_creates_all_six_reason_tags(self, db): assert stats["created"] == len(NO_MATCH_REASON_TAGS) names = set( - Tag.objects.filter(name__in=[name for name, _description in NO_MATCH_REASON_TAGS]).values_list( - "name", flat=True - ) + Tag.objects.filter( + name__in=[name for name, _description, _display_name in NO_MATCH_REASON_TAGS] + ).values_list("name", flat=True) ) - assert names == {name for name, _description in NO_MATCH_REASON_TAGS} + assert names == {name for name, _description, _display_name in NO_MATCH_REASON_TAGS} + + def test_display_name_set_at_creation(self, db): + seed_no_match_reason_tags() + for name, _description, display_name in NO_MATCH_REASON_TAGS: + assert Tag.objects.get(name=name).display_name == display_name def test_rerunning_does_not_duplicate(self, db): seed_no_match_reason_tags() count_after_first_run = Tag.objects.filter( - name__in=[name for name, _description in NO_MATCH_REASON_TAGS] + name__in=[name for name, _description, _display_name in NO_MATCH_REASON_TAGS] ).count() stats = seed_no_match_reason_tags() count_after_second_run = Tag.objects.filter( - name__in=[name for name, _description in NO_MATCH_REASON_TAGS] + name__in=[name for name, _description, _display_name in NO_MATCH_REASON_TAGS] ).count() assert stats["created"] == 0 + assert stats["updated"] == 0 # display_name already set on the first run, nothing left to backfill assert count_after_first_run == count_after_second_run == len(NO_MATCH_REASON_TAGS) + def test_backfills_display_name_only_when_null_never_overwrites_manual_edit(self, db): + name, _description, seeded_display_name = NO_MATCH_REASON_TAGS[0] + Tag.objects.create(name=name, aliases=[], display_name=None) + + stats = seed_no_match_reason_tags() + + assert stats["created"] == len(NO_MATCH_REASON_TAGS) - 1 + assert stats["updated"] == 1 + assert Tag.objects.get(name=name).display_name == seeded_display_name + + # a second run must never clobber a manual edit make after seeding + Tag.objects.filter(name=name).update(display_name="Admin's custom label") + seed_no_match_reason_tags() + assert Tag.objects.get(name=name).display_name == "Admin's custom label" + def test_reason_tags_are_case_distinct_from_default_tags(self, db): # "upscaled" (reason tag) and "Upscaled" (DEFAULT_TAGS) are deliberately two separate # rows covering related but distinct vote populations - see reason_tags.py's header # comment. Exact-string collision (not just case-insensitive overlap) would mean # seeding silently reused an existing row instead of creating a new one. - default_tag_names = {name for name, _aliases in DEFAULT_TAGS} - reason_tag_names = {name for name, _description in NO_MATCH_REASON_TAGS} + default_tag_names = {name for name, _aliases, _display_name in DEFAULT_TAGS} + reason_tag_names = {name for name, _description, _display_name in NO_MATCH_REASON_TAGS} assert default_tag_names.isdisjoint(reason_tag_names) diff --git a/MPCAutofill/cardpicker/tests/test_views.py b/MPCAutofill/cardpicker/tests/test_views.py index 4d9c41e0b..aca8cdfb0 100644 --- a/MPCAutofill/cardpicker/tests/test_views.py +++ b/MPCAutofill/cardpicker/tests/test_views.py @@ -11,7 +11,7 @@ from cardpicker import views from cardpicker.tests.constants import Cards, DummyImportSite, Sources -from cardpicker.tests.factories import SourceFactory +from cardpicker.tests.factories import SourceFactory, TagFactory def snapshot_response(response: Response, snapshot: SnapshotAssertion): @@ -875,15 +875,30 @@ class TestGetTags: def test_get_no_data_tags(self, client, django_settings): response = client.get(reverse(views.get_tags)) assert response.json()["tags"] == [ - {"name": "NSFW", "parent": None, "aliases": [], "children": [], "isEnabledByDefault": True}, + { + "name": "NSFW", + "displayName": None, + "parent": None, + "aliases": [], + "children": [], + "isEnabledByDefault": True, + }, ] def test_get_one_data_tag(self, client, django_settings, tag_in_data): response = client.get(reverse(views.get_tags)) assert response.json()["tags"] == [ - {"name": "NSFW", "parent": None, "aliases": [], "children": [], "isEnabledByDefault": True}, + { + "name": "NSFW", + "displayName": None, + "parent": None, + "aliases": [], + "children": [], + "isEnabledByDefault": True, + }, { "name": "Tag in Data", + "displayName": None, "parent": None, "aliases": ["TaginData"], "children": [], @@ -896,14 +911,23 @@ def test_get_two_data_tags(self, client, django_settings, tag_in_data, another_t assert response.json()["tags"] == [ { "name": "Another Tag in Data", + "displayName": None, "parent": None, "aliases": ["AnotherTaginData"], "children": [], "isEnabledByDefault": True, }, - {"name": "NSFW", "parent": None, "aliases": [], "children": [], "isEnabledByDefault": True}, + { + "name": "NSFW", + "displayName": None, + "parent": None, + "aliases": [], + "children": [], + "isEnabledByDefault": True, + }, { "name": "Tag in Data", + "displayName": None, "parent": None, "aliases": ["TaginData"], "children": [], @@ -914,21 +938,31 @@ def test_get_two_data_tags(self, client, django_settings, tag_in_data, another_t def test_get_hierarchical_tags(self, client, django_settings, grandchild_tag): response = client.get(reverse(views.get_tags)) assert response.json()["tags"] == [ - {"name": "NSFW", "parent": None, "aliases": [], "isEnabledByDefault": True, "children": []}, + { + "name": "NSFW", + "displayName": None, + "parent": None, + "aliases": [], + "isEnabledByDefault": True, + "children": [], + }, { "name": "Tag in Data", + "displayName": None, "parent": None, "aliases": ["TaginData"], "isEnabledByDefault": True, "children": [ { "name": "Child Tag", + "displayName": None, "parent": "Tag in Data", "aliases": ["ChildTag"], "isEnabledByDefault": True, "children": [ { "name": "Grandchild Tag", + "displayName": None, "parent": "Child Tag", "aliases": ["GrandchildTag"], "isEnabledByDefault": True, @@ -940,6 +974,12 @@ def test_get_hierarchical_tags(self, client, django_settings, grandchild_tag): }, ] + def test_get_tag_with_display_name(self, client, django_settings, db): + TagFactory(name="custom-art", display_name="Custom art") + response = client.get(reverse(views.get_tags)) + [custom_art_tag] = [tag for tag in response.json()["tags"] if tag["name"] == "custom-art"] + assert custom_art_tag["displayName"] == "Custom art" + def test_post_request(self, client, django_settings, snapshot): response = client.post(reverse(views.get_tags)) snapshot_response(response, snapshot) diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index 5cd26049c..103115986 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -360,23 +360,93 @@ rows. [[../infrastructure.md]]'s Docker/backend deploy section for the mechanism and the now-standard extra restart step. +## Stage 5: decouple tag identity (`name`) from presentation (`display_name`) + +`Tag.name` is both the machine key (votes, `Card.tags`, filename-bracket +matching, federation — see `docs/federation-v1.md`) and, until now, the +only text ever shown to a human. That coupling meant a purely cosmetic +relabel (fixing a typo, adding nicer casing) was indistinguishable from +a breaking rename — both touched the same field. `Tag.display_name` +(nullable `CharField`, additive migration) splits them: `name` stays +forever immutable post-creation, `display_name` is freely editable +presentation text, admin-editable via the already-registered `Tag` +admin (now also in `list_display`/`search_fields`). + +**Serialization**: `Tag.serialise()` includes `displayName`; +`schemas/schemas/Tag.json` gained the field (quicktype-regenerated, not +hand-edited — see the note above). Nullable/optional, so it doesn't +disturb any response that doesn't set it. + +**Frontend**: one shared lookup, `frontend/src/common/tagDisplayNames.ts`'s +`useTagDisplayName()` — built off the same already-cached +`useGetTagsQuery()` other consumers (e.g. `TagFilter`) already use, so +adding a lookup call site never triggers a new fetch. Flattens the tag +tree (children included) into a `name -> displayName` map and returns a +`(name) => displayName ?? name` function. Wired into every render site +that showed a raw tag name: `TagFilter.tsx` (filter dropdown labels), +`CardDetailedViewModal.tsx` (a card's resolved tag badges), +`TagVotePicker.tsx`, `QueueTagQuestion.tsx`, `NoMatchReasonStrip.tsx`, +`PrintingConfirmStrip.tsx`. API submissions/filters (`includesTags`, +`excludesTags`, `APISubmitTagVote`'s `tagName`, ...) are untouched — +they always send `name`. + +`NoMatchReasonStrip`/`PrintingConfirmStrip` previously hardcoded their +own chip label strings, duplicating what `display_name` now owns - +refactored both to look the label up dynamically instead, so editing a +`display_name` in admin changes what's shown without a frontend deploy. +One visible, intentional side effect: `PrintingConfirmStrip`'s "Full +art" chip (a hand-picked lowercase label) now reads "Full Art" (the +seeded `display_name`, matching `Tag.name`'s own casing exactly). + +**Seeding**: `seed_no_match_reason_tags` sets `display_name` for its six +tags at creation, and backfills it on an already-existing tag only when +still null (never clobbers a manual admin edit). `seed_default_tags` +gained the identical idempotent pattern, but only for `Full Art`/ +`Borderless` — `display_name = name` verbatim for those two (not +renamed, just given an explicit row so no actively-displayed tag +silently relies on fallback); the other eleven `DEFAULT_TAGS` entries +are left with no `display_name` (already nice Title Case `name`s, the +`displayName ?? name` fallback covers them for free). + +**Filename tag-extraction pipeline is unaffected, and here's why that +matters**: `cardpicker/tags.py`'s `Tags.get_tags()` builds its raw-token +lookup as `{tag.name.lower(): tag for tag in [...]}`, matched against +`Tag.name`/`aliases` only (`match_tag_fuzzy`, `extract()`) — never reads +`display_name`. `Card.tags` (the persisted, denormalised ArrayField) +stores `tag_object.name`, again never `display_name`. So adding +`display_name` changes nothing about indexing today. The reverse case - +what a future _rename_ of `name` (not what this stage does) would break + +- is exactly why `name` needed protecting in the first place: an exact + `.lower()` match against old filenames would silently stop firing + unless the old name were preserved as an alias, and every already- + persisted `Card.tags` array containing the old string would go stale + relative to the renamed row, with no migration path to reconcile them + (it's a snapshot array, not a live FK). `display_name` exists precisely + so that presentation changes never need to risk this at all. + ## Key files - Backend: `cardpicker/printing_consensus.py`, `cardpicker/printing_metadata_import.py`, `cardpicker/integrations/game/mtg.py`, `cardpicker/models.py` (migration - `0050_canonicalprintingmetadata_cardprintingtag_and_more.py`), + `0050_canonicalprintingmetadata_cardprintingtag_and_more.py`; + `display_name` — migration `0056_tag_display_name.py`, Stage 5), `cardpicker/search/search_functions.py` (Stage 3 re-rank/filter), `cardpicker/documents.py` (Stage 3 widened indexing; Stage 3.5 `reindex_card_safely`), `cardpicker/tag_consensus.py` (Stage 3.5), - `cardpicker/reason_tags.py`, `cardpicker/management/commands/ seed_no_match_reason_tags.py` (Stage 4) + `cardpicker/reason_tags.py`, `cardpicker/default_tags.py`, + `cardpicker/management/commands/seed_no_match_reason_tags.py` (Stage 4, + display_name seeding Stage 5) - Frontend: `frontend/src/features/printingTags/` (`PrintingTagQueue.tsx`, `PrintingTagPicker.tsx`, `starburstShape.ts`, `useStickyTop`), `frontend/src/features/filters/ResolvedAttributeFilter.tsx` (Stage 3), `frontend/src/common/processing.ts::getPrintingMatchLabel` (Stage 3), `frontend/src/features/attributeVoting/` (`ChipCard.tsx`, - `NoMatchReasonStrip.tsx`, `PrintingConfirmStrip.tsx` — Stage 4) -- `docs/upstreaming/vote-system.md` + `NoMatchReasonStrip.tsx`, `PrintingConfirmStrip.tsx` — Stage 4), + `frontend/src/common/tagDisplayNames.ts` (Stage 5) +- `docs/upstreaming/vote-system.md`, `docs/federation-v1.md` (`name` vs. + `display_name` interchange-key note, Stage 5) ## Known gaps @@ -389,4 +459,6 @@ mechanism and the now-standard extra restart step. - Stage numbering here may need reconciling if PR #11 (deductive printing-tag backfill, also labelled "Stage 4" on its own branch) lands separately from this one — whichever merges second should - renumber to avoid two unrelated "Stage 4"s. + renumber to avoid two unrelated "Stage 4"s (this document currently + reserves "Stage 5" for tag display_name decoupling, landed after + Stage 4's no-match reason tags on this same document's timeline). diff --git a/docs/federation-v1.md b/docs/federation-v1.md index f45745c90..6c910746b 100644 --- a/docs/federation-v1.md +++ b/docs/federation-v1.md @@ -42,7 +42,8 @@ export only locally-resolved consensus — never re-broadcast federated votes `content_hash` is the planned upgrade path for surviving re-uploads. - Artists travel by canonical name; tags by `Tag.name`. **Tag names are therefore a cross-instance contract: renaming a Tag is a breaking data - migration, not an edit.** + migration, not an edit.** `Tag.name` is the immutable interchange key; + `Tag.display_name` is local presentation only and never federates. ## Import rules (normative for future implementation) diff --git a/frontend/src/common/schema_types.ts b/frontend/src/common/schema_types.ts index 7f001d159..acfa2119f 100644 --- a/frontend/src/common/schema_types.ts +++ b/frontend/src/common/schema_types.ts @@ -558,6 +558,7 @@ export interface TagsResponse { export interface Tag { aliases?: string[]; children: ChildElement[]; + displayName?: null | string; isEnabledByDefault?: boolean; name: string; parent: null | string; @@ -566,6 +567,7 @@ export interface Tag { export interface ChildElement { aliases?: string[]; children: ChildElement[]; + displayName?: null | string; isEnabledByDefault?: boolean; name: string; parent: null | string; @@ -1990,6 +1992,11 @@ const typeMap: any = { [ { json: "aliases", js: "aliases", typ: u(undefined, a("")) }, { json: "children", js: "children", typ: a(r("ChildElement")) }, + { + json: "displayName", + js: "displayName", + typ: u(undefined, u(null, "")), + }, { json: "isEnabledByDefault", js: "isEnabledByDefault", @@ -2004,6 +2011,11 @@ const typeMap: any = { [ { json: "aliases", js: "aliases", typ: u(undefined, a("")) }, { json: "children", js: "children", typ: a(r("ChildElement")) }, + { + json: "displayName", + js: "displayName", + typ: u(undefined, u(null, "")), + }, { json: "isEnabledByDefault", js: "isEnabledByDefault", diff --git a/frontend/src/common/tagDisplayNames.ts b/frontend/src/common/tagDisplayNames.ts new file mode 100644 index 000000000..41ca005cc --- /dev/null +++ b/frontend/src/common/tagDisplayNames.ts @@ -0,0 +1,43 @@ +/** + * Every tag-rendering site shows `displayName ?? name` - the human-editable presentation + * text if one's been set on the `Tag`, falling back to the raw machine key (`Tag.name`, + * which is what every site rendered before `displayName` existed) otherwise. API + * submissions/filters never go through this - they always send `name`, the immutable + * interchange key (see cardpicker/models.py's Tag.display_name help_text and + * docs/federation-v1.md). + * + * Built from the same, already-cached `useGetTagsQuery()` other consumers (e.g. TagFilter) + * already use - calling this hook doesn't trigger a new fetch. Flattens the tag tree + * (`children`, recursively) so a lookup works for a child tag too, not just top-level ones. + */ + +import { useMemo } from "react"; + +import { useGetTagsQuery } from "@/store/api"; + +interface TagLike { + name: string; + displayName?: string | null; + children: Array; +} + +function flattenDisplayNames(tags: Array): Map { + const displayNameByName = new Map(); + const visit = (tag: TagLike) => { + if (tag.displayName != null) { + displayNameByName.set(tag.name, tag.displayName); + } + tag.children.forEach(visit); + }; + tags.forEach(visit); + return displayNameByName; +} + +export function useTagDisplayName(): (tagName: string) => string { + const { data } = useGetTagsQuery(); + const displayNameByName = useMemo( + () => flattenDisplayNames(data ?? []), + [data] + ); + return (tagName: string) => displayNameByName.get(tagName) ?? tagName; +} diff --git a/frontend/src/features/attributeVoting/NoMatchReasonStrip.tsx b/frontend/src/features/attributeVoting/NoMatchReasonStrip.tsx index c547332e9..4edebc560 100644 --- a/frontend/src/features/attributeVoting/NoMatchReasonStrip.tsx +++ b/frontend/src/features/attributeVoting/NoMatchReasonStrip.tsx @@ -11,7 +11,9 @@ * `seed_no_match_reason_tags` management command, not a migration - see that module's * header comment for why) - and see the same file for why these are a separate taxonomy * from cardpicker.default_tags.DEFAULT_TAGS and why renaming any of them is a breaking - * change. + * change. Chip labels are NOT hardcoded here - they're the seeded `display_name` for each + * tag, looked up dynamically (useTagDisplayName), so editing a display_name in admin changes + * what's shown here without a frontend deploy. * * Graceful degradation for an instance where that command hasn't been run yet: filters the * six chips down to whichever tags `useGetTagsQuery` (the existing, already-cached `2/tags/` @@ -29,6 +31,7 @@ import Col from "react-bootstrap/Col"; import Row from "react-bootstrap/Row"; import { getOrCreateAnonymousId } from "@/common/cookies"; +import { useTagDisplayName } from "@/common/tagDisplayNames"; import { useAppDispatch } from "@/common/types"; import { ChipCard } from "@/features/attributeVoting/ChipCard"; import { APISubmitTagVote, useGetTagsQuery } from "@/store/api"; @@ -36,13 +39,13 @@ import { setNotification } from "@/store/slices/toastsSlice"; const APPLY = 1; -const NO_MATCH_REASONS: Array<{ tagName: string; label: string }> = [ - { tagName: "custom-art", label: "Custom art" }, - { tagName: "altered-frame", label: "Altered frame" }, - { tagName: "upscaled", label: "Upscaled" }, - { tagName: "ai-art", label: "AI art" }, - { tagName: "no-collector-line", label: "No collector line" }, - { tagName: "non-english", label: "Non-English" }, +const NO_MATCH_REASON_TAG_NAMES: Array = [ + "custom-art", + "altered-frame", + "upscaled", + "ai-art", + "no-collector-line", + "non-english", ]; interface NoMatchReasonStripProps { @@ -58,14 +61,15 @@ export function NoMatchReasonStrip({ onDone, }: NoMatchReasonStripProps) { const dispatch = useAppDispatch(); + const getTagDisplayName = useTagDisplayName(); const [submittingTagName, setSubmittingTagName] = useState( null ); const { data: existingTags } = useGetTagsQuery(); const existingTagNames = existingTags != null ? new Set(existingTags.map((tag) => tag.name)) : null; - const visibleReasons = NO_MATCH_REASONS.filter( - (reason) => existingTagNames == null || existingTagNames.has(reason.tagName) + const visibleReasonTagNames = NO_MATCH_REASON_TAG_NAMES.filter( + (tagName) => existingTagNames == null || existingTagNames.has(tagName) ); const choose = (tagName: string) => { @@ -98,13 +102,13 @@ export function NoMatchReasonStrip({
Why no match?
- {visibleReasons.map((reason) => ( - + {visibleReasonTagNames.map((tagName) => ( + choose(reason.tagName)} - data-testid={`no-match-reason-${reason.tagName}`} + onClick={() => choose(tagName)} + data-testid={`no-match-reason-${tagName}`} /> ))} diff --git a/frontend/src/features/attributeVoting/PrintingConfirmStrip.tsx b/frontend/src/features/attributeVoting/PrintingConfirmStrip.tsx index b0035b9de..9a52bf8f8 100644 --- a/frontend/src/features/attributeVoting/PrintingConfirmStrip.tsx +++ b/frontend/src/features/attributeVoting/PrintingConfirmStrip.tsx @@ -8,7 +8,8 @@ * with polarity matching the previewed state; Skip/Continue moves on without voting. * Deliberately reuses the existing Full Art/Borderless taxonomy rather than minting new * tags - this strip is just a fast, pre-filled way to cast the same votes TagVotePicker - * already supports. + * already supports. Chip labels are the seeded `display_name` for each tag (useTagDisplayName), + * not hardcoded text. */ import React, { useState } from "react"; @@ -18,6 +19,7 @@ import Row from "react-bootstrap/Row"; import { getOrCreateAnonymousId } from "@/common/cookies"; import { PrintingCandidate } from "@/common/schema_types"; +import { useTagDisplayName } from "@/common/tagDisplayNames"; import { useAppDispatch } from "@/common/types"; import { ChipCard } from "@/features/attributeVoting/ChipCard"; import { APISubmitTagVote } from "@/store/api"; @@ -28,7 +30,6 @@ const NOT_APPLICABLE = -1; interface ConfirmToggle { tagName: string; - label: string; previewValue: boolean; } @@ -47,6 +48,7 @@ export function PrintingConfirmStrip({ onDone, }: PrintingConfirmStripProps) { const dispatch = useAppDispatch(); + const getTagDisplayName = useTagDisplayName(); const [confirmedTagNames, setConfirmedTagNames] = useState>( new Set() ); @@ -55,12 +57,8 @@ export function PrintingConfirmStrip({ ); const toggles: ConfirmToggle[] = [ - { tagName: "Full Art", label: "Full art", previewValue: candidate.fullArt }, - { - tagName: "Borderless", - label: "Borderless", - previewValue: candidate.isBorderless, - }, + { tagName: "Full Art", previewValue: candidate.fullArt }, + { tagName: "Borderless", previewValue: candidate.isBorderless }, ]; const confirm = (toggle: ConfirmToggle) => { @@ -100,7 +98,7 @@ export function PrintingConfirmStrip({ {toggles.map((toggle) => ( (false); const submit = (polarity: number) => { @@ -63,7 +65,7 @@ export function QueueTagQuestion({ return (
- Does {tagName} apply? + Does {getTagDisplayName(tagName)} apply?
diff --git a/frontend/src/features/cardDetailedView/CardDetailedViewModal.tsx b/frontend/src/features/cardDetailedView/CardDetailedViewModal.tsx index bda23be62..ae43c1adb 100644 --- a/frontend/src/features/cardDetailedView/CardDetailedViewModal.tsx +++ b/frontend/src/features/cardDetailedView/CardDetailedViewModal.tsx @@ -12,6 +12,7 @@ import Row from "react-bootstrap/Row"; import { getCardDataAttributes } from "@/common/cardDom"; import { PrintingConsensusResponse } from "@/common/schema_types"; +import { useTagDisplayName } from "@/common/tagDisplayNames"; import { CardDocument, useAppDispatch, useAppSelector } from "@/common/types"; import { imageSizeToMBString, toTitleCase } from "@/common/utils"; import { AutofillTable } from "@/components/AutofillTable"; @@ -51,6 +52,7 @@ export function CardDetailedViewModal({ const queueImageDownload = useDoImageDownload(); const getLanguagesQuery = useGetLanguagesQuery(); const backendURL = useAppSelector(selectRemoteBackendURL); + const getTagDisplayName = useTagDisplayName(); const [printingConsensus, setPrintingConsensus] = useState(null); @@ -124,7 +126,7 @@ export function CardDetailedViewModal({ <> {cardDocument.tags.map((tag) => ( - {tag} + {getTagDisplayName(tag)} ))} diff --git a/frontend/src/features/filters/TagFilter.tsx b/frontend/src/features/filters/TagFilter.tsx index 0708c340f..6aea8c61e 100644 --- a/frontend/src/features/filters/TagFilter.tsx +++ b/frontend/src/features/filters/TagFilter.tsx @@ -52,7 +52,7 @@ export const TagFilter = ({ (checkedTags: Array): Array => { const processTag = (tag: Tag): TreeNode => { return { - label: tag.name, + label: tag.displayName ?? tag.name, value: tag.name, checked: checkedTags.includes(tag.name), expanded: expandedNodes.includes(tag.name), diff --git a/frontend/src/mocks/handlers.ts b/frontend/src/mocks/handlers.ts index 21e2643d2..193d17951 100644 --- a/frontend/src/mocks/handlers.ts +++ b/frontend/src/mocks/handlers.ts @@ -545,26 +545,34 @@ export const tagsTwoResults = http.get(buildRoute("2/tags/"), () => HttpResponse.json({ tags: ["Tag 1", "Tag 2"] }, { status: 200 }) ); -const serialisedTag = (name: string) => ({ +const serialisedTag = (name: string, displayName: string | null = null) => ({ name, + displayName, aliases: [], isEnabledByDefault: true, parent: null, children: [], }); +// keep in sync with cardpicker/reason_tags.py's NO_MATCH_REASON_TAGS - real seeded +// (name, displayName) pairs, mirrored here so mocked tests exercise the same +// displayName-lookup path a real seeded backend would. +const NO_MATCH_REASON_TAG_DISPLAY_NAMES: Array<[string, string]> = [ + ["custom-art", "Custom art"], + ["altered-frame", "Altered frame"], + ["upscaled", "Upscaled"], + ["ai-art", "AI art"], + ["no-collector-line", "No collector line"], + ["non-english", "Non-English"], +]; + // all six no-match reason tags exist server-side - NoMatchReasonStrip shows every chip export const tagsAllNoMatchReasonTags = http.get(buildRoute("2/tags/"), () => HttpResponse.json( { - tags: [ - "custom-art", - "altered-frame", - "upscaled", - "ai-art", - "no-collector-line", - "non-english", - ].map(serialisedTag), + tags: NO_MATCH_REASON_TAG_DISPLAY_NAMES.map(([name, displayName]) => + serialisedTag(name, displayName) + ), }, { status: 200 } ) @@ -574,11 +582,42 @@ export const tagsAllNoMatchReasonTags = http.get(buildRoute("2/tags/"), () => // run, or ran on an older version of the taxonomy) - NoMatchReasonStrip should hide the rest export const tagsSomeNoMatchReasonTags = http.get(buildRoute("2/tags/"), () => HttpResponse.json( - { tags: ["custom-art", "ai-art"].map(serialisedTag) }, + { + tags: NO_MATCH_REASON_TAG_DISPLAY_NAMES.filter(([name]) => + ["custom-art", "ai-art"].includes(name) + ).map(([name, displayName]) => serialisedTag(name, displayName)), + }, { status: 200 } ) ); +// one tag with no displayName set (falls back to raw name) alongside one with a real +// displayName - for asserting the fallback-vs-lookup behaviour directly. +export const tagsOneWithDisplayNameOneWithout = http.get( + buildRoute("2/tags/"), + () => + HttpResponse.json( + { + tags: [ + serialisedTag("custom-art", "Custom art"), + serialisedTag("altered-frame", null), + ], + }, + { status: 200 } + ) +); + +// "Borderless" has a displayName deliberately different from its name, so a test asserting +// the mapped label is visible (and the raw name isn't) can't pass by coincidence. +export const tagsBorderlessWithDisplayName = http.get( + buildRoute("2/tags/"), + () => + HttpResponse.json( + { tags: [serialisedTag("Borderless", "Frameless Border")] }, + { status: 200 } + ) +); + //# endregion //# region sample cards diff --git a/frontend/tests/TagVotePicker.spec.ts b/frontend/tests/TagVotePicker.spec.ts index 098c7e119..0aa508492 100644 --- a/frontend/tests/TagVotePicker.spec.ts +++ b/frontend/tests/TagVotePicker.spec.ts @@ -14,6 +14,7 @@ import { sourceDocumentsOneResult, submitTagVoteResolvesToApply, tagConsensusTwoUnresolvedTags, + tagsBorderlessWithDisplayName, } from "@/mocks/handlers"; import { test } from "../playwright.setup"; @@ -54,6 +55,41 @@ test.describe("TagVotePicker tests", () => { await expect(tagPicker.getByText("Extended")).toBeVisible(); }); + test("shows a tag's displayName when set, falling back to its name when not", async ({ + page, + network, + }) => { + network.use( + cardDocumentsOneResult, + cardbacksTwoOtherResults, + sourceDocumentsOneResult, + searchResultsOneResult, + printingCandidatesTwoResults, + printingConsensusUnresolved, + artistCandidatesTwoResults, + artistConsensusUnresolved, + tagConsensusTwoUnresolvedTags, + tagsBorderlessWithDisplayName, // only "Borderless" has a displayName; "Extended" doesn't exist in this list at all + ...defaultHandlers + ); + await loadPageWithDefaultBackend(page); + + await importText( + page, + `my search query${SelectedImageSeparator}${cardDocument1.identifier}` + ); + await openDetailedView(page, cardDocument1.name); + + const tagPicker = page.getByTestId("tag-vote-picker"); + // "Borderless" has displayName "Frameless Border" set - shown instead of the raw name + await expect(tagPicker.getByText("Frameless Border")).toBeVisible(); + await expect( + tagPicker.getByText("Borderless", { exact: true }) + ).not.toBeVisible(); + // "Extended" has no displayName - falls back to its raw name + await expect(tagPicker.getByText("Extended")).toBeVisible(); + }); + test("clicking a tag chip submits a vote and updates that chip's state", async ({ page, network, diff --git a/schemas/schemas/Tag.json b/schemas/schemas/Tag.json index 84a6b2fe8..2ced8bdba 100644 --- a/schemas/schemas/Tag.json +++ b/schemas/schemas/Tag.json @@ -4,6 +4,7 @@ "type": "object", "properties": { "name": { "type": "string" }, + "displayName": { "type": ["string", "null"] }, "aliases": { "type": "array", "items": { "type": "string" } }, "isEnabledByDefault": { "type": "boolean" }, "parent": { "type": ["string", "null"] },