From 9a39267bdb32929618d463bb3e0ee437d5cae1d6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 29 Jun 2026 23:40:19 +0000 Subject: [PATCH] Address code-review findings: ping guard, dual filter, inline helper - Restore winnerScoreChanged guard on winnerDiscordUserId so advancing-only winners are shown in the embed but not Discord-pinged - Add display-name filter at the end of the scoredMatches chain so scoredMatches always reflects exactly what appears in the embed, eliminating the dual-gate split between scoring-calculator.ts and discord.ts - Remove isMatchNotable export and inline x.winnerScoreChanged || x.showLoser at its sole call site; move the reasoning into the existing comment block Co-Authored-By: Claude --- .../__tests__/process-match-result.test.ts | 19 +------- app/models/scoring-calculator.ts | 43 +++++++------------ app/services/__tests__/discord.test.ts | 4 +- 3 files changed, 18 insertions(+), 48 deletions(-) diff --git a/app/models/__tests__/process-match-result.test.ts b/app/models/__tests__/process-match-result.test.ts index 4484a18..df825ee 100644 --- a/app/models/__tests__/process-match-result.test.ts +++ b/app/models/__tests__/process-match-result.test.ts @@ -59,7 +59,7 @@ function makeDb(existingResult?: { id: string; isPartialScore: boolean }) { }; } -import { processMatchResult, isLoserNotifiable, isMatchNotable, processPlayoffEvent } from "../scoring-calculator"; +import { processMatchResult, isLoserNotifiable, processPlayoffEvent } from "../scoring-calculator"; import { doesLoserAdvance } from "../playoff-match"; import { updateProbabilitiesAfterResult } from "~/services/probability-updater"; @@ -399,23 +399,6 @@ describe("isLoserNotifiable", () => { }); }); -describe("isMatchNotable", () => { - it("returns true when winner scored (loser not notifiable)", () => { - expect(isMatchNotable(true, false)).toBe(true); - }); - - it("returns true when loser is notifiable (winner did not score)", () => { - expect(isMatchNotable(false, true)).toBe(true); - }); - - it("returns true when both winner scored and loser is notifiable", () => { - expect(isMatchNotable(true, true)).toBe(true); - }); - - it("returns false when winner only advanced (no points) and loser is not notifiable", () => { - expect(isMatchNotable(false, false)).toBe(false); - }); -}); describe("processPlayoffEvent - NBA Play-In Round 1 loserAdvances fix", () => { function makePlayInDb(matches: object[]) { diff --git a/app/models/scoring-calculator.ts b/app/models/scoring-calculator.ts index 9a40961..f5cb685 100644 --- a/app/models/scoring-calculator.ts +++ b/app/models/scoring-calculator.ts @@ -623,24 +623,6 @@ export function isLoserNotifiable( return scoreChanged || finalizedLoserIds.has(loserId); } -/** - * Returns true if a scored match should be displayed in a Discord standings - * notification at all. - * - * A match is shown only when the winner earned points this round or the loser is - * notifiable (eliminated / scored — see {@link isLoserNotifiable}). A drafted - * winner that merely advanced without earning points is NOT enough on its own. - * - * The winner's owner is still credited on any match that passes this gate (even - * a points-less advance) — this function only decides whether the match line - * appears, not which managers are labelled within it. - */ -export function isMatchNotable( - winnerScoreChanged: boolean, - loserNotifiable: boolean -): boolean { - return winnerScoreChanged || loserNotifiable; -} export function getGuaranteedMinimumPosition( round: string, @@ -1845,13 +1827,17 @@ export async function recalculateAffectedLeagues( ); // Build scored matches for the notification. - // A match is included only when it is "notable": either the winner earned points this - // round OR the loser is notifiable (eliminated / scored). A drafted winner that merely - // advanced without earning points is NOT enough on its own (see isMatchNotable). - // On any match that IS included, the winning team's owner is always credited — even - // when the winner only advanced. Losers appear if their score changed or they were - // definitively eliminated (finalPosition set, non-partial); losers who advance to - // another match (loserAdvances=true, e.g. NBA 7v8 → PIR2) are correctly suppressed. + // A match is included only when it is "notable": winner earned points this round OR + // the loser is notifiable (eliminated / scored). A drafted winner that merely advanced + // without earning points is NOT enough on its own. + // On any included match, the winning team's owner is shown in the embed (winnerUsername), + // but Discord-pinged only when they actually scored (winnerDiscordUserId gated on + // winnerScoreChanged) — advancing-only wins don't warrant a ping. + // Losers appear if their score changed or they were definitively eliminated (finalPosition + // set, non-partial); losers who advance to another match (loserAdvances=true, e.g. NBA + // 7v8 → PIR2) are correctly suppressed. + // Only entries with at least one displayable username are kept, so scoredMatches always + // reflects exactly what will be shown in the embed. let scoredMatches: ScoredMatch[] | undefined; if (allCompletedMatches.length > 0) { const relevant = allCompletedMatches.filter( @@ -1879,15 +1865,16 @@ export async function recalculateAffectedLeagues( const showLoser = isLoserNotifiable(m.loserId, loserTeamId, changedTeamIds, finalizedLoserIds); return { m, winnerScoreChanged, showLoser, winnerOwnerId, loserOwnerId }; }) - .filter((x) => isMatchNotable(x.winnerScoreChanged, x.showLoser)) + .filter((x) => x.winnerScoreChanged || x.showLoser) .map((x) => ({ winnerName: x.m.winnerName ?? "", loserName: x.m.loserName ?? "", winnerUsername: x.winnerOwnerId ? usernameByUserId.get(x.winnerOwnerId) : undefined, loserUsername: x.showLoser && x.loserOwnerId ? usernameByUserId.get(x.loserOwnerId) : undefined, - winnerDiscordUserId: x.winnerOwnerId ? discordIdByUserId.get(x.winnerOwnerId) : undefined, + winnerDiscordUserId: x.winnerScoreChanged && x.winnerOwnerId ? discordIdByUserId.get(x.winnerOwnerId) : undefined, loserDiscordUserId: x.showLoser && x.loserOwnerId ? discordIdByUserId.get(x.loserOwnerId) : undefined, - })); + })) + .filter((m) => m.winnerUsername !== undefined || m.loserUsername !== undefined); } } diff --git a/app/services/__tests__/discord.test.ts b/app/services/__tests__/discord.test.ts index db897a7..45d97a3 100644 --- a/app/services/__tests__/discord.test.ts +++ b/app/services/__tests__/discord.test.ts @@ -188,8 +188,8 @@ describe("sendStandingsUpdateNotification", () => { it("shows winner's owner even when winner only advanced (loser was eliminated)", async () => { // Mirrors the Brazil/Japan scenario: Brazil advanced without earning points, - // Japan was eliminated. The calculator now sets winnerUsername unconditionally - // on any match that passes the notability gate. + // Japan was eliminated. winnerUsername is shown in the embed; winnerDiscordUserId + // is left unset (no ping) because the winner did not score this round. await sendStandingsUpdateNotification({ webhookUrl: WEBHOOK_URL, seasonName: "Rumble League 2026",