From 475bfadacdd4a25c18ad64384b0d33b278536607 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 7 Jun 2026 09:53:34 -0400 Subject: [PATCH] docs(plans): design for template-alarm CLI ergonomics + alarm-override coverage --- ...-alarm-cli-and-override-coverage-design.md | 110 ++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 docs/plans/2026-06-07-template-alarm-cli-and-override-coverage-design.md diff --git a/docs/plans/2026-06-07-template-alarm-cli-and-override-coverage-design.md b/docs/plans/2026-06-07-template-alarm-cli-and-override-coverage-design.md new file mode 100644 index 00000000..9ee1f338 --- /dev/null +++ b/docs/plans/2026-06-07-template-alarm-cli-and-override-coverage-design.md @@ -0,0 +1,110 @@ +# Template-Alarm CLI Ergonomics + InstanceConfigure Alarm-Override Coverage — Design + +**Date:** 2026-06-07 +**Status:** Approved (brainstorming complete) → ready for writing-plans +**Component:** #9 Central UI (`InstanceConfigure`) + the `scadabridge` CLI + the Playwright harness +**Predecessor / context:** closes the one functional gap deferred across the [4-wave coverage-fill effort](2026-06-06-playwright-coverage-fill-design.md) (alarm-override UI), plus the cheap Wave-4 review-hygiene items. + +## Goal + +Three sequential pieces of work: + +- **(A)** Make template alarms *ergonomically* CLI-provisionable — add typed setpoint flags to the existing `template alarm add` verb so callers don't hand-write trigger-config JSON; add a `CliRunner.AddAlarmAsync` test helper. +- **(B)** Add the deferred **alarm-override UI coverage** on `InstanceConfigure` (Edit → set priority override → Save → badge → Clear), gated on additive `data-test` hooks. +- **(C)** Tackle the cheap, safe Wave-4 hygiene items. + +## Premise correction (verified during brainstorming) + +The Wave-1 deferral TODO (`InstanceConfigureTests.cs`) says *"template alarms are not CLI-provisionable today."* **That is STALE.** The CLI already has a fully-wired `template alarm add | update | delete`: + +``` +template alarm add --template-id --name \ + --trigger-type \ + --priority <0-1000> [--description ] [--trigger-config ] [--locked] +``` + +End-to-end path (no cluster change needed to add an alarm): +`CLI BuildAlarm` → `AddTemplateAlarmCommand` DTO → `POST /management` → `ManagementActor.HandleAddAlarm` (role-gated `Designer`) → `TemplateService.AddAlarmAsync` → `ITemplateEngineRepository.AddTemplateAlarmAsync` + audit. + +So the only *real* gaps are: (1) no ergonomic typed flags (you must hand-write `--trigger-config` JSON for setpoints); (2) no `CliRunner.AddAlarmAsync` test helper; (3) the `InstanceConfigure` alarm-override section is hook-poor and untested. + +## Verified domain facts (grounding for the plan) + +**Alarm entity — `TemplateAlarm`** (`src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Templates/TemplateAlarm.cs`, table `TemplateAlarms`): +- Child of **`Template`** (FK `TemplateId`), NOT of `TemplateAttribute`. Binds to an attribute *by name* inside the `TriggerConfiguration` JSON (`attributeName` key). +- Fields: `Id`, `TemplateId`, `Name` (unique within template), `Description?`, `PriorityLevel` (0–1000), `IsLocked`, `TriggerType` (`AlarmTriggerType` enum), `TriggerConfiguration?` (JSON ≤4000 chars), `OnTriggerScriptId?`, `IsInherited`, `LockedInDerived`. +- `AlarmTriggerType` enum: `ValueMatch | RangeViolation | RateOfChange | HiLo | Expression`. +- Minimal valid alarm: `Name` + `TriggerType` + `PriorityLevel`. No trigger-config required to make a row exist (it just won't *fire*); an unlocked alarm is enough to render an override row. + +**Trigger-config JSON shapes** (authoritative key names from `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/AlarmTriggerConfigCodec.cs`, which is `internal`): +- `ValueMatch` → `{ attributeName, matchValue }` (a `"!=X"` prefix on matchValue means not-equals) +- `RangeViolation` → `{ attributeName, min, max }` +- `RateOfChange` → `{ attributeName, thresholdPerSecond, windowSeconds, direction }` +- `HiLo` → `{ attributeName, loLo, lo, hi, hiHi, loLoPriority, loPriority, hiPriority, hiHiPriority, *Deadband, *Message }` +- `Expression` → `{ expression }` (no attributeName) + +**Instance alarm override — `InstanceAlarmOverride`** (`src/.../Commons/Entities/Instances/InstanceAlarmOverride.cs`, table `InstanceAlarmOverrides`): +- Fields: `Id`, `InstanceId` (FK, cascade), `AlarmCanonicalName` (matches `TemplateAlarm.Name` for direct alarms), `TriggerConfigurationOverride?` (JSON; partial-merge for HiLo, whole-replace otherwise), `PriorityLevelOverride?`. Unique `(InstanceId, AlarmCanonicalName)`. No "enabled" flag — only trigger-config and/or priority. +- Save/clear service: `InstanceService.SetAlarmOverrideAsync` / `DeleteAlarmOverrideAsync`. CLI verbs already exist: `instance alarm-override set|delete|list`. +- **Read-back:** `GetInstanceByIdAsync` eager-loads `.Include(i => i.AlarmOverrides)`, so the existing `CliRunner.GetInstanceDocumentAsync` (`instance get`) surfaces an `alarmOverrides` array alongside the `connectionBindings`/`attributeOverrides` the current tests already read. (Plan must confirm the exact camelCase JSON key.) + +**InstanceConfigure alarm-override UI** (`src/.../CentralUI/Components/Pages/Deployment/InstanceConfigure.razor`, single-file `@code`): +- Section identified only by header text `Alarm Overrides` — **no `data-test` hooks anywhere in the alarm section** (the whole file has only 4 hooks: `audit-link`, `instance-error-alert`, `binding-bulk-select`, `area-select`). `AlarmTriggerEditor.razor` has zero hooks. +- Rows render from `_overridableAlarms = alarms.Where(a => !a.IsLocked)`. Empty → `"No overridable (non-locked) alarms on this template."` So the fixture template **must carry a non-locked alarm**. +- Per row: `Edit` button → opens a modal (`@if (_editingAlarm != null)` → `.modal.show.d-block`). Modal has an `` + a **priority override** `` (placeholder = inherited priority) + footer `Cancel` / `Save Override` (`.btn-success.btn-sm`) / `Clear Override` (when an override exists). +- **Save semantics:** if the trigger-config diff is empty AND the priority field is empty, Save **deletes** any existing override. So the test's reliable "create override" delta is **setting the priority field**. +- Override-set badge: `●` in the row, shown only when `HasOverride(name)`. Row-level `Clear` button (`.btn-outline-danger.btn-sm`) → `ClearAlarmOverride` → `DeleteAlarmOverrideAsync` **immediately, no confirm dialog**; toast `"Cleared override on '{name}'."` + +**Existing fixture/test** (`tests/.../Deployment/InstanceConfigureFixture.cs` / `InstanceConfigureTests.cs`): +- Fixture (CLI-provisioned on real `site-a`, instance **not deployed**): template + one `Double` attribute `"Value"` (`AddAttributeAsync`, `--data-source "Value"`) + data-connection + area + instance. Public surface: `int SiteAId/TemplateId/ConnectionId/AreaId/InstanceId`, `string AttributeName => "Value"`, `string ConnectionName`, `bool Available`. +- Test idioms: `[Collection("Playwright")]` + `IClassFixture`; `Skip.IfNot(_cfg.Available, ClusterAvailability.SkipReason)`; `_fixture.NewAuthenticatedPageAsync("multi-role","password")`; web-first `Assertions.Expect(...).ToBeVisibleAsync(new(){Timeout=15_000})`; toast `Expect(.toast).ToHaveCountAsync(1)`; persistence via `using var doc = await CliRunner.GetInstanceDocumentAsync(InstanceId)`. + +**Build/transport reality:** +- CLI is a **host-side** client (`OutputType=Exe`, only references `Commons`); runs from `src/.../CLI/bin/Debug/net10.0/scadabridge.dll`. A CLI-only change needs just `dotnet build src/ZB.MOM.WW.ScadaBridge.CLI`. +- `ManagementActor`/`TemplateService`/CentralUI run in the cluster image; a `.razor` change needs `bash docker/deploy.sh` to reach the running cluster the tests hit. + +## Decisions (settled during brainstorming) + +| # | Decision | Choice | Rationale | +|---|----------|--------|-----------| +| D1 | Part-A scope | **Typed CLI flags + test helper** (not just the helper) | User wants the ergonomic enhancement; CLI serializes the JSON. CLI-only, no server change. | +| D2 | Trigger-config serializer location | **New CLI-side `AlarmTriggerConfigJson`** (don't move the UI's `internal` codec) | Keeps the change CLI-only / no docker rebuild; the round-trip test verifies the JSON against the live server (the real contract). Cross-reference the codec for key names. | +| D3 | Part-B selectors | **Add 6 additive `data-test` hooks** → `bash docker/deploy.sh` | Matches the existing InstanceConfigure convention (`binding-bulk-select`/`area-select`); removes selector drift (esp. the priority input, which collides with `AlarmTriggerEditor` number inputs). | +| D4 | Override-test delta | **Priority override** (not trigger-config) | Simplest reliable "create override" path; a no-delta save deletes instead. | +| D5 | Fixture alarm type | **HiLo** via the new typed flags (`--attribute Value --hi 80 --hihi 95`) | Dogfoods the Part-A flags end-to-end. (Any non-locked alarm works for the override row.) | +| D6 | Part-C scope | **Cheap & safe only** | `plant-a→site-a`, locator scoping, next-enabled re-assert, doc note. Leave the harmless suite-wide `WaitForLoadState` pattern. | + +## Architecture / components + +### Part A — CLI ergonomic alarm flags + test helper *(host-side build only)* +1. **`AlarmTriggerConfigJson` (new, CLI project):** `Build(triggerType, attribute, matchValue, notEquals, min, max, thresholdPerSecond, windowSeconds, direction, loLo, lo, hi, hiHi, expression) → string?` emitting the exact camelCase keys from `AlarmTriggerConfigCodec`. Only emits fields relevant to the trigger type; returns `null` when no typed flags are given. Comment cross-references the codec as the source of truth. +2. **`template alarm add` typed options** in `CLI/Commands/TemplateCommands.cs::BuildAlarm`: `--attribute`, `--match-value`, `--not-equals`, `--min`, `--max`, `--threshold-per-second`, `--window-seconds`, `--direction`, `--lolo`, `--lo`, `--hi`, `--hihi`, `--expression`. When any are present, the handler builds the JSON and sets it as `AddTemplateAlarmCommand.TriggerConfiguration`. Raw `--trigger-config` (if supplied) takes precedence. **No new DTO, no server change.** (HiLo per-level priorities/deadbands/messages stay JSON-only — YAGNI.) +3. **`CliRunner.AddAlarmAsync(int templateId, string name, string triggerType = "HiLo", int priority = 500, string? attribute = null, double? hi = null, double? hiHi = null, ...)`** in `CliRunner.Helpers.cs` (mirrors `AddAttributeAsync`; throws via `RequireId`). +4. **Round-trip `[SkippableFact]`** in `CliRunnerHelpersTests.cs`: create a `zztest` template → `AddAlarmAsync` a HiLo via typed flags → assert returned id > 0 (proves the server accepted the serialized JSON) → `finally` delete template (cascade). + +### Part B — alarm-override coverage *(app change → one docker rebuild)* +1. **`InstanceConfigure.razor` hooks (additive, non-functional):** `data-test="alarm-override-row-{alarm.Name}"` (row), `alarm-edit-btn` (Edit), `alarm-priority-input` (modal priority input), `alarm-save-override` (Save Override), `alarm-override-badge` (●), `alarm-clear-btn` (row Clear). **`bash docker/deploy.sh`** so the live UI exposes them. +2. **`InstanceConfigureFixture`:** after `AddAttributeAsync`, call `CliRunner.AddAlarmAsync(TemplateId, AlarmName, "HiLo", priority:500, attribute:"Value", hi:80, hiHi:95)`; expose `string AlarmName`. No teardown (template delete cascades the alarm). +3. **`AlarmOverride_SetPriority_ThenClear_RoundTrips` (extends `InstanceConfigureTests.cs`):** navigate to the configure page → assert the alarm row shows "inherited" → `alarm-edit-btn` → fill `alarm-priority-input` = `750` → `alarm-save-override` → assert `.toast` count 1 + `alarm-override-badge` visible → CLI read-back `GetInstanceDocumentAsync` shows `alarmOverrides[]` with `priorityLevelOverride == 750` → `alarm-clear-btn` → assert badge gone + read-back empty. `finally`: best-effort `instance alarm-override delete` (new `CliRunner.DeleteInstanceAlarmOverrideAsync` helper) so the **shared fixture instance is left as found**. + +### Part C — hygiene (cheap & safe) +- `sourceSite:"plant-a"` → `"site-a"` in the older SiteCalls/Notification seeding tests. +- Scope the pager-indicator locators (`span.text-muted.small` → pager-container-scoped) in the SiteCalls + Notification pagination tests. +- Add the `next` enabled re-assert on the return-to-page-1 leg of both pagination tests. +- Add the "update all three-word deletes together" maintenance note to `DeleteRoleMappingAsync`. +- Test-only; verified by re-running the affected suites. + +## Error handling & verification + +- **Validation-behavior protocol** still applies: before asserting the override badge/persistence, the implementer reads the real save handler (a no-delta save deletes; priority is the reliable delta) and the `alarmOverrides` JSON key. +- **Build map:** A → `dotnet build src/...CLI` (host) + test-project build; B → **`bash docker/deploy.sh`** (the only docker rebuild, for the hooks) then test-project build; C → test-project build. +- **Residue & shared-state:** alarm round-trip uses a `zztest` template (cascade-deleted). The override test mutates the *shared* fixture instance's alarm override and clears it (+ `finally` cleanup) → instance left as found; the fixture's `DisposeAsync` tears down template/connection/area/instance. No other InstanceConfigure test touches alarm overrides; serial collection. +- **Per-part gate:** each part ends with its new tests green against the live cluster, the full suite at 0 failed, zero residue, clean build under `TreatWarningsAsErrors`. Part B additionally confirms the `data-test` additions don't alter rendered behavior. + +## Scope guard (YAGNI) + +No new server command/DTO (Part A rides the existing `AddTemplateAlarmCommand`). No HiLo per-level priority/deadband/message flags (raw JSON escape hatch remains). No suite-wide `WaitForLoadState` refactor. The `data-test` hooks are the only app-code change and are purely additive. No new fixtures beyond extending `InstanceConfigureFixture`. + +## Success criteria + +`template alarm add` accepts typed setpoint flags (verified by a round-trip test); `InstanceConfigure`'s alarm-override flow (set-priority → badge → clear) has functional coverage asserting persistence via CLI read-back; the cheap hygiene items are applied; full suite green with logged skips; zero residue; `site-a` and the shared fixture instance left as found.