[TTAHUB-5387] Add notification handlers by thewatermethod · Pull Request #3674 · HHS/Head-Start-TTADP

thewatermethod · 2026-06-03T15:21:08Z

Description of change

Add handlers for actionable notifications. Note that only a few operations actually require API endpoints, most of the notification CRUD actions will be handled programmatically by triggers

How to test

Confirm that the spec is accurate to the UI expectations. Review the code and the tests.

Issue(s)

https://jira.acf.gov/browse/TTAHUB-5387

Checklists

Every PR

Before merge to main

OHS demo complete
Ready to create production PR

Production Deploy

PR created as Draft
Staging smoke test completed
PR transitioned to Open (this ready_for_review transition triggers the Slack/Jira automation)
Reviewer added after the PR is Open (elainaparrish is the authorized approver under normal circumstances)
- Sequence: Draft PR → Smoke test → Open PR (automation runs) → Add reviewer
- Confirm that the Slack notification was sent after the PR was opened
- Confirm that linked Jira ticket(s) transitioned as expected; if not, review the GitHub Actions workflow logs

After merge/deploy

Update JIRA ticket status

- Fix Math.max -> Math.min bug in getNotifications limit calculation - Add handler unit tests (src/routes/notifications/handlers.test.ts) - Add policy unit tests (src/policies/notifications.test.ts) - Fix and expand service tests for sort field/direction validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…5387/add-notification-handlers

github-actions · 2026-06-03T15:21:25Z

⚠️ Review count advisory: 0 of 2 required human approvals. 2 more needed.

github-actions · 2026-06-03T15:21:25Z

⚠️ Diff size advisory: This PR is 2950 lines (2949+, 1−), exceeding the 500-line guideline. Consider splitting into smaller changes.

Copilot

Pull request overview

This PR introduces initial backend support for “actionable notifications” by adding an API router + handlers, a notifications authorization policy, and hardening the notification listing query with allowlisted sorting/pagination.

Changes:

Added /api/.../notifications route module with handlers for listing, updating, and admin creation of global notifications.
Added Notifications policy (admin-or-owner) and checkNotificationIdParam middleware export + tests.
Updated getNotifications to allowlist sort fields/directions and added tests for sort fallbacks.

Impact Assessment

Benefits: Medium — establishes the backend surface area (handlers/policy/service hardening) needed for actionable notifications.
Risks: High — new routes/handlers have a couple of wiring/data-shape issues that will prevent successful runtime use unless addressed.

Key Issues Noted (severity)

(high) Notifications router is not mounted in the main API router, so endpoints will be unreachable until wired into src/routes/apiDirectory.js.
(high) createGlobalNotificationHandler treats triggeredAt as a Date and calls .toISOString(); real JSON request bodies will provide a string, causing a runtime exception.
(medium) getNotifications pagination parsing allows negative limit/offset values, which can cause Sequelize errors or inconsistent behavior.
(medium) Policy/tests model “global notification” as userId: undefined, but persisted global notifications use userId: null.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
src/services/types/notifications.ts	Tweaks notification type alias to a direct indexed lookup of NOTIFICATION_TYPES values.
src/services/notifications.ts	Adds allowlisted sort fields/directions and clamps/normalizes pagination inputs (partially).
src/services/notifications.test.js	Adds tests for allowed sort and sort fallback behavior.
src/routes/notifications/index.ts	Adds notifications router (POST admin global create, PUT update, GET list).
src/routes/notifications/handlers.ts	Adds handlers for list/update/admin global create.
src/routes/notifications/handlers.test.ts	Adds unit tests for the new handlers (mocked services/models).
src/policies/notifications.ts	Adds Notifications policy (admin-or-owner update authorization).
src/policies/notifications.test.ts	Adds unit tests for the Notifications policy.
src/middleware/checkIdParamMiddleware.js	Exposes `checkNotificationIdParam` helper.
src/middleware/checkIdParamMiddleware.test.js	Adds tests for `checkNotificationIdParam`.

…tates table The Notifications table previously stored viewedAt and archivedAt directly on the notification row. Global notifications (userId=null) could not track whether different users had independently viewed or archived them — a single row cannot hold state for multiple users. Changes: - Migration: creates NotificationUserStates (notificationId, userId, viewedAt, archivedAt, UNIQUE on notificationId+userId with FK cascades), backfills existing per-user state, and drops viewedAt/archivedAt from Notifications - New model: NotificationUserState with belongsTo Notification+User - Notification model: removed archivedAt/viewedAt/isInformational, added hasMany(NotificationUserState, { as: 'userStates' }) - Service: updateNotification → updateNotificationState(notificationId, userId, { viewedAt?, archivedAt? }) — upserts per-user state row; getNotifications(userId, scopes, options) — LEFT JOINs state, filters archived, returns NotificationWithState[] - Types: added NotificationUserStateModel and NotificationWithState - Policy: added isGlobalNotification(); canUpdateNotification() now allows admin, owner, or global notification (any user can set their own state) - Handlers: getNotificationsHandler passes userId as first arg; updateNotificationHandler calls updateNotificationState - Spec: updated to document new table, service signatures, cleanup logic - Tests: 121/121 passing across 10 suites Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…5387/add-notification-handlers

…B-5387/add-notification-handlers

AdamAdHocTeam

Overall looks good might need minor tweaks as we implement it on the FE but overall good.

AI says (but might just be a result of what we are starting out with implementing):

High — Undefined keys in NOTIFICATION_TYPE_MAP
notificationType.ts

The trainingReport and other buckets reference keys that do not exist in NOTIFICATION_TYPES (e.g., TRAINING_REPORT_POC_ADDED_DIGEST, COMMUNICATION_LOG_TTA_STAFF_ADDED_DIGEST, etc.). Since constants.js defines no digest types, these all evaluate to undefined at runtime. That means Op.in queries will contain undefined values, which can generate invalid SQL or silently corrupt filtering.

The coverage test only checks in one direction (every NOTIFICATION_TYPES value is in a bucket), so it won't catch these extra undefined entries.

Fix: Either add the missing digest enum keys to NOTIFICATION_TYPES / the migration's enum array, or remove the undefined references from the map until those types are defined.

kryswisnaskas · 2026-06-05T16:35:54Z

  }

-  return notification.update(fieldsToUpdate);
+  let state = await NotificationUserState.findOne({


Could we match the spec and make this a real upsert instead of findOne followed by create/update? My concern is that, even inside the request transaction, the current read then write flow can still race under concurrent requests and fail on the unique constraint rather than behaving like an atomic upsert.

@kryswisnaskas I can if you want, but I was told that Sequelize + Postgres runs the upsert (really an insert... on conflict do nothing) outside of the transaction wrapper. That's why we converted all our upserts to this pattern years ago

* feat: add Notifications schema (model, migrations, seed, tests) - Add NOTIFICATION_TYPES and actionable_notifications feature flag to constants - Create Notifications table migration with all spec columns including archivedAt/viewedAt (DATEONLY) - Add actionable_notifications to enum_Users_flags migration - Create Notification Sequelize model with User association - Add User.hasMany(Notification) association - Add seed data with user-specific and global notifications - Add model tests (8 passing) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Simplify model * Update to add "triggeredAt" to model * Add all enum types * Scopes, work in progress * Remove digest from notifications enum * Add notification scopes * Register scopes * Fix bugs, add integration test * Add services and tests * Update model with other needed field * Accomodate missing column in services * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Add notification configuration tests * fix: replace generic deleteNotification(scopes) with targeted delete helpers - deleteNotification(notificationId) now accepts a single ID, throws when falsy — eliminates the empty-scopes full-table-delete risk - adds deleteNotificationsByEntityAndType(entityId, notificationType) for event-driven stale-notification cleanup; throws when either arg is falsy - updates and expands tests to cover both new signatures and their guards - updates spec doc with typed signatures and safety-guard notes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix bad test * [TTAHUB-5387] Add notification handlers (#3674) * fix: cap getNotifications limit at 100 and add comprehensive tests - Fix Math.max -> Math.min bug in getNotifications limit calculation - Add handler unit tests (src/routes/notifications/handlers.test.ts) - Add policy unit tests (src/policies/notifications.test.ts) - Fix and expand service tests for sort field/direction validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add param middleware * Updates from code review * refactor: separate per-user notification state into NotificationUserStates table The Notifications table previously stored viewedAt and archivedAt directly on the notification row. Global notifications (userId=null) could not track whether different users had independently viewed or archived them — a single row cannot hold state for multiple users. Changes: - Migration: creates NotificationUserStates (notificationId, userId, viewedAt, archivedAt, UNIQUE on notificationId+userId with FK cascades), backfills existing per-user state, and drops viewedAt/archivedAt from Notifications - New model: NotificationUserState with belongsTo Notification+User - Notification model: removed archivedAt/viewedAt/isInformational, added hasMany(NotificationUserState, { as: 'userStates' }) - Service: updateNotification → updateNotificationState(notificationId, userId, { viewedAt?, archivedAt? }) — upserts per-user state row; getNotifications(userId, scopes, options) — LEFT JOINs state, filters archived, returns NotificationWithState[] - Types: added NotificationUserStateModel and NotificationWithState - Policy: added isGlobalNotification(); canUpdateNotification() now allows admin, owner, or global notification (any user can set their own state) - Handlers: getNotificationsHandler passes userId as first arg; updateNotificationHandler calls updateNotificationState - Spec: updated to document new table, service signatures, cleanup logic - Tests: 121/121 passing across 10 suites Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update to remove findOrCreate * Update seeder * Define relation in both directions * Update test * Map scopes to types directly to prevent drift * Updates from code review --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Regenerate Logical Data Model * Condense down notifications table * Remove extraneous migration * Fix LDM bugs * Remove extra columns from migration --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

thewatermethod and others added 3 commits June 3, 2026 11:07

Add param middleware

553a230

Merge branch mb/TTAHUB-5384/add-notification-services into mb/TTHAUB-…

49d5843

…5387/add-notification-handlers

thewatermethod changed the base branch from main to mb/TTAHUB-5384/add-notification-services June 3, 2026 15:21

thewatermethod mentioned this pull request Jun 3, 2026

[TTAHUB-5384] Add notification services #3673

Merged

17 tasks

thewatermethod requested a review from Copilot June 3, 2026 15:23

Copilot started reviewing on behalf of thewatermethod June 3, 2026 15:24 View session

Copilot AI reviewed Jun 3, 2026

View reviewed changes

Updates from code review

4cb11ef

thewatermethod marked this pull request as ready for review June 4, 2026 13:04

thewatermethod requested review from AdamAdHocTeam, Andrew565, hardwarehuman, kryswisnaskas and tommaroh June 4, 2026 13:04

thewatermethod and others added 7 commits June 4, 2026 13:38

Update to remove findOrCreate

6b8f070

Update seeder

ad2aa1c

Define relation in both directions

86342e5

Merge branch mb/TTAHUB-5384/add-notification-services into mb/TTHAUB-…

e535847

…5387/add-notification-handlers

Update test

2394335

Merge branch 'mb/TTAHUB-5384/add-notification-services' into mb/TTHAU…

608c27c

…B-5387/add-notification-handlers

AdamAdHocTeam reviewed Jun 5, 2026

View reviewed changes

Comment thread src/routes/notifications/handlers.ts

AdamAdHocTeam approved these changes Jun 5, 2026

View reviewed changes

Map scopes to types directly to prevent drift

6ef87a7

kryswisnaskas reviewed Jun 5, 2026

View reviewed changes

Comment thread src/services/notifications.ts Outdated

kryswisnaskas reviewed Jun 5, 2026

View reviewed changes

Comment thread src/routes/notifications/handlers.ts

kryswisnaskas reviewed Jun 5, 2026

View reviewed changes

Comment thread src/routes/notifications/handlers.ts Outdated

kryswisnaskas reviewed Jun 5, 2026

View reviewed changes

Updates from code review

6defd31

thewatermethod merged commit 64bf0c5 into mb/TTAHUB-5384/add-notification-services Jun 8, 2026
10 checks passed

thewatermethod deleted the mb/TTHAUB-5387/add-notification-handlers branch June 8, 2026 13:05

Uh oh!

Conversation

thewatermethod commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

How to test

Issue(s)

Checklists

Every PR

Before merge to main

Production Deploy

After merge/deploy

Uh oh!

github-actions Bot commented Jun 3, 2026

Uh oh!

github-actions Bot commented Jun 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Impact Assessment

Key Issues Noted (severity)

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AdamAdHocTeam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kryswisnaskas Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

thewatermethod Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thewatermethod commented Jun 3, 2026 •

edited

Loading

thewatermethod Jun 5, 2026 •

edited

Loading