Moved automations service to separate file#28343
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR extracts the Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
fa3995c to
0a6768c
Compare
towards https://linear.app/ghost/issue/NY-1286 ref #28120 This change should have no impact on functionality. We have [a lint rule that limits the length of `index.js` files][0]. In [an upcoming change][1], the automations service will cross that limit. This patch moves it into a separate file. Doing so also makes it a little easier to test. [0]: https://github.com/TryGhost/eslint-plugin-ghost/blob/22d154a8e3807e4e30219fb8449e7aa9b90a110b/lib/config/node.js#L32-L37 [1]: #28120
0a6768c to
2eb8a65
Compare
There was a problem hiding this comment.
Git makes this diff look huge! Here's what actually changed in this file (it's just one line!):
-module.exports = new AutomationsService();
+module.exports = AutomationsService;There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/automations/service.test.js (2)
48-54: ⚡ Quick winCover all
init()side effects in the idempotence test.This currently proves only that subscriptions stay at two. A regression that re-dispatches the initial poll or re-registers with
schedulerAdapteron laterinit()calls would still pass, even though both are part of the extracted service contract.💡 Suggested test expansion
it('subscribes each poller only once when init is called multiple times', function () { automations.init(initOptions); automations.init(initOptions); automations.init(initOptions); automations.init(initOptions); + sinon.assert.calledTwice(domainEvents.subscribe); + sinon.assert.calledOnce(domainEvents.dispatch); + sinon.assert.calledOnceWithExactly(schedulerAdapter.register, automations); });🤖 Prompt for 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. In `@ghost/core/test/unit/server/services/automations/service.test.js` around lines 48 - 54, The test should assert all idempotent side effects of automations.init: after calling automations.init(initOptions) multiple times, verify domainEvents.subscribe is still called twice, schedulerAdapter.register is called only once, and the pollers' startup is not re-dispatched by spying on the poller start method (e.g., Poller.prototype.start or the concrete poller start function used) and asserting it was called only once; add these additional assertions to the existing test so it fails if init() re-registers with schedulerAdapter or re-dispatches the initial poll on subsequent calls.
4-4: ⚡ Quick winDrop the import change; focus on strengthening
init()idempotence assertionsThis suite already imports
ghost/core/server/services/automations/servicedirectly, so the.constructor/wrapper coupling concern (and related diff) isn’t applicable. Strengthen theinit()idempotence test to also assert that observable side effects likeschedulerAdapter.registerand the initial poll/dispatch happen only on the firstinit()call.🤖 Prompt for 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. In `@ghost/core/test/unit/server/services/automations/service.test.js` at line 4, The test incorrectly changed the import; revert any import modifications and keep requiring the concrete service module (restore original require), then strengthen the init() idempotence test on AutomationsService by asserting observable side effects only occur once: call service.init() twice and verify schedulerAdapter.register was called exactly once and that the initial poll/dispatch (the code path that triggers scheduling/polling) ran only on the first call (use spies/mocks on schedulerAdapter.register and the poll/dispatch entry point used by AutomationsService to assert single invocation). Ensure you reference AutomationsService and its init() method along with schedulerAdapter.register and the initial poll/dispatch function when locating where to add the assertions.
🤖 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.
Nitpick comments:
In `@ghost/core/test/unit/server/services/automations/service.test.js`:
- Around line 48-54: The test should assert all idempotent side effects of
automations.init: after calling automations.init(initOptions) multiple times,
verify domainEvents.subscribe is still called twice, schedulerAdapter.register
is called only once, and the pollers' startup is not re-dispatched by spying on
the poller start method (e.g., Poller.prototype.start or the concrete poller
start function used) and asserting it was called only once; add these additional
assertions to the existing test so it fails if init() re-registers with
schedulerAdapter or re-dispatches the initial poll on subsequent calls.
- Line 4: The test incorrectly changed the import; revert any import
modifications and keep requiring the concrete service module (restore original
require), then strengthen the init() idempotence test on AutomationsService by
asserting observable side effects only occur once: call service.init() twice and
verify schedulerAdapter.register was called exactly once and that the initial
poll/dispatch (the code path that triggers scheduling/polling) ran only on the
first call (use spies/mocks on schedulerAdapter.register and the poll/dispatch
entry point used by AutomationsService to assert single invocation). Ensure you
reference AutomationsService and its init() method along with
schedulerAdapter.register and the initial poll/dispatch function when locating
where to add the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46ea6ca6-99d6-4f54-985e-fb79ce2c23f9
📒 Files selected for processing (4)
ghost/core/core/server/services/automations/index.jsghost/core/core/server/services/automations/service.jsghost/core/test/unit/server/services/automations/index.test.jsghost/core/test/unit/server/services/automations/service.test.js
towards https://linear.app/ghost/issue/NY-1286
ref #28120
This change should have no impact on functionality.
We have a lint rule that limits the length of
index.jsfiles. In an upcoming change, the automations service will cross that limit.This patch moves it into a separate file. Doing so also makes it a little easier to test.