Added ability to test member welcome emails #25692
Conversation
github-actions
bot
added
the
migration
WalkthroughAdds a test-send feature for automated member welcome emails. Frontend: a new mutation hook Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 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
force-pushed
the
NY-788_send-test-welcome-emails
branch
from
4446c0b to
a45fed3
Compare
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
119-123: Consider extracting siteSettings construction to reduce duplication.The siteSettings object construction is duplicated between the
send()method (lines 62-66) and thissendTestEmail()method. Consider extracting this into a private helper method.Apply this refactor to reduce duplication:
+ #buildSiteSettings() { + return { + title: settingsCache.get('title') || 'Ghost', + url: urlUtils.urlFor('home', true), + accentColor: settingsCache.get('accent_color') || '#15212A' + }; + } + async send({member, memberStatus}) { // ... - const siteSettings = { - title: settingsCache.get('title') || 'Ghost', - url: urlUtils.urlFor('home', true), - accentColor: settingsCache.get('accent_color') || '#15212A' - }; + const siteSettings = this.#buildSiteSettings(); // ... } async sendTestEmail({email, automatedEmailId}) { // ... - const siteSettings = { - title: settingsCache.get('title') || 'Ghost', - url: urlUtils.urlFor('home', true), - accentColor: settingsCache.get('accent_color') || '#15212A' - }; + const siteSettings = this.#buildSiteSettings(); // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/admin-x-framework/src/api/automated-emails.ts(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(5 hunks)ghost/core/core/server/api/endpoints/automated-emails.js(2 hunks)ghost/core/core/server/data/schema/fixtures/fixtures.json(1 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/service.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: API routes should be defined in `ghost/core/core/server/api/`
Applied to files:
ghost/core/core/server/web/api/endpoints/admin/routes.js
🧬 Code graph analysis (3)
apps/admin-x-framework/src/api/automated-emails.ts (1)
apps/admin-x-framework/src/utils/api/hooks.ts (1)
createMutation(184-215)
ghost/core/core/server/api/endpoints/automated-emails.js (4)
ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
require(3-3)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
require(4-4)ghost/core/test/e2e-api/admin/automated-emails.test.js (1)
require(1-1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
apps/admin-x-framework/src/api/automated-emails.ts (1)
useSendTestWelcomeEmail(52-56)
⏰ 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). (10)
- GitHub Check: ActivityPub tests
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (7)
ghost/core/core/server/data/schema/fixtures/fixtures.json (1)
539-543: LGTM!The new permission entry for sending test welcome emails is properly structured and follows the established pattern used for other permissions in the system.
ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
186-186: LGTM!The new test email route is properly secured with brute-force protection and admin authentication, following the same pattern as the email preview test endpoint (line 343).
apps/admin-x-framework/src/api/automated-emails.ts (1)
52-56: LGTM!The mutation hook is properly typed and follows the established pattern for API mutations in this file.
ghost/core/core/server/api/endpoints/automated-emails.js (3)
4-4: LGTM!The import is necessary for the new sendTestEmail functionality.
142-148: Service initialization is idempotent and safe.The
memberWelcomeEmailService.init()method uses a guard clause (if (this.api) { return; }) to ensure the service is only initialized once. Subsequent calls on every request are harmless and incur negligible overhead.
139-141: Remove this review comment—the factual basis is incorrect.The fixtures.json does not contain a
sendTestEmailpermission forautomated_email. The permissions defined forautomated_emailare:browse,read,edit,add, anddestroy(lines 516–537). ThesendTestEmailaction type only exists foremail_previewresource (line 471), not forautomated_email.The endpoint correctly uses
method: 'edit'which is a valid permission for theautomated_emailresource. There is no inconsistency with email preview endpoints—they use different resources (email_previewvsautomated_email) with different permission models.Likely an incorrect or invalid review comment.
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
2-2: LGTM!The imports and state management for the test email functionality are well-structured.
Also applies to: 11-11, 50-50, 53-54
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Outdated
Show resolved
Hide resolved
ghost/core/core/server/services/member-welcome-emails/service.js
Outdated
Show resolved
Hide resolved
troyciesco
force-pushed
the
NY-788_send-test-welcome-emails
branch
2 times, most recently
from
0677b52 to
56087c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
116-135: Validatelexicalcontent before rendering.The method retrieves
lexicalfrom the automated email but doesn't validate it before passing to the renderer. Iflexicalisnullor empty, the renderer may fail or produce unexpected output. This mirrors the validation done inloadMemberWelcomeEmails(line 28).const lexical = automatedEmail.get('lexical'); const subject = automatedEmail.get('subject'); + +if (!lexical) { + throw new errors.ValidationError({ + message: MESSAGES.INVALID_LEXICAL_STRUCTURE + }); +}
🧹 Nitpick comments (1)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
119-123: Consider extractingsiteSettingsconstruction to a helper.This block duplicates the same logic from lines 62-66 in the
sendmethod. A small private helper like#getSiteSettings()would reduce duplication.+ #getSiteSettings() { + return { + title: settingsCache.get('title') || 'Ghost', + url: urlUtils.urlFor('home', true), + accentColor: settingsCache.get('accent_color') || '#15212A' + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/admin-x-framework/src/api/automated-emails.ts(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(5 hunks)ghost/core/core/server/api/endpoints/automated-emails.js(2 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/service.js
🧬 Code graph analysis (2)
apps/admin-x-framework/src/api/automated-emails.ts (1)
apps/admin-x-framework/src/utils/api/hooks.ts (1)
createMutation(184-215)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
ghost/core/core/server/services/member-welcome-emails/constants.js (2)
MEMBER_WELCOME_EMAIL_LOG_KEY(1-1)MESSAGES(8-13)
⏰ 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). (7)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy 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: Build & Push Docker Image
🔇 Additional comments (3)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
105-144: Well-structured test email implementation.The method follows the existing patterns in the service, with appropriate logging, error handling, and the
[Test]subject prefix to clearly identify test emails. The hardcoded test member name "Jamie Larson" is a reasonable approach for previews.apps/admin-x-framework/src/api/automated-emails.ts (1)
52-56: LGTM!The mutation hook correctly follows the established patterns in this file. Using
unknownfor the response type is appropriate since the endpoint returns 204 No Content, and omitting cache invalidation is correct as test emails don't modify any data.ghost/core/core/server/api/endpoints/automated-emails.js (1)
142-148: Service initialization pattern is correct.The
init()call before accessingapi.sendTestEmail()follows the wrapper pattern used throughout Ghost services, ensuring the service instance is created before use.
troyciesco
force-pushed
the
NY-788_send-test-welcome-emails
branch
from
56087c9 to
18e8dc5
Compare
troyciesco
force-pushed
the
NY-788_send-test-welcome-emails
branch
2 times, most recently
from
1a4edeb to
ac0258d
Compare
troyciesco
force-pushed
the
NY-788_send-test-welcome-emails
branch
from
ac0258d to
5e3dc97
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
105-144: Add a defensive check for missinglexicalbefore rendering test emailsThe new
sendTestEmailflow looks good overall: it reuses the existing renderer/mailer pipeline, logs clearly, and prefixes subjects with[Test]to distinguish these messages.There are two edge cases worth tightening up:
Missing lexical content (duplicate of earlier review concern)
You readconst lexical = automatedEmail.get('lexical');and pass it straight intothis.#renderer.render(...). Iflexicalis null/empty, the renderer may fail or produce confusing output. Adding a simple guard here (and returning a clearValidationError/IncorrectUsageErrormessage) would make failures deterministic and easier to debug.Non–welcome automated emails
AutomatedEmail.findOne({id: automatedEmailId})does not verify that the record’s slug/type matches one ofMEMBER_WELCOME_EMAIL_SLUGS. That means any automated email id could be used with the member‑welcome renderer. If that’s not intended, consider asserting the slug is one of the known welcome‑email slugs and throwing aNotFoundError(or a more specific error) otherwise.Both of these are relatively small guards that would make the endpoint more robust without changing the main behavior.
🧹 Nitpick comments (1)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
4-5: Email validation forsendTestEmailis correctly moved into the custom validator; consider error type consistencyThe
sendTestEmailvalidator doingtypeof email === 'string' && validator.isEmail(email)and throwing on failure is aligned with how other endpoints handle email format checks and matches the earlier review direction to keep this logic in the validator module.One minor point to double‑check: other validations in this file use
ValidationError, while this path usesBadRequestError. If the API layer or clients rely on consistent error classes for input issues, you may want to standardize on one of them here (or confirm thatBadRequestErroris the intended distinction for this endpoint).Also applies to: 16-18, 69-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/automated-emails.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
apps/admin-x-framework/src/api/automated-emails.ts(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(4 hunks)ghost/core/core/server/api/endpoints/automated-emails.js(2 hunks)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js(3 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/automated-emails.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/test/e2e-api/admin/automated-emails.test.js
- apps/admin-x-framework/src/api/automated-emails.ts
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 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/service.jsghost/core/core/server/api/endpoints/automated-emails.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/api/endpoints/automated-emails.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/api/endpoints/automated-emails.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/api/endpoints/automated-emails.js
📚 Learning: 2025-07-09T18:06:09.856Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 24291
File: apps/admin-x-settings/package.json:45-45
Timestamp: 2025-07-09T18:06:09.856Z
Learning: In Ghost admin-x-settings, webhook URL validation uses {require_tld: false} to allow localhost URLs for local integration testing, while social URL validation uses the default {require_tld: true} since social URLs should be public-facing.
Applied to files:
ghost/core/core/server/api/endpoints/automated-emails.js
🧬 Code graph analysis (2)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
ghost/core/core/server/api/endpoints/automated-emails.js (2)
tpl(1-1)messages(6-8)
ghost/core/core/server/api/endpoints/automated-emails.js (3)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
require(5-5)ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
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). (8)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Ghost-CLI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: ActivityPub tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (1)
ghost/core/core/server/api/endpoints/automated-emails.js (1)
4-4: Endpoint wiring forsendTestEmaillooks consistent with existing patternsThe new
sendTestEmailcontroller action is wired cleanly:
- Uses
statusCode: 204with no cache, matching other “fire‑and‑forget” endpoints.- Accepts
options: ['id']anddata: ['email'], withidrequired and email validation delegated to theautomated_emailsinput validator.- Initializes the
memberWelcomeEmailServicewrapper before callingapi.sendTestEmail({ email, automatedEmailId: frame.options.id }), which matches how other services are used in this layer.- Permissions piggy‑back on the
editmethod, in line with the stated intent that editing and sending test welcome emails are coupled for admins.No issues from a controller/API‑layer perspective.
Also applies to: 116-143
troyciesco
force-pushed
the
NY-788_send-test-welcome-emails
branch
from
04cc23f to
a45ccd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
4-20: Test-email validator is solid; consider also enforcing lexical JSON validityThe new
sendTestEmailvalidator cleanly enforces:
- Well-formed email (via
isEmail).- Non-empty
subject.- Non-empty
lexical.That’s a good baseline.
One improvement to keep behavior aligned with
add/editand to avoid renderer-time failures: also ensurelexicalis valid JSON here, so malformed payloads result in a clear 400 error instead of bubbling up as a 500 when the renderer parses it.For example:
if (typeof lexical !== 'string' || !lexical.trim()) { throw new BadRequestError({ message: tpl(messages.lexicalRequired) }); } + + try { + JSON.parse(lexical); + } catch (e) { + throw new BadRequestError({ + message: tpl(messages.invalidLexical) + }); + }This keeps all input-shape validation in one place and makes the test endpoint more robust against bad clients.
Also applies to: 70-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/automated-emails.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
apps/admin-x-framework/src/api/automated-emails.ts(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(4 hunks)ghost/core/core/server/api/endpoints/automated-emails.js(2 hunks)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js(3 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/automated-emails.test.js(3 hunks)
🧰 Additional context used
🧠 Learnings (19)
📚 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: API routes should be defined in `ghost/core/core/server/api/`
Applied to files:
ghost/core/core/server/web/api/endpoints/admin/routes.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/service.jsghost/core/core/server/api/endpoints/automated-emails.jsapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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/e2e-api/admin/automated-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 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/e2e-api/admin/automated-emails.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/e2e-api/admin/automated-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/e2e-api/admin/automated-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/e2e-api/admin/automated-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: Each test receives fresh Ghost instance for automatic isolation
Applied to files:
ghost/core/test/e2e-api/admin/automated-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/e2e-api/admin/automated-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 : Manual login should not be used - authentication is automatic via fixture
Applied to files:
ghost/core/test/e2e-api/admin/automated-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 Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Applied to files:
ghost/core/test/e2e-api/admin/automated-emails.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: Applies to e2e/**/*.{ts,js} : E2E tests should use Playwright with Docker container isolation
Applied to files:
ghost/core/test/e2e-api/admin/automated-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: Always refer to `.claude/E2E_TEST_WRITING_GUIDE.md` for comprehensive testing guidelines and patterns when creating or modifying E2E tests
Applied to files:
ghost/core/test/e2e-api/admin/automated-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/api/endpoints/automated-emails.jsapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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/api/endpoints/automated-emails.jsapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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/api/endpoints/automated-emails.js
📚 Learning: 2025-07-09T18:06:09.856Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 24291
File: apps/admin-x-settings/package.json:45-45
Timestamp: 2025-07-09T18:06:09.856Z
Learning: In Ghost admin-x-settings, webhook URL validation uses {require_tld: false} to allow localhost URLs for local integration testing, while social URL validation uses the default {require_tld: true} since social URLs should be public-facing.
Applied to files:
ghost/core/core/server/api/endpoints/automated-emails.js
📚 Learning: 2025-11-10T11:30:41.316Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25357
File: apps/admin/test-utils/test-helpers.ts:6-12
Timestamp: 2025-11-10T11:30:41.316Z
Learning: In apps/admin/test-utils/test-helpers.ts, the waitForQuerySettled helper is intentionally designed to timeout for idle/disabled queries. It should only treat queries as settled when they reach a terminal state (isSuccess or isError) and are not fetching. This ensures tests properly wait for active queries to complete.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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 : Never use hard-coded waits like `waitForTimeout()`
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
🧬 Code graph analysis (4)
ghost/core/core/server/web/api/endpoints/admin/routes.js (2)
ghost/core/core/server/web/api/endpoints/content/routes.js (3)
router(12-12)mw(5-5)api(3-3)ghost/core/core/server/web/api/endpoints/admin/middleware.js (1)
shared(4-4)
ghost/core/test/e2e-api/admin/automated-emails.test.js (3)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
require(5-5)ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
require(3-3)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (2)
apps/admin-x-framework/src/api/automated-emails.ts (1)
useSendTestWelcomeEmail(52-56)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
validator(4-4)
apps/admin-x-framework/src/api/automated-emails.ts (1)
apps/admin-x-framework/src/utils/api/hooks.ts (1)
createMutation(184-215)
⏰ 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). (10)
- GitHub Check: ActivityPub tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (6)
ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
182-188: New automated email test route wiring looks correctThe
POST /automated_emails/:id/testroute is wired consistently with other email test/preview routes (rate-limited viapreviewEmailLimiterand guarded byauthAdminApi), and correctly delegates toapi.automatedEmails.sendTestEmail. No issues from the routing/security perspective.ghost/core/core/server/services/member-welcome-emails/service.js (1)
105-142: sendTestEmail implementation is consistent with existing welcome-email flowThe
sendTestEmailmethod correctly:
- Verifies the
AutomatedEmailexists (404 otherwise).- Reuses the same
siteSettingspattern as the main send path.- Renders using the caller-supplied
lexicalandsubjectfor a synthetic test member.- Sends to the provided address with a
[Test]subject prefix.This matches the intended behavior for testing unsaved welcome email content without impacting real members.
ghost/core/core/server/api/endpoints/automated-emails.js (1)
4-5: Controller-level wiring for sendTestEmail is soundThe new
sendTestEmailendpoint:
- Uses a 204 response with
cacheInvalidate: false, consistent with other “test/send-only” actions.- Requires
options.idand passes it through asautomatedEmailId.- Delegates all business logic to
memberWelcomeEmailService.api.sendTestEmailafter initialization.- Uses
{method: 'edit'}permissions, aligning with the PR’s intent that edit + sendTestEmail are coupled for member welcome emails.No issues spotted here.
Also applies to: 116-147
ghost/core/test/e2e-api/admin/automated-emails.test.js (1)
3-5: E2E coverage for SendTestEmail is comprehensive and isolation-safe
- Truncating
bruteandautomated_emailsinbeforeEachkeeps the rate-limiter/table state clean between tests.- Stubbing
GhostMailer.prototype.sendavoids real email sends while still exercising the end-to-end flow.- The
SendTestEmailsuite covers:
- 204 success for a valid request.
- 404 for non-existent automated email id.
- 400s for missing email, invalid email, missing subject, and missing lexical.
This gives solid confidence in the new endpoint’s behavior.
Also applies to: 36-39, 320-472
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
2-3: Test-welcome-email UI flow is well-integrated and handles state/cleanup correctly
- Client-side checks (email via
validator.isEmailanduseForm’s subject/lexical validation) prevent most bad requests before hitting the API.sendState+sendStateTimeoutRefgive clear “sending/sent” feedback, and the unmount cleanup (useEffectwithclearTimeout) avoids lingering timers or setState-on-unmounted issues.- Error handling maps JSON API errors (including validation messages from the new endpoint) into concise user-visible copy, falling back to a generic message when needed.
- The dropdown, test email field, and Send button are wired cleanly into the existing modal without interfering with the main save flow.
No changes needed here.
Also applies to: 9-13, 47-58, 62-85, 87-93, 95-109, 116-127, 129-167, 187-216
apps/admin-x-framework/src/api/automated-emails.ts (1)
52-56: API hook matches backend contract for test welcome emails
useSendTestWelcomeEmailcorrectly:
- Targets
/automated_emails/{id}/test/with method POST.- Sends
{email, subject, lexical}in the body, matching the server’s expected payload.- Uses
unknownresponse type, appropriate for a 204/empty response.Hook signature lines up with the modal’s usage and the new backend endpoint.
troyciesco
force-pushed
the
NY-788_send-test-welcome-emails
branch
from
a45ccd5 to
99b3a60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
105-115: Validatelexicalandsubjectinputs before rendering.The method passes
lexicalandsubjectdirectly to the renderer without validation. If either is empty/null, the renderer may fail or produce unexpected output. While the API validator enforcesrequired: true, adding a defensive check here would provide a clearer error message and protect against direct service calls.if (!automatedEmail) { throw new errors.NotFoundError({ message: MESSAGES.NO_MEMBER_WELCOME_EMAIL }); } + + if (!lexical) { + throw new errors.ValidationError({ + message: 'Email content is required to send a test email' + }); + } + + if (!subject) { + throw new errors.ValidationError({ + message: 'Email subject is required to send a test email' + }); + }
🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
129-129: Non-null assertion on potentially null ref is unnecessary.
clearTimeout(null)is safe in JavaScript, so the!assertion is misleading. Consider removing it for clarity.- useEffect(() => () => clearTimeout(sendStateTimeoutRef.current!), []); + useEffect(() => () => { + if (sendStateTimeoutRef.current) { + clearTimeout(sendStateTimeoutRef.current); + } + }, []);Alternatively, since
clearTimeout(null)is safe, you can simply remove the assertion:- useEffect(() => () => clearTimeout(sendStateTimeoutRef.current!), []); + useEffect(() => () => clearTimeout(sendStateTimeoutRef.current ?? undefined), []);ghost/core/core/server/api/endpoints/automated-emails.js (1)
140-140: Initialization on every request is safe but consider lazy initialization.Calling
memberWelcomeEmailService.init()on every request is safe due to the guard inMemberWelcomeEmailServiceWrapper.init(), but if performance becomes a concern, consider initializing the service at server startup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/automated-emails.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
apps/admin-x-framework/src/api/automated-emails.ts(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(4 hunks)ghost/core/core/server/api/endpoints/automated-emails.js(2 hunks)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js(3 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/automated-emails.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js
- ghost/core/test/e2e-api/admin/automated-emails.test.js
- apps/admin-x-framework/src/api/automated-emails.ts
🧰 Additional context used
🧠 Learnings (7)
📚 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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsxghost/core/core/server/api/endpoints/automated-emails.js
📚 Learning: 2025-11-10T11:30:41.316Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25357
File: apps/admin/test-utils/test-helpers.ts:6-12
Timestamp: 2025-11-10T11:30:41.316Z
Learning: In apps/admin/test-utils/test-helpers.ts, the waitForQuerySettled helper is intentionally designed to timeout for idle/disabled queries. It should only treat queries as settled when they reach a terminal state (isSuccess or isError) and are not fetching. This ensures tests properly wait for active queries to complete.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsxghost/core/core/server/api/endpoints/automated-emails.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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsxghost/core/core/server/api/endpoints/automated-emails.jsghost/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 : Never use hard-coded waits like `waitForTimeout()`
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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/api/endpoints/automated-emails.js
📚 Learning: 2025-07-09T18:06:09.856Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 24291
File: apps/admin-x-settings/package.json:45-45
Timestamp: 2025-07-09T18:06:09.856Z
Learning: In Ghost admin-x-settings, webhook URL validation uses {require_tld: false} to allow localhost URLs for local integration testing, while social URL validation uses the default {require_tld: true} since social URLs should be public-facing.
Applied to files:
ghost/core/core/server/api/endpoints/automated-emails.js
⏰ 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). (10)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: ActivityPub tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (6)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
117-142: Implementation follows existing patterns.The method correctly mirrors the structure of the
send()method with appropriate siteSettings construction, renderer usage, and mailer invocation. The[Test]subject prefix clearly distinguishes test emails.apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (4)
2-15: Imports are well-organized.The new imports for
validator,JSONError, anduseSendTestWelcomeEmailare correctly placed and necessary for the test email functionality.
131-167: Test email handler is well-implemented.The function properly:
- Validates email format client-side before sending
- Validates form state using the existing
validate()hook- Manages send state transitions correctly
- Clears existing timeout before setting a new one (line 155)
- Extracts error messages from both
JSONErrorand genericErrortypes
208-215: Button state feedback provides good UX.The button correctly changes color to green on success and shows appropriate labels for each state (
Send,Sending...,Sent). Disabling duringsendingprevents duplicate submissions.
147-153: > Likely an incorrect or invalid review comment.ghost/core/core/server/api/endpoints/automated-emails.js (1)
116-148: Endpoint structure follows Ghost conventions.The
sendTestEmailendpoint correctly:
- Uses 204 status code for success without response body
- Disables cache invalidation
- Requires
idoption- Declares expected data fields
- Reuses
editpermission method as described in PR objectives
ghost/core/core/server/services/member-welcome-emails/service.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
131-167: Well-structured error handling and state management.The validation flow is solid: email format check, form validation, API call, and comprehensive error handling. The timeout management correctly clears any pending timeout before scheduling a new one.
Minor note on Line 155: The non-null assertion
sendStateTimeoutRef.current!is safe here since you're immediately assigning a new timeout, but considerclearTimeout(sendStateTimeoutRef.current ?? undefined)for consistency with the cleanup effect on Line 129.Optional consistency improvement:
- clearTimeout(sendStateTimeoutRef.current!); + clearTimeout(sendStateTimeoutRef.current ?? undefined);ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
71-101: Consider making sendTestEmail async for pattern consistency.The validation logic is solid: email format validation, non-empty checks with trim, and JSON parsing. However,
sendTestEmailis synchronous whileaddandeditare async functions that return Promises.For consistency with the existing validator pattern in this module, consider making
sendTestEmailasync even though it doesn't perform async operations.Apply this diff for consistency:
- sendTestEmail(apiConfig, frame) { + async sendTestEmail(apiConfig, frame) { const email = frame.data.email; const subject = frame.data.subject; const lexical = frame.data.lexical; if (typeof email !== 'string' || !validator.isEmail(email)) { throw new ValidationError({ message: tpl(messages.invalidEmailReceived) }); } if (typeof subject !== 'string' || !subject.trim()) { throw new ValidationError({ message: tpl(messages.subjectRequired) }); } if (typeof lexical !== 'string' || !lexical.trim()) { throw new ValidationError({ message: tpl(messages.lexicalRequired) }); } try { JSON.parse(lexical); } catch (e) { throw new ValidationError({ message: tpl(messages.invalidLexical) }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(4 hunks)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js(3 hunks)ghost/core/core/server/services/member-welcome-emails/constants.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(3 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 (6)
📚 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: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
ghost/core/core/server/services/member-welcome-emails/constants.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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 Learning: 2025-11-10T11:30:41.316Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25357
File: apps/admin/test-utils/test-helpers.ts:6-12
Timestamp: 2025-11-10T11:30:41.316Z
Learning: In apps/admin/test-utils/test-helpers.ts, the waitForQuerySettled helper is intentionally designed to timeout for idle/disabled queries. It should only treat queries as settled when they reach a terminal state (isSuccess or isError) and are not fetching. This ensures tests properly wait for active queries to complete.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsxghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.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 : Never use hard-coded waits like `waitForTimeout()`
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
🧬 Code graph analysis (2)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (2)
apps/admin-x-framework/src/api/automated-emails.ts (1)
useSendTestWelcomeEmail(52-56)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
validator(4-4)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
ghost/core/core/server/api/endpoints/automated-emails.js (2)
tpl(1-1)messages(6-8)
🔇 Additional comments (9)
ghost/core/core/server/services/member-welcome-emails/constants.js (1)
12-13: LGTM!The new validation messages are clear, user-friendly, and follow the existing naming conventions in this file.
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (6)
2-2: LGTM!The new imports are properly utilized:
validatorfor email validation,JSONErrorfor structured error handling, anduseSendTestWelcomeEmailfor the test email mutation.Also applies to: 9-9, 12-12
53-57: LGTM!State variables are well-typed and purposeful. The ref-based timeout management properly addresses the cleanup concern from previous reviews.
62-62: LGTM!Exposing
validatefromuseFormallows the test email handler to programmatically check form validity before sending, which is the correct approach.
129-129: LGTM!The cleanup effect properly prevents memory leaks by clearing the timeout when the component unmounts.
203-205: LGTM!The decision not to clear
testEmailErroron change is intentional to avoid focus loss issues with theTextFieldcomponent, as discussed in previous reviews. The error is cleared when the user attempts to send again (Line 132), which is acceptable UX.
208-215: LGTM!Excellent UX: the button provides clear visual feedback through label, color, and disabled state changes. The error hint is well-positioned to guide users when validation or API errors occur.
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (2)
4-4: LGTM!The
validatorimport is properly utilized for email validation in thesendTestEmailmethod.
16-19: LGTM!The validation messages are clear and follow the existing pattern. The trailing comma on Line 16 improves formatting consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx (3)
29-29: Consider simplifying the cleanup pattern.The
?? undefinedpattern works but could be more idiomatic. Consider using optional chaining or a conditional check for better readability.Apply this diff to use a more conventional pattern:
- useEffect(() => () => clearTimeout(sendStateTimeoutRef.current ?? undefined), []); + useEffect(() => { + return () => { + if (sendStateTimeoutRef.current) { + clearTimeout(sendStateTimeoutRef.current); + } + }; + }, []);
62-63: Avoid non-null assertion when the value can be null.On the first successful send,
sendStateTimeoutRef.currentisnull, making the non-null assertion semantically incorrect. WhileclearTimeout(null)is safe in JavaScript, the assertion is misleading in TypeScript.Apply this diff to safely clear the timeout:
- clearTimeout(sendStateTimeoutRef.current!); - sendStateTimeoutRef.current = setTimeout(() => setSendState('idle'), 2000); + if (sendStateTimeoutRef.current) { + clearTimeout(sendStateTimeoutRef.current); + } + sendStateTimeoutRef.current = setTimeout(() => setSendState('idle'), 2000);
76-98: Consider adding programmatic label association for accessibility.While the visual label and placeholder provide good UX, the label (line 79) is not programmatically connected to the TextField. This is a minor accessibility gap.
Apply this diff to improve accessibility:
<div className='absolute right-0 top-full z-10 mt-2 w-[260px] rounded border border-grey-200 bg-white p-4 shadow-lg'> <div className='mb-3'> - <label className='mb-2 block text-sm font-semibold'>Send test email</label> + <label htmlFor='test-email-input' className='mb-2 block text-sm font-semibold'>Send test email</label> <TextField + id='test-email-input' className='!h-[36px]' placeholder='you@yoursite.com' value={testEmail}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/automated-emails.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(3 hunks)ghost/core/test/e2e-api/admin/automated-emails.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/test/e2e-api/admin/automated-emails.test.js
- apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
⏰ 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). (10)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: ActivityPub tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (3)
apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx (3)
1-13: LGTM!The imports and interface definition are clean and well-structured. The use of
validatorfor email validation and the typed props interface withvalidateFormcallback provide a solid foundation for the component.
31-50: LGTM!The email synchronization effect and validation logic are well-implemented. The email validation using
validator.isEmailand the form validation through thevalidateFormprop provide comprehensive input validation before attempting to send.
64-73: LGTM!The error handling is comprehensive and correctly extracts error messages from both
JSONError(with proper null checks forerror.data.errors[0]) and genericErrorinstances, falling back to a user-friendly default message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx (1)
44-82: LGTM with an optional suggestion for error handling clarity.The function handles validation, API calls, state management, and error handling correctly. The timeout management (lines 68-71) properly clears previous timeouts to avoid multiple pending timeouts.
Consider making the error array checking more explicit for improved code clarity:
- if (error instanceof JSONError && error.data && error.data.errors[0]) { - message = error.data.errors[0].context || error.data.errors[0].message; + if (error instanceof JSONError && error.data?.errors && Array.isArray(error.data.errors) && error.data.errors.length > 0) { + message = error.data.errors[0].context || error.data.errors[0].message;The current code is functionally safe (the truthy check on
errors[0]handles empty/missing arrays), but explicit array validation makes the intent clearer.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx (2)
apps/admin-x-framework/src/api/current-user.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/src/api/automated-emails.ts (1)
useSendTestWelcomeEmail(52-56)
⏰ 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). (10)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Admin-X Settings tests
- GitHub Check: ActivityPub tests
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (6)
apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx (6)
1-6: LGTM! Clean and necessary imports.All imports are utilized appropriately throughout the component.
8-13: LGTM! Well-defined interface.The props interface is clear and appropriately typed. The
validateFormcallback pattern is a good approach for coordinating validation with the parent component.
22-28: LGTM! Proper state management and hooks usage.The state initialization is well-structured:
- Safe optional chaining for initial email state
- Well-typed
sendStatewith literal union type- Appropriate use of
useReffor timeout management
29-35: LGTM! Correct cleanup pattern.The cleanup effect properly prevents memory leaks by clearing the timeout on component unmount.
38-42: LGTM! Handles async user data loading correctly.The effect appropriately updates the test email field when the current user data becomes available.
84-107: LGTM! Well-structured accessible UI.The render implementation is solid:
- Proper label/input association for accessibility (line 87, 90)
- Clear visual feedback for send states via button color and label
- Conditional error display with appropriate styling
- Clean component composition using the design system
no ref Original PR first saves the automated email before sending the test email, but that creates a scenario where you'll _also_ have the changes live, which you might not want. This PR, pointed at that one for comparison, switches the logic around so whatever is currently in the form for subject and lexical is what gets sent. This also happens to make validation and error/loading/success states a lot less jumpy.
troyciesco
force-pushed
the
NY-788_send-test-welcome-emails
branch
from
a94b7c5 to
a16eb7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/e2e-api/admin/automated-emails.test.js (1)
349-361: Consider removing redundant login call.Line 351 calls
loginAsOwner(), but the mainbefore()hook at line 33 already performs this login. Based on learnings, each test receives a fresh Ghost instance, so the redundant login in thisbeforeEachis unnecessary.🔎 Apply this diff to remove the redundant login:
beforeEach(async function () { sinon.stub(mailService.GhostMailer.prototype, 'send').resolves('Mail sent'); - await agent.loginAsOwner(); const automatedEmail = await createAutomatedEmail({ status: 'active', lexical: validLexical }); automatedEmailId = automatedEmail.id; });Based on learnings, each test receives a fresh Ghost instance for automatic isolation.
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
71-101: Consider aligning with existing async validation pattern.The
sendTestEmailvalidator uses synchronousthrowstatements, while the existingaddandeditvalidators useasync/awaitwithPromise.reject. This pattern inconsistency may cause confusion for future maintainers.🔎 Apply this diff to align with the established pattern:
- sendTestEmail(apiConfig, frame) { + async sendTestEmail(apiConfig, frame) { const email = frame.data.email; const subject = frame.data.subject; const lexical = frame.data.lexical; if (typeof email !== 'string' || !validator.isEmail(email)) { - throw new ValidationError({ + return Promise.reject(new ValidationError({ message: tpl(messages.invalidEmailReceived) - }); + })); } if (typeof subject !== 'string' || !subject.trim()) { - throw new ValidationError({ + return Promise.reject(new ValidationError({ message: tpl(messages.subjectRequired) - }); + })); } if (typeof lexical !== 'string' || !lexical.trim()) { - throw new ValidationError({ + return Promise.reject(new ValidationError({ message: tpl(messages.lexicalRequired) - }); + })); } try { JSON.parse(lexical); } catch (e) { - throw new ValidationError({ + return Promise.reject(new ValidationError({ message: tpl(messages.invalidLexical) - }); + })); } + + return Promise.resolve(); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/automated-emails.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
apps/admin-x-framework/src/api/automated-emails.ts(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(3 hunks)ghost/core/core/server/api/endpoints/automated-emails.js(2 hunks)ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js(3 hunks)ghost/core/core/server/services/member-welcome-emails/constants.js(1 hunks)ghost/core/core/server/services/member-welcome-emails/service.js(3 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/automated-emails.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- apps/admin-x-framework/src/api/automated-emails.ts
- ghost/core/core/server/api/endpoints/automated-emails.js
- apps/admin-x-settings/src/components/settings/membership/member-emails/test-email-dropdown.tsx
- ghost/core/core/server/services/member-welcome-emails/constants.js
🧰 Additional context used
🧠 Learnings (18)
📚 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/e2e-api/admin/automated-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 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/e2e-api/admin/automated-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: Each test receives fresh Ghost instance for automatic isolation
Applied to files:
ghost/core/test/e2e-api/admin/automated-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/e2e-api/admin/automated-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/e2e-api/admin/automated-emails.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/e2e-api/admin/automated-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/e2e-api/admin/automated-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 : Manual login should not be used - authentication is automatic via fixture
Applied to files:
ghost/core/test/e2e-api/admin/automated-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 Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Applied to files:
ghost/core/test/e2e-api/admin/automated-emails.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: Applies to e2e/**/*.{ts,js} : E2E tests should use Playwright with Docker container isolation
Applied to files:
ghost/core/test/e2e-api/admin/automated-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: Always refer to `.claude/E2E_TEST_WRITING_GUIDE.md` for comprehensive testing guidelines and patterns when creating or modifying E2E tests
Applied to files:
ghost/core/test/e2e-api/admin/automated-emails.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/service.jsapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsxghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 Learning: 2025-11-10T11:30:41.316Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25357
File: apps/admin/test-utils/test-helpers.ts:6-12
Timestamp: 2025-11-10T11:30:41.316Z
Learning: In apps/admin/test-utils/test-helpers.ts, the waitForQuerySettled helper is intentionally designed to timeout for idle/disabled queries. It should only treat queries as settled when they reach a terminal state (isSuccess or isError) and are not fetching. This ensures tests properly wait for active queries to complete.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 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 : Never use hard-coded waits like `waitForTimeout()`
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
📚 Learning: 2025-10-27T11:59:33.968Z
Learnt from: peterzimon
Repo: TryGhost/Ghost PR: 25261
File: apps/admin/src/layout/app-sidebar/UserMenu.tsx:24-29
Timestamp: 2025-10-27T11:59:33.968Z
Learning: In apps/admin/src/layout/app-sidebar/UserMenu.tsx, the hardcoded placeholder data (avatar URL, initials "US", name "User Name", email "userexample.com") is a known limitation and should not be flagged for replacement with real user data.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
🧬 Code graph analysis (1)
ghost/core/test/e2e-api/admin/automated-emails.test.js (3)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
require(5-5)ghost/core/core/server/services/member-welcome-emails/service.js (2)
require(9-9)require(11-11)ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
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). (10)
- GitHub Check: Ghost-CLI tests
- GitHub Check: ActivityPub tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Admin-X Settings 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: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (10)
ghost/core/test/e2e-api/admin/automated-emails.test.js (3)
3-4: LGTM! Appropriate test dependencies.The imports for
sinonandmailServiceare correctly added to enable stubbing the mail-sending functionality in tests, preventing actual emails from being sent during test execution.
37-37: Good addition for test isolation.Truncating the
brutetable ensures rate-limiting state is reset between tests, which is essential for testing the brute-force protection mentioned in the PR objectives.
320-472: Excellent test coverage for the SendTestEmail feature.The test suite comprehensively covers:
- Happy path (204 response)
- Resource not found (404)
- All validation scenarios (missing/invalid email, missing subject, missing lexical)
The test structure follows best practices with proper setup/teardown, stubbing, and snapshot matching.
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (3)
8-8: Good refactoring to extract test email functionality.Replacing the inline test email UI with the dedicated
TestEmailDropdowncomponent improves code modularity and reusability.
54-54: LGTM! Exposing validate for component integration.Adding
validateto the destructured return allowsTestEmailDropdownto trigger form validation before saving, ensuring a consistent validation flow.
141-141: Clean component integration.The
TestEmailDropdownreceives all necessary props (automatedEmailId,lexical,subject,validateForm) to handle the test email flow independently.ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (2)
4-4: LGTM! Validator import for email validation.The
@tryghost/validatorimport is correctly added to support email format validation in the newsendTestEmailfunction.
16-19: Clear validation messages added.The new error messages (
invalidEmailReceived,subjectRequired,lexicalRequired) provide clear feedback for validation failures in the test email flow.ghost/core/core/server/services/member-welcome-emails/service.js (2)
77-77: LGTM! Using the extracted helper.Replacing inline site settings construction with
this.#getSiteSettings()maintains existing behavior while improving code clarity.
107-148: Well-implemented test email functionality.The
sendTestEmailmethod correctly:
- Validates the automated email exists (for permission/resource checks)
- Performs defensive validation of required fields
- Creates a test member with a fixed name for consistency
- Prefixes the subject with
[Test]to clearly distinguish test emails- Reuses existing rendering and mailing infrastructure
The implementation is clean and follows established patterns in the service.
| #getSiteSettings() { | ||
| return { | ||
| title: settingsCache.get('title') || 'Ghost', | ||
| url: urlUtils.urlFor('home', true), | ||
| accentColor: settingsCache.get('accent_color') || '#15212A' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Excellent refactoring to eliminate duplication.
Extracting site settings retrieval into a private helper method improves maintainability and consistency across the service.
🤖 Prompt for AI Agents
In ghost/core/core/server/services/member-welcome-emails/service.js around lines
24 to 30, the new private helper #getSiteSettings() has been introduced to
centralize site settings retrieval; ensure all previous duplicated site-settings
code paths in this service are replaced to call this helper, keep the method
private, and add/adjust unit tests to cover the helper's output (title, url,
accentColor) so behavior remains identical to the previous implementation.
closes https://linear.app/ghost/issue/NY-788
sendTestEmail- where edit and sendTestEmail can be pretty distinct on posts depending the permissions of the staff user, in the case of member welcome emails the concepts are pretty tightly coupled (and only admins have permission anyway)