🐛 Improved reliability of email analytics events - #29895
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 48s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 12s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 48s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 10s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 2m 5s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 30s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 21s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 27s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 5s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-13 17:54:54 UTC
d9e5218 to
c5fde44
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## email-analytics-service-wrapper-should-always-have-a-service #29895 +/- ##
===============================================================================================
Coverage ? 75.40%
===============================================================================================
Files ? 1609
Lines ? 142364
Branches ? 17614
===============================================================================================
Hits ? 107355
Misses ? 33965
Partials ? 1044
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
abadf2b to
6c6a1b8
Compare
6c6a1b8 to
3b4fbcf
Compare
| id: new ObjectID().toHexString(), | ||
| name: jobName, | ||
| [updateField]: date.toISOString(), // force to iso string for sqlite | ||
| created_at: date.toISOString(), // force to iso string for sqlite |
There was a problem hiding this comment.
thought: this is so subtle, I wonder if we could catch a missing required field like this at compile time somehow 🤔
There was a problem hiding this comment.
I don't see this as blocking, but I think we could. Knex supports declaration merging via knex/types/tables and CompositeTableType, which lets us define separate row, insert, and update types. We could make created_at required for inserts but optional for updates, causing the original code here to fail type-checking. Ghost already uses this pattern for gift links and member custom fields.
Quick example pasted from Codex:
import type {Knex} from 'knex';
interface JobRow {
id: string;
name: string;
status: 'started' | 'finished' | 'failed' | 'queued';
started_at: Date | string | null;
finished_at: Date | string | null;
created_at: Date | string;
updated_at: Date | string | null;
metadata: string | null;
queue_entry: number | null;
}
type JobInsert =
Pick<JobRow, 'id' | 'name' | 'created_at'>
& Partial<Omit<JobRow, 'id' | 'name' | 'created_at'>>;
declare module 'knex/types/tables' {
interface Tables {
jobs: Knex.CompositeTableType<
JobRow, // what SELECT returns
JobInsert, // what INSERT accepts
Partial<JobRow> // what UPDATE accepts
>;
}
}
There was a problem hiding this comment.
Love this and didn't know about it. Opened #29953 as a draft—not sure about some of the SQLite changes I made there.
3b4fbcf to
baa8610
Compare
baa8610 to
75179f0
Compare
75179f0 to
c8a7645
Compare
c8a7645 to
d372b1d
Compare
ref #29890 (review) Big picture ----------- We sometimes failed to create the job row for the "missing" job. This could cause: - unnecessary extra fetches from Mailgun - events to be dropped entirely, if the site was off for awhile Details ------- There are four jobs for email analytics: 1. opened 2. non-opened 3. scheduled 4. missing Before this change, `setJobTimestamp` failed to upsert the job row. For the first three job types, it turned out that was fine, because they were already created by previous code paths. But not for the "missing" job! Before this change, we'd only create the "missing" job row when: - no missing events were found - there was an error processing the batch Now, we create it at the right time as intended.
d372b1d to
c660d16
Compare
ref #29890 (review)
Big picture
We sometimes failed to create the job row for the "missing" job. This could cause:
Details
There are four jobs for email analytics:
Before this change,
setJobTimestampfailed to upsert the job row. For the first three job types, it turned out that was fine, because they were already created by previous code paths. But not for the "missing" job!Before this change, we'd only create the "missing" job row when:
Now, we create it at the right time as intended.