Changed member welcome email job to run based on config#25465
Merged
troyciesco merged 5 commits intomainfrom Nov 18, 2025
Merged
Changed member welcome email job to run based on config#25465troyciesco merged 5 commits intomainfrom
troyciesco merged 5 commits intomainfrom
Conversation
Contributor
WalkthroughThe PR replaces feature-flag checks for member welcome emails (previously Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-06-10T11:07:10.800ZApplied to files:
📚 Learning: 2025-10-09T15:31:06.587ZApplied to files:
📚 Learning: 2025-10-30T17:13:26.190ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
30-36: Consider extracting the config stubbing pattern to reduce duplication.The same config stubbing pattern is repeated across all five tests. While the current approach is correct and functional, extracting it to a reusable helper would improve maintainability.
For example, you could add a helper function:
function stubMemberWelcomeEmailConfig(enabled) { const originalGet = config.get.bind(config); sinon.stub(config, 'get').callsFake((key) => { if (key === 'memberWelcomeEmailTestInbox') { return enabled ? 'test-inbox@example.com' : undefined; } return originalGet(key); }); }Then use it in each test:
- const originalGet = config.get.bind(config); - sinon.stub(config, 'get').callsFake((key) => { - if (key === 'memberWelcomeEmailTestInbox') { - return 'test-inbox@example.com'; - } - return originalGet(key); - }); + stubMemberWelcomeEmailConfig(true);Also applies to: 60-66, 81-87, 102-108, 123-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/server/services/member-welcome-emails/jobs/index.js(1 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(2 hunks)ghost/core/test/integration/services/member-welcome-emails.test.js(6 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 25288
File: ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js:46-64
Timestamp: 2025-11-10T23:10:17.470Z
Learning: In ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js and the outbox processing flow, entries are marked as PROCESSING before being processed. If a failure occurs after email send but before deletion, the entry remains stuck in PROCESSING state (not reprocessed). This intentional design prevents duplicate emails. Handling stuck PROCESSING entries is planned for a separate PR.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
📚 Learning: 2025-06-10T11:07:10.800Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Applied to files:
ghost/core/test/integration/services/member-welcome-emails.test.jsghost/core/core/server/services/members/members-api/repositories/MemberRepository.jsghost/core/core/server/services/member-welcome-emails/jobs/index.js
📚 Learning: 2025-10-09T15:31:06.587Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
Applied to files:
ghost/core/test/integration/services/member-welcome-emails.test.jsghost/core/core/server/services/members/members-api/repositories/MemberRepository.jsghost/core/core/server/services/member-welcome-emails/jobs/index.js
📚 Learning: 2025-11-10T23:10:17.470Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 25288
File: ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js:46-64
Timestamp: 2025-11-10T23:10:17.470Z
Learning: In ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js and the outbox processing flow, entries are marked as PROCESSING before being processed. If a failure occurs after email send but before deletion, the entry remains stuck in PROCESSING state (not reprocessed). This intentional design prevents duplicate emails. Handling stuck PROCESSING entries is planned for a separate PR.
Applied to files:
ghost/core/test/integration/services/member-welcome-emails.test.jsghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.jsghost/core/core/server/services/member-welcome-emails/jobs/index.js
📚 Learning: 2025-10-30T17:13:26.190Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
Applied to files:
ghost/core/core/server/services/member-welcome-emails/jobs/index.js
🧬 Code graph analysis (4)
ghost/core/test/integration/services/member-welcome-emails.test.js (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/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/core/server/services/members/members-api/repositories/MemberRepository.js (5)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (2)
config(7-7)require(6-6)ghost/core/core/server/services/member-welcome-emails/jobs/index.js (1)
config(3-3)ghost/core/test/integration/services/member-welcome-emails.test.js (2)
config(6-6)require(4-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/core/boot.js (1)
config(445-445)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (1)
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/index.js (2)
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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: E2E Tests (Shard 1/2)
- GitHub Check: Inspect Docker Image
- GitHub Check: E2E Tests (Shard 2/2)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
🔇 Additional comments (5)
ghost/core/core/server/services/member-welcome-emails/jobs/index.js (2)
3-3: LGTM! Clean migration from labs flag to config.The change successfully replaces the labs-based feature flag with a configuration-driven approach. The config key
memberWelcomeEmailTestInboxclearly conveys its purpose, and the truthy check appropriately gates the job scheduling based on whether the test inbox is configured.Also applies to: 11-11
6-6: LGTM! Comprehensive test updates for config-based gating.The test updates consistently replace labs stubs with config stubs across all test cases. The pattern of wrapping the original
config.getmethod ensures that other config keys remain accessible, and the tests properly cover both enabled (returns email address) and disabled (returns undefined) scenarios.Also applies to: 30-36, 59-66, 81-87, 102-108, 123-129
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (1)
11-11: LGTM! Successful migration to config-based gating.The change correctly replaces the labs service check with a direct config module call. This aligns with the PR's objective to enable stress testing via a configured test inbox rather than toggling labs flags across sites.
Note: The
labsServiceremains in the constructor (line 94), which is expected as per the PR description—the labs flag may be retained for future UI-related toggles or other features.Also applies to: 341-341
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (1)
7-7: LGTM! Well-structured test updates for config-based feature gating.The unit test updates correctly replace labs service mocks with config stubs for the welcome email feature. The new "outbox integration" test suite (lines 465-632) properly excludes
labsServicefrom the constructor, while earlier tests retain it for testing other features—maintaining appropriate test isolation.Also applies to: 530-530, 554-555, 571-572, 596-597, 615-616
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
6-6: LGTM!The config import is correctly added to support the new configuration-based gating approach.
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19445075834 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
cmraible
approved these changes
Nov 18, 2025
Collaborator
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
No notes, nice one 👨🍳
So nice and easy to review 🙌
troyciesco
added a commit
that referenced
this pull request
Nov 18, 2025
ref https://linear.app/ghost/issue/NY-763 - Modifies the member welcome email job so it no longer references the `welcomeEmails` labs flag, and instead checks to ensure `memberWelcomeEmailTestInbox` is set in config - This will allow us to stress test a higher volume of emails than we'd get by manually turning on the labs flag for a few sites - By making sure the config exists before scheduling the job there won't be any unnecessary impact to self-hosted instances - Doesn't send the email to any real members, just to an internal (to Ghost) test inbox - The `welcomeEmails` lab flag itself isn't deleted yet - we'll likely use it for UI-related toggles in the future
This was referenced Jan 21, 2026
This was referenced Jan 21, 2026
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.
ref https://linear.app/ghost/issue/NY-763
welcomeEmailslabs flag, and instead checks to ensurememberWelcomeEmailTestInboxis set in configwelcomeEmailslab flag itself isn't deleted yet - we'll likely use it for UI-related toggles in the future