diff --git a/bgg-shelf-pipeline-spec.md b/bgg-shelf-pipeline-spec.md index 013096d..1ceeaab 100644 --- a/bgg-shelf-pipeline-spec.md +++ b/bgg-shelf-pipeline-spec.md @@ -60,6 +60,7 @@ Each stage reads the previous stage's artifact and writes its own. Re-running a - `unmatched` — nothing plausible found. - Expansions: BGG returns `boardgameexpansion` as a distinct type. Keep them — I own expansions and want them in the collection — but tag them so review can catch base-game/expansion confusion (a spine reading "Wingspan Europe" must not match base Wingspan). - Cache all BGG responses on disk (keyed by query/ID) so re-runs don't re-hit the API. +- **Post-resolve dedupe**: rows resolving to the same (bgg_id, version_id — or both version-unknown) are the same physical game read twice (typo, partial spine) unless their extraction cues conflict (two editions). Losers get `match_status=merged` + a `merged_into` column pointing at the survivor — rows never silently disappear, the survivor keeps the combined photo provenance downstream, and review surfaces every merge with a veto that restores the row as a distinct approved match. - **Version resolution**: once a game ID is settled (auto or approved), fetch `/thing?id=&versions=1` and score the version list against the extraction's edition cues (publisher, year, language, edition wording). Same three-way classification: a single clear winner is `version_auto`; multiple plausible → `version_ambiguous` (goes to review); no cues at all → `version_unknown` (acceptable — BGG allows collection entries with no version set, and guessing wrong is worse than leaving it blank). - Output: `matches.csv` with columns: `title_raw, bgg_id, bgg_name, year, type, match_status (auto|ambiguous|unmatched|approved|rejected), version_id, version_name, version_status (version_auto|version_ambiguous|version_unknown|version_approved), candidates_json, version_candidates_json, source_photos`. diff --git a/src/bggpipe/diff.py b/src/bggpipe/diff.py index 8eb74c5..d62135f 100644 --- a/src/bggpipe/diff.py +++ b/src/bggpipe/diff.py @@ -54,6 +54,7 @@ class DiffResult: unseen: list[CollectionItem] = field(default_factory=list) pending: list[str] = field(default_factory=list) # ambiguous/unmatched titles rejected: int = 0 + merged: int = 0 recognized: int = 0 @@ -86,11 +87,22 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu seen_object_ids: set[int] = set() consumed_collids: set[int] = set() + # photos of merged-away duplicate reads belong to their survivor + merged_photos: dict[str, set[str]] = {} + for row in rows: + if row["match_status"] == "merged" and row.get("merged_into"): + merged_photos.setdefault(row["merged_into"], set()).update( + p for p in row["source_photos"].split(";") if p + ) + for row in rows: status = row["match_status"] if status == "rejected": result.rejected += 1 continue + if status == "merged": + result.merged += 1 # represented by its survivor row + continue if status not in ("auto", "approved") or not row["bgg_id"]: result.pending.append(row["title_raw"]) continue @@ -105,6 +117,8 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu version_id = int(row["version_id"]) if confident else None if not copies: + photos = {p for p in row["source_photos"].split(";") if p} + photos |= merged_photos.get(row["title_raw"], set()) result.to_add.append( { "bgg_id": row["bgg_id"], @@ -114,7 +128,7 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu "version_id": row["version_id"] if confident else "", "version_name": row["version_name"] if confident else "", "title_raw": row["title_raw"], - "source_photos": row["source_photos"], + "source_photos": ";".join(sorted(photos)), } ) continue @@ -191,11 +205,12 @@ def run_diff(cfg: Config, *, client: BGGClient | None = None) -> DiffResult: _write_csv(cfg.data_dir / "to_add.csv", TO_ADD_COLUMNS, result.to_add) _write_csv(cfg.data_dir / "to_update.csv", TO_UPDATE_COLUMNS, result.to_update) + merged_note = f" · {result.merged} merged duplicate(s)" if result.merged else "" typer.echo( f"\n{result.recognized} recognized · {len(result.already_owned)} already " f"owned · {len(result.to_add)} to add · {len(result.to_update)} version " f"update(s) · {len(result.pending)} pending review · " - f"{result.rejected} rejected" + f"{result.rejected} rejected{merged_note}" ) if result.disagreements: typer.echo("\nVersion disagreements (left untouched):") diff --git a/src/bggpipe/resolve.py b/src/bggpipe/resolve.py index 1c92cd9..8e30b10 100644 --- a/src/bggpipe/resolve.py +++ b/src/bggpipe/resolve.py @@ -20,6 +20,7 @@ from rapidfuzz import fuzz from bggpipe.bgg_client import BGGAuthError, BGGClient from bggpipe.config import Config +from bggpipe.extract import _cues_conflict from bggpipe.models import GameVersion from bggpipe.normalize import normalize_title @@ -43,6 +44,7 @@ MATCH_COLUMNS = [ "candidates_json", "version_candidates_json", "source_photos", + "merged_into", ] @@ -370,11 +372,86 @@ def _row_key(title_raw: str, source_photos: str) -> tuple[str, str]: return (title_raw, source_photos) +@dataclass(frozen=True) +class MergeEvent: + loser_title: str + survivor_title: str + bgg_name: str + bgg_id: str + + +def dedupe_matches(rows: list[dict], titles: list[TitleEntry]) -> list[MergeEvent]: + """Post-resolve dedupe: rows resolving to the same (bgg_id, version_id — + or both version-unknown) are the same physical game seen twice (a typo + read, a partial spine) UNLESS their extraction cues conflict, which + means two editions. Losers are marked match_status="merged" pointing at + the survivor via merged_into — no row is ever deleted, and the review + UI can veto the merge.""" + entry_by_key = {(e.title_raw, ";".join(e.source_photos)): e for e in titles} + entry_by_title: dict[str, TitleEntry] = {} + for e in titles: + entry_by_title.setdefault(e.title_raw, e) + + def cues(row: dict) -> dict: + entry = entry_by_key.get( + (row["title_raw"], row["source_photos"]) + ) or entry_by_title.get(row["title_raw"]) + if entry is None: + return {} + return { + "publisher_hint": entry.publisher_hint, + "edition_hint": entry.edition_hint, + "language_hint": entry.language_hint, + "year_hint": entry.year_hint, + } + + groups: dict[tuple[str, str], list[dict]] = {} + for row in rows: + if row["match_status"] not in ("auto", "approved") or not row["bgg_id"]: + continue + confident = ( + row["version_status"] in ("version_auto", "version_approved") + and row["version_id"] + ) + key = (row["bgg_id"], row["version_id"] if confident else "") + groups.setdefault(key, []).append(row) + + events: list[MergeEvent] = [] + for (bgg_id, _version), group in groups.items(): + if len(group) < 2: + continue + # survivor: the row whose transcription best matches the BGG name + group = sorted( + group, + key=lambda r: ( + normalize_title(r["title_raw"]) != normalize_title(r["bgg_name"] or "") + ), + ) + survivor = group[0] + for loser in group[1:]: + if _cues_conflict(cues(survivor), cues(loser)): + continue # conflicting edition cues: genuinely two copies + loser["match_status"] = "merged" + loser["merged_into"] = survivor["title_raw"] + events.append( + MergeEvent( + loser_title=loser["title_raw"], + survivor_title=survivor["title_raw"], + bgg_name=survivor["bgg_name"], + bgg_id=bgg_id, + ) + ) + return events + + def read_matches(path: Path) -> list[dict[str, str]]: if not path.exists(): return [] with path.open(newline="") as f: - return list(csv.DictReader(f)) + rows = list(csv.DictReader(f)) + for row in rows: # files written before the merged_into column existed + row.setdefault("merged_into", "") + return rows def write_matches(path: Path, rows: list[dict[str, str]]) -> None: @@ -382,7 +459,9 @@ def write_matches(path: Path, rows: list[dict[str, str]]) -> None: path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_name(path.name + ".tmp") with tmp.open("w", newline="") as f: - writer = csv.DictWriter(f, fieldnames=MATCH_COLUMNS, extrasaction="ignore") + writer = csv.DictWriter( + f, fieldnames=MATCH_COLUMNS, extrasaction="ignore", restval="" + ) writer.writeheader() writer.writerows(rows) os.replace(tmp, path) @@ -438,6 +517,17 @@ def run_resolve( append_rows(cfg.matches_path, new_rows) + all_rows = read_matches(cfg.matches_path) + merges = dedupe_matches(all_rows, entries) + if merges: + write_matches(cfg.matches_path, all_rows) + typer.echo("") + for m in merges: + typer.echo( + f" merged {m.loser_title!r} into {m.survivor_title!r} — same " + f"game ({m.bgg_name}, {m.bgg_id}); veto in review if wrong" + ) + counts: dict[str, int] = {} for row in new_rows: counts[row.match_status] = counts.get(row.match_status, 0) + 1 diff --git a/src/bggpipe/review.py b/src/bggpipe/review.py index 8b7bec0..30b78f1 100644 --- a/src/bggpipe/review.py +++ b/src/bggpipe/review.py @@ -116,7 +116,22 @@ class ReviewSession: return [r for r in self.rows if r["match_status"] in ("ambiguous", "unmatched")] def version_rows(self) -> list[dict]: - return [r for r in self.rows if r["version_status"] == "version_ambiguous"] + return [ + r + for r in self.rows + if r["version_status"] == "version_ambiguous" + and r["match_status"] != "merged" + ] + + def merged_rows(self) -> list[dict]: + return [r for r in self.rows if r["match_status"] == "merged"] + + def veto_merge(self, row: dict) -> None: + """The human says these are NOT the same physical game: restore the + row as a distinct, human-confirmed match.""" + row["match_status"] = "approved" + row["merged_into"] = "" + self._save() def cues_for(self, title_raw: str): return self._titles.get(title_raw) diff --git a/src/bggpipe/templates/review.html b/src/bggpipe/templates/review.html index 7f9a702..3a8b9bf 100644 --- a/src/bggpipe/templates/review.html +++ b/src/bggpipe/templates/review.html @@ -196,6 +196,18 @@ .chip.wait { background: #f3e7cd; color: var(--brass-deep); } .chip.no { background: #f0ddd8; color: var(--reject); } .chip.open { background: #e4e4ef; color: #4c4c78; } + .chip.merged { background: #e2e8e4; color: var(--felt-deep); } + + /* merge notices: slim, undoable */ + .card.merge { padding: .55rem 1rem; align-items: center; } + .card.merge .body { display: flex; align-items: center; gap: .8rem; flex-wrap: wrap; } + .card.merge .arrow { color: var(--ink-soft); } + .card.merge button { + font: inherit; font-size: .8rem; margin-left: auto; + background: none; border: 1px solid var(--paper-edge); + border-radius: 6px; padding: .2rem .6rem; cursor: pointer; + } + .card.merge button:hover { border-color: var(--reject); color: var(--reject); } @media (max-width: 700px) { .card, .ticket { flex-direction: column; } .shots { flex-basis: auto; } @@ -209,7 +221,7 @@ j/k move · 1–9 pick · r reject · m manual id · u unknown · - d dismiss + v veto merge · d dismiss
@@ -341,6 +353,7 @@ function render() { `${s.pending.length} matches ${s.versions.length} editions ${s.unidentified.length} reshoot + ${s.merges.length ? `${s.merges.length} merged` : ""} ${s.summary.unresolved ? `${s.summary.unresolved} awaiting resolve` : ""} ${s.decisions} decided this sitting`; @@ -374,6 +387,20 @@ function render() { html += `

Editions — optional pass, never blocks uploads

`; html += s.versions.map(versionCard).join(""); } + if (s.merges.length) { + html += `

Merges — duplicate reads folded into one game; veto if wrong

`; + html += s.merges.map(mg => ` +
+
+ ${esc(mg.title_raw)} + merged into + ${esc(mg.merged_into)} + ${esc(mg.bgg_name)} · ${esc(mg.bgg_id)} + +
+
`).join(""); + } if (s.unidentified.length) { html += `

Reshoot — boxes seen but not identified

`; html += s.unidentified.map(ticket).join(""); @@ -383,6 +410,7 @@ function render() { if (c.status === "awaiting_resolve") return `awaiting BGG`; if (c.status === "auto" || c.status === "approved") return `${c.status}`; if (c.status === "rejected") return `rejected`; + if (c.status === "merged") return `merged → ${esc(c.merged_into)}`; return `${esc(c.status)}`; }; html += `

Catalog — every title extracted so far (${s.catalog.length})

@@ -417,6 +445,8 @@ function render() { version(b.closest(".card"), "unknown")); m.querySelectorAll(".ticket button").forEach(b => b.onclick = () => dismiss(b.closest(".ticket"))); + m.querySelectorAll(".veto").forEach(b => b.onclick = () => + vetoMerge(b.closest(".card"))); m.querySelectorAll(".rowactions input").forEach(inp => { inp.onkeydown = e => { if (e.key === "Enter" && inp.value.trim().match(/^\d+$/)) { @@ -448,6 +478,9 @@ const dismiss = t => post("/api/dismiss", { photo: t.dataset.photo, location: t.dataset.location, partial_text: t.dataset.partial, art_notes: t.dataset.art, }); +const vetoMerge = card => post("/api/veto-merge", { + title_raw: card.dataset.title, source_photos: card.dataset.photos, +}); document.addEventListener("keydown", e => { if (e.target.tagName === "INPUT") return; @@ -468,6 +501,7 @@ document.addEventListener("keydown", e => { else if (e.key === "r" && kind === "match") decide(card, "reject"); else if (e.key === "u" && kind === "version") version(card, "unknown"); else if (e.key === "d" && kind === "ticket") dismiss(card); + else if (e.key === "v" && kind === "merge") vetoMerge(card); else if (e.key === "m" && kind === "match") { card.querySelector("input")?.focus(); e.preventDefault(); } }); diff --git a/src/bggpipe/webreview.py b/src/bggpipe/webreview.py index 3dc87dd..9e95279 100644 --- a/src/bggpipe/webreview.py +++ b/src/bggpipe/webreview.py @@ -90,6 +90,11 @@ class VersionBody(BaseModel): version_id: int | None = None +class VetoBody(BaseModel): + title_raw: str + source_photos: str + + class DismissBody(BaseModel): photo: str location: str = "" @@ -168,6 +173,7 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: "bgg_id": row["bgg_id"] if row else "", "bgg_name": row["bgg_name"] if row else "", "version_name": row["version_name"] if row else "", + "merged_into": row.get("merged_into", "") if row else "", } ) unresolved_count = sum( @@ -189,9 +195,20 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: sightings.append( {**s, "photo": photo, "photo_exists": photo in available} ) + merges = [ + { + "title_raw": r["title_raw"], + "source_photos": r["source_photos"], + "merged_into": r.get("merged_into", ""), + "bgg_name": r["bgg_name"], + "bgg_id": r["bgg_id"], + } + for r in session.merged_rows() + ] return { "pending": [row_payload(r) for r in session.pending_rows()], "versions": [version_payload(r) for r in session.version_rows()], + "merges": merges, "unidentified": sightings, "catalog": catalog, "decisions": session.decisions, @@ -250,6 +267,14 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: raise HTTPException(400, f"unknown action {body.action!r}") return state() + @app.post("/api/veto-merge") + def api_veto_merge(body: VetoBody) -> dict: + row = find_row(body.title_raw, body.source_photos) + if row["match_status"] != "merged": + raise HTTPException(400, "row is not merged") + session.veto_merge(row) + return state() + @app.post("/api/dismiss") def api_dismiss(body: DismissBody) -> dict: dismissed.add(_sighting_key(body.photo, body.model_dump(exclude={"photo"}))) diff --git a/tests/test_diff.py b/tests/test_diff.py index 9696937..7f4c28d 100644 --- a/tests/test_diff.py +++ b/tests/test_diff.py @@ -164,3 +164,17 @@ def test_pending_rejected_and_unseen_are_reported(): assert result.rejected == 1 assert [c.object_id for c in result.unseen] == [9209] # informational assert result.recognized == 1 + + +def test_merged_rows_are_skipped_but_photos_carry_to_survivor(): + matches = [ + _match("Joking Hazard", "193621"), + {**_match("Jokin Ha...", "193621", status="merged"), + "merged_into": "Joking Hazard", "source_photos": "other.jpg"}, + ] + result = compute_diff(matches, []) # empty collection -> to_add + assert result.merged == 1 + assert result.pending == [] # merged is not "needs review" + (row,) = result.to_add + assert row["title_raw"] == "Joking Hazard" + assert row["source_photos"] == "other.jpg;x.jpg" # combined diff --git a/tests/test_resolve.py b/tests/test_resolve.py index 4c9f897..e68cdec 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -343,3 +343,142 @@ def test_run_resolve_saves_progress_when_token_missing(tmp_path): ) rows2 = run_resolve(cfg, client=full) assert [r.title_raw for r in rows2] == ["Wingspan"] + + +# -- post-resolve dedupe ------------------------------------------------ + +from bggpipe.bgg_client import cache_key as _cache_key # noqa: E402 +from bggpipe.resolve import dedupe_matches # noqa: E402 + + +def _mrow( + title, bgg_id, photos, name="Joking Hazard", + vstatus="version_unknown", vid="", status="auto", +): + return { + "title_raw": title, + "bgg_id": bgg_id, + "bgg_name": name, + "year": "2016", + "type": "boardgame", + "match_status": status, + "version_id": vid, + "version_name": "", + "version_status": vstatus, + "candidates_json": "[]", + "version_candidates_json": "[]", + "source_photos": photos, + "merged_into": "", + } + + +def _tentry(title, photos, **cues): + return TitleEntry( + title_raw=title, + title_normalized=normalize_title(title), + publisher_hint=cues.get("publisher_hint", ""), + edition_hint=cues.get("edition_hint", ""), + year_hint=cues.get("year_hint"), + language_hint=cues.get("language_hint", ""), + source_photos=tuple(photos), + ) + + +def test_dedupe_merges_typo_read_into_canonical(): + rows = [ + _mrow("Jokin Ha...", "193621", "a.jpg"), + _mrow("Joking Hazard", "193621", "b.jpg;c.jpg"), + _mrow("Catan", "13", "d.jpg", name="CATAN"), + ] + events = dedupe_matches(rows, []) + (event,) = events + assert event.loser_title == "Jokin Ha..." + assert event.survivor_title == "Joking Hazard" # name-matching read survives + by_title = {r["title_raw"]: r for r in rows} + assert by_title["Jokin Ha..."]["match_status"] == "merged" + assert by_title["Jokin Ha..."]["merged_into"] == "Joking Hazard" + assert by_title["Joking Hazard"]["match_status"] == "auto" # untouched + assert by_title["Catan"]["match_status"] == "auto" + assert len(rows) == 3 # nothing disappears + + +def test_dedupe_respects_conflicting_edition_cues(): + rows = [ + _mrow("Cosmic Encounter", "40529", "a.jpg", name="Cosmic Encounter"), + _mrow("COSMIC ENCOUNTER", "40529", "b.jpg", name="Cosmic Encounter"), + ] + titles = [ + _tentry("Cosmic Encounter", ["a.jpg"], edition_hint="42nd Anniversary Edition"), + _tentry("COSMIC ENCOUNTER", ["b.jpg"], edition_hint="Eon 1977 edition"), + ] + assert dedupe_matches(rows, titles) == [] + assert all(r["match_status"] == "auto" for r in rows) + + +def test_dedupe_versions_must_agree(): + # different confident versions: two physical editions, never merged + rows = [ + _mrow("Wingspan", "266192", "a.jpg", vstatus="version_auto", vid="465063"), + _mrow("WINGSPAN", "266192", "b.jpg", vstatus="version_auto", vid="521212"), + ] + assert dedupe_matches(rows, []) == [] + # same confident version: same box seen twice + rows2 = [ + _mrow("Wingspan", "266192", "a.jpg", vstatus="version_auto", vid="465063"), + _mrow("WINGSPAN", "266192", "b.jpg", vstatus="version_auto", vid="465063"), + ] + assert len(dedupe_matches(rows2, [])) == 1 + # confident version vs unknown: conservative, no merge + rows3 = [ + _mrow("Wingspan", "266192", "a.jpg", vstatus="version_auto", vid="465063"), + _mrow("WINGSPAN", "266192", "b.jpg"), + ] + assert dedupe_matches(rows3, []) == [] + + +def test_dedupe_is_idempotent_and_skips_merged(): + rows = [ + _mrow("Jokin Ha...", "193621", "a.jpg"), + _mrow("Joking Hazard", "193621", "b.jpg"), + ] + assert len(dedupe_matches(rows, [])) == 1 + assert dedupe_matches(rows, []) == [] # second pass: nothing new + + +def test_run_resolve_dedupes_and_keeps_all_rows(tmp_path): + from bggpipe.resolve import write_matches as _wm # noqa: F401 + + cache = tmp_path / "cache" + cache.mkdir() + wingspan_xml = ( + '' + '' + "" + ) + for query in ("Wingspan", "WINGSPAN!"): + key = _cache_key( + "search", {"query": query, "type": "boardgame,boardgameexpansion"} + ) + (cache / key).write_text(wingspan_xml) + + data_dir = tmp_path / "data" + data_dir.mkdir() + (data_dir / "titles.json").write_text( + json.dumps( + [ + {"title_raw": "Wingspan", "source_photos": ["a.jpg"]}, + {"title_raw": "WINGSPAN!", "source_photos": ["b.jpg"]}, # variant read + ] + ) + ) + cfg = Config(data_dir=data_dir) + client = BGGClient(cache_dir=cache, transport=httpx.MockTransport(_no_network)) + + run_resolve(cfg, client=client) + from bggpipe.resolve import read_matches as _rm + + saved = {r["title_raw"]: r for r in _rm(cfg.matches_path)} + assert len(saved) == 2 # no row disappeared + assert saved["Wingspan"]["match_status"] == "auto" + assert saved["WINGSPAN!"]["match_status"] == "merged" + assert saved["WINGSPAN!"]["merged_into"] == "Wingspan" diff --git a/tests/test_webreview.py b/tests/test_webreview.py index 492f849..3b544d6 100644 --- a/tests/test_webreview.py +++ b/tests/test_webreview.py @@ -286,3 +286,41 @@ def test_catalog_and_unresolved_backlog_are_visible(tmp_path): by_title = {c["title_raw"]: c for c in state["catalog"]} assert by_title["Fresh Off The Shelf"]["status"] == "awaiting_resolve" assert by_title["Citadels"]["status"] == "ambiguous" + + +def test_merge_veto_roundtrip(tmp_path): + cfg = make_cfg(tmp_path) + rows = read_matches(cfg.matches_path) + rows.append( + _row( + title_raw="Jokin Ha...", + match_status="merged", + bgg_id="193621", + bgg_name="Joking Hazard", + source_photos="shelf.jpg", + ) + ) + rows[-1]["merged_into"] = "Joking Hazard" + write_matches(cfg.matches_path, rows) + web = TestClient(create_app(cfg, client=unauthorized_client(tmp_path))) + + state = web.get("/api/state").json() + (merge,) = state["merges"] + assert merge["title_raw"] == "Jokin Ha..." + assert merge["merged_into"] == "Joking Hazard" + + state = web.post( + "/api/veto-merge", + json={"title_raw": "Jokin Ha...", "source_photos": "shelf.jpg"}, + ).json() + assert state["merges"] == [] + saved = {r["title_raw"]: r for r in read_matches(cfg.matches_path)} + assert saved["Jokin Ha..."]["match_status"] == "approved" + assert saved["Jokin Ha..."]["merged_into"] == "" + + # veto on a non-merged row is refused + bad = web.post( + "/api/veto-merge", + json={"title_raw": "Jokin Ha...", "source_photos": "shelf.jpg"}, + ) + assert bad.status_code == 400