From a0d335b235e1b2296e545cf1f340ea1208766080 Mon Sep 17 00:00:00 2001 From: DEls Date: Fri, 11 Sep 2026 10:48:51 +0200 Subject: [PATCH] fix(timesheets): stamp site on log rows, ending read-time drift The timesheet log carried neither site nor project_code; attribution was synthesised at read time from employees.site, so moving a worker to another site silently migrated their entire payroll history with them. docs/data-model.md flags this as the schema's worst drift risk. Adds an optional site relation (maxSelect 1, cascadeDelete false so deleting a site cannot destroy payroll history) and stamps all 1367 rows from the employee's current site. 1367/1367 written, 0 orphans. Tightened to required in Task 16, once the import route writes it on every new row. Co-Authored-By: Claude Opus 5 --- .../2026-09-11-backfill-timesheet-sites.py | 276 ++++++++++++++++++ 1 file changed, 276 insertions(+) create mode 100644 scripts/migrations/2026-09-11-backfill-timesheet-sites.py diff --git a/scripts/migrations/2026-09-11-backfill-timesheet-sites.py b/scripts/migrations/2026-09-11-backfill-timesheet-sites.py new file mode 100644 index 0000000..fad51c7 --- /dev/null +++ b/scripts/migrations/2026-09-11-backfill-timesheet-sites.py @@ -0,0 +1,276 @@ +#!/usr/bin/env python3 +""" +Add timesheets.site and backfill it from employees.site. + + PB_URL=... PB_ADMIN_EMAIL=... PB_ADMIN_PASSWORD=... \\ + python3 scripts/migrations/2026-09-11-backfill-timesheet-sites.py [--apply] + +DRY RUN BY DEFAULT. Pass --apply to write. + +WHY THIS RUNS NOW +----------------- +Today the timesheet log has neither site nor project_code; site is +synthesised at read time from employees.site. That is the collection with +the worst attribution drift in the system (docs/data-model.md section 2): +move a worker between sites and their entire payroll history silently +follows them to the new site. + +Stamping site at import time fixes that permanently, and makes the future +per-user site allowlist a single `site IN (...)` filter clause instead of +a join through employees. + +THE SAFETY PROPERTY +------------------- +site is resolved from exactly one source: the employee's own site +relation. There is no second source to cross-check against, so the rule +is simply that a row whose employee does not resolve to exactly one site +is reported with the employee it points at and skipped - never guessed. +Inventing a fallback site here would write a wrong payroll attribution +that looks indistinguishable from a correct one afterwards. + +The field is added optional. PocketBase rejects a required field that +existing rows already violate, so tightening to required happens in +Phase 3 once the import route writes it on every new row. + +Idempotent: rows that already carry a site are left alone. +""" +import json +import os +import sys +import urllib.error +import urllib.request + +APPLY = "--apply" in sys.argv +COLLECTION = "timesheets" + + +def admin_auth(base_url): + body = json.dumps( + { + "identity": os.environ["PB_ADMIN_EMAIL"], + "password": os.environ["PB_ADMIN_PASSWORD"], + } + ).encode() + req = urllib.request.Request( + f"{base_url}/api/collections/_superusers/auth-with-password", + data=body, + headers={"Content-Type": "application/json"}, + method="POST", + ) + with urllib.request.urlopen(req) as resp: + return json.loads(resp.read())["token"] + + +def api(base_url, token, method, path, body=None): + """ + A 404 is only soft for GET - "this collection/record is absent" is a + legitimate answer to a read. On a write it is not: a PATCH against a + since-deleted record returning None would be counted as a successful + write. Mutations raise. + """ + data = json.dumps(body).encode() if body is not None else None + req = urllib.request.Request( + f"{base_url}{path}", + data=data, + method=method, + headers={"Authorization": token, "Content-Type": "application/json"}, + ) + try: + with urllib.request.urlopen(req) as resp: + txt = resp.read().decode() + return json.loads(txt) if txt else None + except urllib.error.HTTPError as e: + if e.code == 404 and method == "GET": + return None + raise RuntimeError( + f"{method} {path}: {e.code} {e.read().decode() if e.fp else ''}" + ) from e + + +def get_all(base_url, token, coll, fields): + out, page = [], 1 + while True: + data = api( + base_url, + token, + "GET", + f"/api/collections/{coll}/records" + f"?perPage=500&page={page}&fields={fields}", + ) + out.extend(data["items"]) + if page >= data["totalPages"]: + return out + page += 1 + + +def rel_id(value): + """ + Normalise a PocketBase relation field to a single id, or "" when it + does not resolve to exactly one. + + Single relations come back as a bare string, multi relations as a + list. employees.site being a multi-relation with two entries is not + an answer to "which site does this employee's labour belong to" - it + is an ambiguity, and writing element [0] would silently pick one. + Anything that is not exactly one id is "unresolved", which routes the + row to the skip report rather than to a write. + """ + if isinstance(value, list): + return value[0] if len(value) == 1 else "" + return (value or "").strip() + + +def main(): + base_url = os.environ.get("PB_URL", "http://127.0.0.1:8090").rstrip("/") + token = admin_auth(base_url) + # Say where we are writing. This script mutates production by default + # target, and a run whose output names no host is a run you cannot + # audit afterwards. + print(f"target: {base_url} mode: {'APPLY' if APPLY else 'DRY RUN'}") + + coll = api(base_url, token, "GET", f"/api/collections/{COLLECTION}") + if coll is None: + print(f"ABORT: collection {COLLECTION!r} not found on {base_url}") + return 1 + fields = coll.get("fields", []) + have = {f["name"] for f in fields} + + sites_coll = api(base_url, token, "GET", "/api/collections/sites") + if sites_coll is None: + print(f"ABORT: collection 'sites' not found on {base_url}") + return 1 + site_collection_id = sites_coll["id"] + + # cascadeDelete MUST stay False. A cascade-delete relation here would + # mean deleting a site deletes every timesheet row attributed to it - + # i.e. destroys that site's payroll history. maxSelect 1 keeps the + # field single-valued so the future `site IN (...)` filter is exact. + site_field = { + "name": "site", + "type": "relation", + "required": False, + "collectionId": site_collection_id, + "maxSelect": 1, + "cascadeDelete": False, + } + + if "site" not in have: + print(f"\n{COLLECTION}: would add optional `site` relation") + print(f" -> sites collection id: {site_collection_id}") + print(f" -> field def: {json.dumps(site_field, sort_keys=True)}") + if APPLY: + api( + base_url, + token, + "PATCH", + f"/api/collections/{COLLECTION}", + {"fields": fields + [site_field]}, + ) + print(f"{COLLECTION}: added optional `site` relation") + else: + existing = next(f for f in fields if f["name"] == "site") + print(f"\n{COLLECTION}: `site` field already present") + print(f" -> existing def: {json.dumps(existing, sort_keys=True)}") + + employees = get_all(base_url, token, "employees", "id,site") + emp_site = {e["id"]: rel_id(e.get("site")) for e in employees} + + rows = get_all(base_url, token, COLLECTION, "id,employee,site") + + todo, orphan, done = [], [], 0 + for r in rows: + if rel_id(r.get("site")): + done += 1 + continue + emp = rel_id(r.get("employee")) + s = emp_site.get(emp, "") + if s: + todo.append((r["id"], s)) + continue + + # Name the reason. "This row has no employee at all" is a very + # different finding from "the employee exists but carries no + # site", and a single undifferentiated orphan count hides which + # one happened - and therefore who has to fix it. + if not emp: + reason = "no employee on row" + elif emp not in emp_site: + reason = "employee not found" + else: + reason = "employee has no site" + orphan.append((r["id"], emp or "-", reason)) + + print(f"\nemployees={len(employees)} sites_collection={site_collection_id}") + print( + f"\n{COLLECTION} total={len(rows)} to_write={len(todo)} " + f"no_employee_site={len(orphan)}" + ) + print(f"{COLLECTION} already_had_site={done}") + + for o in orphan[:20]: + print(f" ORPHAN id={o[0]} employee={o[1]} ({o[2]})") + if len(orphan) > 20: + print(f" ... and {len(orphan) - 20} more") + + if not APPLY: + if orphan: + print( + f"\nNOTE: {len(orphan)} row(s) would be skipped. The plan " + f"expects 0. Report these before applying - these rows " + f"cannot be attributed without a human decision, and a " + f"guessed site is an invented payroll fact." + ) + print("\nDRY RUN. Re-run with --apply to write.") + return 0 + + # Per-record write failures are tracked apart from the data-quality + # orphans above. A row we chose not to touch and a row we tried to + # touch and failed on are not the same fact; counting the latter as + # the former makes a half-broken run read as a clean one. + errors = [] + for i, (rec_id, site_id) in enumerate(todo, 1): + try: + api( + base_url, + token, + "PATCH", + f"/api/collections/{COLLECTION}/records/{rec_id}", + {"site": site_id}, + ) + except Exception as e: # noqa: BLE001 - report, never swallow + errors.append(f"{rec_id} ({e})") + if i % 100 == 0: + print(f" wrote {i}/{len(todo)}") + + for n in errors[:20]: + print(f" ERROR {n}") + + remaining = [ + r + for r in get_all(base_url, token, COLLECTION, "id,site") + if not rel_id(r.get("site")) + ] + print( + f"\nwrote {len(todo) - len(errors)}/{len(todo)} " + f"write errors: {len(errors)}" + ) + print( + f"{COLLECTION} still missing site: {len(remaining)} " + f"(expected {len(orphan)} = the rows we deliberately skipped)" + ) + + if errors: + print(f"\nFAILED: {len(errors)} record(s) did not write. Re-run.") + return 1 + if len(remaining) != len(orphan): + print("\nFAILED: post-run count does not match the plan. Investigate.") + return 1 + return 0 + + +if __name__ == "__main__": + try: + sys.exit(main()) + except KeyError as e: + print(f"ERROR: missing environment variable {e}", file=sys.stderr) + sys.exit(2)