Aug 6 - Add external inspector role

2026-08-06 10:40:33 -04:00
parent 0830585ff5
commit c988f8cabb
32 changed files with 329 additions and 99 deletions
@@ -193,7 +193,13 @@ users: id, username (unique, indexed), full_name, email (unique, indexed),
       password_set, set_password_token (indexed), set_password_token_expires
 ```

-**Role ENUM:** `admin`, `director`, `inspector`, `project_manager`, `customer`, `auditor`
+**Role ENUM:** `admin`, `director`, `inspector`, `project_manager`, `customer`, `auditor`, `external_inspector`
+
+**`external_inspector` (Phase 49):** An inspector employed by **the customer or a third party** rather than by us. It has **exactly the same capabilities as `inspector`** and is scoped the **same way** — through `InspectorAssignment` rows resolved by `get_inspector_scope()`, i.e. an admin grants it the customer's contracts on the existing **Assign Contracts** page (`/auth/users/<id>/assign-contracts`, now gated on `user.is_inspector`). Strict scoping applies unchanged: no assignments = sees nothing.
+
+The two roles are distinguished by **display only**. `User.INSPECTOR_ROLES = ('inspector', 'external_inspector')` and the `User.is_inspector` property are the single definition — **every** capability/scoping check tests `is_inspector`, never `role == 'inspector'` (rule 87). `User.is_external_inspector` and `User.role_label` (backed by the `ROLE_LABELS` map) drive the "External" badges: users list, dashboard **Inspector Activity**, **Inspector Performance** report (HTML + the Excel export, where the name cell is suffixed `(External)` rather than gaining a column so the index-based cell styling stays correct), and every assignee dropdown (`(External)` suffix — issues create/update, issue-list quick-assign, inspection flag-issue).
+
+Assignable (rule 80 set becomes `director`/`inspector`/`external_inspector`/`auditor`, plus `project_manager` on the inspection flag-issue dropdown), included in Inspector Performance and Inspector Activity, and has **mobile-API access** — `external_inspector` is in the `_ALLOWED_ROLES` of every `app/api/*` module and falls into the inspector branch of every scoping check there. It gets its **own Notification Matrix column** (`external_inspector`), whose defaults mirror the Inspector column (see §11).

 **`auditor` (Phase 40):** A staff role with the **same access as `project_manager`** (it is included in `@project_manager_required` and everywhere `project_manager` is checked) **plus full issue-management powers** — create, assign, quick-assign, handler/vendor triage, request-verification, and verify/bulk-verify/verification-queue (via the new `@issue_manager_required` decorator). **Auditor does NOT get issue deletion** (that stays admin/director via `@supervisor_required`), nor any other admin/director-only area PM lacks (users, audit trail, notification matrix, customers, templates). Auditors are **assignable** as an issue/inspection assignee; **admin was removed** from the assignable set at the same time (assignee dropdowns are now `director`/`inspector`/`auditor`, plus `project_manager` on the inspection flag-issue dropdown). The issue-update route defensively keeps any pre-existing out-of-set assignee (e.g. a legacy admin assignment) in the dropdown so saving never silently unassigns. Auditor **has mobile-API access** — it is included in the `_ALLOWED_ROLES` set of every `app/api/*` module (comments, inspections, issues, photos, scheduled, stats, templates), so the iPad app accepts auditor logins. In every API endpoint that scopes by role, auditor falls into the non-inspector/non-customer (privileged) branch — org-wide data, same as admin/director/PM.

@@ -502,6 +508,10 @@ Management (`/scheduled-inspections/new|edit|delete`) is `@project_manager_requi
@project_manager_required  # role in ('admin', 'director', 'project_manager', 'auditor')
@issue_manager_required    # role in ('admin', 'director', 'auditor') — issue verification (NOT delete)
@customer_required         # role == 'customer' only
+
+# Not a decorator, but the same idea for the two inspector roles:
+#   user.is_inspector  → role in ('inspector', 'external_inspector')
+#   Never write `role == 'inspector'` for a capability or scoping check.
 ```

 ---
@@ -760,6 +770,10 @@ EVENT_SCHEDULED_INSPECTION = 'scheduled_inspection'   ← Phase 36
 EVENT_FOLLOWUP_REQUESTED   = 'followup_requested'     ← Phase 46
 ```

+### External Inspector column (Phase 49)
+
+`MATRIX_ROLES` gains `('external_inspector', 'External Inspector')`, and `notify_by_matrix()`'s `role_to_db` map routes it to the `external_inspector` DB role. `MATRIX_DEFAULTS` **mirrors** the Inspector column for every event (a comprehension, not 14 more literals) so a future event added for `inspector` automatically gets a matching external default. The `inspection_completed` scoping below applies to **both** inspector columns — without that, enabling the External column would notify every third-party inspector on every submission.
+
 ### Inspector role scoping for `inspection_completed`

 `notify_by_matrix()` special-cases the **inspector** role for the `inspection_completed` event: instead of notifying every active inspector, it notifies **only the inspection's own inspector** (`Inspection.inspector_id`, resolved from the passed `inspection_id`). So enabling the "Inspector" column for "Inspection completed" in the matrix alerts just the inspector who submitted that inspection — not the whole inspector pool. All three dispatch sites (web `routes/inspections.py`, both mobile-API `api/inspections.py`) pass `inspection_id`, so the scoping applies uniformly; if `inspection_id` is ever omitted for this event, the inspector role notifies no one (fail-closed). Other roles/events are unaffected.
@@ -888,7 +902,21 @@ phase1_projects_roles → phase6_features → phase7_mobile_api → phase8_notif
                                                                                    → phase44_sched_end_date
                                                                                        → phase45_sched_parent_insp
                                                                                            → phase46_followup_req_by
-                                                → phase47_sched_acknowledged   ← HEAD
+                                                → phase47_sched_acknowledged
+                                                    → phase48_user_ui_theme
+                                                        → phase49_external_inspector   ← HEAD
+
+#### phase49 — External Inspector role
+
+Revision id `phase49_external_inspector` (file `phase49_external_inspector_role.py`, down_revision `phase48_user_ui_theme` — note phase48, the design A/B test, is the real head, NOT phase47). Adds `external_inspector` to the `users.role` ENUM. **Pure ENUM expansion** (adds a value, migrates nothing), so the 3-step ENUM protocol does not apply and the `MODIFY` is idempotent — safe to re-run. `downgrade()` reassigns any `external_inspector` rows to `inspector` first, which preserves their `InspectorAssignment` scoping exactly.
+
+**No matrix rows are seeded.** `MATRIX_DEFAULTS` mirrors every `('<event>', 'inspector')` default into `('<event>', 'external_inspector')` at import time, and `is_enabled()` falls back to that default when a row is absent — so an un-seeded install behaves identically to the Inspector column until an admin saves the matrix page.
+
+**Deploy order:**
+```bash
+flask db upgrade            # expands users.role ENUM with 'external_inspector'
+sudo systemctl restart gunicorn
+```
 ```

 #### phase47 — scheduled inspection receipt acknowledgement
@@ -1457,6 +1485,7 @@ timeout = 30
 | 82 | **A schedule's recurrence columns must be CLEARED when they don't apply to the chosen frequency** | `_apply_recurrence()` in `routes/scheduled_inspections.py` is the single write path for `frequency` + `weekdays`/`month_mode`/`day_of_month`/`nth_week`/`nth_weekday`, and it NULLs the blocks that don't apply. Setting `sched.frequency` directly (as create/edit used to) leaves stale settings behind — a weekly→monthly switch would keep `weekdays` and `recurrence_label` would lie. The hidden form blocks still POST their values, so client-side hiding is not enough. |
 | 84 | **"Instructions" is a LABEL over `notes` — never rename the field, attribute, column or API key** | `ScheduledInspectionForm.notes` renders as "Instructions" and both the web execute page and the iPad say "Instructions". The wire key stays `notes` (`api/scheduled.py::_scheduled_payload`), which is what `APIScheduledInspection.notes` decodes into `LocalScheduledInspection.notes`; the iPad exposes it through a computed `instructions` accessor that also trims blank text. Renaming any of the storage identifiers would silently break the iPad decode — the field is `try?`-decoded, so it would fail to nil rather than throwing. |
 | 85 | **`next_due_date` is mutable state, `end_date` is a fixed boundary — never conflate them** | `fulfill()` rewrites `next_due_date` after every completed inspection; `end_date` is set by the manager and never touched by the app. The old single label "Start / Due Date" said both at once, which is what users reported as confusing. The label now follows context — `form.next_due_date.label.text` is set to "Start Date" in `create()` and "Next Due Date" in `edit()`. Do not rename the `next_due_date` column to match a label: it is indexed, it is the API payload key the iPad decodes, and the reminder cron filters on it. |
+| 87 | **Never write `role == 'inspector'` — use `user.is_inspector` (`User.INSPECTOR_ROLES`)** | phase49 added `external_inspector`, which must behave as an inspector everywhere. An equality check silently drops it into the *privileged* branch of every `if inspector: scope … else: org-wide` block — i.e. a third-party inspector would see **every contract in the system**. This is a fail-OPEN mistake: nothing errors, the data just leaks. The sweep converted ~44 Python sites and 7 template sites; the only surviving `== 'inspector'` literals are the matrix docstring, the `MATRIX_DEFAULTS` mirror comprehension, and the default-checked box in `admin/broadcast.html`. Query-level checks use `User.role.in_(User.INSPECTOR_ROLES)` (never `filter_by(role='inspector')`). A **new** `app/api/*` blueprint's `_ALLOWED_ROLES` must include `external_inspector`, same as rule 79 requires for `auditor`. |
 | 81 | **Photo timestamp/geo overlay is burned at UPLOAD, never on `PATCH /issues/<id>/photos`** | That PATCH receives only path strings — the bytes are already in storage and the payload carries no capture metadata. Burning there would need a read-modify-write per key plus an overwrite-in-place primitive (`storage.save()` mints a NEW uuid key, and §22 requires key == DB path), and would risk a **double burn** since the endpoint is deliberately idempotent/retry-safe (rule 45). Stamp in `POST /photos/upload`, where the raw bytes + EXIF are in hand and each call writes exactly one already-stamped object. Stamping failures must always fall back to storing the ORIGINAL bytes — never lose a photo to a stamping bug. See §23. |

 ---