From 2d68bad966ae8dda21d836e44b0be0dbd3734677 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Thu, 27 Aug 2026 11:54:07 -0400 Subject: [PATCH] Aug 27 - Update code to follow-up with ST functions --- CLAUDE.md | 85 ++++++++++- app/api/inspections.py | 51 ++++++- app/api/photos.py | 22 ++- app/api/stats.py | 7 +- app/models/inspection.py | 58 +++++++ app/routes/dashboard.py | 6 +- app/routes/inspections.py | 144 +++++++++++++++++- app/templates/inspections/view.html | 58 ++++++- .../versions/phase56_followup_assignee.py | 71 +++++++++ tests/test_followup_ownership.py | 131 ++++++++++++++++ 10 files changed, 617 insertions(+), 16 deletions(-) create mode 100644 migrations/versions/phase56_followup_assignee.py create mode 100644 tests/test_followup_ownership.py diff --git a/CLAUDE.md b/CLAUDE.md index 05273ec..acac20e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -843,7 +843,7 @@ limiter = Limiter( ## 17. Alembic Migration Chain -**Current HEAD:** `phase55_template_contracts`. +**Current HEAD:** `phase56_followup_assignee`. **Chain root:** `0003_add_user_active` — a guarded squashed baseline (MT-2) that recreates the full 25-table schema with INFORMATION_SCHEMA guards. The original baseline migrations (0001/0002/0003) were lost; this file restores the chain root so Alembic can build the revision map. `down_revision = None`. @@ -884,7 +884,8 @@ limiter = Limiter( → phase48_schedule_parent_inspection → phase49_followup_requested_by → phase50_sched_acknowledged → phase51_external_inspector → phase52_user_ui_theme → phase53_knowledge_sort_order - → phase54_user_notif_matrix → phase55_template_contracts ← HEAD + → phase54_user_notif_matrix → phase55_template_contracts + → phase56_followup_assignee ← HEAD ``` `phase54` / `phase55` port the ST August-2026 work (ST calls them phase51 / @@ -907,7 +908,22 @@ means SHARED**, so every pre-existing form stays available everywhere and the migration cannot change behaviour on deploy. Table-existence check — safe to re-run. -**Deploy order for both (tenant DBs):** +#### phase56 — assign a follow-up to another inspector + +Adds `inspections.follow_up_assigned_to` (FK → `users.id`, ON DELETE SET NULL) +— see §27c. **No backfill:** NULL means the follow-up belongs to the +inspection's own inspector, which is what every existing row already means, so +deploying cannot change who owns anything. + +**This is the THIRD FK from `inspections` to `users`** (`inspector_id`, +`follow_up_requested_by`, and now this). Every relationship spanning the two +tables must pin `foreign_keys` explicitly or the mapper is ambiguous — and it +raises on first ORM *use*, not at import, so the app starts cleanly and then +every request 500s. Column + constraint checks — safe to re-run. + +ST calls this phase53; MT's chain was already past that number. Match by NAME. + +**Deploy order (tenant DBs):** ```bash python -m control.tenant_migrate upgrade --tenant all sudo systemctl restart gunicorn @@ -1507,6 +1523,8 @@ set -a; . /etc/jqc/control.env; set +a | 105 | **The flag-issue assignee list is contract-scoped, and BOTH call sites must use `_assignable_staff_for()`; a failed flag-issue POST must return non-2xx** | `execute()` renders the dropdown, `flag_issue()` builds the choices that validate the POST — the choices are the security boundary. An org-wide list let anyone assign another client's Customer Inspector, who was then emailed the facility name and issue description. And the offcanvas JS branches on `res.ok`, so a 200 re-render of an invalid form reads as success: the panel closes, the page reloads, and no issue exists. | | 106 | **Name the Groq model in the chat error log, and keep `_DEFAULT_GROQ_MODEL` current** | Groq retires models without notice; when the configured one disappears the API 404s and EVERY question returns the generic "problem reaching the AI assistant" reply, with nothing else broken — invisible until a customer complains. The fix needs no deploy, only `GROQ_MODEL`, which is exactly what the log line must say. | | 107 | **`viewer_is_our_staff` in `issues/view.html` is an explicit role ALLOWLIST, and `external_inspector` is absent on purpose** | `not current_user.is_customer_account` fails OPEN — a missing attribute yields Jinja `Undefined`, `not Undefined` is true, and the internal-process chrome renders for exactly the accounts it must be hidden from. This is not a rule-87 violation: rule 87 governs capability/scoping, where a Customer Inspector must behave like our inspector; this asks "does this person work for us?", the one place the two genuinely differ. | +| 108 | **A follow-up has exactly ONE owner: use `follow_up_owner` (row) / `follow_up_owned_by()` (query) — never re-derive it** | Assignee when set, original inspector otherwise. The API's two arms must be mutually exclusive (`follow_up_assigned_to == me` OR `assigned_to IS NULL AND inspector_id == me`); drop the `IS NULL` and two people turn up for the same re-inspection. The authorship filter must be DEFERRED when `follow_up_required=true` is requested, or the rows the assignee needs are hidden before the ownership test runs. | +| 109 | **Inspector READ access is facility scope; WRITE access is authorship** | `index()` lists by facility (rule 58), so `view()`/`export_pdf()` must too — scoping reads by authorship made the list offer rows that said "Access denied" on click, and locked the follow-up assignee out of the parent inspection. `execute`, `save_draft_ajax`, `upload_photo_ajax` and `flag_issue` keep the authorship check: readable is not editable. | | 98 | **Reports R1 + R2 contract cascade is client-side only — facility_id is the sole DB filter** | The Contract dropdown in `reports/issues_aging.html` and `reports/sla_compliance.html` has no `name` attribute and is never submitted. It exists only to narrow the Facility ` + + {# ── Assign it (phase56) ──────────────────────────────────────── + Optional. Left blank, the follow-up stays with whoever performed + the original inspection — the behaviour before this existed. The + list is contract-scoped in _followup_assignees_for(), so a + Customer Director only ever sees inspectors on their own + contracts. #} + {% if followup_assignees %} +
+ + +
+ Choose someone else to carry out the re-inspection. They are + notified and it appears in their list on the web and the iPad; + the original inspector is not asked to do it. +
+
+ {% endif %}
diff --git a/migrations/versions/phase56_followup_assignee.py b/migrations/versions/phase56_followup_assignee.py new file mode 100644 index 0000000..2f0c892 --- /dev/null +++ b/migrations/versions/phase56_followup_assignee.py @@ -0,0 +1,71 @@ +"""phase56 — assign a follow-up re-inspection to a specific inspector + +Adds `inspections.follow_up_assigned_to` (FK -> users.id, ON DELETE SET NULL). + +Until now a follow-up implicitly belonged to whoever performed the original +inspection: they were the one notified, and the mobile API only ever showed +follow-ups where `inspector_id == the caller`. A director could not hand the +re-inspection to somebody else. + +NULL means exactly what it meant before — the follow-up belongs to the +inspection's own inspector — so every existing row keeps its current behaviour +and no backfill is needed. `Inspection.follow_up_owner` is the one place that +resolves assignee-or-inspector. + +**This is the THIRD FK from inspections to users** (inspector_id, +follow_up_requested_by, and now this). Rule 86: any relationship between the two +tables must pin `foreign_keys` explicitly or the mapper is ambiguous — and it +raises on first ORM *use*, not at import, so the app starts fine and then every +request 500s. `Inspection.follow_up_assignee` pins it; `User.inspections` was +already pinned in phase46. + +MT note: ST calls this phase53; MT's chain was already past that number, so +the revision id differs. Match by NAME, not number, when comparing the trees. + +INFORMATION_SCHEMA checks — safe to re-run. +""" + +revision = 'phase56_followup_assignee' +down_revision = 'phase55_template_contracts' +branch_labels = None +depends_on = None + +from alembic import op +import sqlalchemy as sa + + +def _has_column(conn, table, column): + return conn.execute(sa.text( + "SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS " + "WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = :t AND COLUMN_NAME = :c" + ), {'t': table, 'c': column}).scalar() > 0 + + +def _has_constraint(conn, table, name): + return conn.execute(sa.text( + "SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS " + "WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = :t AND CONSTRAINT_NAME = :n" + ), {'t': table, 'n': name}).scalar() > 0 + + +def upgrade(): + conn = op.get_bind() + if not _has_column(conn, 'inspections', 'follow_up_assigned_to'): + op.add_column('inspections', + sa.Column('follow_up_assigned_to', sa.Integer, nullable=True)) + if not _has_constraint(conn, 'inspections', 'fk_inspections_followup_assignee'): + op.create_foreign_key( + 'fk_inspections_followup_assignee', 'inspections', 'users', + ['follow_up_assigned_to'], ['id'], ondelete='SET NULL', + ) + + +def downgrade(): + conn = op.get_bind() + if _has_constraint(conn, 'inspections', 'fk_inspections_followup_assignee'): + op.drop_constraint('fk_inspections_followup_assignee', 'inspections', + type_='foreignkey') + if _has_column(conn, 'inspections', 'follow_up_assigned_to'): + # Assignments are discarded; every follow-up reverts to belonging to the + # inspection's own inspector, which is where it started. + op.drop_column('inspections', 'follow_up_assigned_to') diff --git a/tests/test_followup_ownership.py b/tests/test_followup_ownership.py new file mode 100644 index 0000000..2d0322e --- /dev/null +++ b/tests/test_followup_ownership.py @@ -0,0 +1,131 @@ +""" +tests/test_followup_ownership.py +--------------------------------- +phase56 — a follow-up has exactly ONE owner, and every surface must agree. + +`Inspection.follow_up_owner` (the loaded-row property) and +`Inspection.follow_up_owned_by()` (the SQL predicate) are two expressions of +the same rule: the assignee when one is set, the inspection's own inspector +otherwise. They are declared next to each other because they must not drift. + +What these guard +---------------- +The predicate's two arms are mutually exclusive ON PURPOSE. Drop the +`is_(None)` from the second arm and the original inspector keeps matching a +follow-up that was handed to somebody else — two people turn up to do the same +re-inspection, and nothing errors. + +The three query surfaces (mobile list filter, web dashboard card, iPad stats +KPI) each used to express ownership by hand, and three of them tested +AUTHORSHIP: an assignee saw the work in their list while both dashboards read +0. These tests pin the predicate itself; the surfaces now call it rather than +re-deriving it. +""" + +import pytest + + +@pytest.fixture +def client(app): + with app.app_context(): + from app import db + db.drop_all() + db.create_all() + yield app.test_client() + db.session.remove() + + +def _user(username, role='inspector'): + from app import db + from app.models.user import User + u = User(username=username, full_name=username.title(), role=role, + email=f'{username}@example.com', active=True, password_set=True) + u.set_password('pw-correct1') + db.session.add(u) + db.session.commit() + return u + + +def _inspection(inspector, assignee=None, follow_up=True): + from app import db + from app.models.facility import Facility + from app.models.inspection import Inspection, InspectionTemplate + + fac = Facility.query.first() + if fac is None: + fac = Facility(name='Main Office', active=True) + db.session.add(fac) + db.session.commit() + tmpl = InspectionTemplate.query.first() + if tmpl is None: + tmpl = InspectionTemplate(name='Restroom Check', active=True, form_schema=[]) + db.session.add(tmpl) + db.session.commit() + + insp = Inspection(template_id=tmpl.id, facility_id=fac.id, + inspector_id=inspector.id, status='completed', + follow_up_required=follow_up, + follow_up_assigned_to=(assignee.id if assignee else None)) + db.session.add(insp) + db.session.commit() + return insp + + +def _owned_ids(user): + from app.models.inspection import Inspection + return sorted(i.id for i in Inspection.query + .filter(Inspection.follow_up_owned_by(user.id)).all()) + + +# ── the property ───────────────────────────────────────────────────────────── + +def test_owner_is_the_inspector_when_unassigned(client): + ivy = _user('ivy') + insp = _inspection(ivy) + assert insp.follow_up_owner.id == ivy.id + + +def test_owner_is_the_assignee_when_assigned(client): + ivy, sam = _user('ivy'), _user('sam') + insp = _inspection(ivy, assignee=sam) + assert insp.follow_up_owner.id == sam.id + + +# ── the SQL predicate ──────────────────────────────────────────────────────── + +def test_predicate_matches_unassigned_own_work(client): + ivy = _user('ivy') + insp = _inspection(ivy) + assert _owned_ids(ivy) == [insp.id] + + +def test_predicate_matches_work_handed_to_me(client): + ivy, sam = _user('ivy'), _user('sam') + insp = _inspection(ivy, assignee=sam) + assert _owned_ids(sam) == [insp.id] + + +def test_predicate_releases_the_original_inspector_once_assigned(client): + """The `is_(None)` on the second arm. Without it Ivy still matches, and two + people turn up to do the same re-inspection.""" + ivy, sam = _user('ivy'), _user('sam') + _inspection(ivy, assignee=sam) + assert _owned_ids(ivy) == [] + + +def test_every_row_has_exactly_one_owner(client): + """The property and the predicate must partition the rows, not overlap.""" + from app.models.inspection import Inspection + + ivy, sam, zoe = _user('ivy'), _user('sam'), _user('zoe') + _inspection(ivy) # unassigned, Ivy's + _inspection(ivy, assignee=sam) # Ivy's work, Sam's follow-up + _inspection(sam, assignee=sam) # Sam's work, explicitly Sam's + _inspection(zoe) # unassigned, Zoe's + + everyone = [ivy, sam, zoe] + for insp in Inspection.query.all(): + owners = [u.id for u in everyone if insp.id in _owned_ids(u)] + assert owners == [insp.follow_up_owner.id], ( + f'inspection {insp.id}: predicate says {owners}, ' + f'property says {insp.follow_up_owner.id}')