Require date in new manual expense flow for distance, time, and invoice#94545
Conversation
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@MelvinBot Please merge main. After that replace all |
…dNewManualExpenseFlow Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@thelullabyy Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🤖 Done,
|
| routeError, | ||
| isNewManualExpenseFlowEnabled, | ||
| isReadOnly, | ||
| shouldShowDate: shouldShowSmartScanFields || isDistanceRequest, |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The date-visibility expression shouldShowSmartScanFields || isDistanceRequest is duplicated here. The exact same expression already exists as the canonical shouldShowDate derived flag (with an explanatory comment) in src/components/MoneyRequestConfirmationListFooter/hooks/useFooterDerivedFlags.ts:82. Because this PR explicitly intends to "mirror the footer's date-visibility logic exactly," the two copies must now be kept in sync by hand — if the footer's rule for showing the date ever changes (e.g. adds per-diem/time handling), the validation will silently drift out of sync, which is exactly the bug class this PR is fixing.
Extract the date-visibility rule into a single shared source of truth and consume it in both places, e.g. a small helper:
// src/libs/MoneyRequestConfirmationUtils.ts (or similar shared module)
function shouldShowConfirmationDate(shouldShowSmartScanFields: boolean, isDistanceRequest: boolean): boolean {
return shouldShowSmartScanFields || isDistanceRequest;
}Then use shouldShowConfirmationDate(shouldShowSmartScanFields, isDistanceRequest) here and in useFooterDerivedFlags.ts instead of re-deriving the boolean in each location.
Reviewed at: b343641 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good catch — addressed in ca11f36.
The date-visibility rule is now a single shared source of truth, shouldShowConfirmationDate in MoneyRequestUtils.ts, consumed in both places:
MoneyRequestConfirmationList.tsx:481(validation)useFooterDerivedFlags.ts:83(footer rendering)
If the footer's rule for showing the date changes, both stay in sync automatically. Prettier, typecheck, lint, and React Compiler compliance all pass.
|
🤖 I analyzed the failing checks — there's no code fix needed here. The only failing check is
This is a human-only check that just needs the author to tick the remaining boxes in the PR description — I can't complete that on your behalf, and it doesn't reflect a problem with the code. Every functional check passes: Still running when I checked: Next step: complete the remaining PR Author Checklist items in the description to clear the |
|
@MelvinBot Please check this comment #94545 (comment) |
…alidation in sync Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@puneetlath Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Let me know when we have some vid on this one 👍 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-29.at.02.00.29.movScreen.Recording.2026-06-29.at.02.01.26.mov |
|
@trjExpensify Could you please check the recording? Thx |
|
@MelvinBot You are using the wrong PR description template, please use this one https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md |
|
@MelvinBot can you address this comment? #94545 (comment) |
|
@MelvinBot You are using the wrong PR description template, please use this one https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md |
|
@MelvinBot Please replace all |
|
Done — I've regenerated the PR description using the current The previous body had dropped the template's inline guidance comments (including the The |
|
Done — replaced all |
|
🤖 I looked into the failing checks — there's nothing to fix in the code. The only failing check is The
Once the boxes are checked, the check will re-run and pass. No push from me is needed here. |
|
@puneetlath We can merge the PR now |
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.26-0 🚀
|
|
🤖 Help site review: no changes required. I reviewed the changes in this PR against the help site articles under Why: This PR is an internal validation bug fix. It gates the "date is missing" check on the same
Because no help site content becomes inaccurate as a result of this change, I did not create a docs draft PR. @thelullabyy, no linked help site PR was created since no documentation changes are required. If you believe a specific article should call out that a date is required when creating distance/time/invoice expenses, let me know and I'll draft the update. |
Explanation of Change
In the new manual expense flow, the date is shown as an inline, clearable
DatePicker(DateField) for every expense type that displays it — manual, distance, time, and invoice — whenever the fields aren't read-only. But the confirmation-step validation only blocked creation when the date was missing for the manual request type:Distance and time expenses use different request types (
distance/distance-manual/time), so this guard was skipped and the expense could be created with an empty date (reported for distance, time, and invoice).This change gates the date-missing check on the same condition that renders the inline clearable picker —
shouldShowDate && !isReadOnly— instead of restricting it to theMANUALrequest type. This keeps the validation and the UI in sync: any flow that shows an editable, clearable date now requires it, while read-only/scan flows (where the date is populated server-side) are correctly skipped.shouldShowDate(shouldShowSmartScanFields || isDistanceRequest) andisReadOnlyare passed intouseConfirmationValidationfromMoneyRequestConfirmationList, mirroring the footer's date-visibility logic exactly.Fixed Issues
$ #94244
PROPOSAL: #94244 (comment)
Tests
Offline tests
Same as Tests.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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