Skip to content

Updated test welcome emails to send in-progress content#25702

Merged
troyciesco merged 1 commit intoNY-788_send-test-welcome-emailsfrom
mod-NY-788_dont-save-on-send
Dec 15, 2025
Merged

Updated test welcome emails to send in-progress content#25702
troyciesco merged 1 commit intoNY-788_send-test-welcome-emailsfrom
mod-NY-788_dont-save-on-send

Conversation

@troyciesco
Copy link
Contributor

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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 11, 2025

Walkthrough

This PR extends the test welcome email workflow across the stack by introducing subject and lexical as explicit parameters. The frontend component now validates these fields before sending test emails, the API endpoint accepts and forwards them, validators enforce their presence, the service method accepts them as direct parameters instead of extracting from stored records, and tests are updated to include these fields in request bodies and validate error scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20-30 minutes

  • Changes span 6 files with consistent patterns of adding two new fields (subject and lexical), but each file serves a different purpose requiring separate contextual understanding
  • Frontend validation logic and error handling flow need careful review for timing and user feedback
  • Service method signature change from reading values from automatedEmail record to accepting them as parameters represents a meaningful architectural shift in that layer
  • Validator rules and test snapshot expectations require verification for correctness
  • Consistency must be verified across the API contract, validation, service layer, and test coverage

Areas requiring extra attention:

  • Validation logic in automated_emails.js validators to ensure subject and lexical are properly validated as non-empty strings
  • Service method signature change in member-welcome-emails/service.js and confirmation that callers are updated correctly
  • Test snapshot updates to ensure they reflect the new request payload structure accurately
  • Frontend error handling in welcome-email-modal to verify the validate function is called before payload construction and errors are properly surfaced

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: updating test welcome emails to send current form content instead of saved content.
Description check ✅ Passed The description is related to the changeset, explaining the rationale for sending in-progress form content instead of saving first.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mod-NY-788_dont-save-on-send

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e3dc97 and a962887.

⛔ Files ignored due to path filters (1)
  • ghost/core/test/e2e-api/admin/__snapshots__/automated-emails.test.js.snap is excluded by !**/*.snap
📒 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 (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 (2 hunks)
  • ghost/core/core/server/services/member-welcome-emails/service.js (1 hunks)
  • ghost/core/test/e2e-api/admin/automated-emails.test.js (4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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 : 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-05-29T10:37:26.369Z
Learnt from: ErisDS
Repo: TryGhost/Ghost PR: 23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.

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 : Test names should be lowercase and follow the format 'what is tested - expected outcome'

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 : 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-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/api/endpoints/automated-emails.js
  • ghost/core/core/server/services/member-welcome-emails/service.js
🧬 Code graph analysis (3)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
ghost/core/core/server/services/member-welcome-emails/constants.js (1)
  • MEMBER_WELCOME_EMAIL_LOG_KEY (1-1)
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)
apps/admin-x-framework/src/api/automated-emails.ts (2)
apps/admin-x-framework/src/utils/api/hooks.ts (1)
  • createMutation (184-215)
ghost/core/test/e2e-api/admin/automated-emails.test.js (7)
  • id (73-73)
  • id (178-178)
  • id (200-200)
  • id (223-223)
  • id (245-245)
  • id (267-267)
  • id (292-292)
🔇 Additional comments (11)
ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)

71-93: LGTM!

The validation logic for the test email endpoint is well-implemented. The checks for subject and lexical follow the same pattern as the email validation, ensuring all required fields are non-empty strings before proceeding.

apps/admin-x-framework/src/api/automated-emails.ts (1)

52-55: LGTM!

The mutation signature correctly extends the payload to include subject and lexical, and the body mapping properly forwards these fields to the API endpoint.

ghost/core/core/server/api/endpoints/automated-emails.js (1)

116-148: LGTM!

The endpoint correctly declares subject and lexical in the data array and forwards them to the service layer. This aligns with the PR objective of sending in-progress content without saving the automated email first.

ghost/core/test/e2e-api/admin/automated-emails.test.js (3)

36-39: LGTM!

Truncating the brute table before each test ensures a clean state and prevents potential rate-limiting issues during test execution. This improves test isolation and reliability.


363-377: LGTM!

The test correctly includes subject and lexical in the request body, ensuring the test email can be sent with in-progress content as intended by the PR.


415-471: LGTM!

The new validation tests comprehensively cover the error scenarios for invalid email format, missing subject, and missing lexical. Each test is correctly structured with appropriate expectations and snapshot matching.

ghost/core/core/server/services/member-welcome-emails/service.js (1)

105-142: LGTM!

The service method correctly accepts subject and lexical as explicit parameters and uses them directly for rendering the test email, achieving the PR objective of sending in-progress content without saving. The comment on line 108 helpfully explains why the automated email record is still retrieved (for permission validation).

apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (4)

62-85: LGTM!

The form setup correctly includes the validate function and defines comprehensive validation rules for subject and lexical. The isEmptyLexical helper provides a robust check for empty content.


156-156: Minor UX improvement.

Reducing the timeout from 2500ms to 2000ms provides slightly faster feedback to users after sending a test email.


215-215: LGTM!

The error hint is correctly displayed after the send button when testEmailError is set, providing clear feedback to users.


131-167: Validation flow correctly prevents sending invalid content.

The implementation correctly validates the form before sending the test email and uses in-progress content (formState.subject and formState.lexical) in the payload, achieving the PR objective. The error handling comprehensively extracts messages from different error types.

The validate() function returns true when the form is valid and false when validation fails, confirming that the conditional logic on line 140 (if (!validate())) correctly aborts the email send operation when validation errors exist.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@troyciesco
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@troyciesco troyciesco merged commit 04cc23f into NY-788_send-test-welcome-emails Dec 15, 2025
38 checks passed
@troyciesco troyciesco deleted the mod-NY-788_dont-save-on-send branch December 15, 2025 14:52
troyciesco added a commit that referenced this pull request Dec 15, 2025
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 added a commit that referenced this pull request Dec 16, 2025
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 added a commit that referenced this pull request Dec 18, 2025
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 added a commit that referenced this pull request Jan 5, 2026
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 added a commit that referenced this pull request Jan 7, 2026
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 added a commit that referenced this pull request Jan 7, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments