Refactored notifications service into a domain-driven module#27868
Draft
rob-ghost wants to merge 8 commits into
Draft
Refactored notifications service into a domain-driven module#27868rob-ghost wants to merge 8 commits into
rob-ghost wants to merge 8 commits into
Conversation
Replaced the separate e2e-api, unit, and update-check notification test files with a single TypeScript integration suite covering the full notification domain (admin API create/browse/destroy plus the update-check producer). This removes duplicate setup and snapshot fixtures, exercises the real bookshelf/settings layer rather than stubs, and gives one canonical place to extend coverage for upcoming GHSA notification work.
Introduced a runtime-validated Notification type as the foundation for the refactored notifications domain. The schema models the fields the current storage already carries (id, message, type, custom, dismissible, top, createdAtVersion, addedAt, seenBy) and explicitly omits legacy fields (location, status) that the domain doesn't need — the API output serializer will re-attach them for client compatibility.
Introduced a repository abstraction over the settings.notifications JSON blob. Reads come from the in-process settings cache; writes go through the Settings model (Bookshelf) to preserve existing cache invalidation. The class is consumed via constructor-injected settingsCache and settingsModel — no module-level globals. Not yet consumed by any caller; the legacy notifications service stays in place until the next phase switches the API endpoint over.
Dropped the monolithic services/notifications/notifications.js in favour
of a NotificationService that orchestrates the repository, the domain
operations (dismiss/markSeen co-located with the Notification schema),
and the new methods (browse, add, dismiss, dismissAll).
Also:
- API endpoint controller becomes a thin frame-to-service translator;
storage logic moves into the service.
- Output serializer explicitly projects the wire shape (location,
status) instead of deleting internal fields.
- models/base/listeners.js notifications prune listener removed — the
repository owns pruning, invoked by the service after every add.
- Dropped the legacy regex-based version filters, dangerousDestroyAll
self-heal, and the seen single-user fallback. The seenBy array is
the only dismissal tracker now.
- Service methods take userId strings instead of {id} wrappers; the
defensive frame.user && frame.user.id checks are gone (permissions
middleware guarantees frame.user exists).
Moved the wire-shape-to-NotificationInput translation out of update-check-service.js into a focused handler in services/update-check/notification-handler.ts. The handler: - Validates the wire payload via a zod schema (rejects malformed input with an empty result rather than crashing). - Applies the notificationGroups version-matching filter at the handler boundary. - Sanitises unknown notification types to 'info'. - Lives in the update-check domain (the consumer), not in the notifications domain — dependency direction is UpdateCheck → Notification. Also expanded the integration test to pin every field of the stored Notification produced by an UpdateCheck wire response, so the mapping contract is locked down.
…actor
Two related layering improvements:
1. UpdateCheck calls NotificationService.add() directly instead of
going through api.notifications.add. The HTTP-mirroring API shim
is for HTTP callers; internal callers should hit the domain.
Removes a permissions-middleware no-op and the {context: {internal:
true}} ceremony at the call site.
2. The alert-email side-effect moves out of UpdateCheck into a reactor
in the notifications domain. NotificationService.add() invokes the
reactor (afterAdd hook) for each newly-stored notification. The
reactor only fires for type:alert and uses the site URL from
urlUtils and the admin email lookup from the User model.
Behaviour change: alert-type notifications now fire emails regardless
of source. Previously only UpdateCheck's path emailed. The unified
reactor makes the side-effect consistent — integration test updated
to reflect this.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
migration
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/core/server/models/user.js`:
- Around line 513-515: findActiveAdministrators currently hardcodes filter:
'status:active' which excludes statuses like warn-1..warn-4; update the
updatedOptions assignment to use the model's existing active status set (e.g.,
ACTIVE_STATUSES / this.activeStatuses / activeStatusSet) instead of the literal
'status:active' so the filter includes all active-state values; construct the
filter from that set and assign it to updatedOptions.filter within
findActiveAdministrators so warn-1..warn-4 are treated as active.
In `@ghost/core/core/server/services/notifications/alert-email-reactor.ts`:
- Around line 22-24: The calls to deps.fetchAdminEmails() and deps.getSiteUrl()
can throw outside the per-recipient try/catch and must be guarded: wrap the
calls that set adminEmails and siteUrl in a try/catch (around
deps.fetchAdminEmails() and deps.getSiteUrl()) inside alert-email-reactor.ts,
log the error via the reactor logger and return/exit the function early or set
safe defaults so the subsequent for (const email of adminEmails) loop and
per-recipient try/catch do not allow dependency failures to bubble up and fail
the add flow.
In `@ghost/core/core/server/services/update-check/notification-handler.ts`:
- Around line 47-56: The current block inside notification.custom builds a
RegExp from options.notificationGroups and uses wireVersion.match(new
RegExp(g)), which is unnecessary and introduces ReDoS risk; instead change the
matched logic to use plain string matching (e.g. replace the RegExp call so
matched is computed with groups.some(g => wireVersion.includes(g)) or
groups.includes(wireVersion) depending on desired semantics), keeping the same
variables (notification.custom, options.notificationGroups, wireVersion,
matched) and returning [] when no match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d4747f5-2e75-44e5-9ed3-8230e3001125
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/notifications.test.js.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
ghost/core/core/server/api/endpoints/notifications.jsghost/core/core/server/api/endpoints/utils/serializers/output/notifications.jsghost/core/core/server/data/schema/default-settings/default-settings.jsonghost/core/core/server/models/base/listeners.jsghost/core/core/server/models/user.jsghost/core/core/server/services/notifications/alert-email-reactor.tsghost/core/core/server/services/notifications/index.jsghost/core/core/server/services/notifications/notification.tsghost/core/core/server/services/notifications/notifications.jsghost/core/core/server/services/notifications/repository.tsghost/core/core/server/services/notifications/service.tsghost/core/core/server/services/update-check/index.jsghost/core/core/server/services/update-check/notification-handler.tsghost/core/core/server/services/update-check/update-check-service.jsghost/core/package.jsonghost/core/test/e2e-api/admin/notifications.test.jsghost/core/test/integration/services/notifications/notification.test.tsghost/core/test/unit/server/models/user.test.jsghost/core/test/unit/server/services/notifications/notifications.test.jsghost/core/test/unit/server/services/settings/settings-bread-service.test.jsghost/core/test/unit/server/services/update-check.test.js
💤 Files with no reviewable changes (6)
- ghost/core/core/server/data/schema/default-settings/default-settings.json
- ghost/core/core/server/services/notifications/notifications.js
- ghost/core/test/e2e-api/admin/notifications.test.js
- ghost/core/core/server/models/base/listeners.js
- ghost/core/test/unit/server/services/notifications/notifications.test.js
- ghost/core/test/unit/server/services/update-check.test.js
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #27868 +/- ##
==========================================
+ Coverage 73.74% 73.88% +0.13%
==========================================
Files 1515 1523 +8
Lines 127419 128173 +754
Branches 15236 15381 +145
==========================================
+ Hits 93963 94696 +733
- Misses 32509 32544 +35
+ Partials 947 933 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Moved the admin/owner email lookup off `api.users.browse` into a new User.findActiveAdministrators bookshelf method. Filtering by role happens server-side. The reactor previously called the API just to pull a list of admin emails, which crossed a layer it didn't need to. - Inverted the opt-out filter for getEmailAlertUsers: pass the notification-flag column name, find users who don't opt out, then filter to active administrators. Reads cleaner than the previous boolean fan-out. Existing call sites unchanged; the inversion is internal. - The notifications bootstrap uses User.findActiveAdministrators directly instead of api.users.browse. Drops the lazy-require workaround for the previous circular-load risk.
The notification refactor deleted the `settings.notifications.edited` bookshelf listener (pruning moved into the service). The legacy listener tests still asserted against it; drop the dead describe. Also adopt structured logging for the alert email reactor and guard the recipient-resolution step so a `fetchAdminEmails` failure doesn't bubble up into `notifications.add` and break the producer flow.
rob-ghost
force-pushed
the
feat/ber-3651-ghsa-notifications
branch
from
f8fa616 to
be73213
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The notifications domain had grown organically over many years — wire parsing, storage, dismissal tracking, pruning, and email side effects were tangled across the notifications service, the update-check service, and a stray prune listener on the settings model. Test coverage was split across e2e, unit, and integration suites that each exercised partial slices.
In isolation, none of this was broken. But it made the next planned step — adding a GitHub Security Advisories (GHSA) feed that emits critical security notifications with their own email template — much harder than it should have been. Every new producer would have needed to relearn storage, permissions, dispatch, admin lookup, and email logic individually.
Solution
Make-the-change-easy, then make-the-easy-change. This PR is the first half: a no-op architectural refactor that prepares the domain for a new feed to drop in cleanly. Adding GHSA in a follow-up should be primarily a new feed handler plus a new email template — everything else already routes through the right seams.
The safety net is the first commit: a single consolidated integration test suite that locks down today's observable behaviour. Every subsequent commit can reshape internals aggressively because the suite proves the contract is preserved.
The refactor moves toward:
The commits are structured as discrete reviewable phases. Each commit is a coherent architectural step: tests, domain, repository, service cutover, anti-corruption layer for UpdateCheck, reactor extraction, cleanup.
Behavioural change to flag
The refactor is a no-op except for one deliberate change: alert-type notifications now trigger admin emails regardless of source. Previously only UpdateCheck's path emailed; the admin API create path did not. Unifying the side effect into the reactor makes the behaviour consistent and is the necessary precondition for the GHSA feed to also fire emails via the same path.