From e9005b9b9c755575b101f247b0eff8e4011906fa Mon Sep 17 00:00:00 2001 From: NguyenND Date: Fri, 4 Sep 2026 16:53:53 -0400 Subject: [PATCH] Sep 4 - Add link relavant issues function --- CLAUDE.md | 120 +- app/api/inspections.py | 4 +- app/api/issues.py | 4 +- app/api/notifications.py | 2 +- app/api/scheduled.py | 4 +- app/api/stats.py | 9 +- app/models/issue.py | 143 +++ app/routes/dashboard.py | 14 +- app/routes/inspections.py | 6 +- app/routes/issues.py | 283 ++++- app/routes/reports.py | 38 +- app/templates/issues/view.html | 265 +++++ app/utils/scope.py | 36 +- app/utils/sla.py | 35 +- config.py | 36 + migrations/versions/phase57_issue_links.py | 79 ++ scripts/db_health.py | 1161 ++++++++++++++++++++ tests/test_issue_links.py | 501 +++++++++ tests/test_scoped_pages_smoke.py | 341 ++++++ tests/test_sla_candidate_query.py | 199 ++++ 20 files changed, 3208 insertions(+), 72 deletions(-) create mode 100644 migrations/versions/phase57_issue_links.py create mode 100644 scripts/db_health.py create mode 100644 tests/test_issue_links.py create mode 100644 tests/test_scoped_pages_smoke.py create mode 100644 tests/test_sla_candidate_query.py diff --git a/CLAUDE.md b/CLAUDE.md index acac20e..85b9d62 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,6 +35,7 @@ 25. [Health Dashboard](#25-health-dashboard-health-on-panel) 26. [Coding Rules for AI Assistants](#26-coding-rules-for-ai-assistants) 29. [Photo Object Storage (R2)](#29-photo-object-storage-r2) +30. [Database Health Check](#30-database-health-check-scriptsdb_healthpy) --- @@ -224,6 +225,8 @@ lt_janitorial_quality_control/ | `REDIS_URL` | Optional. When set, Flask-Limiter uses Redis for shared rate-limit counters across Gunicorn workers. | | `GROQ_API_KEY` | Optional. When set, enables the AI chatbot at `/support/chat`. Absent → chat input disabled; customers see a "Submit to Support" fallback only. | | `GROQ_MODEL` | Optional. Groq model ID. Defaults to `llama-3.3-70b-versatile`. | +| `DB_POOL_RECYCLE` | Optional, default `1800` (seconds). Retires a pooled connection on the **default bind** after this long. **Must stay below the server's `wait_timeout`** or MySQL closes the socket first and the next request gets `OperationalError 2006`. `scripts/db_health.py` cross-checks the two. Per-tenant engines use `TENANT_ENGINE_POOL_RECYCLE` instead. | +| `DB_POOL_SIZE` / `DB_MAX_OVERFLOW` | Optional, default `5` / `5`. Per-**worker** pool on the default bind. In MT the real ceiling is `workers x [ (default pool) + TENANT_ENGINE_CACHE_MAX x (tenant pool) ]` — the library defaults (5+10) alone put a 9-worker box at 135 against a `max_connections` of 151, before any tenant engine is counted. | | `MULTI_TENANT_ENABLED` | `false` by default. Set `true` to activate Host→tenant routing. Requires all control-plane vars below. | | `CONTROL_DATABASE_URL` | Control-plane MySQL URI, e.g. `mysql+pymysql://jqc_control:pw@127.0.0.1/jqc_control`. Required when `MULTI_TENANT_ENABLED=true`. | | `CONTROL_FERNET_KEY` | Fernet key for encrypting tenant DB passwords. Generate once; store in `/etc/jqc/control.env`. | @@ -363,6 +366,28 @@ notifications: id, user_id, title, body, link, is_read, created_at, issue_id, notification_preferences: id, user_id, event_type, email_enabled, digest_mode, digest_frequency ``` +### IssueLink (phase57) + +``` +issue_links: id, issue_id (FK→issues CASCADE, indexed), + linked_issue_id (FK→issues CASCADE, indexed), + link_type ENUM('duplicate','related') NOT NULL DEFAULT 'related', + created_by (FK→users SET NULL), created_at + UniqueConstraint(issue_id, linked_issue_id) — uq_issue_link +``` + +**Connects a duplicate to its original, or two issues about the same thing**, so whoever picks one up can reach the other. + +**One row is stored per pair and shown on BOTH issues.** The stored direction carries meaning for `duplicate` — `issue_id` is a duplicate *of* `linked_issue_id` — so the same row reads differently at each end: "Duplicate of #B" on one, "Duplicated by #A" on the other. `related` is symmetric and reads "Related to" from either side. `IssueLink.LABELS` is keyed `(link_type, is_source)` and is the only place that wording lives; `label_for(viewing_issue_id)` / `other_issue(viewing_issue_id)` resolve a row against whichever issue is on screen. + +Storing one row rather than a mirrored pair keeps the direction unambiguous and makes unlinking a single delete. The cost: **uniqueness cannot be expressed by the UniqueConstraint alone.** `(A,B)` and `(B,A)` are distinct rows to MySQL but the same link to a person, so **`IssueLink.exists_between(a, b)` is the only correct duplicate check** — it looks both ways. The constraint catches the exact-duplicate row; `exists_between()` catches the reverse. + +**A link is PURELY NAVIGATIONAL.** Marking a duplicate does **not** touch either issue's status, `resolved_at`, SLA, assignee or followers, and fires no notification. Closing the duplicate stays a separate, deliberate action. Do not add side effects here without saying so in the UI — the link control reads as navigation, and a status write from it would be invisible. + +**Two FKs from one table to `issues`, so both relationships pin `foreign_keys`** — the same failure mode as phase56's third `inspections`→`users` FK (§17), which raises on first ORM *use*, not at import. `Issue.links_from` / `Issue.links_to` are the two storage directions; **`Issue.all_links()` merges them** into the single list a person actually sees. Both relationships cascade `all, delete-orphan` (and both FKs are `ON DELETE CASCADE`), so deleting an issue takes its links from *either* end — a surviving link would render a dead row on the other issue's page. + +**Nothing here is tenant-aware, deliberately.** The table lives in the tenant DB and routes through `RoutingSession`, so a link can only reach an issue in the same tenant. Scope *within* a tenant is the caller's job — **see rule 110.** + ### IssueComment ``` @@ -565,7 +590,7 @@ The `DeviceRegistration` model and the duplicate `api_devices` blueprint were ** | `customers` | `/customers` | list, invite, set-password, manage, import CSV | | `inspections` | `/inspections` | list, start, execute, view, PDF export, flag-issue, save-draft (AJAX), flag-followup, reinspect, upload-photo (AJAX) | | `templates` | `/templates` | list, create, edit, delete, form editor, preview | -| `issues` | `/issues` | list, view, create, update, verify, comment, follow/unfollow, verification queue, bulk-verify, delete, quick-assign | +| `issues` | `/issues` | list, view, create, update, verify, comment, follow/unfollow, verification queue, bulk-verify, delete, quick-assign, **issue links** (`POST //links` add, `POST //links//delete` remove, `GET //link-search` scoped JSON picker — phase57) | | `notifications` | `/notifications` | list, mark-read, preferences, send-digest (cron), check-sla (cron), cleanup-tokens (cron), trial-reminders (cron), dunning-reminders (cron) | | `audit` | `/audit` | list (admin only), view, purge | | `reports` | `/reports` | index, facility report, scorecard, CSV/PDF/Excel export, issues-aging, sla-compliance, followup-closure, facility summary PDF | @@ -843,7 +868,7 @@ limiter = Limiter( ## 17. Alembic Migration Chain -**Current HEAD:** `phase56_followup_assignee`. +**Current HEAD:** `phase57_issue_links`. **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`. @@ -885,7 +910,7 @@ limiter = Limiter( → phase50_sched_acknowledged → phase51_external_inspector → phase52_user_ui_theme → phase53_knowledge_sort_order → phase54_user_notif_matrix → phase55_template_contracts - → phase56_followup_assignee ← HEAD + → phase56_followup_assignee → phase57_issue_links ← HEAD ``` `phase54` / `phase55` port the ST August-2026 work (ST calls them phase51 / @@ -923,6 +948,33 @@ 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. +#### phase57 — link related and duplicate issues + +Creates `issue_links` (§5 `IssueLink`). **Purely additive** — nothing reads the +table until a person creates a link, so an empty table is exactly today's +behaviour and there is nothing to backfill. + +Both issue FKs are `ON DELETE CASCADE`, so a direct SQL delete of an issue +cannot leave a link pointing at a row that no longer exists (the ORM cascade on +`links_from`/`links_to` covers the application path). The index names +deliberately match what SQLAlchemy's `index=True` generates — +`ix_issue_links_issue_id`, `ix_issue_links_linked_issue_id` — which matters more +in MT than in ST: a tenant **bootstrapped** from the baseline and one +**upgraded** through the chain must end up with the same schema, down to the +index names. + +Table-existence check — safe to re-run, as every migration from phase33 on must +be. `downgrade()` drops the table, discarding every link; no issue is affected, +since a link never held state belonging to one. + +ST calls this phase54; 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 +``` + **Deploy order (tenant DBs):** ```bash python -m control.tenant_migrate upgrade --tenant all @@ -1525,6 +1577,9 @@ set -a; . /etc/jqc/control.env; set +a | 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. | +| 110 | **An issue link is a pointer to another issue — filter it by scope on ALL THREE surfaces** | A link exposes the far issue's id, description, facility and status, so an unfiltered panel lets a customer read an issue at a facility they hold no assignment to, simply because one of our staff linked it. Three surfaces have to hold the line and only one is a real boundary: `_readable_links()` filters what the panel RENDERS, `link_search()` scopes what the picker FINDS, and `add_link()` re-checks on POST — the search is a convenience and must never be trusted as the gate. All three resolve scope through `_viewer_facility_scope()` / `_issue_in_scope()`, the same pair `issues.view()` now uses, so the panel cannot end up more permissive than the page it sits on. `_issue_in_scope` takes a resolved scope rather than a user, so filtering a list costs one assignment query, not one per row. A link to an issue outside your scope reports "not found", never "access denied" — whether another customer's issue exists is not something the link box should confirm. Cross-TENANT isolation is a different layer and is already handled: `RoutingSession` has bound the session to `g.tenant`'s database, so an id from another tenant does not resolve at all. | +| 111 | **The MT connection ceiling is `workers x [ default pool + cache_cap x tenant pool ]` — never just the default pool** | `config.SQLALCHEMY_ENGINE_OPTIONS` governs only the default bind; in MT nearly every request runs on a per-tenant engine from `app/tenancy/engine_cache.py` with its own `TENANT_ENGINE_*` pool, and each worker caches up to `TENANT_ENGINE_CACHE_MAX` of them. Sizing against the default pool alone is how a box reaches "Too many connections" while the health check reports headroom. `scripts/db_health.py` does the full arithmetic; the cache cap is the multiplier worth lowering first. Both pools set `pool_pre_ping` — without it an idle overnight surfaces as `OperationalError 2006` on the next request. | +| 112 | **The SLA cron narrows to candidates in SQL; the prefilter is a conservative SUPERSET, never an equality** | `send_sla_alerts()` runs every 30 minutes **per tenant**, so reading the whole open-issue backlog to decide in Python multiplied by the tenant count. The three filters each mirror a `continue` in the loop, and one of them — `reported_at IS NOT NULL` — is a correctness fix, not a speed one: `sla_status()` raises `TypeError` on a NULL and one such row aborted that tenant's entire run. The prefilter deliberately does NOT replicate the "already notified at_risk and still only at_risk" skip (that would mean writing the per-severity deadline arithmetic a second time, in SQL); the loop still applies it, so extra rows are read but no extra notification is sent. Pinned by `tests/test_sla_candidate_query.py` — a change that makes the prefilter narrower than the loop is a silently unsent alert. | | 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 ` + + + + {% endif %} + + + {% endfor %} + + {% else %} +

+ + No linked issues. + {% if can_manage_links %} + Use Link an Issue to point at a duplicate or a related issue. + {% endif %} +

+ {% endif %} + + + {# ── Comments ───────────────────────────────────────────────────────── #}
@@ -564,6 +642,77 @@ {% endif %}
+{# ── Link an issue ──────────────────────────────────────────────────────────── + Search is scoped server-side to issues this viewer could already open, so the + picker can never be used to enumerate another contract's issues. The POST + re-checks access — the search is only a convenience. #} +{% if can_manage_links %} + +{% endif %} + {% if current_user.role in ['admin', 'director'] %}