Changed member welcome email job to run based on config#8
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bde2541dfa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| 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 new guard only adds the job when hasScheduled.processOutbox is already true, so in any non-test environment with memberWelcomeEmailTestInbox configured the job never gets scheduled on the first call and process-outbox.js won’t run. This prevents welcome emails from ever being processed despite the config being enabled.
Useful? React with 👍 / 👎.
| const welcomeEmailConfig = config.get('memberWelcomeEmailTestInbox'); | ||
| if (welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source)) { |
There was a problem hiding this comment.
The condition welcomeEmailConfig || WELCOME_EMAIL_SOURCES.includes(source) allows outbox entries to be created whenever the source is member, even if the config is unset/empty. In environments where memberWelcomeEmailTestInbox is not configured (the intended “off” state), a normal signup will still enqueue welcome emails, which contradicts “run based on config” and can silently fill the outbox.
Useful? React with 👍 / 👎.
| 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.
The root AGENTS.md explicitly forbids var declarations; introducing var member violates that compliance rule. When lint/compliance checks run (CI or pre-commit), this can fail the build, and var’s function scoping/hoisting makes it easier for future branch additions to accidentally reuse a stale value.
Useful? React with 👍 / 👎.
2 participants
Benchmark PR from agentic-review-benchmarks#6