From 5dfe39bba2f7459087a8dcf31858af4d392d88de Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 18 Jun 2026 23:46:59 +0000 Subject: [PATCH] Fix code-review findings on qualifying-bracket QP scoring - #1 (correctness): processQualifyingEvent regrouped event_results by placement using the live group size as the tie count. The "Process Qualifying Points" button could run mid-bracket and average four placement-3 QF winners down to 7 QP instead of their 9 floor. processQualifyingEvent now delegates bracket events to processQualifyingBracketEvent (structural tie spans) and skips the live-count regroup. finalize-bracket simplified to call processQualifyingEvent. - #2: reprocess-bracket on a finalized qualifying season re-runs finalizeQualifyingPoints so the deleted final placements are restored. - #3: replace direct Drizzle deletes in the route with the existing deleteParticipantResultsBySportsSeasonId model helper. - #4: extract shared writeEventResultsQP helper; assignCs2EliminationQP and processQualifyingBracketEvent both use it instead of duplicating the upsert + recalculateParticipantQP loop. - #5: extract scoreQualifyingBracket route helper for the repeated branch. - #6: document that qualifying brackets intentionally skip the fantasy Recent Scores feed (they surface via QP standings + Discord); scope the set-winner Discord notification to the entered match. Adds a regression test locking the structural-tieCount contract (9 QP, not 7). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QVLqaYGXCeSYYqTHZmbRZi --- .../qualifying-bracket-scoring.test.ts | 19 +++ app/models/cs2-major-stage.ts | 35 +---- app/models/participant-result.ts | 5 +- app/models/qualifying-points.ts | 51 +++++++ app/models/scoring-calculator.ts | 136 ++++++++---------- ...sons.$id.events.$eventId.bracket.server.ts | 68 ++++++--- 6 files changed, 185 insertions(+), 129 deletions(-) diff --git a/app/models/__tests__/qualifying-bracket-scoring.test.ts b/app/models/__tests__/qualifying-bracket-scoring.test.ts index a638123..eb6325f 100644 --- a/app/models/__tests__/qualifying-bracket-scoring.test.ts +++ b/app/models/__tests__/qualifying-bracket-scoring.test.ts @@ -170,6 +170,25 @@ describe("guaranteed-minimum QP per outcome (default config)", () => { }); }); +// ── Regression guard: structural tieCount, not live count (the #1 fix) ───────── + +describe("structural tie span vs. live-count regrouping", () => { + it("4 QF winners share placement 3 but each keeps the 2-slot (9 QP) floor", () => { + const s = states(QF_ALL); + const winners = ["t1", "t2", "t3", "t4"].map((id) => s.get(id)!); + // All four sit at placement 3, but with the STRUCTURAL tie span of 2. + for (const w of winners) { + expect(w).toEqual({ placement: 3, tieCount: 2 }); + expect(qpFor(w.placement, w.tieCount)).toBeCloseTo(9); + } + // A naive regroup by LIVE count (4 rows at placement 3) would average over + // slots 3–6 and wrongly yield 7 QP — the bug processQualifyingEvent must avoid + // by delegating bracket events to the structural-tieCount path. + expect(qpFor(3, 4)).toBeCloseTo(7); + expect(qpFor(3, 4)).not.toBeCloseTo(9); + }); +}); + // ── Regression guard: reused ROUND_CONFIG floors are intact ──────────────────── describe("ROUND_CONFIG floors (regression guard)", () => { diff --git a/app/models/cs2-major-stage.ts b/app/models/cs2-major-stage.ts index 98fd409..5008079 100644 --- a/app/models/cs2-major-stage.ts +++ b/app/models/cs2-major-stage.ts @@ -13,9 +13,9 @@ */ import { database } from "~/database/context"; -import { cs2MajorStageResults, seasonParticipants, eventResults } from "~/database/schema"; +import { cs2MajorStageResults, seasonParticipants } from "~/database/schema"; import { eq, and, sql } from "drizzle-orm"; -import { getQPConfig, recalculateParticipantQP, calculateSplitQualifyingPoints } from "~/models/qualifying-points"; +import { getQPConfig, calculateSplitQualifyingPoints, writeEventResultsQP } from "~/models/qualifying-points"; export interface Cs2StageResult { id: string; @@ -394,34 +394,5 @@ export async function assignCs2EliminationQP( } } - const now = new Date(); - - await Promise.all( - [...resultsByParticipant.entries()].map(([seasonParticipantId, { qp, placement }]) => - db - .insert(eventResults) - .values({ - scoringEventId, - seasonParticipantId, - qualifyingPointsAwarded: qp.toString(), - placement, - createdAt: now, - updatedAt: now, - }) - .onConflictDoUpdate({ - target: [eventResults.scoringEventId, eventResults.seasonParticipantId], - set: { - qualifyingPointsAwarded: qp.toString(), - placement, - updatedAt: now, - }, - }) - ) - ); - - await Promise.all( - [...resultsByParticipant.keys()].map((seasonParticipantId) => - recalculateParticipantQP(seasonParticipantId, sportsSeasonId) - ) - ); + await writeEventResultsQP(scoringEventId, sportsSeasonId, resultsByParticipant, db); } diff --git a/app/models/participant-result.ts b/app/models/participant-result.ts index 8c76e04..4e36257 100644 --- a/app/models/participant-result.ts +++ b/app/models/participant-result.ts @@ -95,9 +95,10 @@ export async function deleteParticipantResult(id: string): Promise { } export async function deleteParticipantResultsBySportsSeasonId( - sportsSeasonId: string + sportsSeasonId: string, + providedDb?: ReturnType ): Promise { - const db = database(); + const db = providedDb || database(); await db .delete(schema.seasonParticipantResults) .where(eq(schema.seasonParticipantResults.sportsSeasonId, sportsSeasonId)); diff --git a/app/models/qualifying-points.ts b/app/models/qualifying-points.ts index d069ac9..cde2d03 100644 --- a/app/models/qualifying-points.ts +++ b/app/models/qualifying-points.ts @@ -283,6 +283,57 @@ export async function recalculateParticipantQP( return { totalQP, eventsScored }; } +/** + * Upsert qualifying points + placement into event_results for a set of participants, + * then recalculate each participant's QP total. + * + * Shared by the QP writers (assignCs2EliminationQP for Swiss-stage exits, + * processQualifyingBracketEvent for Champions-Stage / knockout brackets) so the + * upsert + recalc loop lives in one place. Upserts on + * (scoringEventId, seasonParticipantId), so repeated calls overwrite cleanly. + * + * @param entries Map from seasonParticipantId → { qp, placement } to write. + */ +export async function writeEventResultsQP( + scoringEventId: string, + sportsSeasonId: string, + entries: Map, + providedDb?: ReturnType +): Promise { + if (entries.size === 0) return; + const db = providedDb || database(); + const now = new Date(); + + await Promise.all( + [...entries.entries()].map(([seasonParticipantId, { qp, placement }]) => + db + .insert(schema.eventResults) + .values({ + scoringEventId, + seasonParticipantId, + qualifyingPointsAwarded: qp.toFixed(2), + placement, + createdAt: now, + updatedAt: now, + }) + .onConflictDoUpdate({ + target: [schema.eventResults.scoringEventId, schema.eventResults.seasonParticipantId], + set: { + qualifyingPointsAwarded: qp.toFixed(2), + placement, + updatedAt: now, + }, + }) + ) + ); + + await Promise.all( + [...entries.keys()].map((seasonParticipantId) => + recalculateParticipantQP(seasonParticipantId, sportsSeasonId, db) + ) + ); +} + /** * Recalculate eventsScored for a participant by counting unique events with QP awarded * @deprecated Use recalculateParticipantQP instead for more accurate recalculation diff --git a/app/models/scoring-calculator.ts b/app/models/scoring-calculator.ts index 8852f49..4b28bf6 100644 --- a/app/models/scoring-calculator.ts +++ b/app/models/scoring-calculator.ts @@ -23,6 +23,7 @@ import { getQPConfig, hasProcessedQualifyingPlacement, recalculateParticipantQP, + writeEventResultsQP, getQPStandings, updateFinalRankings, } from "./qualifying-points"; @@ -749,11 +750,13 @@ export function deriveBracketQualifyingStates( * qualifying_points sport the per-major fantasy placement is meaningless — final * fantasy placements come solely from finalizeQualifyingPoints across all majors. * - * Invariant: progressive QP totals come from recalculateParticipantQP summing the - * directly-written qualifyingPointsAwarded — never from interim re-grouping. - * processQualifyingEvent's placement-grouping recompute only runs at finalization, - * by which point the bracket is complete and the placement groups have their - * structural sizes, so both paths produce identical QP. + * QP is written directly with the placement tier's STRUCTURAL tie span (QF tier + * spans 4 slots, SF tier 2, finalist/champion 1) — not the live count of teams + * currently at a placement. This is what makes a QF-winner floor 9 QP (avg of + * 3rd+4th) even while all four QF winners transiently share placement 3. + * processQualifyingEvent delegates to this function for bracket events precisely + * so it does NOT re-group those rows by live count (which would average four + * placement-3 rows down to 7). * * Idempotent: re-running on the same match state recomputes identical values. */ @@ -799,37 +802,14 @@ export async function processQualifyingBracketEvent( qpConfigArray.map((c) => [c.placement, parseFloat(c.points)]) ); - const now = new Date(); - - await Promise.all( - [...states.entries()].map(([seasonParticipantId, { placement, tieCount }]) => { - const qp = calculateSplitQualifyingPoints(placement, tieCount, qpMap); - return db - .insert(schema.eventResults) - .values({ - scoringEventId: eventId, - seasonParticipantId, - qualifyingPointsAwarded: qp.toFixed(2), - placement, - createdAt: now, - updatedAt: now, - }) - .onConflictDoUpdate({ - target: [schema.eventResults.scoringEventId, schema.eventResults.seasonParticipantId], - set: { - qualifyingPointsAwarded: qp.toFixed(2), - placement, - updatedAt: now, - }, - }); - }) + const entries = new Map( + [...states.entries()].map(([id, { placement, tieCount }]) => [ + id, + { qp: calculateSplitQualifyingPoints(placement, tieCount, qpMap), placement }, + ]) ); - await Promise.all( - [...states.keys()].map((seasonParticipantId) => - recalculateParticipantQP(seasonParticipantId, event.sportsSeasonId, db) - ) - ); + await writeEventResultsQP(eventId, event.sportsSeasonId, entries, db); } /** @@ -864,52 +844,62 @@ export async function processQualifyingEvent( // Check if this was already processed (for majorsCompleted counter) const wasAlreadyProcessed = hasProcessedQualifyingPlacement(results); - // Clear stale QP before reprocessing so removed/null placements do not keep old awards. - for (const result of results) { - await db - .update(schema.eventResults) - .set({ - qualifyingPointsAwarded: "0", - updatedAt: new Date(), - }) - .where(eq(schema.eventResults.id, result.id)); - } - - const qpConfig = await getQPConfig(event.sportsSeasonId, db); - const pointsByPlacement = new Map( - qpConfig.map((config) => [config.placement, parseFloat(config.points)]) - ); - - // Group results by placement to handle ties - const placementGroups = new Map(); - for (const result of results) { - if (result.placement) { - const group = placementGroups.get(result.placement) || []; - group.push(result); - placementGroups.set(result.placement, group); - } - } - - // Process each placement group and update event_results with QP awarded - for (const [placement, group] of placementGroups) { - const tieCount = group.length; - - const qpPerParticipant = calculateSplitQualifyingPoints( - placement, - tieCount, - pointsByPlacement - ); - - // Update the event result with the QP awarded - for (const result of group) { + if (event.bracketTemplateId) { + // Bracket-based qualifying event (e.g. CS2 Champions Stage): QP is owned by the + // bracket/stage writers, which assign each placement its STRUCTURAL tie span. + // Re-derive via processQualifyingBracketEvent and leave the Swiss-exit rows + // (written by assignCs2EliminationQP) untouched. This deliberately skips the + // placement-regroup below — re-grouping provisional floors by their LIVE count + // would average four placement-3 QF winners down to 7 QP instead of their 9 floor. + await processQualifyingBracketEvent(eventId, db); + } else { + // Clear stale QP before reprocessing so removed/null placements do not keep old awards. + for (const result of results) { await db .update(schema.eventResults) .set({ - qualifyingPointsAwarded: qpPerParticipant.toFixed(2), + qualifyingPointsAwarded: "0", updatedAt: new Date(), }) .where(eq(schema.eventResults.id, result.id)); } + + const qpConfig = await getQPConfig(event.sportsSeasonId, db); + const pointsByPlacement = new Map( + qpConfig.map((config) => [config.placement, parseFloat(config.points)]) + ); + + // Group results by placement to handle ties + const placementGroups = new Map(); + for (const result of results) { + if (result.placement) { + const group = placementGroups.get(result.placement) || []; + group.push(result); + placementGroups.set(result.placement, group); + } + } + + // Process each placement group and update event_results with QP awarded + for (const [placement, group] of placementGroups) { + const tieCount = group.length; + + const qpPerParticipant = calculateSplitQualifyingPoints( + placement, + tieCount, + pointsByPlacement + ); + + // Update the event result with the QP awarded + for (const result of group) { + await db + .update(schema.eventResults) + .set({ + qualifyingPointsAwarded: qpPerParticipant.toFixed(2), + updatedAt: new Date(), + }) + .where(eq(schema.eventResults.id, result.id)); + } + } } // Recalculate totals for all participants from scratch diff --git a/app/routes/admin.sports-seasons.$id.events.$eventId.bracket.server.ts b/app/routes/admin.sports-seasons.$id.events.$eventId.bracket.server.ts index c8ea068..ee227ab 100644 --- a/app/routes/admin.sports-seasons.$id.events.$eventId.bracket.server.ts +++ b/app/routes/admin.sports-seasons.$id.events.$eventId.bracket.server.ts @@ -27,13 +27,18 @@ import { processMatchResult, processQualifyingBracketEvent, processQualifyingEvent, + finalizeQualifyingPoints, recalculateAffectedLeagues, recalculateStandings, autoCompleteRoundIfDone, } from "~/models/scoring-calculator"; import { updateProbabilitiesAfterResult } from "~/services/probability-updater"; import { getBracketTemplate, ALL_16_SEEDS, type BracketRegion } from "~/lib/bracket-templates"; -import { setParticipantResult, findParticipantResultsBySportsSeasonId } from "~/models/participant-result"; +import { + setParticipantResult, + findParticipantResultsBySportsSeasonId, + deleteParticipantResultsBySportsSeasonId, +} from "~/models/participant-result"; import { findSeasonSportsBySportsSeasonId } from "~/models/season-sport"; import { createDailySnapshot } from "~/models/standings"; import { @@ -105,6 +110,28 @@ export async function loader({ params }: Route.LoaderArgs) { }; } +/** + * Score a qualifying-event bracket (e.g. CS2 Champions Stage): derive each team's + * guaranteed-minimum QP from the bracket and refresh affected league standings. + * + * Qualifying brackets award QUALIFYING POINTS, not fantasy points, so this never + * writes seasonParticipantResults and never records team_score_events — match + * results surface via the QP Standings and the Discord standings update. Final + * fantasy placements come from finalizeQualifyingPoints across all majors. + */ +async function scoreQualifyingBracket( + event: { id: string; sportsSeasonId: string; name: string | null }, + db: ReturnType, + recalcOptions?: Parameters[2] +): Promise { + await processQualifyingBracketEvent(event.id, db); + await recalculateAffectedLeagues( + event.sportsSeasonId, + db, + recalcOptions ?? { eventId: event.id, eventName: event.name ?? undefined } + ); +} + export async function action({ request, params }: Route.ActionArgs) { const formData = await request.formData(); const intent = formData.get("intent"); @@ -273,12 +300,11 @@ export async function action({ request, params }: Route.ActionArgs) { if (event.isQualifyingEvent) { // Qualifying major (e.g. CS2): bracket results award QUALIFYING POINTS, not - // fantasy points. Derive each team's guaranteed-minimum QP floor from the - // bracket and write it to event_results — do NOT touch seasonParticipantResults. - await processQualifyingBracketEvent(event.id, db); - await recalculateAffectedLeagues(event.sportsSeasonId, db, { + // fantasy points. matchIds scopes the Discord notification to just this match. + await scoreQualifyingBracket(event, db, { eventId: event.id, eventName: event.name ?? undefined, + matchIds: [matchId], }); } else { // Immediately score this match: loser gets their final placement, @@ -504,12 +530,7 @@ export async function action({ request, params }: Route.ActionArgs) { // processQualifyingBracketEvent — there are no seasonParticipantResults rows to // validate against, and final fantasy placements come from finalizeQualifyingPoints. if (event.isQualifyingEvent) { - const qpDb = database(); - await processQualifyingBracketEvent(params.eventId, qpDb); - await recalculateAffectedLeagues(event.sportsSeasonId, qpDb, { - eventId: params.eventId, - eventName: event.name ?? undefined, - }); + await scoreQualifyingBracket(event, database()); return { success: `${round} completed and qualifying points updated` }; } @@ -605,12 +626,17 @@ export async function action({ request, params }: Route.ActionArgs) { // finalizeQualifyingPoints across all majors. if (event.isQualifyingEvent) { const db = database(); - await db - .delete(schema.seasonParticipantResults) - .where(eq(schema.seasonParticipantResults.sportsSeasonId, event.sportsSeasonId)); + await deleteParticipantResultsBySportsSeasonId(event.sportsSeasonId, db); await processQualifyingBracketEvent(params.eventId, db); - // skipDiscord: reprocess is a data-correction tool, not a result announcement. - await recalculateAffectedLeagues(event.sportsSeasonId, db, { skipDiscord: true }); + // If the season's QP was already finalized, the delete above wiped the final + // placements — recompute them from QP totals so standings aren't left blank. + const sportsSeason = await findSportsSeasonById(params.id); + if (sportsSeason?.qualifyingPointsFinalized) { + await finalizeQualifyingPoints(event.sportsSeasonId, db); // recalcs leagues itself + } else { + // skipDiscord: reprocess is a data-correction tool, not a result announcement. + await recalculateAffectedLeagues(event.sportsSeasonId, db, { skipDiscord: true }); + } return { success: `Reprocessed qualifying bracket: cleared stale fantasy points and recomputed QP (${completed.length} completed match(es)).`, }; @@ -624,11 +650,7 @@ export async function action({ request, params }: Route.ActionArgs) { // Only deleting partial rows leaves stale finalized rows that block // the "never un-finalize" guard in upsertParticipantResult. const db = database(); - await db - .delete(schema.seasonParticipantResults) - .where( - eq(schema.seasonParticipantResults.sportsSeasonId, event.sportsSeasonId) - ); + await deleteParticipantResultsBySportsSeasonId(event.sportsSeasonId, db); // Replay each completed match in bracket order (earlier rounds first). const template = event.bracketTemplateId ? getBracketTemplate(event.bracketTemplateId) : null; @@ -732,7 +754,9 @@ export async function action({ request, params }: Route.ActionArgs) { // fantasy placements come from finalizeQualifyingPoints across all of them. if (event.isQualifyingEvent) { const db = database(); - await processQualifyingBracketEvent(params.eventId, db); + // processQualifyingEvent derives the bracket QP (via processQualifyingBracketEvent), + // increments majorsCompleted, and recalcs participant QP totals. Season-wide + // fantasy finalization stays with finalizeQualifyingPoints across all majors. await processQualifyingEvent(params.eventId, db); await db .update(schema.scoringEvents)