Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ only) for deployment, hosting, and domain specifics.
- [`docs/lessons.md`](docs/lessons.md) — terse, reusable cross-session
lessons (CI vs. local venv trust, worktree port collisions, debug-color
verification, cyclic-animation sampling, verifying cross-session claims,
ES mapping drift, test-factory isolation, sticky/overflow CSS gotchas).
ES mapping drift, test-factory isolation, sticky/overflow CSS gotchas,
migration-vs-command tag seeding).
- [`docs/features/image-cdn.md`](docs/features/image-cdn.md) — the Worker +
R2 bucket image CDN.
- [`docs/features/pdf-generator.md`](docs/features/pdf-generator.md) — PDF
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from typing import Any

from django.core.management.base import BaseCommand

from cardpicker.reason_tags import seed_no_match_reason_tags


class Command(BaseCommand):
help = "Seeds the 'why no match?' reason-code Tag taxonomy (custom-art, altered-frame, ...). Safe to re-run."

def handle(self, *args: Any, **kwargs: Any) -> None:
stats = seed_no_match_reason_tags()
print(f"No-match reason tags: {stats['created']} created.")
1 change: 1 addition & 0 deletions MPCAutofill/cardpicker/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ def serialise_as_printing_candidate(self) -> PrintingCandidate:
smallThumbnailUrl=self.small_thumbnail_url,
mediumThumbnailUrl=self.medium_thumbnail_url,
fullArt=metadata.full_art if metadata is not None else False,
isBorderless=metadata.border_color == "borderless" if metadata is not None else False,
frame=metadata.frame if metadata is not None else "",
releasedAt=metadata.released_at.isoformat() if metadata is not None and metadata.released_at else None,
)
Expand Down
53 changes: 53 additions & 0 deletions MPCAutofill/cardpicker/reason_tags.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
"""
The "why no match?" reason-code taxonomy - shown as a follow-up strip in the printing-tag
queue after a human casts an explicit "No match" printing vote (see
docs/features/printing-tags.md, "no-match reason tags"). Kept in its own module and its own
management command (mirroring cardpicker.default_tags/seed_default_tags exactly) rather than
a data migration: a data migration would run automatically at DB-setup time (including the
test database), unconditionally seeding these rows into every fresh DB - which breaks every
test that asserts on the *complete* set of `Tag` rows (e.g. test_views.py::TestGetTags, which
documents that a fresh DB has zero real Tag rows besides the synthetic never-persisted "NSFW"
pseudo-tag - see cardpicker.tags). A manual, idempotent command avoids that coupling, exactly
like the existing descriptor taxonomy already does.

These six tag names are a federation interchange contract (see docs/features/printing-tags.md)
- other instances that consume our vote export are expected to recognise these exact strings.
Renaming any of them is a breaking data migration, not a refactor.

Deliberately a separate, lowercase-kebab-case taxonomy from `cardpicker.default_tags`'s Title
Case DEFAULT_TAGS (which parses filename bracket content at upload time, e.g. "Upscaled",
"Custom", "AI-Generated"). `upscaled`/`custom-art`/`ai-art` below cover near-identical
concepts to those but are cast by a human as the *reason* they picked "no match" in the
printing-tag queue, not inferred from a filename - kept as distinct rows rather than reusing
the existing tags so the two vote populations (upload-time inference vs. human no-match
reasoning) don't get silently merged into one consensus.
"""

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"),
]


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.
"""

created = 0
for name, _description in NO_MATCH_REASON_TAGS:
_tag, was_created = Tag.objects.get_or_create(name=name, defaults={"aliases": []})
if was_created:
created += 1
return {"created": created}


__all__ = ["seed_no_match_reason_tags", "NO_MATCH_REASON_TAGS"]
4 changes: 4 additions & 0 deletions MPCAutofill/cardpicker/schema_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1187,6 +1187,7 @@ class PrintingCandidate(BaseModel):
frame: str
fullArt: bool
identifier: str
isBorderless: bool
mediumThumbnailUrl: str
smallThumbnailUrl: str
releasedAt: Optional[str] = None
Expand All @@ -1202,6 +1203,7 @@ def from_dict(obj: Any) -> "PrintingCandidate":
frame = from_str(obj.get("frame"))
fullArt = from_bool(obj.get("fullArt"))
identifier = from_str(obj.get("identifier"))
isBorderless = from_bool(obj.get("isBorderless"))
mediumThumbnailUrl = from_str(obj.get("mediumThumbnailUrl"))
smallThumbnailUrl = from_str(obj.get("smallThumbnailUrl"))
releasedAt = from_union([from_none, from_str], obj.get("releasedAt"))
Expand All @@ -1214,6 +1216,7 @@ def from_dict(obj: Any) -> "PrintingCandidate":
frame,
fullArt,
identifier,
isBorderless,
mediumThumbnailUrl,
smallThumbnailUrl,
releasedAt,
Expand All @@ -1229,6 +1232,7 @@ def to_dict(self) -> dict:
result["frame"] = from_str(self.frame)
result["fullArt"] = from_bool(self.fullArt)
result["identifier"] = from_str(self.identifier)
result["isBorderless"] = from_bool(self.isBorderless)
result["mediumThumbnailUrl"] = from_str(self.mediumThumbnailUrl)
result["smallThumbnailUrl"] = from_str(self.smallThumbnailUrl)
if self.releasedAt is not None:
Expand Down
20 changes: 19 additions & 1 deletion MPCAutofill/cardpicker/tests/test_printing_tags_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ def test_explicit_query_searches_by_name_even_when_a_link_exists(self, client, d
def test_candidate_shape_includes_printing_metadata_fields(self, client, django_settings):
card = CardFactory(name="Brainstorm")
printing = CanonicalCardFactory(name="Brainstorm")
CanonicalPrintingMetadataFactory(canonical_card=printing, full_art=True, frame="1997")
CanonicalPrintingMetadataFactory(
canonical_card=printing, full_art=True, frame="1997", border_color="borderless"
)

response = client.post(
reverse(views.post_printing_candidates),
Expand All @@ -128,9 +130,24 @@ def test_candidate_shape_includes_printing_metadata_fields(self, client, django_

[result] = response.json()["results"]
assert result["fullArt"] is True
assert result["isBorderless"] is True
assert result["frame"] == "1997"
assert result["artist"] == printing.artist.name

def test_candidate_with_non_borderless_border_color(self, client, django_settings):
card = CardFactory(name="Brainstorm")
printing = CanonicalCardFactory(name="Brainstorm")
CanonicalPrintingMetadataFactory(canonical_card=printing, border_color="black")

response = client.post(
reverse(views.post_printing_candidates),
{"identifier": card.identifier},
content_type="application/json",
)

[result] = response.json()["results"]
assert result["isBorderless"] is False

def test_candidate_without_printing_metadata_uses_defaults(self, client, django_settings):
card = CardFactory(name="Brainstorm")
CanonicalCardFactory(name="Brainstorm") # no CanonicalPrintingMetadataFactory for this one
Expand All @@ -143,6 +160,7 @@ def test_candidate_without_printing_metadata_uses_defaults(self, client, django_

[result] = response.json()["results"]
assert result["fullArt"] is False
assert result["isBorderless"] is False
assert result["frame"] == ""
assert result["releasedAt"] is None

Expand Down
39 changes: 39 additions & 0 deletions MPCAutofill/cardpicker/tests/test_reason_tags.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
from cardpicker.default_tags import DEFAULT_TAGS
from cardpicker.models import Tag
from cardpicker.reason_tags import NO_MATCH_REASON_TAGS, seed_no_match_reason_tags


class TestSeedNoMatchReasonTags:
def test_creates_all_six_reason_tags(self, db):
stats = seed_no_match_reason_tags()
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
)
)
assert names == {name for name, _description in NO_MATCH_REASON_TAGS}

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]
).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]
).count()
assert stats["created"] == 0
assert count_after_first_run == count_after_second_run == len(NO_MATCH_REASON_TAGS)

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}
assert default_tag_names.isdisjoint(reason_tag_names)
95 changes: 93 additions & 2 deletions docs/features/printing-tags.md
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,90 @@ field (`inferred_canonical_artist`/`artist_vote_status` only,
serialise-time-only) — confirmed against `documents.py`'s field list, no
hook needed.

## Stage 4: no-match reason tags + post-vote follow-up strips

A resolved printing vote and an explicit "No match" vote both used to
advance the queue with zero follow-up (or, for no-match, the general
`AttributeVotingPanel`) — no fast way to capture _why_ a card had no
match, and no prompt to confirm full-art/borderless while the card was
still on screen. Two new, narrowly-scoped strips render in
`PrintingTagQueue.tsx` between a vote submitting and the queue
auto-advancing, both a brief, skippable dwell that never blocks
advancing:

- `PrintingConfirmStrip` (after a vote resolves a printing) — two chips,
"Full art"/"Borderless", pre-filled (highlighted) from the resolved
candidate's own `fullArt`/`isBorderless` flags. A tap casts one
`CardTagVote` for the existing `Full Art`/`Borderless` tags (seeded by
`cardpicker.default_tags`, not new) with polarity matching the
preview. No new tags, no new endpoint.
- `NoMatchReasonStrip` (after an explicit "No match" vote) — six chips
for a new reason-code taxonomy (below). One tap casts one positive
`CardTagVote` and advances. Replaces `AttributeVotingPanel` only in
this specific branch — a card that's still contested from a candidate
pick (not an explicit no-match) keeps showing `AttributeVotingPanel`
unchanged.

Both strips reuse the existing `CardTagVote`/`submitTagVote` machinery
end to end — no new backend endpoints or vote types, just narrower UI
over what Stage "attribute voting" already shipped.

**`isBorderless` added to `PrintingCandidate`**: the schema had
`fullArt` but no border-color-derived field, so `PrintingConfirmStrip`
couldn't pre-fill a borderless preview from data "already in the
payload" as originally assumed — it wasn't. Added via the quicktype
regeneration step (`schemas/schemas/PrintingCandidate.json` →
`cd schemas && npm run build`), wired in
`CanonicalCard.serialise_as_printing_candidate()` as
`metadata.border_color == "borderless"`. Verified against live data
first (read-only `CanonicalPrintingMetadata.objects.values("border_color").annotate(...)`
query) rather than assumed from general Scryfall knowledge: the stored
values are exactly `black`/`borderless`/`white`/`gold`/`silver`/`yellow`.

**Reason-code taxonomy — six new `Tag` rows, seeded by a management
command, not a migration**: `custom-art`, `altered-frame`, `upscaled`,
`ai-art`, `no-collector-line`, `non-english`
(`cardpicker/reason_tags.py`, `manage.py seed_no_match_reason_tags`,
mirroring the existing `cardpicker/default_tags.py`/
`seed_default_tags` pattern exactly). **These names are a federation
interchange contract** — other instances consuming our vote export
expect these exact strings; renaming any of them is a breaking change,
not a refactor.

A first pass seeded these via a data migration instead, per the
original task spec. That broke 5 unrelated tests
(`test_views.py::TestGetTags::*`, `test_tag_votes.py:: TestPostTagConsensus::test_returns_an_entry_for_every_seeded_tag`) —
they assert the _complete_ set of `Tag` rows, and document that a fresh
DB has zero real `Tag` rows besides the synthetic, never-persisted
`"NSFW"` pseudo-tag (`cardpicker/tags.py`). `seed_default_tags` is
deliberately **not** wired into any migration for the same reason: a
migration runs unconditionally at DB-setup time (including the test
DB), permanently seeding rows nothing asked for. Switched to a command
to match that established convention; suite back to the known 4-failure
baseline (2 unrelated `moxfield` network tests, 2 unrelated
`test_sources.py` path issues) afterward.

**Deliberately a separate taxonomy from `DEFAULT_TAGS`**, not a reuse:
`upscaled`/`custom-art`/`ai-art` cover near-identical concepts to the
existing `Upscaled`/`Custom`/`AI-Generated` (which parse filename
bracket content at _upload_ time), but these are cast by a _human_ as
the reason they picked "no match" in the queue — kept as distinct rows
(exact-string-distinct, case included) so the two vote populations
don't silently merge into one consensus.

`Tag` has no `description` field — the descriptions given in the task
spec live as documentation only (`reason_tags.py`'s module comment,
mirrored as frontend display copy in `NoMatchReasonStrip.tsx`), not a
new DB column or serializer field.

**Activation note**: `manage.py seed_no_match_reason_tags` must be run
once after this deploys, or `NoMatchReasonStrip` votes 400
(`post_submit_tag_vote` does `Tag.objects.get(name=...)`, not
`get_or_create` — a miss raises `BadRequestException`, not a silent
no-op). Confirmed live: `Full Art`/`Borderless` (used by
`PrintingConfirmStrip`) already exist in production — `seed_default_tags`
has been run there before — so that strip needs no activation step.

## Key files

- Backend: `cardpicker/printing_consensus.py`,
Expand All @@ -256,11 +340,14 @@ hook needed.
`0050_canonicalprintingmetadata_cardprintingtag_and_more.py`),
`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)
`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)
- 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/common/processing.ts::getPrintingMatchLabel` (Stage 3),
`frontend/src/features/attributeVoting/` (`ChipCard.tsx`,
`NoMatchReasonStrip.tsx`, `PrintingConfirmStrip.tsx` — Stage 4)
- `docs/upstreaming/vote-system.md`

## Known gaps
Expand All @@ -271,3 +358,7 @@ hook needed.
- Client-side (Orama) search has no Stage 3 parity — see above.
- Upstreaming this feature is deprioritized — see
[[../infrastructure.md]]'s Upstreaming section.
- 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.
21 changes: 21 additions & 0 deletions docs/lessons.md
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,24 @@ artifact in a real browser (Chromium at
`executablePath: /opt/pw-browsers/chromium`; default Playwright download is
absent). State clearly in the report that live behavior itself was not
observed.

## Seeding `Tag` rows via a data migration breaks tests that assert on the whole table — use a management command instead

Tried seeding six new `Tag` rows via a Django data migration (RunPython).
Broke 5 unrelated tests (`test_views.py::TestGetTags::*`,
`test_tag_votes.py::TestPostTagConsensus::test_returns_an_entry_for_every_seeded_tag`)
because they assert the _complete_ `Tag` table is empty in a fresh DB
(besides the synthetic, never-persisted `"NSFW"` pseudo-tag from
`cardpicker/tags.py`) — a migration runs unconditionally at DB-setup time,
including the test database, so any migration-seeded row becomes
permanent baseline state for every test in the suite, not just the ones
that care about it. The repo's existing 13-tag `DEFAULT_TAGS` taxonomy
(`cardpicker/default_tags.py`) is deliberately _not_ wired into any
migration for exactly this reason — it's a manual, idempotent
`seed_default_tags` management command only. Any future tag/taxonomy
seeding should follow that same pattern (a `..._tags.py` module +
`get_or_create` + a thin management-command wrapper), never a migration,
regardless of how the request is phrased ("data migration" in a task spec
should be read as "a repeatable seeding step," not literally
`migrations.RunPython`, when the target table has DB-wide list-all
consumers).
2 changes: 2 additions & 0 deletions frontend/src/common/schema_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,7 @@ export interface PrintingCandidate {
frame: string;
fullArt: boolean;
identifier: string;
isBorderless: boolean;
mediumThumbnailUrl: string;
releasedAt?: null | string;
smallThumbnailUrl: string;
Expand Down Expand Up @@ -1863,6 +1864,7 @@ const typeMap: any = {
{ json: "frame", js: "frame", typ: "" },
{ json: "fullArt", js: "fullArt", typ: true },
{ json: "identifier", js: "identifier", typ: "" },
{ json: "isBorderless", js: "isBorderless", typ: true },
{ json: "mediumThumbnailUrl", js: "mediumThumbnailUrl", typ: "" },
{ json: "releasedAt", js: "releasedAt", typ: u(undefined, u(null, "")) },
{ json: "smallThumbnailUrl", js: "smallThumbnailUrl", typ: "" },
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/common/test-constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,7 @@ export const printingCandidate1: PrintingCandidate = {
smallThumbnailUrl: "https://example.com/small1.png",
mediumThumbnailUrl: "https://example.com/medium1.png",
fullArt: false,
isBorderless: false,
frame: "2015",
releasedAt: "2020-01-01",
};
Expand All @@ -534,6 +535,7 @@ export const printingCandidate2: PrintingCandidate = {
smallThumbnailUrl: "https://example.com/small2.png",
mediumThumbnailUrl: "https://example.com/medium2.png",
fullArt: true,
isBorderless: true,
frame: "2003",
releasedAt: "2010-06-15",
};
Expand Down
Loading
Loading