Skip to content
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-08-28] [Tracking Browser back button] [$1000] mWeb- Clicking on back button on Concierge after leaving the thread gives us access to the same thread again #20908

Closed
1 of 6 tasks
kbecciv opened this issue Jun 16, 2023 · 131 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Jun 16, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to mweb chrome and go to any chat
  2. Send a message and then click on reply thread on the message so that a new thread page opens up
  3. Click on header and leave the room
  4. Notice it shows you to Concierge page
  5. Click on back button and notice that it brings the same thread chat again that you left few moments back. If a user has left the thread, clicking on back button should have brought the user to LHN instead of giving re-access to the same thread

Expected Result:

Clicking on back button on Concierge after leaving the thread shouldn't give access to the same thread again (since the user had already decided to leave)

Actual Result:

licking on back button on Concierge after leaving the thread gives us access to the same thread again

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.2.7.6

Reproducible in staging?: yes

Reproducible in production?: yes

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

Notes/Photos/Videos: Any additional supporting documentation

thread.1.mp4
Screen.Recording.20230615.000715.Chrome.mp4

Expensify/Expensify Issue URL:

Issue reported by: @priya-zha

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686249319392879

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01448a7bf541bf3868
  • Upwork Job ID: 1670845602095009792
  • Last Price Increase: 2023-07-10
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 16, 2023

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jun 16, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@hoangzinh
Copy link
Contributor

hoangzinh commented Jun 16, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

mWeb- Clicking on back button on Concierge after leaving the thread gives us access to the same thread again

What is the root cause of that problem?

When user click on the back arrow in the Header view, it simply call the Navigation.goBack function which will navigate to the previous route history, then we will see the thread report again.

What changes do you think we should make in order to solve the problem?

I think in the leaveRoom action, we need to call Navigation.goBack() 2 times before navigate to Concierge report, so we will pop the thread report and thread report details route out of histories. It will also fix the bug if we click on the browser back arrow.

@melvin-bot
Copy link

melvin-bot bot commented Jun 16, 2023

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@namhihi237
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Clicking on back button on Concierge after leaving the thread shouldn't give access to the same thread again (since the user had already decided to leave)

What is the root cause of that problem?

The problem is that after pressing back it will return to the previous screen which is router /r/reportID/details because we haven't cleared the screen thread.

What changes do you think we should make in order to solve the problem?

Define function pop screen in Navigation.js

function popScreens(numberScreen = 1) {
    navigationRef.current.dispatch(StackActions.pop(numberScreen));
}

In the function leaveRoom we will pop 2 screen before navigateToConciergeChat

Navigation.popScreens(2)
navigateToConciergeChat();

What alternative solutions did you explore? (Optional)

N/A

@melvin-bot
Copy link

melvin-bot bot commented Jun 16, 2023

📣 @ginsuma! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@eh2077
Copy link
Contributor

eh2077 commented Jun 17, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

On mWeb, clicking on back button on Concierge after leaving the thread gives us access to the same thread again

What is the root cause of that problem?

The root cause of this issue is that, when clicking header go back button, the route state still saves previous pages(including the details page) after clicking Leave Thread, see leaveRoom, navigateToConciergeChat, and Header Navigation.goBack.

What changes do you think we should make in order to solve the problem?

From method leaveRoom

navigateToConciergeChat();

We know that we want to navigate to the concierge chat when clicking Leave Thread button. After navigating to the concierge chat page from the details page, we don't want to navigate back to the details page if clicking the header go back button. Instead, we want to go back to the home page.

To fix this issue, we can reset the route state of navigation to the concierge chat when calling method leaveRoom.

To achieve this, we can

  1. Add a new type RESET support for method Navigation.navigate

/**
* Main navigation method for redirecting to a route.
* @param {String} route
* @param {String} type - Type of action to perform. Currently UP is supported.
*/
function navigate(route = ROUTES.HOME, type) {

  1. Change the condition of linkTo method to reset the state of navigation. If type is RESET, then reset the state like
if (action !== undefined && type !== 'RESET') {
    root.dispatch(action);
} else {
    root.reset(state);
}
  1. Add an optional parameter to method navigateToConciergeChat

function navigateToConciergeChat() {

like

function navigateToConciergeChat(shouldResetNavigationState = false) {

If shouldResetNavigationState is true, then pass type RESET to method Navigation.navigate, like Navigation.navigate(route, 'RESET').

  1. Call navigateToConciergeChat(true); in method leaveRoom

navigateToConciergeChat();

Click to find the demo video
Simulator.Screen.Recording.-.iPhone.14.-.2023-06-18.at.01.20.27.mp4

What alternative solutions did you explore? (Optional)

We can also navigate to home page and reset route state of navigation when leaving thread by changing this line

navigateToConciergeChat();

to

Navigation.navigate(ROUTES.HOME, 'RESET');

This alternative will be applicable if the product team agrees with navigating to home page after leaving thread.

@melvin-bot melvin-bot bot added the Overdue label Jun 19, 2023
@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Jun 19, 2023
@melvin-bot melvin-bot bot changed the title mWeb- Clicking on back button on Concierge after leaving the thread gives us access to the same thread again [$1000] mWeb- Clicking on back button on Concierge after leaving the thread gives us access to the same thread again Jun 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 19, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01448a7bf541bf3868

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 19, 2023

Current assignee @abekkala is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jun 19, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

@abekkala
Copy link
Contributor

@allroundexperts we already have a couple proposals for this one

@abekkala
Copy link
Contributor

@allroundexperts have you had a chance to review the proposals that have come in?

@allroundexperts
Copy link
Contributor

@allroundexperts have you had a chance to review the proposals that have come in?

@abekkala I did review these but I'm not concretely sure about the best approach. I'm tilting towards @namhihi237's proposal since we've used navigation pop method in another PR as well. Given that this issue occurred after navigation refactor that we did, I'd like to get @WoLewicki or @mountiny's opinion on this as well.

@eh2077
Copy link
Contributor

eh2077 commented Jun 22, 2023

@allroundexperts Thanks for reviewing proposals. Can I have your feedback about my proposal? I proposed a solution to reset the state of navigation stack, which should be useful and complementary to the current navigating methods provided in Navigation.js.

For example, we can open a page of App in a new tab through a link. If we perform an exit-and-redirect like navigation action, like leaving a thread, from that page, then having a reseting navigation stack solution will be helpful to avoid navigating to the Hmm... it's not here page in various use cases.

cc @mountiny @WoLewicki

@namhihi237
Copy link
Contributor

Thanks @allroundexperts for appreciating my proposal, I think if the user has pressed the back button it is their intention and they want to go back to the previous page.

@WoLewicki
Copy link
Contributor

Is this behavior the same on web? How does the forward browser button works in these flows if so? @eh2077 could you elaborate more on how would the RESET action API look like? It uses the whole state object as its argument (https://reactnavigation.org/docs/navigation-actions/#reset). How would we handle that in order to decide which part of the state we would keep etc?

The only problem with @namhihi237 proposal that I can think of is that it dispatches 2 navigation actions one after another, being popping the 2 screens and then navigating. I am afraid it can introduce some animation and browser buttons glitches. Normally resetting the state is the proper solution with removing the 2 last screens and adding another one at the end, but if you confirm that those mentioned things work as they should, I think we can also go with it.

@namhihi237
Copy link
Contributor

Thanks @WoLewicki Yes, It's works well with my proposal

Screen.Recording.2023-06-23.at.21.28.17.mov

@WoLewicki
Copy link
Contributor

@namhihi237 and what happens if you click the forward browser button after leaving the thread? Seems weird that it is possible to even do that.

@mountiny
Copy link
Contributor

We are going to be more strict on these requests but given this issue has 114 comments and its been open since June 16 I think its valid to give the classic 25% so $250 for your help here. Albeit its not probably matching the efforts, there are other issues which end up being less time consuming so it balances out I believe. Thanks for your help

@hayata-suenaga
Copy link
Contributor

@dylanexpensify so the issue was fixed in another PR. The is the payment decision.

@dylanexpensify
Copy link
Contributor

Got it! @allroundexperts @priya-zha mind applying here

@allroundexperts
Copy link
Contributor

I'll be paid by our App. Can you please add a payment summary here @dylanexpensify?

@priya-zha
Copy link

Hi. I'm not being able to apply to the job since it says 'The job is no longer available'

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 Daily KSv2 labels Aug 28, 2023
@dylanexpensify
Copy link
Contributor

Woot! Payment summary:

Upwork job! Please apply!

@dylanexpensify
Copy link
Contributor

New job ^ fyi @priya-zha

@priya-zha
Copy link

Applied. Thanks

@JmillsExpensify
Copy link

Reviewed the details for @allroundexperts. $250 approved for payment via NewDot.

@dylanexpensify
Copy link
Contributor

TY! Payment coming now @priya-zha

@dylanexpensify
Copy link
Contributor

Offer sent @priya-zha !

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@dylanexpensify
Copy link
Contributor

bump @priya-zha

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@priya-zha
Copy link

@dylanexpensify Accepted the offer already. Waiting for the payment. Thanks.

@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@flaviadefaria
Copy link
Contributor

@dylanexpensify just to confirm have you paid @priya-zha? I see the contract is still open but it's unclear to me if they have been paid or not.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 11, 2023
@dylanexpensify
Copy link
Contributor

Payment sent!

@melvin-bot melvin-bot bot removed the Overdue label Sep 13, 2023
@dylanexpensify
Copy link
Contributor

Contract ended!

@flaviadefaria
Copy link
Contributor

Perfect so closing this! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
No open projects
Development

No branches or pull requests