-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[HOLD for payment 2023-11-29] [$500] Status - Different chat report opens when clicking on the back button in Status modal #30509
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01f903eb3ac10fe198 |
Triggered auto assignment to @CortneyOfstad ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Different chat report opens when clicking on the back button in Status modal What is the root cause of that problem?In the StatusPage we pass navigateBackToSettingsPage into onBackButtonPress prop. What changes do you think we should make in order to solve the problem?I think we can remove the shouldPopToTop param Navigation.goBack(ROUTES.SETTINGS_PROFILE, false) What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.When go to status page after navigating several reports, back button navigates to the report other than the current report What is the root cause of that problem?When we navigate to status page, we set
And when we go back, we call navigate.goBack with shouldPopToTop = true
This makes the route stack pop to the first page, which is home page. App/src/libs/Navigation/Navigation.js Lines 107 to 113 in 1fc2bbc
For wide screen devices, we navigate to the last accessed report when navigating to home page
We determine the last access report by its lastReadTime. Line 553 in ee5e6de
The problem is here. When we call This is the root cause What changes do you think we should make in order to solve the problem?I think this issue should be fixed in the backend. What alternative solutions did you explore? (Optional)We can simply call |
ProposalPlease re-state the problem that we are trying to solve in this issue.A different chat report opens when clicking back button in status modal. What is the root cause of that problem?We're setting This is so that:
But this causes a side effect, which is this issue, where we go back to a different report, because the report that was open when avatar status is clicked might not be the last read report of the LHN. What changes do you think we should make in order to solve the problem?We just need to add a condition, if there's a report open in the background of the status page (in that case there exists a topmost report id), we should go back to that report instead. We can update this to
This will keep our intended behavior for using What alternative solutions did you explore? (Optional)NA |
@shubham1206agra thoughts on the proposals above? Thanks! |
@dukenv0307's proposal works correctly. |
Triggered auto assignment to @Beamanator, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
I suggest to try with backend updates updating |
@s-alves10 @shubham1206agra that bug has its own separate GH here, and was closed due to this reasoning. I was also trying to get it reopened, but if it should be fixed, it should be fixed there, also I don't think it's the root cause of this issue so we can go forward with this one. That's because, @s-alves10 even after that BE issue is fixed, if you open the Status page, then in another device you read another report, then coming back to this device and click back on Status modal, it will go to that other report rather than the report in the background of the Status page. |
Ok let's go with @dukenv0307 proposal then |
Maybe you're right, but from my research, I don't think so By the way, don't you think not updating Anyway, these are just my personal thoughts. |
@s-alves10 Please don't worry about this. Let me ask this in Slack. |
Yes, because with OpenReport we're just fetching reports for user from the server. The report is read when we actually render them on the screen. These are two closely related but different actions and hence we have different API calls. This is the answer I got @s-alves10 |
@s-alves10 yes I agree with you on this, but it's part of this separate issue, which I'm advocating to reopen. It can be fixed separately IMO |
@Beamanator Friendly bump |
@shubham1206agra thanks for bringing that convo to slack, can you please link it here so it's easy access? If you're having trouble getting that separate issue to reopen, we may also want to bring that up in slack (if you haven't already) So @shubham1206agra you're still advocating for going with @dukenv0307 's proposal, right? Anything you need from me other than a review as well? |
@Beamanator You can proceed with @dukenv0307 right now I do have seperate issue for this too which are open. I will start a discussion there only (maybe in slack too) |
Thanks @shubham1206agra ! |
📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@shubham1206agra The PR is ready for review |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.1-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-11-29. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@shubham1206agra @dukenv0307 — both of you have been paid in NewDot and regression test was created here — closing! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v1.3.92-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @
Action Performed:
Precondition: User has set a status.
Expected Result:
The current chat report remains open.
Actual Result:
A different chat report opens.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Bug6253028_1698396389798.20231027_164311.mp4
MacOS: Desktop
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: