Updated member welcome email with automated_email content #25626
Conversation
WalkthroughThis pull request replaces a procedural, template-and-config-driven welcome-email flow with a service-oriented implementation. It adds MemberWelcomeEmailRenderer and MemberWelcomeEmailService (with a lazy wrapper), introduces AutomatedEmail model usage and MEMBER_WELCOME_EMAIL_SLUGS/MESSAGES constants, removes legacy template modules and mail-config/send-member-welcome-email helpers, updates outbox processing to use the new service (removing mailConfig parameters), and adjusts member repository logic to conditionally create outbox entries based on automated email activation. Tests and e2e factories were added/updated to exercise the new flow. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Key areas that need extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
troyciesco
changed the title
troyciesco
force-pushed
the
NY-773_render-lexical-in-welcome-email
branch
from
103a136 to
0d6f884
Compare
| }); | ||
| } | ||
|
|
||
| if (template.status !== 'active') { |
There was a problem hiding this comment.
I added this part with the thinking that I want to avoid a scenario where an outbox entry is added while a template is active, and somehow, like maybe if the immediate send fails, and if the template is then sent to inactive, I don't then want this outbox row to get processed on the next run.
And if the template is reset to active like a week later, I don't want to then reprocess the outbox row.
So right now I'm just throwing an error, but I'm wondering if I should like actually delete the row or something. I feel like this is something we'll probably fine-tune with usage, and it's very much an edge case.
ghost/core/core/server/services/member-welcome-emails/service.js
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
ghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.js (1)
13-255: Good coverage of process-entries behavior; consider one extra assertion.The suite exercises handler dispatch, payload parsing, retry + truncation logic, delete failure handling, and error logging thoroughly. As a small enhancement, in the “handler not found” case you could also assert the updated
status(e.g.FAILED) in addition to themessageto fully lock in the state transition, but the current coverage is already strong.ghost/core/test/integration/jobs/process-outbox.test.js (1)
4-61: Integration wiring for automated_emails + config looks solid; slug could reuse constant.The test setup correctly:
- Stubs mail sending and
config.getwhile delegating other keys to the original implementation.- Seeds
automated_emailswith a realistic Lexical payload and active status.- Cleans up both
outboxandautomated_emailsinafterEach.If you want to reduce coupling to magic strings, you could import the free welcome-email slug from
member-welcome-emails/constantsinstead of duplicating'member-welcome-email-free'here, but that’s optional.ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js (1)
2-83: Outbox job flow and service initialization look correct; consider guardinginitif it ever throws.The refactor to:
- Use a transactional
fetchPendingEntrieswithFOR UPDATEand PROCESSING status updates, and- Initialize + preload member welcome emails via
memberWelcomeEmailServicebefore processingis consistent and reads well.
If
memberWelcomeEmailService.init()is ever changed to perform work that can throw, you might want to move it inside thetryblock or make it async/awaited there, but for a synchronous initializer this is fine.ghost/core/test/unit/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.test.js (1)
1-207: Renderer tests give excellent coverage; one assertion could be slightly less brittle.This suite does a great job exercising:
- Lexical rendering delegation,
- Member/site variable substitution (including first name + fallbacks),
- Accent color inlining and wrapper usage,
- Plain‑text extraction, and
- Mapping Lexical errors to
IncorrectUsageErrorwith preserved context.The only minor nit is in the “falls back to empty string when member email is missing” test, where checking for
'Email: </p>'ties the expectation to a specific HTML structure. As a small hardening step, you could assert on'Email: 'instead to keep the test robust to markup changes.ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.js (2)
158-168: Test entries missing required fields.The entries at lines 160-161 only have
idbut are missingevent_typeandpayloadfields that other tests include (e.g., line 149). While this may work sinceprocessEntriesStubis mocked, it could mask issues if the stub is ever removed or the test is used as documentation for entry structure.Consider adding the missing fields for consistency:
queryStub.select - .onFirstCall().resolves([{id: '1'}]) - .onSecondCall().resolves([{id: '2'}]) + .onFirstCall().resolves([{id: '1', event_type: 'MemberCreatedEvent', payload: '{}'}]) + .onSecondCall().resolves([{id: '2', event_type: 'MemberCreatedEvent', payload: '{}'}]) .onThirdCall().resolves([]);
227-235: Consider testing with a proper Error object.Line 229 rejects with an empty object
{}rather than an Error instance. While this tests the "unknown error" path, it may not accurately represent real error scenarios.it('handles unknown errors during template loading', async function () { - memberWelcomeEmailServiceStub.api.loadMemberWelcomeEmails.rejects({}); + memberWelcomeEmailServiceStub.api.loadMemberWelcomeEmails.rejects(new Error()); const result = await processOutbox(); result.should.containEql('Job aborted'); });ghost/core/test/unit/server/services/member-welcome-emails/service.test.js (1)
104-115: Consider testing error message content.The test verifies that
IncorrectUsageErroris thrown when no template is found, but doesn't verify the error message matchesMESSAGES.NO_MEMBER_WELCOME_EMAIL. This would help catch message constant regressions.await serviceModule.api.send({ member: {name: 'John', email: '[email protected]'} - }).should.be.rejectedWith(errors.IncorrectUsageError); + }).should.be.rejectedWith(errors.IncorrectUsageError, /No member welcome email found/);ghost/core/core/server/services/member-welcome-emails/service.js (1)
95-102: Consider enforcing template loading during initialization.The wrapper's
init()method creates the service instance but doesn't ensure thatloadMemberWelcomeEmails()is called. Ifsend()is invoked before templates are loaded, it will throw anIncorrectUsageErrorat line 50-54.While the error handling provides a safety net, explicitly loading templates during initialization would make the service more robust and fail earlier if the database is unreachable.
Consider updating the wrapper to load templates during initialization:
class MemberWelcomeEmailServiceWrapper { - init() { + async init() { if (this.api) { return; } this.api = new MemberWelcomeEmailService(); + await this.api.loadMemberWelcomeEmails(); } }Note: This would make
init()async, which may require updates to callers. Verify the impact before applying this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/constants.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.html.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.txt.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/email-templates/wrapper.hbs(1 hunks)ghost/core/core/server/services/member-welcome-emails/get-mail-config.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/index.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/send-member-welcome-email.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/services/outbox/handlers/member-created.js(2 hunks)ghost/core/core/server/services/outbox/jobs/lib/process-entries.js(3 hunks)ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js(3 hunks)ghost/core/test/integration/jobs/process-outbox.test.js(2 hunks)ghost/core/test/unit/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.test.js(1 hunks)ghost/core/test/unit/server/services/member-welcome-emails/send-member-welcome-email.test.js(0 hunks)ghost/core/test/unit/server/services/member-welcome-emails/service.test.js(1 hunks)ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js(1 hunks)ghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.js(1 hunks)ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.js(1 hunks)
💤 Files with no reviewable changes (5)
- ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.txt.js
- ghost/core/core/server/services/member-welcome-emails/send-member-welcome-email.js
- ghost/core/core/server/services/member-welcome-emails/get-mail-config.js
- ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.html.js
- ghost/core/test/unit/server/services/member-welcome-emails/send-member-welcome-email.test.js
🧰 Additional context used
🧠 Learnings (14)
📚 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/unit/server/services/outbox/jobs/lib/process-outbox.test.jsghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.jsghost/core/test/integration/jobs/process-outbox.test.jsghost/core/core/server/services/outbox/jobs/lib/process-entries.jsghost/core/test/unit/server/services/outbox/handlers/member-created.test.jsghost/core/core/server/services/member-welcome-emails/service.jsghost/core/core/server/services/outbox/handlers/member-created.jsghost/core/core/server/services/outbox/jobs/lib/process-outbox.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.jsghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.jsghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/unit/server/services/member-welcome-emails/service.test.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Applied to files:
ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.jsghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.jsghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/unit/server/services/member-welcome-emails/service.test.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/constants.jsghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.jsghost/core/core/server/services/member-welcome-emails/index.jsghost/core/core/server/services/outbox/jobs/lib/process-entries.jsghost/core/core/server/services/member-welcome-emails/service.jsghost/core/core/server/services/outbox/handlers/member-created.jsghost/core/core/server/services/outbox/jobs/lib/process-outbox.js
📚 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/core/server/services/member-welcome-emails/constants.js
📚 Learning: 2025-06-13T11:57:58.226Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
Applied to files:
ghost/core/core/server/services/member-welcome-emails/email-templates/wrapper.hbs
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Each test receives fresh Ghost instance for automatic isolation
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/unit/server/services/member-welcome-emails/service.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Ensure tests have fresh Ghost instance isolation (automatic) and do not create test dependencies where Test B needs Test A
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Run `yarn test:integration` for integration tests in ghost/core
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.js
📚 Learning: 2025-09-02T13:06:50.918Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js:1388-1392
Timestamp: 2025-09-02T13:06:50.918Z
Learning: In Ghost's test files, sinon.restore() in afterEach hooks handles cleanup of all sinon modifications including fake timers, spies, and stubs, so explicit restoration of individual sinon features like clock.restore() is typically not needed when there's a top-level sinon.restore() call.
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.js
📚 Learning: 2025-10-06T14:19:50.385Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 24966
File: ghost/core/test/unit/server/services/mail/GhostMailer.test.js:330-421
Timestamp: 2025-10-06T14:19:50.385Z
Learning: The GhostMailer class in `ghost/core/core/server/services/mail/GhostMailer.js` does not accept external Mailgun-specific parameters (like `o:tag`) from callers. Tags are generated internally via the `getTags()` method. This design keeps the class generic and avoids mixing transport-specific concerns into the public interface.
Applied to files:
ghost/core/core/server/services/member-welcome-emails/service.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Applied to files:
ghost/core/test/unit/server/services/member-welcome-emails/service.test.js
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/test/unit/**/*.test.{ts,tsx,js} : Vitest unit tests should be located in `test/unit/*` with pattern `test/unit/**/*.(test).(ts|tsx|js)` and use the `render` helper from `test/unit/utils/test-utils.tsx`
Applied to files:
ghost/core/test/unit/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.test.js
🧬 Code graph analysis (11)
ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.js (1)
ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js (2)
require(4-4)require(5-5)
ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js (1)
ghost/core/core/server/services/member-welcome-emails/constants.js (1)
MESSAGES(8-13)
ghost/core/core/server/services/member-welcome-emails/index.js (1)
ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)
ghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.js (1)
ghost/core/test/integration/jobs/process-outbox.test.js (2)
sinon(3-3)require(7-7)
ghost/core/test/integration/jobs/process-outbox.test.js (2)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (4)
ObjectId(7-7)require(6-6)require(8-8)config(11-11)ghost/core/core/server/services/outbox/index.js (1)
require(6-6)
ghost/core/core/server/services/outbox/jobs/lib/process-entries.js (1)
ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js (4)
processEntries(6-6)processEntries(66-66)entries(10-10)entries(60-60)
ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js (4)
ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/core/server/services/outbox/handlers/member-created.js (1)
require(1-1)ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js (2)
require(4-4)require(5-5)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (2)
require(6-6)require(8-8)
ghost/core/core/server/services/member-welcome-emails/service.js (3)
ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js (1)
require(7-7)ghost/core/core/server/services/member-welcome-emails/constants.js (3)
MEMBER_WELCOME_EMAIL_SLUGS(3-6)MEMBER_WELCOME_EMAIL_LOG_KEY(1-1)MESSAGES(8-13)ghost/core/core/server/models/automated-email.js (1)
AutomatedEmail(5-37)
ghost/core/test/unit/server/services/member-welcome-emails/service.test.js (2)
ghost/core/core/server/services/member-welcome-emails/service.js (3)
require(9-9)require(11-11)errors(2-2)ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js (2)
require(7-7)errors(6-6)
ghost/core/test/unit/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.test.js (1)
ghost/core/test/unit/server/services/member-welcome-emails/service.test.js (4)
sinon(1-1)should(2-2)rewire(3-3)errors(4-4)
ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js (4)
ghost/core/core/server/services/member-welcome-emails/service.js (3)
require(9-9)require(11-11)logging(1-1)ghost/core/core/server/services/outbox/jobs/lib/process-entries.js (4)
require(2-2)require(3-3)MemberCreatedEvent(4-4)logging(1-1)ghost/core/core/server/services/outbox/index.js (2)
require(6-6)logging(1-1)ghost/core/core/server/services/outbox/jobs/lib/constants.js (1)
OUTBOX_LOG_KEY(4-4)
🔇 Additional comments (19)
ghost/core/core/server/services/outbox/handlers/member-created.js (1)
2-21: Handler delegation and logging helpers look correct.
handlecleanly forwards the full payload tomemberWelcomeEmailService.api.sendwithmemberStatus: 'free', andgetLogInfo’s fallbacks are sensible and are well covered by tests. This file looks good as-is; the TODO about derivingmemberStatusfrom payload is clear for future work.ghost/core/core/server/services/member-welcome-emails/email-templates/wrapper.hbs (1)
1-45: Wrapper template matches Ghost’s email conventions.The structure, inline styles, and use of
{{subject}},{{accentColor}},{{{content}}}, and{{siteTitle}}are consistent with existing email templates, and align with the renderer tests. No changes needed here.ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js (1)
1-87: Unit tests thoroughly cover handler behavior and log key.The tests nicely validate:
- Delegation to
memberWelcomeEmailService.api.sendwith full member payload andmemberStatus: 'free'.- All
getLogInfobranches (name+email, email‑only, unknown member cases).- The format of
LOG_KEY.This provides solid protection around the handler; no changes needed.
ghost/core/core/server/services/member-welcome-emails/index.js (1)
1-7: No action needed—all call sites import from./servicedirectly rather than relying on the index.js exports.The search found no references to
sendMemberWelcomeEmailorgetMailConfiganywhere in the codebase. All existing callers (member-created.jsandprocess-outbox.js) import directly from'../../member-welcome-emails/service', so the public API shift in index.js poses no compatibility risk.ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js (2)
1-19: LGTM - Clean renderer initialization.The synchronous file read in the constructor is acceptable for service initialization, and creating a dedicated Handlebars instance provides good isolation.
30-76: Consider the security implications of compiling dynamic content as Handlebars templates.Lines 59-60 compile
content(rendered from lexical) andsubjectas Handlebars templates. While this enables template variable substitution, it also means any Handlebars expressions in the lexical content will be executed.Since this content comes from the
AutomatedEmailmodel (admin-controlled), this is likely acceptable, but worth confirming that:
- Only admin users can create/edit AutomatedEmail records
- The lexical editor sanitizes or escapes Handlebars-like syntax if untrusted input is possible
ghost/core/core/server/services/member-welcome-emails/constants.js (1)
3-19: LGTM - Well-organized constants.Good practice to centralize slugs and messages. The function-based message for dynamic
memberStatusis appropriate for parameterized messages.ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.js (1)
185-200: Test for MAX_ENTRIES_PER_JOB respects the limit correctly.Good test that validates batch processing respects the configured limits. The test properly sets up multiple batches and verifies the call count.
ghost/core/core/server/services/outbox/jobs/lib/process-entries.js (4)
43-49: Good addition of missing handler validation.The early return when no handler exists prevents undefined behavior and properly marks the entry as failed with a descriptive error message.
51-66: Error handling correctly differentiates parsing vs handler failures.The logic properly distinguishes between payload parsing failures (when
payloadis undefined) and handler execution failures, providing appropriate log messages for each case.
68-77: Robust cleanup error handling.Good pattern - marking the entry as COMPLETED when deletion fails prevents reprocessing while still returning success (since the email was sent). This aligns with the intentional design to prevent duplicate emails. Based on learnings, handling stuck PROCESSING entries is planned for a separate PR.
79-93: Clean function signature update.The removal of
mailConfigparameter aligns with the service-based architecture where theMemberWelcomeEmailServicenow manages email configuration internally.ghost/core/test/unit/server/services/member-welcome-emails/service.test.js (3)
165-240: Thorough send method test coverage.Good coverage of the send flow including:
- Template loading requirements
- Inactive template rejection
- Correct renderer invocation with member/site data
- Mailer parameters including
forceTextContent: true- Config-driven inbox override
269-296: Good logging verification tests.Testing both the presence and absence of member name in log messages ensures proper log output for debugging and monitoring.
298-327: Good default value coverage.Testing that
titledefaults to "Ghost" andaccentColordefaults to "#15212A" when settings are null ensures robust fallback behavior.ghost/core/core/server/services/member-welcome-emails/service.js (4)
1-11: LGTM!The imports are well-organized and the TypeScript comment is helpful for understanding the dynamic export limitation.
13-22: LGTM!The constructor properly initializes dependencies and sets up the internal email cache structure.
104-104: LGTM!The singleton wrapper export pattern is appropriate for this service.
85-91: Sender information is loaded but never used in the email send call.Lines 37-39 extract
senderEmailandsenderReplyTofrom the database, but these are not passed tothis.#mailer.send()at lines 85-91. GhostMailer supports optionalfromandreplyToparameters to customize the sender, so the configured values should be included:await this.#mailer.send({ to: toEmail, subject, html, text, + from: memberWelcomeEmail.senderEmail, + replyTo: memberWelcomeEmail.senderReplyTo, forceTextContent: true });If sender customization is not needed, remove the unused data extraction at lines 37-39 to keep the code clean.
⛔ Skipped due to learnings
Learnt from: sam-lord Repo: TryGhost/Ghost PR: 24966 File: ghost/core/test/unit/server/services/mail/GhostMailer.test.js:330-421 Timestamp: 2025-10-06T14:19:50.385Z Learning: The GhostMailer class in `ghost/core/core/server/services/mail/GhostMailer.js` does not accept external Mailgun-specific parameters (like `o:tag`) from callers. Tags are generated internally via the `getTags()` method. This design keeps the class generic and avoids mixing transport-specific concerns into the public interface.
| async loadMemberWelcomeEmails() { | ||
| for (const [memberStatus, slug] of Object.entries(MEMBER_WELCOME_EMAIL_SLUGS)) { | ||
| const row = await AutomatedEmail.findOne({slug}); | ||
|
|
||
| if (!row || !row.get('lexical')) { | ||
| this.#memberWelcomeEmails[memberStatus] = null; | ||
| continue; | ||
| } | ||
|
|
||
| this.#memberWelcomeEmails[memberStatus] = { | ||
| lexical: row.get('lexical'), | ||
| subject: row.get('subject'), | ||
| status: row.get('status'), | ||
| senderName: row.get('sender_name'), | ||
| senderEmail: row.get('sender_email'), | ||
| senderReplyTo: row.get('sender_reply_to') | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for database queries.
The AutomatedEmail.findOne() calls at line 26 lack error handling. If the database query fails, the error will propagate uncaught, potentially leaving the service in an inconsistent state.
Consider wrapping the loop or individual queries in try-catch blocks and logging failures:
async loadMemberWelcomeEmails() {
- for (const [memberStatus, slug] of Object.entries(MEMBER_WELCOME_EMAIL_SLUGS)) {
- const row = await AutomatedEmail.findOne({slug});
-
- if (!row || !row.get('lexical')) {
- this.#memberWelcomeEmails[memberStatus] = null;
- continue;
- }
-
- this.#memberWelcomeEmails[memberStatus] = {
- lexical: row.get('lexical'),
- subject: row.get('subject'),
- status: row.get('status'),
- senderName: row.get('sender_name'),
- senderEmail: row.get('sender_email'),
- senderReplyTo: row.get('sender_reply_to')
- };
- }
+ for (const [memberStatus, slug] of Object.entries(MEMBER_WELCOME_EMAIL_SLUGS)) {
+ try {
+ const row = await AutomatedEmail.findOne({slug});
+
+ if (!row || !row.get('lexical')) {
+ this.#memberWelcomeEmails[memberStatus] = null;
+ continue;
+ }
+
+ this.#memberWelcomeEmails[memberStatus] = {
+ lexical: row.get('lexical'),
+ subject: row.get('subject'),
+ status: row.get('status'),
+ senderName: row.get('sender_name'),
+ senderEmail: row.get('sender_email'),
+ senderReplyTo: row.get('sender_reply_to')
+ };
+ } catch (err) {
+ logging.error(`${MEMBER_WELCOME_EMAIL_LOG_KEY} Failed to load member welcome email for ${memberStatus}:`, err);
+ this.#memberWelcomeEmails[memberStatus] = null;
+ }
+ }
}🤖 Prompt for AI Agents
In ghost/core/core/server/services/member-welcome-emails/service.js around lines
24 to 42, wrap each AutomatedEmail.findOne() call in a try-catch so DB query
failures don't bubble up and leave the service in an inconsistent state; on
error log the exception using the service logger (or console as fallback), set
this.#memberWelcomeEmails[memberStatus] = null (or preserve previous value if
present) and continue the loop, ensuring the method completes instead of
throwing for a single failed query.
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19940501714 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19940501714 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (1)
346-349: LGTM! Logic correctly validates welcome email is active.The code properly checks if the free welcome email template exists, has lexical content, and is active before creating an outbox entry. This ensures only valid, active templates trigger welcome emails, which aligns with the PR objectives.
Consider caching the welcome email active status to avoid a database query on every member creation. This could be loaded once at service initialization or refreshed periodically, especially if member creation is a high-frequency operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/services/members/api.js(1 hunks)ghost/core/core/server/services/members/members-api/members-api.js(2 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(5 hunks)ghost/core/test/integration/services/member-welcome-emails.test.js(2 hunks)ghost/core/test/unit/server/services/member-welcome-emails/service.test.js(1 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/member-welcome-emails/service.js
🧰 Additional context used
🧠 Learnings (10)
📓 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.
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
ghost/core/test/unit/server/services/member-welcome-emails/service.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Applied to files:
ghost/core/test/unit/server/services/member-welcome-emails/service.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Each test receives fresh Ghost instance for automatic isolation
Applied to files:
ghost/core/test/unit/server/services/member-welcome-emails/service.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Applied to files:
ghost/core/test/unit/server/services/member-welcome-emails/service.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Ensure tests have fresh Ghost instance isolation (automatic) and do not create test dependencies where Test B needs Test A
Applied to files:
ghost/core/test/unit/server/services/member-welcome-emails/service.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Follow the AAA (Arrange, Act, Assert) pattern in test structure
Applied to files:
ghost/core/test/integration/services/member-welcome-emails.test.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/unit/server/services/members/members-api/repositories/MemberRepository.test.js
📚 Learning: 2025-09-02T13:06:50.918Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js:1388-1392
Timestamp: 2025-09-02T13:06:50.918Z
Learning: In Ghost's test files, sinon.restore() in afterEach hooks handles cleanup of all sinon modifications including fake timers, spies, and stubs, so explicit restoration of individual sinon features like clock.restore() is typically not needed when there's a top-level sinon.restore() call.
Applied to files:
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js
🧬 Code graph analysis (4)
ghost/core/core/server/services/members/api.js (6)
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
models(4-4)ghost/core/test/integration/jobs/process-outbox.test.js (1)
models(6-6)ghost/core/core/boot.js (1)
models(93-93)ghost/core/test/unit/server/models/outbox.test.js (1)
models(2-2)ghost/core/test/legacy/models/model_settings.test.js (1)
models(6-6)ghost/core/test/unit/server/data/exporter/index.test.js (1)
models(7-7)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (3)
ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (2)
require(6-6)AutomatedEmail(471-471)ghost/core/core/server/services/member-welcome-emails/constants.js (1)
MEMBER_WELCOME_EMAIL_SLUGS(3-6)
ghost/core/test/integration/services/member-welcome-emails.test.js (2)
ghost/core/test/integration/jobs/process-outbox.test.js (3)
ObjectId(4-4)require(7-7)require(11-11)ghost/core/core/server/services/member-welcome-emails/constants.js (1)
MEMBER_WELCOME_EMAIL_SLUGS(3-6)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (1)
ghost/core/core/server/models/outbox.js (1)
Outbox(10-19)
⏰ 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)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (11)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (3)
525-532: LGTM! Clean stub setup for AutomatedEmail.The stub correctly mimics the AutomatedEmail model's behavior with default active status and lexical content, allowing individual tests to override as needed.
548-548: LGTM! AutomatedEmail dependency correctly wired.All test cases in the outbox integration suite now correctly pass the AutomatedEmail stub to the repository constructor, maintaining consistency with the updated production code.
Also applies to: 574-574, 592-592, 618-618, 638-638, 664-664
648-671: LGTM! Test correctly verifies inactive email behavior.This test properly validates that no outbox entry is created when the welcome email template is inactive, which is a key requirement of this PR. The test structure follows existing patterns and clearly demonstrates the expected behavior.
ghost/core/core/server/services/members/api.js (1)
241-242: LGTM! AutomatedEmail correctly exposed in public API.The addition of AutomatedEmail to the models list follows the existing pattern and enables downstream components to access automated email templates for member welcome emails.
ghost/core/test/integration/services/member-welcome-emails.test.js (2)
22-44: LGTM! Comprehensive test data setup for automated emails.The integration test correctly seeds an active welcome email with properly formatted Lexical content and all required fields. Using
MEMBER_WELCOME_EMAIL_SLUGS.freeensures consistency with the application code.
50-50: LGTM! Proper test cleanup.The cleanup correctly removes the seeded automated email using the same slug constant, ensuring test isolation.
ghost/core/core/server/services/members/members-api/members-api.js (1)
65-66: LGTM! AutomatedEmail properly wired into MemberRepository.The dependency injection follows the established pattern and enables MemberRepository to check if welcome emails are active before creating outbox entries.
Also applies to: 100-100
ghost/core/test/unit/server/services/member-welcome-emails/service.test.js (2)
18-69: LGTM! Comprehensive test setup with well-structured stubs.The test setup properly mocks all service dependencies including logging, configuration, mail sending, rendering, and the AutomatedEmail model. Using rewire for dependency injection is appropriate for unit testing this service.
165-327: LGTM! Thorough test coverage for send method.The test suite comprehensively validates the send method, including error conditions (missing/inactive templates, missing config), correct parameter passing to renderer and mailer, logging behavior, and fallback to default settings. The tests are well-structured and clearly demonstrate expected behavior.
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (2)
13-13: LGTM! Appropriate constant import.Importing
MEMBER_WELCOME_EMAIL_SLUGSfrom the constants file promotes maintainability and ensures consistency across the codebase.
58-58: LGTM! AutomatedEmail dependency properly added.The AutomatedEmail dependency is correctly documented in JSDoc, added to the constructor signature, and stored as an instance variable, following the established pattern for other model dependencies.
Also applies to: 79-80, 99-99
troyciesco
force-pushed
the
NY-773_render-lexical-in-welcome-email
branch
from
558adce to
9f69ada
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/services/member-welcome-emails/service.js (2)
24-42: Consider adding error handling for database queries.The
AutomatedEmail.findOne()calls lack error handling. If a database query fails during initialization, the error will propagate uncaught, potentially leaving the service in an inconsistent state.While this method is called during initialization and failures should be surfaced, consider wrapping individual queries in try-catch blocks to log failures and continue loading other templates, setting failed entries to
null.async loadMemberWelcomeEmails() { for (const [memberStatus, slug] of Object.entries(MEMBER_WELCOME_EMAIL_SLUGS)) { + try { const row = await AutomatedEmail.findOne({slug}); if (!row || !row.get('lexical')) { this.#memberWelcomeEmails[memberStatus] = null; continue; } this.#memberWelcomeEmails[memberStatus] = { lexical: row.get('lexical'), subject: row.get('subject'), status: row.get('status'), senderName: row.get('sender_name'), senderEmail: row.get('sender_email'), senderReplyTo: row.get('sender_reply_to') }; + } catch (err) { + logging.error(`${MEMBER_WELCOME_EMAIL_LOG_KEY} Failed to load welcome email for ${memberStatus}:`, err); + this.#memberWelcomeEmails[memberStatus] = null; + } } }
94-103: Consider using cached data in isMemberWelcomeEmailActive.The
isMemberWelcomeEmailActivemethod queries the database each time, even thoughloadMemberWelcomeEmails()already caches this data in#memberWelcomeEmails. If this method is only called after initialization, consider using the cached data instead:async isMemberWelcomeEmailActive(memberStatus = 'free') { const memberWelcomeEmail = this.#memberWelcomeEmails[memberStatus]; return Boolean(memberWelcomeEmail && memberWelcomeEmail.lexical && memberWelcomeEmail.status === 'active'); }This would eliminate redundant database queries. However, if this method may be called before
loadMemberWelcomeEmails(), the current implementation is safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/constants.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.html.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.txt.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/email-templates/wrapper.hbs(1 hunks)ghost/core/core/server/services/member-welcome-emails/get-mail-config.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/index.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/send-member-welcome-email.js(0 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/services/members/api.js(1 hunks)ghost/core/core/server/services/members/members-api/members-api.js(2 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(6 hunks)ghost/core/core/server/services/outbox/handlers/member-created.js(2 hunks)ghost/core/core/server/services/outbox/jobs/lib/process-entries.js(3 hunks)ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js(3 hunks)ghost/core/test/integration/jobs/process-outbox.test.js(2 hunks)ghost/core/test/integration/services/member-welcome-emails.test.js(2 hunks)ghost/core/test/unit/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.test.js(1 hunks)ghost/core/test/unit/server/services/member-welcome-emails/send-member-welcome-email.test.js(0 hunks)ghost/core/test/unit/server/services/member-welcome-emails/service.test.js(1 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(8 hunks)ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js(1 hunks)ghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.js(1 hunks)ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.js(1 hunks)
💤 Files with no reviewable changes (5)
- ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.html.js
- ghost/core/core/server/services/member-welcome-emails/send-member-welcome-email.js
- ghost/core/core/server/services/member-welcome-emails/get-mail-config.js
- ghost/core/test/unit/server/services/member-welcome-emails/send-member-welcome-email.test.js
- ghost/core/core/server/services/member-welcome-emails/email-templates/welcome.txt.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/unit/server/services/member-welcome-emails/service.test.js
🚧 Files skipped from review as they are similar to previous changes (10)
- ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js
- ghost/core/test/unit/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.test.js
- ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js
- ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js
- ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js
- ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.js
- ghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.js
- ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js
- ghost/core/core/server/services/member-welcome-emails/email-templates/wrapper.hbs
- ghost/core/core/server/services/members/members-api/members-api.js
🧰 Additional context used
🧠 Learnings (15)
📓 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.
📚 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/index.jsghost/core/core/server/services/member-welcome-emails/constants.jsghost/core/core/server/services/outbox/jobs/lib/process-entries.jsghost/core/core/server/services/outbox/handlers/member-created.jsghost/core/test/integration/jobs/process-outbox.test.jsghost/core/core/server/services/member-welcome-emails/service.js
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
ghost/core/core/server/services/member-welcome-emails/constants.js
📚 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/core/server/services/member-welcome-emails/constants.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/core/server/services/outbox/jobs/lib/process-entries.jsghost/core/core/server/services/outbox/handlers/member-created.jsghost/core/core/server/services/members/api.jsghost/core/test/integration/jobs/process-outbox.test.jsghost/core/core/server/services/member-welcome-emails/service.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Run `yarn test:integration` for integration tests in ghost/core
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Each test receives fresh Ghost instance for automatic isolation
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Ensure tests have fresh Ghost instance isolation (automatic) and do not create test dependencies where Test B needs Test A
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-09-02T13:06:50.918Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js:1388-1392
Timestamp: 2025-09-02T13:06:50.918Z
Learning: In Ghost's test files, sinon.restore() in afterEach hooks handles cleanup of all sinon modifications including fake timers, spies, and stubs, so explicit restoration of individual sinon features like clock.restore() is typically not needed when there's a top-level sinon.restore() call.
Applied to files:
ghost/core/test/integration/jobs/process-outbox.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Follow the AAA (Arrange, Act, Assert) pattern in test structure
Applied to files:
ghost/core/test/integration/services/member-welcome-emails.test.js
📚 Learning: 2025-08-11T19:39:00.428Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.
Applied to files:
ghost/core/core/server/services/member-welcome-emails/service.js
📚 Learning: 2025-10-06T14:19:50.385Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 24966
File: ghost/core/test/unit/server/services/mail/GhostMailer.test.js:330-421
Timestamp: 2025-10-06T14:19:50.385Z
Learning: The GhostMailer class in `ghost/core/core/server/services/mail/GhostMailer.js` does not accept external Mailgun-specific parameters (like `o:tag`) from callers. Tags are generated internally via the `getTags()` method. This design keeps the class generic and avoids mixing transport-specific concerns into the public interface.
Applied to files:
ghost/core/core/server/services/member-welcome-emails/service.js
🧬 Code graph analysis (6)
ghost/core/core/server/services/member-welcome-emails/index.js (2)
ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/core/server/services/member-welcome-emails/MemberWelcomeEmailRenderer.js (1)
require(7-7)
ghost/core/core/server/services/member-welcome-emails/constants.js (3)
ghost/core/test/unit/server/services/outbox/jobs/lib/process-outbox.test.js (1)
MESSAGES(21-24)ghost/core/core/server/services/outbox/jobs/lib/constants.js (1)
MESSAGES(6-9)ghost/core/core/server/services/outbox/handlers/member-created.js (1)
memberStatus(8-8)
ghost/core/core/server/services/outbox/jobs/lib/process-entries.js (1)
ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js (4)
processEntries(6-6)processEntries(66-66)entries(10-10)entries(60-60)
ghost/core/core/server/services/outbox/handlers/member-created.js (5)
ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js (3)
memberWelcomeEmailService(7-7)require(4-4)require(5-5)ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/core/server/services/outbox/jobs/lib/process-entries.js (3)
require(2-2)require(3-3)payload(51-51)ghost/core/core/server/services/outbox/index.js (1)
require(6-6)ghost/core/core/server/services/outbox/jobs/lib/constants.js (1)
OUTBOX_LOG_KEY(4-4)
ghost/core/core/server/services/members/api.js (6)
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
models(4-4)ghost/core/test/integration/jobs/process-outbox.test.js (1)
models(6-6)ghost/core/core/boot.js (1)
models(93-93)ghost/core/test/unit/server/models/outbox.test.js (1)
models(2-2)ghost/core/test/legacy/models/model_settings.test.js (1)
models(6-6)ghost/core/test/unit/server/data/exporter/index.test.js (1)
models(7-7)
ghost/core/test/integration/jobs/process-outbox.test.js (1)
ghost/core/core/server/services/member-welcome-emails/constants.js (1)
MEMBER_WELCOME_EMAIL_SLUGS(3-6)
⏰ 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). (16)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Lint
- GitHub Check: E2E Tests (Ember 4/4)
- GitHub Check: [Optional] E2E Tests (React 4/4)
- GitHub Check: [Optional] E2E Tests (React 3/4)
- GitHub Check: [Optional] E2E Tests (React 1/4)
- GitHub Check: [Optional] E2E Tests (React 2/4)
- GitHub Check: E2E Tests (Ember 3/4)
- GitHub Check: E2E Tests (Ember 1/4)
- GitHub Check: E2E Tests (Ember 2/4)
- GitHub Check: Inspect Docker Image
🔇 Additional comments (17)
ghost/core/core/server/services/members/api.js (1)
241-242: LGTM! AutomatedEmail model properly exposed.The addition follows the existing pattern for model exports and aligns with the PR's objective to use the
automated_emailstable.ghost/core/core/server/services/member-welcome-emails/index.js (1)
1-7: LGTM! Clean API surface consolidation.The refactoring from individual function exports to a unified service export improves maintainability while preserving constant exports for compatibility.
ghost/core/test/integration/services/member-welcome-emails.test.js (2)
2-2: LGTM! Appropriate test dependencies added.The ObjectId and MEMBER_WELCOME_EMAIL_SLUGS imports are correctly used in the test setup and cleanup.
Also applies to: 8-8
22-44: LGTM! Proper test data setup and teardown.The automated_emails test data is correctly seeded with a valid lexical structure and properly cleaned up in afterEach, ensuring test isolation.
Also applies to: 50-50
ghost/core/core/server/services/outbox/handlers/member-created.js (2)
2-2: LGTM! Clean service delegation.Removing the
mailConfigparameter and delegating tomemberWelcomeEmailServicesimplifies the handler and aligns with the service-based architecture.Also applies to: 6-6
7-9: LGTM! Hardcoded memberStatus is acceptable for now.The TODO clearly documents that
memberStatusshould be derived from the payload when paid welcome emails are added. This incremental approach is reasonable.ghost/core/test/integration/jobs/process-outbox.test.js (2)
4-4: LGTM! Proper config stubbing with delegation.The config stub correctly returns the test inbox for
memberWelcomeEmailTestInboxwhile delegating other keys to the original method, preventing side effects on unrelated config calls.Also applies to: 10-11, 26-31
33-55: LGTM! Consistent test data setup and cleanup.The automated_emails test data setup is consistent with other test files and properly cleaned up, ensuring test isolation.
Also applies to: 61-61
ghost/core/core/server/services/member-welcome-emails/constants.js (2)
3-6: LGTM! Clear and consistent slug naming.The slug constants follow a clear naming convention and will facilitate consistent references across the codebase.
8-13: LGTM! Well-organized error messages.The MESSAGES object centralizes error messages and appropriately uses a function for the parameterized
memberWelcomeEmailInactivemessage.ghost/core/core/server/services/outbox/jobs/lib/process-entries.js (3)
43-43: LGTM! Consistent mailConfig removal.The removal of
mailConfigfrom function signatures and call sites is consistent and aligns with the service-based architecture where mail configuration is handled internally.Also applies to: 54-54, 79-79, 84-84
45-49: LGTM! Good defensive handler check.Adding validation that a handler exists for the event type prevents runtime errors and provides clear logging when handlers are missing.
59-64: LGTM! Improved error handling and cleanup robustness.The conditional logging based on payload presence provides better debugging context, and the try/catch around
deleteProcessedEntrywith fallback tomarkEntryCompletedensures entries don't remain stuck in PROCESSING state on cleanup failures.Based on learnings, preventing stuck PROCESSING entries is important to avoid duplicate emails.
Also applies to: 68-74
ghost/core/core/server/services/member-welcome-emails/service.js (4)
18-22: LGTM! Proper service initialization.The constructor correctly initializes dependencies including the mailer and renderer. The
emailAddressService.init()call ensures proper service setup.
44-76: LGTM! Proper validation and rendering logic.The method correctly validates that the welcome email exists and is active before rendering, and builds appropriate site settings with sensible defaults.
78-91: Test inbox routing is intentional for staged rollout.The email is currently routed to
config.get('memberWelcomeEmailTestInbox')instead ofmember.email. Based on past review comments, this is intentional for the current PR and will be addressed in follow-up PR #25637.
106-115: LGTM! Appropriate lazy initialization pattern.The wrapper class with lazy
init()and singleton export is a good pattern for service initialization, ensuring the service is only created when needed.
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
Overall looking pretty good, nice work! I think the implementation is pretty solid. Most of my comments are on the unit tests, which generally feel quite closely coupled to the implementation. I'm wondering if we can simplify them or eliminate some of them or change them to more integration-y tests so we can focus more on the behavior that the internal implementation details.
| }); | ||
| } | ||
|
|
||
| const memberName = member.name || 'there'; |
There was a problem hiding this comment.
lol idk why but this made me laugh
There was a problem hiding this comment.
lol same - i'm actually not sure if we want it, because if someone uses {{member.name}} for anything other than the greeting (like "Hi Jon Doe," vs "Hi there,") it'll look weird.
I'm gonna add a ticket to check on this next week
| accentColor: siteSettings.accentColor | ||
| }; | ||
|
|
||
| const contentWithReplacements = this.Handlebars.compile(content)(templateData); |
There was a problem hiding this comment.
interesting 🤔 so if the lexical includes i.e. {{member.name}}, this should substitute it for us?
There was a problem hiding this comment.
yeah! results in something like this:
| await sendMemberWelcomeEmail({payload, mailConfig}); | ||
| async function handle({payload}) { | ||
| // TODO: derive memberStatus from payload when paid welcome emails are added | ||
| const memberStatus = 'free'; |
There was a problem hiding this comment.
Does this mean that we're currently just sending the free welcome email to all members, regardless of status?
There was a problem hiding this comment.
yeah in my head i was thinking that if we don't have the differentiation on the frontend yet, it makes sense that you'd want to send a welcome email to everyone...though we do refer to things as "free" everywhere else 🤔
There was a problem hiding this comment.
Cool, I think that's fine for now
| loggingStub = {info: sinon.stub()}; | ||
| configStub = {get: sinon.stub()}; | ||
| settingsCacheStub = {get: sinon.stub()}; | ||
| urlUtilsStub = { | ||
| urlFor: sinon.stub().returns('https://example.com/') | ||
| }; | ||
| emailAddressServiceStub = {init: sinon.stub()}; | ||
|
|
||
| mailerSendStub = sinon.stub().resolves(); | ||
| mailStub = { | ||
| GhostMailer: sinon.stub().returns({ | ||
| send: mailerSendStub | ||
| }) | ||
| }; | ||
|
|
||
| rendererRenderStub = sinon.stub().resolves({ | ||
| html: '<html>Test</html>', | ||
| text: 'Test', | ||
| subject: 'Welcome!' | ||
| }); | ||
|
|
||
| const MockRenderer = sinon.stub().returns({ | ||
| render: rendererRenderStub | ||
| }); | ||
|
|
||
| automatedEmailStub = { | ||
| findOne: sinon.stub().callsFake(() => { | ||
| const data = { | ||
| lexical: '{"root":{}}', | ||
| subject: 'Welcome to {{site.title}}', | ||
| status: 'active', | ||
| sender_name: 'Test', | ||
| sender_email: '[email protected]', | ||
| sender_reply_to: '[email protected]' | ||
| }; | ||
| return Promise.resolve({ | ||
| get: sinon.stub().callsFake(key => data[key]) | ||
| }); | ||
| }) | ||
| }; | ||
|
|
||
| serviceModule = rewire('../../../../../core/server/services/member-welcome-emails/service'); | ||
| serviceModule.__set__('logging', loggingStub); | ||
| serviceModule.__set__('config', configStub); | ||
| serviceModule.__set__('settingsCache', settingsCacheStub); | ||
| serviceModule.__set__('urlUtils', urlUtilsStub); | ||
| serviceModule.__set__('emailAddressService', emailAddressServiceStub); | ||
| serviceModule.__set__('mail', mailStub); | ||
| serviceModule.__set__('MemberWelcomeEmailRenderer', MockRenderer); | ||
| serviceModule.__set__('AutomatedEmail', automatedEmailStub); |
There was a problem hiding this comment.
note (non-blocking): I don't have a better suggestion, but just wanted to call out that this is a lot of stubs, which then couples these tests quite closely with the implementation. Wondering if we need all of these tests, or if some of these might be better placed as integration tests 🤔
| beforeEach(function () { | ||
| loggingStub = { | ||
| warn: sinon.stub(), | ||
| error: sinon.stub() | ||
| }; | ||
|
|
||
| mockHandler = { | ||
| handle: sinon.stub().resolves(), | ||
| getLogInfo: sinon.stub().returns('[email protected]'), | ||
| LOG_KEY: '[OUTBOX][MEMBER-WELCOME-EMAIL]' | ||
| }; | ||
|
|
||
| knexChain = { | ||
| where: sinon.stub().returnsThis(), | ||
| delete: sinon.stub().resolves(), | ||
| update: sinon.stub().resolves() | ||
| }; | ||
|
|
||
| knexFn = sinon.stub().returns(knexChain); | ||
| knexFn.raw = sinon.stub().returns('CURRENT_TIMESTAMP'); | ||
|
|
||
| dbStub = {knex: knexFn}; | ||
|
|
||
| processEntries = rewire('../../../../../../../core/server/services/outbox/jobs/lib/process-entries'); | ||
| processEntries.__set__('logging', loggingStub); | ||
| processEntries.__set__('OUTBOX_STATUSES', OUTBOX_STATUSES); | ||
| processEntries.__set__('EVENT_HANDLERS', { | ||
| MemberCreatedEvent: mockHandler | ||
| }); | ||
| }); |
There was a problem hiding this comment.
note (non-blocking): similar to above comment, this is a lot of stubs. I wonder if most of these cases are already covered adequately by integration tests (and if not, maybe they could be?)
| beforeEach(function () { | ||
| loggingStub = { | ||
| info: sinon.stub(), | ||
| error: sinon.stub() | ||
| }; | ||
|
|
||
| processEntriesStub = sinon.stub().resolves({processed: 0, failed: 0}); | ||
|
|
||
| memberWelcomeEmailServiceStub = { | ||
| init: sinon.stub(), | ||
| api: { | ||
| loadMemberWelcomeEmails: sinon.stub().resolves() | ||
| } | ||
| }; | ||
|
|
||
| queryStub = { | ||
| where: sinon.stub().returnsThis(), | ||
| whereNull: sinon.stub().returnsThis(), | ||
| orWhere: sinon.stub().returnsThis(), | ||
| orderBy: sinon.stub().returnsThis(), | ||
| limit: sinon.stub().returnsThis(), | ||
| forUpdate: sinon.stub().returnsThis(), | ||
| select: sinon.stub().resolves([]), | ||
| whereIn: sinon.stub().returnsThis(), | ||
| update: sinon.stub().resolves() | ||
| }; | ||
|
|
||
| trxStub = sinon.stub().returns(queryStub); | ||
|
|
||
| dbStub = { | ||
| knex: { | ||
| transaction: sinon.stub().callsFake(async (callback) => { | ||
| await callback(trxStub); | ||
| }), | ||
| raw: sinon.stub().returns('CURRENT_TIMESTAMP') | ||
| } | ||
| }; | ||
|
|
||
| processOutbox = rewire('../../../../../../../core/server/services/outbox/jobs/lib/process-outbox'); | ||
| processOutbox.__set__('logging', loggingStub); | ||
| processOutbox.__set__('db', dbStub); | ||
| processOutbox.__set__('processEntries', processEntriesStub); | ||
| processOutbox.__set__('memberWelcomeEmailService', memberWelcomeEmailServiceStub); | ||
| processOutbox.__set__('OUTBOX_STATUSES', OUTBOX_STATUSES); | ||
| processOutbox.__set__('MESSAGES', MESSAGES); | ||
| processOutbox.__set__('BATCH_SIZE', 100); | ||
| processOutbox.__set__('MAX_ENTRIES_PER_JOB', 1000); | ||
| }); |
There was a problem hiding this comment.
note (non-blocking): same thing here - given that we have the integration tests for this job, I'm wondering how much value these unit tests add, given that we have to mock so much to test it effectively. Seems like these unit tests are really closely coupled with the implementation.
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942223985 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942223985 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942448129 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942448129 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942515478 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942515478 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942515478 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19942515478 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19943466989 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19943466989 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/data-factory/factories/automated-email-factory.ts (1)
21-39: Consider documenting the hardcoded default slug.The build method uses hardcoded defaults (particularly
slug: 'member-welcome-email-free') that appear tailored to the free member welcome email use case. While this simplifies test setup, consider adding a brief comment explaining that these defaults match backend expectations for the free member welcome email flow.Example:
build(options: Partial<AutomatedEmail> = {}): AutomatedEmail { const now = new Date(); + // Defaults configured for free member welcome email testing + // slug 'member-welcome-email-free' matches backend expectations const defaults: AutomatedEmail = { id: generateId(), status: 'active',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/data-factory/factories/automated-email-factory.ts(1 hunks)e2e/data-factory/index.ts(2 hunks)e2e/data-factory/setup.ts(2 hunks)e2e/tests/public/member-signup.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/data-factory/setup.tse2e/data-factory/index.tse2e/data-factory/factories/automated-email-factory.tse2e/tests/public/member-signup.test.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/data-factory/setup.tse2e/data-factory/index.tse2e/data-factory/factories/automated-email-factory.tse2e/tests/public/member-signup.test.ts
e2e/**/*.test.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/**/*.test.ts: Always follow ADRs in../adr/folder (ADR-0001: AAA pattern, ADR-0002: Page Objects)
Never use CSS/XPath selectors - only use semantic locators or data-testid
Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Test names should be lowercase and follow the format 'what is tested - expected outcome'
Each test should represent one scenario only - never mix multiple scenarios in a single test
Follow the AAA (Arrange, Act, Assert) pattern in test structure
Prefer semantic locators (getByRole,getByLabel,getByText) over test IDs and never use CSS selectors, XPath, nth-child, or class names
UsegetByTestId()only when semantic locators are unavailable, and suggest addingdata-testidto Ghost codebase when needed
Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Never use hard-coded waits likewaitForTimeout()
Never usenetworkidlein waits
Use Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Usetest.only()for debugging single tests
Manual login should not be used - authentication is automatic via fixture
Files:
e2e/tests/public/member-signup.test.ts
🧠 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.
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
e2e/data-factory/setup.tse2e/data-factory/index.tse2e/data-factory/factories/automated-email-factory.tse2e/tests/public/member-signup.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Manual login should not be used - authentication is automatic via fixture
Applied to files:
e2e/tests/public/member-signup.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Follow the AAA (Arrange, Act, Assert) pattern in test structure
Applied to files:
e2e/tests/public/member-signup.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Always follow ADRs in `../adr/` folder (ADR-0001: AAA pattern, ADR-0002: Page Objects)
Applied to files:
e2e/tests/public/member-signup.test.ts
🧬 Code graph analysis (3)
e2e/data-factory/setup.ts (2)
e2e/data-factory/index.ts (4)
createAutomatedEmailFactory(27-27)HttpClient(16-16)AutomatedEmailFactory(9-9)GhostAdminApiAdapter(17-17)e2e/data-factory/factories/automated-email-factory.ts (1)
AutomatedEmailFactory(18-59)
e2e/data-factory/factories/automated-email-factory.ts (1)
e2e/data-factory/index.ts (4)
AutomatedEmail(10-10)AutomatedEmailFactory(9-9)Factory(2-2)generateId(21-21)
e2e/tests/public/member-signup.test.ts (2)
e2e/data-factory/index.ts (1)
createAutomatedEmailFactory(27-27)e2e/data-factory/setup.ts (1)
createAutomatedEmailFactory(41-47)
⏰ 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)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (5)
e2e/data-factory/index.ts (1)
9-10: LGTM! Factory exports follow the established pattern.The new AutomatedEmail factory exports are consistent with existing factory exports (Post, Tag, Member), maintaining a uniform API surface for test data creation.
Also applies to: 27-27
e2e/data-factory/factories/automated-email-factory.ts (2)
4-16: LGTM! Interface is well-typed.The AutomatedEmail interface clearly defines all required fields with appropriate types, including a proper union type for status.
41-58: LGTM! Lexical structure is valid.The defaultLexicalContent method returns a properly structured Lexical JSON object with valid field values.
e2e/tests/public/member-signup.test.ts (1)
52-53: LGTM! Automated email creation is correctly placed.Creating the automated email at the start of the test (as part of Arrange in AAA pattern) ensures it exists in the database before the signup flow triggers the welcome email send. The await ensures synchronous execution.
e2e/data-factory/setup.ts (1)
41-47: LGTM! Factory setup follows the established pattern.The createAutomatedEmailFactory function correctly mirrors the structure of existing factory setup functions, using GhostAdminApiAdapter with the appropriate 'automated_emails' resource.
| const jobStartISO = new Date(jobStartMs).toISOString().slice(0, 19).replace('T', ' '); | ||
|
|
||
| let mailConfig; | ||
| memberWelcomeEmailService.init(); |
There was a problem hiding this comment.
it feels like there's probably a better place for this so the outbox doesn't have to care at all, but it also keeps us from processing jobs that are going to fail anyway if the email service doesn't work for some reason.
but i think we can probably clean this up as we add more handlers etc, outbox is way less coupled to welcome emails in general now
There was a problem hiding this comment.
Yeah agreed 🙌 this is where I think having some kind of registry for the outbox will come in handy, so it can decide if anything is listening to a particular event type, without having to know anything about the subscriber's implementation
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
Looks good to me! Tests are much easier to reason about now, thanks for that!
Left one note for future refactor opportunity, but nothing blocking 🙌
| this.tokenService = tokenService; | ||
| this._newslettersService = newslettersService; | ||
| this._labsService = labsService; | ||
| this._AutomatedEmail = AutomatedEmail; |
There was a problem hiding this comment.
note: this is fine for now, but I don't think the MemberRepository should "know" about automated emails. Haven't thought deeply about this, but off the cuff here's what I'm thinking:
- Rather than passing the Outbox model and AutomatedEmail model to the MemberRepository, we should pass the Outbox service
- The Outbox service would have an
emit(event_type, payload, transaction)method, which the MemberRepository would always call - The Outbox service would then decide if it should actually publish to the outbox (depending on whether any handlers are registered, i.e. if any automated_emails exist)
- ... profit?
| await sendMemberWelcomeEmail({payload, mailConfig}); | ||
| async function handle({payload}) { | ||
| // TODO: derive memberStatus from payload when paid welcome emails are added | ||
| const memberStatus = 'free'; |
There was a problem hiding this comment.
Cool, I think that's fine for now
| const jobStartISO = new Date(jobStartMs).toISOString().slice(0, 19).replace('T', ' '); | ||
|
|
||
| let mailConfig; | ||
| memberWelcomeEmailService.init(); |
There was a problem hiding this comment.
Yeah agreed 🙌 this is where I think having some kind of registry for the outbox will come in handy, so it can decide if anything is listening to a particular event type, without having to know anything about the subscriber's implementation
ref https://linear.app/ghost/issue/NY-773
automated_emailsinstead of hard-coded