Switched from automated emails table to welcome email automation tables#27185
Merged
Switched from automated emails table to welcome email automation tables#27185
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughReplaces the single Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
github-actions
bot
added
the
migration
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
cmraible
requested changes
Apr 7, 2026
Collaborator
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
Nice one! I've tested this out with welcome emails and it's all working flawlessly 👌
One suggestion for the migration, but otherwise all looks good to me.
...ations/versions/6.27/2026-04-06-15-55-20-split-automated-emails-into-welcome-email-tables.js
Outdated
Show resolved
Hide resolved
towards https://linear.app/ghost/issue/NY-1188 ref #27183 ref #27184 Previous patches added new dormant tables and models. This change actually uses them. More specifically, this does a database migration to move `automated_emails` to `welcome_email_automations` and `welcome_email_automated_emails`. Then, it updates all relevant code to use those new tables. The old model is deleted, but the tables are not. (That's forthcoming.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
ghost/core/core/server/api/endpoints/automated-emails.js (1)
104-116: Minor: Redundantwelcome_email_automation_idassignment.Line 106 assigns
emailData.welcome_email_automation_id = automation.id, and line 110 sets it again in the object literal. The second assignment is sufficient; line 106 can be removed.♻️ Suggested cleanup
return models.Base.transaction(async (transacting) => { const automation = await models.WelcomeEmailAutomation.add(automationData, {...frame.options, transacting}); - emailData.welcome_email_automation_id = automation.id; const email = await models.WelcomeEmailAutomatedEmail.add( { ...emailData, welcome_email_automation_id: automation.id, delay_days: 0 }, {...frame.options, transacting} ); return flattenAutomation(automation, email); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/api/endpoints/automated-emails.js` around lines 104 - 116, Remove the redundant assignment to emailData.welcome_email_automation_id inside the transaction: after creating the automation with models.WelcomeEmailAutomation.add, don’t mutate emailData (the line setting emailData.welcome_email_automation_id = automation.id); rely on the explicit property passed into the object literal for models.WelcomeEmailAutomatedEmail.add (which already sets welcome_email_automation_id: automation.id), leaving flattenAutomation(automation, email) unchanged.ghost/core/core/server/models/welcome-email-automation.js (1)
45-51: Consider clarifying the logged field name.The log entry uses
automated_email_id: model.id, butmodel.idis actually theWelcomeEmailAutomationID, not the email ID. This naming might be intentional for backward compatibility with existing log parsing, but could be confusing.If this naming is intentional for compatibility, consider adding a brief comment. Otherwise,
automation_idwould more accurately describe the value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/models/welcome-email-automation.js` around lines 45 - 51, The log field automated_email_id is misleading because it contains the WelcomeEmailAutomation ID (model.id) rather than an email ID; update the logging.info call (the object passed to logging.info in the isEnableTransition block) to either rename the field to automation_id and update any downstream log parsers/consumers to use automation_id, or, if keeping automated_email_id for backward compatibility, add a concise inline comment next to the logging.info call explaining that model.id is the WelcomeEmailAutomation ID and the name is preserved for compatibility (refer to logging.info, automated_email_id, model.id, isEnableTransition and slug to locate the code).ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js (1)
22-30: Add coverage for the “automation exists but has no related email” branch.The handler now has a separate warning/early-return path when
welcomeEmailAutomatedEmailis missing, but this suite still only exercisesfindOne()returningnull.🧪 Suggested test case
+ it('logs warning when automation has no related email content', async function () { + WelcomeEmailAutomationStub.findOne.resolves({ + id: 'automation123', + related: sinon.stub().withArgs('welcomeEmailAutomatedEmail').returns({}) + }); + + await handler.handle({ + payload: { + memberId: 'member123', + uuid: 'uuid-123', + email: 'test@example.com', + name: 'Test Member', + status: 'free' + } + }); + + const warningLog = findByEvent(logCapture.output, 'outbox.member_created.no_automated_email'); + assert.ok(warningLog); + sinon.assert.notCalled(AutomatedEmailRecipientStub.add); + });Also applies to: 104-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js` around lines 22 - 30, Add a test in the member-created suite that simulates WelcomeEmailAutomationStub.findOne resolving to an automation object whose related('welcomeEmailAutomatedEmail') returns undefined (i.e., no related email) to exercise the early-return/warning branch; in the new test stub the automation should have an id but its related method should return undefined for 'welcomeEmailAutomatedEmail', then call the handler (the member-created handler under test) and assert it logs the expected warning and does not proceed with sending an automated email. Also add the analogous case referenced around the other assertions (lines ~104-105) to ensure both branches are covered.ghost/core/test/integration/jobs/process-outbox.test.js (1)
31-31: Please turn this into an end-to-end assertion of the tracking write.
member-created.jscatchesAutomatedEmailRecipient.add()errors, so this setup can still go green even if the newwelcome_email_automated_emailslookup orautomated_email_idinsert is broken. Keep the seeded email id around, assert the matchingautomated_email_recipientsrow, and delete those recipient rows before deleting the automation.Also applies to: 67-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/jobs/process-outbox.test.js` at line 31, Keep the seeded automated email id and replace the mere deletion of welcome_email_automations with an end-to-end assertion that the tracking write created the corresponding automated_email_recipients row: after triggering the job (member-created.js which calls AutomatedEmailRecipient.add()), query the welcome_email_automated_emails table to retrieve the seeded automated_email_id, assert that a row exists in automated_email_recipients with that automated_email_id and the expected member info, then delete the automated_email_recipients rows before finally deleting the welcome_email_automations/welcome_email_automated_emails seed rows; apply the same change for the other setup/teardown block referenced (lines ~67-83).ghost/core/core/server/services/outbox/handlers/member-created.js (1)
49-55: Use a distinct warning event for the new misconfiguration path.Lines 52-55 reuse
outbox.member_created.no_automated_email, so log-based alerts cannot distinguish “no automation for this slug” from “automation exists but has no related email row”. A separatesystem.eventwould make the new warning actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/outbox/handlers/member-created.js` around lines 49 - 55, The existing logging.warn call in the member-created outbox handler reuses system.event 'outbox.member_created.no_automated_email' causing ambiguity; change that event name to a distinct identifier (e.g. 'outbox.member_created.automated_email_missing_row' or similar) so alerts can distinguish the new misconfiguration path where an automation exists but has no related email row; update the logging.warn invocation that uses LOG_KEY and slug (located in member-created.js near the block that checks if (!email || !email.id)) to emit the new event string while preserving the same message and metadata shape.ghost/core/core/server/services/members/members-api/repositories/member-repository.js (1)
67-68: Consider centralizing the welcome-email activation lookup.The
WelcomeEmailAutomationrewrite looks right, but the samefindOne(...withRelated...) + lexical + statuspredicate now exists for free signups, paid upgrades, andmember-welcome-emails/service.js. Pulling that into one helper would reduce the odds of enqueue-time and send-time behavior drifting on the next schema tweak.Also applies to: 87-106, 362-373, 1480-1491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/repositories/member-repository.js` around lines 67 - 68, Multiple places duplicate the WelcomeEmailAutomation lookup (findOne(...withRelated...) plus checking lexical and status) across MemberRepository and member-welcome-emails/service.js; extract that logic into a single exported helper (e.g., a function like getActiveWelcomeEmailAutomation or WelcomeEmailAutomation.findActive) and replace the inline queries in member-repository methods (the blocks around WelcomeEmailAutomation usage at ~67-68, 87-106, 362-373, 1480-1491) and in member-welcome-emails/service.js to call this helper so all callers share the same withRelated, lexical and status predicates and reduce drift on future schema changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ghost/core/core/server/data/migrations/versions/6.27/2026-04-06-15-55-20-split-automated-emails-into-welcome-email-tables.js`:
- Around line 60-65: Add a clear comment inside the async function down()
explaining that this rollback depends on the foreign key being removed by the
separate migration
2026-04-07-15-10-32-update-automated-email-recipients-foreign-key.js (which
drops the FK before this down() runs) and that the FK is restored to point back
to automated_emails; explicitly state that manually running this down() out of
order may fail or require first removing/restoring the FK on
automated_email_recipients to avoid constraint violations so maintainers know
the dependency when reversing migrations.
In `@ghost/core/core/server/services/member-welcome-emails/service.js`:
- Around line 195-197: The existence check for the related welcome email is
using object truthiness on automation.related('welcomeEmailAutomatedEmail'),
which is unreliable; change the logic to first ensure WelcomeEmailAutomation
(automation) exists, then fetch the related model into a variable (e.g., const
email = automation.related('welcomeEmailAutomatedEmail')) and validate by
checking email and email.id (e.g., if (!automation || !email || !email.id) ...)
so automations with no child row are correctly rejected; update any branches
that rely on the prior check to use this new email/id check.
---
Nitpick comments:
In `@ghost/core/core/server/api/endpoints/automated-emails.js`:
- Around line 104-116: Remove the redundant assignment to
emailData.welcome_email_automation_id inside the transaction: after creating the
automation with models.WelcomeEmailAutomation.add, don’t mutate emailData (the
line setting emailData.welcome_email_automation_id = automation.id); rely on the
explicit property passed into the object literal for
models.WelcomeEmailAutomatedEmail.add (which already sets
welcome_email_automation_id: automation.id), leaving
flattenAutomation(automation, email) unchanged.
In `@ghost/core/core/server/models/welcome-email-automation.js`:
- Around line 45-51: The log field automated_email_id is misleading because it
contains the WelcomeEmailAutomation ID (model.id) rather than an email ID;
update the logging.info call (the object passed to logging.info in the
isEnableTransition block) to either rename the field to automation_id and update
any downstream log parsers/consumers to use automation_id, or, if keeping
automated_email_id for backward compatibility, add a concise inline comment next
to the logging.info call explaining that model.id is the WelcomeEmailAutomation
ID and the name is preserved for compatibility (refer to logging.info,
automated_email_id, model.id, isEnableTransition and slug to locate the code).
In
`@ghost/core/core/server/services/members/members-api/repositories/member-repository.js`:
- Around line 67-68: Multiple places duplicate the WelcomeEmailAutomation lookup
(findOne(...withRelated...) plus checking lexical and status) across
MemberRepository and member-welcome-emails/service.js; extract that logic into a
single exported helper (e.g., a function like getActiveWelcomeEmailAutomation or
WelcomeEmailAutomation.findActive) and replace the inline queries in
member-repository methods (the blocks around WelcomeEmailAutomation usage at
~67-68, 87-106, 362-373, 1480-1491) and in member-welcome-emails/service.js to
call this helper so all callers share the same withRelated, lexical and status
predicates and reduce drift on future schema changes.
In `@ghost/core/core/server/services/outbox/handlers/member-created.js`:
- Around line 49-55: The existing logging.warn call in the member-created outbox
handler reuses system.event 'outbox.member_created.no_automated_email' causing
ambiguity; change that event name to a distinct identifier (e.g.
'outbox.member_created.automated_email_missing_row' or similar) so alerts can
distinguish the new misconfiguration path where an automation exists but has no
related email row; update the logging.warn invocation that uses LOG_KEY and slug
(located in member-created.js near the block that checks if (!email ||
!email.id)) to emit the new event string while preserving the same message and
metadata shape.
In `@ghost/core/test/integration/jobs/process-outbox.test.js`:
- Line 31: Keep the seeded automated email id and replace the mere deletion of
welcome_email_automations with an end-to-end assertion that the tracking write
created the corresponding automated_email_recipients row: after triggering the
job (member-created.js which calls AutomatedEmailRecipient.add()), query the
welcome_email_automated_emails table to retrieve the seeded automated_email_id,
assert that a row exists in automated_email_recipients with that
automated_email_id and the expected member info, then delete the
automated_email_recipients rows before finally deleting the
welcome_email_automations/welcome_email_automated_emails seed rows; apply the
same change for the other setup/teardown block referenced (lines ~67-83).
In `@ghost/core/test/unit/server/services/outbox/handlers/member-created.test.js`:
- Around line 22-30: Add a test in the member-created suite that simulates
WelcomeEmailAutomationStub.findOne resolving to an automation object whose
related('welcomeEmailAutomatedEmail') returns undefined (i.e., no related email)
to exercise the early-return/warning branch; in the new test stub the automation
should have an id but its related method should return undefined for
'welcomeEmailAutomatedEmail', then call the handler (the member-created handler
under test) and assert it logs the expected warning and does not proceed with
sending an automated email. Also add the analogous case referenced around the
other assertions (lines ~104-105) to ensure both branches are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 758332e3-18c4-4623-a654-e799407251c0
📒 Files selected for processing (24)
ghost/core/core/server/api/endpoints/automated-emails.jsghost/core/core/server/data/migrations/versions/6.27/2026-04-06-15-55-20-split-automated-emails-into-welcome-email-tables.jsghost/core/core/server/data/migrations/versions/6.27/2026-04-07-15-10-32-update-automated-email-recipients-foreign-key.jsghost/core/core/server/data/schema/schema.jsghost/core/core/server/models/automated-email-recipient.jsghost/core/core/server/models/automated-email.jsghost/core/core/server/models/welcome-email-automated-email.jsghost/core/core/server/models/welcome-email-automation.jsghost/core/core/server/services/member-welcome-emails/service.jsghost/core/core/server/services/members/api.jsghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/repositories/event-repository.jsghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/core/server/services/outbox/handlers/member-created.jsghost/core/test/e2e-api/admin/automated-emails.test.jsghost/core/test/integration/jobs/process-outbox.test.jsghost/core/test/integration/services/member-welcome-emails.test.jsghost/core/test/unit/server/data/schema/integrity.test.jsghost/core/test/unit/server/models/automated-email.test.jsghost/core/test/unit/server/models/welcome-email-automated-email.test.jsghost/core/test/unit/server/models/welcome-email-automation.test.jsghost/core/test/unit/server/services/members/members-api/repositories/event-repository.test.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.jsghost/core/test/unit/server/services/outbox/handlers/member-created.test.js
💤 Files with no reviewable changes (2)
- ghost/core/test/unit/server/models/automated-email.test.js
- ghost/core/core/server/models/automated-email.js
...ations/versions/6.27/2026-04-06-15-55-20-split-automated-emails-into-welcome-email-tables.js
Show resolved
Hide resolved
ghost/core/core/server/services/member-welcome-emails/service.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
107-112: Consider checking the relation's ID for consistency.The
.related()returns a model object even when the related row is absent. The code works correctly because!email.get('lexical')catches empty models, but checking the ID would be more explicit and consistent with the pattern used at line 197.♻️ Optional: More explicit empty-relation check
const email = row.related('welcomeEmailAutomatedEmail'); - if (!email || !email.get('lexical')) { + if (!email?.id || !email.get('lexical')) { this.#memberWelcomeEmails[memberStatus] = null; continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/member-welcome-emails/service.js` around lines 107 - 112, The current check uses `!email` which is ineffective for Bookshelf relations because `.related()` returns a model object even when missing; update the condition to explicitly check the related model's id (e.g. `!email.id` or `!email.get('id')`) along with the lexical check so the block that sets `this.#memberWelcomeEmails[memberStatus] = null` and continues runs when the relation is absent or has no lexical content; locate the code around the `const email = row.related('welcomeEmailAutomatedEmail')` line in the member-welcome-emails service and replace the `if (!email || !email.get('lexical'))` check accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/member-welcome-emails/service.js`:
- Around line 107-112: The current check uses `!email` which is ineffective for
Bookshelf relations because `.related()` returns a model object even when
missing; update the condition to explicitly check the related model's id (e.g.
`!email.id` or `!email.get('id')`) along with the lexical check so the block
that sets `this.#memberWelcomeEmails[memberStatus] = null` and continues runs
when the relation is absent or has no lexical content; locate the code around
the `const email = row.related('welcomeEmailAutomatedEmail')` line in the
member-welcome-emails service and replace the `if (!email ||
!email.get('lexical'))` check accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a54b62b9-7e25-4f9a-9d3b-38028eb00495
📒 Files selected for processing (1)
ghost/core/core/server/services/member-welcome-emails/service.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/member-welcome-emails/service.js (1)
213-215: Use the already-definedautomatedEmailvariable in the existence check.The
automatedEmailvariable is defined on line 213 but the check on line 215 calls.related()again. This is redundant since the variable is already used later (line 239).♻️ Suggested simplification
const automatedEmail = automation?.related('welcomeEmailAutomatedEmail'); - if (!automation || !automation.related('welcomeEmailAutomatedEmail')?.id) { + if (!automation || !automatedEmail?.id) { throw new errors.NotFoundError({ message: MESSAGES.NO_MEMBER_WELCOME_EMAIL }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/member-welcome-emails/service.js` around lines 213 - 215, The check redundantly calls automation.related('welcomeEmailAutomatedEmail') again instead of using the already-defined automatedEmail variable; update the condition to use automatedEmail (e.g., if (!automation || !automatedEmail?.id)) so the code references the precomputed automatedEmail and avoids the duplicate related() call while preserving the same early-return behavior in the member-welcome-emails service.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/member-welcome-emails/service.js`:
- Around line 213-215: The check redundantly calls
automation.related('welcomeEmailAutomatedEmail') again instead of using the
already-defined automatedEmail variable; update the condition to use
automatedEmail (e.g., if (!automation || !automatedEmail?.id)) so the code
references the precomputed automatedEmail and avoids the duplicate related()
call while preserving the same early-return behavior in the
member-welcome-emails service.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 295e33ca-a55c-4ae8-bb12-b2edc9fb5f11
📒 Files selected for processing (2)
ghost/core/core/server/services/member-welcome-emails/service.jsghost/core/test/integration/services/member-welcome-emails.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/integration/services/member-welcome-emails.test.js
Contributor
Author
|
0d5af31 was a little disruptive here, so I had to make a few changes. |
|
cmraible
approved these changes
Apr 8, 2026
EvanHahn
added a commit
that referenced
this pull request
Apr 8, 2026
closes https://linear.app/ghost/issue/NY-1188 ref #27185 This removes the old automated emails table now that we aren't using it anymore.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
towards https://linear.app/ghost/issue/NY-1188
ref #27183
ref #27184
Previous patches added new dormant tables and models. This change actually uses them.
More specifically, this does a database migration to move
automated_emailstowelcome_email_automationsandwelcome_email_automated_emails. Then, it updates all relevant code to use those new tables.The old model is deleted, but the tables are not. (That's forthcoming.)