Changed member welcome email job to run based on config#21
Conversation
Code Review by Qodo
1. var member used
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| 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.
1. var member used 📘 Rule violation ✓ Correctness
• var member; is introduced in MemberRepository, violating the requirement to use block-scoped let/const instead of var. • Using var can cause hoisting and scoping surprises, increasing the risk of subtle runtime bugs in async-heavy repository code.
Agent prompt
## Issue description
`MemberRepository` introduces `var member;`, which violates the codebase requirement to use `let`/`const` and risks function-scoped leakage/hoisting issues.
## Issue Context
This occurs in the member creation flow where variables may be used across async boundaries.
## Fix Focus Areas
- ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js[339-342]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+15
to
18
| if (hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { | ||
| jobsService.addJob({ | ||
| at: '0 */5 * * * *', | ||
| job: path.resolve(__dirname, 'process-outbox.js'), |
There was a problem hiding this comment.
2. Welcome job not scheduled 🐞 Bug ✓ Correctness
• The scheduling guard was inverted (hasScheduled.processOutbox instead of !hasScheduled.processOutbox), so on first boot the recurring job is never added. • As a result, welcome-email outbox entries will never be processed, effectively disabling the feature even when configured. • Because scheduling happens during background service init, there is no later retry point to recover from this.
Agent prompt
### Issue description
The recurring job for processing member welcome email outbox entries is never scheduled because the guard condition was inverted.
### Issue Context
`hasScheduled.processOutbox` initializes to `false`, but scheduling currently only happens when it is `true`, so `jobsService.addJob()` never runs.
### Fix Focus Areas
- ghost/core/core/server/services/member-welcome-emails/jobs/index.js[11-25]
Suggested change:
- Replace `if (hasScheduled.processOutbox && ...)` with `if (!hasScheduled.processOutbox && ...)`.
- (Optional hardening) use `process.env.NODE_ENV?.startsWith('test')` or rely on the existing boot-level test guard to avoid duplicating the env check.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+340
to
+342
| var member; | ||
| const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox'); | ||
| if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) { |
There was a problem hiding this comment.
3. Outbox gating uses or 🐞 Bug ⛯ Reliability
• Member creation now enters the outbox-writing path when either the config is set OR the source is allowed; since the allowed source list includes normal member signups, this creates outbox entries even when the config is empty/undefined. • This contradicts the updated tests, which expect no outbox entry when the config is not set. • It also conflicts with the sending code, which throws if memberWelcomeEmailTestInbox is missing, causing failures/retries (or a growing backlog if the processor job is not running).
Agent prompt
### Issue description
Outbox entries for welcome emails are created even when the feature is effectively disabled (config unset), because an OR condition is used.
### Issue Context
- `WELCOME_EMAIL_SOURCES` includes `member`, so normal signup sources make the OR condition true even if the config is empty.
- The sender (`send-member-welcome-email`) throws if the config is missing.
- Tests explicitly expect no outbox entry when the config is not set.
### Fix Focus Areas
- ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js[339-378]
Suggested change:
- Replace:
- `if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source))`
- With:
- `if (welcomeEmailConfig && WELCOME_EMAIL_SOURCES.includes(source))`
- Consider renaming `welcomeEmailConfig` to something like `testInbox` and explicitly checking for non-empty string if needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
Benchmark PR from agentic-review-benchmarks#6