diff --git a/CHANGELOG.md b/CHANGELOG.md index 40872c7..b543177 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12558,3 +12558,71 @@ Neste steg: igjen etter verifisering. **Rullet ut:** venter på bekreftelse. + +114. **Duplikat-spørring ved spillerimport uten e-post — 2026-08-18.** + Bruker meldte fra, med skjermbilde av org 8db22cb9 sin spillerpool: + "Dobbel-import erstatter ikke eksisterende, men blir liggende to + ganger." Undersøkt (les-only mot ekte `teecup_db`, ingen skriving): + ekte bug -- ni spillere i den organisasjonen (Anders Sperre, Daniel + Rypdal, Einar Vegheim, m.fl.) dublert, alle med IDENTISKE felt i + begge kopiene, alle uten e-post. Rotårsak i `create_players_bulk` + (`app/routers/players.py`): matching mot en eksisterende spiller + skjedde UTELUKKENDE på e-post -- en rad uten e-post hadde derfor + INGEN matchingsnøkkel og ble alltid satt inn på nytt, uansett om + en identisk spiller allerede fantes. + + Bruker presiserte eksplisitt hvordan dette skal løses: "Det bør + spørres om den enkelte av de det gjelder skal merges, overskrives + eller importeres som en tilsynelatende dobbeloppføring" -- IKKE en + stille automatisk sammenslåing på navn alene (for risikabelt: to + ulike ekte personer kan dele navn). + + **Backend:** `/orgs/{id}/players/bulk` sitt request-format endret + fra `list[PlayerCreate]` til `list[PlayerBulkRow]` (`data` + nye, + valgfrie `match_player_id`/`duplicate_action`). E-post-matching + UENDRET (fortsatt automatisk, entydig nok til at spørring ikke + trengs der). Når frontend har spurt organisator og fått svar for en + e-postløs rad, sendes svaret som `match_player_id`/`duplicate_ + action`: "merge" = samme COALESCE-oppførsel som e-post-matching + alltid har hatt (fyll kun tomme felt), "overwrite" = NY oppførsel, + eneste sti som erstatter allerede-satte felt, kun for denne ene + raden, "create_new" = uendret opprinnelig oppførsel (bevisst + dobbeloppføring, ikke en utilsiktet bug lenger). Ubesvarte/uten- + e-post-rader uten noe svar oppfører seg akkurat som før (opprett ny) + -- ingen regresjon for eksisterende kallere. + + **Frontend (`player-import-panel.tsx`):** "Lagre" i redigerings- + tabellen sjekker nå FØRST om noen rader mangler e-post OG + navnetreffer en spiller som allerede finnes i org-poolen (egen + `GET .../players`-sjekk). Finnes slike, stoppes lagringen og en ny + `DuplicateStep` vises -- én radiogruppe (Slå sammen/Overskriv/ + Importer som ny) per funnet duplikat, forhåndsvalgt "Slå sammen" + (tryggeste, ikke-destruktive standardvalg dersom organisator ikke + endrer det). Bevisst HÅNDKODET av Claude, ikke V0-eksportert som + resten av import-visningen (`player-import-view.tsx`) -- vurdert + som en liten, funksjonell, avgrenset spørre-skjerm satt inn i en + eksisterende lagre-flyt, ikke en ny selvstendig UI-flate (se + [[feedback_frontend_via_v0]]), og bygget med de samme globale + design-tokenene (border/bg-card/bg-primary) som resten av appen. + + **Verifisert:** `tsc --noEmit` rent, `vitest run` 55/55. Backend: + `tests/test_players_bulk.py` utvidet med tre nye tester (merge/ + overwrite/create_new), alle eksisterende oppdatert til ny + `PlayerBulkRow`-form -- 120/120 grønt (opp fra 117), kjørt via + `scripts/run_backend_tests.sh`. Frontend: midlertidig lokal + `/dupsteppreviewtmp`-side (kun i `next dev`, ALDRI committet) + rendret `DuplicateStep` med to falske duplikater -- skjermbilde + + a11y-snapshot bekreftet ekte radiogruppe-semantikk (label/legend, + ikke bare visuell styling), og at å klikke "Overskriv" faktisk + endrer valgt tilstand for riktig rad uendret for den andre. + Midlertidig side slettet igjen etter verifisering. + + **Produksjonsopprydding (de ni allerede dublerte spillerne i org + 8db22cb9):** IKKE utført ennå -- krever eksplisitt bekreftelse + (skriving mot ekte `teecup_db`), se egen plan lagt frem for bruker + samme dag. Alle ni par er byte-identiske i alle felt; den ene + kopien av hvert par er allerede meldt på turneringen (`tournament_ + participant`), den andre er helt ubrukt -- planen er å slette KUN + den ubrukte kopien av hvert par. + + **Rullet ut:** venter på bekreftelse. diff --git a/app/routers/players.py b/app/routers/players.py index ba21f04..fd9aff2 100644 --- a/app/routers/players.py +++ b/app/routers/players.py @@ -116,12 +116,27 @@ async def create_player( class PlayerBulkResult(BaseModel): player: Player - status: str # "created" | "updated" + status: str # "created" | "updated" | "overwritten" + + +class PlayerBulkRow(BaseModel): + """Én rad i en masseimport. `data` er selve spillerfeltene (uendret + `PlayerCreate`). `match_player_id`/`duplicate_action` er NYE + (2026-08-18) -- se docstringen på `create_players_bulk` for hvorfor.""" + + data: PlayerCreate + # Satt av frontend NÅR organisatoren har blitt bedt om, og svart på, et + # "dette ser ut som en duplikat"-varsel for denne raden (ingen e-post på + # raden, men navnet traff en eksisterende spiller i poolen) -- IKKE satt + # for rader uten et slikt varsel, som fortsatt går via den opprinnelige + # e-post-matchingen under. + match_player_id: str | None = None + duplicate_action: str | None = Field(default=None, pattern="^(merge|overwrite|create_new)$") @router.post("/orgs/{organization_id}/players/bulk", response_model=list[PlayerBulkResult], status_code=201) async def create_players_bulk( - body: list[PlayerCreate], + body: list[PlayerBulkRow], organization_id: str = Depends(get_authorized_org), ) -> list[PlayerBulkResult]: """Massimport (CSV-basert, se FEATURE_BACKLOG.md). Samme "match på @@ -130,21 +145,71 @@ async def create_players_bulk( utvidet til ALLE `PlayerCreate`-felt (registration.py sin variant mangler first_name/last_name/paid/comment, som ikke fantes da den ble skrevet). `display_name` og `paid` røres ALDRI på en eksisterende - match -- førstnevnte er organisators kanoniske navn, sistnevnte er en - ikke-nullbar boolean (COALESCE mot NULL er da alltid en no-op, som er - riktig oppførsel: en import skal ikke stille kunne endre en allerede - satt betalt-status). Hele lista i én transaksjon (org_connection sin - egen) -- ingen delvis-suksess-håndtering, brukeren har allerede - validert/rettet radene i tabellen før "Lagre" trykkes.""" + match via COALESCE-stien -- førstnevnte er organisators kanoniske navn, + sistnevnte er en ikke-nullbar boolean (COALESCE mot NULL er da alltid + en no-op, som er riktig oppførsel: en import skal ikke stille kunne + endre en allerede satt betalt-status). Hele lista i én transaksjon + (org_connection sin egen) -- ingen delvis-suksess-håndtering, brukeren + har allerede validert/rettet radene i tabellen før "Lagre" trykkes. + + `match_player_id`/`duplicate_action` (2026-08-18) -- ekte bug funnet i + produksjon: rader UTEN e-post har ingen matchingsnøkkel i det hele + tatt, så reimport av samme CSV la dem inn på nytt hver gang i stedet + for å oppdatere (ni spillere dublert i org 8db22cb9 ved reimport). + Fikset ved å la FRONTEND oppdage mulige duplikater (ingen e-post + + navnetreff mot en eksisterende spiller i poolen) og SPØRRE organisator + eksplisitt om hva som skal skje -- IKKE en stille automatisk + sammenslåing på navn alene (for risikabelt: to ulike ekte personer kan + dele navn). Svaret sendes tilbake her som `match_player_id` (hvilken + eksisterende spiller raden gjelder) + `duplicate_action`: + - "merge": samme COALESCE-oppførsel som e-post-matching allerede har + (fyll kun tomme felt på den eksisterende raden). + - "overwrite": erstatt ALLE feltene på den eksisterende raden med + verdiene fra denne importraden, også der den eksisterende raden + allerede hadde en verdi -- eneste sted i denne funksjonen som gjør + det, og KUN når organisator eksplisitt har bedt om det for akkurat + denne raden. + - "create_new" (eller `match_player_id` ikke satt i det hele tatt): + uendret -- opprett en ny rad, som før. + Rader MED e-post er upåvirket -- e-post-matching er entydig nok til at + den fortsatt skjer automatisk, ingen spørring nødvendig der.""" results: list[PlayerBulkResult] = [] async with org_connection(organization_id) as conn, translate_db_errors(): - for p in body: + for item in body: + p = item.data existing = None if p.email: existing = await conn.fetchrow( "SELECT id FROM player WHERE lower(email) = lower($1)", p.email ) - if existing is not None: + elif item.match_player_id and item.duplicate_action in ("merge", "overwrite"): + existing = {"id": item.match_player_id} + + if existing is not None and item.duplicate_action == "overwrite": + row = await conn.fetchrow( + f""" + UPDATE player SET + handicap_index = $2, gender = $3, mobile = $4, birth_date = $5, + nickname = $6, country = $7, club = $8, club_member_number = $9, + first_name = $10, last_name = $11, comment = $12 + WHERE id = $1 + RETURNING {_PLAYER_COLUMNS} + """, + existing["id"], + p.handicap_index, + p.gender, + p.mobile, + p.birth_date, + p.nickname, + p.country, + p.club, + p.club_member_number, + p.first_name, + p.last_name, + p.comment, + ) + results.append(PlayerBulkResult(player=Player(**dict(row)), status="overwritten")) + elif existing is not None: row = await conn.fetchrow( f""" UPDATE player SET diff --git a/frontend/components/player-import-panel.tsx b/frontend/components/player-import-panel.tsx index d676d61..2c4e57f 100644 --- a/frontend/components/player-import-panel.tsx +++ b/frontend/components/player-import-panel.tsx @@ -9,6 +9,7 @@ import { useState } from "react" import Papa from "papaparse" +import { TriangleAlert } from "lucide-react" import { PlayerImportView, type DraftRow, @@ -150,6 +151,20 @@ type ApiPlayer = { comment: string | null } +type DuplicateAction = "merge" | "overwrite" | "create_new" + +// Mulig duplikat (2026-08-18, ekte produksjonsbug): en importrad UTEN +// e-post har ingen pålitelig matchingsnøkkel i seg selv (se players.py +// sin create_players_bulk-docstring for hele historikken), så et +// navnetreff mot en spiller som allerede finnes i poolen blir aldri +// slått sammen automatisk -- for risikabelt, to ulike ekte personer kan +// dele navn. Organisator spørres eksplisitt i stedet, se DuplicateStep. +type DuplicateCandidate = { + row: DraftRow + existing: ApiPlayer + action: DuplicateAction +} + export function PlayerImportPanel({ organizationId, tournamentId, @@ -163,13 +178,14 @@ export function PlayerImportPanel({ onClose: () => void onImportComplete: (createdOrUpdated: { player: ApiPlayer; status: string }[]) => void }) { - const [step, setStep] = useState<"upload" | "map" | "edit" | "saving" | "done">("upload") + const [step, setStep] = useState<"upload" | "map" | "edit" | "duplicates" | "saving" | "done">("upload") const [csvHeaders, setCsvHeaders] = useState([]) const [csvRows, setCsvRows] = useState([]) const [mapping, setMapping] = useState>({}) const [rows, setRows] = useState([]) const [error, setError] = useState(null) const [saveWarnings, setSaveWarnings] = useState([]) + const [duplicates, setDuplicates] = useState([]) function handleFile(file: File) { setError(null) @@ -235,31 +251,70 @@ export function PlayerImportPanel({ return match?.id ?? null } + // Trykk på "Lagre" i redigeringstabellen -- sjekker FØRST om noen av + // radene ser ut som mulige duplikater (ingen e-post + navnetreff mot en + // spiller som allerede finnes i poolen). Finnes det slike, stopper vi + // her og spør organisator eksplisitt (DuplicateStep) i stedet for å + // stille velge -- se DuplicateCandidate-kommentaren over. Ellers går vi + // rett til doSave(). async function handleSave() { const validRows = rows.filter((r) => r.display_name.trim().length > 0) if (validRows.length === 0) { setError("Ingen rader med navn å lagre.") return } - setStep("saving") setError(null) + + const noEmailRows = validRows.filter((r) => !r.email.trim()) + if (noEmailRows.length > 0) { + const existingRes = await fetch(`/orgs/${organizationId}/players`, { credentials: "include" }) + if (existingRes.ok) { + const existingPlayers: ApiPlayer[] = await existingRes.json() + const byName = new Map() + for (const p of existingPlayers) byName.set(p.display_name.trim().toLowerCase(), p) + const candidates: DuplicateCandidate[] = noEmailRows + .map((row) => { + const existing = byName.get(row.display_name.trim().toLowerCase()) + return existing ? { row, existing, action: "merge" as DuplicateAction } : null + }) + .filter((c): c is DuplicateCandidate => c !== null) + if (candidates.length > 0) { + setDuplicates(candidates) + setStep("duplicates") + return + } + } + } + await doSave(validRows, []) + } + + async function doSave(validRows: DraftRow[], resolvedDuplicates: DuplicateCandidate[]) { + setStep("saving") setSaveWarnings([]) try { - const payload = validRows.map((r) => ({ - display_name: r.display_name.trim(), - email: r.email.trim() || null, - handicap_index: r.handicap_index.trim() ? Number(r.handicap_index) : null, - gender: toBackendGender(r.gender), - birth_date: r.birth_date.trim() || null, - mobile: r.mobile.trim() || null, - club: r.club.trim() || null, - nickname: r.nickname.trim() || null, - country: r.country.trim() || null, - club_member_number: r.club_member_number.trim() || null, - first_name: r.first_name.trim() || null, - last_name: r.last_name.trim() || null, - comment: r.comment.trim() || null, - })) + const resolutionByRowKey = new Map(resolvedDuplicates.map((d) => [d.row.key, d])) + const payload = validRows.map((r) => { + const resolution = resolutionByRowKey.get(r.key) + return { + data: { + display_name: r.display_name.trim(), + email: r.email.trim() || null, + handicap_index: r.handicap_index.trim() ? Number(r.handicap_index) : null, + gender: toBackendGender(r.gender), + birth_date: r.birth_date.trim() || null, + mobile: r.mobile.trim() || null, + club: r.club.trim() || null, + nickname: r.nickname.trim() || null, + country: r.country.trim() || null, + club_member_number: r.club_member_number.trim() || null, + first_name: r.first_name.trim() || null, + last_name: r.last_name.trim() || null, + comment: r.comment.trim() || null, + }, + match_player_id: resolution && resolution.action !== "create_new" ? resolution.existing.id : null, + duplicate_action: resolution?.action ?? null, + } + }) const bulkRes = await fetch(`/orgs/${organizationId}/players/bulk`, { method: "POST", headers: { "Content-Type": "application/json" }, @@ -313,6 +368,19 @@ export function PlayerImportPanel({ } } + if (step === "duplicates") { + return ( + + setDuplicates((prev) => prev.map((d) => (d.row.key === rowKey ? { ...d, action } : d))) + } + onCancel={() => setStep("edit")} + onContinue={() => void doSave(rows.filter((r) => r.display_name.trim().length > 0), duplicates)} + /> + ) + } + return ( ) } + +// Duplikat-varsel (2026-08-18, se DuplicateCandidate-kommentaren over) -- +// funksjonell, håndkodet av Claude (ikke V0-eksportert som resten av +// import-visningen): en liten, avgrenset spørre-skjerm satt inn FØR selve +// lagringen, ikke en ny selvstendig UI-flate. Bruker de samme globale +// design-tokenene (border/bg-card/bg-primary osv.) som resten av appen. +function DuplicateStep({ + duplicates, + onChangeAction, + onCancel, + onContinue, +}: { + duplicates: DuplicateCandidate[] + onChangeAction: (rowKey: string, action: DuplicateAction) => void + onCancel: () => void + onContinue: () => void +}) { + const ACTIONS: { value: DuplicateAction; label: string; hint: string }[] = [ + { value: "merge", label: "Slå sammen", hint: "Fyll kun inn tomme felt på den eksisterende spilleren" }, + { value: "overwrite", label: "Overskriv", hint: "Erstatt ALLE felt på den eksisterende spilleren" }, + { value: "create_new", label: "Importer som ny", hint: "Behold begge -- bevisst dobbeloppføring" }, + ] + + return ( +
+
+
+ +
    + {duplicates.map((d) => ( +
  • +

    {d.row.display_name.trim()}

    +

    + Finnes allerede i spillerpoolen + {d.existing.club ? ` -- ${d.existing.club}` : ""} + {d.existing.handicap_index != null ? `, HCP ${d.existing.handicap_index}` : ""} +

    +
    + Hva skal skje med {d.row.display_name.trim()}? + {ACTIONS.map((a) => { + const id = `dup-${d.row.key}-${a.value}` + const checked = d.action === a.value + return ( + + ) + })} +
    +
    • + ))} +
+ +
+ + +
+
+ ) +} diff --git a/tests/test_players_bulk.py b/tests/test_players_bulk.py index 9ae255c..27f6a23 100644 --- a/tests/test_players_bulk.py +++ b/tests/test_players_bulk.py @@ -3,19 +3,33 @@ Massimport av spillere (`POST /orgs/{id}/players/bulk`, se FEATURE_ BACKLOG.md "Massimport av spillere til organisasjon/turnering"). Samme dedup-mønster som registration.py sin selvregistrering (ADR-017 Beslutning B), utvidet til alle PlayerCreate-felt. + +`match_player_id`/`duplicate_action` (2026-08-18) -- ekte produksjonsbug: +rader uten e-post har ingen matchingsnøkkel, så reimport av samme CSV +dublerte dem i stedet for å oppdatere. Frontend oppdager nå slike mulige +duplikater selv (navnetreff, ingen e-post) og spør organisator eksplisitt +-- svaret sendes hit som disse to feltene per rad. """ import app.db as app_db -from app.routers.players import PlayerCreate, create_players_bulk +from app.routers.players import PlayerBulkRow, PlayerCreate, create_players_bulk from tests.conftest import create_org, create_player +def _row(**kwargs) -> PlayerBulkRow: + match_player_id = kwargs.pop("match_player_id", None) + duplicate_action = kwargs.pop("duplicate_action", None) + return PlayerBulkRow( + data=PlayerCreate(**kwargs), match_player_id=match_player_id, duplicate_action=duplicate_action + ) + + async def test_bulk_creates_new_players_without_email(pool): org_id = await create_org() rows = [ - PlayerCreate(display_name="Ny Spiller A"), - PlayerCreate(display_name="Ny Spiller B", handicap_index=12.5), + _row(display_name="Ny Spiller A"), + _row(display_name="Ny Spiller B", handicap_index=12.5), ] results = await create_players_bulk(rows, organization_id=org_id) assert len(results) == 2 @@ -35,7 +49,7 @@ async def test_bulk_matches_existing_player_by_email_and_fills_only_empty_fields ) rows = [ - PlayerCreate( + _row( display_name="Annet Navn Fra CSV", # skal IKKE overskrive display_name email="match@example.com", country="Sverige", # skal IKKE overskrive et allerede satt felt @@ -55,10 +69,14 @@ async def test_bulk_matches_existing_player_by_email_and_fills_only_empty_fields async def test_bulk_without_email_never_matches_existing_and_always_creates(pool): + """Uendret standardoppførsel når INGEN duplikat-varsel er besvart + (match_player_id ikke satt) -- ren e-postløs rad oppretter fortsatt + alltid en ny rad, akkurat som før denne funksjonen fikk + duplikat-håndtering.""" org_id = await create_org() await create_player(org_id, display_name="Har Ikke E-post") - rows = [PlayerCreate(display_name="Har Ikke E-post")] + rows = [_row(display_name="Har Ikke E-post")] results = await create_players_bulk(rows, organization_id=org_id) assert results[0].status == "created" @@ -77,7 +95,7 @@ async def test_bulk_paid_flag_never_overwritten_on_existing_match(pool): "UPDATE player SET email = 'paid@example.com', paid = true WHERE id = $1", existing_id ) - rows = [PlayerCreate(display_name="X", email="paid@example.com", paid=False)] + rows = [_row(display_name="X", email="paid@example.com", paid=False)] results = await create_players_bulk(rows, organization_id=org_id) assert results[0].player.paid is True # uendret, COALESCE mot ikke-nullbar kolonne er en no-op @@ -87,7 +105,81 @@ async def test_bulk_is_transactional_across_rows(pool): partial-suksess-forventning her, men bekreft at flere rader i samme kall faktisk lander i samme, konsistente organisasjon.""" org_id = await create_org() - rows = [PlayerCreate(display_name=f"Spiller {i}") for i in range(5)] + rows = [_row(display_name=f"Spiller {i}") for i in range(5)] results = await create_players_bulk(rows, organization_id=org_id) assert len(results) == 5 assert len({r.player.id for r in results}) == 5 + + +async def test_bulk_duplicate_merge_fills_only_empty_fields_like_email_match(pool): + """Organisator svarte "Slå sammen" på et duplikat-varsel -- COALESCE- + oppførsel, identisk til e-post-matchingen, men rettet mot en + eksplisitt oppgitt match_player_id i stedet.""" + org_id = await create_org() + existing_id = await create_player(org_id, display_name="Anders Sperre") + async with app_db.org_connection(org_id) as conn: + await conn.execute("UPDATE player SET club = 'Gamle Klubb' WHERE id = $1", existing_id) + + rows = [ + _row( + display_name="Anders Sperre", + club="Ny Klubb", # skal IKKE overskrive (allerede satt) + handicap_index=14.0, # skal FYLLES INN (var tomt) + match_player_id=existing_id, + duplicate_action="merge", + ) + ] + results = await create_players_bulk(rows, organization_id=org_id) + assert results[0].status == "updated" + assert results[0].player.id == existing_id + assert results[0].player.club == "Gamle Klubb" # uendret + assert results[0].player.handicap_index == 14.0 # fylt inn + + async with app_db.org_connection(org_id) as conn: + count = await conn.fetchval("SELECT count(*) FROM player WHERE display_name = 'Anders Sperre'") + assert count == 1 # ingen ny duplikat opprettet + + +async def test_bulk_duplicate_overwrite_replaces_already_set_fields(pool): + """Organisator svarte "Overskriv" -- eneste sti i endepunktet som + erstatter et allerede satt felt, og kun for akkurat denne raden.""" + org_id = await create_org() + existing_id = await create_player(org_id, display_name="Anders Sperre") + async with app_db.org_connection(org_id) as conn: + await conn.execute("UPDATE player SET club = 'Gamle Klubb' WHERE id = $1", existing_id) + + rows = [ + _row( + display_name="Anders Sperre", + club="Ny Klubb", + match_player_id=existing_id, + duplicate_action="overwrite", + ) + ] + results = await create_players_bulk(rows, organization_id=org_id) + assert results[0].status == "overwritten" + assert results[0].player.id == existing_id + assert results[0].player.club == "Ny Klubb" # overskrevet, ikke bevart + + +async def test_bulk_duplicate_create_new_still_creates_a_second_row(pool): + """Organisator svarte "Importer som ny" (bevisst duplikat) -- + match_player_id kan følge med, men skal IGNORERES når + duplicate_action er create_new.""" + org_id = await create_org() + existing_id = await create_player(org_id, display_name="Anders Sperre") + + rows = [ + _row( + display_name="Anders Sperre", + match_player_id=existing_id, + duplicate_action="create_new", + ) + ] + results = await create_players_bulk(rows, organization_id=org_id) + assert results[0].status == "created" + assert results[0].player.id != existing_id + + async with app_db.org_connection(org_id) as conn: + count = await conn.fetchval("SELECT count(*) FROM player WHERE display_name = 'Anders Sperre'") + assert count == 2