[No QA] Compare whole days when computing the monthly auto-submit ETA - #100239
[No QA] Compare whole days when computing the monthly auto-submit ETA#100239MelvinBot wants to merge 1 commit into
Conversation
Co-authored-by: Rory Abraham <roryabraham@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
🤖 Web test steps verification results: ✅ pass This PR's
Two caveats worth knowing before this merges. First, the banner renders the ordinal day only, so it reads "the 3rd of each month" whether the underlying ETA is September 3 or October 3 — the day shown is right, but the month/year that this PR actually changes is not printed anywhere in the UI. Second, the "no JS console errors" checkbox could only be answered by proxy, as noted above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef53e9b39
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| jest.useFakeTimers(); | ||
| jest.setSystemTime(new Date('2026-01-02T23:59:59.999Z')); | ||
|
|
||
| expect(buildMonthlyOnThe2ndNextStep()).toMatchObject(expectedResultWithEtaDate('2026-01-02')); |
There was a problem hiding this comment.
Exercise a later clock read in the regression test
Because jest.useFakeTimers() and jest.setSystemTime() freeze the clock, the previous implementation's new Date() and the later clock read inside isPast() both observe exactly 23:59:59.999. Consequently, the pre-fix instant-based implementation also returns 2026-01-02, so this test remains green if the regression is restored. Mock sequential clock reads or extract the date calculation to accept an explicit reference time so this test actually fails against the parent implementation.
Useful? React with 👍 / 👎.



Explanation of Change
Follow-up to #100205, which stopped
NextStepUtilsTest › monthly on the 2ndfrom failing every month on the 2nd. That PR made the test deterministic; this PR removes the underlying source of the disagreement so the same class of failure can't come back.Two things were wrong:
1.
buildOptimisticNextStepcompared instants instead of days. For a monthly workspace with a day-of-month offset, the ETA was computed as:setDatepreserves the current time of day, so on the offset day itselfetaDateTimeis this instant.isPastcompares againstDate.now(), read a moment later — so whether the ETA stays today or jumps a month ahead depends on whether a millisecond ticked between the two clock reads. That is real, user-visible non-determinism on the one day of the month it matters most: the day the report actually auto-submits.This PR compares whole days instead, which is what the ETA actually means:
2.
DateUtils.getNextNthOfMonthencoded contradictory semantics and is now dead code. It returns next month when today is the nth (if (day < nth)), the opposite of whatbuildOptimisticNextStepdoes. The test used it to build its expected value, which is exactly why the two disagreed on the 2nd. #100205 removed its only caller, leaving an unused helper whose behaviour contradicts production — a trap for the next person who reaches for it. Removed.A test was added covering the offset day late in the day (
23:59:59.999), which pins the "still today, not next month" contract rather than leaving it to clock luck.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/677843
PROPOSAL:
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
AI tests run by MelvinBot: none.
node_modulesis not installed in this sandbox and dependency installation is blocked here, sonpm run lint,npm run typecheckandnpm testcould not be run. CI on this PR is the first real signal — please confirmtests/unit/NextStepUtilsTest.tspasses before marking ready for review.Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari