Added comprehensive E2E snapshot tests for post analytics CSV export#26969
Added comprehensive E2E snapshot tests for post analytics CSV export#26969
Conversation
Added a dedicated test file with rich fixture data covering all export code paths: email analytics (sends/opens/clicks), multiple newsletters, member feedback, signup/conversion attribution, and all access types. Includes 6 settings variation tests that snapshot different column sets when features are toggled off.
WalkthroughThe changes modify the posts exporter service and add comprehensive end-to-end tests for post analytics CSV export functionality. The exporter service is updated to apply a default sort order of 🚥 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 |
The export query relied on the Post model's default orderDefaultRaw which uses a CASE expression that behaves differently across MySQL and SQLite. Added an explicit default order (published_at DESC, id DESC) to the exporter, and pinned published_at dates on test fixture posts to avoid timing-dependent ordering between runs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/e2e-api/admin/post-analytics-export.test.js (2)
187-190: RedundantafterEach- outer block already covers this.The
afterEachat line 173-175 already runs after every test, including those in nesteddescribeblocks. This innerafterEachis redundant.♻️ Remove redundant afterEach
describe('Settings variations', function () { - afterEach(function () { - mockManager.restore(); - }); - it('Hides email columns when members disabled', async function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/admin/post-analytics-export.test.js` around lines 187 - 190, Remove the redundant inner afterEach inside the describe('Settings variations', function () { block that calls mockManager.restore(); the outer afterEach already restores the mock for all nested tests, so delete the inner afterEach containing mockManager.restore() (leave the describe and its tests intact) to avoid duplicate teardown calls.
48-81: Add defensive check forsecondNewsletter.Line 50 accesses
newsletters.models[1]without verifying at least 2 newsletters exist. If the fixture changes or runs in a different context,secondNewsletterwould be undefined, causing a TypeError at line 77.🛡️ Proposed defensive check
const newsletters = await models.Newsletter.findAll(); const firstNewsletter = newsletters.models[0]; const secondNewsletter = newsletters.models[1]; await models.Newsletter.edit({feedback_enabled: true}, {id: firstNewsletter.id}); // 2. Update existing email fixtures to have rich analytics data // Email[0] is linked to posts[0] (HTML Ipsum), Email[1] is linked to posts[1] (Ghostly Kitchen Sink) const emails = await models.Email.findAll(); if (emails.models.length >= 1) { await models.Email.edit({ email_count: 256, delivered_count: 240, opened_count: 180, track_opens: true, track_clicks: true, feedback_enabled: true, newsletter_id: firstNewsletter.id, status: 'submitted', recipient_filter: 'all' }, {id: emails.models[0].id}); } - if (emails.models.length >= 2) { + if (emails.models.length >= 2 && secondNewsletter) { await models.Email.edit({ email_count: 128, delivered_count: 120, opened_count: 90, track_opens: true, track_clicks: true, feedback_enabled: true, newsletter_id: secondNewsletter.id, status: 'submitted', recipient_filter: 'status:-free' }, {id: emails.models[1].id}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/admin/post-analytics-export.test.js` around lines 48 - 81, The test assumes two newsletters but accesses newsletters.models[1] without checking length; update the setup to defensively verify newsletters.models.length >= 2 before assigning secondNewsletter (or set secondNewsletter = null) and guard any use of secondNewsletter.id in the Email.edit block (models.Email.edit calls) so the second-email update is skipped when a second newsletter is missing; reference newsletters, firstNewsletter, secondNewsletter and the models.Email.edit blocks when making the changes.
🤖 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/test/e2e-api/admin/post-analytics-export.test.js`:
- Around line 187-190: Remove the redundant inner afterEach inside the
describe('Settings variations', function () { block that calls
mockManager.restore(); the outer afterEach already restores the mock for all
nested tests, so delete the inner afterEach containing mockManager.restore()
(leave the describe and its tests intact) to avoid duplicate teardown calls.
- Around line 48-81: The test assumes two newsletters but accesses
newsletters.models[1] without checking length; update the setup to defensively
verify newsletters.models.length >= 2 before assigning secondNewsletter (or set
secondNewsletter = null) and guard any use of secondNewsletter.id in the
Email.edit block (models.Email.edit calls) so the second-email update is skipped
when a second newsletter is missing; reference newsletters, firstNewsletter,
secondNewsletter and the models.Email.edit blocks when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2c9b326-6c7b-48d7-b06c-edb08d99cd24
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/post-analytics-export.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
ghost/core/core/server/services/posts/posts-exporter.jsghost/core/test/e2e-api/admin/post-analytics-export.test.js
| const posts = await this.#models.Post.findPage({ | ||
| filter: filter ?? 'status:published,status:sent', | ||
| order, | ||
| order: order ?? 'published_at DESC, id DESC', |
There was a problem hiding this comment.
Added a fallback ordering so the sort order of the CSV file is deterministic for snapshot tests
|
1 similar comment
|
1 participant
refs https://linear.app/ghost/issue/NY-645/add-post-analytics-export-loaderdata-stream-in-settings
Added a dedicated test file with rich fixture data covering all export code paths: email analytics (sends/opens/clicks), multiple newsletters, member feedback, signup/conversion attribution, and all access types. Includes 6 settings variation tests that snapshot different column sets when features are toggled off.
This change is in preparation for a fairly significant refactor of the post analytics export to use data streaming.