Skip to content

Changed member welcome email job to run based on config#17

Open
tomerqodo wants to merge 6 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_changed_member_welcome_email_job_to_run_based_on_config_pr246from
sentry_combined_20260121_augment_sentry_coderabbit_1_head_changed_member_welcome_email_job_to_run_based_on_config_pr246
Open

Changed member welcome email job to run based on config#17
tomerqodo wants to merge 6 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_changed_member_welcome_email_job_to_run_based_on_config_pr246from
sentry_combined_20260121_augment_sentry_coderabbit_1_head_changed_member_welcome_email_job_to_run_based_on_config_pr246

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from qodo-benchmark#246

}

if (!hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) {
if (hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The condition if (hasScheduled.processOutbox && ...) will never execute because hasScheduled.processOutbox is initialized to false and only set to true inside the unreachable block.
Severity: HIGH

Suggested Fix

The condition should be inverted to check if the job has not been scheduled yet. Change if (hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')) to if (!hasScheduled.processOutbox && !process.env.NODE_ENV.startsWith('test')).

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: ghost/core/core/server/services/member-welcome-emails/jobs/index.js#L15

Potential issue: The `scheduleMemberWelcomeEmailJob` function contains a logical
deadlock. The `hasScheduled.processOutbox` flag is initialized to `false`. The condition
to schedule the job checks `if (hasScheduled.processOutbox && ...)`, which will always
evaluate to false on the first run. The line that would set `hasScheduled.processOutbox
= true` is inside this unreachable conditional block. As a result, the
`process-member-welcome-emails` job will never be scheduled, and member welcome emails
will never be sent.

Did we get this right? 👍 / 👎 to inform future reviews.

if (this._labsService.isSet('welcomeEmails') && WELCOME_EMAIL_SOURCES.includes(source)) {
var member;
const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox');
if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The condition welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source) should use && instead of ||. The current logic sends welcome emails when it shouldn't, such as for imported members.
Severity: MEDIUM

Suggested Fix

The logical OR || operator should be replaced with a logical AND && operator. The condition should be welcomeEmailConfig && WELCOME_EMAIL_SOURCES.includes(source) to ensure an outbox entry is created only when both the feature is configured and the member creation source is valid.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js#L342

Potential issue: A logical condition was changed from `&&` to `||`, causing incorrect
behavior for creating member welcome email outbox entries. The new condition
`welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)` creates an entry if
either the config is set OR the source is 'member'. This means emails will be sent for
all sources (e.g., 'import') when the config is enabled, and will also be sent when the
config is disabled if the source is 'member'. Both behaviors are incorrect and break
existing tests.

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants