Added migration to create default email_design_settings row#26975
Added migration to create default email_design_settings row#26975
email_design_settings row#26975Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change introduces email design settings functionality to the Ghost platform. A new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
github-actions
bot
added
the
migration
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
cmraible
changed the base branch from
main
to
chris-ny-1137-create-email_templates-table-migration
| @@ -0,0 +1,30 @@ | |||
| const ghostBookshelf = require('./base'); | |||
There was a problem hiding this comment.
note: creating the model is required for the fixtures to work
yarn knex-migrator init doesn't run all the migrations in the versions directory, so we have to add any default data like this in the migration (for existing sites) and the fixtures (for new sites). The fixture manager depends on the model, hence including it in this PR.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/models/email-design-setting.js`:
- Around line 1-30: Add the EmailDesignSetting model to the models index by
importing the model and exporting it from the central models registry so
fixtures can load it; specifically, require the exported EmailDesignSetting (the
value produced by ghostBookshelf.model('EmailDesignSetting',
EmailDesignSetting)) in the models index and add it to module.exports (e.g.,
exports.EmailDesignSetting = require(...).EmailDesignSetting or equivalent) so
the symbol EmailDesignSetting is registered alongside the other models.
🪄 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: d55c4294-9340-4132-b6e4-9ae5e8f741ad
📒 Files selected for processing (4)
ghost/core/core/server/data/migrations/versions/6.23/2026-03-26-05-50-35-insert-default-email-design-settings-row.jsghost/core/core/server/data/schema/fixtures/fixtures.jsonghost/core/core/server/models/email-design-setting.jsghost/core/test/unit/server/data/schema/integrity.test.js
| return; | ||
| } | ||
|
|
||
| await knex('email_design_settings').insert({ |
There was a problem hiding this comment.
i'd meant to bring this up yesterday: would it make sense to seed these styles from the first active newsletter (avoiding saying "default" lol) if it exists?
one spot it could get clunky is if there's something like a header image in the newsletter, it might not make sense in the welcome email.
i think seeding like how you have it is probably right but just wanted to bring it up before we ship this piece. A "copy design from newsletter" button would probably be a better way to smooth out the experience for people that do want stuff the same or want to save most of the effort of copying colors etc.
There was a problem hiding this comment.
The risk of this, I think, is if someone already has a welcome email set up, they might be surprised to see its design change. But you could make the argument the other way too.
There was a problem hiding this comment.
Yeah I think I agree that this is probably right as-is, to avoid surprises with the welcome email design changing without any user intervention.
A "copy design from newsletter" button would probably be a better way to smooth out the experience for people that do want stuff the same or want to save most of the effort of copying colors etc.
This is a good idea though, might be something we can fit in before GA
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
A few non-blocking comments.
| if (existing) { | ||
| logging.warn('Default email_design_settings row already exists, skipping'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
question: When would this ever happen? If it did, should we throw an error instead?
There was a problem hiding this comment.
When would this ever happen?
In the normal course of a Ghost site, it's highly unlikely that it would, so honest answer is probably never.
If it did, should we throw an error instead?
Good question - I think not, but I'm happy to be challenged on that. Rationale for not throwing an error: it doesn't hurt anything to no-op here: if the row already exists, nothing really breaks in Ghost.
If we instead throw an error, it would block any subsequent migrations from running, and prevent Ghost from booting. I do think there's a valid fail-fast argument to be made that preventing Ghost from booting if the schema/data is in an unexpected state is the right thing to do, which would be an interesting discussion. Practically speaking though, I'm mostly just following the existing conventions in previous migrations:
rg -U 'if \(existing\w*\).*\n.*\n.*return;' ghost/core/core/server/data/migrations/versions/
ghost/core/core/server/data/migrations/versions/5.40/2023-03-21-19-02-add-self-serve-integration-api-key.js
39: if (existingKey) {
40: logging.warn('Admin API key for "Self-Serve Migration Integration" already exists');
41: return;
ghost/core/core/server/data/migrations/versions/6.14/2026-01-15-12-01-00-add-transistor-integration-api-key.js
37: if (existingKey) {
38: logging.warn('Admin API key already exists for Transistor integration');
39: return;
ghost/core/core/server/data/migrations/versions/5.40/2023-03-21-18-42-add-self-serve-integration-role.js
14: if (existingRole) {
15: logging.warn('"Self-Serve Migration Integration" role already exists, skipping');
16: return;
ghost/core/core/server/data/migrations/versions/6.14/2026-01-15-12-00-00-add-transistor-integration.js
14: if (existing) {
15: logging.warn('Found existing Transistor integration');
16: return;
ghost/core/core/server/data/migrations/versions/5.113/2025-03-07-12-24-00-add-super-editor.js
14: if (existingRole) {
15: logging.warn('"Super Editor" role already exists, skipping');
16: return;
ghost/core/core/server/data/migrations/versions/5.40/2023-03-21-18-52-add-self-serve-integration.js
15: if (existingIntegration) {
16: logging.warn('Integration already exists, skipping');
17: return;
ghost/core/core/server/data/migrations/versions/6.23/2026-03-26-05-50-35-insert-default-email-design-settings-row.js
13: if (existing) {
14: logging.warn('Default email_design_settings row already exists, skipping');
15: return;
ghost/core/core/server/data/migrations/versions/5.3/2022-07-06-09-26-add-ghost-explore-integration-api-key.js
39: if (existingKey) {
40: logging.warn('Admin API key already exists');
41: return;
ghost/core/core/server/data/migrations/versions/5.100/2024-11-06-04-45-15-add-activitypub-integration.js
17: if (existing) {
18: logging.warn('Found existing Ghost ActivityPub integration');
19: return;
ghost/core/core/server/data/migrations/versions/5.3/2022-07-06-07-58-add-ghost-explore-integration-role.js
14: if (existingRole) {
15: logging.warn('"Ghost Explore Integration" role already exists, skipping');
16: return;
ghost/core/core/server/data/migrations/versions/5.3/2022-07-06-09-17-add-ghost-explore-integration.js
15: if (existingIntegration) {
16: logging.warn('Integration already exists, skipping');
17: return;
| return; | ||
| } | ||
|
|
||
| await knex('email_design_settings').insert({ |
There was a problem hiding this comment.
The risk of this, I think, is if someone already has a welcome email set up, they might be surprised to see its design change. But you could make the argument the other way too.
| /** | ||
| * @returns {object} Default values for email design settings columns | ||
| */ |
There was a problem hiding this comment.
nit: This reduces type safety, I believe. TypeScript can infer the value that's being returned here. But if you specify it as object, all it'll know is that it's some object. I think it's worth removing this entire annotation.
| /** | |
| * @returns {object} Default values for email design settings columns | |
| */ |
troyciesco
left a comment
troyciesco
left a comment
There was a problem hiding this comment.
dropping the same "request changes" here as the parent pr just to make sure we bump the migration verison
troyciesco
force-pushed
the
chris-ny-1137-create-email_templates-table-migration
branch
2 times, most recently
from
2bf276e to
b24b3ad
Compare
cmraible
force-pushed
the
chris-ny-1179-insert-default-email-template-row
branch
from
c76a362 to
937a040
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/test/integration/exporter/exporter.test.js (1)
40-44:⚠️ Potential issue | 🔴 CriticalDuplicate entry:
email_design_settingsappears twice in thetablesarray.Line 40 already contains
'email_design_settings', and line 44 adds it again. This duplicate will cause the test to fail becauseObject.keys(exportData.data)returns unique keys while thetablesarray will have an extra entry.Remove the duplicate entry at line 44.
Proposed fix
'email_spam_complaint_events', - 'email_design_settings', 'emails',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/exporter/exporter.test.js` around lines 40 - 44, The tests fail because the tables array contains a duplicate entry 'email_design_settings'; locate the tables array in exporter.test.js (the array named tables) and remove the duplicate 'email_design_settings' entry so each table name appears only once, ensuring Object.keys(exportData.data) matches the tables list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ghost/core/test/integration/exporter/exporter.test.js`:
- Around line 40-44: The tests fail because the tables array contains a
duplicate entry 'email_design_settings'; locate the tables array in
exporter.test.js (the array named tables) and remove the duplicate
'email_design_settings' entry so each table name appears only once, ensuring
Object.keys(exportData.data) matches the tables list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6cc5d07e-d331-47a1-8c47-2ade79f355cb
📒 Files selected for processing (6)
ghost/core/core/server/data/migrations/versions/6.23/2026-03-26-01-56-35-add-email-design-settings-table.jsghost/core/core/server/data/migrations/versions/6.23/2026-03-26-05-50-35-insert-default-email-design-settings-row.jsghost/core/core/server/data/schema/fixtures/fixtures.jsonghost/core/core/server/models/email-design-setting.jsghost/core/test/integration/exporter/exporter.test.jsghost/core/test/unit/server/data/schema/integrity.test.js
✅ Files skipped from review due to trivial changes (3)
- ghost/core/test/unit/server/data/schema/integrity.test.js
- ghost/core/core/server/models/email-design-setting.js
- ghost/core/core/server/data/migrations/versions/6.23/2026-03-26-01-56-35-add-email-design-settings-table.js
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/data/schema/fixtures/fixtures.json
- ghost/core/core/server/data/migrations/versions/6.23/2026-03-26-05-50-35-insert-default-email-design-settings-row.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.24/2026-03-26-15-47-00-insert-default-email-design-settings-row.js`:
- Around line 19-20: The migration's down deletes by slug and can remove
pre-existing rows if up was skipped; change it so the migration only deletes the
exact row it inserted: generate and store the new id in a module-scoped variable
when inserting (e.g., INSERTED_ID), set it only when up actually performs the
insert (use DEFAULT_SLUG and the (new ObjectID()).toHexString() value), and
update down to delete by that stored id (or no-op if INSERTED_ID is undefined)
instead of deleting by slug.
🪄 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: 62c623cc-e3d1-40f1-ac13-1a80e256bf27
📒 Files selected for processing (1)
ghost/core/core/server/data/migrations/versions/6.24/2026-03-26-15-47-00-insert-default-email-design-settings-row.js
...ata/migrations/versions/6.25/2026-03-26-15-47-00-insert-default-email-design-settings-row.js
Show resolved
Hide resolved
ref NY-1137 Stores design settings for email templates, initially for welcome emails
ref NY-1137 Added isIn validations, fixed nullable on button_color/link_color, removed fieldtype from footer_content
cmraible
force-pushed
the
chris-ny-1179-insert-default-email-template-row
branch
from
2e460d5 to
f0ff03d
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/data/migrations/versions/6.25/2026-03-26-15-47-00-insert-default-email-design-settings-row.js (1)
18-35: Prefer DB defaults here to reduce drift risk.These fields duplicate schema defaults. If defaults evolve, migration behavior can diverge from schema/fixture expectations. Consider inserting only required non-default fields (
id,slug,created_at).Suggested simplification
await knex('email_design_settings').insert({ id: (new ObjectID()).toHexString(), slug: DEFAULT_SLUG, - background_color: 'light', - header_background_color: 'transparent', - show_header_title: true, - button_color: 'accent', - button_corners: 'rounded', - button_style: 'fill', - link_color: 'accent', - link_style: 'underline', - body_font_category: 'sans_serif', - title_font_category: 'sans_serif', - title_font_weight: 'bold', - image_corners: 'square', - show_badge: true, created_at: knex.raw('current_timestamp') });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/data/migrations/versions/6.25/2026-03-26-15-47-00-insert-default-email-design-settings-row.js` around lines 18 - 35, The migration currently inserts many columns that duplicate schema defaults; change the insert into the email_design_settings table to only provide the required non-default fields (keep id via (new ObjectID()).toHexString(), slug using DEFAULT_SLUG, and created_at if you need an explicit timestamp—otherwise omit created_at to use the DB default) and remove the columns that mirror schema defaults (background_color, header_background_color, show_header_title, button_color, button_corners, button_style, link_color, link_style, body_font_category, title_font_category, title_font_weight, image_corners, show_badge) so future schema default changes won’t drift from this migration.
🤖 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/data/migrations/versions/6.25/2026-03-26-15-47-00-insert-default-email-design-settings-row.js`:
- Around line 18-35: The migration currently inserts many columns that duplicate
schema defaults; change the insert into the email_design_settings table to only
provide the required non-default fields (keep id via (new
ObjectID()).toHexString(), slug using DEFAULT_SLUG, and created_at if you need
an explicit timestamp—otherwise omit created_at to use the DB default) and
remove the columns that mirror schema defaults (background_color,
header_background_color, show_header_title, button_color, button_corners,
button_style, link_color, link_style, body_font_category, title_font_category,
title_font_weight, image_corners, show_badge) so future schema default changes
won’t drift from this migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77e96f75-13e4-40df-aae3-c701159e646d
📒 Files selected for processing (5)
ghost/core/core/server/data/migrations/versions/6.25/2026-03-26-15-47-00-insert-default-email-design-settings-row.jsghost/core/core/server/data/schema/fixtures/fixtures.jsonghost/core/core/server/models/email-design-setting.jsghost/core/package.jsonghost/core/test/unit/server/data/schema/integrity.test.js
✅ Files skipped from review due to trivial changes (3)
- ghost/core/package.json
- ghost/core/test/unit/server/data/schema/integrity.test.js
- ghost/core/core/server/data/schema/fixtures/fixtures.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/models/email-design-setting.js
3 participants
closes https://linear.app/ghost/issue/NY-1179/insert-default-email-design-settings-row
Summary
Seeds a default row into the
email_design_settingstable. This row will be referenced by all existingautomated_emailsrows when the FK is added in a subsequent migration.Changes
email_design_settingsrow with slugdefault-automated-email, using schema defaults for all design columns. Idempotent — skips if the row already exists.EmailDesignSettingBookshelf model (required by the fixture system)fixtures.jsonso new databases created viaknex-migrator initalso get the default row