Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
This comment was marked as outdated.
This comment was marked as outdated.
towards https://linear.app/ghost/issue/NY-1188 This change should have no user impact. It adds `welcome_email_automations` and `welcome_email_automated_emails` tables. These are split up from the existing `automated_emails` table with two additions in `welcome_email_automated_emails`: - `delay_days` - `next_welcome_email_automated_email_id` You may wish to review a diff between `automated_emails` and the new tables: ```diff diff --git a/j.js b/j.js index dded3c3..68070f4 100644 --- a/j.js +++ b/j.js @@ -1,8 +1,16 @@ -automated_emails: { +welcome_email_automations: { id: {type: 'string', maxlength: 24, nullable: false, primary: true}, status: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'inactive', validations: {isIn: [['active', 'inactive']]}}, name: {type: 'string', maxlength: 191, nullable: false, unique: true}, slug: {type: 'string', maxlength: 191, nullable: false, unique: true}, + created_at: {type: 'dateTime', nullable: false}, + updated_at: {type: 'dateTime', nullable: true} +}, +welcome_email_automated_emails: { + id: {type: 'string', maxlength: 24, nullable: false, primary: true}, + welcome_email_automation_id: {type: 'string', maxlength: 24, nullable: false, references: 'welcome_email_automations.id', constraintName: 'weae_automation_id_foreign', cascadeDelete: true}, + next_welcome_email_automated_email_id: {type: 'string', maxlength: 24, nullable: true, references: 'welcome_email_automated_emails.id', constraintName: 'weae_next_email_id_foreign', cascadeDelete: false}, + delay_days: {type: 'integer', nullable: false}, subject: {type: 'string', maxlength: 300, nullable: false}, lexical: {type: 'text', maxlength: 1000000000, fieldtype: 'long', nullable: true}, sender_name: {type: 'string', maxlength: 191, nullable: true}, @@ -10,9 +18,5 @@ automated_emails: { sender_reply_to: {type: 'string', maxlength: 191, nullable: true, validations: {isEmail: true}}, email_design_setting_id: {type: 'string', maxlength: 24, nullable: false, references: 'email_design_settings.id'}, created_at: {type: 'dateTime', nullable: false}, - updated_at: {type: 'dateTime', nullable: true}, - '@@indexes@@': [ - ['slug'], - ['status'] - ] + updated_at: {type: 'dateTime', nullable: true} }, ```
towards https://linear.app/ghost/issue/NY-1188 ref #27183 We just added two new tables: `welcome_email_automations` and `welcome_email_automated_emails`. This adds dormant models for those, which are based on the existing `AutomatedEmail` model. **You may wish to review a diff of these with their associated `AutomatedEmail` counterpart, which will make these changes look much smaller.** For example, I ran these commands as part of self-review: ```sh vimdiff ghost/core/core/server/models/{automated-email,welcome-email-automation}.js vimdiff ghost/core/core/server/models/{automated-email,welcome-email-automated-email}.js vimdiff ghost/core/test/unit/server/models/{automated-email,welcome-email-automation}.test.js vimdiff ghost/core/test/unit/server/models/{automated-email,welcome-email-automated-email}.test.js ```
This comment was marked as outdated.
This comment was marked as outdated.
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.)
closes https://linear.app/ghost/issue/NY-1188 This removes the old automated emails table now that we aren't using it anymore.
|
| async function up(knex) { | ||
| const exists = await knex.schema.hasTable('automated_emails'); | ||
| if (!exists) { | ||
| logging.warn('Skipping dropping table automated_emails - does not exist'); | ||
| return; | ||
| } | ||
|
|
||
| logging.info('Dropping table: automated_emails'); | ||
| await commands.deleteTable('automated_emails', knex); | ||
| }, |
There was a problem hiding this comment.
suggestion: use the dropTables migration util
cmraible
left a comment
There was a problem hiding this comment.
One minor suggestion on the migration, otherwise LGTM!
|
Moving this to draft until #27185 is merged. I'll regenerate the migration and address the PR review comment. |



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.