From 98c0dcb7ccd0b4b8caae639ecb39507ad2bec3c7 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Mon, 17 Aug 2026 15:55:17 -0400 Subject: [PATCH] Aug 17 - Update forms will be assigned per contract, edit template page fix --- CLAUDE.md | 13 +++++++- app/routes/templates.py | 37 ++++++++++++++++++++--- app/templates/templates/list.html | 50 ++++++++++++++++++++++++++++++- 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ef1bba3..fbde703 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -335,7 +335,17 @@ template_contracts: id, template_id (FK→inspection_templates CASCADE, indexed) `InspectionTemplate` helpers: `contract_ids`, `is_shared`, `available_for_project(project_id)`, `set_contracts([ids])` (does **not** commit), and the static **`available_query(project_id)`** — the single definition of "which forms may this contract use", used by every picker, by the POST validation behind it, and by the mobile API, so they cannot disagree. A facility with **no** contract can only use shared forms (fail-closed). -Managed on the template create/edit pages via an "Available on contracts" multi-select (admin/director); the template list shows a **Shared** badge or one badge per contract. +Managed by admin/director in **three** places, because the template screens have three separate edit paths — all must keep the picker or a form silently stays shared: + +| Where | Route | Notes | +|---|---|---| +| **Edit Template modal** on the template list | `POST /templates//rename` | The one most people actually use. Posts a hidden `contracts_present=1` marker so an empty selection means "make it shared"; a POST **without** the marker (an older client, or another caller of this route) leaves the existing restrictions untouched rather than wiping them. Ids are validated against active contracts. | +| Create Template | `POST /templates/new` | | +| Full form editor | `POST /templates//edit` | `obj=` cannot read association rows, so the multi-select is seeded from `contract_ids` on GET. | + +`duplicate_template()` copies the restrictions across — duplicating a customer's bespoke form must not yield a copy shared with everyone. + +The template list shows a **Shared** badge or one badge per contract. ### Notification / NotificationPreference @@ -1662,6 +1672,7 @@ timeout = 30 | 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`. | | 88 | **`app/enrollment/` writes no DB row and has exactly ONE read — keep the vertical slice sealed** | The enrollment form describes accounts that do NOT exist yet (no contract, facility or user to key a row against), so it stores flat JSON in `ENROLLMENT_DIR` and owns its own templates. The single permitted model access is `mailer._admin_recipients()` reading active `admin` users to address the new-enrollment alert — function-local, read-only, and guarded so a DB failure cannot break a submission. Adding a model/migration for enrollment, or letting the public POST **create** Users, would couple an unauthenticated endpoint to the account system — the exact thing the separation buys. If enrollment must ever provision accounts, do it as a separate admin-triggered action that reads a stored submission. Submission ids are filesystem paths: validate against `_ID_RE` before every open (path traversal). See §24. | +| 97 | **The template list's Edit modal (`/rename`) is a THIRD edit path — keep it in sync with create and the form editor** | The modal on the template list posts to `rename_template`, not `edit_template`, so a field added only to the two WTForms pages is invisible to the people who edit templates from the list. It carries a hidden `contracts_present=1` marker: an empty selection with the marker means "make this shared", while a POST without it leaves restrictions untouched — otherwise any other caller of that route would silently share a restricted form with every customer. | | 95 | **A template with NO `template_contracts` rows is SHARED, not hidden** | The empty set means "available on every contract" — that is what makes phase52 additive and why it needed no backfill. Reading it the other way would hide every pre-phase52 form from every contract at once. The convention lives in exactly one place, `InspectionTemplate.available_query()`; every picker, the POST validation behind it, and the mobile API call it rather than writing their own filter. A facility with no contract gets shared forms only (fail-closed). | | 96 | **An explicit `?project_id=` / `?facility_id=` filter must still be intersected with the caller's own scope** | Accepting a caller-supplied contract filter *instead of* their scope is a leak, not a filter: a Customer Inspector could pass another customer's facility id and get that customer's form names back. `_visible_templates()` returns `[]` for an out-of-scope contract — empty rather than an error, so the endpoint does not confirm the contract exists either. Applies to any future endpoint that takes a scope-shaped query parameter. | | 93 | **The flag-issue assignee list is contract-scoped, and BOTH call sites must use `_assignable_staff_for()`** | `execute()` renders the dropdown, `flag_issue()` builds the choices that validate the POST — the choices are the security boundary. Two separate queries had already drifted (offcanvas offered project_manager/auditor, choices rejected them), which silently discarded issues. Contract scoping applies to the two inspector roles for EVERY actor, not just customer ones: an org-wide list let anyone assign another client's Customer Inspector, who was then emailed that facility's name and issue description. Never widen this back to an unscoped `User.query.filter(role.in_(...))`. | diff --git a/app/routes/templates.py b/app/routes/templates.py index 238b7f1..a25c661 100644 --- a/app/routes/templates.py +++ b/app/routes/templates.py @@ -33,7 +33,12 @@ def _populate_contract_choices(form): @login_required def index(): templates = InspectionTemplate.query.order_by(InspectionTemplate.name).all() - return render_template('templates/list.html', templates=templates) + # Contract options for the Edit Template modal's "Available on contracts" + # picker (phase52) — this modal is the edit UI reached from the list, so it + # needs the same control the full editor has. + contracts = Project.query.filter_by(active=True).order_by(Project.name).all() + return render_template('templates/list.html', + templates=templates, contracts=contracts) @bp.route('/new', methods=['GET', 'POST']) @@ -144,11 +149,29 @@ def rename_template(template_id): template.description = request.form.get('description', '').strip() or None template.frequency = new_frequency + # phase52 — contract restrictions are edited from this modal too, since it + # is the Edit Template dialog people actually reach from the list. The + # hidden marker distinguishes "the form posted an empty selection" (make + # the template shared) from "the form has no contracts field at all", which + # must leave the existing restrictions untouched rather than silently + # sharing the template with every customer. + if request.form.get('contracts_present') == '1': + valid_pids = { + p.id for p in Project.query.filter_by(active=True).all() + } + posted = { + pid for pid in request.form.getlist('contract_ids', type=int) + if pid in valid_pids + } + template.set_contracts(posted) + db.session.commit() - logger.info('TEMPLATES | rename | user=%s | template_id=%s name=%r', - current_user.username, template.id, template.name) + logger.info('TEMPLATES | rename | user=%s | template_id=%s name=%r contracts=%s', + current_user.username, template.id, template.name, + template.contract_ids or 'shared') log_action(ACTION_UPDATE, 'Template', template.id, template.name, - f'frequency={template.frequency}; via=rename') + f'frequency={template.frequency}; ' + f'contracts={template.contract_ids or "shared"}; via=rename') flash(f'Template "{template.name}" updated successfully.', 'success') return redirect(url_for('templates.index')) @@ -212,6 +235,12 @@ def duplicate_template(template_id): db.session.add(new_tpl) db.session.flush() # get new_tpl.id before committing + # phase52 — carry the contract restrictions across. Duplicating a + # customer's bespoke form must not produce a copy that is silently shared + # with every other customer; copying a shared form still yields a shared + # one (no links to copy). + new_tpl.set_contracts(src.contract_ids) + # Duplicate all checklist items for item in src.checklist_items.order_by(ChecklistItem.display_order).all(): new_item = ChecklistItem( diff --git a/app/templates/templates/list.html b/app/templates/templates/list.html index 6d383ef..1b57fc4 100644 --- a/app/templates/templates/list.html +++ b/app/templates/templates/list.html @@ -74,6 +74,7 @@ data-template-name="{{ template.name }}" data-template-description="{{ template.description or '' }}" data-template-frequency="{{ template.frequency or 'daily' }}" + data-template-contracts="{{ template.contract_ids|join(',') }}" title="Edit template details"> Edit @@ -160,7 +161,7 @@ placeholder="Optional description"> -
+
+ + {# phase52 — which contracts may use this form. The hidden + marker tells the route this modal really did include the + field, so an empty selection means "share it" rather + than "no field was posted, leave it alone". #} +
+ + + +
+ Leave nothing selected to share this form with + every contract (this is how all existing forms are set). + Select one or more contracts to restrict it to them — it is + then hidden from every other customer, on the web and in the + iPad app. Ctrl/Cmd-click to select several. +
+ +