Aug 27 - Update code to follow-up with ST functions
This commit is contained in:
@@ -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 `<select>` in the browser via `GET /inspections/facilities_for_project/<id>`. The routes receive and filter on `facility_id`; `contract_id` plays no role server-side. Do not add server-side `contract_id` filtering to these routes — it would duplicate what `facility_id` already provides. |
|
||||
|
||||
---
|
||||
@@ -1968,6 +1986,67 @@ copies the restrictions.
|
||||
|
||||
---
|
||||
|
||||
## 27c. Follow-up assignment + inspector read access (Aug 2026 ST parity)
|
||||
|
||||
Ported from ST (its phase53 + the two fixes shipped beside it).
|
||||
|
||||
### Assigning a follow-up (phase56)
|
||||
|
||||
A follow-up used to belong implicitly to whoever performed the original
|
||||
inspection. `inspections.follow_up_assigned_to` lets a director — or a
|
||||
**Customer Director**, for their own facilities — hand the re-inspection to
|
||||
someone else. `Inspection.follow_up_owner` (assignee *or* inspector) is the
|
||||
single definition of ownership, so the web display, the notification and the
|
||||
mobile API filter cannot disagree.
|
||||
|
||||
**The assignee takes over**: only the owner is notified, and only the owner
|
||||
sees it. In `GET /api/v1/inspections?follow_up_required=true` the two arms are
|
||||
mutually exclusive — without `is_(None)` on the second arm the original
|
||||
inspector keeps seeing a follow-up handed to someone else and two people turn
|
||||
up to do it. The generic "inspectors see only their own inspections" filter is
|
||||
**deferred** when follow-ups are requested, because an assigned follow-up lives
|
||||
on an inspection somebody else performed.
|
||||
|
||||
The picker (`_followup_assignees_for()`) is contract-scoped for the same reason
|
||||
the flag-issue list is (rule 105), offers only the two INSPECTOR roles, and the
|
||||
POST re-validates against it. A facility with no contract offers nobody —
|
||||
fail-closed, the follow-up stays with the original inspector.
|
||||
|
||||
**MT-only gap closed on the way:** MT's `GET /api/v1/inspections` had no
|
||||
`follow_up_required` filter at all, so the iPad's Follow-up Requests screen
|
||||
received the inspector's entire history. The filter now matches the web's
|
||||
definition of "follow-up" — flagged, completed, and not yet answered by a
|
||||
linked re-inspection (`~follow_ups.any()`).
|
||||
|
||||
### Inspector READ access follows the list, not authorship
|
||||
|
||||
`index()` scopes an inspector by FACILITY (rule 58), but `view()` and
|
||||
`export_pdf()` scoped by authorship — so the list offered rows that answered
|
||||
"Access denied" on click, and the follow-up assignee could not open the parent
|
||||
inspection they had just been asked to re-inspect. Both reads now use
|
||||
`_inspector_may_read()` (facility scope). **Writes stay owner-only**: `execute`,
|
||||
`save_draft_ajax`, `upload_photo_ajax` and `flag_issue` keep the authorship
|
||||
check. `reinspect()` belongs to the follow-up's owner; the buttons render only
|
||||
for `is_own_inspection or owns_follow_up`, so the page never shows a control
|
||||
that fails on click.
|
||||
|
||||
### One rule, two expressions, three callers
|
||||
|
||||
Ownership has to be stated twice — once for a loaded row, once in SQL — so both
|
||||
live together in `models/inspection.py`:
|
||||
|
||||
* `follow_up_owner` — the property (assignee, else inspector)
|
||||
* `follow_up_owned_by(user_id)` — the query predicate
|
||||
|
||||
Every query that scopes follow-ups calls the predicate: the mobile list filter,
|
||||
the web dashboard card, and the iPad stats KPI. They each used to write their
|
||||
own version and three tested AUTHORSHIP, so an assignee saw the work in their
|
||||
list while both dashboards read 0 — the stats KPI sitting directly above the
|
||||
Follow-up Requests list it disagreed with. Fixed in ST at the same time.
|
||||
Pinned by `tests/test_followup_ownership.py`.
|
||||
|
||||
---
|
||||
|
||||
## 28. Coding Rules for AI Assistants
|
||||
|
||||
These rules apply to every change made to this codebase, without exception.
|
||||
|
||||
+49
-2
@@ -228,6 +228,11 @@ def _inspection_payload(inspection):
|
||||
'inspector_notes': inspector_notes,
|
||||
# ── Follow-up / re-inspection fields ──────────────────────────────
|
||||
'follow_up_required': inspection.follow_up_required,
|
||||
# phase56 — 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),
|
||||
'follow_up_note': inspection.follow_up_note,
|
||||
'parent_inspection_id': inspection.parent_inspection_id,
|
||||
# ── Originating schedule (MT-14) ──────────────────────────────────
|
||||
@@ -257,6 +262,9 @@ def list_inspections():
|
||||
offset int default 0
|
||||
facility_id int filter by facility
|
||||
status str filter by status (completed, in_progress, flagged)
|
||||
follow_up_required
|
||||
bool 'true'/'1' — only inspections awaiting a re-inspection,
|
||||
scoped to the caller's own follow-ups (see below)
|
||||
from_date str ISO date (YYYY-MM-DD) — include inspections on/after this date
|
||||
to_date str ISO date (YYYY-MM-DD) — include inspections on/before this date
|
||||
|
||||
@@ -282,8 +290,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 (phase56), 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
|
||||
@@ -295,6 +310,38 @@ def list_inspections():
|
||||
if status:
|
||||
query = query.filter(Inspection.status == status)
|
||||
|
||||
if wants_follow_ups:
|
||||
# MT had no follow_up_required filter at all, so the iPad's Follow-up
|
||||
# Requests screen — which calls ?follow_up_required=true — received the
|
||||
# inspector's ENTIRE history and presented it as outstanding requests.
|
||||
#
|
||||
# "Follow-up" must mean exactly what it means everywhere on the web
|
||||
# (inspections.index / reports status_filter == 'follow_up'): flagged,
|
||||
# completed, and not yet answered by a linked re-inspection.
|
||||
#
|
||||
# The ~follow_ups.any() clause is the one that matters. The web execute
|
||||
# route never clears follow_up_required on the parent — it only stops
|
||||
# listing it once a child exists — so filtering on the flag alone would
|
||||
# return follow-ups that were already satisfied on the web, forever.
|
||||
# On the iPad those rows are undismissable: pull_follow_up_requests()
|
||||
# keeps receiving them and update(from:) resets fulfilledLocally, so the
|
||||
# FOLLOW-UP REQUESTED card would never clear. (The mobile POST path does
|
||||
# clear the parent flag, so only web-completed re-inspections stick.)
|
||||
query = query.filter(
|
||||
Inspection.follow_up_required.is_(True),
|
||||
Inspection.status == 'completed',
|
||||
).filter(~Inspection.follow_ups.any())
|
||||
|
||||
# Ownership, not authorship (phase56). 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(Inspection.follow_up_owned_by(user.id))
|
||||
|
||||
from_date_str = request.args.get('from_date')
|
||||
if from_date_str:
|
||||
try:
|
||||
|
||||
+21
-1
@@ -79,16 +79,27 @@ def upload_photo():
|
||||
if user.role not in _ALLOWED_ROLES:
|
||||
return api_error('Access denied', 403)
|
||||
|
||||
# Every rejection below is logged at WARNING with the user. Only SUCCESSES
|
||||
# were logged before, so when an inspector's photos failed repeatedly there
|
||||
# was nothing server-side to explain why — and a photo that exhausts its
|
||||
# upload attempts costs the inspection its evidence (see the iPad's
|
||||
# PendingPhoto.lastUploadError for the device half of this).
|
||||
if 'file' not in request.files:
|
||||
logger.warning('API PHOTOS | rejected | reason=no_file_part | user=%s',
|
||||
user.username)
|
||||
return api_error('No file provided', 400)
|
||||
|
||||
file_obj = request.files['file']
|
||||
entity_type = request.form.get('entity_type', 'inspection')
|
||||
|
||||
if not file_obj or not file_obj.filename:
|
||||
logger.warning('API PHOTOS | rejected | reason=empty_file | user=%s',
|
||||
user.username)
|
||||
return api_error('Empty file', 400)
|
||||
|
||||
if not _allowed_file(file_obj.filename):
|
||||
logger.warning('API PHOTOS | rejected | reason=bad_extension | file=%r | user=%s',
|
||||
file_obj.filename, user.username)
|
||||
return api_error(
|
||||
f'File type not allowed. Accepted: {", ".join(sorted(_ALLOWED_EXTENSIONS))}',
|
||||
400
|
||||
@@ -120,7 +131,16 @@ def upload_photo():
|
||||
# stamped FileStorage keeps the original filename, so the derived key — and
|
||||
# the tenant prefix applied inside S3Backend — are unaffected.
|
||||
from app.utils import storage
|
||||
server_path = storage.save(file_obj, subfolder)
|
||||
try:
|
||||
server_path = storage.save(file_obj, subfolder)
|
||||
except Exception as exc:
|
||||
# A storage failure is the most likely cause of a REPEATED upload
|
||||
# failure (disk full, R2 credentials/quota). Name it explicitly —
|
||||
# otherwise it surfaces only as a generic 500 with no link to the
|
||||
# inspector who is losing evidence photos.
|
||||
logger.error('API PHOTOS | STORAGE WRITE FAILED | user=%s | entity_type=%s | '
|
||||
'subfolder=%s | error=%s', user.username, entity_type, subfolder, exc)
|
||||
return api_error('Could not store the photo. Please retry.', 500)
|
||||
|
||||
logger.info(
|
||||
'API PHOTOS | uploaded | entity_type=%s | path=%s | user=%s | '
|
||||
|
||||
+6
-1
@@ -152,9 +152,14 @@ def dashboard_stats():
|
||||
if not fids:
|
||||
followup_q = followup_q.filter(False)
|
||||
else:
|
||||
# OWNERSHIP, not authorship: a follow-up handed to this inspector
|
||||
# belongs to them even though somebody else performed the original.
|
||||
# This tile sits directly above the Follow-up Requests list, which
|
||||
# filters the same way — counting authorship here made the two
|
||||
# disagree on the same screen.
|
||||
followup_q = followup_q.filter(
|
||||
Inspection.facility_id.in_(fids),
|
||||
Inspection.inspector_id == user.id,
|
||||
Inspection.follow_up_owned_by(user.id),
|
||||
)
|
||||
pending_followups = followup_q.count()
|
||||
|
||||
|
||||
@@ -207,6 +207,25 @@ class Inspection(db.Model):
|
||||
)
|
||||
follow_up_requested_at = db.Column(db.DateTime, nullable=True)
|
||||
|
||||
# phase56 — 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: 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 — the app
|
||||
# starts cleanly and then every request 500s.
|
||||
follow_up_assigned_to = db.Column(
|
||||
db.Integer,
|
||||
db.ForeignKey('users.id', ondelete='SET NULL',
|
||||
name='fk_inspections_followup_assignee'),
|
||||
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')
|
||||
follow_ups = db.relationship('Inspection', backref=db.backref('parent', remote_side='Inspection.id'),
|
||||
@@ -215,6 +234,45 @@ class Inspection(db.Model):
|
||||
# users.id, so SQLAlchemy cannot infer which column this relationship uses.
|
||||
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
|
||||
|
||||
@staticmethod
|
||||
def follow_up_owned_by(user_id):
|
||||
"""SQL predicate: *user_id* owns this inspection's follow-up.
|
||||
|
||||
The query-side mirror of `follow_up_owner` above. Ownership has to be
|
||||
expressed twice — once for a loaded row, once in SQL — so both live
|
||||
here, together, and every caller uses one of them.
|
||||
|
||||
The two arms are mutually exclusive on purpose. Drop the `is_(None)`
|
||||
from the second and an inspector keeps matching a follow-up that was
|
||||
handed to someone else: two people turn up for the same re-inspection.
|
||||
|
||||
Callers: the mobile list filter, the web dashboard card, and the iPad
|
||||
stats KPI. They previously each wrote their own version, and three of
|
||||
them tested AUTHORSHIP — so an assignee saw the work in their list but
|
||||
a 0 on both dashboards.
|
||||
"""
|
||||
return db.or_(
|
||||
Inspection.follow_up_assigned_to == user_id,
|
||||
db.and_(
|
||||
Inspection.follow_up_assigned_to.is_(None),
|
||||
Inspection.inspector_id == user_id,
|
||||
),
|
||||
)
|
||||
|
||||
# The schedule this inspection was started from / materialised by, so the
|
||||
# detail view can show the cadence and who set it up. Explicit foreign_keys
|
||||
# again: inspection_schedules.parent_inspection_id points back here (phase48),
|
||||
|
||||
@@ -175,9 +175,13 @@ def index():
|
||||
if not inspector_facility_ids:
|
||||
followup_q = followup_q.filter(False)
|
||||
else:
|
||||
# OWNERSHIP, not authorship — see Inspection.follow_up_owned_by().
|
||||
# An assigned follow-up lives on an inspection somebody else
|
||||
# performed, so testing inspector_id made the card read 0 for the
|
||||
# very person who had been asked to do the work.
|
||||
followup_q = followup_q.filter(
|
||||
Inspection.facility_id.in_(inspector_facility_ids),
|
||||
Inspection.inspector_id == current_user.id,
|
||||
Inspection.follow_up_owned_by(current_user.id),
|
||||
)
|
||||
elif is_customer:
|
||||
if customer_facility_ids:
|
||||
|
||||
+139
-5
@@ -786,7 +786,11 @@ def view(inspection_id):
|
||||
if inspection is None:
|
||||
abort(404)
|
||||
|
||||
if current_user.is_inspector and inspection.inspector_id != current_user.id:
|
||||
# Read access matches the LIST (rule 58) — an inspector may open anything
|
||||
# at their contracted facilities, not only what they performed. Editing
|
||||
# someone else's inspection is still refused (execute / save-draft /
|
||||
# upload-photo / flag-issue keep the authorship check).
|
||||
if current_user.is_inspector and not _inspector_may_read(inspection, current_user):
|
||||
flash('Access denied.', 'danger')
|
||||
return redirect(url_for('inspections.index'))
|
||||
if current_user.role == 'customer':
|
||||
@@ -983,7 +987,23 @@ def view(inspection_id):
|
||||
'unchanged': sum(1 for r in rows if r['delta'] == 0),
|
||||
}
|
||||
|
||||
followup_assignees = _followup_assignees_for(inspection, current_user)
|
||||
# An inspector viewing SOMEBODY ELSE's inspection gets a read-only page.
|
||||
# Without this the buttons would all render and then fail on click — the
|
||||
# same list-says-yes / page-says-no mismatch this change removes.
|
||||
is_own_inspection = (not current_user.is_inspector
|
||||
or inspection.inspector_id == current_user.id)
|
||||
# Whoever is expected to carry out the follow-up (phase56) may start the
|
||||
# re-inspection even though the original inspection is not theirs.
|
||||
owns_follow_up = bool(
|
||||
inspection.follow_up_required
|
||||
and inspection.follow_up_owner
|
||||
and inspection.follow_up_owner.id == current_user.id
|
||||
)
|
||||
return render_template('inspections/view.html',
|
||||
followup_assignees=followup_assignees,
|
||||
is_own_inspection=is_own_inspection,
|
||||
owns_follow_up=owns_follow_up,
|
||||
inspection=inspection,
|
||||
form_fields=form_fields,
|
||||
form_data=form_data,
|
||||
@@ -1325,7 +1345,11 @@ def export_pdf(inspection_id):
|
||||
if inspection is None:
|
||||
abort(404)
|
||||
|
||||
if current_user.is_inspector and inspection.inspector_id != current_user.id:
|
||||
# Read access matches the LIST (rule 58) — an inspector may open anything
|
||||
# at their contracted facilities, not only what they performed. Editing
|
||||
# someone else's inspection is still refused (execute / save-draft /
|
||||
# upload-photo / flag-issue keep the authorship check).
|
||||
if current_user.is_inspector and not _inspector_may_read(inspection, current_user):
|
||||
flash('Access denied.', 'danger')
|
||||
return redirect(url_for('inspections.index'))
|
||||
if current_user.role == 'customer':
|
||||
@@ -1410,6 +1434,68 @@ def _view_url(inspection_id):
|
||||
return url_for('inspections.view', inspection_id=inspection_id)
|
||||
|
||||
|
||||
def _inspector_may_read(inspection, user):
|
||||
"""May this inspector OPEN someone else's inspection?
|
||||
|
||||
Yes, when it happened at a facility on one of their contracts — the same
|
||||
scope `index()` uses (rule 58: an inspector's scope covers all data in their
|
||||
contracted facilities, not just their own work).
|
||||
|
||||
This used to test authorship instead, and the two disagreed: the list
|
||||
showed every inspection at the inspector's facilities, then clicking one
|
||||
said "Access denied". It also blocked the phase56 follow-up assignee from
|
||||
opening the parent inspection they had just been asked to re-inspect —
|
||||
the button they needed was on a page they could not reach.
|
||||
|
||||
READ only. Editing someone else's inspection is still refused: execute,
|
||||
save-draft, upload-photo and flag-issue all keep the authorship check.
|
||||
"""
|
||||
fids = get_inspector_scope(user)
|
||||
if fids is None: # not an inspector — no scoping applies here
|
||||
return True
|
||||
return bool(fids) and inspection.facility_id in fids
|
||||
|
||||
|
||||
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.
|
||||
|
||||
@@ -1613,6 +1699,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()
|
||||
@@ -1678,22 +1765,48 @@ def flag_followup(inspection_id):
|
||||
|
||||
note = request.form.get('follow_up_note', '').strip() or None
|
||||
|
||||
# ── Assignee (phase56) ────────────────────────────────────────────────
|
||||
# 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,
|
||||
@@ -1752,6 +1865,7 @@ def clear_followup(inspection_id):
|
||||
# whoever raised the previous one.
|
||||
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}',
|
||||
@@ -1776,6 +1890,26 @@ def reinspect(inspection_id):
|
||||
flash('Access denied.', 'danger')
|
||||
return redirect(url_for('inspections.index'))
|
||||
|
||||
# An inspector may re-inspect their OWN work, or work they have been
|
||||
# handed the follow-up for (phase56). Anything else at a contracted
|
||||
# facility is readable but not theirs to redo — starting a re-inspection
|
||||
# of a colleague's inspection uninvited only creates confusion about who
|
||||
# is doing it.
|
||||
if current_user.is_inspector:
|
||||
if parent.follow_up_required and parent.follow_up_owner:
|
||||
# A live follow-up has exactly ONE owner (phase56). Even the
|
||||
# original inspector does not start it once it has been handed to
|
||||
# someone else — that is the whole point of assigning it, and two
|
||||
# people turning up is the failure being designed out.
|
||||
may = parent.follow_up_owner.id == current_user.id
|
||||
else:
|
||||
# No follow-up outstanding: re-inspecting your own work is fine,
|
||||
# someone else's is not yours to redo uninvited.
|
||||
may = parent.inspector_id == current_user.id
|
||||
if not may:
|
||||
flash('That re-inspection has been assigned to someone else.', 'warning')
|
||||
return redirect(_view_url(inspection_id))
|
||||
|
||||
session['reinspect_parent_id'] = parent.id
|
||||
session['reinspect_template_id'] = parent.template_id
|
||||
session['reinspect_facility_id'] = parent.facility_id
|
||||
|
||||
@@ -359,7 +359,12 @@
|
||||
<button onclick="window.print()" class="btn btn-sm btn-outline-secondary">
|
||||
<i class="bi bi-printer"></i> Print
|
||||
</button>
|
||||
{% if current_user.role not in ['customer'] %}
|
||||
{# Offered to managers, to the inspector who did this inspection, and to
|
||||
whoever the follow-up was assigned to (phase56). Not to any other
|
||||
inspector who can merely SEE it: reinspect() refuses them, and showing
|
||||
a button that fails on click is the mismatch this page just fixed. #}
|
||||
{% if current_user.role not in ['customer']
|
||||
and (is_own_inspection or owns_follow_up) %}
|
||||
<a href="{{ url_for('inspections.reinspect', inspection_id=inspection.id) }}"
|
||||
class="btn btn-sm btn-outline-primary"
|
||||
title="Start a follow-up re-inspection with the same template and facility">
|
||||
@@ -420,9 +425,24 @@
|
||||
{% if inspection.follow_up_requested_at %}
|
||||
<span class="small text-muted ms-1">{{ inspection.follow_up_requested_at.strftime('%b %d, %Y %I:%M %p') }}</span>
|
||||
{% 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 %}
|
||||
<div class="small mt-1">
|
||||
<i class="bi bi-person-check me-1"></i>Assigned to
|
||||
<strong>{{ inspection.follow_up_owner.display_name }}</strong>
|
||||
{% if not inspection.follow_up_assignee %}
|
||||
<span class="text-muted">(original inspector)</span>
|
||||
{% elif inspection.follow_up_owner.id == current_user.id %}
|
||||
<span class="badge bg-warning text-dark ms-1">You</span>
|
||||
{% endif %}
|
||||
</div>
|
||||
{% endif %}
|
||||
{% if inspection.follow_up_note %}<br><span class="small">{{ inspection.follow_up_note }}</span>{% endif %}
|
||||
{# Re-inspection is staff work — reinspect() already refuses customers. #}
|
||||
{% if current_user.role != 'customer' %}
|
||||
{# Re-inspection is staff work — reinspect() already refuses customers —
|
||||
and among inspectors it belongs to the follow-up's OWNER. #}
|
||||
{% if current_user.role != 'customer'
|
||||
and (is_own_inspection or owns_follow_up) %}
|
||||
<div class="mt-2">
|
||||
<a href="{{ url_for('inspections.reinspect', inspection_id=inspection.id) }}"
|
||||
class="btn btn-sm btn-warning">
|
||||
@@ -956,6 +976,38 @@ document.addEventListener('keydown', e => { if (e.key === 'Escape') closeMedia()
|
||||
</label>
|
||||
<textarea name="follow_up_note" class="form-control" rows="3"
|
||||
placeholder="Describe what needs to be addressed in the follow-up inspection…"></textarea>
|
||||
|
||||
{# ── 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 %}
|
||||
<div class="mt-3">
|
||||
<label class="form-label fw-semibold">
|
||||
Assign to
|
||||
<span class="text-muted small">(optional)</span>
|
||||
</label>
|
||||
<select name="follow_up_assigned_to" class="form-select">
|
||||
<option value="">
|
||||
— {{ inspection.inspector.display_name }} (original inspector) —
|
||||
</option>
|
||||
{% for u in followup_assignees %}
|
||||
{% if u.id != inspection.inspector_id %}
|
||||
<option value="{{ u.id }}">
|
||||
{{ u.display_name }}{{ ' (Customer)' if u.is_external_inspector }}
|
||||
</option>
|
||||
{% endif %}
|
||||
{% endfor %}
|
||||
</select>
|
||||
<div class="form-text">
|
||||
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.
|
||||
</div>
|
||||
</div>
|
||||
{% endif %}
|
||||
</div>
|
||||
<div class="modal-footer">
|
||||
<button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Cancel</button>
|
||||
|
||||
@@ -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')
|
||||
@@ -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}')
|
||||
Reference in New Issue
Block a user