Changed member welcome email job to run based on config#19
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request changes the member welcome email feature from using a lab feature flag (welcomeEmails) to a config-based approach (memberWelcomeEmailTestInbox). The change aims to control welcome email functionality through configuration rather than feature flags.
Changes:
- Replaced
labsService.isSet('welcomeEmails')checks withconfig.get('memberWelcomeEmailTestInbox')checks - Updated test files to use config stubs/utilities instead of labs stubs
- Removed
labsServicedependency from affected components
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js | Updated member creation logic to check config instead of lab flag for welcome email outbox entry creation |
| ghost/core/core/server/services/member-welcome-emails/jobs/index.js | Modified job scheduler to check config instead of lab flag before scheduling welcome email processing |
| ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js | Updated unit tests to stub config instead of labsService |
| ghost/core/test/integration/services/member-welcome-emails.test.js | Updated integration tests to use configUtils instead of sinon stubs for labs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (this._labsService.isSet('welcomeEmails') && WELCOME_EMAIL_SOURCES.includes(source)) { | ||
| var member; | ||
| const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox'); | ||
| if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) { |
There was a problem hiding this comment.
The logical operator should be AND (&&) instead of OR (||). The current logic creates an outbox entry whenever either the config is set OR the source is in the allowed list, but it should only create an entry when BOTH conditions are true (config is set AND source is allowed). This means welcome emails would be sent for disallowed sources like 'import' or 'admin' as long as the config is set.
| if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) { | |
| if (welcomeEmailConfig && WELCOME_EMAIL_SOURCES.includes(source)) { |
| } | ||
|
|
||
| if (!hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { | ||
| if (hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { |
There was a problem hiding this comment.
The negation operator is missing. This should be !hasScheduled.processOutbox instead of hasScheduled.processOutbox. With the current logic, the job will only be scheduled if it has already been scheduled, which means it will never be scheduled on the first run. This prevents the welcome email job from ever being scheduled.
| if (hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { | |
| if (!hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) { |
| 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' or 'const' instead of 'var' for block-scoped variable declaration. The use of 'var' is discouraged in modern JavaScript as it has function scope rather than block scope, which can lead to unexpected behavior.
| var member; | |
| let member; |
|
|
||
| it('does NOT create outbox entry when config is not set', async function () { | ||
| configUtils.set('memberWelcomeEmailTestInbox', ''); | ||
There was a problem hiding this comment.
Remove trailing whitespace at the end of this line.
Benchmark PR from qodo-benchmark#246