[TTAHUB-5539] Specification for actionable notification backend#3661
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a backend design specification for actionable notifications, outlining proposed data modeling, services, scopes, handlers, scheduled cleanup, email-notification simplification, and related frontend follow-up work.
Changes:
- Adds a new
specs/actionable-notifications/index.mddesign document. - Defines proposed notification schema, configuration, service APIs, scopes, and handler responsibilities.
- Lists planned backend tickets plus frontend follow-up areas.
Impact Assessment:
- Benefits: Medium — provides a shared implementation plan for a cross-cutting notification system.
- Risks: Medium — unresolved schema and cleanup-query issues could lead to implementation mistakes if copied directly.
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>
AdamAdHocTeam
left a comment
There was a problem hiding this comment.
I think this looks good. Should we mention something about cleaning up no longer valid notifications. For example two approvers added to report and submitted. Then the report is moved back to needs action?
Overall good start (minor comments)
|
|
|
|
@AdamAdHocTeam good call, I updated to add a new service type |
kryswisnaskas
left a comment
There was a problem hiding this comment.
Adding four inline review findings from Codex.
* 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 * Remove digest from notifications enum * Update model with other needed field --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* 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 * Update model with other needed field --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…le-notification-spec
* 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>
…able-notification-spec
📊 Review Metrics
Review Timeline
|
Description of change
Design backend for actionable notifications. Frontend tickets are briefly listed but work will be broken down separately.
How to test
Review implementation proposal. Identify additional work that may need to be completed. Ask questions where specification is unclear.
Issue(s)
Checklists
Every PR
Before merge to main
Production Deploy
ready_for_reviewtransition triggers the Slack/Jira automation)elainaparrishis the authorized approver under normal circumstances)After merge/deploy