-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[No QA] [Odometer] Removal of backTo from Odometer flow #79339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[No QA] [Odometer] Removal of backTo from Odometer flow #79339
Conversation
|
For now there is some issue during the edition of an already existing expense so I am currently working on figuring that out |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Fixed that. The issue that I am battling now is that when we are creating the expense from quick action then we should have the discardChangesConfirmation turned on but thats an issue since in quick action we skip everything else so the initial input is effectively the last screen so when we press Screen.Recording.2026-01-12.at.13.14.03.mov |
|
@DylanDylann 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] |
|
There is a pretty big number of test steps but they are not that complicated so don't be scared by that 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 146c2acd22
ℹ️ 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".
| report, | ||
| route: { | ||
| params: {action, iouType, reportID, transactionID, backTo, backToReport}, | ||
| params: {action, iouType, reportID, transactionID, backToReport}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need backToReport? From my check, it always be undefined. Should we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it isn't always undefined. There are situations where it is needed. When a money request is launched directly from a report, that report ID is passed as backToReport. The route builders include it as an optional segment, and the submission handler uses it to navigate back to that report instead of defaulting to the active one. Without it, navigation would fall back to the active report and break that “return where you came from” behavior.
Screen.Recording.2026-01-13.at.11.07.45.mov
|
@jakubkalinski0 Thanks for the PR. Instead of adding a new parameter, could we derive this value from the current route? In the creation flow:
|
…aviour_in_Odometer_flow
Now that you mention it I think there is a chance that will work. I will look into it and let you know if it is possible to cover all required cases 🫡 |
| const startOfFlow = routeName === SCREENS.MONEY_REQUEST.DISTANCE_CREATE; | ||
| const isEditing = action === CONST.IOU.ACTION.EDIT; | ||
| const isCreatingNewRequest = !(backTo ?? isEditing); | ||
| const isEditingConfirmation = !startOfFlow && !isEditing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const isEditingConfirmation = !startOfFlow && !isEditing; | |
| const isEditingConfirmation = routeName !== SCREENS.MONEY_REQUEST.DISTANCE_CREATE && !isEditing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it like that to be more readable in the future for people without the context, but we can simplify it like that if you want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakubkalinski0 ok, so could you rename to isStartOfFlow ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanDylann I already changed it to your suggestion. I came to the conclusion that actually you can quite easily deduce what it means if you simply check what is that screen. Maybe we should add a one-line comment explaining whats the difference between isEditingConfirmation and isEditing because I can actually see that someone could find it confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the comment seems like a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Julesssss Then I will add it right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Julesssss Added ✅
| }); | ||
|
|
||
| if (shouldSkipConfirmation) { | ||
| setShouldEnableDiscardConfirmation(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakubkalinski0 Why do we need to add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of skipConfirmation flow we don't choose the recipient nor do we go to confirmation page and create the expense right away thus closing the RHP which DiscardChangesConfirmation tries to block so we have to turn it off right before submission. I described it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please allow me more time to sleep on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no problemo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakubkalinski0 could we remove setShouldEnableDiscardConfirmation and using directly
<DiscardChangesConfirmation
isEnabled={!isEditingConfirmation && !isEditing && !shouldSkipConfirmation}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we can't do it this way. The DiscardChangesConfirmation has to be working in the case when shouldSkipConfirmation is True but has to be turned off just before submitting the expense in order to not show the discard modal when RHP closes on creation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we could just turn it off completely in this case but then we would have inconsistent behaviour during the odometer expense creation flow depending where we start from which in my opinion would be pretty bad
|
|
||
| if (backToReport) { | ||
| Navigation.goBack(backTo); | ||
| if (isEditingConfirmation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you change this condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just incorrect. backToReport simply isn't indicative of editing from confirmation page thus the condition had to be changed
|
@jakubkalinski0 I still question why do we need to introduce isEnabled prop in DiscardChangesConfirmation. Should we use the same approach as other places instead introducing new way? For example, please see
|
@DylanDylann I didn't notice this approach and it felt more natural to me to add this |
|
For new features, I prefer to focus on the core logic of feature and limit refactoring anything. I think it's better to stick with the existing pattern for now and refactor in a separate PR later. This will save time and help us ship the feature faster. Thanks |
|
@DylanDylann I think we will have to wait with this change of |
|
@DylanDylann I would propose that we move on with this PR and resolve the @Julesssss What do you think? Maybe we should wait for the changes of |
I agree, especially as it's blocking the main feature work. DiscardChangesConfirmation PR also shouldn't be blocking our progress. I created a follow up issue here and added it to the tracker. @DylanDylann could you complete your final review? I believe your latest comments have been addressed or responded to, but let me know if I missed something. |
…aviour_in_Odometer_flow
…ngConfirmation in odometer
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-14.at.10.51.45.movAndroid: mWeb ChromeScreen.Recording.2026-01-14.at.10.50.16.moviOS: HybridAppScreen.Recording.2026-01-14.at.10.50.59.moviOS: mWeb SafariScreen.Recording.2026-01-14.at.10.49.12.movMacOS: Chrome / SafariScreen.Recording.2026-01-14.at.10.46.42.mov |
|
@Julesssss All yours |
|
✋ 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/Julesssss in version: 9.3.2-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.3.3-8 🚀
|

Explanation of Change
According to new contributing guide
backTois deprecated and shouldn't be added in newly added routes. For simpler implementation we initially addedbackToto Odometer flow. This PR takes care of the complete removal ofbackTofrom this newly added flow.App/contributingGuides/NAVIGATION.md
Lines 440 to 441 in f330976
In the case of this flow the main issue when removing
backTois that we no longer posses information whether we are editing values from confirmation page or we are inputting the initial values for the first time on the first screen.In order to differentiate those 2 situations the
startOfFlowprop was added to theIOURequestStepDistanceOdometerfunction signature. We set this flag totruewhen we use it directly inTabSelectorhere:App/src/pages/iou/request/DistanceRequestStartPage.tsx
Lines 224 to 235 in b3bddac
This way we can easily define
isEditingConfirmationas a simple check if the current state is different thanstartOfFlowandisEditing. Then we can also replacebackToby simply creatingconfirmationRoutefrom the data we have and navigate directly to it. This way we keep clean url withoutbackTothroughout the whole Odometer flow.It was also very important that
DiscardChangesConfirmationis shown only when user tries to leave the odometer page in the following situation:start readingorend readingFixed Issues
$ #79036
PROPOSAL: N/A
Tests
Note:
When you want the Odometer to be show in the tab selector you need to first remove the false flag from here:
App/src/pages/iou/request/DistanceRequestStartPage.tsx
Lines 224 to 235 in b3bddac
Track distanceOdometerDiscardChangesConfirmationmodal is shownOdometertabstart readingorend readingor both (doesn't matter - the behaviour should be the same in each case)<back buttonDiscardChangesConfirmationmodal is shownCancelon theDiscardChangesConfirmationmodal and thenNextDistance(you should be navigated to odometer page with readings fields filled)<back buttonDiscardChangesConfirmationmodal is shownDistanceDiscardChangesConfirmationmodal is shown and that RHP closesDistanceSaveDiscardChangesConfirmationmodal is shown and that the total distance changed appropriatelyDistancein order to edit the values<back button should work the same)DiscardChangesConfirmationmodal is shown and that the RHP closes correctly (i.e. the odometer page closes)Distance<back button or outside of RHP (both should behave the same)DiscardChangesConfirmationmodal is shown and that RHP closes properlyDistanceSaveDiscardChangesConfirmationmodal is shown and that the total distance changed appropriatelyTrack distanceallowing you to create odometer expense to the same recipient to which you created the previous odometer expensestart readingorend readingor both (doesn't matter the behaviour should be the same in each case)<back buttonDiscardChangesConfirmationmodal is shownCancelon theDiscardChangesConfirmationmodal and thenCreate expenseDiscardChangesConfirmationmodal is shown; the RHP closes properly and the expense is correctly createdOffline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
NO QA
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
From steps 1 to 40:
1to40.mp4
From steps 41 to 43:
41to43.mov