Changed member welcome email job to run based on config#44
Conversation
WalkthroughThis pull request migrates member welcome email configuration from a feature flag-based approach (labs.isSet) to a configuration-based approach (config.get). The change affects job scheduling, member creation logic, and corresponding test implementations across integration and unit tests. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/member-welcome-emails/jobs/index.js (1)
15-23: Critical logic bug: Job will never be scheduled.The condition
hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')will always evaluate tofalseon the first call becausehasScheduled.processOutboxis initialized tofalse. Since the job is only added inside this block, andhasScheduled.processOutboxis only set totrueinside this block, the job can never be scheduled.Based on the AI summary indicating this should schedule "when there is no existing scheduled outbox," the condition should be inverted:
🐛 Proposed fix
- if (hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { + if (!hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { jobsService.addJob({ at: '0 */5 * * * *', job: path.resolve(__dirname, 'process-outbox.js'), name: 'process-member-welcome-emails' }); hasScheduled.processOutbox = true; }
🤖 Fix all issues with AI agents
In
`@ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js`:
- Line 340: Replace the function-scoped declaration "var member;" with a
block-scoped "let member;" in MemberRepository.js so the variable uses proper
block scoping; locate the standalone declaration of member (currently "var
member;") within the method of MemberRepository and change it to "let member"
ensuring no other references rely on function hoisting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/server/services/member-welcome-emails/jobs/index.jsghost/core/core/server/services/members/members-api/repositories/MemberRepository.jsghost/core/test/integration/services/member-welcome-emails.test.jsghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js
🧰 Additional context used
🧬 Code graph analysis (3)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (3)
ghost/core/core/server/services/member-welcome-emails/jobs/index.js (1)
config(3-3)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (3)
config(11-11)require(6-6)require(8-8)ghost/core/core/server/services/member-welcome-emails/jobs/lib/send-member-welcome-email.js (2)
config(4-4)require(3-3)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (2)
ghost/core/core/server/services/member-welcome-emails/jobs/index.js (1)
config(3-3)ghost/core/core/server/services/member-welcome-emails/jobs/lib/send-member-welcome-email.js (2)
config(4-4)require(3-3)
ghost/core/core/server/services/member-welcome-emails/jobs/index.js (1)
ghost/core/core/server/services/member-welcome-emails/jobs/lib/send-member-welcome-email.js (2)
config(4-4)require(3-3)
🪛 ESLint
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js
[error] 340-340: Unexpected var, use let or const instead.
(no-var)
🔇 Additional comments (6)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (1)
340-342: Config-based gating logic looks correct.The change from labs-based feature flag to config-based toggle is consistent with the PR objectives. The condition
welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)properly gates the enhanced member creation path:
- When
memberWelcomeEmailTestInboxconfig is set, the enhanced path executes- When config is not set, it falls back to checking if source is in
WELCOME_EMAIL_SOURCESThis aligns with the related changes in the job scheduler and tests.
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (2)
529-551: Test correctly validates config-based outbox creation.The test properly stubs
config.getto return a test inbox value and verifies that the outbox entry is created with the correct payload structure. Good coverage of the happy path.
554-569: Test accurately reflects the config-driven behavior.The test name and implementation correctly verify that no outbox entry is created when
memberWelcomeEmailTestInboxconfig returnsundefined. This aligns with the production code logic where a falsy config value skips the enhanced path.ghost/core/core/server/services/member-welcome-emails/jobs/index.js (1)
10-13: Config guard is correctly implemented.The early return when
memberWelcomeEmailTestInboxconfig is not set properly gates the job scheduling, consistent with the config-driven approach used in MemberRepository.ghost/core/test/integration/services/member-welcome-emails.test.js (2)
52-65: Test correctly validates disabled config behavior.Setting
memberWelcomeEmailTestInboxto an empty string effectively makes it falsy, which correctly simulates the "config not set" scenario. The test properly verifies that no outbox entry is created when the config is disabled.
21-25: Good cleanup with configUtils.restore().Adding
configUtils.restore()inafterEachensures test isolation by resetting config changes between tests. This prevents config state leakage across test cases.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const memberAddOptions = {...(options || {}), withRelated}; | ||
| let member; | ||
| if (this._labsService.isSet('welcomeEmails') && WELCOME_EMAIL_SOURCES.includes(source)) { | ||
| var member; |
There was a problem hiding this comment.
Use let instead of var for block-scoped variable.
ESLint flags this as an error. The var keyword should be replaced with let for proper block scoping and consistency with modern JavaScript practices used throughout this codebase.
🛠️ Suggested fix
- var member;
+ let member;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var member; | |
| let member; |
🧰 Tools
🪛 ESLint
[error] 340-340: Unexpected var, use let or const instead.
(no-var)
🤖 Prompt for AI Agents
In
`@ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js`
at line 340, Replace the function-scoped declaration "var member;" with a
block-scoped "let member;" in MemberRepository.js so the variable uses proper
block scoping; locate the standalone declaration of member (currently "var
member;") within the method of MemberRepository and change it to "let member"
ensuring no other references rely on function hoisting.
2 participants
Benchmark PR from agentic-review-benchmarks#6
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.