-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Reset request flow when logout #55384
Conversation
@ishpaul777 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] |
8586492
to
a7ff340
Compare
Thanks for the bump! will review over weekend |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-02-11.at.2.17.04.AM.movAndroid: mWeb ChromeScreen.Recording.2025-02-11.at.1.14.23.AM.moviOS: NativeScreen.Recording.2025-02-11.at.1.14.23.AM.moviOS: mWeb SafariScreen.Recording.2025-02-07.at.3.09.53.AM.movMacOS: Chrome / SafariScreen.Recording.2025-02-07.at.2.50.33.AM.movScreen.Recording.2025-02-07.at.2.51.17.AM.movMacOS: DesktopScreen.Recording.2025-02-11.at.2.21.29.AM-1.mov |
it seems to be not fixed @cretadn22 Screen.Recording.2025-01-27.at.1.28.28.AM.movalso when I try to track a expense it redirects me back to amount page Screen.Recording.2025-01-27.at.1.34.02.AM.mov |
@ishpaul777 Your steps differ from the step of this PR. We only reset the money request flow when the user logs out. In your case, since you didn’t log out, the transaction draft still remains |
@cretadn22 i think even then this is still a bug, no?
|
@ishpaul777 It looks like you're following a different set of test steps than the ones mentioned in the PR description. Could you clarify the steps you're using and what the expected behavior should be? |
i tried to reproduce this issue after merging main locally and its not reproducible, I'll continue testing and review, meanwhile please merge main |
@ishpaul777 Merged main |
if (transaction?.transactionID) { | ||
return; | ||
} |
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.
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.
@ishpaul777 Could you please clarify your issue? How are you reproducing it, and what is the expected outcome? It seems like you're following a different test process, but you haven’t specified that, making it difficult for me to reproduce the issue. This is causing delays in completing this PR.
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.
Same as test steps in Pr description, once you got redirected to start of flow close it, and paste the copied url again it will link to empty confirmation page
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.
@ishpaul777 Fixed
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.
still not fixed on ios/android, here I deeplink to create/submit/confirmation/1/<Reportid>
it don't not redirect me to start of flow
Screen.Recording.2025-02-07.at.3.25.01.AM.mov
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.
@ishpaul777 I can't reproduce that on IOS
Screen.Recording.2025-02-07.at.20.54.15.mov
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 can still reproduce this issue same steps as in my initial comment
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.
@ishpaul777 Could you try again using a new account and record your screen, including all the steps?
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 just tried many time but still can't reproduce your bug
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 could not reproduce this anymore
i still seem to have other issue like on IOS when I try to submit expense and select participant I am moved back to starting step. this is reproducable consistently now Screen.Recording.2025-02-07.at.11.36.48.PM.mov |
This bug is fixed |
@cretadn22 Please complete all platform demo video its still pending for some in author checklist |
Ooh yeah please upload the missing videos @cretadn22 🙏 |
@ishpaul777 @Beamanator Sorry, I didn't upload videos on mobile platforms because this bug can't reproduce on these platforms. I updated the video by using deep link on mobile and desktop |
@cretadn22 that's understandable why you'd do that, but we still want to take care to make sure the native apps work from absolutely any change, so in the future please test all platforms no matter what 🙏 |
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.97-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.97-1 🚀
|
Explanation of Change
Fixed Issues
$ #54432
PROPOSAL: #54432 (comment)
Tests
Offline tests
QA Steps
Precondition: Your default currency should not be USD when you create expense
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
Screen.Recording.2025-02-11.at.19.46.00.mov
Android: mWeb Chrome
20.mp4
iOS: Native
Screen.Recording.2025-02-11.at.19.46.24.mov
iOS: mWeb Safari
19.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-01-17.at.10.55.11.mp4
MacOS: Desktop
Screen.Recording.2025-02-11.at.19.44.44.mov