Changed member welcome email job to run based on config#17
Conversation
WalkthroughThe PR migrates the member welcome emails feature flag from a labs-based check to a config-based approach using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
15-23: Critical: Inverted scheduling condition prevents job from ever being scheduled.The condition
hasScheduled.processOutbox && ...will never be true on the first call becausehasScheduled.processOutboxis initialized tofalse(line 6). SincehasScheduled.processOutbox = trueis only set inside this block, the job can never be scheduled.The condition should be negated to match the original intent:
🐛 Proposed fix
- if (hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { + if (!hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) {
🤖 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 declaration using "let member;" (i.e., change the identifier
declared as var to let) so it adheres to the no-var ESLint rule; locate the "var
member" declaration in MemberRepository.js (the variable used later in this
method) and update it to "let" and run lint/tests to ensure no other scope
issues arise.
- Around line 341-342: The condition that decides whether to create a
welcome-email outbox entry is using a logical OR and thus allows entries
whenever memberWelcomeEmailTestInbox is set regardless of event source; update
the check in MemberRepository.js so that it uses logical AND: require both
welcomeEmailConfig (memberWelcomeEmailTestInbox) to be truthy and
WELCOME_EMAIL_SOURCES.includes(source) to be true before creating the outbox
entry (the symbols to change are welcomeEmailConfig, WELCOME_EMAIL_SOURCES, and
source in the conditional that precedes the outbox creation).
🧹 Nitpick comments (1)
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
52-65: Consider usingundefinedinstead of empty string to simulate unset config.Setting the config to an empty string may behave differently than an unset/undefined config value depending on how the code evaluates truthiness. The unit tests use
undefinedfor this case.♻️ Suggested consistency improvement
it('does NOT create outbox entry when config is not set', async function () { - configUtils.set('memberWelcomeEmailTestInbox', ''); + // Explicitly not setting the config, or: + // configUtils.set('memberWelcomeEmailTestInbox', undefined);
📜 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 (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/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/member-welcome-emails/jobs/index.js (4)
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/integration/services/member-welcome-emails.test.js (1)
require(4-4)ghost/core/test/unit/server/models/outbox.test.js (1)
require(3-3)ghost/core/test/integration/jobs/process-outbox.test.js (1)
require(7-7)
🪛 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/member-welcome-emails/jobs/index.js (1)
11-13: Config-based feature gate looks correct.The early return when
memberWelcomeEmailTestInboxis not configured properly gates the feature.ghost/core/test/integration/services/member-welcome-emails.test.js (2)
6-6: Test infrastructure migration looks good.The switch from
sinon.restore()toconfigUtils.restore()is appropriate for config-driven tests.Also applies to: 24-24
27-50: Test coverage for enabled welcome emails is comprehensive.The test properly validates outbox entry creation with correct payload fields (memberId, email, name, source) and status.
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (3)
529-552: Test correctly validates outbox creation for allowed source with config set.The stub pattern for config.get is appropriate for isolating the config dependency.
554-569: Test correctly validates that outbox is skipped when config is not set.Using
undefinedto represent unset config is the right approach.
571-594: This test will fail with the current MemberRepository.js implementation.The test expects that disallowed sources (import, admin, api) should NOT create outbox entries even when config is set. However, the
||operator inMemberRepository.jsline 342 causes the enhanced path (which creates the outbox entry) to be taken whenever config is truthy, ignoring the source check.Once the
||is changed to&&inMemberRepository.js, this test will pass as expected.
✏️ 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 a violation of the no-var rule.
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 declaration using "let member;" (i.e., change the identifier
declared as var to let) so it adheres to the no-var ESLint rule; locate the "var
member" declaration in MemberRepository.js (the variable used later in this
method) and update it to "let" and run lint/tests to ensure no other scope
issues arise.
| const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox'); | ||
| if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MemberRepository.js" | head -5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 170
🏁 Script executed:
wc -l ghost/core/core/server/services/members/members-api/repositories/MemberRepository.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 173
🏁 Script executed:
sed -n '330,355p' ghost/core/core/server/services/members/members-api/repositories/MemberRepository.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1182
🏁 Script executed:
# Find unit tests related to welcome email or this repository
find . -type f -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "does not create outbox entry for disallowed sources" 2>/dev/nullRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 180
🏁 Script executed:
# Search for WELCOME_EMAIL_SOURCES definition
rg "WELCOME_EMAIL_SOURCES" --type js -B 2 -A 2Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1329
🏁 Script executed:
grep -n "does not create outbox entry for disallowed sources" ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 173
🏁 Script executed:
# Get context around that test
sed -n '1,3000p' ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js | grep -B 20 -A 40 "does not create outbox entry for disallowed sources"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 2492
🏁 Script executed:
# Look for tests that verify outbox entry IS created (positive case)
sed -n '540,575p' ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1572
🏁 Script executed:
# Check how _resolveContextSource works to understand source resolution
rg "_resolveContextSource" ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js -B 2 -A 8Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 3812
🏁 Script executed:
# Check if there's any other context or comments explaining this feature
sed -n '320,380p' ghost/core/core/server/services/members/members-api/repositories/MemberRepository.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 2451
🏁 Script executed:
# Check if there are other tests that create outbox when config IS set
grep -B 5 -A 15 "creates outbox entry\|creates an outbox" ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js | head -40Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 862
Fix logic operator: || should be && to restrict outbox entries to allowed sources.
The condition welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source) incorrectly creates outbox entries whenever the config is set, regardless of source. This contradicts the unit test "does not create outbox entry for disallowed sources," which sets memberWelcomeEmailTestInbox but expects NO outbox entry for import/admin/api sources.
Change the operator to && so outbox entries are created only when BOTH the config is enabled AND the source is 'member':
🐛 Fix
const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox');
- 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 condition that decides whether to create a
welcome-email outbox entry is using a logical OR and thus allows entries
whenever memberWelcomeEmailTestInbox is set regardless of event source; update
the check in MemberRepository.js so that it uses logical AND: require both
welcomeEmailConfig (memberWelcomeEmailTestInbox) to be truthy and
WELCOME_EMAIL_SOURCES.includes(source) to be true before creating the outbox
entry (the symbols to change are welcomeEmailConfig, WELCOME_EMAIL_SOURCES, and
source in the conditional that precedes the outbox creation).
2 participants
Benchmark PR from qodo-benchmark#246
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.