Trophy fixes (#3306)

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;
 }

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,

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<number>().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<number>().as("count"))
 		.where("submitterUserId", "=", submitterUserId)
 		.where("declinedAt", "is", null)
-		.where((eb) =>
-			eb(
-				eb
-					.selectFrom("PendingTrophyApproval")
-					.select((eb2) => eb2.fn.countAll<number>().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<number>().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")

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

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<typeof loader>;
@@ -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,

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,

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<any>): Promise<void> {
+	// 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);
+	});
+}