Fix sheet error
This commit is contained in:
@@ -38,6 +38,7 @@ Discord bot for the TipiLAN community. Manages member roles and nicknames via Go
|
|||||||
### 3. Google Sheet Format
|
### 3. Google Sheet Format
|
||||||
|
|
||||||
Row 1 = headers (exact names). Row 2 = formula/stats row (skipped by bot). Data starts row 3.
|
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 |
|
| Column | What the bot does with it |
|
||||||
|---|---|
|
|---|---|
|
||||||
|
|||||||
@@ -40,6 +40,11 @@ SCOPES = [
|
|||||||
# - Roll : role value - maps to a Discord role
|
# - Roll : role value - maps to a Discord role
|
||||||
# - Discordis synced? : TRUE/FALSE - bot writes this after confirming sync
|
# - Discordis synced? : TRUE/FALSE - bot writes this after confirming sync
|
||||||
# - Groupi lisatud? : group membership flag (managed externally)
|
# - 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 = [
|
EXPECTED_HEADERS = [
|
||||||
@@ -62,6 +67,9 @@ _worksheet: gspread.Worksheet | None = None
|
|||||||
# In-memory cache: list of dicts (one per row)
|
# In-memory cache: list of dicts (one per row)
|
||||||
_cache: list[dict] = []
|
_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:
|
def _get_worksheet() -> gspread.Worksheet:
|
||||||
"""Authenticate and return the first worksheet of the configured sheet."""
|
"""Authenticate and return the first worksheet of the configured sheet."""
|
||||||
@@ -73,32 +81,44 @@ def _get_worksheet() -> gspread.Worksheet:
|
|||||||
return _worksheet
|
return _worksheet
|
||||||
|
|
||||||
|
|
||||||
def _ensure_headers(ws: gspread.Worksheet) -> None:
|
def _ensure_headers(ws: gspread.Worksheet) -> list[str]:
|
||||||
"""Verify the header row matches what we expect.
|
"""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
|
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
|
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`
|
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
|
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)
|
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(
|
log.warning(
|
||||||
"Sheet header row does not match EXPECTED_HEADERS "
|
"Sheet header row is missing expected columns %s (found %s). "
|
||||||
"(missing/renamed: %s). Expected %s, found %s. "
|
"Not writing to the (protected) header row; fix it manually.",
|
||||||
"Not writing to the (protected) header row; fix it manually if needed.",
|
missing,
|
||||||
missing or "none — order/whitespace differs",
|
|
||||||
EXPECTED_HEADERS,
|
|
||||||
existing,
|
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]:
|
def _refresh_sync() -> list[dict]:
|
||||||
global _cache
|
global _cache, _headers
|
||||||
ws = _get_worksheet()
|
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
|
# head=1: row 1 is the header; row 2 is a formula/stats row - skip it
|
||||||
records = ws.get_all_records(head=1)
|
records = ws.get_all_records(head=1)
|
||||||
_cache = records[1:] # drop the formula row (row 2) from the cache
|
_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:
|
if row_idx is None:
|
||||||
return False
|
return False
|
||||||
|
|
||||||
try:
|
col_idx = _col_index(ws, column_name)
|
||||||
col_idx = EXPECTED_HEADERS.index(column_name) + 1
|
if col_idx is None:
|
||||||
except ValueError:
|
|
||||||
return False
|
return False
|
||||||
|
|
||||||
ws.update([[value]], gspread.utils.rowcol_to_a1(row_idx, col_idx),
|
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:
|
def _batch_set_synced_sync(updates: list[tuple[int, bool]]) -> None:
|
||||||
ws = _worksheet or _get_worksheet()
|
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 = []
|
cells = []
|
||||||
for discord_id, synced in updates:
|
for discord_id, synced in updates:
|
||||||
row_idx = _row_index_for_member(discord_id=discord_id)
|
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:
|
def _add_new_member_row_sync(username: str, discord_id: int) -> None:
|
||||||
ws = _worksheet or _get_worksheet()
|
ws = _worksheet or _get_worksheet()
|
||||||
row = [""] * len(EXPECTED_HEADERS)
|
values = {"Discord": username, "User ID": str(discord_id), "Discordis synced?": "FALSE"}
|
||||||
row[EXPECTED_HEADERS.index("Discord")] = username
|
cols = {name: _col_index(ws, name) for name in values}
|
||||||
row[EXPECTED_HEADERS.index("User ID")] = str(discord_id)
|
missing = [name for name, col in cols.items() if col is None]
|
||||||
row[EXPECTED_HEADERS.index("Discordis synced?")] = "FALSE"
|
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")
|
ws.append_row(row, value_input_option="USER_ENTERED")
|
||||||
new_entry = {h: row[i] for i, h in enumerate(EXPECTED_HEADERS)}
|
_cache.append(dict(zip(_headers, row)))
|
||||||
_cache.append(new_entry)
|
|
||||||
|
|
||||||
|
|
||||||
async def add_new_member_row(username: str, discord_id: int) -> None:
|
async def add_new_member_row(username: str, discord_id: int) -> None:
|
||||||
|
|||||||
90
tests/test_sheet_columns.py
Normal file
90
tests/test_sheet_columns.py
Normal file
@@ -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 == []
|
||||||
Reference in New Issue
Block a user