Files
ScadaBridge/docs/plans/2026-06-19-sms-notifications.md
T

281 lines
22 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# SMS Notifications (T9 pivot + T10) Implementation Plan
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development to implement this plan task-by-task.
**Goal:** Add `NotificationType.Sms` + a Twilio-REST `SmsNotificationDeliveryAdapter` behind the existing `INotificationDeliveryAdapter` seam, plus the T10 enum/UI/CLI Type-selector plumbing, so notification lists can deliver to ZB team members by SMS. Email is unchanged.
**Architecture:** Mirror the Email/SMTP stack. A list's existing `NotificationList.Type` decides the channel; `Notify.To` stays type-agnostic; central stamps the Notification's Type from the list at ingest; the outbox actor already routes `Dictionary<NotificationType, adapter>` and parks on a missing adapter. Twilio called directly over REST (no SDK). Secrets encrypted via the existing Data-Protection `EncryptedStringConverter`.
**Tech Stack:** C#/.NET 10, Akka.NET, EF Core (MS SQL), Blazor Server, System.CommandLine, `IHttpClientFactory`, ASP.NET Data Protection. **No new NuGet packages** (`Microsoft.Extensions.Http` is already centrally managed).
**Design doc:** `docs/plans/2026-06-19-sms-notifications-design.md` (committed 28cab6e8).
**Execution rules:** dedicated worktree `feat/sms-notifications` off `main` `c72d7b79`. Implementers do NOT create worktrees. Pathspec commits only (`git commit -m "msg" -- <paths>`, `-m` before `--`, never `git add -A`; `git add <explicit path>` for new files; retry on index.lock). ≤2-3 concurrent committers per wave + post-wave HEAD-presence check (`git merge-base --is-ancestor <sha> HEAD`). Targeted builds/tests per task; full-solution build + docker rebuild + Playwright + live smoke only at INT (S11). TreatWarningsAsErrors=true.
---
## Wave / dependency map
- **Wave 1:** S1 (Commons foundation) — blocks all.
- **Wave 2:** S2 (Config-DB migration) ∥ S5 (Management commands+handlers). Both ⟵ S1.
- **Wave 3:** S3 (adapter) ⟵ S1,S2 ∥ S4 (ingest stamping) ⟵ S1 ∥ S6 (CLI) ⟵ S5.
- **Wave 4:** S7 (list form) ∥ S8 (list page Type column) ∥ S9 (SMS config page) ∥ S10 (Transport). All ⟵ S1/S5 (S9,S10 also ⟵ S2). Cap at 3 concurrent; queue the 4th.
- **Wave 5:** S11 (INT) ⟵ all.
---
### Task S1: Commons foundation — NotificationType.Sms, recipient phone, SmsConfiguration entity, repo interface
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** none (foundational)
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Types/Enums/NotificationType.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/NotificationRecipient.cs`
- Create: `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmsConfiguration.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/INotificationRepository.cs`
- Test: `tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/` (add a recipient-factory + SmsConfiguration shape test if a suitable test class exists; else a small new test file)
**Steps:**
1. Add `Sms` to `NotificationType` (after `Email`). Update the enum's XML doc to "Email and SMS supported." NOTE: `Commons.Tests` may have an enum-count assertion — if so, update it (this is the known enum-count-drift pattern).
2. `NotificationRecipient`: add `public string? PhoneNumber { get; set; }`; relax `EmailAddress` to `string?`. Keep the existing `NotificationRecipient(string name, string emailAddress)` ctor (email path). Add static factories `ForEmail(string name, string email)` and `ForSms(string name, string phoneNumber)` (set the appropriate field, leave the other null; validate name non-empty). Keep a parameterless-friendly path for EF.
3. Create `SmsConfiguration` POCO (persistence-ignorant): `int Id`, `string AccountSid`, `string? AuthToken`, `string FromNumber`, `string? MessagingServiceSid`, `string? ApiBaseUrl`, `int ConnectionTimeoutSeconds`, `int MaxRetries`, `TimeSpan RetryDelay`. Mirror `SmtpConfiguration` constructor/validation style.
4. `INotificationRepository`: add `Task<SmsConfiguration?> GetSmsConfigurationAsync(CancellationToken=default)`, `Task<IReadOnlyList<SmsConfiguration>> GetAllSmsConfigurationsAsync(CancellationToken=default)`, `Task AddSmsConfigurationAsync(SmsConfiguration, CancellationToken=default)`, `Task UpdateSmsConfigurationAsync(SmsConfiguration, CancellationToken=default)` (match the SMTP method shapes already present).
5. Build `dotnet build src/ZB.MOM.WW.ScadaBridge.Commons/...`; run any touched Commons tests.
6. Commit (pathspec; `git add` the new SmsConfiguration.cs).
**Acceptance:** Commons builds 0/0; `NotificationType.Sms` exists; recipient carries nullable phone + factories; `SmsConfiguration` + repo methods defined.
---
### Task S2: Configuration Database — EF mappings, encryption, idempotent migration
**Classification:** high-risk
**Estimated implement time:** ~5 min
**Parallelizable with:** S5
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Configurations/NotificationConfiguration.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ScadaBridgeDbContext.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/NotificationRepository.cs` (or wherever `INotificationRepository` is implemented — locate it)
- Create: new EF migration under `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Migrations/`
- Test: `tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/` (mapping/round-trip + encryption test)
**Steps:**
1. `NotificationRecipientConfiguration`: `EmailAddress``.IsRequired(false)`; add `PhoneNumber` `.HasMaxLength(32)` (nullable).
2. Add `SmsConfigurationConfiguration : IEntityTypeConfiguration<SmsConfiguration>` (key, `AccountSid` required maxlen, `FromNumber` required, `AuthToken`/`MessagingServiceSid`/`ApiBaseUrl` nullable, sensible maxlens). Add `DbSet<SmsConfiguration>` + apply config.
3. `ScadaBridgeDbContext.ApplySecretColumnEncryption`: add `modelBuilder.Entity<SmsConfiguration>().Property(s => s.AuthToken).HasConversion(converter);`.
4. Implement the four `INotificationRepository` SMS-config methods (mirror SMTP impls exactly).
5. Generate the migration (`dotnet ef migrations add AddSmsNotifications ...`). Then make it **idempotent** per repo convention (guard column add / table create / column-alter the way prior migrations in this repo do — match an existing idempotent migration as reference). Migration must: add `NotificationRecipients.PhoneNumber` (nullable), alter `NotificationRecipients.EmailAddress` to nullable, create `SmsConfigurations` table.
6. Verify NO `PendingModelChangesWarning`: `dotnet build` + a model-drift check (the repo has a pattern/test for this — run it).
7. Build Config-DB + run touched Config-DB tests. Commit (pathspec).
**Acceptance:** Config-DB builds; migration idempotent + no model drift; encryption applied to `AuthToken`; recipient phone + nullable email mapped; round-trip test green.
---
### Task S3: SmsNotificationDeliveryAdapter (Twilio REST) + classifier + options + DI + adapter tests
**Classification:** high-risk
**Estimated implement time:** ~5 min
**Parallelizable with:** S4, S6
**Files:**
- Create: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs`
- Create: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsErrorClassifier.cs`
- Create: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/SmsOptions.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ServiceCollectionExtensions.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ZB.MOM.WW.ScadaBridge.NotificationOutbox.csproj`
- Modify (test): `tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/ServiceRegistrationTests.cs`
- Create (test): `tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs`
**Steps:**
1. `SmsErrorClassifier`: static `IsTransient(HttpStatusCode)` (5xx/408/429) + `IsTransient(Exception, CancellationToken)` (HttpRequestException/Timeout/TaskCanceled when not caller-cancelled). Mirror `SmtpErrorClassifier`'s enum+static-method shape.
2. `SmsOptions` (bind `ScadaBridge:Sms`): `int MaxMessageLength = 1600`, `int ConnectionTimeoutSeconds = 30` (config-row value overrides). Validate.
3. `SmsNotificationDeliveryAdapter : INotificationDeliveryAdapter`, `Type => NotificationType.Sms`. Ctor: `INotificationRepository`, `IHttpClientFactory`, `ILogger`, `IOptions<SmsOptions>`. `DeliverAsync`:
- Load list by `notification.ListName``Permanent` if missing.
- Load recipients; filter to non-empty `PhoneNumber``Permanent` if none.
- Load `GetSmsConfigurationAsync()``Permanent` if null/incomplete.
- Compose body = `Subject` + "\n" + `Body`, truncate to `MaxMessageLength`.
- For each phone: `POST {ApiBaseUrl}/2010-04-01/Accounts/{AccountSid}/Messages.json`, Basic auth header `AccountSid:AuthToken`, form `To`/`From`(or MessagingServiceSid)/`Body`. Classify per response.
- Roll up per D6 (all-accepted→Success; any-transient→Transient; mix accepted+permanent→Success + LastError note; all-permanent→Permanent). `ResolvedTargets` = accepted numbers.
- Scrub AuthToken from any logged error via `CredentialRedactor.Scrub`.
4. `ServiceCollectionExtensions.AddNotificationOutbox`: register `SmsNotificationDeliveryAdapter` as scoped `INotificationDeliveryAdapter` (alongside Email); `services.AddHttpClient("Twilio")`; bind `SmsOptions`.
5. `.csproj`: add `<PackageReference Include="Microsoft.Extensions.Http" />` (version comes from Directory.Packages.props — do NOT add a version attribute).
6. **Update `ServiceRegistrationTests`**: the `Assert.Single(adapters)` test must now assert both Email and SMS adapters are registered (e.g. `Assert.Equal(2, adapters.Count)` + both types present); keep/add a `SmsNotificationDeliveryAdapter.Type == Sms` assertion.
7. Adapter unit tests with a fake `HttpMessageHandler`: 201 success (single + multi recipient), 500/429/timeout → Transient, 400/401 → Permanent, mix accepted+400 → Success with LastError note, no-config/no-recipients/list-missing → Permanent, AuthToken never appears in returned error string.
8. Build NotificationOutbox + run NotificationOutbox.Tests. Commit (pathspec; `git add` new files).
**Acceptance:** adapter builds; both adapters registered; all adapter unit tests + the updated ServiceRegistration test green; no AuthToken leakage.
---
### Task S4: Ingest type-stamping in NotificationOutboxActor
**Classification:** standard
**Estimated implement time:** ~3 min
**Parallelizable with:** S3, S6
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxActor.cs` (the ingest path that currently hard-codes `NotificationType.Email`, ~line 1147)
- Test: `tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/` (ingest type-stamping test)
**Steps:**
1. At ingest, resolve the target list (`GetListByNameAsync(msg.ListName)`); stamp `Type = list?.Type ?? NotificationType.Email` onto the new `Notification`. Keep the at-least-once insert-if-not-exists semantics intact (do NOT change idempotency). Add a brief comment replacing the old "all notifications are email" comment.
2. Test: ingest for an Email list stamps Email; ingest for an Sms list stamps Sms; ingest for a missing list defaults Email (and later parks at delivery — assert default only).
3. Build + run NotificationOutbox.Tests (filtered). Commit (pathspec).
**Acceptance:** notifications inherit the list's Type at ingest; idempotency unchanged; tests green.
---
### Task S5: Management — list-command Type + SMS-config commands/handlers
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** S2
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/NotificationCommands.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs`
- Test: `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/` (handler tests)
**Steps:**
1. `NotificationCommands.cs`: add `NotificationType Type = NotificationType.Email` to `CreateNotificationListCommand` and `UpdateNotificationListCommand` (additive trailing param). Add `record ListSmsConfigsCommand;` and `record UpdateSmsConfigCommand(int SmsConfigId, string AccountSid, string FromNumber, string? MessagingServiceSid = null, string? ApiBaseUrl = null, string? AuthToken = null);` (mirror `UpdateSmtpConfigCommand`, preserve-if-null for AuthToken).
2. `ManagementActor`: in Create/Update list handlers, persist `cmd.Type` on the `NotificationList`. Add `HandleListSmsConfigs` + `HandleUpdateSmsConfig` (preserve-if-null AuthToken/ApiBaseUrl), role-gate (Designer/Admin per SMTP), and audit via a credential-free `SmsConfigPublicShape` (mirror `SmtpConfigPublicShape` — never log/return AuthToken). Wire the command routing entries.
3. Handler tests: create list with Type=Sms persists Sms; update SMS config preserves AuthToken when null, never surfaces AuthToken in audit/response.
4. Build Commons + ManagementService + run ManagementService.Tests (filtered). Commit (pathspec).
**Acceptance:** list commands carry Type; SMS-config commands/handlers exist, role-gated, secret-safe; tests green.
---
### Task S6: CLI — list --type/--phones + notification sms group
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** S3, S4
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.CLI/Commands/NotificationCommands.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.CLI/README.md`
- Test: `tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/`
**Steps:**
1. List `create`/`update`: add `--type email|sms` (default email) and `--phones` (comma-separated E.164). Validate: email type requires `--emails`; sms type requires `--phones`; reject mismatch. Build `Create/UpdateNotificationListCommand` with `Type` + the right recipient set. (Recipients for SMS map to phone numbers — coordinate with how recipients are submitted; if recipients are added via a separate command, set Type on the list and feed phones through the recipient path.)
2. Add `notification sms list` (→ `ListSmsConfigsCommand`) and `notification sms update --id --account-sid --from-number [--messaging-service-sid] [--api-base-url] [--auth-token]` (→ `UpdateSmsConfigCommand`), mirroring the `notification smtp` group. Render JSON/table; never print AuthToken.
3. Update CLI README (notification section: `--type`/`--phones`, `sms` group).
4. Build CLI + run CLI.Tests (filtered). Commit (pathspec).
**Acceptance:** CLI creates SMS lists + manages SMS config; validation enforced; secret never printed; tests + README updated.
---
### Task S7: Central UI — NotificationListForm Type selector + per-type recipient input
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** S8, S9, S10
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/NotificationListForm.razor`
- Test: `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/` (NotificationListForm tests)
**Steps:**
1. Add an **adapter-gated** Type selector: source the options from the registered `INotificationDeliveryAdapter` Types (inject the adapter set or a small accessor; if injecting the adapters into the UI is awkward, expose a server-side accessor that returns the registered `NotificationType` set). Today renders Email + SMS. Selector enabled on create; **read-only on edit** (Id.HasValue) to prevent recipient-kind mismatch.
2. Recipient editor: when Type=Email show the Email input + validate email; when Type=Sms show a Phone input + validate E.164. Build the recipient via `NotificationRecipient.ForEmail/ForSms`. Update the recipients table to show Email or Phone per type.
3. On save, pass `Type` to the create command / new list.
4. bUnit tests: selector renders Email+SMS; choosing SMS shows phone input + validation; create persists Type; edit shows Type read-only. **Razor hygiene:** any `@* *@` comment must sit OUTSIDE element start tags (the recurring circuit-crash landmine).
5. Build CentralUI + run the NotificationListForm tests (filtered). Commit (pathspec).
**Acceptance:** Type selector adapter-gated + create-only; per-type recipient input + validation; tests green; no Razor start-tag comments.
---
### Task S8: Central UI — NotificationLists Type column
**Classification:** small
**Estimated implement time:** ~3 min
**Parallelizable with:** S7, S9, S10
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/NotificationLists.razor`
- Test: `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs`
**Steps:**
1. Add a **Type** column to the lists table (render `NotificationType`). Keep markup tokenized/consistent with the M10 table conventions.
2. Update/extend `NotificationListsPageTests` to assert the Type column renders (e.g. an SMS list shows "Sms").
3. Build CentralUI + run NotificationListsPageTests. Commit (pathspec).
**Acceptance:** Type column renders; test green.
---
### Task S9: Central UI — SMS configuration page
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** S7, S8, S10
**Files:**
- Create: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/SmsConfiguration.razor`
- Modify: the nav/menu component that lists `/notifications/smtp` (add `/notifications/sms`)
- Test: `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/` (SmsConfiguration page test)
**Steps:**
1. New page `/notifications/sms` (Admin-only, mirror `SmtpConfiguration.razor`): list SMS configs, create/edit form (AccountSid, FromNumber, MessagingServiceSid, ApiBaseUrl, AuthToken — AuthToken write-only/masked, preserve-if-blank on edit), save via repository/management. Never render the stored AuthToken back.
2. Add the nav entry next to the SMTP settings entry.
3. bUnit: page renders config rows; AuthToken input is masked/not pre-filled; save calls the repository. Razor start-tag comment hygiene.
4. Build CentralUI + run the page test. Commit (pathspec; `git add` new file).
**Acceptance:** SMS config page works + Admin-gated + secret-safe; nav entry added; test green.
---
### Task S10: Transport — recipient PhoneNumber DTO + SmsConfigDto round-trip
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** S7, S8, S9
**Files:**
- Modify: `src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs`
- Modify: `src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs`
- Test: `tests/ZB.MOM.WW.ScadaBridge.Transport.*Tests/`
**Steps:**
1. `NotificationRecipientDto`: add `string? PhoneNumber` (trailing, nullable → backward-compatible). `SmsConfigDto` mirroring `SmtpConfigDto` (secret in `SecretsBlock`). Add `IReadOnlyList<SmsConfigDto> SmsConfigs` to `BundleContentDto` and `IReadOnlyList<SmsConfiguration> SmsConfigurations` to `EntityAggregate`.
2. `EntitySerializer`: map recipient PhoneNumber both directions; map SMS configs export/import (mirror SMTP, including SecretsBlock handling). Keep `schemaVersion` evolution additive per repo convention.
3. Tests: recipient phone round-trips; SMS config round-trips with secret in SecretsBlock; old bundle without PhoneNumber deserializes to null.
4. Build Transport + run Transport tests (filtered). Commit (pathspec).
**Acceptance:** SMS recipient phone + SMS config travel in bundles; backward-compatible; tests green.
---
### Task S11: Integration — build, migration drift, docker, Playwright, live smoke, docs, review
**Classification:** high-risk
**Estimated implement time:** ~10 min (+ docker rebuild wall-time)
**Parallelizable with:** none
**Files:**
- Modify: `docs/requirements/Component-NotificationService.md`, `docs/requirements/Component-NotificationOutbox.md`
- Modify: `CLAUDE.md` (NotificationType = Email+Sms; T9/T10 delivered as SMS — flip the M6 deferral note; add the SMS-adapter decision bullet)
- Modify: `README.md` (component table / notes if needed)
- Create (test): `tests/ZB.MOM.WW.ScadaBridge.CentralUI.PlaywrightTests/Notifications/SmsNotificationListTests.cs` (or extend existing notification Playwright)
**Steps:**
1. Full-solution build: `dotnet build ZB.MOM.WW.ScadaBridge.slnx` — 0 warnings/errors.
2. EF model-drift check (no `PendingModelChangesWarning`).
3. Full unfiltered managed test run `dotnet test ZB.MOM.WW.ScadaBridge.slnx` (run 23× for load-flake surfacing per the integration memory). Triage any red: prove pre-existing via `git diff --stat c72d7b79..HEAD -- <target files>` before attributing.
4. Docker rebuild: `bash docker/deploy.sh`; `/health/ready` 200 on the active node.
5. Live smoke via CLI against the cluster: set an SMS config (fake AccountSid + `--api-base-url` pointed at a mock, OR document Twilio sandbox), create an `--type sms` list with a phone recipient, submit a notification, verify it routes to the SMS adapter and parks-with-reason when the fake returns an error (proves routing + classification end-to-end). Also verify an Email list still delivers (no regression).
6. Playwright: SMS-list create flow (Type selector → SMS → phone recipient → save) against the freshly-rebuilt cluster; broad notification-page smoke. Razor circuit-crash check: load `/notifications/lists/create` + `/notifications/sms` in a real browser (the bUnit-invisible render-crash gate).
7. Docs reconciliation: Component docs + CLAUDE.md + README + CLI README synced; confirm the M6 deferral note is flipped to "delivered (SMS pivot)."
8. Whole-branch integration review over `git diff c72d7b79..HEAD` (Bash-capable reviewer): end-to-end trace of the new `NotificationType.Sms` path (submit → ingest stamp → adapter dispatch → Twilio POST → park/retry), shared-component injection regressions (any new ctor/`[Inject]` dep → grep every fixture across ALL test projects), and secret-leak audit (AuthToken never logged/returned/rendered).
9. Commit docs (pathspec). Then hand to finishing-a-development-branch.
**Acceptance:** full build + full managed tests green; docker healthy; live smoke proves SMS routing + Email no-regression; Playwright + render-crash gate green; docs synced; whole-branch review INTEGRATION-CLEAN.