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 - Infinite loader appears when navigate to restricted links #34800

Closed
1 of 6 tasks
lanitochka17 opened this issue Jan 19, 2024 · 17 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 19, 2024

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.27-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. Log in to staging.new.expensify.com
  2. Navigate to each of the following links
    https://staging.new.expensify.com/r/6821704960193694/details
    https://staging.new.expensify.com/r/6821704960193694/participants
    https://staging.new.expensify.com/r/6821704960193694/settings

Expected Result:

User should be redirected to a blank conversation with a message indicating that the user doesn't have access in web and to LHN in mobile view

Actual Result:

"Hmm it is not here..." page appear but there is Infinite loader on RHN

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6347438_1705651403600.Recording__1857.mp4

View all open jobs on GitHub

@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 Jan 19, 2024
@melvin-bot melvin-bot bot changed the title Web - Infinite loader appears when navigate to restricted links [$500] Web - Infinite loader appears when navigate to restricted links Jan 19, 2024
Copy link

melvin-bot bot commented Jan 19, 2024

Job added to Upwork: https://www.upwork.com/jobs/~013c4e3dfa7c42f2b3

Copy link

melvin-bot bot commented Jan 19, 2024

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

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

melvin-bot bot commented Jan 19, 2024

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

@shahinyan11
Copy link

shahinyan11 commented Jan 19, 2024

Proposal

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

Web - Infinite loader appears when navigate to restricted links

What is the root cause of that problem?

The pages use WithReportOrNotFound in Compose which contains condition to display NotFoundPage if the report not found or can not access

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

We should change this condition to navigate to where we want it instead of displaying NotFoundPage. To do that we can change it with below code

if (shouldShowNotFoundPage) {
     Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(props.report?.reportID))
    return
}

And for mobile we can just navigate to home screen

if (shouldShowNotFoundPage) {
    Navigation.goBack(ROUTES.HOME)
    return
}

Addition: If we still need to display NotFound Page in some cases, we can add a new condition above this one with navigation and return . For example

+   if (!ReportUtils.canAccessReport(props.report, props.policies, props.betas)) {
+       Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(props.report.reportID))
+       return 
+   } 
  
  if (shouldShowNotFoundPage) {
      return <NotFoundPage />;
  }

What alternative solutions did you explore? (Optional)

@DylanDylann
Copy link
Contributor

DylanDylann commented Jan 22, 2024

Proposal

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

  • Web - NotFoundPage appears when navigating to restricted links

What is the root cause of that problem?

  • The behavior mentioned in this issue no longer appeared (fixed in issue). The latest behavior is that, the NotFoundPage displayed in the RHN instead of Infinite loader
  • The NotFoundPage displayed because we had a logic below:
    if (shouldShowNotFoundPage) {
    return <NotFoundPage />;
    }
    }
  • As we can see, when the shouldShowNotFoundPage is true, the NotFoundPage is displayed.

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

  • If we want "User should be redirected to a blank conversation with a message indicating that the user doesn't have access in web and to LHN in mobile view", we just need to add the useEffect to withReportOrNotFound:
                    useEffect(() => {
                    if (shouldShowNotFoundPage) {
                        Navigation.goBack();
                    }
                }, [shouldShowNotFoundPage]);
+              if (shouldShowNotFoundPage) {
                    return null;
                }
  • Also, we can create an additional param in withReportOrNotFound, named shouldGoBack and just apply the above logic once shoukdGoBack: true for the backward compatibility

What alternative solutions did you explore? (Optional)

  • NA

@DylanDylann
Copy link
Contributor

@trjExpensify Can you confirm the expected behavior in this issue. Is this "User should be redirected to a blank conversation with a message indicating that the user doesn't have access in web and to LHN in mobile view" ?

@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@cubuspl42
Copy link
Contributor

@DylanDylann

  • The behavior mentioned in this issue no longer appeared (fixed in issue). The latest behavior is that, the NotFoundPage displayed in the RHN instead of Infinite loader

Thanks for investigating this!

@trjExpensify

We definitely need to update the issue (or even close it) based on these findings

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@trjExpensify
Copy link
Contributor

The behavior mentioned in this issue no longer appeared (fixed in #34466). The latest behavior is that, the NotFoundPage displayed in the RHN instead of Infinite loader

Does that mean someone will see two "NotFound" errors?

image

@DylanDylann
Copy link
Contributor

@trjExpensify Yes. It is the current behavior

@DylanDylann
Copy link
Contributor

@trjExpensify FYI, In staging:
Screenshot from 2024-01-23 15-39-19

@trjExpensify
Copy link
Contributor

trjExpensify commented Jan 23, 2024

Got it, thanks! So we've come across the "double Hmm..." in the workspace settings in the new ideal nav we're working on, and opted for a full page error on desktop instead (ref). @shawnborton @dannymcclain @dubielzyk-expensify I wonder if we want to do the same here?

It's slightly different insofar as the main pane (chat) and request money flow chat details (RHP) are not accessible, but the Chats list (LHN) still is. Equally, the main pane is not in focus.

If we're okay with leaving this as is, then I agree we can close out this issue as the infinite spinners as reported are resolved.

@shahinyan11
Copy link

@trjExpensify @cubuspl42 I think there is not important the infinity loader or NotFound page displayed in RHN. The expected result says that user should be redirected to another place. We should fix this behavior

@trjExpensify
Copy link
Contributor

Ah wait.. these are links are for any given room's details, members and settings pages in the LHN.

- https://staging.new.expensify.com/r/6821704960193694/details
- https://staging.new.expensify.com/r/6821704960193694/participants
- https://staging.new.expensify.com/r/6821704960193694/settings

If we do what you're suggesting, when you click one of these links won't we end up:

  1. opening the RHP while we figure out you don't have access
  2. immediately close it so you're back in the room with the one NotFound error

Equally, how do we handle if someone has access to the chat in question, but not a particular page in its settings? The main pane chat shouldn't result in NotFound, only the applicable RHP setting page, right?

Overall, I think we probably leave this as it is. Though @lanitochka17, perhaps you can shed some light on where this bug report came from in the test suite.

@DylanDylann
Copy link
Contributor

DylanDylann commented Jan 23, 2024

Equally, how do we handle if someone has access to the chat in question, but not a particular page in its settings? The main pane chat shouldn't result in NotFound, only the applicable RHP setting page, right?

@trjExpensify

  • Additional information, the RHN displays NotFound if: Central report displays NotFound or other reasons (user does not have permission to access the report's setting, report's participants, archived room, ...). So we just close the RHN if the Central report displays NotFound. In other cases, the RHN still displays

@trjExpensify
Copy link
Contributor

Right, so I'm questioning if it's really worth adding that code as a condition here? We've established we independently need a NotFound page on the main chat page, and the RHP for when you have access to the room but not certain settings pages.

So given that we have this dynamic, and it's not a case where it warrants a full page NotFound error like the ref here, I'm inclined to do nothing. Happy for a second opinion from the design team tagged though!

@shawnborton
Copy link
Contributor

Down to follow Tom's suggestion :)

@trjExpensify
Copy link
Contributor

Alrighty, closing it out!

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

6 participants