Added email_design_settings table migration#26973
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds 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 title
email_templates table migration
| title_font_category: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'sans_serif'}, | ||
| title_font_weight: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'bold'}, |
There was a problem hiding this comment.
note: this is the heading font and weight field, named as "title_*" to match the newsletters table. for newsletters, these options apply to both the post title and the headings; for welcome emails, there is no title, so it only applies to headings.
There was a problem hiding this comment.
yeah good call - came across this on the frontend and it felt kinda odd but better to keep the naming consistent
There was a problem hiding this comment.
Could be worth leaving a comment to this effect.
There was a problem hiding this comment.
Good shout, added an inline comment in the schema and migration files
This comment was marked as outdated.
This comment was marked as outdated.
cmraible
changed the title
email_templates table migrationemail_design_settings table migration
|
@troyciesco you may notice that I've changed the name from |
|
@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
🧹 Nitpick comments (1)
ghost/core/test/integration/exporter/exporter.test.js (1)
42-44: Alphabetical ordering is inconsistent.The table list appears to be alphabetically sorted, but
email_design_settingsshould come beforeemail_spam_complaint_events(since 'd' < 's').Suggested fix
'email_batches', + 'email_design_settings', 'email_recipient_failures', 'email_recipients', '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 42 - 44, The table list in exporter.test.js is out of alphabetical order; move the 'email_design_settings' entry so it appears before 'email_spam_complaint_events' in the array (the entries 'email_spam_complaint_events', 'email_design_settings', 'emails' should be reordered to be alphabetically consistent), ensuring any test expectations that rely on ordering reference the updated array.
🤖 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/schema/schema.js`:
- Around line 1146-1168: The schema defines an email_design_settings table but
there is no corresponding Bookshelf model; create a new Bookshelf model file
named email-design-settings.js that follows the same pattern as existing models
(e.g., automated-email.js, settings.js): export a Bookshelf model with tableName
set to 'email_design_settings', set hasTimestamps or timestamp fields for
created_at/updated_at, include any relevant visibility/serializers/defaults
consistent with other design/settings models, and register/require it where
other models are loaded so the app can access email_design_settings via the
standard model layer.
---
Nitpick comments:
In `@ghost/core/test/integration/exporter/exporter.test.js`:
- Around line 42-44: The table list in exporter.test.js is out of alphabetical
order; move the 'email_design_settings' entry so it appears before
'email_spam_complaint_events' in the array (the entries
'email_spam_complaint_events', 'email_design_settings', 'emails' should be
reordered to be alphabetically consistent), ensuring any test expectations that
rely on ordering reference the updated array.
🪄 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: ba369fc9-ab6c-4ac6-b8ac-3020c02f7683
📒 Files selected for processing (5)
ghost/core/core/server/data/exporter/table-lists.jsghost/core/core/server/data/migrations/versions/6.23/2026-03-26-01-56-35-add-email-design-settings-table.jsghost/core/core/server/data/schema/schema.jsghost/core/test/integration/exporter/exporter.test.jsghost/core/test/unit/server/data/schema/integrity.test.js
| title_font_category: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'sans_serif'}, | ||
| title_font_weight: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'bold'}, |
There was a problem hiding this comment.
yeah good call - came across this on the frontend and it felt kinda odd but better to keep the naming consistent
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
LGTM. Three non-blocking comments.
| module.exports = addTable('email_design_settings', { | ||
| id: {type: 'string', maxlength: 24, nullable: false, primary: true}, | ||
| slug: {type: 'string', maxlength: 191, nullable: false, unique: true}, | ||
| background_color: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'light'}, |
There was a problem hiding this comment.
nit: I think 50 is a reasonable upper limit, but I could see lowering it given that we expect these values to be shorter. Same comment applies to the other color columns in this file. Feel free to ignore.
There was a problem hiding this comment.
yeah I considered this too and generally agree. Ultimately I decided to keep it 1:1 with the newsletters table to avoid any complications in migrating these settings from the newsletters table in the future.
| title_font_category: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'sans_serif'}, | ||
| title_font_weight: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'bold'}, |
There was a problem hiding this comment.
Could be worth leaving a comment to this effect.
| show_header_title: {type: 'boolean', nullable: false, defaultTo: true}, | ||
| footer_content: {type: 'text', maxlength: 1000000000, nullable: true}, | ||
| button_color: {type: 'string', maxlength: 50, nullable: true, defaultTo: 'accent'}, | ||
| button_corners: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'rounded', validations: {isIn: [['square', 'rounded', 'pill']]}}, |
There was a problem hiding this comment.
General Ghost question: why don't we do these validations at the database level (possibly in addition)?
There was a problem hiding this comment.
It's a good question but I don't really have an answer. If I had to speculate, I'd guess that it has something to do with compatibility between knex & mysql & sqlite not being consistent, at least back when migrations were first setup for Ghost.
troyciesco
left a comment
troyciesco
left a comment
There was a problem hiding this comment.
throwing request changes on here real quick bc 6.23 was released so we gotta move the migration!
troyciesco
force-pushed
the
chris-ny-1137-create-email_templates-table-migration
branch
from
8bb74fc to
2bf276e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26973 +/- ##
==========================================
- Coverage 73.21% 73.18% -0.03%
==========================================
Files 1539 1540 +1
Lines 121718 121724 +6
Branches 14733 14711 -22
==========================================
- Hits 89118 89087 -31
- Misses 31568 31627 +59
+ Partials 1032 1010 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
troyciesco
force-pushed
the
chris-ny-1137-create-email_templates-table-migration
branch
from
2bf276e to
b24b3ad
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/data/schema/schema.js (1)
1146-1148: Add an inline note for intentionally omitted newsletter fields.A short comment here would preserve intent and reduce future accidental “parity fixes” for fields intentionally excluded in this phase.
💡 Suggested tweak
+ // NOTE: intentionally excludes `title_alignment` and `post_title_color` for current automated-email scope email_design_settings: { id: {type: 'string', maxlength: 24, nullable: false, primary: true}, slug: {type: 'string', maxlength: 191, nullable: false, unique: true},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/data/schema/schema.js` around lines 1146 - 1148, Add an inline comment near the email_design_settings schema object to state that newsletter-related fields were intentionally omitted in this phase to avoid accidental "parity fixes" later; update the block around the email_design_settings definition (referencing the email_design_settings object and its properties id and slug) with a concise note explaining the omission and linking to any relevant issue or migration plan if available.
🤖 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/schema/schema.js`:
- Around line 1146-1148: Add an inline comment near the email_design_settings
schema object to state that newsletter-related fields were intentionally omitted
in this phase to avoid accidental "parity fixes" later; update the block around
the email_design_settings definition (referencing the email_design_settings
object and its properties id and slug) with a concise note explaining the
omission and linking to any relevant issue or migration plan if available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6189f8e5-1e34-49f4-8184-2a1a69dc135d
📒 Files selected for processing (7)
ghost/admin/package.jsonghost/core/core/server/data/exporter/table-lists.jsghost/core/core/server/data/migrations/versions/6.24/2026-03-26-15-46-51-add-email-design-settings-table.jsghost/core/core/server/data/schema/schema.jsghost/core/package.jsonghost/core/test/integration/exporter/exporter.test.jsghost/core/test/unit/server/data/schema/integrity.test.js
✅ Files skipped from review due to trivial changes (5)
- ghost/core/test/unit/server/data/schema/integrity.test.js
- ghost/core/package.json
- ghost/core/core/server/data/exporter/table-lists.js
- ghost/admin/package.json
- ghost/core/core/server/data/migrations/versions/6.24/2026-03-26-15-46-51-add-email-design-settings-table.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/integration/exporter/exporter.test.js
The title_font_category and title_font_weight fields are named to match the newsletters table; they apply to headings (and post title in newsletters)
|
cmraible
deleted the
chris-ny-1137-create-email_templates-table-migration
branch
3 participants
closes https://linear.app/ghost/issue/NY-1137/create-email-design-settings-table-migration
Summary
Adds a new
email_design_settingstable to store email design/styling settings independently from newsletters. The table columns mirror the design-related columns from thenewsletterstable (background colors, fonts, button styles, image corners, etc.) with identical types, constraints, and defaults.title_alignmentandpost_title_colorare intentionally omitted — this table initially supports automated emails which don't use those columns. They can be added later if/when we extend templates for newsletters.Changes
email_design_settingstable definition inschema.jswithisInvalidations for enum fieldsaddTablemigration for the new tableemail_design_settingsto the backup tables list intable-lists.js