From c9b86fea4c0dc0416b40471bac3f4adf5bc894de Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Mon, 25 May 2026 10:56:30 +0800 Subject: [PATCH 01/24] chore(openspec): fix config.yaml block scalar indentation --- openspec/config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openspec/config.yaml b/openspec/config.yaml index 6cbbe24b2df..f362a196fcb 100644 --- a/openspec/config.yaml +++ b/openspec/config.yaml @@ -35,7 +35,7 @@ context: openspec/ # change proposals and archive (this directory) .claude/ # Claude Code tooling (tracked except .context/) - Custom code isolation (CRITICAL): + Custom code isolation (CRITICAL): | - Backend: org.pih.warehouse.custom./ - Frontend: src/js/custom// - Migrations: grails-app/migrations/custom/ (or clearly-prefixed) From 9d6f329a80e1f96f1c9c5053f61617fb12865c01 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Mon, 25 May 2026 10:56:30 +0800 Subject: [PATCH 02/24] docs(notifications): add openspec changes for in-app notifications --- .../in-app-notifications/.openspec.yaml | 2 + .../changes/in-app-notifications/design.md | 219 ++++++++++++++++++ .../changes/in-app-notifications/proposal.md | 35 +++ .../specs/in-app-notifications/spec.md | 163 +++++++++++++ .../changes/in-app-notifications/tasks.md | 145 ++++++++++++ .../.openspec.yaml | 2 + .../notifications-decouple-email/design.md | 116 ++++++++++ .../notifications-decouple-email/proposal.md | 39 ++++ .../specs/in-app-notifications/spec.md | 92 ++++++++ .../notifications-decouple-email/tasks.md | 60 +++++ 10 files changed, 873 insertions(+) create mode 100644 openspec/changes/in-app-notifications/.openspec.yaml create mode 100644 openspec/changes/in-app-notifications/design.md create mode 100644 openspec/changes/in-app-notifications/proposal.md create mode 100644 openspec/changes/in-app-notifications/specs/in-app-notifications/spec.md create mode 100644 openspec/changes/in-app-notifications/tasks.md create mode 100644 openspec/changes/notifications-decouple-email/.openspec.yaml create mode 100644 openspec/changes/notifications-decouple-email/design.md create mode 100644 openspec/changes/notifications-decouple-email/proposal.md create mode 100644 openspec/changes/notifications-decouple-email/specs/in-app-notifications/spec.md create mode 100644 openspec/changes/notifications-decouple-email/tasks.md diff --git a/openspec/changes/in-app-notifications/.openspec.yaml b/openspec/changes/in-app-notifications/.openspec.yaml new file mode 100644 index 00000000000..4a1c6774314 --- /dev/null +++ b/openspec/changes/in-app-notifications/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-22 diff --git a/openspec/changes/in-app-notifications/design.md b/openspec/changes/in-app-notifications/design.md new file mode 100644 index 00000000000..4c779f26ad9 --- /dev/null +++ b/openspec/changes/in-app-notifications/design.md @@ -0,0 +1,219 @@ +## Context + +Today, OpenBoxes only delivers event signals to users via email — there's no persistent in-app surface listing things that need their attention. The TJK customer has asked for an in-app notification center. The dominant constraint is **upstream isolation**: this is a fork, and the more we touch upstream files, the worse our merges get. So the design's goal is to deliver the feature with the smallest possible upstream-edit footprint. + +The fortunate fact that makes the design clean: every email currently flows through one method, `MailService.doSendMail` (`grails-app/services/org/pih/warehouse/core/MailService.groovy:139`), which all 14+ public `sendMail*` overloads on the same class delegate to (lines 62, 66, 70, 74, 78, 82, 86, 90, 94, 101, 105, 109, 113, 117, 121). One intercept point catches everything. The success exit is also unambiguous: `doSendMail` has exactly one `return true` at line 203, immediately after `email.send()` at line 202. The hook goes between those two lines. + +Second fortunate fact: `User extends Person` via **joined-table inheritance** (`User.groovy:17`, `Person.groovy:27-31` — `tablePerHierarchy false`), so `user` and `person` are separate tables joined by primary key. `Person.email` is the canonical email field (`Person.groovy:21`, validated with `email: true, nullable: true, maxSize: 255` at line 39) and `User.findAllByEmail(...)` is a sound dynamic finder against the inherited property. **It is NOT unique** — the constraints block has no `unique:` clause for `email`, so two `Person`/`User` rows can share an email. Our hook accounts for this by using `findAllByEmail` (returns 0..N users) rather than `findByEmail` (returns 0..1) and creates one notification per matching user. + +There is also a legacy wrinkle: `User.groovy:20` comments that `username` holds "email or username", so some pre-2017-style accounts may have their email *only* in `username`. The hook falls back to `findAllByUsername` when the email lookup misses. + +The custom migrations aggregator is already wired (`grails-app/migrations/custom/changelog.groovy`) — adding a new changeset is one file plus one `include file:` line. The URL mapping file is `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy` (not the conventional `grails-app/conf/` location), and it uses **explicit per-route declarations** (sample at lines 45–192: `/api/categories`, `/api/users`, `/api/products`, etc.), with no `/api/**` wildcard. Three new mapping lines will be needed. + +## Goals / Non-Goals + +**Goals:** + +- Persist a per-user notification record every time the app sends an email to a recipient that maps to a known `User`. +- Expose a tiny REST surface (list / mark-read / mark-all-read) scoped to the authenticated user. +- Render a bell-icon notification dropdown in the application header with unread badge and 30s polling. +- Keep the upstream-file footprint to exactly two surgical edits: one call in `MailService.doSendMail`, one `` insertion in the layout header. +- Place all custom backend code under `org.pih.warehouse.custom.notifications` and all custom frontend code under `src/js/custom/notifications/`, per `rules/custom-package-isolation.md`. +- Designed so the feature can be cherry-picked from TJK up to EST without refactoring. + +**Non-Goals:** + +- **Per-user opt-in/out preferences.** Defer to a v2. +- **Per-notification-type categorization or filtering.** v2. +- **WebSocket / push delivery.** 30s polling is sufficient for this use case and avoids new infrastructure. +- **Mobile push, browser push, desktop notifications.** Out of scope. +- **Replacing or rewriting any existing email trigger.** Notifications are purely additive — every email that's sent today is still sent. +- **Notifying users without a `User` account** (external vendors, carriers, etc.). They keep receiving email; they just don't get a `custom_notification` row, because there's nowhere persistent to attach it. +- **Backfilling notifications from historical emails.** New emails only. + +## Decisions + +### D1. Single intercept point at `MailService.doSendMail` + +**Choice:** Hook the notification-creation call into `doSendMail` itself, immediately after the email-send succeeds. + +**Rationale:** All 14+ `sendMail*` overloads on `MailService` delegate to `doSendMail` (verified: `MailService.groovy:62,66,70,74,78,82,86,90,94,101,105,109,113,117,121`). The agent scan found ~16 distinct notification call-sites across `NotificationService`, shipment, requisition, and user lifecycle code — every one of them ultimately calls one of the public `sendMail*` overloads, so they all funnel through `doSendMail`. One hook covers all of them. + +**Alternatives considered:** + +- **Hook at each call site (16+ places).** Rejected: massive upstream-edit footprint, easy to miss, every new upstream email trigger silently bypasses notifications. +- **Spring AOP around the `MailService` bean.** Rejected: works, but on Grails 3.3 it requires extra config in `resources.groovy`, debugging aspect proxies is harder than reading one line of code, and the conditional logic ("only fire on `true` return") is more naturally expressed inline. +- **Event-driven via Grails events (`notify('mail:sent', ...)` + listener).** Rejected as initial approach: requires touching all 16 call sites to fire the event *and* a listener service. Strictly worse than a single hook. Could be retrofitted later if the hook proves too coupled. +- **Cron job that polls a "sent mail" log.** Rejected: there is no persisted log of sends, so we'd have to add one first — at which point we've added a side-effect to `doSendMail` anyway. + +**The hook itself** is a single call after the existing `return true` (or after the success branch, depending on the flow inside `doSendMail`): `customNotificationService.recordSendAsNotifications(to, subject)`. The custom service handles email→User lookup, swallows lookup failures (returns silently if no user matches), and never throws back into `MailService` so a notification-persistence bug can never break an email send. + +### D2. Side table, no upstream schema edits + +**Choice:** New `custom_notification` table with FK to `user(id)`. No changes to upstream tables. + +**Rationale:** Per `rules/upstream-entity-extension.md` — never inject columns onto upstream tables. The notification record is its own concept anyway (read state, link URL, type), not an extension of `User`. The FK is plain referential, not a 1-to-1 extension, so this isn't even the "side-table extension" pattern — it's just a related table that happens to point at `user`. + +**Schema (Liquibase changeset under `grails-app/migrations/custom/`):** + +``` +custom_notification + id varchar(255) PK (uuid) + user_id varchar(255) FK -> user(id), NOT NULL + notification_type varchar(64) NOT NULL -- e.g. 'EMAIL_PIGGYBACK' for now + title varchar(255) NOT NULL + body text NULL + link_url varchar(2048) NULL + is_read boolean NOT NULL DEFAULT false + read_at datetime NULL + date_created datetime NOT NULL + last_updated datetime NOT NULL + + INDEX idx_custom_notification_user_unread (user_id, is_read, date_created DESC) + INDEX idx_custom_notification_user_created (user_id, date_created DESC) +``` + +The composite `(user_id, is_read, date_created DESC)` index serves the bell-poll query (`unreadOnly=true` ordered by `date_created` desc). + +### D3. Email → User resolution (multi-match aware) + +**Choice:** For each recipient email, run `User.findAllByEmail(emailString)`. If empty, fall back to `User.findAllByUsername(emailString)`. For every matching `User` in either result list, create one `CustomNotification` row. If both lookups return empty, silently skip. + +**Rationale:** Verified by reading `Person.groovy:35-41` — the `static constraints` block declares `email(nullable: true, email: true, maxSize: 255)` with **no `unique:` constraint**. Duplicates are possible (rare in practice, but the schema permits them). `findByEmail` returns only the first match and would silently drop notifications for the other matched users; `findAllByEmail` returns the full list and the hook iterates. + +The `username` fallback handles the legacy case documented at `User.groovy:20` (`String username // email or username`) — older accounts may hold the email only in `username`. `findAllByUsername` is used for symmetry, even though `username` IS constrained `unique: true` at `User.groovy:48` (so it'll always return 0 or 1). + +Silent skip is correct behavior for external recipients (vendors, carriers, partner admins): the email still gets sent; we just don't have a persistent home for an in-app notification. + +The lookup runs *after* `doSendMail` succeeds, so a slow lookup can't delay an email; and any exception in the lookup is caught and logged (never rethrown), so a transient DB failure on the lookup path can't break email sends. + +### D4. Notification content from email subject + body + +**Choice:** For v1, the notification's `title` is the email subject (stripped of the configured `[OpenBoxes]` prefix that `doSendMail` adds at line 181), and `body` is left null (the email body is HTML, not a clean fit for an in-app row). `link_url` is also null in v1. + +**Rationale:** Subject lines for the existing 16 trigger types are already human-readable (verified by spot-checking `NotificationService.groovy`'s shipment/requisition/user-lifecycle methods — each builds a subject like `"Shipment ${shipment.name} has been shipped"`). The HTML body is appropriate for an email but not for a 200px-wide dropdown row, and reformatting it for the dropdown is more work than it's worth for v1. Surfaces for type-specific titles, bodies, and `link_url` arrive in v2 when we have user feedback. + +### D5. Frontend delivery is 30-second polling, not WebSocket + +**Choice:** `NotificationBell` polls `GET /api/custom/notifications?unreadOnly=true&limit=20` every 30 seconds while mounted. + +**Rationale:** OpenBoxes has no existing WebSocket / SSE infrastructure. Adding it would 5×+ the project. Notifications are low-frequency, low-latency-tolerant ("you have a new shipment to review" doesn't need sub-second delivery). 30s × N concurrent users × a couple of small JSON responses is comfortably within the Grails server's headroom. A small ETag / `If-Modified-Since` optimization can be added later if load is measurable. + +### D6. Bell component is composed into the upstream Header, not a fork of the Header + +**Choice:** Add `` as a new sibling JSX element inside the existing `
    ` block in `src/js/components/Layout/Header.jsx`, positioned between `` and `` (the current children at `Header.jsx:43-44`). The component itself lives at `src/js/custom/notifications/components/NotificationBell.jsx`. + +**Rationale:** Per `rules/custom-package-isolation.md`, custom React lives under `src/js/custom//`. The Header is upstream; we don't fork it. Adding the bell as a sibling of `` rather than editing `NavbarIcons.jsx` (also upstream) confines the upstream touch to a single component. The `
      ` already wraps the bar's right-hand icon group, so a third sibling sits naturally there visually. + +Verified by reading `src/js/components/Layout/Header.jsx` (82 lines, single component): +- Lines 41–46 contain the `
      ` wrapper. +- Lines 42–45 contain `
      `. +- Insertion is a new `` line between `` and ``, plus one `import NotificationBell from '…'` line at the top. + +Note that an unrelated upstream `notification.jsx` already exists in `src/js/components/Layout/notifications/` (read 2026-05-22: it's a `react-s-alert` toast wrapper, not a persistent notification system). `react-s-alert` manages its own state internally — verified by grepping `src/js/reducers/`, `src/js/store/`, and `src/js/App.jsx` for `s-alert|sAlert|react-s-alert`, which returns zero hits. So a new Redux slice (or local hook state) under `src/js/custom/notifications/` cannot collide with the toast system. No name collision either because our feature lives under `src/js/custom/notifications/`. + +### D7. Feature name `notifications` (camelCase) for code paths + +**Choice:** Backend package `org.pih.warehouse.custom.notifications`, frontend folder `src/js/custom/notifications/`. The OpenSpec change name stays `in-app-notifications` (kebab-case per OpenSpec convention). + +**Rationale:** `rules/custom-package-isolation.md` explicitly requires camelCase for `` in code paths. The OpenSpec change name uses kebab-case per its own tooling convention. The two conventions live in different namespaces and don't conflict. + +### D8. URL mapping registration + +**Choice:** Add three mapping lines to `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy`: + +```groovy +"/api/custom/notifications"(controller: 'customNotification', action: 'list', method: 'GET') +"/api/custom/notifications/read-all"(controller: 'customNotification', action: 'markAllRead', method: 'PUT') +"/api/custom/notifications/$id/read"(controller: 'customNotification', action: 'markRead', method: 'PUT') +``` + +**Rationale:** Verified by reading the actual `UrlMappings.groovy` (file lives at `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy`, NOT the conventional `grails-app/conf/` — confirmed by `find`). Sample mappings at lines 45–192 show explicit per-route declarations (`/api/categories`, `/api/users`, `/api/products`, etc.) with no `/api/**` wildcard fallback, so each new endpoint needs its own mapping line. Three lines is the minimal surface for the three actions. Placement: append to the existing block of `/api/*` mappings, with a `// in-app-notifications (custom)` comment delimiter so the touch is easy to spot during upstream merges. + +`method:` constraints in the mapping mean a `POST` to `/api/custom/notifications` returns 405 rather than hitting an unrelated action — small but worthwhile. + +## Upstream Touch Points + +The following upstream files are modified by this change. Each edit is surgical and documented here so future upstream merges can target these locations. + +| File | Edit | Reason | +|---|---|---| +| `grails-app/services/org/pih/warehouse/core/MailService.groovy` | One `def customNotificationService` injection property near the existing service injections; one `try { customNotificationService.recordSendAsNotifications(to, subject) } catch (Exception ex) { log.error(...) }` block inserted between `email.send()` (current line 202) and `return true` (current line 203) | The one funnel point all emails flow through. See D1, D3. | +| `src/js/components/Layout/Header.jsx` | One `import NotificationBell from '…'` at the top; one `` JSX element inserted between `` and `` inside the `
        ` at current lines 42–45 | The component's mount point in the layout. See D6. | +| `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy` | Three mapping lines under a `// in-app-notifications (custom)` comment delimiter, appended to the existing `/api/*` block | Route mapping for the three REST endpoints. See D8. | +| `grails-app/i18n/messages.properties` | Append seven keys under a `# in-app-notifications (custom)` comment block (per `rules/custom-package-isolation.md` § "i18n exception") | Bell tooltip, empty/empty-unread, mark-all-read, two tab labels, unread-count aria-label. The root bundle is the only file Grails 3.3's `messageSource` actually loads. | +| `grails-app/controllers/org/pih/warehouse/SecurityInterceptor.groovy` | Append `'customNotification'` to `controllersWithLocationNotRequired` | The bell polls on every page including `chooseLocation`, so the controller must be reachable before the user has selected a warehouse. Without the exemption, every poll redirects to `chooseLocation` and the frontend gets an HTML body back instead of JSON. | + +## Risks / Trade-offs + +- **[Hook in upstream file = merge-conflict surface.]** `MailService.doSendMail` is the touch point most likely to drift in upstream releases. → Mitigation: keep the call to one line, document the touch point above, rely on the OpenSpec archive as the merge hitlist, and `git rerere` (documented in `.claude/docs/FORK_MAINTENANCE.md`) replays the same resolution automatically. +- **[Email-to-User mapping is heuristic.]** A user with their email in `username` rather than `email` will only be found via the fallback lookup; if they have *neither* `email` nor `username` set to their actual email, we silently skip — they keep getting emails but no in-app notification. → Mitigation: accept as a known limitation; add a v2 task to backfill `Person.email` for users where `username` looks like an email and `email` is null. +- **[`Person.email` is non-unique.]** Two `Person` rows (and therefore two `User` rows under joined inheritance) can share an email. Using `findByEmail` would silently drop notifications for all but the first match. → Mitigated by D3: use `findAllByEmail` and create one notification per match. Cost: a single email send to a shared mailbox creates N notifications (one per user); acceptable, since each user logs in independently and expects their own copy. +- **[Polling cost at high user counts.]** 30s polling × N logged-in users = N/30 RPS of small JSON requests against the API. At 500 concurrent users that's ~17 RPS, well within Grails headroom. At 5000+ it's worth revisiting. → Mitigation: defer; add ETag support if/when measured. +- **[Notification title quality depends on upstream subject lines.]** If a future upstream email trigger uses a generic subject like "Notification", users will see a generic title in the bell. → Mitigation: acceptable for v1; v2 introduces typed notifications with custom titles for the triggers we care about. +- **[Confusion with existing `notification.jsx`.]** Upstream already has a `notification.jsx` (toast wrapper). A reader looking for "the notification code" might land on the wrong file. → Mitigation: feature folder is `notifications` (not `notifications`); the component is `NotificationBell` (not `Notification`). +- **[Side effect inside `doSendMail` couples mail and notifications.]** A bug in the custom notification service could theoretically affect mail send timing. → Mitigation: the hook is wrapped in a try/catch that logs but never rethrows; the call happens *after* mail send returns true, so a notification-service exception cannot prevent or undo the email. + +## Migration Plan + +1. **Schema:** Liquibase changeset under `grails-app/migrations/custom/2026-05-22-create-custom-notification.groovy` adds the table and indexes (with `rollback` block dropping the table). One-line include in `grails-app/migrations/custom/changelog.groovy`. +2. **Backend:** Create domain class, service, controller, command-object (if needed for the controller). All under `org.pih.warehouse.custom.notifications`. +3. **Hook:** Edit `MailService.doSendMail` to inject the post-send call. +4. **Frontend:** Create `NotificationBell`, dropdown, API client, Redux slice (or local hook-based state) under `src/js/custom/notifications/`. +5. **Layout integration:** Edit `Header.jsx` to import and render ``. +6. **URL mapping:** Add the three mapping lines listed in D8 to `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy` (file location verified; no `/api/**` wildcard exists). +7. **Smoke test:** Trigger a known email (e.g., user creation), confirm a row appears in `custom_notification`, confirm the bell badge increments after the next 30s poll. + +**Rollback:** Drop the table (`rollback { dropTable(...) }` block in the changeset). Revert the upstream edits (the `MailService` injection + try/catch hook, the `Header.jsx` import + `` line, the three `UrlMappings.groovy` lines, the four `messages.properties` keys). Delete the custom files. No data outside the new table is touched. + +## Open Questions + +- **OQ1.** Should the bell's "click a notification" action open the related entity if we don't have `link_url` populated yet in v1? → Proposed answer: no, it just marks the row read in v1; v2 introduces typed notifications with deep-links. +- **OQ2.** Should the API be paginated, or is `limit=20` enough for the dropdown? → Proposed answer: `limit` is enough for the dropdown; if/when we add a full notifications page, add cursor pagination then. +- **OQ3.** Where should the `` sit in `Header.jsx` — left of the user menu, or right of the localization toggle? → Defer to the implementer reading the current header layout. + +## Validation + +Each criterion below is verifiable from an artifact outside this design (a file:line in the repo, a captured fixture, or a runnable test). + +- [ ] **V1. The `doSendMail` hook fires for every email send funneled through `MailService`.** Verifiable by reading `grails-app/services/org/pih/warehouse/core/MailService.groovy` (post-implementation) and confirming the call to `customNotificationService.recordSendAsNotifications` appears between `email.send()` (current line 202) and `return true` (current line 203) inside `doSendMail` — and NOT in any of the public `sendMail*` overloads above it. +- [ ] **V2. All public `sendMail*` overloads route through `doSendMail`.** Pre-implementation receipt: the 14 `return doSendMail(...)` lines at `MailService.groovy:62,66,70,74,78,82,86,90,94,101,105,109,113,117,121`. Post-implementation: those lines remain unchanged. +- [ ] **V3. `MailService.doSendMail` has exactly one success exit.** Pre-implementation receipt: `MailService.groovy:200-207` shows one `try { … email.send(); return true } catch { … return false }` block plus a dead unreachable `return false` at line 208. Hook placement is unambiguous. Verifiable post-implementation by grepping `return true` inside the `doSendMail` method body. +- [ ] **V4. `User.email` lookup against the inherited `Person.email` property works.** Receipt: `Person.groovy:21` declares `String email`; `Person.groovy:27-31` declares joined inheritance via `tablePerHierarchy false`; `User.groovy:17` declares `class User extends Person`. Therefore `User.findAllByEmail(string)` is a valid dynamic finder per `rules/groovy/patterns.md` "Dynamic finders" decision tree. +- [ ] **V5. `Person.email` is non-unique → `findAllByEmail` is required, not `findByEmail`.** Receipt: `Person.groovy:35-41` declares `static constraints { … email(nullable: true, email: true, maxSize: 255) … }` with no `unique:` clause. Verifiable post-implementation by reading `CustomNotificationService` and confirming it calls `User.findAllByEmail(...)` (returns a list) and iterates, not `User.findByEmail(...)`. +- [ ] **V6. `username` fallback covers the documented legacy case.** Receipt: `User.groovy:20` literally comments `String username // email or username` and `User.groovy:48` declares `username(blank: false, unique: true, maxSize: 255)` — the uniqueness constraint means the fallback returns 0 or 1 user. +- [ ] **V7. The custom migrations aggregator already exists.** Receipt: `grails-app/migrations/custom/changelog.groovy` exists with `databaseChangeLog = { /* No custom migrations yet — append `include file:` lines here as they ship. */ }`. The new changeset will be the first entry. +- [ ] **V8. The bell mounts in the upstream header at the documented position.** Receipt: `Header.jsx:42-45` currently contains `
        `. Verifiable by post-implementation diff: exactly one new `import NotificationBell from …;` line near the existing imports (lines 1–15) and exactly one new `` JSX element between `` and `` — no other edits, no reformatting (per `rules/custom-package-isolation.md` Boy Scout suspension). +- [ ] **V9. `UrlMappings.groovy` actually lives in `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy`.** Receipt: `find grails-app -name "UrlMappings*.groovy"` returns that single path; the conventional `grails-app/conf/` location does not have one. Verifiable by the diff: the three new `/api/custom/notifications*` mapping lines appear in `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy`, not anywhere else. +- [ ] **V10. No `/api/**` wildcard already covers the new endpoints.** Receipt: `grep -n "api" grails-app/controllers/org/pih/warehouse/UrlMappings.groovy` returns ~30 distinct `/api/` declarations (e.g. lines 45, 49, 55, 61, 67, 73, 79, 84, 89, 94, 99, 104, 109, 114, 119, 123, 128, 133, 138, 143, 148, 153, 158, 163, 168, 173, 178, 183, 188, 192…), none of which is a wildcard. Adding three explicit mappings is therefore necessary, not redundant. +- [ ] **V11. All backend custom files live under the camelCase feature package.** Verifiable with `find grails-app src/main -path '*custom/notifications/*' -o -path '*custom.notifications*'` matching every new backend file in the diff. +- [ ] **V12. All frontend custom files live under `src/js/custom/notifications/`.** Verifiable with `find src/js/custom/notifications -type f` matching every new frontend file in the diff. +- [ ] **V13. The new table has the read-state composite index.** Verifiable by reading the Liquibase changeset post-implementation: a `createIndex` (or `addIndex`) entry on `(user_id, is_read, date_created)` for `custom_notification`. +- [ ] **V14. The hook is exception-safe.** Verifiable by reading the post-implementation `doSendMail`: the call to `customNotificationService.recordSendAsNotifications(...)` is wrapped in a `try { ... } catch (Exception ex) { log.error(...) }` block so it never throws into the mail-send path. +- [ ] **V15. A scenario test exercises the multi-recipient case (3 emails, 2 known, 1 external → 2 notifications).** Verifiable: a Spock test under `src/test/groovy/org/pih/warehouse/custom/notifications/` that mocks `User.findAllByEmail` accordingly and asserts the exact row count. +- [ ] **V16. A scenario test exercises the non-unique-email case (1 email → 2 users → 2 notifications).** Verifiable: a Spock test that mocks `User.findAllByEmail(email)` to return a 2-element list and asserts both notifications are created with distinct `user_id` values. +- [ ] **V17. Polling interval is 30s.** Verifiable: grep `src/js/custom/notifications/` for `30000` or `30 * 1000` in the `setInterval`/`useInterval` call inside the polling hook. +- [ ] **V18. The custom notification feature has no Redux name collisions with the existing toast system.** Receipt: `grep -rn "s-alert\|sAlert\|react-s-alert" src/js/reducers/ src/js/store/ src/js/App.jsx` returns zero hits — the toast library manages its own state outside Redux. Verifiable post-implementation by confirming our Redux slice (if added) registers under a unique key (e.g. `customNotifications`) in the root reducer. +- [ ] **V19. Email subject lines used by all upstream triggers are user-readable.** Pre-implementation receipt: `grep -n "subject\|setSubject" grails-app/services/org/pih/warehouse/report/NotificationService.groovy` shows 10 subjects — five Groovy string-interpolated descriptive forms (e.g. `"Shipment ${shipmentInstance?.shipmentNumber} has been shipped"`, `"Expiry Alerts - ${location.name}"`, `"Application Error: ${exception?.message}"`) and five i18n-localized via `messageSource.getMessage(...)` / `messageLocalizer.localize(...)`. None are placeholder strings. Verifiable as a one-time visual check; titles render as their subjects in v1 without per-trigger title customization. +- [ ] **V20. The `[OpenBoxes]` prefix added by `doSendMail:181` is NOT propagated to notification titles.** Receipt: the `email.setSubject("${prefix} ${subject}")` call at line 181 mutates only the outgoing `Email` object's subject. The local `subject` parameter remains the raw input. The hook receives the raw `subject` argument, so notifications get the clean title with no `[OpenBoxes]` prefix. +- [ ] **V21. No upstream files outside the touch-point list are modified.** Verifiable: `git diff --name-only ..HEAD` against the merge base shows that every changed path is either under `custom/` (backend or frontend), under `grails-app/migrations/custom/`, or one of the four documented upstream files above (`MailService.groovy`, `Header.jsx`, `UrlMappings.groovy`, `messages.properties`). Anything else is a rule violation. + +## Verified Assumptions + +All assumptions that were flagged as unverified in the initial draft have been resolved against direct source reads or captured grep output. The receipts are inline in the Validation section above; the summary: + +- **UA1 → V4, V5** ✅ `Person.email` is non-unique. Design corrected to use `findAllByEmail` in D3. Service iterates over the result list. +- **UA2 → V3** ✅ `doSendMail` has exactly one success exit at line 203 (after `email.send()` at line 202). Hook placement is unambiguous. +- **UA3 → V9, V10** ✅ `UrlMappings.groovy` lives at `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy`, has no `/api/**` wildcard, requires three explicit mapping lines. D8 updated. +- **UA4 → V18** ✅ `react-s-alert` manages state internally; zero Redux collision risk. +- **UA5 → V8** ✅ Exact insertion point in `Header.jsx` confirmed (between `` and `` inside the `
          ` at lines 42–45). D6 updated. +- **UA6 → V19, V20** ✅ Subjects across the 10 spotted call-sites in `NotificationService.groovy` are descriptive or i18n-localized. The `[OpenBoxes]` prefix is added inside the outgoing email object only; the hook sees the clean subject. + +## Confidence: 9/10 + +**Rationale:** Every external claim in the design now has a direct receipt — a file:line, a grep result, or a captured pattern from the actual source. The architecture decisions (single funnel point at `MailService:200-207`, joined inheritance enabling `User.findAllByEmail`, no wildcard in `UrlMappings`, clean header insertion point at `Header.jsx:42-45`, zero Redux collision risk) are each backed by a verification step that can be re-run by any reader. The remaining 1-point uncertainty is honest implementation-time risk that no amount of pre-reads can eliminate: + +- Adding the Spring bean injection (`def customNotificationService` in `MailService`) on a Grails 3.3 service that already has a non-trivial set of bean dependencies — should "just work" via convention, but Grails 3.3 occasionally surprises with proxy/init ordering issues that only surface at boot. +- Polling-hook cancellation correctness across unmount + route changes — straightforward but a frequent source of `act()` warnings in Jest, so the test will be more iterative than the spec implies. + +Neither is a design issue; both are routine implementation friction. The plan is implementation-ready. diff --git a/openspec/changes/in-app-notifications/proposal.md b/openspec/changes/in-app-notifications/proposal.md new file mode 100644 index 00000000000..437c0958c04 --- /dev/null +++ b/openspec/changes/in-app-notifications/proposal.md @@ -0,0 +1,35 @@ +## Why + +Users only learn about supply-chain events (shipments, requisitions, stock alerts, account changes) through email, which forces them out of the app and is easy to miss in a busy inbox. Surfacing the same events as in-app notifications keeps users in context, gives them a persistent list they can act on, and unblocks a recurring TJK ask without changing how triggers work today. + +## What Changes + +- New `custom_notification` table holding per-user notification records (title, body, link, read state, type). +- New backend service `CustomNotificationService` (in `org.pih.warehouse.custom.notifications`) that creates and queries notifications. +- New REST controller exposing `GET /api/custom/notifications`, `PUT /api/custom/notifications/:id/read`, `PUT /api/custom/notifications/read-all`. +- Single surgical hook in upstream `MailService.doSendMail` that, after a successful send, resolves each recipient email to a `User` and creates a matching notification. Recipients that don't map to a `User` are skipped. +- React `NotificationBell` component (under `src/js/custom/notifications/`) rendered in the top nav: unread-count badge, dropdown with "Unread" (default) / "All" tabs, per-row mark-as-read with same-origin `link_url` navigation, "mark all read" action. +- Polling-based delivery: frontend polls `GET /api/custom/notifications` every 30s with the active tab's `unreadOnly` flag (no WebSocket). Response envelope `{ data, unreadCount }` lets the badge stay accurate without a second request. +- Liquibase changeset under `grails-app/migrations/custom/` adding the table and indexes. + +No existing behavior changes. No existing emails are removed or rewritten — notifications are an additive side-effect of the existing mail send. + +## Capabilities + +### New Capabilities + +- `in-app-notifications`: Per-user persistent notification records, REST API to list/mark-read, and an automatic creation hook that fires whenever the app sends an email to a recipient that matches a known user account. + +### Modified Capabilities + +_None._ This change does not alter any existing requirements — it's purely additive. + +## Impact + +- **Backend (new files only):** domain class, service, controller, URL mapping entry, Liquibase changeset under `custom/`. +- **Backend (upstream touch, surgical):** `grails-app/services/org/pih/warehouse/core/MailService.groovy` — a single post-send hook call after `doSendMail` returns `true`. Documented in `design.md` touch points. +- **Frontend (new files only):** `src/js/custom/notifications/` — bell component, dropdown with tabs, hook owning fetch + poll + tab state, API client. +- **Frontend (upstream touch, surgical):** the header/navbar component to slot in ``. One import + one tag. +- **Database:** new `custom_notification` table; no schema changes to existing tables. +- **External systems:** none. No new dependencies, no SMTP changes, no auth changes. +- **Rollout scope:** TJK branch first. Designed for clean cherry-pick to EST once stable. diff --git a/openspec/changes/in-app-notifications/specs/in-app-notifications/spec.md b/openspec/changes/in-app-notifications/specs/in-app-notifications/spec.md new file mode 100644 index 00000000000..14ff6e0233b --- /dev/null +++ b/openspec/changes/in-app-notifications/specs/in-app-notifications/spec.md @@ -0,0 +1,163 @@ +## ADDED Requirements + +### Requirement: Notification record persistence + +The system SHALL persist a notification record for each `User` account matched by an outgoing application email's recipient list. A single recipient email SHALL be matched against `Person.email` first; if no match is found, it SHALL be matched against `User.username` as a fallback. Recipient emails that match no `User` SHALL NOT cause a record to be persisted. A recipient email that matches multiple `User` accounts SHALL produce one record per matched user. + +#### Scenario: Email sent to a recipient matching exactly one user + +- **WHEN** the application successfully sends an email to `alice@example.com` and exactly one `User` has that email (via `Person.email` or `User.username`) +- **THEN** the system creates exactly one `custom_notification` row owned by that user, with `is_read = false`, a `date_created` timestamp, a non-empty `title`, and the email subject preserved as the notification title source + +#### Scenario: Email sent to a recipient matching multiple users + +- **WHEN** the application sends an email to `shared@team.example.com` and two `User` accounts share that email value on `Person.email` +- **THEN** the system creates exactly two `custom_notification` rows — one per matched user — each with a distinct `user_id` referencing one of the matches + +#### Scenario: Email matches only via username fallback + +- **WHEN** the application sends an email to `legacy.user@example.com` and no `Person.email` matches but a `User.username` does +- **THEN** the system creates one `custom_notification` row owned by the user whose `username` matched + +#### Scenario: Email sent to an unknown recipient + +- **WHEN** the application sends an email to `external-vendor@partner.com` and neither `Person.email` nor `User.username` matches +- **THEN** no `custom_notification` row is created and the email send is not affected + +#### Scenario: Email send fails + +- **WHEN** `MailService.doSendMail` returns `false` (mail send disabled, exception during construction, or exception during `send()`) +- **THEN** no `custom_notification` row is created for any recipient of that send + +#### Scenario: Multi-recipient email with mixed matches + +- **WHEN** an email is sent to three recipients — two whose emails each match a single distinct `User`, and one external recipient with no `User` match +- **THEN** exactly two `custom_notification` rows are created, one per matched user; the external recipient is silently skipped + +#### Scenario: Notification persistence failure does not break the email + +- **WHEN** the email send succeeds but a transient database error occurs while creating the `custom_notification` row +- **THEN** the error is logged and `doSendMail` still returns `true`; no exception propagates back into the mail send path + +### Requirement: Listing notifications for the current user + +The system SHALL expose `GET /api/custom/notifications` returning the authenticated user's notifications, newest first, with optional filters for unread-only and a result limit. The response SHALL be a JSON object of the shape `{ "data": [...], "unreadCount": N }` where `data` is the (filtered) list and `unreadCount` is the total unread count for the user — independent of the filter — so clients can drive a badge in one round trip. + +#### Scenario: Default list returns latest 20 + +- **WHEN** an authenticated user requests `GET /api/custom/notifications` with no parameters +- **THEN** the response returns up to 20 of that user's notifications, ordered by `created_at` descending, including read and unread, and the response body has the shape `{ "data": [...], "unreadCount": }` + +#### Scenario: Unread-only filter + +- **WHEN** the user requests `GET /api/custom/notifications?unreadOnly=true` +- **THEN** the response contains only notifications where `is_read = false` for the authenticated user + +#### Scenario: Limit parameter respected + +- **WHEN** the user requests `GET /api/custom/notifications?limit=5` +- **THEN** the response contains at most 5 notifications + +#### Scenario: Unauthenticated request rejected + +- **WHEN** an unauthenticated client requests `GET /api/custom/notifications` +- **THEN** the response is a 401 (or the framework's standard auth-failure response) and no notification data is returned + +#### Scenario: User cannot see another user's notifications + +- **WHEN** user A requests their notification list +- **THEN** the response contains only rows where `user_id` matches user A; user B's rows are never included + +### Requirement: Marking notifications as read + +The system SHALL allow the authenticated user to mark a single notification or all of their notifications as read. + +#### Scenario: Mark a single notification read + +- **WHEN** the user calls `PUT /api/custom/notifications/{id}/read` for a notification they own +- **THEN** the row's `is_read` becomes `true` and the response indicates success + +#### Scenario: Mark all read + +- **WHEN** the user calls `PUT /api/custom/notifications/read-all` +- **THEN** every notification owned by that user has `is_read` set to `true` + +#### Scenario: Cannot mark another user's notification + +- **WHEN** user A calls `PUT /api/custom/notifications/{id}/read` where the notification belongs to user B +- **THEN** the response is 403/404 and the row is unchanged + +### Requirement: In-app notification UI + +The system SHALL render a notification bell in the main application header for authenticated users, showing an unread count and an interactive dropdown. The dropdown SHALL expose two tabs — "Unread" (default) and "All" — so users can review history without losing the unread-first scanning model. + +#### Scenario: Unread badge reflects current state + +- **WHEN** the authenticated user has 3 unread notifications +- **THEN** the bell icon displays a badge with the value "3", sourced from the server-provided `unreadCount` field (not from filtering the list client-side) + +#### Scenario: Dropdown opens on the Unread tab by default + +- **WHEN** the user clicks the bell icon +- **THEN** the dropdown opens with the "Unread" tab active and lists the user's most recent unread notifications (newest first), each showing title, short body, relative timestamp, and a visual unread indicator +- **AND** the "Unread" tab label includes the current unread count + +#### Scenario: Switching to the All tab shows read + unread + +- **WHEN** the user clicks the "All" tab in the open dropdown +- **THEN** the client re-fetches `GET /api/custom/notifications?unreadOnly=false&limit=20` and the list shows both read and unread notifications, newest first +- **AND** read rows are visually distinct from unread (no unread indicator, lower-emphasis background) + +#### Scenario: Empty states are tab-specific + +- **WHEN** the active tab is "Unread" and the user has no unread notifications +- **THEN** the dropdown shows an "No unread notifications" empty state +- **WHEN** the active tab is "All" and the user has no notifications at all +- **THEN** the dropdown shows a "No notifications yet" empty state + +#### Scenario: Clicking an unread notification marks it read and navigates + +- **WHEN** the user clicks an unread notification row +- **THEN** the client calls `PUT /api/custom/notifications/{id}/read`, the row's unread indicator clears +- **AND** if the notification has a `link_url` that is a same-origin path (starts with `/`, not `//`), the user is navigated there via the SPA router; external or protocol-relative URLs are ignored for safety + +#### Scenario: Clicking an already-read notification does not re-mark it + +- **WHEN** the user clicks a notification that is already read +- **THEN** the client does NOT call `PUT /api/custom/notifications/{id}/read` +- **AND** `link_url` navigation still applies if present and same-origin + +#### Scenario: Mark-all-read action + +- **WHEN** the user clicks the "Mark all read" control in the dropdown +- **THEN** the client calls `PUT /api/custom/notifications/read-all` and the badge clears + +#### Scenario: Polling refresh + +- **WHEN** the bell component is mounted and the user is authenticated +- **THEN** the client polls `GET /api/custom/notifications` every 30 seconds with the parameters of the currently-active tab (`unreadOnly=true&limit=20` for "Unread", `unreadOnly=false&limit=20` for "All") and updates the badge and the visible list when the response changes +- **AND** the badge value is always derived from the response's `unreadCount` field, so it is accurate regardless of which tab is active + +### Requirement: Upstream isolation + +The system SHALL keep all custom notification code under `org.pih.warehouse.custom.notifications` (backend) and `src/js/custom/notifications/` (frontend), and SHALL place the database migration under `grails-app/migrations/custom/`. Edits to upstream files SHALL be limited to surgical hooks (one call site in `MailService.doSendMail`, one tag insertion in the header component). + +#### Scenario: Backend new files placement + +- **WHEN** a reviewer lists new backend files in the change +- **THEN** every new `.groovy` file resides under `grails-app/**/org/pih/warehouse/custom/notifications/` or `src/main/groovy/org/pih/warehouse/custom/notifications/` + +#### Scenario: Frontend new files placement + +- **WHEN** a reviewer lists new frontend files in the change +- **THEN** every new `.js`/`.jsx`/`.scss` file resides under `src/js/custom/notifications/` + +#### Scenario: Migration placement + +- **WHEN** a reviewer lists new Liquibase changesets +- **THEN** every new `.groovy` changeset resides under `grails-app/migrations/custom/` + +#### Scenario: Upstream edits are surgical + +- **WHEN** a reviewer diffs the upstream files touched by the change +- **THEN** `MailService.doSendMail` shows only an added post-send call to the custom notification service, and the header component shows only an import + a single `` tag — no reformatting, import reordering, or unrelated edits diff --git a/openspec/changes/in-app-notifications/tasks.md b/openspec/changes/in-app-notifications/tasks.md new file mode 100644 index 00000000000..b514945c9de --- /dev/null +++ b/openspec/changes/in-app-notifications/tasks.md @@ -0,0 +1,145 @@ +# Implementation Tasks — in-app-notifications + +Implementation order follows the `Migration Plan` in `design.md`. Each task is sized for one focused commit. + +## 0. Pre-implementation verification (resolved 2026-05-22) + +All Unverified Assumptions from the initial draft were resolved against direct source reads before implementation began. Receipts captured in `design.md` § Validation and § Verified Assumptions. Summary of the relevant findings (so implementation can move directly to step 1): + +- [x] **0.1** `Person.email` is **not unique** (`Person.groovy:35-41` — no `unique:` constraint). Service uses `User.findAllByEmail(...)` and iterates; one notification per matching `User`. +- [x] **0.2** `doSendMail` has a single success exit: `email.send()` at `MailService.groovy:202` followed by `return true` at line 203. Hook is inserted between these two lines. +- [x] **0.3** `UrlMappings.groovy` lives at `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy` (NOT `grails-app/conf/`). No `/api/**` wildcard exists — three explicit mapping lines are required. +- [x] **0.4** `Header.jsx:42-45` contains `
          `. The bell inserts between `` and `` as a third sibling inside the `
            `. +- [x] **0.5** `react-s-alert` (the existing `notification.jsx`) does not register a Redux slice (grep of `src/js/reducers/`, `src/js/store/`, `src/js/App.jsx` returned 0 hits). Our Redux slice / hook state cannot collide. + +## 1. Schema + +- [x] **1.1** Create `grails-app/migrations/custom/2026-05-22-create-custom-notification.groovy`: + - `createTable(tableName: 'custom_notification', ...)` with columns per `design.md` D2. + - `addForeignKeyConstraint` from `user_id` → `user(id)`. + - `createIndex` on `(user_id, is_read, date_created)`. + - `createIndex` on `(user_id, date_created)`. + - `rollback { dropTable(tableName: 'custom_notification') }`. + - `author: 'eyeseetea'`, id `2026-05-22-01-create-custom-notification`. +- [x] **1.2** Append `include file: '2026-05-22-create-custom-notification.groovy'` to `grails-app/migrations/custom/changelog.groovy`. +- [ ] **1.3** Run `./gradlew bootRun` (or apply migrations via standard mechanism) against a dev DB; confirm the table and indexes exist. + +## 2. Backend domain + service + +- [x] **2.1** Create `grails-app/domain/org/pih/warehouse/custom/notifications/CustomNotification.groovy`: + - Properties matching the table (`user`, `notificationType`, `title`, `body`, `linkUrl`, `isRead`, `readAt`, `dateCreated`, `lastUpdated`). + - `belongsTo = [user: User]`. + - `static constraints` with appropriate nullable/maxSize. + - `static mapping { table 'custom_notification'; id generator: 'uuid' }`. +- [x] **2.2** Create `grails-app/services/org/pih/warehouse/custom/notifications/CustomNotificationService.groovy`: + - `recordSendAsNotifications(Collection recipientEmails, String subject)`: + - For each email in the (de-duplicated) recipient list: + - `List matches = User.findAllByEmail(email)`. + - If `matches.empty`: `matches = User.findAllByUsername(email)` (legacy fallback per `User.groovy:20`). + - For each `User` in `matches`: create a `CustomNotification` row with `user`, `notificationType = 'EMAIL_PIGGYBACK'`, `title = subject`, `body = null`, `linkUrl = null`, `isRead = false`. + - Catch all exceptions and log via `log.error`; never rethrow (the hook in `MailService` is wrapped in a try/catch too, this is belt-and-suspenders). + - `listForUser(User user, Boolean unreadOnly, Integer limit)` — returns at most `limit` (default 20) rows owned by `user`, newest first. Uses the `(user_id, is_read, date_created)` index when `unreadOnly = true`. + - `markRead(String notificationId, User user)` — sets `isRead = true, readAt = now()` only if the row is owned by `user`. Returns `true` on success, `false` if the row doesn't exist or isn't owned by `user`. Does NOT throw — the controller decides the HTTP status from the return value. + - `markAllRead(User user)` — bulk update via HQL `executeUpdate("update CustomNotification n set n.isRead = true, n.readAt = :now where n.user = :user and n.isRead = false", [now: new Date(), user: user])`. Returns the number of rows updated. + - `countUnread(User user)` — `CustomNotification.countByUserAndIsRead(user, false)`. +- [x] **2.3** Unit tests (Spock) under `src/test/groovy/org/pih/warehouse/custom/notifications/CustomNotificationServiceSpec.groovy`: + - V15 — multi-recipient with mixed matches: 3 recipients, 2 known (each matched by `Person.email`), 1 external → exactly 2 rows created with distinct `user_id`. + - V16 — non-unique email: 1 recipient whose email matches 2 `User` rows → 2 notifications created, both referencing distinct users. + - Username-fallback case: `Person.email` returns empty, `User.username` returns 1 → 1 notification created. + - Cross-user mark-read attempt → returns `false`; row unchanged. + - `countUnread` after `markAllRead` → returns 0. + +## 3. Backend controller + URL mapping + +- [x] **3.1** Create `grails-app/controllers/org/pih/warehouse/custom/notifications/CustomNotificationController.groovy`: + - `def list()` → GET `/api/custom/notifications?unreadOnly=&limit=` returning JSON list. + - `def markRead(String id)` → PUT `/api/custom/notifications/:id/read`. + - `def markAllRead()` → PUT `/api/custom/notifications/read-all`. + - Use `springSecurityService.currentUser` for the actor; 401 if absent. + - Delegate all logic to `customNotificationService`; controller stays thin. +- [x] **3.2** Edit `grails-app/controllers/org/pih/warehouse/UrlMappings.groovy` (file location verified — NOT in `grails-app/conf/`). Append, after the existing `/api/*` block, under a `// in-app-notifications (custom)` comment delimiter: + ```groovy + // in-app-notifications (custom) + "/api/custom/notifications"(controller: 'customNotification', action: 'list', method: 'GET') + "/api/custom/notifications/read-all"(controller: 'customNotification', action: 'markAllRead', method: 'PUT') + "/api/custom/notifications/$id/read"(controller: 'customNotification', action: 'markRead', method: 'PUT') + ``` + No other edits to the file. No reformatting. +- [x] **3.3** Integration test under `src/integration-test/groovy/org/pih/warehouse/custom/notifications/CustomNotificationControllerIntegrationSpec.groovy` covering: + - List with default params, unread-only, and custom limit. + - Mark read on owned row. + - Mark read on someone else's row → 403/404. + - Unauthenticated request → 401. + +## 4. MailService hook (the one upstream backend edit) + +- [x] **4.1** Edit `grails-app/services/org/pih/warehouse/core/MailService.groovy`. Exactly two surgical changes: + + **(a)** Add `def customNotificationService` near the existing top-of-class service-injection properties (matches the convention from `rules/groovy/patterns.md` § "Dependency injection is property-based and convention-driven"). No other edits in the property block. + + **(b)** Inside `doSendMail`, between the existing `email.send()` and `return true` (currently lines 202 and 203 — exact line numbers may have shifted by a constant offset due to (a), but the structural location is unambiguous: it's between the only `email.send()` call and its immediately-following `return true` inside the `try` at lines 200–203), insert: + ```groovy + try { + customNotificationService.recordSendAsNotifications(to, subject) + } catch (Exception ex) { + log.error("custom_notification_record_failed subject='${subject}'", ex) + } + ``` + No reformatting of the surrounding `try { … } catch (Exception e) { … }` block. No reordering of imports. Boy Scout rule is suspended for this file. +- [ ] **4.2** Confirm via Spock test on `MailService` (or by running the user-create flow on a dev instance) that a real send produces a row. +- [ ] **4.3** Confirm an early-return (e.g., `!isMailEnabled` branch at line 154) does NOT create a notification — V3 / scenario "Email send fails". + +## 5. Frontend — bell component + API client + +- [x] **5.1** Create `src/js/custom/notifications/api/notificationsApi.js` — `getNotifications({unreadOnly, limit})`, `markRead(id)`, `markAllRead()` using the standard `apiClient`. +- [x] **5.2** Create `src/js/custom/notifications/hooks/useNotifications.js` — manages list state, polling (30s interval), mark-read/mark-all-read mutations. Cleanly cancels on unmount. +- [x] **5.3** Create `src/js/custom/notifications/components/NotificationBell.jsx`: + - Bell icon (use the existing `react-icons/ri` family for visual consistency — see existing `Layout/notifications/notification.jsx` imports). + - Unread badge with count (hidden when zero). + - Click toggles dropdown. +- [x] **5.4** Create `src/js/custom/notifications/components/NotificationDropdown.jsx`: + - Renders the list (newest first). + - Each row: title, relative timestamp, unread dot. + - Row click → `markRead(id)`. + - "Mark all read" action at the top. + - Empty state when no notifications. +- [x] **5.5** Add SCSS at `src/js/custom/notifications/styles/_bell.scss` (or `.scss` co-located). Bootstrap 4.6 base. + +## 6. Layout integration (the one upstream frontend edit) + +- [x] **6.1** Edit `src/js/components/Layout/Header.jsx` (82 lines; small, simple file). Exactly two surgical changes: + + **(a)** Add `import NotificationBell from 'custom/notifications/components/NotificationBell';` to the absolute-imports group (existing absolute imports are at lines 7–13). Place it alphabetically — between `Logo` and `Menu`. Match the existing `eslint-plugin-simple-import-sort` order from `rules/web/coding-style.md`. + + **(b)** Inside the `
              ` element (currently at lines 42–45), insert `` as a new sibling element between `` and ``. The resulting `
                ` block reads: + ```jsx +
                  + + + +
                + ``` + **No other edits.** No reformatting, no PropTypes additions, no `mapStateToProps` changes. Boy Scout rule is suspended for this file. + +## 7. i18n + +- [x] **7.1** Append the feature's keys to the root `grails-app/i18n/messages.properties` under a `# in-app-notifications (custom)` comment block. Keys at minimum: + - `notifications.bell.title=Notifications` + - `notifications.bell.empty=No notifications yet` + - `notifications.bell.markAllRead=Mark all as read` + - `notifications.bell.unreadCount={0} unread` +- [x] **7.2** Document the touch in `design.md` touch points (already covered in the rules; `messages.properties` is the one upstream file that always gets appended). + +## 8. Smoke test + verification gate + +- [ ] **8.1** Run `./gradlew test`. +- [ ] **8.2** Run `npm test`. +- [ ] **8.3** Manual smoke: trigger one of the known email flows (account creation is the easiest), confirm a row appears in `custom_notification`, confirm the bell badge increments within 30s, confirm clicking the row clears the badge. +- [ ] **8.4** Walk through `design.md` § Validation V1–V21 and check each off. +- [ ] **8.5** Run `git diff --name-only` against the merge base; confirm only the four upstream files in the touch-points list are changed (`MailService.groovy`, `Header.jsx`, `UrlMappings.groovy`, `messages.properties`), everything else is under `custom/`. + +## 9. Documentation + close-out + +- [ ] **9.1** Update PR description to link to OpenSpec change + summarize. +- [ ] **9.2** Confirm `design.md` § Upstream Touch Points lists every upstream file actually modified (including the conditional `UrlMappings.groovy` if it was edited). +- [ ] **9.3** Open the PR onto `release/est/tjk/0.9.7`. Note in the description that promotion to EST is a follow-up cherry-pick once stabilized. diff --git a/openspec/changes/notifications-decouple-email/.openspec.yaml b/openspec/changes/notifications-decouple-email/.openspec.yaml new file mode 100644 index 00000000000..9e883bff049 --- /dev/null +++ b/openspec/changes/notifications-decouple-email/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-25 diff --git a/openspec/changes/notifications-decouple-email/design.md b/openspec/changes/notifications-decouple-email/design.md new file mode 100644 index 00000000000..87ebf877c68 --- /dev/null +++ b/openspec/changes/notifications-decouple-email/design.md @@ -0,0 +1,116 @@ +## Context + +The in-app notifications feature (see the `in-app-notifications` change) was deliberately built as a side-effect of email: a hook at `MailService.doSendMail` (`grails-app/services/org/pih/warehouse/core/MailService.groovy:205`) fires `customNotificationService.recordSendAsNotifications(to, subject, body)` immediately after `email.send()` succeeds. That was the minimal-upstream-footprint choice for v1, and it works — but it bakes in three structural limits: + +1. **Mail-config coupling.** `doSendMail` returns early at line 155 (`if (!isMailEnabled && !override) { ... return false }`) *before* the hook. Turn email off → notifications stop. +2. **Send-success coupling.** The hook is inside the `try { email.send(); ...; return true }` block. If SMTP is down or the send throws, no notification is recorded even though the business event happened. +3. **Email-address coupling.** The hook only sees `to` — a list of email *strings*. Recipients are built upstream as `users.collect { it.email }`, so a `User` with no email (`Person.email` is `nullable: true`, `Person.groovy:39`) is dropped from `to` and is invisible at the hook. There is no token representing that user to attach a notification to. + +An agent scan of all 23 `MailService` call sites found that **11 of the 18 "user objects in scope" sites funnel through one file** — `grails-app/services/org/pih/warehouse/report/NotificationService.groovy` — at the points where `User`/`Person` recipients are resolved (shipment shipped/received, requisition pending-approval/status, fulfillment, stock/expiry alerts, user-account create/confirm, application errors). That file is the natural choke point for an event-driven, user-keyed notification trigger. + +This change moves the trigger from "an email was sent" to "the app decided to notify person X," recorded by `User`, independent of email. + +## Goals / Non-Goals + +**Goals:** + +- Record in-app notifications from the originating business event, keyed by `User`, regardless of whether an email is also sent, whether the send succeeds, or whether the mail-enabled config is on. +- Reach users who have no email address. +- Expose a first-class `notifyUsers(Collection, String title, String body, NotificationType type)` on `CustomNotificationService`. +- Add an independent enable flag `openboxes.notifications.inApp.enabled` (default `true`), configured **only** in `docker/openboxes.yml` and `docker/openboxes.client-template.yml`. +- Keep all decision/logic in our custom service so the upstream touch is just call insertions — leaving a clean future upstream-contribution path ("promote `notifyUsers` into `NotificationService`, drop the `custom` namespacing"). + +**Non-Goals:** + +- **DHIS2 sync.** This change only makes notifications user-keyed and email-independent (which *enables* a later sync). The sync itself is a separate change. +- **The ~7 controller-level mail sends** outside `NotificationService` (`UserController` ×3, `ProductController`, `ShipmentController`, `CreateShipmentWorkflowController`, `ErrorsController`). Phase 2. +- **Per-user notification preferences, typing/filtering, WebSocket delivery.** Unchanged from the v1 non-goals. +- **Changing what emails are sent.** Every email sent today is still sent; only notification *recording* moves. +- **Frontend changes.** The bell/dropdown/modal/API are untouched. + +## Decisions + +### D1. New `notifyUsers(Collection, title, body, type)` on `CustomNotificationService` + +**Choice:** Add a method that takes resolved `User` objects (not email strings) and writes one `CustomNotification` per user, reusing the existing `withNewSession { withTransaction { ... } }` wrapper (background-thread safe — stock/expiry alerts run on GPars workers, see `recordSendAsNotifications`). + +**Rationale:** The recipients are already `User`/`Person` objects at the `NotificationService` call sites; passing them directly removes the email→user re-resolution entirely (no `findAllByEmail`, no non-uniqueness handling, no `username` fallback). The notification is keyed by the authoritative `User`, which is exactly what a future DHIS2-UID mapping needs. + +**Alternatives considered:** +- **Keep `recordSendAsNotifications(emails, ...)` and also pass users.** Rejected — two code paths, double-recording risk, and the email-string path is the thing we're trying to eliminate. +- **Spring event (`notify('user:notify', ...)`) + listener.** Rejected for now — adds a listener service and indirection for no benefit over a direct method call at a single choke-point file. Could be retrofitted if the call sites multiply. + +### D2. Remove the `MailService.doSendMail` hook + +**Choice:** Delete the `try { customNotificationService?.recordSendAsNotifications(...) } catch (Throwable t) { ... }` block (currently `MailService.groovy:204-208`) and the `def customNotificationService` injection if it's otherwise unused. + +**Rationale:** Once `NotificationService` records notifications by user, leaving the mail hook in place would double-record for every emailed user. Removing it also *reverts* an upstream touch point — a net reduction in fork surface on `MailService` — and breaks the email coupling at its root. `recordSendAsNotifications` is removed (or kept only if Phase 2 controller sites still need an email-based fallback; current plan: remove). + +### D3. Enable flag owned by the custom service, configured in docker only + +**Choice:** `CustomNotificationService.notifyUsers` checks `grailsApplication.config.openboxes.notifications.inApp.enabled` (default `true` when unset) and no-ops when disabled. The key is declared **only** in `docker/openboxes.yml` and `docker/openboxes.client-template.yml`. + +**Rationale:** Keeping the flag-read in *our* service means the upstream `NotificationService` calls stay unconditional one-liners — no flag logic leaks into upstream files. Per project config rules and the user's explicit instruction, custom config goes in the docker config + client template, never in `application.yml`/`application.groovy`. Default-on preserves current behavior unless a deployment opts out. Email (`grails.mail.*` / `isMailEnabled`) and in-app notifications are now independently switchable. + +### D4. Hook placement inside `NotificationService` — capture users before `.collect { it.email }` + +**Choice:** At each of the ~11 recipient-resolution points, insert one `customNotificationService.notifyUsers(, subject, body, )` call on the `User`/`Person` collection that already exists in scope, *before* it is mapped to emails. Group under a `// in-app-notifications (custom)` comment for merge visibility. + +**Rationale:** These are the semantic events. The `users`/`recipients`/`subscribers`/`recipient` variables are already in scope (the agent scan captured the variable per site). Recording there is email-independent and covers the high-value notifications in one file. Per-site `NotificationType` lets v2 typing land without re-touching these lines. + +**Per-site inventory (verified by agent scan; confirm exact lines at implementation time):** `NotificationService.groovy` ~lines 128, 131 (stock/expiry alerts, `subscribers`), 166 (shipment-items shipped, `recipient`), 175 (shipment notifications, `users`), 199–206 (receipt notifications, `recipient`), 221 (application error, `subscribers`), 237 (user-account creation, `recipients`), 252 (user-account confirmation, `userInstance`), 290 (requisition pending-approval, `recipient`), 302 (requisition status update, `recipient`), 312 (fulfillment, `requestor`). + +### D5. No schema, no frontend, no API change + +**Choice:** Reuse the existing `custom_notification` table, `NotificationType` enum (add values only if a site needs a distinct type), controller, and React/GSP bells unchanged. + +**Rationale:** The DTO contract (`{ id, type, title, body, linkUrl, read, createdAt }` + `unreadCount`) and the bell behavior are independent of what writes the rows. Only the write trigger moves. + +## Upstream Touch Points + +| File | Edit | Reason | +|---|---|---| +| `grails-app/services/org/pih/warehouse/report/NotificationService.groovy` | ~11 one-line `customNotificationService.notifyUsers(...)` calls at recipient-resolution points, under a `// in-app-notifications (custom)` comment; one `def customNotificationService` injection | The event-level choke point. See D1, D4. **New touch point.** | +| `grails-app/services/org/pih/warehouse/core/MailService.groovy` | **Remove** the post-send hook block and (if now unused) the `def customNotificationService` injection | Eliminates double-recording and the email coupling. See D2. **Reverts a prior touch point.** | +| `docker/openboxes.yml` | Add `openboxes.notifications.inApp.enabled: true` | Deployment default for the new flag. See D3. | +| `docker/openboxes.client-template.yml` | Add the same key (documented, default `true`) | Per-client override surface. See D3. | + +Net effect on `MailService` is a *reduction* in fork surface (one touch point removed). `application.yml` / `application.groovy` are intentionally **not** touched. + +## Risks / Trade-offs + +- **[New touch point in `NotificationService` = merge surface.]** ~11 insertions in an upstream file that changes across upstream releases. → Mitigation: one line each under a single comment delimiter; documented above; `git rerere` (see `.claude/docs/FORK_MAINTENANCE.md`) replays resolutions; and the planned upstream contribution would retire the surface entirely. +- **[Behavior change: record on event, not on successful send.]** Notifications now appear even if the email later fails or is disabled. → This is the *intended* semantics (independent channel), but it is a change from v1. Called out in the proposal; verify with a smoke test that disabling mail still yields notifications. +- **[Coverage gap vs. the old funnel.]** The old `MailService` hook caught *all* 23 sites; `NotificationService` covers ~11. The ~7 controller-level sites (and any non-`NotificationService` mail) will no longer create notifications until Phase 2. → Mitigation: the dropped sites are low-value (test email, photo-changed, product-created, error reports, admin free-text, stocklist free-text); enumerate them in tasks and schedule Phase 2. Confirm with PM that none are must-haves for the first cut. +- **[Double-recording during transition.]** If the `NotificationService` calls land before the `MailService` hook is removed, emailed users get two rows. → Mitigation: remove the hook in the same change/commit as adding the calls; a test asserts a single email-triggering event yields exactly one notification per user. +- **[Flag default drift.]** A deployment whose `openboxes.yml` predates this change has no key set. → Mitigation: default-to-`true` in the service when the config is absent, so missing config preserves current behavior. + +## Migration Plan + +1. **Service:** Add `notifyUsers(Collection, String title, String body, NotificationType type)` to `CustomNotificationService` (reuse `withNewSession { withTransaction { } }`); add the `openboxes.notifications.inApp.enabled` config read with default-true. +2. **Hook in:** Add the ~11 `notifyUsers(...)` calls in `NotificationService.groovy` at the resolution points (D4), plus the `def customNotificationService` injection. +3. **Hook out:** Remove the `MailService.doSendMail` post-send hook and unused injection (D2). Remove `recordSendAsNotifications` (and its email-resolution tests) unless retained for a documented Phase 2 reason. +4. **Config:** Add the flag to `docker/openboxes.yml` and `docker/openboxes.client-template.yml`. +5. **Tests:** Unit/integration for `notifyUsers` (records one row per user incl. an email-less user; no-ops when flag disabled); a test that a representative `NotificationService` event records exactly one notification per recipient user; a smoke check that mail-disabled still notifies. +6. **Verify:** `./gradlew compileGroovy test --tests "*.custom.notifications.*"`; manual check that a stock alert with `isMailEnabled=false` still creates rows. + +**Rollback:** Re-add the `MailService` hook, remove the `NotificationService` calls and the config key, restore `recordSendAsNotifications`. No schema change, so no DB rollback. The `custom_notification` table and frontend are untouched. + +## Open Questions + +- **OQ1 (RESOLVED — yes).** Email-less-user coverage IS a requirement for this cut. That is the core motivation; `notifyUsers` keys by `User` so an absent email never excludes a recipient. An empty recipient set is a no-op (nothing to record), not an error. +- **OQ2.** Do any of the ~7 Phase-2 controller-level sends need to be in the first cut? Default assumption: no. +- **OQ3 (RESOLVED — domain-level types).** Type each notification by domain at the source (cheap now, costly to back-fill later since it means re-touching the upstream hooks). Add to the custom `NotificationType` enum: `SHIPMENT` (shipped/received), `REQUISITION` (pending-approval/status), `FULFILLMENT`, `STOCK_ALERT` (stock/expiry), `USER_ACCOUNT` (creation/confirmation), `SYSTEM` (application errors); keep `EMAIL_TRIGGER` as the fallback for anything not cleanly classifiable. Granularity is domain-level, not per-action (finer than any UI/DHIS2 mapping will use). The bell does not filter by type yet — the value is future filtering/icons and DHIS2 categorization without re-touching `NotificationService`. + +## Validation + +- [ ] **V1. `notifyUsers` records by `User`, not email.** Verifiable by reading `CustomNotificationService` post-implementation: signature takes `Collection`; body constructs `new CustomNotification(user: user, ...)` with no `findAllByEmail`/`findAllByUsername` call. +- [ ] **V2. Email-less user is reachable.** Integration test: a `User` with `email == null` passed to `notifyUsers` gets a `custom_notification` row. (Contrast with the old path, where they never appeared.) +- [ ] **V3. The mail coupling is gone.** Verifiable by grepping `MailService.groovy` post-implementation: no reference to `customNotificationService` / `recordSendAsNotifications` remains inside `doSendMail`. +- [ ] **V4. Notifications independent of mail-enabled.** Test/smoke: with `isMailEnabled=false`, invoking a `NotificationService` event still creates notification rows (records before, and regardless of, the `doSendMail` early return). +- [ ] **V5. Flag disables recording.** Unit test: with `openboxes.notifications.inApp.enabled=false`, `notifyUsers` writes zero rows; with the key absent, it writes rows (default-true). +- [ ] **V6. No double-recording.** Test: a single `NotificationService` event whose recipients are all emailed yields exactly one notification per user (not two). +- [ ] **V7. Flag is only in docker config.** Verifiable: `grep -rn "notifications.inApp.enabled"` matches `docker/openboxes.yml` and `docker/openboxes.client-template.yml` and **not** `grails-app/conf/application.yml` or `application.groovy`. +- [ ] **V8. Upstream touch is surgical and reversible.** Verifiable by diff: `NotificationService.groovy` changes are only `notifyUsers(...)` call lines + one injection under the comment delimiter; `MailService.groovy` change is a pure deletion of the prior hook; no reformatting elsewhere. +- [ ] **V9. No frontend/schema/API change.** Verifiable: diff touches no file under `src/js/`, no Liquibase changeset, no controller/UrlMappings change. +- [ ] **V10. Phase-2 sites are enumerated.** The tasks list names the ~7 deferred controller-level sites so the coverage gap is explicit, not silent. diff --git a/openspec/changes/notifications-decouple-email/proposal.md b/openspec/changes/notifications-decouple-email/proposal.md new file mode 100644 index 00000000000..f003116548e --- /dev/null +++ b/openspec/changes/notifications-decouple-email/proposal.md @@ -0,0 +1,39 @@ +## Why + +In-app notifications currently exist only as a side-effect of sending email: a hook inside `MailService.doSendMail` fires *after* a successful send. That couples a user-facing channel to an unrelated transport, with three consequences — notifications never record when the mail-enabled config is off, they never record when an SMTP send fails, and they can never reach users who have no email address (those users are dropped from the recipient list before `doSendMail` is ever called). A notification channel should stand on its own: triggered by the business event, delivered to a `User`, regardless of whether an email also goes out. + +## What Changes + +- **BREAKING (internal):** Remove the post-send hook in `MailService.doSendMail`. Notifications are no longer a byproduct of email delivery. +- Add a first-class `notifyUsers(Collection users, String title, String body, NotificationType type)` method to `CustomNotificationService` that records one notification per user — keyed by `User`, never by email string. +- Hook `notifyUsers` into `NotificationService` (upstream `org.pih.warehouse.report.NotificationService`) at the ~11 points where `User`/`Person` recipients are already resolved, *before* they are reduced to email strings via `.collect { it.email }`. This is the single choke point for the high-value events (shipment shipped/received, requisition pending-approval and status, fulfillment, stock/expiry alerts, user-account events, application errors). +- Add an independent enable flag `openboxes.notifications.inApp.enabled` (default `true`), read by `CustomNotificationService` so the on/off logic lives in our code, not in the upstream mail path. Email and in-app notifications become independently switchable. +- Configure the flag **only** in `docker/openboxes.yml` and the docker client template (`docker/openboxes.client-template.yml`) — **not** in `application.yml`/`application.groovy`. +- Email-less users are now reachable (their `User` is captured before the email filter), which lays the groundwork for a future DHIS2 sync (notifications keyed by `User` → mappable to a DHIS2 user UID). The DHIS2 sync itself is **out of scope** for this change. + +Out of scope / Phase 2: the ~7 scattered controller-level mail sends (`UserController`, `ProductController`, `ShipmentController`, `CreateShipmentWorkflowController`, `ErrorsController`) that build recipients outside `NotificationService`. + +Resolved: email-less-user coverage IS required (it is the core motivation). Notification types are domain-level (`SHIPMENT`, `REQUISITION`, `FULFILLMENT`, `STOCK_ALERT`, `USER_ACCOUNT`, `SYSTEM`, with `EMAIL_TRIGGER` as fallback) — see design.md OQ3. + +## Capabilities + +### New Capabilities + +_None._ This refines an existing capability rather than introducing a new one. + +### Modified Capabilities + +- `in-app-notifications`: The notification-creation requirement changes from "created as a side-effect of a successful email send" to "created from the originating business event, by `User`, independent of email transport and of the mail-enabled config." Adds the `openboxes.notifications.inApp.enabled` flag requirement. Removes the `MailService` recipient-email→user resolution requirement. + +## Impact + +- **Backend (our custom files):** `CustomNotificationService` gains `notifyUsers(...)` + the enable-flag read; existing `recordSendAsNotifications` (email-string based) is removed or superseded. +- **Backend (upstream touch, surgical):** + - `grails-app/services/org/pih/warehouse/core/MailService.groovy` — **remove** the existing post-send hook (reverts a touch point we previously added). + - `grails-app/services/org/pih/warehouse/report/NotificationService.groovy` — add ~11 one-line `notifyUsers(...)` calls at the recipient-resolution points. New upstream touch point; documented in `design.md`. +- **Config:** new key `openboxes.notifications.inApp.enabled` in `docker/openboxes.yml` and `docker/openboxes.client-template.yml` only. +- **Database:** none. Reuses the existing `custom_notification` table. +- **Frontend:** none. The bell, dropdown, modal, and REST API are unchanged — only *what triggers a notification row* changes server-side. +- **External systems:** none in this change. (Enables a later, separate DHIS2 sync.) +- **Upstream-contribution note:** keeping `notifyUsers` in `CustomNotificationService` and the calls surgical means a future upstream PR is mostly "promote `notifyUsers` into `NotificationService` natively and drop the `custom` namespacing." DHIS2 stays in the fork. +- **Rollout scope:** TJK/EST branch. Clean cherry-pick target once stable. diff --git a/openspec/changes/notifications-decouple-email/specs/in-app-notifications/spec.md b/openspec/changes/notifications-decouple-email/specs/in-app-notifications/spec.md new file mode 100644 index 00000000000..2fd890a9741 --- /dev/null +++ b/openspec/changes/notifications-decouple-email/specs/in-app-notifications/spec.md @@ -0,0 +1,92 @@ +## MODIFIED Requirements + +### Requirement: Notification record persistence + +The system SHALL persist a notification record for each `User` resolved at the originating business event, keyed by the `User` object (not by email address). Notification persistence SHALL be independent of email: it SHALL occur regardless of whether an email is also sent for the event, whether any email send succeeds, and whether the mail-enabled configuration is on. A `User` without an email address SHALL still receive a notification record. Persistence failures SHALL be logged and SHALL NOT propagate into the calling business logic. + +#### Scenario: Event notifies a single user + +- **WHEN** a business event in `NotificationService` resolves a recipient `User` and invokes `notifyUsers([user], title, body, type)` +- **THEN** the system creates exactly one `custom_notification` row owned by that user, with `is_read = false`, a `date_created` timestamp, and a non-empty `title` + +#### Scenario: Event notifies a user with no email address + +- **WHEN** a business event resolves a `User` whose `Person.email` is null and invokes `notifyUsers([user], ...)` +- **THEN** the system creates one `custom_notification` row owned by that user (the absence of an email does not prevent the notification) + +#### Scenario: Notification recorded when email is disabled + +- **WHEN** the mail-enabled configuration is off (`isMailEnabled = false`) and a business event invokes `notifyUsers([user], ...)` +- **THEN** the system still creates the `custom_notification` row (no email is sent, but the in-app notification is recorded) + +#### Scenario: Notification recorded when email send fails + +- **WHEN** a business event invokes `notifyUsers([user], ...)` and the corresponding email send later throws or returns false +- **THEN** the `custom_notification` row is still present (notification recording does not depend on send success) + +#### Scenario: Event notifies multiple users + +- **WHEN** a business event resolves three recipient users and invokes `notifyUsers(users, ...)` +- **THEN** the system creates exactly three `custom_notification` rows, one per user, each with a distinct `user_id` + +#### Scenario: No double recording for emailed users + +- **WHEN** a single business event both records notifications via `notifyUsers` and sends an email to the same users +- **THEN** exactly one `custom_notification` row exists per user for that event (the email send does NOT additionally create notifications) + +#### Scenario: Persistence failure does not break the business event + +- **WHEN** `notifyUsers` is invoked and a transient database error occurs while creating a row +- **THEN** the error is logged and the exception does not propagate back into the calling `NotificationService` method or the email path + +## ADDED Requirements + +### Requirement: Independent in-app notification toggle + +The system SHALL provide a configuration flag `openboxes.notifications.inApp.enabled` that enables or disables in-app notification recording independently of the email configuration. When the flag is absent, the system SHALL default to enabled. The flag SHALL be configured only in the docker configuration (`docker/openboxes.yml`) and the docker client template (`docker/openboxes.client-template.yml`), and SHALL NOT be added to `application.yml` or `application.groovy`. + +#### Scenario: Recording disabled by flag + +- **WHEN** `openboxes.notifications.inApp.enabled` is `false` and a business event invokes `notifyUsers(...)` +- **THEN** no `custom_notification` row is created + +#### Scenario: Recording enabled by flag + +- **WHEN** `openboxes.notifications.inApp.enabled` is `true` and a business event invokes `notifyUsers(...)` +- **THEN** notification rows are created as specified by "Notification record persistence" + +#### Scenario: Default-on when unconfigured + +- **WHEN** the flag is not present in any loaded configuration and a business event invokes `notifyUsers(...)` +- **THEN** the system behaves as if the flag were `true` and creates notification rows + +#### Scenario: Independent of email toggle + +- **WHEN** email is enabled but `openboxes.notifications.inApp.enabled` is `false` +- **THEN** emails are still sent but no in-app notifications are recorded; and conversely, when email is disabled but the in-app flag is on, notifications are recorded while no email is sent + +## MODIFIED Requirements + +### Requirement: Upstream isolation + +The system SHALL keep all custom notification code under `org.pih.warehouse.custom.notifications` (backend) and `src/js/custom/notifications/` (frontend), and SHALL place any database migration under `grails-app/migrations/custom/`. Edits to upstream files SHALL be limited to surgical hooks: `notifyUsers(...)` call sites in `NotificationService` at points where recipient `User` objects are already resolved, removal of the prior `MailService.doSendMail` notification hook, and the in-app enable flag in the docker configuration files. The on/off and recording logic SHALL live in the custom service so the upstream call sites remain unconditional one-liners. + +#### Scenario: Backend new files placement + +- **WHEN** a reviewer lists new backend files in the change +- **THEN** every new `.groovy` file resides under `grails-app/**/org/pih/warehouse/custom/notifications/` or `src/main/groovy/org/pih/warehouse/custom/notifications/` + +#### Scenario: NotificationService hook is surgical + +- **WHEN** a reviewer diffs `grails-app/services/org/pih/warehouse/report/NotificationService.groovy` +- **THEN** the change shows only added `customNotificationService.notifyUsers(...)` call lines (and one service injection) grouped under a `// in-app-notifications (custom)` comment — no reformatting, import reordering, or unrelated edits + +#### Scenario: MailService hook removed + +- **WHEN** a reviewer diffs `grails-app/services/org/pih/warehouse/core/MailService.groovy` +- **THEN** the prior post-send notification hook block (and its now-unused service injection) is removed, and no other lines are reformatted + +#### Scenario: Flag config placement + +- **WHEN** a reviewer searches for `notifications.inApp.enabled` +- **THEN** it appears only in `docker/openboxes.yml` and `docker/openboxes.client-template.yml`, and not in `grails-app/conf/application.yml` or `application.groovy` diff --git a/openspec/changes/notifications-decouple-email/tasks.md b/openspec/changes/notifications-decouple-email/tasks.md new file mode 100644 index 00000000000..2b2a524f26b --- /dev/null +++ b/openspec/changes/notifications-decouple-email/tasks.md @@ -0,0 +1,60 @@ +# Implementation Tasks — notifications-decouple-email + +Implementation order follows the `Migration Plan` in `design.md`. Each task is sized for one focused commit. The custom service changes (group 1) land before the upstream hook-in/hook-out (groups 2–3) so notifications keep working throughout; the `NotificationService` calls and the `MailService` hook removal MUST ship in the same change to avoid double-recording (design.md Risks). + +## 0. Pre-implementation verification + +- [ ] **0.1** Confirm the exact recipient-resolution lines in `grails-app/services/org/pih/warehouse/report/NotificationService.groovy` and the in-scope `User`/`Person` variable at each (design.md D4 lists the agent-scanned estimates ~128, 131, 166, 175, 199–206, 221, 237, 252, 290, 302, 312). Capture the real line numbers + variable names as receipts before editing. +- [ ] **0.2** Confirm `Person.email` is `nullable: true` (`grails-app/domain/org/pih/warehouse/core/Person.groovy:39`) so the email-less scenario is real. +- [ ] **0.3** Confirm the current `MailService.doSendMail` hook block location (`grails-app/services/org/pih/warehouse/core/MailService.groovy` ~204–208) and the `def customNotificationService` injection, to scope the removal. +- [ ] **0.4** Confirm the config-read idiom used elsewhere (`grailsApplication.config.openboxes...`) and how `docker/openboxes.yml` keys map into `grailsApplication.config`, so the flag read is correct. + +## 1. Custom service — `notifyUsers` + enable flag + +- [ ] **1.1** Add `notifyUsers(Collection users, String title, String body, NotificationType type)` to `CustomNotificationService`: + - No-op (return) when the resolved enable flag is false. + - Reuse `CustomNotification.withNewSession { CustomNotification.withTransaction { ... } }` (background-thread safe; stock/expiry alerts run on GPars workers). + - De-duplicate the user collection; for each `User`, create a `CustomNotification` with `user: user`, `notificationType: type?.name() ?: NotificationType.EMAIL_TRIGGER.name()`, the `title`/`body`, `isRead: false`. + - Per-user `try/catch` logging; never rethrow into the caller. + - Apply the same blank-title fallback already used by the service (`'(no subject)'`). +- [ ] **1.2** Add the enable-flag read: a private helper reading `grailsApplication.config.openboxes.notifications.inApp.enabled`, defaulting to `true` when unset/null. Inject `def grailsApplication` if not already present. +- [ ] **1.3** Decide the fate of `recordSendAsNotifications`: remove it (and its email-resolution unit/integration tests) unless a documented Phase-2 reason keeps it. Default: remove. + +## 2. Hook in — `NotificationService` + +- [ ] **2.1** Add `def customNotificationService` injection to `NotificationService`. +- [ ] **2.2** Extend the custom `NotificationType` enum with domain values: `SHIPMENT`, `REQUISITION`, `FULFILLMENT`, `STOCK_ALERT`, `USER_ACCOUNT`, `SYSTEM` (keep `EMAIL_TRIGGER` as fallback). See design.md OQ3. +- [ ] **2.3** At each recipient-resolution point confirmed in 0.1, insert one `customNotificationService.notifyUsers(, subject, body, )` call on the in-scope `User`/`Person` collection, BEFORE any `.collect { it.email }`. Group/annotate with a `// in-app-notifications (custom)` comment for merge visibility. Pass the matching domain `NotificationType` per site (shipment→`SHIPMENT`, requisition→`REQUISITION`, etc.); fall back to `EMAIL_TRIGGER` only where the domain isn't clear. +- [ ] **2.4** For single-`Person` sites (e.g. `recipient`, `requestor`), wrap as a single-element collection (`[recipient]`) — `notifyUsers` takes a `Collection`. + +## 3. Hook out — `MailService` + +- [ ] **3.1** Remove the post-send hook block in `doSendMail` (the `try { customNotificationService?.recordSendAsNotifications(...) } catch (Throwable t) { ... }`). +- [ ] **3.2** Remove the now-unused `def customNotificationService` injection from `MailService` (verify no other reference first). + +## 4. Config + +- [ ] **4.1** Add `openboxes.notifications.inApp.enabled: true` to `docker/openboxes.yml`. +- [ ] **4.2** Add the same key (with a brief comment, default `true`) to `docker/openboxes.client-template.yml`. +- [ ] **4.3** Confirm the key is NOT added to `grails-app/conf/application.yml` or `application.groovy`. + +## 5. Tests + +- [ ] **5.1** Integration: `notifyUsers` creates one row per user, including a `User` whose `email` is null (the email-less scenario). Assert concrete `user_id`s and `title`. +- [ ] **5.2** Integration/unit: with the enable flag `false`, `notifyUsers` writes zero rows; with the key absent, it writes rows (default-true). +- [ ] **5.3** Integration: a representative `NotificationService` event records exactly one notification per recipient user (no double-recording). +- [ ] **5.4** Smoke/behavioral note: with `isMailEnabled=false`, the same event still records notifications (records independent of the `doSendMail` early-return). +- [ ] **5.5** Update/remove the controller and integration specs that asserted the old email-string path (`recordSendAsNotifications`) per task 1.3. + +## 6. Verify + +- [ ] **6.1** `./gradlew compileGroovy test --tests "*.custom.notifications.*"` is green. +- [ ] **6.2** Rebuild + redeploy the docker image; trigger a stock/expiry alert and confirm a `custom_notification` row appears for a recipient (including one with no email, if available), and that the bell shows it. +- [ ] **6.3** Diff review against design.md § Validation V1–V10 (user-keyed, email-independent, flag-gated, surgical upstream touch, docker-only config, no frontend/schema change). + +## 7. Documentation + +- [ ] **7.1** Update this change's `design.md` "Upstream Touch Points" if the confirmed line numbers/variables differ from the estimates. +- [ ] **7.2** On archive, update the `in-app-notifications` archived `design.md`/spec to reflect that the `MailService` hook was removed and the trigger moved to `NotificationService` (the archive is the patch manifest). +- [ ] **7.3** Note the Phase-2 follow-up (the ~7 controller-level mail sends: `UserController`, `ProductController`, `ShipmentController`, `CreateShipmentWorkflowController`, `ErrorsController`) so the coverage gap is tracked, not silent. +- [ ] **7.4** Record the open PM question (email-less coverage requirement) and its resolution. From 97b84378a463dfe634a3fd431628885a2e233e25 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Mon, 25 May 2026 10:58:38 +0800 Subject: [PATCH 03/24] feat(notifications): add notification domain, service, controller, API --- .../org/pih/warehouse/UrlMappings.groovy | 6 ++ .../CustomNotificationController.groovy | 89 +++++++++++++++++++ .../notifications/CustomNotification.groovy | 38 ++++++++ .../notifications/NotificationType.groovy | 11 +++ ...26-05-22-create-custom-notification.groovy | 58 ++++++++++++ grails-app/migrations/custom/changelog.groovy | 6 +- .../CustomNotificationService.groovy | 84 +++++++++++++++++ 7 files changed, 290 insertions(+), 2 deletions(-) create mode 100644 grails-app/controllers/org/pih/warehouse/custom/notifications/CustomNotificationController.groovy create mode 100644 grails-app/domain/org/pih/warehouse/custom/notifications/CustomNotification.groovy create mode 100644 grails-app/domain/org/pih/warehouse/custom/notifications/NotificationType.groovy create mode 100644 grails-app/migrations/custom/2026-05-22-create-custom-notification.groovy create mode 100644 grails-app/services/org/pih/warehouse/custom/notifications/CustomNotificationService.groovy diff --git a/grails-app/controllers/org/pih/warehouse/UrlMappings.groovy b/grails-app/controllers/org/pih/warehouse/UrlMappings.groovy index dbd7159a405..610fa6de385 100644 --- a/grails-app/controllers/org/pih/warehouse/UrlMappings.groovy +++ b/grails-app/controllers/org/pih/warehouse/UrlMappings.groovy @@ -1120,6 +1120,12 @@ class UrlMappings { action = [GET: "getExpirationHistoryReport"] } + // in-app-notifications (custom) + "/api/custom/notifications"(controller: 'customNotification', action: 'list', method: 'GET') + "/api/custom/notifications/unread-count"(controller: 'customNotification', action: 'unreadCount', method: 'GET') + "/api/custom/notifications/read-all"(controller: 'customNotification', action: 'markAllRead', method: 'PUT') + "/api/custom/notifications/$id/read"(controller: 'customNotification', action: 'markRead', method: 'PUT') + // Error handling "401"(controller: "errors", action: "handleUnauthorized") diff --git a/grails-app/controllers/org/pih/warehouse/custom/notifications/CustomNotificationController.groovy b/grails-app/controllers/org/pih/warehouse/custom/notifications/CustomNotificationController.groovy new file mode 100644 index 00000000000..ad9c52c3cfc --- /dev/null +++ b/grails-app/controllers/org/pih/warehouse/custom/notifications/CustomNotificationController.groovy @@ -0,0 +1,89 @@ +package org.pih.warehouse.custom.notifications + +import grails.converters.JSON +import org.pih.warehouse.auth.AuthService + +class CustomNotificationController { + + private static final List ISO_DATE_FORMATS = [ + "yyyy-MM-dd'T'HH:mm:ss.SSSX", + "yyyy-MM-dd'T'HH:mm:ssX", + ] + + def customNotificationService + + private Date parseIsoDate(String value) { + if (!value) { + return null + } + for (String fmt : ISO_DATE_FORMATS) { + try { + return Date.parse(fmt, value) + } catch (Exception ignored) { + // try next format + } + } + log.warn "custom_notifications_unparseable_date value='${value}'" + return null + } + + def list() { + def currentUser = AuthService.currentUser + if (!currentUser) { + render status: 401 + return + } + Boolean unreadOnly = params.boolean('unreadOnly', false) + Integer limit = Math.min(Math.max(params.int('limit', 20), 1), 100) + Integer offset = Math.max(params.int('offset', 0), 0) + Date since = parseIsoDate(params.since as String) + Date updatedSince = parseIsoDate(params.updatedSince as String) + List notifications = customNotificationService.listForUser(currentUser, unreadOnly, limit, offset, since, updatedSince) + Integer unreadCount = customNotificationService.countUnread(currentUser) + def data = notifications.collect { notification -> + [ + id : notification.id, + type : notification.notificationType, + title : notification.title, + body : notification.body, + linkUrl : notification.linkUrl, + read : notification.isRead, + createdAt : notification.dateCreated, + ] + } + render([data: data, unreadCount: unreadCount] as JSON) + } + + def unreadCount() { + def currentUser = AuthService.currentUser + if (!currentUser) { + render status: 401 + return + } + render([unreadCount: customNotificationService.countUnread(currentUser)] as JSON) + } + + def markRead(String id) { + def currentUser = AuthService.currentUser + if (!currentUser) { + render status: 401 + return + } + Boolean success = customNotificationService.markRead(id, currentUser) + if (!success) { + render status: 404 + return + } + render status: 200 + } + + def markAllRead() { + def currentUser = AuthService.currentUser + if (!currentUser) { + render status: 401 + return + } + Integer count = customNotificationService.markAllRead(currentUser) + render([updatedCount: count] as JSON) + } +} diff --git a/grails-app/domain/org/pih/warehouse/custom/notifications/CustomNotification.groovy b/grails-app/domain/org/pih/warehouse/custom/notifications/CustomNotification.groovy new file mode 100644 index 00000000000..0a7ff6c9178 --- /dev/null +++ b/grails-app/domain/org/pih/warehouse/custom/notifications/CustomNotification.groovy @@ -0,0 +1,38 @@ +package org.pih.warehouse.custom.notifications + +import org.pih.warehouse.core.User + +class CustomNotification { + + String id + User user + String notificationType + String title + String body + String linkUrl + Boolean isRead = false + Date readAt + Date dateCreated + Date lastUpdated + + static constraints = { + notificationType blank: false, maxSize: 64 + title blank: false, maxSize: 255 + body nullable: true + linkUrl nullable: true, maxSize: 2048 + readAt nullable: true + user nullable: false + } + + static mapping = { + table 'custom_notification' + id generator: 'uuid' + user column: 'user_id' + isRead column: 'is_read' + readAt column: 'read_at' + dateCreated column: 'date_created' + lastUpdated column: 'last_updated' + notificationType column: 'notification_type' + linkUrl column: 'link_url' + } +} diff --git a/grails-app/domain/org/pih/warehouse/custom/notifications/NotificationType.groovy b/grails-app/domain/org/pih/warehouse/custom/notifications/NotificationType.groovy new file mode 100644 index 00000000000..6cdf73a2936 --- /dev/null +++ b/grails-app/domain/org/pih/warehouse/custom/notifications/NotificationType.groovy @@ -0,0 +1,11 @@ +package org.pih.warehouse.custom.notifications + +/** + * Source that produced a CustomNotification. Stored as its name() in the + * custom_notification.notification_type column. + */ +enum NotificationType { + + /** Recorded as a side-effect of an application email send (MailService.doSendMail). */ + EMAIL_TRIGGER +} diff --git a/grails-app/migrations/custom/2026-05-22-create-custom-notification.groovy b/grails-app/migrations/custom/2026-05-22-create-custom-notification.groovy new file mode 100644 index 00000000000..b35c2f27cbb --- /dev/null +++ b/grails-app/migrations/custom/2026-05-22-create-custom-notification.groovy @@ -0,0 +1,58 @@ +databaseChangeLog = { + changeSet(id: '2026-05-22-01-create-custom-notification', author: 'eyeseetea') { + createTable(tableName: 'custom_notification') { + column(name: 'id', type: 'varchar(255)') { + constraints(nullable: false, primaryKey: true, primaryKeyName: 'custom_notificationPK') + } + column(name: 'version', type: 'bigint') { + constraints(nullable: false) + } + column(name: 'user_id', type: 'varchar(255)') { + constraints(nullable: false) + } + column(name: 'notification_type', type: 'varchar(64)') { + constraints(nullable: false) + } + column(name: 'title', type: 'varchar(255)') { + constraints(nullable: false) + } + column(name: 'body', type: 'text') { + constraints(nullable: true) + } + column(name: 'link_url', type: 'varchar(2048)') { + constraints(nullable: true) + } + column(name: 'is_read', type: 'boolean', defaultValueBoolean: false) { + constraints(nullable: false) + } + column(name: 'read_at', type: 'datetime') { + constraints(nullable: true) + } + column(name: 'date_created', type: 'datetime') { + constraints(nullable: false) + } + column(name: 'last_updated', type: 'datetime') { + constraints(nullable: false) + } + } + addForeignKeyConstraint( + baseTableName: 'custom_notification', + baseColumnNames: 'user_id', + referencedTableName: 'user', + referencedColumnNames: 'id', + constraintName: 'fk_custom_notification_user' + ) + createIndex(indexName: 'idx_custom_notification_user_unread', tableName: 'custom_notification') { + column(name: 'user_id') + column(name: 'is_read') + column(name: 'date_created') + } + createIndex(indexName: 'idx_custom_notification_user_created', tableName: 'custom_notification') { + column(name: 'user_id') + column(name: 'date_created') + } + rollback { + dropTable(tableName: 'custom_notification') + } + } +} diff --git a/grails-app/migrations/custom/changelog.groovy b/grails-app/migrations/custom/changelog.groovy index 9fc44bae2e8..9dfb1f88133 100644 --- a/grails-app/migrations/custom/changelog.groovy +++ b/grails-app/migrations/custom/changelog.groovy @@ -6,11 +6,13 @@ * * 1. Drop a file under grails-app/migrations/custom/-.groovy * with a `databaseChangeLog = { changeSet(...) { ... rollback { ... } } }` block. - * 2. Append a one-line `include file: '-.groovy'` below. + * 2. Append a one-line `include file: 'custom/-.groovy'` below. + * (Liquibase resolves include paths relative to the migrations root, not + * this aggregator's directory — so the `custom/` prefix is required.) * * Order include lines by FK dependency (target tables above holder tables). * See .claude/rules/custom-package-isolation.md for the full rules. */ databaseChangeLog = { - // No custom migrations yet — append `include file:` lines here as they ship. + include file: 'custom/2026-05-22-create-custom-notification.groovy' } diff --git a/grails-app/services/org/pih/warehouse/custom/notifications/CustomNotificationService.groovy b/grails-app/services/org/pih/warehouse/custom/notifications/CustomNotificationService.groovy new file mode 100644 index 00000000000..5658a67a4f5 --- /dev/null +++ b/grails-app/services/org/pih/warehouse/custom/notifications/CustomNotificationService.groovy @@ -0,0 +1,84 @@ +package org.pih.warehouse.custom.notifications + +import grails.gorm.transactions.Transactional +import org.pih.warehouse.core.User + +class CustomNotificationService { + + void recordSendAsNotifications(Collection recipientEmails, String subject, String body = null) { + String trimmedSubject = subject?.trim() + String safeTitle = !trimmedSubject ? '(no subject)' + : (trimmedSubject.size() > 255 ? trimmedSubject[0..251] + '...' : trimmedSubject) + Set uniqueEmails = (recipientEmails ?: []) as Set + // Bind a Hibernate session and transaction explicitly: this method is + // called from MailService.doSendMail which runs on a variety of threads + // (HTTP request, Quartz jobs, GPars workers). Background threads do + // not have an OSIV-bound session, so GORM queries throw 'No Session + // found for current thread' without an explicit withNewSession. + CustomNotification.withNewSession { + CustomNotification.withTransaction { + uniqueEmails.each { String email -> + try { + List users = User.findAllByEmail(email) + if (!users) { + users = User.findAllByUsername(email) + } + users.each { User user -> + CustomNotification notification = new CustomNotification( + user: user, + notificationType: NotificationType.EMAIL_TRIGGER.name(), + title: safeTitle, + body: body, + ) + if (!notification.save(flush: false)) { + log.error "custom_notification_record_failed email='${email}' user_id='${user.id}' errors=${notification.errors.allErrors*.code}" + } + } + } catch (Exception ex) { + log.error "custom_notification_record_failed email='${email}' subject='${subject}'", ex + } + } + } + } + } + + List listForUser(User user, Boolean unreadOnly = false, Integer limit = 20, Integer offset = 0, Date since = null, Date updatedSince = null) { + CustomNotification.createCriteria().list(max: limit, offset: offset) { + eq('user', user) + if (unreadOnly) { + eq('isRead', false) + } + if (since) { + ge('dateCreated', since) + } + if (updatedSince) { + ge('lastUpdated', updatedSince) + } + order('dateCreated', 'desc') + } + } + + @Transactional + Boolean markRead(String notificationId, User user) { + CustomNotification notification = CustomNotification.get(notificationId) + if (!notification || notification.user?.id != user.id) { + return false + } + notification.isRead = true + notification.readAt = new Date() + notification.save(flush: true) + return true + } + + @Transactional + Integer markAllRead(User user) { + CustomNotification.executeUpdate( + 'update CustomNotification n set n.isRead = true, n.readAt = :now where n.user = :user and n.isRead = false', + [now: new Date(), user: user] + ) + } + + Integer countUnread(User user) { + CustomNotification.countByUserAndIsRead(user, false) + } +} From 221ca0bd64ca1afea762abc644afff74500185e7 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Mon, 25 May 2026 10:58:38 +0800 Subject: [PATCH 04/24] feat(notifications): record notifications on email send --- .../org/pih/warehouse/SecurityInterceptor.groovy | 2 +- .../services/org/pih/warehouse/core/MailService.groovy | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/grails-app/controllers/org/pih/warehouse/SecurityInterceptor.groovy b/grails-app/controllers/org/pih/warehouse/SecurityInterceptor.groovy index a47c29df03c..6e153a749d2 100644 --- a/grails-app/controllers/org/pih/warehouse/SecurityInterceptor.groovy +++ b/grails-app/controllers/org/pih/warehouse/SecurityInterceptor.groovy @@ -18,7 +18,7 @@ class SecurityInterceptor { static ArrayList controllersWithAuthUserNotRequired = ['test', 'errors'] static ArrayList actionsWithAuthUserNotRequired = ['status', 'test', 'login', 'logout', 'handleLogin', 'signup', 'handleSignup', 'json', 'updateAuthUserLocale', 'viewLogo', 'changeLocation', 'menu'] - static ArrayList controllersWithLocationNotRequired = ['categoryApi', 'productApi', 'genericApi', 'api'] + static ArrayList controllersWithLocationNotRequired = ['categoryApi', 'productApi', 'genericApi', 'api', 'customNotification'] static ArrayList actionsWithLocationNotRequired = ['status', 'test', 'login', 'logout', 'handleLogin', 'signup', 'handleSignup', 'json', 'updateAuthUserLocale', 'viewLogo', 'chooseLocation', 'menu'] def authService diff --git a/grails-app/services/org/pih/warehouse/core/MailService.groovy b/grails-app/services/org/pih/warehouse/core/MailService.groovy index 401d4cade4e..2589f9307f5 100644 --- a/grails-app/services/org/pih/warehouse/core/MailService.groovy +++ b/grails-app/services/org/pih/warehouse/core/MailService.groovy @@ -21,6 +21,7 @@ import javax.mail.util.ByteArrayDataSource class MailService { Config config = Holders.getConfig() + def customNotificationService String getDefaultFrom() { return config.getProperty("grails.mail.from") @@ -200,6 +201,11 @@ class MailService { try { log.info "sending ${summary}" email.send() + try { + customNotificationService?.recordSendAsNotifications(to, subject, body) + } catch (Throwable t) { + log.error("custom_notification_hook_failed subject='${subject}'", t) + } return true } catch (Exception e) { log.error("could not send ${summary}", e) From 2b36b7a05100c55cde3b3b08d5ad0289cc2ec4c6 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Mon, 25 May 2026 10:58:38 +0800 Subject: [PATCH 05/24] feat(notifications): add i18n keys for notification bell --- grails-app/i18n/messages.properties | 13 +++++++++++++ grails-app/i18n/messages_ru.properties | 13 +++++++++++++ grails-app/i18n/messages_tg.properties | 13 +++++++++++++ 3 files changed, 39 insertions(+) diff --git a/grails-app/i18n/messages.properties b/grails-app/i18n/messages.properties index 4b8333fd77a..b881902626c 100644 --- a/grails-app/i18n/messages.properties +++ b/grails-app/i18n/messages.properties @@ -4961,3 +4961,16 @@ customStockTransferDocument.upload.invalidFilename.error=File name is missing or react.custom.stockTransferDocuments.upload.invalidType.error=Unsupported file type. Allowed: PDF, image, Word, Excel, CSV, ZIP. react.custom.stockTransferDocuments.upload.tooLarge.error=File is too large. react.custom.stockTransferDocuments.upload.invalidFilename.error=File name is missing or invalid. + +# in-app-notifications (custom) +notifications.bell.title=Notifications +notifications.bell.empty=No notifications yet +notifications.bell.emptyUnread=No unread notifications +notifications.bell.markAllRead=Mark all as read +notifications.bell.tabUnread=Unread +notifications.bell.tabAll=All +notifications.bell.loadMore=Load more +notifications.bell.unreadCount={0} unread +notifications.modal.noBody=No additional details +notifications.modal.close=Close +notifications.modal.openLink=Open link diff --git a/grails-app/i18n/messages_ru.properties b/grails-app/i18n/messages_ru.properties index 50569ed5adc..582b6bc7832 100644 --- a/grails-app/i18n/messages_ru.properties +++ b/grails-app/i18n/messages_ru.properties @@ -4919,3 +4919,16 @@ customStockTransferDocument.upload.invalidFilename.error=\u0418\u043c\u044f \u04 react.custom.stockTransferDocuments.upload.invalidType.error=\u041d\u0435\u043f\u043e\u0434\u0434\u0435\u0440\u0436\u0438\u0432\u0430\u0435\u043c\u044b\u0439 \u0442\u0438\u043f \u0444\u0430\u0439\u043b\u0430. \u0420\u0430\u0437\u0440\u0435\u0448\u0435\u043d\u044b: PDF, \u0438\u0437\u043e\u0431\u0440\u0430\u0436\u0435\u043d\u0438\u0435, Word, Excel, CSV, ZIP. react.custom.stockTransferDocuments.upload.tooLarge.error=\u0424\u0430\u0439\u043b \u0441\u043b\u0438\u0448\u043a\u043e\u043c \u0431\u043e\u043b\u044c\u0448\u043e\u0439. react.custom.stockTransferDocuments.upload.invalidFilename.error=\u0418\u043c\u044f \u0444\u0430\u0439\u043b\u0430 \u043e\u0442\u0441\u0443\u0442\u0441\u0442\u0432\u0443\u0435\u0442 \u0438\u043b\u0438 \u043d\u0435\u043a\u043e\u0440\u0440\u0435\u043a\u0442\u043d\u043e. + +# in-app-notifications (custom) +notifications.bell.title=\u0423\u0432\u0435\u0434\u043e\u043c\u043b\u0435\u043d\u0438\u044f +notifications.bell.empty=\u0423\u0432\u0435\u0434\u043e\u043c\u043b\u0435\u043d\u0438\u0439 \u043f\u043e\u043a\u0430 \u043d\u0435\u0442 +notifications.bell.emptyUnread=\u041d\u0435\u0442 \u043d\u0435\u043f\u0440\u043e\u0447\u0438\u0442\u0430\u043d\u043d\u044b\u0445 \u0443\u0432\u0435\u0434\u043e\u043c\u043b\u0435\u043d\u0438\u0439 +notifications.bell.markAllRead=\u041e\u0442\u043c\u0435\u0442\u0438\u0442\u044c \u0432\u0441\u0435 \u043a\u0430\u043a \u043f\u0440\u043e\u0447\u0438\u0442\u0430\u043d\u043d\u044b\u0435 +notifications.bell.tabUnread=\u041d\u0435\u043f\u0440\u043e\u0447\u0438\u0442\u0430\u043d\u043d\u044b\u0435 +notifications.bell.tabAll=\u0412\u0441\u0435 +notifications.bell.loadMore=\u0417\u0430\u0433\u0440\u0443\u0437\u0438\u0442\u044c \u0435\u0449\u0451 +notifications.bell.unreadCount={0} \u043d\u0435\u043f\u0440\u043e\u0447\u0438\u0442\u0430\u043d\u043d\u044b\u0445 +notifications.modal.noBody=\u041d\u0435\u0442 \u0434\u043e\u043f\u043e\u043b\u043d\u0438\u0442\u0435\u043b\u044c\u043d\u044b\u0445 \u0441\u0432\u0435\u0434\u0435\u043d\u0438\u0439 +notifications.modal.close=\u0417\u0430\u043a\u0440\u044b\u0442\u044c +notifications.modal.openLink=\u041e\u0442\u043a\u0440\u044b\u0442\u044c \u0441\u0441\u044b\u043b\u043a\u0443 diff --git a/grails-app/i18n/messages_tg.properties b/grails-app/i18n/messages_tg.properties index 1c507f79f40..33290e9fa20 100644 --- a/grails-app/i18n/messages_tg.properties +++ b/grails-app/i18n/messages_tg.properties @@ -4919,3 +4919,16 @@ customStockTransferDocument.upload.invalidFilename.error=\u041d\u043e\u043c\u043 react.custom.stockTransferDocuments.upload.invalidType.error=\u041d\u0430\u0432\u044a\u0438 \u0444\u0430\u0439\u043b\u0438 \u0434\u0430\u0441\u0442\u0433\u0438\u0440\u043d\u0430\u0448\u0430\u0432\u0430\u043d\u0434\u0430. \u0418\u04b7\u043e\u0437\u0430\u0442 \u0434\u043e\u0434\u0430 \u0448\u0443\u0434\u0430\u0430\u0441\u0442: PDF, \u0442\u0430\u0441\u0432\u0438\u0440, Word, Excel, CSV, ZIP. react.custom.stockTransferDocuments.upload.tooLarge.error=\u0424\u0430\u0439\u043b \u0430\u0437 \u04b3\u0430\u0434 \u0437\u0438\u0451\u0434 \u043a\u0430\u043b\u043e\u043d \u0430\u0441\u0442. react.custom.stockTransferDocuments.upload.invalidFilename.error=\u041d\u043e\u043c\u0438 \u0444\u0430\u0439\u043b \u043d\u0435\u0441\u0442 \u0451 \u043d\u043e\u0434\u0443\u0440\u0443\u0441\u0442 \u0430\u0441\u0442. + +# in-app-notifications (custom) +notifications.bell.title=\u041e\u0433\u043e\u04b3\u0438\u04b3\u043e +notifications.bell.empty=\u04b2\u043e\u043b\u043e \u043e\u0433\u043e\u04b3\u0438\u0435 \u043d\u0435\u0441\u0442 +notifications.bell.emptyUnread=\u041e\u0433\u043e\u04b3\u0438\u0438 \u043d\u0430\u0445\u043e\u043d\u0434\u0430 \u043d\u0435\u0441\u0442 +notifications.bell.markAllRead=\u04b2\u0430\u043c\u0430\u0440\u043e \u0445\u043e\u043d\u0434\u0430 \u0448\u0443\u0434\u0430 \u049b\u0430\u0439\u0434 \u043a\u0443\u043d\u0435\u0434 +notifications.bell.tabUnread=\u041d\u0430\u0445\u043e\u043d\u0434\u0430 +notifications.bell.tabAll=\u04b2\u0430\u043c\u0430 +notifications.bell.loadMore=\u0411\u0435\u0448\u0442\u0430\u0440 +notifications.bell.unreadCount={0} \u043d\u0430\u0445\u043e\u043d\u0434\u0430 +notifications.modal.noBody=\u041c\u0430\u044a\u043b\u0443\u043c\u043e\u0442\u0438 \u0438\u043b\u043e\u0432\u0430\u0433\u04e3 \u043d\u0435\u0441\u0442 +notifications.modal.close=\u041f\u04ef\u0448\u0438\u0434\u0430\u043d +notifications.modal.openLink=\u041a\u0443\u0448\u043e\u0434\u0430\u043d\u0438 \u0438\u0441\u0442\u0438\u043d\u043e\u0434 From 0f4534ab4ccaf02a64f089403366cae6eea61e6b Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Mon, 25 May 2026 11:00:46 +0800 Subject: [PATCH 06/24] feat(notifications): add notification bell UI (React + GSP) --- grails-app/views/common/_menuicons.gsp | 1 + .../views/custom/notifications/_bell.gsp | 380 +++++++++++++++++ src/js/components/Layout/Header.jsx | 2 + .../__tests__/NotificationBell.test.jsx | 100 +++++ .../__tests__/NotificationDropdown.test.jsx | 201 +++++++++ .../__tests__/NotificationModal.test.jsx | 127 ++++++ .../__tests__/notificationsApi.test.js | 55 +++ .../__tests__/useNotifications.test.js | 254 ++++++++++++ .../notifications/api/notificationsApi.js | 15 + .../components/NotificationBell.jsx | 80 ++++ .../components/NotificationDropdown.jsx | 158 +++++++ .../components/NotificationModal.jsx | 125 ++++++ .../notifications/hooks/useNotifications.js | 167 ++++++++ src/js/custom/notifications/styles/_bell.scss | 388 ++++++++++++++++++ 14 files changed, 2053 insertions(+) create mode 100644 grails-app/views/custom/notifications/_bell.gsp create mode 100644 src/js/custom/notifications/__tests__/NotificationBell.test.jsx create mode 100644 src/js/custom/notifications/__tests__/NotificationDropdown.test.jsx create mode 100644 src/js/custom/notifications/__tests__/NotificationModal.test.jsx create mode 100644 src/js/custom/notifications/__tests__/notificationsApi.test.js create mode 100644 src/js/custom/notifications/__tests__/useNotifications.test.js create mode 100644 src/js/custom/notifications/api/notificationsApi.js create mode 100644 src/js/custom/notifications/components/NotificationBell.jsx create mode 100644 src/js/custom/notifications/components/NotificationDropdown.jsx create mode 100644 src/js/custom/notifications/components/NotificationModal.jsx create mode 100644 src/js/custom/notifications/hooks/useNotifications.js create mode 100644 src/js/custom/notifications/styles/_bell.scss diff --git a/grails-app/views/common/_menuicons.gsp b/grails-app/views/common/_menuicons.gsp index f04af0e7b09..2f41596e7ee 100644 --- a/grails-app/views/common/_menuicons.gsp +++ b/grails-app/views/common/_menuicons.gsp @@ -1,5 +1,6 @@ <%@page import="org.pih.warehouse.core.ActivityCode;"%>
                +
                diff --git a/grails-app/views/custom/notifications/_bell.gsp b/grails-app/views/custom/notifications/_bell.gsp new file mode 100644 index 00000000000..8dbb6e23e4a --- /dev/null +++ b/grails-app/views/custom/notifications/_bell.gsp @@ -0,0 +1,380 @@ +<%-- + Notification bell partial for GSP-rendered pages (React-shell pages mount + a separate React bell via src/js/custom/notifications/components/NotificationBell.jsx). + Both surfaces hit the same /api/custom/notifications endpoint so the + unread count stays consistent. + + Designed to be dropped inside a `.navbar-icons` container — it inherits + the `.menu-icon` sizing (40px/22px) from HeaderStyles.scss so the icon + matches the search / help / gear / profile icons that sit next to it. +--%> +
                +
                + + + ${message(code: 'notifications.bell.title', default: 'Notifications')} + +
                + + + +
                + + + + diff --git a/src/js/components/Layout/Header.jsx b/src/js/components/Layout/Header.jsx index 2e3a728bfd9..cd2880c5beb 100644 --- a/src/js/components/Layout/Header.jsx +++ b/src/js/components/Layout/Header.jsx @@ -1,5 +1,6 @@ import React from 'react'; +import NotificationBell from 'custom/notifications/components/NotificationBell'; import PropTypes from 'prop-types'; import { RiMenuLine } from 'react-icons/ri'; import { connect } from 'react-redux'; @@ -41,6 +42,7 @@ const Header = ({ diff --git a/src/js/custom/notifications/__tests__/NotificationBell.test.jsx b/src/js/custom/notifications/__tests__/NotificationBell.test.jsx new file mode 100644 index 00000000000..76038ac1857 --- /dev/null +++ b/src/js/custom/notifications/__tests__/NotificationBell.test.jsx @@ -0,0 +1,100 @@ +/* eslint-env jest */ +import React from 'react'; + +import { fireEvent, render, screen } from '@testing-library/react'; +import useNotifications from 'custom/notifications/hooks/useNotifications'; + +import '@testing-library/jest-dom'; + +// Mock the hook so the component renders without Redux/API setup +jest.mock('custom/notifications/hooks/useNotifications'); +// Stub the dropdown so Bell tests stay focused on bell behaviour +jest.mock('custom/notifications/components/NotificationDropdown', () => { + const MockDropdown = () =>
                ; + return MockDropdown; +}); + +// eslint-disable-next-line import/first +import NotificationBell from 'custom/notifications/components/NotificationBell'; + +const DEFAULT_HOOK = { + notifications: [], + unreadCount: 0, + loading: false, + error: null, + markRead: jest.fn(), + markAllRead: jest.fn(), +}; + +describe('NotificationBell', () => { + beforeEach(() => { + useNotifications.mockReturnValue(DEFAULT_HOOK); + }); + + describe('badge visibility', () => { + it('does not render the badge when unreadCount is 0', () => { + render(); + expect(screen.queryByText(/\d/)).not.toBeInTheDocument(); + }); + + it('renders the badge with the unread count', () => { + useNotifications.mockReturnValue({ ...DEFAULT_HOOK, unreadCount: 5 }); + render(); + expect(screen.getByText('5')).toBeInTheDocument(); + }); + + it('caps the badge at 99+ when unreadCount exceeds 99', () => { + useNotifications.mockReturnValue({ ...DEFAULT_HOOK, unreadCount: 120 }); + render(); + expect(screen.getByText('99+')).toBeInTheDocument(); + }); + }); + + describe('button accessibility', () => { + it('labels the button with unread count when there are unread notifications', () => { + useNotifications.mockReturnValue({ ...DEFAULT_HOOK, unreadCount: 3 }); + render(); + expect(screen.getByRole('button', { name: 'Notifications, 3 unread' })).toBeInTheDocument(); + }); + + it('labels the button without count when nothing is unread', () => { + render(); + expect(screen.getByRole('button', { name: 'Notifications' })).toBeInTheDocument(); + }); + }); + + describe('dropdown toggle', () => { + it('does not render the dropdown initially', () => { + render(); + expect(screen.queryByTestId('notification-dropdown')).not.toBeInTheDocument(); + }); + + it('shows the dropdown after the bell button is clicked', async () => { + render(); + fireEvent.click(screen.getByRole('button', { name: 'Notifications' })); + expect(screen.getByTestId('notification-dropdown')).toBeInTheDocument(); + }); + + it('closes the dropdown when the bell button is clicked a second time', async () => { + render(); + const button = screen.getByRole('button', { name: 'Notifications' }); + fireEvent.click(button); + fireEvent.click(button); + expect(screen.queryByTestId('notification-dropdown')).not.toBeInTheDocument(); + }); + + it('closes the dropdown on outside click', async () => { + render( +
                + + +
                , + ); + fireEvent.click(screen.getByRole('button', { name: 'Notifications' })); + expect(screen.getByTestId('notification-dropdown')).toBeInTheDocument(); + + fireEvent.mouseDown(screen.getByRole('button', { name: 'Outside' })); + expect(screen.queryByTestId('notification-dropdown')).not.toBeInTheDocument(); + }); + }); +}); diff --git a/src/js/custom/notifications/__tests__/NotificationDropdown.test.jsx b/src/js/custom/notifications/__tests__/NotificationDropdown.test.jsx new file mode 100644 index 00000000000..7061cc6adc3 --- /dev/null +++ b/src/js/custom/notifications/__tests__/NotificationDropdown.test.jsx @@ -0,0 +1,201 @@ +/* eslint-env jest */ +import React from 'react'; + +import { fireEvent, render, screen } from '@testing-library/react'; +import NotificationDropdown from 'custom/notifications/components/NotificationDropdown'; +import { MemoryRouter } from 'react-router-dom'; + +import '@testing-library/jest-dom'; + +jest.mock('date-fns', () => ({ + formatDistanceToNow: () => '2 hours ago', +})); + +jest.mock('dompurify', () => ({ sanitize: (html) => html })); + +const NOTIFICATION_UNREAD = { + id: 'n-1', + title: 'Low stock alert', + createdAt: '2026-05-22T08:00:00Z', + read: false, +}; +const NOTIFICATION_READ = { + id: 'n-2', + title: 'Shipment arrived', + createdAt: '2026-05-21T08:00:00Z', + read: true, +}; + +const renderDropdown = (overrides = {}, { initialEntries = ['/'] } = {}) => { + const onMarkRead = jest.fn(); + const onMarkAllRead = jest.fn(); + const onTabChange = jest.fn(); + const onLoadMore = jest.fn(); + const utils = render( + + + , + ); + return { + ...utils, onMarkRead, onMarkAllRead, onTabChange, onLoadMore, + }; +}; + +describe('NotificationDropdown', () => { + describe('empty state', () => { + it('shows the unread-empty message on the Unread tab', () => { + renderDropdown(); + expect(screen.getByText('No unread notifications')).toBeInTheDocument(); + }); + + it('shows the all-empty message on the All tab', () => { + renderDropdown({ activeTab: 'all' }); + expect(screen.getByText('No notifications yet')).toBeInTheDocument(); + }); + + it('does not show the mark-all button when unreadCount is 0', () => { + renderDropdown(); + expect(screen.queryByRole('button', { name: 'Mark all as read' })).not.toBeInTheDocument(); + }); + }); + + describe('notification list', () => { + it('renders each notification title and timestamp', () => { + renderDropdown({ + notifications: [NOTIFICATION_UNREAD, NOTIFICATION_READ], + unreadCount: 1, + }); + expect(screen.getByText('Low stock alert')).toBeInTheDocument(); + expect(screen.getByText('Shipment arrived')).toBeInTheDocument(); + expect(screen.getAllByText('2 hours ago')).toHaveLength(2); + }); + + it('renders unread dot only for unread notifications', () => { + renderDropdown({ + notifications: [NOTIFICATION_UNREAD, NOTIFICATION_READ], + unreadCount: 1, + }); + expect(screen.getAllByLabelText('Unread')).toHaveLength(1); + }); + }); + + describe('mark all as read', () => { + it('shows mark-all button when unreadCount > 0', () => { + renderDropdown({ notifications: [NOTIFICATION_UNREAD], unreadCount: 1 }); + expect(screen.getByRole('button', { name: 'Mark all as read' })).toBeInTheDocument(); + }); + + it('calls onMarkAllRead when mark-all button is clicked', () => { + const { onMarkAllRead } = renderDropdown({ + notifications: [NOTIFICATION_UNREAD], + unreadCount: 1, + }); + fireEvent.click(screen.getByRole('button', { name: 'Mark all as read' })); + expect(onMarkAllRead).toHaveBeenCalledTimes(1); + }); + }); + + describe('mark single notification as read', () => { + it('calls onMarkRead with the notification id when an unread row is clicked', () => { + const { onMarkRead } = renderDropdown({ + notifications: [NOTIFICATION_UNREAD], + unreadCount: 1, + }); + fireEvent.click(screen.getByText('Low stock alert')); + expect(onMarkRead).toHaveBeenCalledWith('n-1'); + }); + + it('does not call onMarkRead when an already-read row is clicked', () => { + const { onMarkRead } = renderDropdown({ + notifications: [NOTIFICATION_READ], + unreadCount: 0, + }); + fireEvent.click(screen.getByText('Shipment arrived')); + expect(onMarkRead).not.toHaveBeenCalled(); + }); + }); + + describe('modal opens on row click', () => { + it('opens the modal with the notification title when a row is clicked', () => { + renderDropdown({ + notifications: [NOTIFICATION_UNREAD], + unreadCount: 1, + }); + fireEvent.click(screen.getByText('Low stock alert')); + expect(screen.getByRole('dialog', { name: 'Low stock alert' })).toBeInTheDocument(); + }); + + it('opens the modal for a read notification without calling onMarkRead', () => { + const { onMarkRead } = renderDropdown({ + notifications: [NOTIFICATION_READ], + unreadCount: 0, + }); + fireEvent.click(screen.getByText('Shipment arrived')); + expect(screen.getByRole('dialog', { name: 'Shipment arrived' })).toBeInTheDocument(); + expect(onMarkRead).not.toHaveBeenCalled(); + }); + + it('closes the modal when the close button is clicked', () => { + renderDropdown({ + notifications: [NOTIFICATION_UNREAD], + unreadCount: 1, + }); + fireEvent.click(screen.getByText('Low stock alert')); + expect(screen.getByRole('dialog', { name: 'Low stock alert' })).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + expect(screen.queryByRole('dialog', { name: 'Low stock alert' })).not.toBeInTheDocument(); + }); + }); + + describe('linkUrl — modal shows Open link', () => { + it('shows "Open link" in the modal when notification has a same-origin linkUrl', () => { + const notification = { ...NOTIFICATION_UNREAD, linkUrl: '/openboxes/shipment/123' }; + renderDropdown({ notifications: [notification], unreadCount: 1 }); + fireEvent.click(screen.getByText('Low stock alert')); + expect(screen.getByRole('button', { name: 'Open link' })).toBeInTheDocument(); + }); + + it('does not show "Open link" for external linkUrl', () => { + const notification = { ...NOTIFICATION_UNREAD, linkUrl: 'https://evil.example/x' }; + renderDropdown({ notifications: [notification], unreadCount: 1 }); + fireEvent.click(screen.getByText('Low stock alert')); + expect(screen.queryByRole('button', { name: 'Open link' })).not.toBeInTheDocument(); + }); + }); + + describe('tabs', () => { + it('renders Unread and All tab buttons', () => { + renderDropdown({ unreadCount: 3 }); + expect(screen.getByRole('tab', { name: /Unread/ })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'All' })).toBeInTheDocument(); + }); + + it('marks the active tab via aria-selected', () => { + renderDropdown({ activeTab: 'all', unreadCount: 0 }); + expect(screen.getByRole('tab', { name: 'All' })).toHaveAttribute('aria-selected', 'true'); + expect(screen.getByRole('tab', { name: 'Unread' })).toHaveAttribute('aria-selected', 'false'); + }); + + it('calls onTabChange when a tab is clicked', () => { + const { onTabChange } = renderDropdown({ unreadCount: 1 }); + fireEvent.click(screen.getByRole('tab', { name: 'All' })); + expect(onTabChange).toHaveBeenCalledWith('all'); + }); + + it('shows the unread count badge on the Unread tab', () => { + renderDropdown({ unreadCount: 7 }); + expect(screen.getByRole('tab', { name: /Unread.*7/ })).toBeInTheDocument(); + }); + }); +}); diff --git a/src/js/custom/notifications/__tests__/NotificationModal.test.jsx b/src/js/custom/notifications/__tests__/NotificationModal.test.jsx new file mode 100644 index 00000000000..e2c9c50065d --- /dev/null +++ b/src/js/custom/notifications/__tests__/NotificationModal.test.jsx @@ -0,0 +1,127 @@ +/* eslint-env jest */ +import React from 'react'; + +import { fireEvent, render, screen } from '@testing-library/react'; +import NotificationModal from 'custom/notifications/components/NotificationModal'; + +import '@testing-library/jest-dom'; + +const NOTIFICATION_BASE = { + id: 'n-1', + title: 'Low stock alert', + body: '

                Stock is below threshold.

                ', + createdAt: '2026-05-22T08:00:00Z', + read: false, + linkUrl: null, +}; + +const renderModal = (props = {}) => { + const onClose = jest.fn(); + const onOpenLink = jest.fn(); + const utils = render( + , + ); + return { ...utils, onClose, onOpenLink }; +}; + +describe('NotificationModal', () => { + describe('null notification', () => { + it('renders nothing when notification is null', () => { + const { container } = render( + , + ); + expect(container.firstChild).toBeNull(); + }); + }); + + describe('content rendering', () => { + it('renders the notification title', () => { + renderModal(); + expect(screen.getByText('Low stock alert')).toBeInTheDocument(); + }); + + it('renders the HTML body inside a sandboxed iframe', () => { + renderModal(); + const frame = document.querySelector('.notification-modal__frame'); + expect(frame).toHaveAttribute('sandbox', ''); + expect(frame).toHaveAttribute('srcdoc', '

                Stock is below threshold.

                '); + }); + + it('renders createdAt value', () => { + renderModal(); + expect(screen.getByText('2026-05-22T08:00:00Z')).toBeInTheDocument(); + }); + + it('shows "No additional details" when body is null', () => { + renderModal({ notification: { ...NOTIFICATION_BASE, body: null } }); + expect(screen.getByText('No additional details')).toBeInTheDocument(); + }); + + it('shows "No additional details" when body is empty string', () => { + renderModal({ notification: { ...NOTIFICATION_BASE, body: '' } }); + expect(screen.getByText('No additional details')).toBeInTheDocument(); + }); + }); + + describe('close behavior', () => { + it('calls onClose when the X close button is clicked', () => { + const { onClose } = renderModal(); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('calls onClose when the backdrop is clicked', () => { + const { onClose } = renderModal(); + const backdrop = document.querySelector('.notification-modal__backdrop'); + fireEvent.click(backdrop); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('calls onClose when Escape key is pressed', () => { + const { onClose } = renderModal(); + fireEvent.keyDown(document, { key: 'Escape' }); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('does not call onClose when a non-Escape key is pressed', () => { + const { onClose } = renderModal(); + fireEvent.keyDown(document, { key: 'Enter' }); + expect(onClose).not.toHaveBeenCalled(); + }); + }); + + describe('Open link button', () => { + it('shows "Open link" button for same-origin linkUrl', () => { + renderModal({ notification: { ...NOTIFICATION_BASE, linkUrl: '/openboxes/shipment/123' } }); + expect(screen.getByRole('button', { name: 'Open link' })).toBeInTheDocument(); + }); + + it('calls onOpenLink with linkUrl when "Open link" is clicked', () => { + const { onOpenLink } = renderModal({ + notification: { ...NOTIFICATION_BASE, linkUrl: '/openboxes/shipment/123' }, + }); + fireEvent.click(screen.getByRole('button', { name: 'Open link' })); + expect(onOpenLink).toHaveBeenCalledWith('/openboxes/shipment/123'); + }); + + it('does not show "Open link" when linkUrl is null', () => { + renderModal({ notification: { ...NOTIFICATION_BASE, linkUrl: null } }); + expect(screen.queryByRole('button', { name: 'Open link' })).not.toBeInTheDocument(); + }); + + it('does not show "Open link" when linkUrl is external', () => { + renderModal({ notification: { ...NOTIFICATION_BASE, linkUrl: 'https://evil.example/x' } }); + expect(screen.queryByRole('button', { name: 'Open link' })).not.toBeInTheDocument(); + }); + + it('does not show "Open link" when linkUrl starts with //', () => { + renderModal({ notification: { ...NOTIFICATION_BASE, linkUrl: '//evil.example/x' } }); + expect(screen.queryByRole('button', { name: 'Open link' })).not.toBeInTheDocument(); + }); + }); +}); diff --git a/src/js/custom/notifications/__tests__/notificationsApi.test.js b/src/js/custom/notifications/__tests__/notificationsApi.test.js new file mode 100644 index 00000000000..ba463b3f564 --- /dev/null +++ b/src/js/custom/notifications/__tests__/notificationsApi.test.js @@ -0,0 +1,55 @@ +/* eslint-env jest */ +import { + getNotifications, + markAllRead, + markRead, +} from 'custom/notifications/api/notificationsApi'; + +import apiClient from 'utils/apiClient'; + +jest.mock('utils/apiClient', () => ({ + get: jest.fn(), + put: jest.fn(), +})); + +const BASE_URL = '/openboxes/api/custom/notifications'; + +describe('notificationsApi', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('getNotifications', () => { + it('calls GET with default params', () => { + apiClient.get.mockResolvedValueOnce({ data: { data: [] } }); + getNotifications(); + expect(apiClient.get).toHaveBeenCalledWith(BASE_URL, { + params: { unreadOnly: false, limit: 20 }, + }); + }); + + it('passes custom params when provided', () => { + apiClient.get.mockResolvedValueOnce({ data: { data: [] } }); + getNotifications({ unreadOnly: true, limit: 5 }); + expect(apiClient.get).toHaveBeenCalledWith(BASE_URL, { + params: { unreadOnly: true, limit: 5 }, + }); + }); + }); + + describe('markRead', () => { + it('calls PUT with the notification id in the path', () => { + apiClient.put.mockResolvedValueOnce({}); + markRead('notif-42'); + expect(apiClient.put).toHaveBeenCalledWith(`${BASE_URL}/notif-42/read`); + }); + }); + + describe('markAllRead', () => { + it('calls PUT on the read-all endpoint', () => { + apiClient.put.mockResolvedValueOnce({}); + markAllRead(); + expect(apiClient.put).toHaveBeenCalledWith(`${BASE_URL}/read-all`); + }); + }); +}); diff --git a/src/js/custom/notifications/__tests__/useNotifications.test.js b/src/js/custom/notifications/__tests__/useNotifications.test.js new file mode 100644 index 00000000000..b6209e1cb45 --- /dev/null +++ b/src/js/custom/notifications/__tests__/useNotifications.test.js @@ -0,0 +1,254 @@ +/* eslint-env jest */ +import { act, renderHook } from '@testing-library/react-hooks'; +import { + getNotifications, + getUnreadCount, + markAllRead as apiMarkAllRead, + markRead as apiMarkRead, +} from 'custom/notifications/api/notificationsApi'; +import useNotifications from 'custom/notifications/hooks/useNotifications'; + +jest.mock('@sentry/react', () => ({ captureException: jest.fn() })); +jest.mock('custom/notifications/api/notificationsApi'); +jest.useFakeTimers(); + +const NOTIFICATION_A = { + id: 'n-1', title: 'First', createdAt: '2026-05-01T10:00:00Z', read: false, +}; +const NOTIFICATION_B = { + id: 'n-2', title: 'Second', createdAt: '2026-05-02T10:00:00Z', read: true, +}; + +const PAGE = Array.from({ length: 20 }, (_, i) => ({ + id: `p-${i}`, title: `Item ${i}`, createdAt: '2026-05-03T10:00:00Z', read: false, +})); +const PAGE_2 = Array.from({ length: 5 }, (_, i) => ({ + id: `q-${i}`, title: `More ${i}`, createdAt: '2026-05-04T10:00:00Z', read: false, +})); + +const mockCount = (count) => { + getUnreadCount.mockResolvedValueOnce({ data: { unreadCount: count } }); +}; + +const mockList = (items = [], unreadCount = items.filter((n) => !n.read).length) => { + getNotifications.mockResolvedValueOnce({ data: { data: items, unreadCount } }); +}; + +describe('useNotifications', () => { + beforeEach(() => { + jest.clearAllMocks(); + getUnreadCount.mockResolvedValue({ data: { unreadCount: 0 } }); + }); + + describe('badge count poll', () => { + it('sets unreadCount from getUnreadCount on mount, list untouched', async () => { + mockCount(3); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications()); + await waitForNextUpdate(); + + expect(getUnreadCount).toHaveBeenCalledTimes(1); + expect(result.current.unreadCount).toBe(3); + expect(result.current.notifications).toEqual([]); + expect(getNotifications).not.toHaveBeenCalled(); + }); + + it('re-fetches the count after 30 seconds', async () => { + mockCount(1); + mockCount(4); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications()); + await waitForNextUpdate(); + + expect(result.current.unreadCount).toBe(1); + + act(() => { + jest.advanceTimersByTime(30000); + }); + await waitForNextUpdate(); + + expect(getUnreadCount).toHaveBeenCalledTimes(2); + expect(result.current.unreadCount).toBe(4); + }); + + it('clears the interval on unmount', async () => { + mockCount(0); + + const { unmount, waitForNextUpdate } = renderHook(() => useNotifications()); + await waitForNextUpdate(); + + unmount(); + act(() => { + jest.advanceTimersByTime(30000); + }); + + expect(getUnreadCount).toHaveBeenCalledTimes(1); + }); + }); + + describe('list fetch', () => { + it('does not fetch the list while open is false', async () => { + mockCount(2); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: false })); + await waitForNextUpdate(); + + expect(getNotifications).not.toHaveBeenCalled(); + expect(result.current.notifications).toEqual([]); + }); + + it('fetches page 1 when open becomes true and sets hasMore', async () => { + mockCount(2); + mockList([NOTIFICATION_A, NOTIFICATION_B], 1); + + const { result, waitForNextUpdate, rerender } = renderHook( + ({ open }) => useNotifications({ open, unreadOnly: false }), + { initialProps: { open: false } }, + ); + await waitForNextUpdate(); + + rerender({ open: true }); + await waitForNextUpdate(); + + expect(getNotifications).toHaveBeenCalledWith({ unreadOnly: false, limit: 20, offset: 0 }); + expect(result.current.notifications).toEqual([NOTIFICATION_A, NOTIFICATION_B]); + expect(result.current.unreadCount).toBe(1); + expect(result.current.hasMore).toBe(false); + }); + + it('sets hasMore true when a full page is returned', async () => { + mockCount(20); + mockList(PAGE, 20); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: true })); + await waitForNextUpdate(); + + expect(result.current.notifications).toEqual(PAGE); + expect(result.current.hasMore).toBe(true); + }); + + it('resets the list when open returns to false', async () => { + mockCount(1); + mockList([NOTIFICATION_A], 1); + + const { result, waitForNextUpdate, rerender } = renderHook( + ({ open }) => useNotifications({ open }), + { initialProps: { open: true } }, + ); + await waitForNextUpdate(); + expect(result.current.notifications).toEqual([NOTIFICATION_A]); + + rerender({ open: false }); + + expect(result.current.notifications).toEqual([]); + expect(result.current.hasMore).toBe(false); + }); + + it('sets error when the list fetch fails', async () => { + mockCount(0); + const fetchError = new Error('network'); + getNotifications.mockRejectedValueOnce(fetchError); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: true })); + await waitForNextUpdate(); + + expect(result.current.error).toBe(fetchError); + expect(result.current.notifications).toEqual([]); + }); + }); + + describe('loadMore', () => { + it('appends the next page and updates hasMore', async () => { + mockCount(25); + mockList(PAGE, 25); + mockList(PAGE_2, 25); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: true })); + await waitForNextUpdate(); + + expect(result.current.hasMore).toBe(true); + + await act(async () => { + await result.current.loadMore(); + }); + + expect(getNotifications).toHaveBeenLastCalledWith({ + unreadOnly: true, limit: 20, offset: 20, + }); + expect(result.current.notifications).toEqual([...PAGE, ...PAGE_2]); + expect(result.current.hasMore).toBe(false); + }); + }); + + describe('markRead', () => { + it('optimistically marks read and decrements unreadCount without re-fetching', async () => { + mockCount(1); + mockList([NOTIFICATION_A, NOTIFICATION_B], 1); + apiMarkRead.mockResolvedValueOnce({}); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: true })); + await waitForNextUpdate(); + + expect(result.current.unreadCount).toBe(1); + + await act(async () => { + await result.current.markRead('n-1'); + }); + + expect(result.current.unreadCount).toBe(0); + expect(result.current.notifications.find((n) => n.id === 'n-1').read).toBe(true); + // No second list fetch — only the initial page-1 call. + expect(getNotifications).toHaveBeenCalledTimes(1); + }); + + it('reverts the optimistic change on error', async () => { + mockCount(1); + mockList([NOTIFICATION_A, NOTIFICATION_B], 1); + apiMarkRead.mockRejectedValueOnce(new Error('boom')); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: true })); + await waitForNextUpdate(); + + await act(async () => { + await result.current.markRead('n-1'); + }); + + expect(result.current.unreadCount).toBe(1); + expect(result.current.notifications.find((n) => n.id === 'n-1').read).toBe(false); + }); + }); + + describe('markAllRead', () => { + it('optimistically marks all read and sets unreadCount to 0', async () => { + mockCount(1); + mockList([NOTIFICATION_A, NOTIFICATION_B], 1); + apiMarkAllRead.mockResolvedValueOnce({}); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: true })); + await waitForNextUpdate(); + + await act(async () => { + await result.current.markAllRead(); + }); + + expect(result.current.unreadCount).toBe(0); + expect(result.current.notifications.every((n) => n.read)).toBe(true); + }); + + it('reverts on error', async () => { + mockCount(1); + mockList([NOTIFICATION_A, NOTIFICATION_B], 1); + apiMarkAllRead.mockRejectedValueOnce(new Error('boom')); + + const { result, waitForNextUpdate } = renderHook(() => useNotifications({ open: true })); + await waitForNextUpdate(); + + await act(async () => { + await result.current.markAllRead(); + }); + + expect(result.current.unreadCount).toBe(1); + expect(result.current.notifications).toEqual([NOTIFICATION_A, NOTIFICATION_B]); + }); + }); +}); diff --git a/src/js/custom/notifications/api/notificationsApi.js b/src/js/custom/notifications/api/notificationsApi.js new file mode 100644 index 00000000000..2b198bd4714 --- /dev/null +++ b/src/js/custom/notifications/api/notificationsApi.js @@ -0,0 +1,15 @@ +import apiClient from 'utils/apiClient'; + +const BASE_URL = '/openboxes/api/custom/notifications'; + +export const getUnreadCount = () => + apiClient.get(`${BASE_URL}/unread-count`); + +export const getNotifications = ({ unreadOnly = false, limit = 20, offset = 0 } = {}) => + apiClient.get(BASE_URL, { params: { unreadOnly, limit, offset } }); + +export const markRead = (id) => + apiClient.put(`${BASE_URL}/${id}/read`); + +export const markAllRead = () => + apiClient.put(`${BASE_URL}/read-all`); diff --git a/src/js/custom/notifications/components/NotificationBell.jsx b/src/js/custom/notifications/components/NotificationBell.jsx new file mode 100644 index 00000000000..6d3811f7052 --- /dev/null +++ b/src/js/custom/notifications/components/NotificationBell.jsx @@ -0,0 +1,80 @@ +import React, { useEffect, useRef, useState } from 'react'; + +import NotificationDropdown from 'custom/notifications/components/NotificationDropdown'; +import useNotifications from 'custom/notifications/hooks/useNotifications'; +import { RiNotification3Line } from 'react-icons/ri'; + +import 'custom/notifications/styles/_bell.scss'; + +const NotificationBell = () => { + const [isOpen, setIsOpen] = useState(false); + const [activeTab, setActiveTab] = useState('unread'); + const containerRef = useRef(null); + const { + notifications, + unreadCount, + markRead, + markAllRead, + loadMore, + hasMore, + loadingMore, + } = useNotifications({ unreadOnly: activeTab === 'unread', open: isOpen, limit: 20 }); + + useEffect(() => { + if (!isOpen) return undefined; + + const handleOutsideClick = (e) => { + if (containerRef.current && !containerRef.current.contains(e.target)) { + setIsOpen(false); + } + }; + + document.addEventListener('mousedown', handleOutsideClick); + return () => { + document.removeEventListener('mousedown', handleOutsideClick); + }; + }, [isOpen]); + + const handleMarkRead = (id) => { + markRead(id); + }; + + const handleMarkAllRead = () => { + markAllRead(); + }; + + return ( +
              • + + {isOpen && ( + + )} +
                • + ); +}; + +export default NotificationBell; diff --git a/src/js/custom/notifications/components/NotificationDropdown.jsx b/src/js/custom/notifications/components/NotificationDropdown.jsx new file mode 100644 index 00000000000..87acab18e08 --- /dev/null +++ b/src/js/custom/notifications/components/NotificationDropdown.jsx @@ -0,0 +1,158 @@ +import React, { useState } from 'react'; + +import NotificationModal from 'custom/notifications/components/NotificationModal'; +import { formatDistanceToNow } from 'date-fns'; +import PropTypes from 'prop-types'; +import { useHistory } from 'react-router-dom'; + +import Translate from 'utils/Translate'; + +const isSameOriginPath = (url) => typeof url === 'string' && url.startsWith('/') && !url.startsWith('//'); + +const formatRelative = (value) => { + const date = new Date(value); + if (Number.isNaN(date.getTime())) { + return ''; + } + return formatDistanceToNow(date, { addSuffix: true }); +}; + +const NotificationDropdown = ({ + notifications, + unreadCount, + activeTab, + onTabChange, + onMarkRead, + onMarkAllRead, + onLoadMore, + hasMore, + loadingMore, +}) => { + const history = useHistory(); + const [selectedNotification, setSelectedNotification] = useState(null); + + const handleActivate = (notification) => { + if (!notification.read) { + onMarkRead(notification.id); + } + setSelectedNotification(notification); + }; + + const handleCloseModal = () => { + setSelectedNotification(null); + }; + + const handleOpenLink = (linkUrl) => { + if (isSameOriginPath(linkUrl)) { + history.push(linkUrl); + } + }; + + return ( +
                +
                +
                + {unreadCount > 0 && ( + + )} +
                +
                + + +
                + {notifications.length === 0 ? ( +

                + {activeTab === 'unread' ? ( + + ) : ( + + )} +

                + ) : ( +
                  + {notifications.map((notification) => ( +
                • + +
                  • + ))} +
                + )} + {hasMore && ( + + )} + +
                + ); +}; + +NotificationDropdown.propTypes = { + notifications: PropTypes.arrayOf( + PropTypes.shape({ + id: PropTypes.string.isRequired, + title: PropTypes.string.isRequired, + createdAt: PropTypes.string.isRequired, + read: PropTypes.bool.isRequired, + linkUrl: PropTypes.string, + }), + ).isRequired, + unreadCount: PropTypes.number.isRequired, + activeTab: PropTypes.oneOf(['unread', 'all']).isRequired, + onTabChange: PropTypes.func.isRequired, + onMarkRead: PropTypes.func.isRequired, + onMarkAllRead: PropTypes.func.isRequired, + onLoadMore: PropTypes.func.isRequired, + hasMore: PropTypes.bool.isRequired, + loadingMore: PropTypes.bool.isRequired, +}; + +export default NotificationDropdown; diff --git a/src/js/custom/notifications/components/NotificationModal.jsx b/src/js/custom/notifications/components/NotificationModal.jsx new file mode 100644 index 00000000000..b1c78400180 --- /dev/null +++ b/src/js/custom/notifications/components/NotificationModal.jsx @@ -0,0 +1,125 @@ +import React, { useEffect } from 'react'; + +import PropTypes from 'prop-types'; + +import Translate from 'utils/Translate'; + +const isSameOriginPath = (url) => typeof url === 'string' && url.startsWith('/') && !url.startsWith('//'); + +const NotificationModal = ({ + notification, + onClose, + onOpenLink, +}) => { + useEffect(() => { + if (!notification) return undefined; + + const handleKeyDown = (e) => { + if (e.key === 'Escape') { + onClose(); + } + }; + + document.addEventListener('keydown', handleKeyDown); + return () => { + document.removeEventListener('keydown', handleKeyDown); + }; + }, [notification, onClose]); + + if (!notification) { + return null; + } + + const hasBody = Boolean(notification.body && notification.body.trim()); + + const handleBackdropClick = (e) => { + if (e.target === e.currentTarget) { + onClose(); + } + }; + + const handleOpenLink = () => { + onOpenLink(notification.linkUrl); + onClose(); + }; + + return ( +
                +
                +
                +
                +
                + {notification.title} +
                + +
                +
                + {notification.createdAt} +
                +
                + {hasBody ? ( + // Reason: body is an email body (untrusted HTML). A sandboxed iframe with no + // allow-scripts token neutralizes scripts/handlers at the browser level and + // isolates the email's own CSS from the app — no sanitizer dependency needed. +