Conversation
|
Important Review skippedDraft detected. 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
|
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23914992609 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
`automated_emails` is now `welcome_email_automations` and `welcome_email_automated_emails`.
|
cmraible
left a comment
There was a problem hiding this comment.
Throwing a bit of a wrench in your plans, sorry! The changes themselves all make sense (though I haven't looked too closely beyond the first commit), but I think we should split up this one migration into a couple more focused migrations — see comment below.
|
|
||
| module.exports = createNonTransactionalMigration( | ||
| async function up(knex) { | ||
| // 1. Create welcome_email_automations table (without FK to emails, to break circular reference) |
There was a problem hiding this comment.
I would probably split this migration into a couple of separate migrations. The main smell for me is that this mixes DDL and DML in a single migration. I wouldn't say this is a hard rule (a few existing migrations do violate this), but unless we have a good reason to mix the two, I think it's worth splitting them up. Beyond just being a bit easier to reason about, MySQL has some quirks where e.g. DML operations like creating tables are committed immediately (regardless of any transactions). The risk there is that parts of the migration could succeed, while others fail, leaving the DB in a partially migrated state.
As written today (though I think some of this will change with the data model changes you mentioned), I'd split this into probably 3 migrations:
- Create both tables
- Backfill from automated_emails, along with any code changes needed for welcome emails to continue to function with these new tables
- Add/update foreign keys and drop automated_emails


closes https://linear.app/ghost/issue/NY-1188
I recommend reviewing this commit-by-commit.
We currently have one table for welcome emails:
automated_emails. Instead, we want a table for automations and a table for all the emails in that automation.This change isn't useful on its own, but will be followed up with the ability to add more than one email to an automation.
This change should have no user impact.