Skip to content

[TTAHUB-5384] Add notification services#3673

Merged
thewatermethod merged 33 commits into
mb/TTAHUB-5539/actionable-notification-specfrom
mb/TTAHUB-5384/add-notification-services
Jun 8, 2026
Merged

[TTAHUB-5384] Add notification services#3673
thewatermethod merged 33 commits into
mb/TTAHUB-5539/actionable-notification-specfrom
mb/TTAHUB-5384/add-notification-services

Conversation

@thewatermethod

@thewatermethod thewatermethod commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Description of change

Add services and tests needed for actionable notifications
Handlers will come in a separate PR and will handle permissions, certain validations (as is our pattern elsewhere in the codebase)

NOTE: updates were made to this code in #3674 to accommodate proper global notification ownership, so your feedback may already have been addressed.

How to test

Validate the code, the tests, and consider the UI requirements

Issue(s)

Checklists

Every PR

  • Meets issue criteria
  • JIRA ticket status updated
  • Code is meaningfully tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • API Documentation updated
  • Boundary diagram updated
  • Logical Data Model updated
  • Architectural Decision Records written for major infrastructure decisions
  • UI review complete
  • QA review complete

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

thewatermethod and others added 17 commits June 1, 2026 12:40
- 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>
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

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

@thewatermethod thewatermethod changed the base branch from main to mb/TTAHUB-5385/add-notification-scopes June 3, 2026 14:15
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review count: 2 human approvals — kryswisnaskas, AdamAdHocTeam.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a backend notification service layer (plus typings and tests) to support actionable notifications, with handlers/permission checks planned for a follow-up PR.

Changes:

  • Added NOTIFICATION_CONFIGURATION in src/constants.js to centralize notification text/link/displayId construction by type.
  • Added src/services/notifications.ts with CRUD-style helpers (create user/global notifications, update timestamps, delete by scopes, list with pagination/sorting).
  • Added service typings and Jest tests validating the service behavior.

Impact assessment:

  • Benefits: Medium — establishes a clear, test-covered foundation for notification creation and retrieval.
  • Risks: High — current TypeScript typings in src/services/types/notifications.ts are inconsistent with the Sequelize model and include a typeof + import type pattern that is likely to break yarn build (tsc). Additionally, getNotifications accepts arbitrary sort inputs without normalization/whitelisting, which can lead to brittle runtime behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/services/types/notifications.ts Introduces TS types for notification service inputs/outputs.
src/services/notifications.ts Adds notification creation, update, delete, and retrieval services.
src/services/notifications.test.js Adds Jest tests covering the new notification services.
src/constants.js Adds notification configuration mapping for text/link/label/displayId builders.

Comment thread src/services/types/notifications.ts Outdated
Comment thread src/services/types/notifications.ts
Comment thread src/services/notifications.ts
thewatermethod and others added 3 commits June 3, 2026 11:15
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@thewatermethod thewatermethod marked this pull request as ready for review June 4, 2026 13:04
Comment thread src/services/notifications.ts Outdated
@kryswisnaskas

Copy link
Copy Markdown
Collaborator

Codex said: src/services/notifications.ts: High: deleteNotification(scopes) is broader than the ticket contract; with an empty scope array it can degrade into a full-table delete, and the PR does not expose the spec's targeted delete helpers.L137

…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>
@thewatermethod

Copy link
Copy Markdown
Collaborator Author

Codex said: src/services/notifications.ts: High: deleteNotification(scopes) is broader than the ticket contract; with an empty scope array it can degrade into a full-table delete, and the PR does not expose the spec's targeted delete helpers.L137

d9c6a39

@kryswisnaskas kryswisnaskas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 👍

Base automatically changed from mb/TTAHUB-5385/add-notification-scopes to mb/TTAHUB-5539/actionable-notification-spec June 8, 2026 13:05
thewatermethod and others added 9 commits June 8, 2026 09:05
* 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>
@thewatermethod thewatermethod merged commit 064fa63 into mb/TTAHUB-5539/actionable-notification-spec Jun 8, 2026
14 checks passed
@thewatermethod thewatermethod deleted the mb/TTAHUB-5384/add-notification-services branch June 8, 2026 15:57
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

📊 Review Metrics

Auto-generated by the PR Review Metrics workflow. Observe-only — no action required.

Metric Value
PR Status ✅ Merged
Reviewer Count 2
Reviewers @kryswisnaskas, @AdamAdHocTeam
First-Review Turnaround 7.4 hr
Total Review Events 3

Review Timeline

State Reviewer Submitted At
💬 COMMENTED @kryswisnaskas 2026-06-04 20:29 UTC
✅ APPROVED @kryswisnaskas 2026-06-05 15:01 UTC
✅ APPROVED @AdamAdHocTeam 2026-06-05 15:15 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants