diff --git a/README.md b/README.md index 8291d47..c38adb8 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,7 @@ Discord bot for the TipiLAN community. Manages member roles and nicknames via Go ### 3. Google Sheet Format Row 1 = headers (exact names). Row 2 = formula/stats row (skipped by bot). Data starts row 3. +Column order doesn't matter and extra columns (e.g. `Käepael`) are fine - the bot finds every column it reads or writes by header name. | Column | What the bot does with it | |---|---| diff --git a/core/sheets.py b/core/sheets.py index 8eba97c..e67e44d 100644 --- a/core/sheets.py +++ b/core/sheets.py @@ -40,6 +40,11 @@ SCOPES = [ # - Roll : role value - maps to a Discord role # - Discordis synced? : TRUE/FALSE - bot writes this after confirming sync # - Groupi lisatud? : group membership flag (managed externally) +# +# The owner adds/reorders columns freely (e.g. "Käepael" was inserted after +# "Nimi"), so NEVER derive a write position from EXPECTED_HEADERS - resolve it +# from the live header row via _col_index(). Positional writes used to land one +# column to the left and overwrote "Roll" with the TRUE/FALSE synced flag. # --------------------------------------------------------------------------- EXPECTED_HEADERS = [ @@ -62,6 +67,9 @@ _worksheet: gspread.Worksheet | None = None # In-memory cache: list of dicts (one per row) _cache: list[dict] = [] +# Live header row (row 1) as last read from the sheet; source of truth for column positions +_headers: list[str] = [] + def _get_worksheet() -> gspread.Worksheet: """Authenticate and return the first worksheet of the configured sheet.""" @@ -73,32 +81,44 @@ def _get_worksheet() -> gspread.Worksheet: return _worksheet -def _ensure_headers(ws: gspread.Worksheet) -> None: - """Verify the header row matches what we expect. +def _ensure_headers(ws: gspread.Worksheet) -> list[str]: + """Read the live header row and warn if a column the bot relies on is missing. The production sheet is owner-managed and its header row (row 1) is a protected range, so the bot must NOT write to it — attempting to do so raises `APIError [400]: You are trying to edit a protected cell or object` and aborts the whole refresh. We only log a mismatch so it can be fixed - by hand; column lookups still work as long as the headers we rely on exist. + by hand. Extra or reordered columns are fine: reads map by header name and + writes resolve positions from the returned row. """ existing = ws.row_values(1) - if existing != EXPECTED_HEADERS: - missing = [h for h in EXPECTED_HEADERS if h not in existing] + missing = [h for h in EXPECTED_HEADERS if h not in existing] + if missing: log.warning( - "Sheet header row does not match EXPECTED_HEADERS " - "(missing/renamed: %s). Expected %s, found %s. " - "Not writing to the (protected) header row; fix it manually if needed.", - missing or "none — order/whitespace differs", - EXPECTED_HEADERS, + "Sheet header row is missing expected columns %s (found %s). " + "Not writing to the (protected) header row; fix it manually.", + missing, existing, ) + return existing + + +def _col_index(ws: gspread.Worksheet, column_name: str) -> int | None: + """1-based position of `column_name` in the live header row, or None if absent.""" + global _headers + if not _headers: + _headers = _ensure_headers(ws) + try: + return _headers.index(column_name) + 1 + except ValueError: + log.error("Column %r not in sheet header row; refusing to write", column_name) + return None def _refresh_sync() -> list[dict]: - global _cache + global _cache, _headers ws = _get_worksheet() - _ensure_headers(ws) + _headers = _ensure_headers(ws) # head=1: row 1 is the header; row 2 is a formula/stats row - skip it records = ws.get_all_records(head=1) _cache = records[1:] # drop the formula row (row 2) from the cache @@ -160,9 +180,8 @@ def _update_cell_for_member_sync( if row_idx is None: return False - try: - col_idx = EXPECTED_HEADERS.index(column_name) + 1 - except ValueError: + col_idx = _col_index(ws, column_name) + if col_idx is None: return False ws.update([[value]], gspread.utils.rowcol_to_a1(row_idx, col_idx), @@ -189,7 +208,9 @@ async def update_cell_for_member( def _batch_set_synced_sync(updates: list[tuple[int, bool]]) -> None: ws = _worksheet or _get_worksheet() - col_idx = EXPECTED_HEADERS.index("Discordis synced?") + 1 + col_idx = _col_index(ws, "Discordis synced?") + if col_idx is None: + return cells = [] for discord_id, synced in updates: row_idx = _row_index_for_member(discord_id=discord_id) @@ -240,13 +261,16 @@ async def update_username(discord_id: int, new_username: str) -> bool: def _add_new_member_row_sync(username: str, discord_id: int) -> None: ws = _worksheet or _get_worksheet() - row = [""] * len(EXPECTED_HEADERS) - row[EXPECTED_HEADERS.index("Discord")] = username - row[EXPECTED_HEADERS.index("User ID")] = str(discord_id) - row[EXPECTED_HEADERS.index("Discordis synced?")] = "FALSE" + values = {"Discord": username, "User ID": str(discord_id), "Discordis synced?": "FALSE"} + cols = {name: _col_index(ws, name) for name in values} + missing = [name for name, col in cols.items() if col is None] + if missing: + raise RuntimeError(f"Sheet header row is missing columns {missing}") + row = [""] * len(_headers) + for name, col in cols.items(): + row[col - 1] = values[name] ws.append_row(row, value_input_option="USER_ENTERED") - new_entry = {h: row[i] for i, h in enumerate(EXPECTED_HEADERS)} - _cache.append(new_entry) + _cache.append(dict(zip(_headers, row))) async def add_new_member_row(username: str, discord_id: int) -> None: diff --git a/tests/test_sheet_columns.py b/tests/test_sheet_columns.py new file mode 100644 index 0000000..507e777 --- /dev/null +++ b/tests/test_sheet_columns.py @@ -0,0 +1,90 @@ +"""Regression tests: member-sheet writes must target columns by live header name. + +The roster sheet gained a "Käepael" column after "Nimi". Writes used to take +their position from EXPECTED_HEADERS, so every write landed one column left - +the TRUE/FALSE synced flag overwrote "Roll", and /check then reported +"Rolli 'FALSE' ei leitud serverist" for every member. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from core import sheets # noqa: E402 +from tests.conftest import run # noqa: E402 + +LIVE_HEADER = [ + "Nimi", "Käepael", "Organisatsioon", "Meil", "Discord", "User ID", "Sünnipäev", + "Telefon", "Valdkond", "Roll", "Discordis synced?", "Groupi lisatud?", "Särk", +] + + +class FakeWorksheet: + def __init__(self, header: list[str]): + self.header = header + self.updates: list[tuple[str, str]] = [] + self.cell_updates: list[tuple[int, int, str]] = [] + self.appended: list[list[str]] = [] + + def row_values(self, index: int) -> list[str]: + assert index == 1 + return list(self.header) + + def update(self, values, range_name, value_input_option=None): + self.updates.append((range_name, values[0][0])) + + def update_cells(self, cells, value_input_option=None): + self.cell_updates.extend((c.row, c.col, c.value) for c in cells) + + def append_row(self, row, value_input_option=None): + self.appended.append(list(row)) + + +@pytest.fixture +def sheet(monkeypatch): + ws = FakeWorksheet(LIVE_HEADER) + row = {h: "" for h in LIVE_HEADER} + row.update({"Nimi": "Mari Tamm", "Discord": "mari", "User ID": "123", "Roll": "Vabatahtlik"}) + monkeypatch.setattr(sheets, "_worksheet", ws) + monkeypatch.setattr(sheets, "_headers", [], raising=False) + monkeypatch.setattr(sheets, "_cache", [row]) + return ws + + +def test_set_synced_writes_live_synced_column_not_roll(sheet): + assert run(sheets.set_synced(123, False)) is True + assert sheet.updates == [("K3", "FALSE")] # K = "Discordis synced?", J would be "Roll" + assert sheets.get_cache()[0]["Roll"] == "Vabatahtlik" + + +def test_batch_set_synced_uses_live_column(sheet): + run(sheets.batch_set_synced([(123, True)])) + assert sheet.cell_updates == [(3, 11, "TRUE")] + + +def test_update_username_writes_discord_column_not_meil(sheet): + run(sheets.update_username(123, "mari_new")) + assert sheet.updates == [("E3", "mari_new")] + + +def test_add_new_member_row_places_values_by_header(sheet): + run(sheets.add_new_member_row("uus", 456)) + (row,) = sheet.appended + assert len(row) == len(LIVE_HEADER) + assert row[LIVE_HEADER.index("Discord")] == "uus" + assert row[LIVE_HEADER.index("User ID")] == "456" + assert row[LIVE_HEADER.index("Discordis synced?")] == "FALSE" + assert row[LIVE_HEADER.index("Roll")] == "" + assert sheets.find_member_by_id(456)["Discord"] == "uus" + + +def test_missing_column_refuses_to_write(monkeypatch, sheet): + sheet.header = [h for h in LIVE_HEADER if h != "Discordis synced?"] + assert run(sheets.set_synced(123, True)) is False + run(sheets.batch_set_synced([(123, True)])) + assert sheet.updates == [] and sheet.cell_updates == []