Changed member welcome email job to run based on config#30
Conversation
WalkthroughFeature-flag based member welcome email control is replaced with config-driven toggling using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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: 2
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)
11-23: Fix inverted scheduling guard (job never schedules).With
hasScheduled.processOutboxinitiallyfalse, the job never schedules; if it were ever settrue, repeated calls would reschedule. Also,NODE_ENVcan be undefined. Suggest restoring the original guard and using optional chaining.🐛 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`:
- Around line 341-342: The conditional that decides whether to create
welcome-email outbox entries incorrectly uses an OR: `welcomeEmailConfig ||
WELCOME_EMAIL_SOURCES.includes(source)`, which allows member-sourced signups to
enqueue when config is unset; change it back to an AND gate so both must be
true. Update the check around the welcome-email outbox creation (referencing
welcomeEmailConfig, WELCOME_EMAIL_SOURCES and source in MemberRepository.js) to
`welcomeEmailConfig && WELCOME_EMAIL_SOURCES.includes(source)` so outbox entries
are only created when the config is present and the source is allowed.
- Line 340: Replace the function-scope declaration using `var member;` with a
block-scoped `let member;` to satisfy the ESLint `no-var` rule; update the
declaration inside MemberRepository (look for the `member` variable in
MemberRepository.js) so all usages remain unchanged but the declaration uses
`let` instead of `var`.
📜 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/core/server/services/members/members-api/repositories/MemberRepository.js (1)
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)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (2)
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)
🪛 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 (2)
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
6-25: Config-driven setup/teardown looks solid.Using
configUtils.set(...)per test andconfigUtils.restore()inafterEachkeeps state isolated and matches the new config-based gating.Also applies to: 29-30, 52-54, 68-69, 83-84, 98-99
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (1)
7-7: Config-based stubbing aligns with new behavior.The
config.get('memberWelcomeEmailTestInbox')stubs cleanly model enable/disable scenarios and stay isolated viasinon.restore().Also applies to: 529-617
✏️ 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the MemberRepository.js file
fd "MemberRepository.js" --type f
# Check if the file exists at the exact path mentioned
if [ -f "ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js" ]; then
echo "=== File found at expected path ==="
echo "=== Content around line 340 ==="
sed -n '335,345p' "ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js" | cat -n
else
echo "File not found at expected path, searching..."
fiRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 887
🏁 Script executed:
#!/bin/bash
# Look for ESLint configuration files
echo "=== ESLint configuration files ==="
find . -maxdepth 3 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) 2>/dev/null | head -20
# Check root .eslintrc files
if [ -f ".eslintrc.js" ]; then
echo "=== .eslintrc.js content ==="
cat ".eslintrc.js" | head -100
fi
if [ -f ".eslintrc.json" ]; then
echo "=== .eslintrc.json content ==="
cat ".eslintrc.json" | head -100
fi
if [ -f ".eslintrc.yml" ] || [ -f ".eslintrc.yaml" ]; then
echo "=== .eslintrc.yml/.yaml content ==="
cat ".eslintrc.yml" 2>/dev/null || cat ".eslintrc.yaml" 2>/dev/null | head -100
fiRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 841
🏁 Script executed:
#!/bin/bash
# Check the ESLint configuration for ghost/core
echo "=== ./ghost/core/.eslintrc.js content ==="
cat "./ghost/core/.eslintrc.js"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 5722
🏁 Script executed:
#!/bin/bash
# Search for 'no-var' rule in the ESLint config
echo "=== Searching for no-var rule ==="
rg "no-var" --type js --type jsonRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 168
Replace var with let to comply with ESLint's no-var rule.
The no-var rule is set to error level in the ghost/core ESLint configuration, which will cause CI to fail.
Proposed 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-scope declaration using `var member;` with a
block-scoped `let member;` to satisfy the ESLint `no-var` rule; update the
declaration inside MemberRepository (look for the `member` variable in
MemberRepository.js) so all usages remain unchanged but the declaration uses
`let` instead of `var`.
| const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox'); | ||
| if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) { |
There was a problem hiding this comment.
Require config and allowed source to gate welcome-email outbox creation.
Using welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source) means member-sourced signups still enqueue outbox entries even when config is unset, which defeats the config-based disable and can leave pending outbox rows never processed. Line 342 should preserve the previous AND-gate behavior.
🐛 Proposed fix
- if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) {
+ if (welcomeEmailConfig && WELCOME_EMAIL_SOURCES.includes(source)) {📝 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.
| const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox'); | |
| if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) { | |
| const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox'); | |
| if (welcomeEmailConfig && WELCOME_EMAIL_SOURCES.includes(source)) { |
🤖 Prompt for AI Agents
In
`@ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js`
around lines 341 - 342, The conditional that decides whether to create
welcome-email outbox entries incorrectly uses an OR: `welcomeEmailConfig ||
WELCOME_EMAIL_SOURCES.includes(source)`, which allows member-sourced signups to
enqueue when config is unset; change it back to an AND gate so both must be
true. Update the check around the welcome-email outbox creation (referencing
welcomeEmailConfig, WELCOME_EMAIL_SOURCES and source in MemberRepository.js) to
`welcomeEmailConfig && WELCOME_EMAIL_SOURCES.includes(source)` so outbox entries
are only created when the config is present and the source is allowed.
2 participants
Benchmark PR from qodo-benchmark#246
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.