-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[Navigation] Fix goBack in IOU steps #56876
[Navigation] Fix goBack in IOU steps #56876
Conversation
@ikevin127 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] |
I am not sure what is the process of choosing reviewers but it would be great if you guys @shubham1206agra and @WojtekBoman could take a look because you're in the loop on this issue. Thanks! 🙇 |
@adamgrzybowski
Current behavior:Users redirect to participant page Expected behavior:Users should redirect to amount page Screen.Recording.2025-02-17.at.16.00.59.mov |
Hey, @dukenv0307! I made some adjustments you requested. Could you please take a look once again? |
@@ -86,6 +86,16 @@ function getActivePolicies(policies: OnyxCollection<Policy> | null, currentUserL | |||
!!policy && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && !!policy.name && !!policy.id && !!getPolicyRole(policy, currentUserLogin), | |||
); | |||
} | |||
|
|||
function getPerDiemCustomUnits(policies: OnyxCollection<Policy> | null, email: string | undefined): Array<{policyID: string; customUnit: CustomUnit}> { |
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.
function getPerDiemCustomUnits(policies: OnyxCollection<Policy> | null, email: string | undefined): Array<{policyID: string; customUnit: CustomUnit}> { | |
function getPerDiemCustomUnits(policies: OnyxCollection<Policy> | null, email: string | undefined): Array<{policyID: string; customUnit: CustomUnit | undefined}> { |
By doing this way, we don't need to cast the result
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.
Hmm, I assume that as a result type we want to have array of objects that have a defined value of CustomUnit
@@ -120,8 +132,16 @@ function IOURequestStepParticipants({ | |||
setCustomUnitRateID(transactionID, rateID); | |||
setMoneyRequestParticipantsFromReport(transactionID, selfDMReport); | |||
const iouConfirmationPageRoute = ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(action, CONST.IOU.TYPE.TRACK, transactionID, selfDMReportID); | |||
Navigation.navigate(iouConfirmationPageRoute); | |||
}, [action, selfDMReport, selfDMReportID, transactionID]); | |||
waitForKeyboardDismiss(() => { |
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 waitForKeyboardDismiss
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.
If we type a participant from the keyboard and select it, we want to first close the keyboard and then navigate
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-02-18.at.10.38.33.movAndroid: mWeb ChromeScreen.Recording.2025-02-18.at.10.33.09.moviOS: NativeScreen.Recording.2025-02-18.at.10.33.09.moviOS: mWeb SafariScreen.Recording.2025-02-18.at.10.31.06.movMacOS: Chrome / SafariScreen.Recording.2025-02-18.at.10.28.59.movMacOS: DesktopScreen.Recording.2025-02-18.at.10.40.45.mov |
@adamgrzybowski Bug:
Current behavior:The initial WS is shown Expected behavior:The selected WS in step 4 is shown Screen.Recording.2025-02-18.at.10.40.45.mov |
@WojtekBoman can you please take over this one? |
I checked the version before our merge and it works the same way there. Here's a video. I believe it's a proper behavior, we remove this screen from the navigation state and push a new one when we open it second time. If we open a new confirmation page, we initially have our default workspace set Screen.Recording.2025-02-18.at.09.32.29.mov |
401f00d
to
b0e37b5
Compare
@dukenv0307 @mountiny |
yeah that flow seems fine to me @dukenv0307 can you please complete the review? thanks! |
Thank you. We're good to merge |
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.
Thanks!
✋ 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/mountiny in version: 9.1.1-0 🚀
|
@adamgrzybowski @mountiny @WojtekBoman Anything to QA here? |
@mvtglobally updated |
🚀 Deployed to production by https://github.com/francoisl in version: 9.1.1-6 🚀
|
This PR fixes the issue: Destination field is not editable & defaulted to 1:1 DM after coming from amount page
Besides I used this PR to fix related problems in the per diem flow discussed here https://swmansion.slack.com/archives/C01GTK53T8Q/p1739480554048859?thread_ts=1739420502.347449&cid=C01GTK53T8Q
Which is a follow-up fix for this PR
Explanation of Change
Fixed Issues
$ #56803
$ #56807
$ #56810
PROPOSAL:
Tests
Precondition:
Precondition:
Account has a default group workspace.
Go to staging.new.expensify.com
Go to FAB > Create expense > Manual.
Click currency.
Select a different currency.
Enter amount > Next.
Click RHP back button.
Currency will not reset to default currency.
Precondition:
Account has a default group workspace.
Go to staging.new.expensify.com
Open FAB > Create expense > Manual.
Enter amount > Next.
Click on the destination field.
Select any user.
On confirmation page, swipe to right (device back button on Android) to go back.
Swipe to right (device back button on Android) to go back again.
App will return to amount input page.
Offline tests
Same as the tests
QA Steps
Same as the tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2025-02-14.at.15.21.31.mov
Screen.Recording.2025-02-14.at.15.31.39.mov
MacOS: Desktop