Premium Analytics: converge both date pickers onto one range-bounds helper - #52064
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! |
Code Coverage SummaryCoverage changed in 3 files.
|
layoutd
left a comment
There was a problem hiding this comment.
Manually tested and verified following the testing instructions, with the site on UTC-10 (since my browser is on UTC+2) so a browser-zone leak would be clear.
Header picker: all scenarios land to where expected in the site zone, and the comparison window matches the primary. Widget-level date field on the WordAds chart tabs: the three presets and a custom range save as before.
Claude-assisted code review identified one thing worth a line in the description, inline below. Not blocking.
| * verbatim; a calendar edit stages midnight, moved to the *site's* end of | ||
| * day because date-fns' bare `endOfDay` would use the visitor's. | ||
| */ | ||
| to: dateToISOStringWithLocalTZ( |
There was a problem hiding this comment.
This is a bit wider than the rule it replaces: year-surface presets now skip the end-of-day move too. Looks behavior-neutral since those ranges already end at the site's end of day, but it isn't mentioned in the description. Might be worth a sentence?
There was a problem hiding this comment.
Good catch, that's intentional but undocumented. I checked all five year-surface presets and each already ends at the site's end of day, so it's a no-op. Added a sentence and a test.
|
|
||
| expect( span( params.compare_from, params.compare_to ) ).toBe( span( params.from, params.to ) ); | ||
|
|
||
| jest.useRealTimers(); |
There was a problem hiding this comment.
If any expect throws, execution will stop before real timers are reinstated, leaving fake timers installed and leaking into following tests. An afterEach would cover it, or a try/finally
There was a problem hiding this comment.
You're right, thanks. I forced a failure and the four tests after it timed out too. Switched to an afterEach.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VubBCJBkJSDBEGD7F42Tbt
Fixes WOOA7S-2079 (item 1 of 8; the rest follow in a separate PR)
Proposed changes
encodeRangeToSearchParamshelper that owns where a staged date range'stobound lands, and have both date pickers call it.buildRangePatch(the URL picker) keeps a preset or already-normalized range verbatim and moves only a calendar edit's midnighttoto the site's end of day.stageDateRange(a widget's own date field) applied that move unconditionally, while its comment pointed atbuild-range-patchas though it followed the same rule.tosurvives staging.The divergence is currently unreachable.
reportParamsAttributeFieldhas one production consumer,wordads-chart-tabs, whose grain offers onlylast-7-days,last-30-daysandlast-12-months, andpresetIdsfilters the period menu as well as the pills. Every one of those ends at end of day, so the extra move was a no-op.last-24-hours(to: endOfHour( now )) is the only selectable preset it would change, and nothing offers it through this field today. What this removes is a trap in a documented extension point, not a live bug.The shared helper also skips the move for the year surface (
all-timeandyear-YYYY), whichbuildRangePatchdid not. That is behavior-neutral: every year-surface range already ends at the site's end of day, so the move was a no-op there too. A test indate-range.test.tspins it.Worth knowing for the review: the unconditional
endOfDayTZcame from #51419, where the concern was that a bareendOfDayused the visitor's timezone rather than the site's. That fix is preserved. A calendar edit still goes throughendOfDayTZ( ..., reportingTimeZone() ); only presets stop being extended.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
The bug this closes cannot be reached from any shipping surface, so these steps check that the one live consumer did not regress.
toshould still be the site's end of day, not the visitor's.To see the behavior the change actually fixes, drive the field's API directly rather than the UI: staging
last-24-hoursthroughstageDateRangepreviously storedtoat end of day, and the comparison derived from it kept that stretch even afternormalizeReportParamsrecomputed the primary window from the preset.packages/fields/src/report-params-field/__tests__/report-params-field.test.tsxpins the clock and asserts both halves.Testing
Tested on a local site with its timezone set to Europe/Amsterdam (+02:00), serving this branch. The discriminator is
last-24-hoursat 09:59 site time: it is the only selectable preset whosetois not end of day, so a regression in the shared bounds helper would showtoat23:59:59.999+02:00and a ~38h primary window instead of 24h.from 2026-08-10T00:00:00.000+02:00,to 2026-09-08T23:59:59.999+02:00, primary window 720hto 2026-09-08T09:59:59.999+02:00, not stretched. Primary 24.000h, comparison 24.000h, interval coerced tohourto 2026-09-08T23:59:59.999+02:00, extended to the site's end of day rather than the browser's. Primary 192h, comparison 192h