Premium Analytics: normalize Stats time-series reports - #49779
Conversation
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2ee039e to
87426da
Compare
a8bab25 to
5893252
Compare
87426da to
ab39774
Compare
5893252 to
48d0ccd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ab39774 to
92cbcd5
Compare
a39be3f to
bf4d0bb
Compare
This comment has been minimized.
This comment has been minimized.
3182cdc to
34c4485
Compare
a34d9c3 to
4af76c8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
e966077 to
8160906
Compare
|
Addressed Claude review feedback from #4775464752 in Changes:
Verification:
|
3cc3dcd to
65eb9da
Compare
|
@claude review plz |
This comment was marked as outdated.
This comment was marked as outdated.
65eb9da to
53fb0cf
Compare
|
Addressed the email breakdown summary observation in Changes:
I left the time-series Verified:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
53fb0cf to
44f6365
Compare
|
Addressed in Changes:
Verified locally:
|
This comment was marked as outdated.
This comment was marked as outdated.
|
Claude finished @kangzj's task in 1m 22s —— View job Re-review — current head
|
| Prior item | Status |
|---|---|
| TZ-dependent month/year parse | ✅ referenceDate = new Date( 2001, 0, 1 ) (time-series.ts:24) — local day-of-month is 1 everywhere, so the yyyy-MM/yyyy round-trip guards hold |
Implicit first-numeric-field value contract |
✅ comment at time-series.ts:84 |
data[] object-row path + isStatsTimeSeriesPayload untested |
✅ objectRowsTimeSeriesFixture + detection test incl. negative case (time-series.test.ts:39, :134) |
| Email breakdown empty path leaking raw objects | ✅ scalar-only summary (email-breakdown.ts:204-215) with nested-empty countries/countries-info regression (email-breakdown.test.ts:26) |
| Matrix vs fieldless email-link label asymmetry | ✅ documented at email-breakdown.ts:186-188 |
Spot checks this pass
- Leap-year boundaries (
2024-02-29,2024-12-31) and invalid-ISO-week fallbacks (2026-W54,2025-W53) are explicit and correct — theformat()round-trip rejects overflowed weeks/months before they reachstartOf/endOf*. index.tsexports (sanitizeStatsTimeSeriesResponse,Visits,EmailSummary,EmailBreakdown+ the two item types) and theStatsNormalizedItemunion additions intypes.tsline up — no dangling exports.sortEmailBreakdownItemskeepsOtherlast regardless of value (email-breakdown.ts:36-48); the link test (Post URL7 → url 4 → url 2 →Like1 →Other3) confirms the ordering contract.email-summary.tscorrectly emitsdata: []on the empty-postspath while still returning a zeroed summary — consistent with the breakdown empty path.
Still a conscious deferral (non-blocking)
summary.valueasymmetry intime-series.ts— present only on the scalar-dayspath, absent on matrix/object paths. Already flagged for the row-contract follow-up (WOOA7S-1585); leaving it stays consistent with that plan.
Note: the Jest command is blocked in this review sandbox, so the above is static-only — your posted local +
TZ=America/New_Yorkruns cover execution.
·split/stats-time-series-normalizers
Fixes #
Proposed changes
fields/data, objectdata, and scalar/objectdaysshapes.date-fnshelpers for week/month/year interval boundaries and validate ISO week labels before normalizing them.daysmaps, month/year/leap-year boundaries, and invalid ISO week fallback.Related product discussion/links
valuecontract follow-up: WOOA7S-1585.Does this pull request change what data or activity we track or use?
No. This only normalizes existing Stats API response shapes.
Testing instructions
pnpm --dir projects/packages/premium-analytics test --runInBand projects/packages/premium-analytics/packages/data/src/processing/stats.pnpm --dir projects/packages/premium-analytics typecheck.pnpm --dir projects/packages/premium-analytics test --runInBand.date,period, anddaysresponse shape expected by the normalizers.Out of scope
valuefield to existing endpoint normalizers. Consumers should choose the primary row metric in a follow-up adapter/row-contract change, tracked in WOOA7S-1585.