fix(dashboard): aggregate order metrics in database - #4994
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
|
Dashboard Preview: https://admin-dashboard-90tt48d08-vendure.vercel.app |
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
Thanks for tackling this — moving the aggregation into a single grouped SQL query is the right direction, and it incidentally fixes a real pre-existing bug where the old widget only summed the first page of orders.items.
However, this cannot merge as-is: as written it returns zero for every order metric on Postgres and MySQL. It only works on SQLite. I verified this by running the exact query through the real drivers, and it also holds up under static review.
Critical: row.date Map-key type mismatch → all-zero metrics on Postgres & MySQL
In metrics.service.ts:
const metricsByDate = new Map(rows.map(row => [row.date, { ... }]));
// ...
const dateKey = currentDate.toISOString().split('T')[0]; // 'YYYY-MM-DD' string
const data = metricsByDate.get(dateKey); // string lookupRawMetricData.date is typed string, but that annotation is a compile-time lie — getRawMany() returns whatever the driver produces for a raw DATE(...) column, and the TypeScript cast doesn't change the runtime value. I tested the PR's exact query against all three supported databases through their actual drivers:
| Database | Driver | row.date returned |
Result |
|---|---|---|---|
| SQLite | better-sqlite3 | '2026-07-15' (string) |
✅ works |
| Postgres | pg | Date object |
❌ every day → 0 |
| MySQL / MariaDB | mysql2 | Date object |
❌ every day → 0 |
Map keys use identity equality, so on Postgres/MySQL the map is keyed by Date objects while the lookup uses a string — every .get() misses, data is always undefined, and every day falls through to ?? 0. The query runs and aggregates correctly; the result is then silently discarded.
Worth noting: the numeric aggregates (orderCount, orderTotal, averageOrderValue) are all correctly wrapped in Number(...) because driver-returned aggregates can be strings — so the risk was clearly understood, it just wasn't applied to date, the one field the whole method hinges on. There's also existing precedent for this exact bug class in custom-field-relation-resolver.service.ts (the "Normalize ids to strings before using them as Map keys… getRawMany() returns driver-native raw values" comment).
Related: timezone off-by-one — pick the fix deliberately
Be careful with the fix. The drivers parse the DB date 2026-07-15 into a Date at local midnight (e.g. 2026-07-14T22:00:00Z in UTC+2). That means the two obvious one-liners disagree:
row.date.toISOString().split('T')[0]→'2026-07-14'(wrong day — UTC read of a local-midnightDate)format(new Date(row.date), 'yyyy-MM-dd')→'2026-07-15'(right day — local read cancels the local parse)
…and whichever is chosen has to agree with how the comparison side (dateKey) is derived. The robust option that sidesteps both bugs is to make SQL return the day as a string so no driver re-parsing happens — a dialect branch on connection.rawConnection.options.type (to_char / DATE_FORMAT / strftime), which we already do in default-scheduler-strategy.ts.
Caching is dead code as wired up
The widget hardcodes refresh: true on both queries, so the if (cachedMetricList && refresh !== true) branch in getMetrics never fires — the cache is written on every request and read never. Either stop forcing refresh from the widget (and trust the cache) or remove the sha1-key/TTL machinery; as-is it misleads the next reader into thinking metrics are cached.
Test coverage can't catch the bug it should
metrics.service.spec.ts mocks getRawMany to return date as a plain string — that's SQLite's behaviour, not Postgres's or MySQL's. The test passes identically whether the date-matching logic is correct or completely broken, because the mock never exercises the string/Date mismatch. Also:
- Both tests use single-day ranges (
startDate === endDate), so the day-by-day gap-filling loop is never exercised with a real gap (e.g. 5 days requested, DB returns rows for 2). - Nothing asserts the
.where/.andWherearguments. For a query whose job is to scope tochannelId+ date range, a regression dropping the channel filter (cross-channel data leak) would sail through untouched.
The single most valuable addition would be a real integration/e2e test against a live DB — the dashboard e2e harness already runs Postgres, which is exactly what would have caught the critical bug above.
Nitpick
"test": "vitest run && vitest --config vitest.plugin.config.mts --run" mixes vitest run and vitest --run styles in one line — pick one.
What's good
- The grouped SQL is correct:
subTotalWithTax + shippingWithTaxmatchesOrder.totalWithTax's own@Calculatedexpression, so the figures will be right once the date bug is fixed. leftJoin→innerJoinis a sound simplification given the immediatechannelIdWHERE.- Splitting out
vitest.plugin.config.mts(node env) for backend plugin code and excluding*.spec.tsfrom the plugin build is the right setup.
Must fix before merge
- Normalise
row.dateto a'YYYY-MM-DD'string on both the map-build and lookup sides, resolving the timezone question deliberately (ideally by formatting the day in SQL). - Resolve the
refresh: truevs. cache contradiction. - Add a test with a multi-day range and partial DB results that asserts the channel/date scoping — ideally a live-DB integration test rather than a mocked one.
Add live database coverage for daily metric aggregation and resolve the cache refresh contradiction.\n\nRelates to #3648
|
2 participants
Description
Moves DashboardPlugin order count, total, and average calculations into a single database-grouped query so metrics include every order without loading or paginating order entities in application memory. Updates the Dashboard orders summary widget to consume the existing metrics query and adds regression coverage for ranges containing more than 1,000 orders. Closes #3648.
Breaking changes
No breaking changes.
Screenshots
Not applicable; this changes the data source and server-side aggregation without altering the widget layout.
Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.