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

[$500] Web - When a user changes their profile status, the app brings you to the end of the conversation. #33110

Closed
1 of 6 tasks
lanitochka17 opened this issue Dec 14, 2023 · 87 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Dec 14, 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!


Version Number: 1.4.13.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:

  1. Launch the app
  2. Log in with any account
  3. Open the 1:1 conversation
  4. Scroll to the middle
  5. Go to the seating - Profile - Status
  6. Change the status

Expected Result:

When a user changes their profile status, the app should stay in the same conversation position

Actual Result:

When a user changes their profile status, the app brings you to the end of the conversation

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • Windows: Chrome
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6313458_1702578893863.Recording__5707.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017a47704cf8762519
  • Upwork Job ID: 1735371288086798336
  • Last Price Increase: 2024-01-04
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 14, 2023
@melvin-bot melvin-bot bot changed the title Web - When a user changes their profile status, the app brings you to the end of the conversation. [$500] Web - When a user changes their profile status, the app brings you to the end of the conversation. Dec 14, 2023
Copy link

melvin-bot bot commented Dec 14, 2023

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

Copy link

melvin-bot bot commented Dec 14, 2023

Job added to Upwork: https://www.upwork.com/jobs/~017a47704cf8762519

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

melvin-bot bot commented Dec 14, 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

Copy link

melvin-bot bot commented Dec 14, 2023

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 14, 2023

Proposal

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

When a user changes their profile status, the app brings you to the end of the conversation.

What is the root cause of that problem?

Currently we are popingToTop after saving a profile status as we set setShouldPopAllStateOnUP when we come from the status button flow (not profile > status flow) here

const showStatusPage = useCallback(() => {
if (isCreateMenuOpen) {
// Prevent opening Settings page when click profile avatar quickly after clicking FAB icon
return;
}
Navigation.setShouldPopAllStateOnUP();
Navigation.navigate(ROUTES.SETTINGS_STATUS);

so the page is unnecessarily reloading and the report screen is scrolled to bottom creating a disturbing user experience and I think we should avoid this unnecessary reloading.

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

We should use goBack when we come from the status button flow without fallbackRoute we can do that by saving an indicator that the user comes from the status button flow (like fromStatusButonFlow in onyx) here

const showStatusPage = useCallback(() => {
if (isCreateMenuOpen) {
// Prevent opening Settings page when click profile avatar quickly after clicking FAB icon
return;
}
Navigation.setShouldPopAllStateOnUP();
Navigation.navigate(ROUTES.SETTINGS_STATUS);

const navigateBackToPreviousScreen = useCallback(() => Navigation.goBack(ROUTES.SETTINGS_PROFILE, false, true), []);

 const navigateBackToPreviousScreen = useCallback(() => {
        if (fromStatusButtonFlow) {
            resetFromStatusButtonFlow();
            Navigation.goBack();
        } else Navigation.goBack(ROUTES.SETTINGS_PROFILE, false, true);
    }, []);

The necessity of saving it in onyx is to make it work perfectly on page refresh. otherwise, we can also use global variable just similar to shouldPopAllStateOnUP. By the way, currently, if you press the status button and then on the status page refresh and press back on the status page it navigates to profile page which is wrong (related bug) it should pop the RHP as it would if you haven't refreshed.

What alternative solutions did you explore? (Optional)

@bernhardoj
Copy link
Contributor

bernhardoj commented Dec 15, 2023

Proposal

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

If we open another chat, then open the status page from the sidebar (settings) avatar, and then close it, a new chat will open.

What is the root cause of that problem?

When we open the status page from the sidebar avatar, we set shouldPopAllStateOnUP.

Navigation.setShouldPopAllStateOnUP();
Navigation.navigate(ROUTES.SETTINGS_STATUS);

Then, on pressing back/save, we call goBack with shouldPopToTop (3rd param) as true.

const navigateBackToPreviousScreen = useCallback(() => Navigation.goBack(ROUTES.SETTINGS_PROFILE, false, true), []);

If both shouldPopAllStateOnUP and shouldPopToTop are true, it will pop all screens up to the LHN.

if (shouldPopToTop) {
if (shouldPopAllStateOnUP) {
shouldPopAllStateOnUP = false;
navigationRef.current?.dispatch(StackActions.popToTop());
return;
}
}

So, the current report is also popped out from the stack, and a new one will be added.

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

I would like to propose a quite different solution. First of all, I think we shouldn't use setShouldPopAllStateOnUP. I see 1 flaw with that approach.

If we open the status page from the sidebar avatar, shouldPopAllStateOnUP will be true. If we close the status page by pressing the overlay, shouldPopAllStateOnUP will stay true (we only reset shouldPopAllStateOnUP when calling goBack).

Because it's still true if we open the status page from the profile page and press back, it will pop out all screens, even though it should go back to the profile page.

So, the solution I would like to propose is to have a different route path for the status page, depending on where we open it. If we open it from the sidebar avatar, we will navigate to /status, but if we open it from the profile page, then we will navigate to the same route we have now, that is /settings/profile/status.

Navigation.setShouldPopAllStateOnUP();
Navigation.navigate(ROUTES.SETTINGS_STATUS);

Navigation.navigate(ROUTES.STATUS);

Then, based on the current route, we can decide whether we need the fallback route or not.

const navigateBackToPreviousScreen = useCallback(() => {
    const path = Navigation.getActiveRoute();
    const fallbackRoute = path.startsWith(ROUTES.SETTINGS_PROFILE) ? ROUTES.SETTINGS_PROFILE : '';
    // or const fallbackRoute = route.name === SCREENS.SETTINGS.PROFILE.STATUS ? ROUTES.SETTINGS_PROFILE : '';
    Navigation.goBack(fallbackRoute);
}, []);
Here is how you add the new route
  1. Add a new entry to ROUTES.ts (this is the route path)
    STATUS: 'status'
  2. Add a new entry to SCREENS.ts (this is the route name)
    STATUS: 'Status'
  3. Add a new entry to linkingConfig.ts (binding it up, you can add it above SCREENS.SETTINGS.PROFILE.STATUS)
[SCREENS.STATUS]: {
    path: ROUTES.STATUS,
    exact: true,
},
  1. Add a new screen in ModalStackNavigators.tsx, specifically in SettingsModalStackNavigator navigator. (you can add it above SCREENS.SETTINGS.PROFILE.STATUS)
[SCREENS.STATUS]: () => require('../../../pages/settings/Profile/CustomStatus/StatusPage').default as React.ComponentType,

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Dec 15, 2023
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Dec 15, 2023
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Dec 15, 2023

Current assignee @puneetlath is eligible for the Engineering assigner, not assigning anyone new.

@suneox
Copy link
Contributor

suneox commented Dec 17, 2023

Proposal

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

Web - When a user changes their profile status, the app brings you to the end of the conversation.

What is the root cause of that problem?

After save status will Navigation.goBack at function navigateBackToPreviousScreen after that the state of the report screen will be reset

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

We have to check the current state has topmostReportId and route history then we will navigate to report screen instead of go back at this line

    const routesHistory = useNavigationState((state) => state.routes || []);
    const navigateBackToPreviousScreen = useCallback(() => {
        const topmostReportId = Navigation.getTopmostReportId();
        if (topmostReportId && routesHistory.length === 1 && !routesHistory[0].path) {
            Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(topmostReportId));
        } else {
            Navigation.goBack(ROUTES.SETTINGS_PROFILE, false, true)
        }
    }, [routesHistory]);

POC

33110_2.mp4

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Dec 17, 2023
@mananjadhav
Copy link
Collaborator

@lanitochka17 Is this still reproducible? I can't seem to be able to reproduce this.

Screen.Recording.2023-12-18.at.12.29.55.AM.mov

@melvin-bot melvin-bot bot removed the Overdue label Dec 17, 2023
@mananjadhav
Copy link
Collaborator

@FitseTLT I don't think so. The linked commit is not merged, but used as a part of the proposal.

@FitseTLT
Copy link
Contributor

@mananjadhav Now it is opening another report when I save status. Before 3 days it was reloading the current report. Odd!!

@suneox
Copy link
Contributor

suneox commented Dec 18, 2023

@lanitochka17 Is this still reproducible? I can't seem to be able to reproduce this.

@mananjadhav it still reproduce by click on a status icon on the left of avatar, the description is missed step click on the icon next to avatar. you can view a video attachment in description

@puneetlath
Copy link
Contributor

I am able to reproduce it @mananjadhav. However, I don't think this needs to be a deploy blocker.

@puneetlath puneetlath removed the DeployBlockerCash This issue or pull request should block deployment label Dec 18, 2023
@suneox
Copy link
Contributor

suneox commented Feb 6, 2024

@suneox I can see the bug in your video screenshot or did I miss something? Pls help correct me

@DylanDylann
I don't know what is regression in your mention? You have provided the regression issue and in my video, after reload and clicking on back button app still opens profile page that is expected behavior for your reference issue.
What is the expected behavior for this case you think it's a regression?

@DylanDylann
Copy link
Contributor

DylanDylann commented Feb 6, 2024

@suneox Oh sorry because I did not mention the expected behavior. Here is the steps:

  1. Go to app.
  2. Open different chat reports.
  3. Click on the status on the top left.
  4. Reload the page.
  5. Click on the back button.

Actual: The profile modal opens
Expected: The profile modal closes

@suneox
Copy link
Contributor

suneox commented Feb 6, 2024

@suneox Oh sorry because I did not mention the expected behavior. Here is the steps:

  1. Go to app.
  2. Open different chat reports.
  3. Click on the status on the top left.
  4. Reload the page.
  5. Click on the back button.

Actual: The profile modal opens Expected: The profile modal closes

@DylanDylann I don't know your reference issue the Expected result is App should open profile page on click on back on status page but your Expected is The profile modal closes
You have provided a reference issue and then you changed the expected behavior.

In my video seconds from 18-23s still match the expected behavior

@DylanDylann
Copy link
Contributor

DylanDylann commented Feb 6, 2024

You have provided a reference issue and then you changed the expected behavior.

  • I do not change the expected behavior. The reproduce steps in here are not the same as the one I mentioned above. So the expected in both cases are not the same

  • Generally, the expected is that, clicking on back button will bring user to the previous screen

@suneox
Copy link
Contributor

suneox commented Feb 6, 2024

  • I do not change the expected behavior. The reproduce steps in here are not the same as the one I mentioned above. So the expected in both cases are not the same

The reproduce step do not include reload step.
@DylanDylann
First you have raised regression is #33002
Screenshot 2024-02-06 at 13 13 23

Generally, the expected is that, clicking on back button will bring user to the previous screen

And now your expected behavior is the same for reload case, I think you not fair when you raise regression and then you update the current behavior is a regression, so how about your the expected behavior on you issue has linked when reload?

@DylanDylann
Copy link
Contributor

The reproduce step do not include reload step.

I mentioned it in step 4

so how about your the expected behavior on you issue #33002 when reload?

I think the expected is: clicking on back button will bring user to the previous screen

@suneox
Copy link
Contributor

suneox commented Feb 6, 2024

so how about your the expected behavior on you issue #33002 when reload?

I think the expected is: clicking on back button will bring user to the previous screen

The expected behavior in #33002 after reload is App should open profile page on click on back on status page so let @puneetlath and @mananjadhav confirm the expected behavior

@DylanDylann
Copy link
Contributor

@suneox Sure. FYI, there was a similar issue #30509

@puneetlath
Copy link
Contributor

@mananjadhav what are your thoughts on this issue?

@mananjadhav
Copy link
Collaborator

I checked the PR, but I'll check all the comments today.

@puneetlath
Copy link
Contributor

@mananjadhav bump!

@mananjadhav
Copy link
Collaborator

Was unwell past few days, will get to this by tomorrow.

@puneetlath
Copy link
Contributor

@mananjadhav any update?

@mananjadhav
Copy link
Collaborator

mananjadhav commented Mar 5, 2024

I went through the PR and the comments discussion. There was substantial delay from my side to review the comments.

But I don't think this is still reproducible anymore. We don't have a Profile modal, we have the Profile page. Opening the status shows the Profile page, moving away from the report page. Reloading the page should go back to the Profile page, so I think this is working as expected.

@lanitochka17 Can you confirm if this is still reproducible at your end?

web-status-profile-reload.mov

@mvtglobally
Copy link

Since the page goes to profile, it resets back to the bottom. If the expectation is to reset, than issue is no longer applicable,
https://github.com/Expensify/App/assets/43995119/f3d7fd48-fee3-40ca-81e1-6056812f6813

@mananjadhav
Copy link
Collaborator

@puneetlath What should we do here?

@puneetlath
Copy link
Contributor

I think we should close this out. The original bug isn't there anymore and I think what is shown in @mvtglobally's video is fine.

Thanks for your input everyone!

@puneetlath
Copy link
Contributor

puneetlath commented Apr 18, 2024

Reopening this since the work was completed, so there should be some payment made. Being discussed here: https://expensify.slack.com/archives/C02NK2DQWUX/p1713323524607299

@puneetlath puneetlath reopened this Apr 18, 2024
@suneox
Copy link
Contributor

suneox commented Apr 21, 2024

Hi @puneetlath,

I believe that fair compensation is warranted for the time I spent investigating solutions, creating and updating the PR. This is because another contributor raised a regression issue that was not clearly communicated. Additionally, this contributor made multiple updates without notifying others after my solution works he also continued to update, and the reviewer also delayed reviewing the PR.

@puneetlath
Copy link
Contributor

Ok, I've reviewed the thread/PR and I think that @suneox is due compensation since they did do the work they were hired for, though we didn't end up using it.

I think @mananjadhav is due 50% compensation since they did partial work, but did not review/test the PR.

Payment summary:
C - @suneox - $500 - to be paid via Upwork
C+ - @mananjadhav $250 - to be paid via NewDot

@suneox can you please send me a link to your upwork profile?
@mananjadhav please request on NewDot.

@mananjadhav
Copy link
Collaborator

mananjadhav commented May 4, 2024

Thanks @puneetlath. Agreed with the assessment.

@suneox
Copy link
Contributor

suneox commented May 4, 2024

@puneetlath Thanks for recognizing the effort put into our work, here is my upwork profile

@JmillsExpensify
Copy link

$250 approved for @mananjadhav

@puneetlath
Copy link
Contributor

Np y'all. Thanks for your patience. @suneox offer is here: https://www.upwork.com/nx/wm/offer/102163593

Please ping me on the issue when you've accepted it.

@suneox
Copy link
Contributor

suneox commented May 4, 2024

Please ping me on the issue when you've accepted it.

@puneetlath I have accepted an offer. Thank you!

@puneetlath
Copy link
Contributor

Ok all paid. Thanks y'all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

10 participants