From 82c6a4b85093f0a0d0c6489cd51229b5de69b59e Mon Sep 17 00:00:00 2001 From: hfcRed <101019309+hfcRed@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:09:14 -0400 Subject: [PATCH] Trophy fixes (#3306) --- app/db/tables.ts | 1 + .../trophies/TrophyRepository.server.test.ts | 30 +++++ .../trophies/TrophyRepository.server.ts | 53 +++----- .../trophies/actions/trophies.new.server.ts | 12 +- .../trophies/loaders/trophies.new.server.ts | 3 +- app/features/trophies/routes/trophies.new.tsx | 2 +- ...260805155023-pending-trophy-accepted-at.ts | 114 ++++++++++++++++++ 7 files changed, 167 insertions(+), 48 deletions(-) create mode 100644 migrations/20260805155023-pending-trophy-accepted-at.ts diff --git a/app/db/tables.ts b/app/db/tables.ts index f14958144..e614ef2f4 100644 --- a/app/db/tables.ts +++ b/app/db/tables.ts @@ -200,6 +200,7 @@ export interface PendingTrophy { declineReason: string | null; declinedAt: number | null; declinedByUserId: number | null; + acceptedAt: number | null; targetTrophyId: number | null; managerId: number | null; } diff --git a/app/features/trophies/TrophyRepository.server.test.ts b/app/features/trophies/TrophyRepository.server.test.ts index 6f61402ab..dc5befca1 100644 --- a/app/features/trophies/TrophyRepository.server.test.ts +++ b/app/features/trophies/TrophyRepository.server.test.ts @@ -122,6 +122,36 @@ describe("trophy approvals", () => { expect(await trophyCount()).toBe(1); }); + test("stays accepted even if approvals drop below the required count", async () => { + for (const userId of reviewerIds.slice(0, TROPHY_APPROVALS_REQUIRED)) { + await TrophyRepository.addApproval({ pendingTrophyId, userId }); + } + + // biome-ignore lint/plugin: simulates raising TROPHY_APPROVALS_REQUIRED after acceptance, which no production code path can do + await db + .deleteFrom("PendingTrophyApproval") + .where("pendingTrophyId", "=", pendingTrophyId) + .where("userId", "=", reviewerIds[0]) + .execute(); + + expect( + await TrophyRepository.addApproval({ + pendingTrophyId, + userId: reviewerIds[TROPHY_APPROVALS_REQUIRED], + }), + ).toBe(null); + + expect( + await TrophyRepository.declinePending({ + id: pendingTrophyId, + reason: "reason", + declinedByUserId: reviewerIds[TROPHY_APPROVALS_REQUIRED], + }), + ).toBe(false); + + expect(await trophyCount()).toBe(1); + }); + test("approvals after a decline do not create a trophy", async () => { await TrophyRepository.declinePending({ id: pendingTrophyId, diff --git a/app/features/trophies/TrophyRepository.server.ts b/app/features/trophies/TrophyRepository.server.ts index 74a3be8d5..73c92d223 100644 --- a/app/features/trophies/TrophyRepository.server.ts +++ b/app/features/trophies/TrophyRepository.server.ts @@ -469,21 +469,7 @@ export async function existsByName(args: { .select("id") .where("name", "=", args.name) .where("declinedAt", "is", null) - .where((eb) => - eb( - eb - .selectFrom("PendingTrophyApproval") - .select(({ fn }) => fn.countAll().as("count")) - .whereRef( - "PendingTrophyApproval.pendingTrophyId", - "=", - "PendingTrophy.id", - ) - .$asScalar(), - "<", - TROPHY_APPROVALS_REQUIRED, - ), - ); + .where("acceptedAt", "is", null); if (args.excludeTrophyId !== undefined) { pendingQuery = pendingQuery.where( @@ -727,6 +713,7 @@ function pendingBaseQuery() { "PendingTrophy.declineReason", "PendingTrophy.declinedAt", "PendingTrophy.declinedByUserId", + "PendingTrophy.acceptedAt", "PendingTrophy.targetTrophyId", "PendingTrophy.managerId", "Submitter.username as submitterUsername", @@ -767,20 +754,7 @@ export async function unreviewedCountBySubmitter(submitterUserId: number) { .select((eb) => eb.fn.countAll().as("count")) .where("submitterUserId", "=", submitterUserId) .where("declinedAt", "is", null) - .where((eb) => - eb( - eb - .selectFrom("PendingTrophyApproval") - .select((eb2) => eb2.fn.countAll().as("approvalCount")) - .whereRef( - "PendingTrophyApproval.pendingTrophyId", - "=", - "PendingTrophy.id", - ), - "<", - TROPHY_APPROVALS_REQUIRED, - ), - ) + .where("acceptedAt", "is", null) .executeTakeFirstOrThrow(); return row.count; @@ -796,13 +770,13 @@ export async function declinePending(args: { declinedByUserId: number; }) { return db.transaction().execute(async (trx) => { - const { count } = await trx - .selectFrom("PendingTrophyApproval") - .select((eb) => eb.fn.countAll().as("count")) - .where("pendingTrophyId", "=", args.id) - .executeTakeFirstOrThrow(); + const pending = await trx + .selectFrom("PendingTrophy") + .select("acceptedAt") + .where("id", "=", args.id) + .executeTakeFirst(); - if (count >= TROPHY_APPROVALS_REQUIRED) { + if (!pending || pending.acceptedAt !== null) { return false; } @@ -850,7 +824,7 @@ export async function addApproval(args: { .where("pendingTrophyId", "=", args.pendingTrophyId) .executeTakeFirstOrThrow(); - if (count !== TROPHY_APPROVALS_REQUIRED) { + if (count < TROPHY_APPROVALS_REQUIRED) { return null; } @@ -867,10 +841,17 @@ export async function addApproval(args: { ]) .where("id", "=", args.pendingTrophyId) .where("declinedAt", "is", null) + .where("acceptedAt", "is", null) .executeTakeFirst(); if (!pending) return null; + await trx + .updateTable("PendingTrophy") + .set({ acceptedAt: dateToDatabaseTimestamp(new Date()) }) + .where("id", "=", args.pendingTrophyId) + .execute(); + if (pending.targetTrophyId !== null) { await trx .updateTable("Trophy") diff --git a/app/features/trophies/actions/trophies.new.server.ts b/app/features/trophies/actions/trophies.new.server.ts index 785ab227c..93820f207 100644 --- a/app/features/trophies/actions/trophies.new.server.ts +++ b/app/features/trophies/actions/trophies.new.server.ts @@ -11,10 +11,7 @@ import { parseFormData } from "~/form/parse.server"; import { errorToastIfFalsy, parseRequestPayload } from "~/utils/remix.server"; import { assertUnreachable } from "~/utils/types"; import * as TrophyRepository from "../TrophyRepository.server"; -import { - TROPHY_APPROVALS_REQUIRED, - TROPHY_PENDING_PER_USER_LIMIT, -} from "../trophies-constants"; +import { TROPHY_PENDING_PER_USER_LIMIT } from "../trophies-constants"; import { pendingTrophyActionSchema, trophyFormSchema, @@ -135,7 +132,7 @@ export const action: ActionFunction = async ({ request }) => { errorToastIfFalsy(pending, "Pending trophy not found"); errorToastIfFalsy(!pending.declinedAt, "Trophy is already declined"); errorToastIfFalsy( - pending.approvals.length < TROPHY_APPROVALS_REQUIRED, + !pending.acceptedAt, "Cannot decline an accepted trophy", ); @@ -170,10 +167,7 @@ export const action: ActionFunction = async ({ request }) => { !pending.declinedAt, "Cannot approve a declined trophy", ); - errorToastIfFalsy( - pending.approvals.length < TROPHY_APPROVALS_REQUIRED, - "Trophy is already accepted", - ); + errorToastIfFalsy(!pending.acceptedAt, "Trophy is already accepted"); errorToastIfFalsy( !pending.approvals.some((a) => a.userId === user.id), "Already approved", diff --git a/app/features/trophies/loaders/trophies.new.server.ts b/app/features/trophies/loaders/trophies.new.server.ts index da0d47be7..95dfd97a4 100644 --- a/app/features/trophies/loaders/trophies.new.server.ts +++ b/app/features/trophies/loaders/trophies.new.server.ts @@ -2,7 +2,6 @@ import type { LoaderFunctionArgs } from "react-router"; import { requireUser } from "~/features/auth/core/user.server"; import type { SerializeFrom } from "~/utils/remix"; import * as TrophyRepository from "../TrophyRepository.server"; -import { TROPHY_APPROVALS_REQUIRED } from "../trophies-constants"; import { canEditAnyTrophy, canReviewTrophies } from "../trophies-utils"; export type NewTrophyLoaderData = SerializeFrom; @@ -25,7 +24,7 @@ export const loader = async (_args: LoaderFunctionArgs) => { const allItems = canReview ? rawItems : rawItems.map(stripReviewerInfo); const isAccepted = (item: (typeof allItems)[number]) => - item.approvals.length >= TROPHY_APPROVALS_REQUIRED; + item.acceptedAt !== null; const pendingTrophies = allItems.filter( (item) => !isAccepted(item) && !item.declinedAt, diff --git a/app/features/trophies/routes/trophies.new.tsx b/app/features/trophies/routes/trophies.new.tsx index 441c23fee..c2a4c030a 100644 --- a/app/features/trophies/routes/trophies.new.tsx +++ b/app/features/trophies/routes/trophies.new.tsx @@ -642,7 +642,7 @@ function TrophyListRow({ const isOwner = pending.submitterUserId === currentUserId; const isDeclined = pending.declinedAt !== null; - const isAccepted = pending.approvals.length >= TROPHY_APPROVALS_REQUIRED; + const isAccepted = pending.acceptedAt !== null; const isReviewed = isDeclined || isAccepted; const alreadyApproved = pending.approvals.some( (a) => a.userId === currentUserId, diff --git a/migrations/20260805155023-pending-trophy-accepted-at.ts b/migrations/20260805155023-pending-trophy-accepted-at.ts new file mode 100644 index 000000000..3bd081079 --- /dev/null +++ b/migrations/20260805155023-pending-trophy-accepted-at.ts @@ -0,0 +1,114 @@ +import { type Kysely, sql } from "kysely"; + +const duplicateMapping = sql` + select + dup."id" as "dupId", + ( + select min(kept."id") + from "Trophy" kept + where kept."code" is null + and kept."name" = dup."name" + and kept."creatorId" is dup."creatorId" + ) as "keepId" + from "Trophy" dup + where dup."code" is null + and exists ( + select 1 + from "Trophy" kept + where kept."code" is null + and kept."name" = dup."name" + and kept."creatorId" is dup."creatorId" + and kept."id" < dup."id" + ) +`; + +/** + * Persists trophy submission acceptance instead of deriving it from the approval + * count, so that raising TROPHY_APPROVALS_REQUIRED cannot put already accepted + * submissions back into the review queue. Also deletes duplicate trophies that + * re-approving an already accepted submission created, keeping the oldest one. + */ +export async function up(db: Kysely): Promise { + // kysely does not wrap sqlite migrations in a transaction, so do it here + await db.transaction().execute(async (trx) => { + await trx.schema + .alterTable("PendingTrophy") + .addColumn("acceptedAt", "integer") + .execute(); + + // Repoint references at duplicates to the kept trophy before deleting, so + // the on-delete cascades cannot destroy owner rows or pending submissions. + // A duplicate is a user-submitted trophy sharing name and creator with an + // older one, which the review flow cannot legitimately produce. + await sql` + update "TrophyOwner" + set "trophyId" = m."keepId" + from (${duplicateMapping}) m + where "TrophyOwner"."trophyId" = m."dupId" + and not exists ( + select 1 + from "TrophyOwner" existing + where existing."tournamentId" = "TrophyOwner"."tournamentId" + and existing."userId" = "TrophyOwner"."userId" + and existing."trophyId" = m."keepId" + ) + `.execute(trx); + + await sql` + update "CalendarEvent" + set "trophyId" = m."keepId" + from (${duplicateMapping}) m + where "CalendarEvent"."trophyId" = m."dupId" + `.execute(trx); + + await sql` + update "PendingTrophy" + set "targetTrophyId" = m."keepId" + from (${duplicateMapping}) m + where "PendingTrophy"."targetTrophyId" = m."dupId" + `.execute(trx); + + await sql` + delete from "Trophy" + where "id" in (select "dupId" from (${duplicateMapping})) + `.execute(trx); + + // Backfill submissions accepted under the old threshold of 2 approvals. + // A row only counts as accepted if its trophy was actually created. + await sql` + update "PendingTrophy" + set "acceptedAt" = ( + select max("createdAt") + from "PendingTrophyApproval" + where "PendingTrophyApproval"."pendingTrophyId" = "PendingTrophy"."id" + ) + where "declinedAt" is null + and ( + select count(*) + from "PendingTrophyApproval" + where "PendingTrophyApproval"."pendingTrophyId" = "PendingTrophy"."id" + ) >= 2 + and ( + ( + "targetTrophyId" is null + and exists ( + select 1 + from "Trophy" + where "Trophy"."name" = "PendingTrophy"."name" + and "Trophy"."creatorId" = "PendingTrophy"."submitterUserId" + ) + ) + or ( + "targetTrophyId" is not null + and exists ( + select 1 + from "Trophy" + where "Trophy"."id" = "PendingTrophy"."targetTrophyId" + and "Trophy"."name" = "PendingTrophy"."name" + and "Trophy"."model" = "PendingTrophy"."model" + ) + ) + ) + `.execute(trx); + }); +}