Sep 4 - Add link relavant issues function

2026-09-04 16:53:53 -04:00
parent d291dfc513
commit e9005b9b9c
20 changed files with 3208 additions and 72 deletions
@@ -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 /<id>/links` add, `POST /<id>/links/<link_id>/delete` remove, `GET /<id>/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 `<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. |

 ---
@@ -2247,3 +2302,62 @@ local directory carries none. Both need `CONTROL_DATABASE_URL` +

 **Rollback is one env var:** `STORAGE_BACKEND=local` + restart. The sync never
 deletes local files, so the old tree is intact indefinitely.
+
+---
+
+## 30. Database Health Check (`scripts/db_health.py`)
+
+A standalone operations tool for the MySQL side. It imports the app factory for
+config and nothing else — no request layer, no uploads tree — and the plain
+invocation is **strictly read-only** (INFORMATION_SCHEMA / SHOW / EXPLAIN only).
+
+```bash
+set -a; . /etc/jqc/control.env; set +a        # needed for any --tenant but 'default'
+
+python scripts/db_health.py                            # default bind, read-only
+python scripts/db_health.py --tenant all               # every tenant schema
+python scripts/db_health.py --tenant acme --json /tmp/db.json
+python scripts/db_health.py --tenant all --analyze     # refresh optimizer stats (safe)
+python scripts/db_health.py --tenant acme --optimize --yes   # rebuild (LOCKS)
+python scripts/db_health.py --tenant all \
+    --emit-migration migrations/versions/phase58_perf_indexes.py \
+    --revision phase58_perf_indexes
+```
+
+**Every tenant has its own database built from the same chain, so a schema check
+is per tenant, not per deployment.** `--tenant` takes `default` (the
+`SQLALCHEMY_DATABASE_URI` bind — the whole story in single-tenant mode, and the
+fallback bind in MT), a slug, or `all`. Tenant modes resolve through the control
+plane exactly as `control/backup.py` does, and open each tenant with a
+`NullPool` engine: a one-shot CLI must not open a full pool per tenant against
+the very `max_connections` it is checking.
+
+The **server-level** checks (`max_connections`, `wait_timeout`,
+`innodb_buffer_pool_size`, slow-query log, STRICT mode) describe the MySQL
+instance, not a schema, so they run **once** against the first database opened.
+Everything schema-shaped — hygiene, footprint, missing/redundant indexes,
+unindexed FKs, EXPLAIN — runs per tenant, and each finding is tagged with the
+tenant it came from.
+
+**It never DROPs anything.** Redundant indexes are reported with the SQL to run
+by hand, because "unused" is a judgement the tool should not make for you. It
+also refuses to offer an **FK-backed** index as a drop candidate — dropping one
+fails with errno 150.
+
+**Prefer `--emit-migration` over `--apply-indexes` in MT.** An index applied by
+hand to one tenant leaves every other tenant's schema different from it; a
+migration reaches all of them through the normal
+`python -m control.tenant_migrate upgrade --tenant all`. `--emit-migration`
+writes ONE re-runnable migration for the union of what every inspected tenant is
+missing (the chain is shared — emitting one per tenant would produce conflicting
+revisions), with INFORMATION_SCHEMA guards per rule 16. It guesses
+`down_revision` from the versions directory — confirm against
+`python -m control.tenant_migrate heads` before committing.
+
+`RECOMMENDED_INDEXES` in the script is the **single place** the index wish-list
+lives, and every entry names the query that justifies it. An index nothing runs
+is pure write-amplification, so keep speculative entries out — and when a new
+hot query lands, add its index there rather than to an ad-hoc migration, so the
+checker keeps agreeing with the schema.
+
+The connection-ceiling check is MT-aware — see **rule 111**.