diff --git a/.github/coverage-acks.txt b/.github/coverage-acks.txt index d2046fce1..83c7a4921 100644 --- a/.github/coverage-acks.txt +++ b/.github/coverage-acks.txt @@ -127,3 +127,13 @@ coverage-ack: frontend/tests/DisplayPage.spec.ts::DisplayPage (Proposal H, Step # Same surface, same assertions, only the URL (and the describe/test title naming it) changed. coverage-ack: frontend/tests/ContrastAudit.spec.ts::Contrast audit - /contributions (owner defect 1+2: accordion header + body) > Contribution Guidelines accordion - collapsed and expanded — route moved /contributions -> /stats in PR #558; same surface, same assertions coverage-ack: frontend/tests/ContrastAudit.spec.ts::Link colour audit (owner-approved open item 2 - $link-color -> $theme-info) > a real production link (contributions guidelines' ISO-639-1 reference, inside the accordion body's panel-bg) clears strict-AAA-normal — route moved /contributions -> /stats in PR #558; same surface, same assertions + +# PR #730 - QuestionFeed.tsx's illustration-group render now emits ONE representative tile per +# group (`group.find((c) => c.artCropUrl) ?? group[0]`) instead of one tile per member, so a +# non-representative member (candidateB in this fixture set) no longer gets its own DOM tile +# anywhere - it's represented by the group's shared vote, not silently dropped. The three old +# titles asserted the retired one-tile-per-member behavior; all three are renamed (not deleted) +# to describe the new representative-tile contract, same surface, same fixture set. +coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > every candidate in a mixed illustration set still renders in the grid - none silently dropped — PR #730: renamed to "every candidate in a mixed illustration set is accounted for - a group collapses to its one representative tile, ungrouped candidates still render individually" now that a group renders one representative tile, not one tile per member. +coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > candidates sharing an illustration render inside one illustration-group container; unique/null-illustration candidates don't — PR #730: renamed to "only the illustration group's representative candidate renders inside its one illustration-group container; unique/null-illustration candidates don't", same reason as the sibling ack above. +coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > clustered tiles render each candidate's art crop, falling back to the printing scan when absent; ungrouped tiles keep the printing scan regardless — PR #730: renamed to "the illustration group's one tile renders its representative candidate's art crop; ungrouped tiles keep the printing scan regardless", same reason as the sibling acks above. diff --git a/frontend/src/features/questionFeed/QuestionFeed.test.tsx b/frontend/src/features/questionFeed/QuestionFeed.test.tsx index 9cd937cc3..167b7cc47 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.test.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.test.tsx @@ -892,7 +892,7 @@ describe("QuestionFeed", () => { expect(illustrationVoteCalled).toBe(false); }); - it("clustered tiles render the candidate's art crop, falling back to the printing scan when absent; ungrouped tiles are unaffected", async () => { + it("renders exactly one representative tile per illustration group, preferring a member with an art crop; ungrouped tiles are unaffected", async () => { const withArtCrop = { ...groupedItem, candidates: [ @@ -930,10 +930,10 @@ describe("QuestionFeed", () => { "src", "https://example.com/art-crop-1.png" ); - expect(within(group).getByAltText("xyz 42")).toHaveAttribute( - "src", - groupedItem.candidates[1].mediumThumbnailUrl - ); + expect(within(group).queryByAltText("xyz 42")).not.toBeInTheDocument(); + expect( + within(group).getByText("Same illustration - 2 printings") + ).toBeInTheDocument(); const ungroupedGrid = await screen.findByTestId( "question-feed-candidate-grid-ungrouped" @@ -943,5 +943,88 @@ describe("QuestionFeed", () => { groupedItem.candidates[2].mediumThumbnailUrl ); }); + + it("falls back to the group's first member when no member has an art crop", async () => { + server.use(groupedQuestionFeedOnce()); + renderFeed(); + await revealCard(); + + const group = await screen.findByTestId( + "question-feed-illustration-group" + ); + expect(within(group).getByAltText("abc 1")).toHaveAttribute( + "src", + groupedItem.candidates[0].mediumThumbnailUrl + ); + expect(within(group).queryByAltText("xyz 42")).not.toBeInTheDocument(); + }); + + it("picks whichever group member has an art crop regardless of position, and still submits the group's shared illustrationId", async () => { + const artCropOnSecondMember = { + ...groupedItem, + candidates: [ + { ...groupedItem.candidates[0], artCropUrl: null }, + { + ...groupedItem.candidates[1], + artCropUrl: "https://example.com/art-crop-2.png", + }, + groupedItem.candidates[2], + ], + }; + server.use( + http.get(buildRoute("2/questionFeed/"), () => + HttpResponse.json( + { + item: artCropOnSecondMember, + remainingEstimate: { + total: 1, + confirmable: 0, + contested: 0, + fresh: 1, + }, + }, + { status: 200 } + ) + ) + ); + let illustrationVoteBody: Record | undefined; + server.use( + http.post( + buildRoute("2/submitIllustrationVote/"), + async ({ request }) => { + illustrationVoteBody = (await request.json()) as Record< + string, + unknown + >; + return HttpResponse.json( + { + illustrationId: sharedIllustrationId, + isUnknown: false, + printingVoteCast: false, + artistVoteCast: true, + }, + { status: 200 } + ); + } + ) + ); + renderFeed(); + await revealCard(); + + const group = await screen.findByTestId( + "question-feed-illustration-group" + ); + const tile = within(group).getByAltText("xyz 42"); + expect(tile).toHaveAttribute("src", "https://example.com/art-crop-2.png"); + expect(within(group).queryByAltText("abc 1")).not.toBeInTheDocument(); + + fireEvent.click(tile); + + await waitFor(() => expect(illustrationVoteBody).toBeDefined()); + expect(illustrationVoteBody).toMatchObject({ + identifier: groupedItem.card.identifier, + illustrationId: sharedIllustrationId, + }); + }); }); }); diff --git a/frontend/src/features/questionFeed/QuestionFeed.tsx b/frontend/src/features/questionFeed/QuestionFeed.tsx index 8e3a37340..7417dc40b 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.tsx @@ -1619,6 +1619,14 @@ export function QuestionFeed() { const illustrationArtist = group .map((candidate) => candidate.artist) .find((artist) => artist.trim() !== ""); + // selectIllustrationGroup already submits one illustrationId for the whole + // cluster (see that function's own comment), so one tile fully represents what + // is being voted on - showing every member here just repeats the same artwork + // up to N times. Prefer a member with an art crop, the same signal a tile's own + // image already prefers (renderCandidateTile's imageUrl default below), and fall + // back to the first member so the same data always picks the same tile. + const representative = + group.find((candidate) => candidate.artCropUrl) ?? group[0]; return ( )} - {group.map((candidate) => - renderCandidateTile( - candidate, - () => - // every member of `group` shares this non-null illustrationId - see the - // grouping logic above, which only clusters candidates that have one. - selectIllustrationGroup( - candidate.illustrationId as string, - candidate - ), - false, - candidate.artCropUrl || candidate.mediumThumbnailUrl - ) + {renderCandidateTile( + representative, + () => + // every member of `group` shares this non-null illustrationId - see + // the grouping logic above, which only clusters candidates that have + // one - so submitting the representative's illustrationId is + // identical to submitting any other member's. + selectIllustrationGroup( + representative.illustrationId as string, + representative + ), + false, + representative.artCropUrl || + representative.mediumThumbnailUrl )} diff --git a/frontend/tests/QuestionFeed.spec.ts b/frontend/tests/QuestionFeed.spec.ts index 709941dd5..2012da6cf 100644 --- a/frontend/tests/QuestionFeed.spec.ts +++ b/frontend/tests/QuestionFeed.spec.ts @@ -232,7 +232,7 @@ test.describe("question feed - Level 3 (conditional open-attribute confirm)", () // frequently-absent shape (CanonicalPrintingMetadata.illustration_id, see // local_illustration.py:137's isnull filter). test.describe("question feed - Level 2 illustration grouping", () => { - test("every candidate in a mixed illustration set still renders in the grid - none silently dropped", async ({ + test("every candidate in a mixed illustration set is accounted for - a group collapses to its one representative tile, ungrouped candidates still render individually", async ({ page, network, }) => { @@ -242,12 +242,26 @@ test.describe("question feed - Level 2 illustration grouping", () => { ); await loadPageWithDefaultBackend(page, "whatsthat"); + // candidateA carries the art crop, so it's the group's chosen representative + // (QuestionFeed.tsx's `group.find((c) => c.artCropUrl) ?? group[0]`) - the group renders + // ONE tile for the whole cluster, not one per member. + await expect( + page.locator( + `[data-card-identifier="${illustrationGroupCandidateA.identifier}"]` + ) + ).toHaveCount(1); + // candidateB shares the illustration but loses the representative pick to A - it renders + // no tile of its own anywhere; its vote is carried by A's tile, not silently dropped. + await expect( + page.locator( + `[data-card-identifier="${illustrationGroupCandidateB.identifier}"]` + ) + ).toHaveCount(0); + // The regression guard this task calls out explicitly: an exact count, not just "at least // one" - a candidate silently vanishing (e.g. because it has no illustrationId) is exactly // the correctness regression a weaker assertion would miss. for (const candidate of [ - illustrationGroupCandidateA, - illustrationGroupCandidateB, illustrationGroupCandidateC, illustrationGroupCandidateD, ]) { @@ -257,7 +271,7 @@ test.describe("question feed - Level 2 illustration grouping", () => { } }); - test("candidates sharing an illustration render inside one illustration-group container; unique/null-illustration candidates don't", async ({ + test("only the illustration group's representative candidate renders inside its one illustration-group container; unique/null-illustration candidates don't", async ({ page, network, }) => { @@ -278,11 +292,13 @@ test.describe("question feed - Level 2 illustration grouping", () => { `[data-card-identifier="${illustrationGroupCandidateA.identifier}"]` ) ).toHaveCount(1); + // candidateB shares the illustration but has no art crop, so A wins the representative + // pick and the group renders only A's tile - not B's. await expect( group.locator( `[data-card-identifier="${illustrationGroupCandidateB.identifier}"]` ) - ).toHaveCount(1); + ).toHaveCount(0); // candidateC (distinct illustrationId, no sibling) and candidateD (null illustrationId) // both render OUTSIDE the clustered group - neither forms (or joins) a cluster of one. @@ -298,7 +314,7 @@ test.describe("question feed - Level 2 illustration grouping", () => { ).toHaveCount(0); }); - test("clustered tiles render each candidate's art crop, falling back to the printing scan when absent; ungrouped tiles keep the printing scan regardless", async ({ + test("the illustration group's one tile renders its representative candidate's art crop; ungrouped tiles keep the printing scan regardless", async ({ page, network, }) => { @@ -314,11 +330,6 @@ test.describe("question feed - Level 2 illustration grouping", () => { `[data-card-identifier="${illustrationGroupCandidateA.identifier}"] img` ) ).toHaveAttribute("src", illustrationGroupCandidateA.artCropUrl as string); - await expect( - group.locator( - `[data-card-identifier="${illustrationGroupCandidateB.identifier}"] img` - ) - ).toHaveAttribute("src", illustrationGroupCandidateB.mediumThumbnailUrl); const ungroupedGrid = page.getByTestId( "question-feed-candidate-grid-ungrouped"