From aced3d060287fffed1b3840e0c341c268cac60cb Mon Sep 17 00:00:00 2001 From: NguyenND Date: Tue, 25 Aug 2026 12:24:34 -0400 Subject: [PATCH] Aug 25 - Implement new function allow Director (internal & customer) to assign and inspection to another inspector --- CLAUDE.md | 43 ++++++++++- app/api/inspections.py | 34 ++++++++- app/models/inspection.py | 28 +++++++ app/routes/inspections.py | 76 ++++++++++++++++++- app/templates/inspections/view.html | 45 +++++++++++ .../versions/phase53_followup_assignee.py | 68 +++++++++++++++++ 6 files changed, 286 insertions(+), 8 deletions(-) create mode 100644 migrations/versions/phase53_followup_assignee.py diff --git a/CLAUDE.md b/CLAUDE.md index 020424c..78bfffd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -575,7 +575,7 @@ Management of the underlying routes is otherwise unchanged; **Start** is the **a | Contracts | ✅ | ✅ | ✅ | read | scoped | | Templates | ✅ | ✅ | ❌ | ❌ | ❌ | | Inspections (execute) | ✅ | ✅ | ✅ | ✅ | read | -| Inspection follow-up (request) | ✅ | ✅ | ❌ | ❌ | ✅ own facilities | +| Inspection follow-up (request + assign) | ✅ | ✅ | ❌ | ❌ | ✅ own facilities | | Scheduled inspections (plan) | ✅ | ✅ | ✅ | ❌ | ✅ own contracts | | Inspection follow-up (clear) | ✅ | ✅ | ❌ | ❌ | ❌ | | Issues (create/assign) | ✅ | ✅ | ✅ | ✅ | ✅ create own | @@ -886,6 +886,28 @@ Customers can only *request*. `clear_followup` remains admin/director, `reinspec **Dispatch** goes through `notify_by_matrix(EVENT_FOLLOWUP_REQUESTED, ...)` — the new `followup_requested` matrix event (admin/director/PM on by default). The inspection's own inspector is notified directly by the route and passed in `exclude_user_ids` so they aren't double-notified; the requester is excluded too. Routing via the matrix (rather than hardcoding managers) is what makes per-contract recipients fire — rule 73. Without it a customer request would reach only the inspector and nobody would own scheduling the re-inspection. +### Assigning a follow-up to another inspector (Phase 53) + +A follow-up used to belong implicitly to whoever performed the original inspection: they were the one notified, and `GET /api/v1/inspections?follow_up_required=true` filtered on `inspector_id == caller`, so nobody else could even see it. `inspections.follow_up_assigned_to` (FK → users, SET NULL) lets a director — or a **Customer Director**, for their own facilities — hand the re-inspection to someone else. + +**NULL means what it always meant**: the follow-up belongs to the inspection's own inspector. No backfill, no behaviour change for existing rows. `Inspection.follow_up_owner` (assignee *or* inspector) is the single definition of ownership, so the web display, the notification and the API filter cannot disagree. + +**The assignee takes over.** Only the owner is notified, and only the owner sees it — the original inspector's list no longer shows a follow-up that was handed to someone else. In the API that means the two arms must be mutually exclusive: + +```python +db.or_( + Inspection.follow_up_assigned_to == user.id, + db.and_(Inspection.follow_up_assigned_to.is_(None), + Inspection.inspector_id == user.id), +) +``` + +Without the `is_(None)` on the second arm the original inspector keeps seeing it and two people turn up to do the same re-inspection. + +**The generic "inspectors see only their own inspections" filter has to be deferred** when `follow_up_required=true` is requested — an assigned follow-up lives on an inspection somebody *else* performed, so applying authorship first hides exactly the rows the assignee needs. + +**The picker is contract-scoped** (`_followup_assignees_for()`), for the same reason the flag-issue list is (rule 93): a Customer Director must never see, or assign work to, another client's inspector. Only the two INSPECTOR roles are offered — directors/PMs/auditors hold no `InspectorAssignment`, so they could not open the re-inspection anyway. The POST re-validates against that list, and a facility with no contract offers nobody (fail-closed, follow-up stays with the original inspector). `clear_followup` (single and bulk) clears the assignment too. + ### Per-Account Overrides for Customer Roles (Phase 51) `notify_by_matrix()` consults `UserNotificationMatrix` (§5) for the two customer-side role columns. One query per dispatch (`overrides_for_event`), then `users = [u for u in users if overrides.get(u.id, enabled)]` — an account with no row falls back to the global column, which is what makes both directions work. @@ -1015,7 +1037,24 @@ phase1_projects_roles → phase6_features → phase7_mobile_api → phase8_notif → phase49_external_inspector → phase50_default_modern → phase51_user_notif_matrix - → phase52_template_contracts ← HEAD + → phase52_template_contracts + → phase53_followup_assignee ← HEAD + +#### phase53 — assign a follow-up to another inspector + +Revision id `phase53_followup_assignee`. Adds `inspections.follow_up_assigned_to` (FK → `users.id`, ON DELETE SET NULL) — see §11 "Assigning a follow-up to another inspector". + +**No backfill.** NULL means the follow-up belongs to the inspection's own inspector, which is exactly what every existing row already means, so this cannot change who owns anything on deploy. + +**This is the THIRD FK from `inspections` to `users`** (rule 86). `Inspection.follow_up_assignee` pins `foreign_keys` explicitly; `User.inspections` was already pinned in phase46. Get this wrong and the mapper is ambiguous — and it raises on first ORM *use*, not at import, so the app starts cleanly and then every request 500s. + +`INFORMATION_SCHEMA` column + constraint checks — safe to re-run. `downgrade()` drops the FK then the column, returning every follow-up to its original inspector. + +**Deploy order:** +```bash +flask db upgrade +sudo systemctl restart gunicorn +``` #### phase52 — restrict forms to specific contracts diff --git a/app/api/inspections.py b/app/api/inspections.py index 1e79968..7ea6891 100644 --- a/app/api/inspections.py +++ b/app/api/inspections.py @@ -210,6 +210,11 @@ def _inspection_payload(inspection): # ── Follow-up / re-inspection fields ────────────────────────────── 'follow_up_required': inspection.follow_up_required, 'follow_up_note': inspection.follow_up_note, + # phase53 — who is to perform the follow-up. NULL means the + # inspection's own inspector, which is what it always meant. + 'follow_up_assigned_to': inspection.follow_up_assigned_to, + 'follow_up_assigned_to_name': (inspection.follow_up_assignee.display_name + if inspection.follow_up_assignee else None), 'parent_inspection_id': inspection.parent_inspection_id, # Set when this inspection was started from a ScheduledInspection — # drives the "Scheduled" badge on the web list and lets the iPad show @@ -267,8 +272,15 @@ def list_inspections(): query = Inspection.query - # Inspectors only see their own inspections - if user.is_inspector: + # Inspectors only see their own inspections. + # + # EXCEPT when asking for follow-up requests: a follow-up can now be handed + # to a different inspector (phase53), and that request lives on an + # inspection somebody ELSE performed. Applying this filter first would hide + # exactly the rows the assignee needs, so it is deferred to the follow-up + # block below, which applies ownership instead of authorship. + wants_follow_ups = request.args.get('follow_up_required', '').lower() in ('true', '1') + if user.is_inspector and not wants_follow_ups: query = query.filter(Inspection.inspector_id == user.id) # Optional filters @@ -280,7 +292,7 @@ def list_inspections(): if status: query = query.filter(Inspection.status == status) - if request.args.get('follow_up_required', '').lower() in ('true', '1'): + if wants_follow_ups: # Must mean exactly what "Follow-up" means everywhere on the web # (inspections.list / reports status_filter == 'follow_up'): flagged, # completed, and not yet answered by a linked re-inspection. @@ -298,6 +310,22 @@ def list_inspections(): Inspection.status == 'completed', ).filter(~Inspection.follow_ups.any()) + # Ownership, not authorship (phase53). Mirrors + # Inspection.follow_up_owner: an assigned follow-up belongs to the + # assignee ALONE, an unassigned one to the inspection's own inspector. + # + # The two arms are mutually exclusive on purpose. Without the second + # arm's `is_(None)` an inspector would keep seeing a follow-up that had + # been handed to someone else, and two people would turn up to do it. + if user.is_inspector: + query = query.filter(db.or_( + Inspection.follow_up_assigned_to == user.id, + db.and_( + Inspection.follow_up_assigned_to.is_(None), + Inspection.inspector_id == user.id, + ), + )) + from_date_str = request.args.get('from_date') if from_date_str: try: diff --git a/app/models/inspection.py b/app/models/inspection.py index cb75691..287c0bf 100644 --- a/app/models/inspection.py +++ b/app/models/inspection.py @@ -202,6 +202,21 @@ class Inspection(db.Model): ) follow_up_requested_at = db.Column(db.DateTime, nullable=True) + # phase53 — who is to PERFORM the follow-up re-inspection. + # + # NULL keeps the original behaviour: the follow-up belongs to the + # inspection's own inspector. When set, that person owns it instead — they + # are the one notified, and the one it appears for on the iPad. Lets a + # director (or a Customer Director) hand a re-inspection to someone other + # than whoever did the original. + # + # This is the THIRD FK from inspections to users (rule 86): every + # relationship spanning the two must pin foreign_keys explicitly, or the + # mapper is ambiguous and blows up on first ORM USE rather than at import. + follow_up_assigned_to = db.Column( + db.Integer, db.ForeignKey('users.id', ondelete='SET NULL'), nullable=True + ) + results = db.relationship('InspectionResult', backref='inspection', lazy='dynamic', cascade='all, delete-orphan') issues = db.relationship('Issue', backref='inspection', lazy='dynamic', cascade='all, delete-orphan') # The ScheduledInspection this inspection was started from (phase36), if any. @@ -212,6 +227,19 @@ class Inspection(db.Model): # Explicit foreign_keys: `inspector_id` also points at users. follow_up_requester = db.relationship('User', foreign_keys=[follow_up_requested_by]) + follow_up_assignee = db.relationship('User', + foreign_keys=[follow_up_assigned_to]) + + @property + def follow_up_owner(self): + """Who is expected to carry out the follow-up. + + The explicit assignee when one is set, otherwise the inspection's own + inspector — the single definition of ownership, so the web display, the + notification and the mobile API filter cannot disagree about who owns a + follow-up. + """ + return self.follow_up_assignee or self.inspector follow_ups = db.relationship('Inspection', backref=db.backref('parent', remote_side='Inspection.id'), lazy='dynamic', foreign_keys='Inspection.parent_inspection_id') diff --git a/app/routes/inspections.py b/app/routes/inspections.py index 6af958a..4c90139 100644 --- a/app/routes/inspections.py +++ b/app/routes/inspections.py @@ -969,7 +969,9 @@ def view(inspection_id): 'unchanged': sum(1 for r in rows if r['delta'] == 0), } + followup_assignees = _followup_assignees_for(inspection, current_user) return render_template('inspections/view.html', + followup_assignees=followup_assignees, inspection=inspection, form_fields=form_fields, form_data=form_data, @@ -1394,6 +1396,46 @@ def _view_url(inspection_id): return url_for('inspections.view', inspection_id=inspection_id) +def _followup_assignees_for(inspection, actor): + """Inspectors who may be handed this inspection's follow-up. + + Contract-scoped, for the same reason the flag-issue list is (rule 93): a + Customer Director must never see — let alone assign work to — another + client's inspector, and one of our own directors picking the wrong name + would leak this facility to an outsider. + + Only the two INSPECTOR roles are offered: a follow-up is an inspection, and + directors/PMs/auditors hold no InspectorAssignment, so they cannot be + scoped to a contract and could not open the re-inspection anyway. + + A facility with no contract yields nobody — fail-closed, leaving the + follow-up with the original inspector. + """ + from app.models.inspector_assignment import InspectorAssignment + + project_id = inspection.facility.project_id if inspection.facility else None + if not project_id: + return [] + + users = ( + User.query + .join(InspectorAssignment, InspectorAssignment.user_id == User.id) + .filter( + InspectorAssignment.project_id == project_id, + User.role.in_(User.INSPECTOR_ROLES), + User.active == True, + ) + .order_by(User.full_name, User.username) + .all() + ) + seen, out = set(), [] + for u in users: # the join repeats across assignments + if u.id not in seen: + seen.add(u.id) + out.append(u) + return out + + def _collect_inspection_photos(inspection): """Relative storage keys owned by an inspection, for cleanup after delete. @@ -1597,6 +1639,7 @@ def bulk_action(): insp.follow_up_note = None insp.follow_up_requested_by = None insp.follow_up_requested_at = None + insp.follow_up_assigned_to = None cleared.append(insp) changed += 1 db.session.commit() @@ -1661,22 +1704,48 @@ def flag_followup(inspection_id): note = request.form.get('follow_up_note', '').strip() or None + # ── Assignee (phase53) ──────────────────────────────────────────────── + # Optional. Blank keeps the original behaviour: the follow-up belongs to + # the inspection's own inspector. Validated against the contract-scoped + # list rather than trusted, so a crafted id cannot hand work to another + # customer's inspector (and tell them this facility's name in the email). + assignee_id = request.form.get('follow_up_assigned_to', type=int) or None + if assignee_id: + allowed = {u.id for u in _followup_assignees_for(inspection, current_user)} + if assignee_id not in allowed: + current_app.logger.warning( + 'FOLLOW-UP | out-of-contract assignee blocked | inspection=%s | ' + 'assignee=%s | by=%s', + inspection_id, assignee_id, current_user.username) + flash('That inspector is not assigned to this facility\'s contract.', + 'danger') + return redirect(_view_url(inspection_id)) + inspection.follow_up_required = True inspection.follow_up_note = note inspection.follow_up_requested_by = current_user.id inspection.follow_up_requested_at = now_eastern() + inspection.follow_up_assigned_to = assignee_id db.session.commit() note_suffix = f' Note: {note}' if note else '' who = (f'The customer ({current_user.display_name})' if is_customer else current_user.display_name) + assigned_suffix = '' + if inspection.follow_up_assignee: + assigned_suffix = (f' It has been assigned to ' + f'{inspection.follow_up_assignee.display_name}.') body = ( f'{who} has requested a follow-up re-inspection ' - f'of "{inspection.template.name}" at {inspection.facility.name}.{note_suffix}' + f'of "{inspection.template.name}" at {inspection.facility.name}.' + f'{assigned_suffix}{note_suffix}' ) - # Notify the original inspector so they see it on the iPad. - inspector = db.session.get(User, inspection.inspector_id) + # Notify whoever now OWNS the follow-up — the assignee when one was named, + # otherwise the original inspector (Inspection.follow_up_owner). Notifying + # the original inspector for work that has been handed to someone else is + # noise, and worse, it implies they are expected to do it. + inspector = inspection.follow_up_owner if inspector and inspector.id != current_user.id: notify( recipient = inspector, @@ -1732,6 +1801,7 @@ def clear_followup(inspection_id): inspection.follow_up_note = None inspection.follow_up_requested_by = None inspection.follow_up_requested_at = None + inspection.follow_up_assigned_to = None db.session.commit() log_action(ACTION_UPDATE, 'Inspection', inspection_id, f'{inspection.template.name} @ {inspection.facility.name}', diff --git a/app/templates/inspections/view.html b/app/templates/inspections/view.html index 251eac2..a227841 100644 --- a/app/templates/inspections/view.html +++ b/app/templates/inspections/view.html @@ -413,6 +413,19 @@ {% if inspection.follow_up_requested_at %} {{ inspection.follow_up_requested_at.strftime('%b %d, %Y %I:%M %p') }} {% endif %} + {# Who is expected to DO it — the assignee when one was named, otherwise + the original inspector (Inspection.follow_up_owner). #} + {% if inspection.follow_up_owner %} +
+ Assigned to + {{ inspection.follow_up_owner.display_name }} + {% if not inspection.follow_up_assignee %} + (original inspector) + {% elif inspection.follow_up_owner.id == current_user.id %} + You + {% endif %} +
+ {% endif %} {% if inspection.follow_up_note %}
{{ inspection.follow_up_note }}{% endif %} {# Re-inspection is staff work — reinspect() already refuses customers. #} {% if current_user.role != 'customer' %} @@ -947,6 +960,38 @@ document.addEventListener('keydown', e => { if (e.key === 'Escape') closeMedia() + + {# ── Assign it (phase53) ──────────────────────────────────────── + 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/phase53_followup_assignee.py b/migrations/versions/phase53_followup_assignee.py new file mode 100644 index 0000000..53f299d --- /dev/null +++ b/migrations/versions/phase53_followup_assignee.py @@ -0,0 +1,68 @@ +"""phase53 — 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. + +INFORMATION_SCHEMA checks — safe to re-run. +""" + +revision = 'phase53_followup_assignee' +down_revision = 'phase52_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')