Tested email analytics database queries - #29890
Conversation
We'll do some DI/testing soon.
Dependency injection!
This was written entirely by GPT-5.6 Sol (Medium thinking) with the following prompt: > Write full unit tests for > `ghost/core/core/server/services/email-analytics/lib/queries.ts`. The > test should create a Knex database with in-memory SQLite—similar to > how > `ghost/core/test/unit/server/services/automations/automations-repository.test.ts` > does it—and then test everything against that Knex database. > > Before you go off and write everything, let's test a single method. I > want to make sure we're on the right track! I think `getJobData` is > probably the simplest, so test it. Make sure to test the case where > it's defined and not defined. > > `ghost/core/core/server/data/schema/schema.js:1098-1108` has the > schema for `jobs`, which you may wish to refer to. You may also wish > to refer to the jobs that are set up as part of email analytics. > > This should be a test-only change.
I continued the conversation from the previous commit with the following: > Great work. I love what you've written. Now add unit tests for the > rest of the public methods. Make sure to test edge cases. I did a *very* cursory scan of the results, and asked it to "Verify that the test schema matches the real schema", which it did.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 11s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 9s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 58s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 46s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 23s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 20s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 30s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 4s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-11 20:56:29 UTC
WalkthroughThe email analytics query module now exports a Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/core/server/services/email-analytics/lib/queries.ts (1)
172-180: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet
created_atwhen inserting a missing job.When no job exists, this insert omits
created_at. Thejobsschema inghost/core/test/unit/server/services/email-analytics/queries.test.tsrequires that column. The insert fails, the catch block hides the error, and the timestamp is not saved. Add a test for this fallback path.Proposed fix
await this.#knex('jobs').insert({ id: new ObjectID().toHexString(), name: jobName, [updateField]: date.toISOString(), // force to iso string for sqlite + created_at: new Date(), updated_at: date.toISOString(), // force to iso string for sqlite status: status });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/email-analytics/lib/queries.ts` around lines 172 - 180, Update the fallback insert in the job update method to include the required created_at timestamp, using the existing date value consistently with the other timestamp fields. Add a unit test in the relevant queries test suite covering insertion when the job is missing and verifying created_at is persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ghost/core/core/server/services/email-analytics/lib/queries.ts`:
- Around line 172-180: Update the fallback insert in the job update method to
include the required created_at timestamp, using the existing date value
consistently with the other timestamp fields. Add a unit test in the relevant
queries test suite covering insertion when the job is missing and verifying
created_at is persisted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f0a0a44b-63b2-49f0-b24c-d57b95695b7f
📒 Files selected for processing (8)
ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.jsghost/core/core/server/services/email-analytics/index.tsghost/core/core/server/services/email-analytics/lib/queries.tsghost/core/test/integration/services/email-analytics/automation-email-analytics.test.jsghost/core/test/integration/services/email-service/newsletter-email-event-storage.test.jsghost/core/test/integration/services/mailgun-email-suppression-list.test.jsghost/core/test/unit/server/services/email-analytics/queries.test.tsghost/core/test/utils/fixture-utils.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29890 +/- ##
==========================================
- Coverage 75.46% 75.46% -0.01%
==========================================
Files 1606 1606
Lines 140769 140787 +18
Branches 17466 17460 -6
==========================================
+ Hits 106238 106248 +10
- Misses 33483 33491 +8
Partials 1048 1048
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:
|
|
CodeRabbit caught a legit bug. Fixed in #29895. |
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.
1 participant
no ref
I recommend reviewing this one commit at a time.
This change should have no user impact.
This change:
In addition to automated testing, I also manually tested this. I sent a newsletter, opened it in Gmail, and verified that I saw the open appear in the newsletter's analytics.