From cdbe3ffcc2a54eddd6eb0a0ab6d79545daeb9f7c Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 6 Aug 2026 04:08:18 +0000 Subject: [PATCH] questionFeed: exclude N>1 illustration-vote answers; dedupe contested_card_ids across the view - _voter_answered_printing_card_ids now also reads CardIllustrationVote, not just CardPrintingTag. The illustration-cluster answer path (cast_illustration_vote, N>1 candidates sharing an illustration) writes only CardIllustrationVote, which the printing-tier exclusion could not see - the card stayed eligible and was immediately re-served to the same voter. - get_next_question_feed_item and get_remaining_estimate both accept an optional contested_card_ids parameter (extending PR #729's convention across the view boundary); views.get_question_feed resolves get_contested_card_ids() once per request and threads it into both, instead of each independently paying the ~520-627ms query cost. --- MPCAutofill/cardpicker/question_feed.py | 60 ++++++++++++++++--- .../cardpicker/tests/test_question_feed.py | 60 +++++++++++++++++++ MPCAutofill/cardpicker/views.py | 9 ++- docs/features/printing-tags.md | 5 +- 4 files changed, 122 insertions(+), 12 deletions(-) diff --git a/MPCAutofill/cardpicker/question_feed.py b/MPCAutofill/cardpicker/question_feed.py index 3b4f9202d..2d3786cb0 100644 --- a/MPCAutofill/cardpicker/question_feed.py +++ b/MPCAutofill/cardpicker/question_feed.py @@ -84,6 +84,7 @@ ArtistVoteStatus, Card, CardArtistVote, + CardIllustrationVote, CardPrintingTag, CardScanLog, CardTagVote, @@ -258,14 +259,38 @@ def _voter_answered_printing_card_ids(anonymous_id: str) -> set[int]: One indexed query, plus the expansion's own (at most two, and zero before PR-1 adds the checksum column - see `printing_consensus._md5_checksums_for_card_ids`). + ALSO reads `CardIllustrationVote` (issue #713). `identify_printing` has two answer paths on + the frontend: a single/unclustered candidate posts straight to `CardPrintingTag`, but a + shared-illustration cluster of N>=2 candidates posts to `illustration_vote. + cast_illustration_vote` instead, which writes `CardPrintingTag` ONLY when the illustration + resolves to exactly one live printing (see that function's own docstring) - at N>1, the + premise of the cluster UI that fired, nothing lands on the printing channel at all. Without + this, a voter who answered via the cluster path cast a real, persisted vote + (`CardIllustrationVote` is always written) that this exclusion could not see, so the card + stayed eligible and was immediately re-served - production evidence: both of the only two + human illustration votes on record were each followed within seconds by an `is_no_match` + escape vote on the same card. Every `CardIllustrationVote` row - not just the N>1 case - is + included unconditionally: for the N=1 case the card is already covered by the + `CardPrintingTag` query above, so this is a no-op union there, not a special case to branch + on; keeping it unconditional means one query shape covers both outcomes of + `cast_illustration_vote` rather than two. + + NOT `CardQuestionAbstention` (issue #712/#731): that model records a human "Not sure" - a + real non-answer - and reusing it here for a real, weighted answer would conflate the two, + corrupting the exact distinction #712 was built to preserve. See this function's own tests + and the issue #713 PR description for the full reasoning. + COMPUTED ONCE PER FEED REQUEST, in `get_next_question_feed_item`, and passed down to every tier that needs it (2026-07-25 gate on PR #482, condition f1: each tier calling this for itself multiplied the cost by the number of tiers consulted, for an answer that cannot change within one request). The tiers keep an optional parameter rather than a required one so a direct caller - a test, a shell - can still ask for one tier by `anonymous_id` alone. """ - voted_card_ids = CardPrintingTag.objects.filter(anonymous_id=anonymous_id).values_list("card_id", flat=True) - return identity_group_expanded_card_ids(voted_card_ids) + voted_card_ids = set(CardPrintingTag.objects.filter(anonymous_id=anonymous_id).values_list("card_id", flat=True)) + illustration_voted_card_ids = set( + CardIllustrationVote.objects.filter(anonymous_id=anonymous_id).values_list("card_id", flat=True) + ) + return identity_group_expanded_card_ids(voted_card_ids | illustration_voted_card_ids) def _voter_answered_artist_card_ids(anonymous_id: str) -> set[int]: @@ -647,7 +672,9 @@ def _log_served(anonymous_id: str, item: QuestionFeedItem, pool: str, origin_rea return item -def get_next_question_feed_item(anonymous_id: str) -> Optional[QuestionFeedItem]: +def get_next_question_feed_item( + anonymous_id: str, contested_card_ids: Optional[list[int]] = None +) -> Optional[QuestionFeedItem]: """ The ranked union itself. When this session's served-mix ratio (`_served_mix_ratio`) is below `settings.QUESTION_FEED_LIKELY_RESOLVE_MIX_RATIO` AND the likely-resolve pool still @@ -670,6 +697,14 @@ def get_next_question_feed_item(anonymous_id: str) -> Optional[QuestionFeedItem] phase-C routing signal: a card a human has declared not-official-art via `reason_tags. NOT_OFFICIAL_ART_REASON_TAGS` stops being served as an artist-shaped question, in both `_tier_2_contested` and `_tier_4_fresh` - printing questions are unaffected). + + `contested_card_ids` is an optional pre-resolved value (issue #713 part 2, extending PR + #729's "compute once, thread as an optional parameter" convention across the view boundary + for the first time): `views.get_question_feed` calls `get_contested_card_ids()` once per + HTTP request and passes the result to both this function and `get_remaining_estimate`, since + both independently called it before (measured 520-562ms per call against live production + data) even though neither can see a vote the other cast mid-request. `None` (every direct + caller - tests, a shell) still resolves it here exactly as before. """ answered_card_ids = _voter_answered_printing_card_ids(anonymous_id) answered_artist_card_ids = _voter_answered_artist_card_ids(anonymous_id) @@ -688,12 +723,13 @@ def get_next_question_feed_item(anonymous_id: str) -> Optional[QuestionFeedItem] if tier_1_item is not None: return _log_served(anonymous_id, tier_1_item, QuestionFeedServedPool.REMAINDER, "tier_1_confirm_suggestion") - # `contested_card_ids`/`contested_artist_card_ids` are resolved ONCE here, only once we've - # actually fallen through to the tiers that consult them (tier 1 and the likely-resolve pool - # above never touch either), and reused by both tier 2 and tier 4 below - each is otherwise + # `contested_card_ids` may already have arrived from the caller (see docstring above); only + # resolve it here if not, and only once we've actually fallen through to the tiers that + # consult it (tier 1 and the likely-resolve pool above never touch it) - each is otherwise # identical on repeat calls within this same request (no vote can be cast mid-request), so # recomputing it once per tier just paid the same cost twice for one answer. - contested_card_ids = get_contested_card_ids() + if contested_card_ids is None: + contested_card_ids = get_contested_card_ids() contested_artist_card_ids = get_contested_artist_card_ids() tier_2_result = _tier_2_contested( @@ -742,7 +778,7 @@ def _tag_review_card_ids_by_status() -> tuple[set[int], set[int]]: return contested_ids, unresolved_ids -def get_remaining_estimate() -> QuestionFeedCounts: +def get_remaining_estimate(contested_card_ids: Optional[list[int]] = None) -> QuestionFeedCounts: """ "Still need help with" counts for the feed header - NOT per-voter (doesn't account for own-vote exclusion, which is comparatively cheap to skip here since this is advisory copy, @@ -772,8 +808,14 @@ def get_remaining_estimate() -> QuestionFeedCounts: per bucket (4 buckets), for 6 queries overall. No per-card sub-queries in a loop - the only Python-side materialization is the tag-status scan, which was already the established pattern for this JSONField (see `_tag_review_card_ids_by_status`'s docstring). + + `contested_card_ids` is an optional pre-resolved value - see `get_next_question_feed_item`'s + matching parameter docstring for why (issue #713 part 2): `views.get_question_feed` calls + `get_contested_card_ids()` once and passes it to both functions, since this one always needs + it and the other needed it as often. `None` (every other caller, e.g. `catalog_stats.py`) + resolves it here exactly as before. """ - contested_printing_ids = get_contested_card_ids() + contested_printing_ids = contested_card_ids if contested_card_ids is not None else get_contested_card_ids() tag_contested_ids, tag_unresolved_ids = _tag_review_card_ids_by_status() confirmable = ( diff --git a/MPCAutofill/cardpicker/tests/test_question_feed.py b/MPCAutofill/cardpicker/tests/test_question_feed.py index 7d5025564..a8ba1ed66 100644 --- a/MPCAutofill/cardpicker/tests/test_question_feed.py +++ b/MPCAutofill/cardpicker/tests/test_question_feed.py @@ -1,3 +1,4 @@ +import uuid from unittest.mock import patch from django.urls import reverse @@ -7,12 +8,14 @@ get_contested_artist_card_ids, resolve_and_persist_artist, ) +from cardpicker.illustration_vote import cast_illustration_vote from cardpicker.local_calculate_verdicts import ( JOIN_KEY_ANONYMOUS_ID, JOIN_KEY_UNKNOWN_SET_CODE_SKIP_REASON, ) from cardpicker.models import ( ArtistVoteStatus, + CardPrintingTag, CardScanLog, PrintingTagStatus, QuestionFeedServedLog, @@ -30,6 +33,7 @@ _artist_item, _scryfall_illustration_url, _tier_1_confirm_suggestion, + _voter_answered_printing_card_ids, get_next_question_feed_item, get_remaining_estimate, is_likely_resolve_printing, @@ -47,6 +51,20 @@ ) +def make_shared_illustration_group(name: str = "Brainstorm") -> tuple: + """Two live printing candidates for a fresh `card`, sharing one `illustration_id` - the N>1 + shared-illustration-group premise `cast_illustration_vote` (illustration_vote.py) requires to + take its no-CardPrintingTag-write branch. Mirrors test_illustration_vote.py's own + `_printing_with_illustration` helper.""" + card = CardFactory(name=name, printing_tag_status=PrintingTagStatus.UNRESOLVED) + illustration_id = uuid.uuid4() + artist = CanonicalArtistFactory(name="Shared Artist") + for _ in range(2): + printing = CanonicalCardFactory(name=name, artist=artist) + CanonicalPrintingMetadataFactory(canonical_card=printing, illustration_id=illustration_id) + return card, illustration_id + + def make_ai_suggested_card(anonymous_id: str = "ai-bot") -> tuple: card = CardFactory(printing_tag_status=PrintingTagStatus.UNRESOLVED) printing = CanonicalCardFactory() @@ -745,3 +763,45 @@ def test_tier_4_does_not_treat_ambiguous_origin_as_quick_negative(self, db): assert item is not None log = QuestionFeedServedLog.objects.get(anonymous_id="anon-1") assert log.origin_reason != "tier_4_quick_negative_to_review" + + +class TestIllustrationVoteAnsweredExclusion: + """Issue #713: `cast_illustration_vote` writes `CardPrintingTag` only when the shared- + illustration group resolves to exactly one live printing - at N>1 (the premise of the + cluster UI that triggers this path) it writes only `CardIllustrationVote`, which the + printing exclusion below must also read or the voter is re-served the very card they just + answered.""" + + def test_voter_answered_printing_card_ids_includes_n_gt_1_illustration_votes(self, db): + card, illustration_id = make_shared_illustration_group() + + outcome = cast_illustration_vote( + card=card, + anonymous_id="voter-1", + illustration_id=illustration_id, + is_unknown=False, + user=None, + vote_surface="question-feed", + ) + + assert outcome.printing_vote_cast is False + assert CardPrintingTag.objects.filter(card=card, anonymous_id="voter-1").count() == 0 + assert card.pk in _voter_answered_printing_card_ids("voter-1") + + def test_a_voter_who_answers_an_n_gt_1_illustration_group_is_not_re_served_that_card(self, db): + card, illustration_id = make_shared_illustration_group() + + first_item = get_next_question_feed_item("voter-1") + assert first_item is not None + assert first_item.card.identifier == card.identifier + + cast_illustration_vote( + card=card, + anonymous_id="voter-1", + illustration_id=illustration_id, + is_unknown=False, + user=None, + vote_surface="question-feed", + ) + + assert get_next_question_feed_item("voter-1") is None diff --git a/MPCAutofill/cardpicker/views.py b/MPCAutofill/cardpicker/views.py index 0e9abccaf..debc4902e 100644 --- a/MPCAutofill/cardpicker/views.py +++ b/MPCAutofill/cardpicker/views.py @@ -2010,8 +2010,13 @@ def get_question_feed(request: HttpRequest) -> HttpResponse: if not anonymous_id: raise BadRequestException("Missing required anonymousId query parameter.") - item = get_next_question_feed_item(anonymous_id) - remaining_estimate = get_remaining_estimate() + # Resolved once and threaded to both calls below: `get_next_question_feed_item` and + # `get_remaining_estimate` each independently called `get_contested_card_ids()` before, + # measured at 520-562ms per call against live production data - on a single-gunicorn-worker + # deployment that's site-wide latency on every request, not one user's. + contested_card_ids = get_contested_card_ids() + item = get_next_question_feed_item(anonymous_id, contested_card_ids=contested_card_ids) + remaining_estimate = get_remaining_estimate(contested_card_ids) return JsonResponse(QuestionFeedResponse(item=item, remainingEstimate=remaining_estimate).model_dump()) diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index 4b47857d7..2cb6c5f00 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -967,7 +967,10 @@ for history (this doc's own established convention — see the `cardPanel.tsx` b illustration resolves, against LIVE data at write time, to exactly one candidate printing for this card (`printings_for_card_and_illustration` — N>1 casts nothing on the printing channel, matching #526's machine-side - rule); and a `CardArtistVote` is derived (`source=USER`, + rule; `question_feed._voter_answered_printing_card_ids` reads + `CardIllustrationVote` as well as `CardPrintingTag` so an N>1 answer still + excludes the card from the printing tiers, issue #713); and a + `CardArtistVote` is derived (`source=USER`, `vote_surface="illustration_vote_derived_artist"`) whenever the resolved artist's name doesn't indicate a combined credit (tests for `' & '` only — see the module's own census comment) and no