From ba55863cef3ae1674b97626b4c199f640b79c12b Mon Sep 17 00:00:00 2001
From: Eric Wagoner <ewagoner@gmail.com>
Date: Wed, 5 Aug 2026 22:36:48 -0400
Subject: [PATCH] Version picker: real pagination, and matching that declines
 to guess
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Five games uploaded; two landed version-less. Neither was the picker's
fault: paging is an AngularJS <ul class="pagination"> of anchors, not
buttons named "next", so the old guess found no control and quit after
page one — and BGG's API version names carry printing qualifiers the
picker omits ("English edition 2018-2" vs "(English edition) (2018)").

_select_version now scans the WHOLE list (verified selectors: rows are
<li>s with a thumbnail; a[title="Next Page"] advances; the parent <li>
disables at the end), collects every candidate, then decides: one exact
match wins; failing that, one match after stripping a trailing year
qualifier wins and says so; several matches are refused outright rather
than guessed, and the reason reaches upload_log.csv. Both call sites
carry the reason through.

docs/bgg-upload-flow.md records what the live site actually does —
including that every login-gate selector the doc called "verified" was
wrong, while the "unverified" dialog structure was right.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jXZFSTZQKzAC8fqpWSz9g
---
 data/upload_log.csv     |   5 ++
 docs/bgg-upload-flow.md |  36 ++++++++++++
 src/bggpipe/upload.py   | 114 +++++++++++++++++++++++++++++--------
 tests/test_upload.py    | 122 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 253 insertions(+), 24 deletions(-)

diff --git a/data/upload_log.csv b/data/upload_log.csv
index c4f96f6..820e676 100644
--- a/data/upload_log.csv
+++ b/data/upload_log.csv
@@ -2,3 +2,8 @@ action,bgg_id,collid,name,version_id,second_copy,status,timestamp,error
 add,334011,,A Gentle Rain,701315,,failed,2026-08-06T02:15:34+00:00,"TimeoutError: Locator.wait_for: Timeout 15000ms exceeded. Call log: - waiting for get_by_role(""dialog"").get_by_role(""heading"", name=re.compile(r""A\ Gentle\ Rain"", re.IGNORECASE)) to be visible"
 add,589,,Wiz-War,27352,,failed,2026-08-06T02:20:33+00:00,"Error: Locator.check: Error: strict mode violation: get_by_role(""dialog"").get_by_label(""Own"") resolved to 2 elements: 1) <input type=""checkbox"" ng-model=""item.status.own"" class=""ng-pristine ng-untouched ng-valid ng-empty""/> aka get_by_role(""checkbox"", name=""Own"", exact=True) 2) <input type=""checkbox"" ng-model=""item.status.prevowned"" class=""ng-pristine ng-untouched ng-valid ng-empty""/> aka get_by_role(""checkbox"", name=""Prev. Owned"") Call log: - waiting for get_by_role(""dialog"").get_by_label(""Own"")"
 add,334011,,A Gentle Rain,701315,,added,2026-08-06T02:21:32+00:00,
+add,125921,,Catan: Junior,476263,,added,2026-08-06T02:24:22+00:00,
+add,31260,,Agricola,297589,,added,2026-08-06T02:24:28+00:00,
+add,181304,,Mysterium,536289,,added_no_version,2026-08-06T02:24:34+00:00,version 'English edition 2018-2' not in picker; added without version
+add,312786,,Poetry for Neanderthals,514050,,added_no_version,2026-08-06T02:24:40+00:00,version 'English edition 2020' not in picker; added without version
+add,273240,,The Red Dragon Inn Smorgasbox,446069,,added,2026-08-06T02:24:46+00:00,
diff --git a/docs/bgg-upload-flow.md b/docs/bgg-upload-flow.md
index d5a329b..2f7b4dd 100644
--- a/docs/bgg-upload-flow.md
+++ b/docs/bgg-upload-flow.md
@@ -100,3 +100,39 @@ the existing one).
   page but did not overlay the form in the probe.
 - Logged-in detection heuristic (unverified): the header shows a "Sign In"
   link only when logged out.
+
+
+## Verified against the live site (2026-08-06, first real uploads)
+
+The add flow works end to end; every failure on the way was in code the
+earlier walkthrough had marked *verified*, and the parts marked
+*unverified* were mostly right. Corrections:
+
+- **Sign In is an `<a class="btn">` with no `href`.** It therefore has no
+  implicit `link` role: `get_by_role("link", name="Sign In")` matches
+  nothing in any state. The header also hydrates after
+  `domcontentloaded`, so for a moment neither Sign In nor Sign Out
+  exists — a check resting on one absence silently concludes "signed in"
+  and browses anonymously. Poll until one control or the other proves the
+  state; treat "neither, after 30s" as an error.
+- **`get_by_label("Own")` also matches "Prev. Owned."** Use
+  `get_by_role("checkbox", name="Own", exact=True)`.
+- **Version rows** are the `<li>`s carrying a thumbnail:
+  `li:has(.summary-item-thumbnail)`. Plain `listitem` also catches the
+  paging `<li>`s ("First", "Prev", "1", "…").
+- **Paging is an AngularJS `<ul class="pagination">` of anchors**, not
+  buttons: `a[title="Next Page"]`, `a[title="First Page"]`, with the
+  parent `<li>` gaining `disabled` at the end. The numbered anchors
+  render twice (mobile + desktop), so step with Next rather than
+  clicking a number. Paging is client-side over an already-loaded list —
+  no request per page.
+- **Row text is `<game name> (<version name>) (<year>)`**, and the game
+  name is localized (a Czech edition's row starts "Spící bohové"). Match
+  the version name inside its parentheses.
+- **The API's version name is not always the picker's string.** BGG's
+  XML gives e.g. `English edition 2018-2` where the picker shows
+  `(English edition) (2018)`. Match the full name first, then retry with
+  a trailing year/printing qualifier stripped — and if that relaxed match
+  hits more than one row, refuse and add version-less (never guess).
+- **An owned game's page has no "Add To" button**; it reads
+  "In Collections (Own…)". That is the update flow's entry point.
diff --git a/src/bggpipe/upload.py b/src/bggpipe/upload.py
index dd939ca..c36cb84 100644
--- a/src/bggpipe/upload.py
+++ b/src/bggpipe/upload.py
@@ -363,14 +363,40 @@ class PlaywrightUploader:
                     raise
         return dialog
 
-    def _select_version(self, dialog, version_name: str) -> bool:
-        """Page through the version sub-view matching the canonical version
-        NAME (the list has no search box). Returns False — with the sub-view
-        cancelled — when the name never shows up."""
+    # Verified 2026-08-06 against the live picker: version rows are the
+    # <li>s carrying a thumbnail (the paging <li>s are not), and paging is
+    # an AngularJS <ul class="pagination"> of anchors — NOT buttons named
+    # "next", which is why the old guess quit after page one.
+    _VERSION_ROWS = "li:has(.summary-item-thumbnail)"
+    _NEXT_PAGE = 'ul.pagination a[title="Next Page"]'
+    _FIRST_PAGE = 'ul.pagination a[title="First Page"]'
+    # Rows read "<game name> (<version name>) (<year>)", so the version name
+    # is matched inside its parentheses — bare substrings would let
+    # "English edition" match "English edition, second printing".
+    _TRAILING_YEAR = re.compile(r"\s+\d{4}(?:-\d+)?$")
+
+    def _rows_on_page(self, dialog) -> list[str]:
+        return [
+            " ".join(t.split())
+            for t in dialog.locator(self._VERSION_ROWS).all_text_contents()
+        ]
+
+    def _has_next_page(self, dialog) -> bool:
+        nxt = dialog.locator(self._NEXT_PAGE)
+        if nxt.count() == 0:
+            return False  # single-page list: the container is ng-show'd off
+        return "disabled" not in (
+            nxt.first.evaluate("e => e.closest('li').className") or ""
+        )
+
+    def _select_version(self, dialog, version_name: str) -> tuple[bool, str]:
+        """Pick the version whose name matches, paging the whole list first
+        so a decision is made against every candidate. Returns (picked,
+        reason); on failure the sub-view is cancelled and the caller adds
+        the game version-less — never a guessed edition (spec)."""
         dialog.get_by_role("button", name="Set version/edition").click()
-        pattern = re.compile(re.escape(version_name), re.I)
         try:
-            dialog.get_by_role("listitem").first.wait_for(timeout=15_000)
+            dialog.locator(self._VERSION_ROWS).first.wait_for(timeout=15_000)
         except self._timeout_error as err:
             # a version resolve found on BGG can't be missing from the
             # picker: an unrendered list means a slow page or changed markup
@@ -378,18 +404,26 @@ class PlaywrightUploader:
                 "version picker never rendered — site slow or markup "
                 "changed; attempt is retryable"
             ) from err
-        for _ in range(MAX_VERSION_PAGES):
-            items = dialog.get_by_role("listitem").filter(has_text=pattern)
-            if items.count():
-                items.first.click()
-                return True
-            # Pagination controls are UNVERIFIED (docs/bgg-upload-flow.md);
-            # best guess is a next-page button, stopping when absent/disabled.
-            nxt = dialog.get_by_role("button", name=re.compile("next|›|»", re.I)).first
-            if nxt.count() == 0 or nxt.is_disabled():
-                break  # genuine end of list: added_no_version is honest
-            nxt.click()
-            self._page.wait_for_timeout(2_000)  # etiquette: paginating hits BGG too
+
+        wanted = f"({version_name})".casefold()
+        # BGG's API name can carry a printing qualifier the picker omits
+        # ("English edition 2018-2" vs "(English edition) (2018)")
+        relaxed = f"({self._TRAILING_YEAR.sub('', version_name).strip()})".casefold()
+        exact: list[tuple[int, str]] = []
+        loose: list[tuple[int, str]] = []
+        pages = 0
+        for page_ix in range(MAX_VERSION_PAGES):
+            pages = page_ix + 1
+            for text in self._rows_on_page(dialog):
+                folded = text.casefold()
+                if wanted in folded:
+                    exact.append((page_ix, text))
+                elif relaxed != wanted and relaxed in folded:
+                    loose.append((page_ix, text))
+            if not self._has_next_page(dialog):
+                break
+            dialog.locator(self._NEXT_PAGE).first.click()
+            self._page.wait_for_timeout(400)  # client-side paging: no request
         else:
             # never saw the end of the list: "not in picker" would be a false
             # verdict frozen into DONE_STATUSES
@@ -397,10 +431,37 @@ class PlaywrightUploader:
                 f"hit MAX_VERSION_PAGES ({MAX_VERSION_PAGES}) without "
                 "finding the version or the end of the list — retryable"
             )
+
+        hits = exact or loose
+        if len(hits) == 1:
+            page_ix, text = hits[0]
+            self._goto_picker_page(dialog, page_ix)
+            dialog.locator(self._VERSION_ROWS).filter(
+                has_text=re.compile(re.escape(text), re.I)
+            ).first.click()
+            how = "" if exact else f" (matched loosely as {text!r})"
+            return True, how
+
         # Two-level dismissal: the sub-view has its own Cancel distinct from
         # the main dialog's.
         dialog.get_by_role("button", name="Cancel").first.click()
-        return False
+        if not hits:
+            return False, f"not offered in the picker ({pages} page(s) scanned)"
+        return False, (
+            f"{len(hits)} versions match {version_name!r} "
+            f"({', '.join(t for _, t in hits[:3])}…) — refusing to guess"
+        )
+
+    def _goto_picker_page(self, dialog, page_ix: int) -> None:
+        """Back to page 1, then forward — the numbered anchors render twice
+        (mobile + desktop variants), so stepping is the unambiguous route."""
+        first = dialog.locator(self._FIRST_PAGE)
+        if first.count():
+            first.first.click()
+            self._page.wait_for_timeout(400)
+        for _ in range(page_ix):
+            dialog.locator(self._NEXT_PAGE).first.click()
+            self._page.wait_for_timeout(400)
 
     def add_game(self, job: UploadJob) -> tuple[str, str]:
         self._ensure_logged_in()
@@ -417,9 +478,13 @@ class PlaywrightUploader:
         # match resolves to both checkboxes (strict-mode violation)
         dialog.get_by_role("checkbox", name="Own", exact=True).check()
         status, note = "added", ""
-        if job.version_name and not self._select_version(dialog, job.version_name):
-            status = "added_no_version"
-            note = f"version {job.version_name!r} not in picker; added without version"
+        if job.version_name:
+            picked, why = self._select_version(dialog, job.version_name)
+            if picked:
+                note = why.strip()
+            else:
+                status = "added_no_version"
+                note = f"version {job.version_name!r}: {why}; added without version"
         dialog.get_by_role("button", name="Save").click()
         # The dialog is hidden after save, not removed from the DOM.
         dialog.wait_for(state="hidden", timeout=15_000)
@@ -445,11 +510,12 @@ class PlaywrightUploader:
         )
         opener = row.get_by_role("link", name=re.compile("own", re.I)).first
         dialog = self._open_dialog(opener)
-        if not self._select_version(dialog, job.version_name):
+        picked, why = self._select_version(dialog, job.version_name)
+        if not picked:
             # Never guess a version: close without touching the entry.
             dialog.get_by_role("button", name="Cancel").first.click()
             raise RuntimeError(
-                f"version {job.version_name!r} not in picker — entry left untouched"
+                f"version {job.version_name!r}: {why} — entry left untouched"
             )
         dialog.get_by_role("button", name="Save").click()
         dialog.wait_for(state="hidden", timeout=15_000)
diff --git a/tests/test_upload.py b/tests/test_upload.py
index 7870fb7..698ae36 100644
--- a/tests/test_upload.py
+++ b/tests/test_upload.py
@@ -545,3 +545,125 @@ def test_undeterminable_state_raises_instead_of_guessing(monkeypatch):
     up = _uploader_with([{"out": 0, "in": 0}])
     with pytest.raises(LoginError, match="could not tell"):
         up._signed_out()
+
+
+# -- version picker: pagination + never-guess matching -------------------
+
+
+class _FakePicker:
+    """Minimal stand-in for the picker sub-view: pages of row texts, a
+    Next anchor that disables on the last page."""
+
+    def __init__(self, pages):
+        self.pages = pages
+        self.page = 0
+        self.clicked = None
+        self.cancelled = False
+
+    # -- locator plumbing the uploader uses ---------------------------
+    def locator(self, selector):
+        picker = self
+
+        class _Loc:
+            def __init__(self, texts=None):
+                self.texts = texts
+
+            def all_text_contents(self):
+                return picker.pages[picker.page]
+
+            def count(self):
+                if "pagination" in selector:
+                    return 1
+                return len(picker.pages[picker.page])
+
+            @property
+            def first(self):
+                return self
+
+            def wait_for(self, **kw):
+                return None
+
+            def evaluate(self, _js):
+                last = picker.page >= len(picker.pages) - 1
+                return "ng-scope disabled" if last else "ng-scope"
+
+            def click(self):
+                if "Next Page" in selector:
+                    picker.page += 1
+                elif "First Page" in selector:
+                    picker.page = 0
+
+            def filter(self, has_text=None):
+                picker.clicked = has_text.pattern if has_text else None
+                return self
+
+        return _Loc()
+
+    def get_by_role(self, role, name=None, **kw):
+        picker = self
+
+        class _Btn:
+            @property
+            def first(self):
+                return self
+
+            def click(self):
+                if name == "Cancel":
+                    picker.cancelled = True
+
+        return _Btn()
+
+
+def _picker_uploader(pages):
+    from bggpipe.upload import PlaywrightUploader
+
+    up = PlaywrightUploader("someone")
+
+    class _Page:
+        def wait_for_timeout(self, ms):
+            pass
+
+    up._page = _Page()
+    return up, _FakePicker(pages)
+
+
+def test_version_found_on_a_later_page():
+    up, dialog = _picker_uploader(
+        [
+            ["Sleeping Gods (Czech edition) (2023)"],
+            ["Sleeping Gods (English Gamefound edition) (2023)"],
+        ]
+    )
+    picked, why = up._select_version(dialog, "English Gamefound edition")
+    assert picked is True  # the old code gave up after page one
+    assert "Gamefound" in dialog.clicked  # regex-escaped row text
+    assert why == ""
+
+
+def test_api_name_with_printing_qualifier_matches_loosely():
+    up, dialog = _picker_uploader([["Mysterium (English edition) (2018)"]])
+    picked, why = up._select_version(dialog, "English edition 2018-2")
+    assert picked is True
+    assert "matched loosely" in why
+
+
+def test_ambiguous_versions_are_refused_not_guessed():
+    up, dialog = _picker_uploader(
+        [
+            [
+                "Munchkin (English edition) (2009)",
+                "Munchkin (English edition) (2015)",
+            ]
+        ]
+    )
+    picked, why = up._select_version(dialog, "English edition 2009")
+    assert picked is False
+    assert "refusing to guess" in why
+    assert dialog.cancelled is True
+
+
+def test_absent_version_reports_pages_scanned():
+    up, dialog = _picker_uploader([["Catan (German edition) (2015)"]])
+    picked, why = up._select_version(dialog, "English edition")
+    assert picked is False
+    assert "not offered" in why and "1 page(s)" in why