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] Restricted access - Redirection does not work #12676

Closed
kbecciv opened this issue Nov 11, 2022 · 47 comments
Closed

[HOLD] Restricted access - Redirection does not work #12676

kbecciv opened this issue Nov 11, 2022 · 47 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Nov 11, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to URL https://staging.new.expensify.com/
  2. Login with any account
  3. 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:

You should be redirected to the concierge conversation

Actual Result:

Redirection does not work

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App

Version Number: 1.2.27.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5816583_Recording__2858.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 11, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 11, 2022

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

@kbecciv
Copy link
Author

kbecciv commented Nov 11, 2022

QA team is Failing steps in TestRail https://expensify.testrail.io/index.php?/tests/view/2936076

@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

Triggered auto assignment to @alex-mechler (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mateocole
Copy link

Having an engineer confirm this is something I can mark external

@melvin-bot melvin-bot bot removed the Overdue label Nov 15, 2022
@alex-mechler
Copy link
Contributor

Yup, looks like something broke here. Since this is just routing this can go external

@alex-mechler alex-mechler removed their assignment Nov 15, 2022
@alex-mechler alex-mechler added the External Added to denote the issue can be worked on by a contributor label Nov 15, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 15, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

Triggered auto assignment to @marcaaron (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title Restricted access - Redirection does not work [$250] Restricted access - Redirection does not work Nov 15, 2022
@peterklamka
Copy link

@kbecciv I logged in my account. but I got this screen when I visit https://staging.new.expensify.com/r/6821704960193694.
so I don't think /details, /participants, /settings will work too.

image

@mateocole
Copy link

@mateocole mateocole added Weekly KSv2 and removed Daily KSv2 labels Nov 15, 2022
@marcaaron
Copy link
Contributor

@mateocole were you able to reproduce on staging? I tried and just see that the modal closes and I get redirected to a chat. I don't get "redirected to Concierge", but not seeing the weird messed up state that tester is reporting either.

2022-11-15_13-41-03.mp4

Couple thoughts...

  • This could be a browser specific issue.
  • Is getting redirected to Concierge actually the correct behavior? Could the TC (test case) be out of date?

@mollfpr
Copy link
Contributor

mollfpr commented Nov 17, 2022

Same as @marcaaron I can't reproduce it.

I feel like this is a regression from this PR where it should redirect to the concierge chat. But the issue #11982 is not stated needs to redirect to the concierge chat.

@marcaaron
Copy link
Contributor

marcaaron commented Nov 17, 2022

Hmm I am so confused about that PR you linked. If a chat doesn't exist in storage yet we are just redirecting randomly? Why does the report not exist? Do we try to access it from the server? I'm totally lost, but the HOC solution seems very strange to me. @Beamanator please share your thoughts here.

@kbecciv Where does the expectation that we should redirect to the Concierge chat come from? Is it an existing TC?

@mateocole I think we need to bring this one into #bug-zero to resolve.

@kbecciv
Copy link
Author

kbecciv commented Nov 18, 2022

@mollfpr
Copy link
Contributor

mollfpr commented Nov 18, 2022

If a chat doesn't exist in storage yet we are just redirecting randomly?

Yes, currently we do dismiss the modal. The expected result from this #11982 is not stated redirecting to the concierge.

Why does the report not exist?

When we try to access other account report IDs (where the current account is not part of the group/room/workspace), the report data is not available in the storage.

Do we try to access it from the server?

We don't fetch the targeted report ID in the ReportDetailsPage, ReportParticipantsPage , and ReportSettingsPage . I think we should fetch the report data where I proposed in #12428 (comment)

I'm totally lost, but the HOC solution seems very strange to me.

We handle the same things for the 3 pages, so I think it's okay to create a HOC for that.

cc @marcaaron @Beamanator

@Beamanator
Copy link
Contributor

Oh shoot I totally thought this was one of the ones holding on navigation reboot, sorry - I will jump into a plan for this tomorrow, may see if someone else will take it off my plate too 😅

@Beamanator
Copy link
Contributor

Still pretty low priority, though some higher priority projects are finishing up soon so will try to get to this one mid march

@melvin-bot melvin-bot bot removed the Overdue label Mar 1, 2023
@mateocole
Copy link

Going to be OOO, reassigning

@Beamanator
Copy link
Contributor

On hold for some higher priority tasks

@melvin-bot melvin-bot bot removed the Overdue label May 5, 2023
@melvin-bot melvin-bot bot added the Overdue label Jun 5, 2023
@Beamanator
Copy link
Contributor

still quite low priority

@melvin-bot melvin-bot bot removed the Overdue label Jun 6, 2023
@melvin-bot melvin-bot bot added the Overdue label Jul 10, 2023
@Beamanator Beamanator added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jul 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Jul 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 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

@Beamanator
Copy link
Contributor

Reassigning Bug since this "Was" on hold for navigation reboot but that went live!

@joekaufmanexpensify I think we need to reevaluate if this is still a bug / if we need to fix something here please 🙏

@joekaufmanexpensify
Copy link
Contributor

Sounds good, I'll take a look today!

@joekaufmanexpensify
Copy link
Contributor

@Beamanator I tested this, and when I try and navigate to the links in OP, I get the "Hm, it's not here" error. It seems like this is one of the expected behavior options discussed here. And then the user has the option to redirect to the home page.

image

@joekaufmanexpensify
Copy link
Contributor

Personally, I feel like this is fine. I don't think it'll be common that someone will be copying an actual report they don't have access to. What feels much more likely to me is that someone would copy a report they do have access to, but with a typo.

And the existing error sufficiently communicates that they did something wrong. At which point, they'll likely realize, and correct the typo, or navigate to the home page using the button on the screen.

If you agree, then I think we're all set to close this one!

@Beamanator
Copy link
Contributor

@joekaufmanexpensify That definitely sounds legit to me!

@joekaufmanexpensify
Copy link
Contributor

Cool cool. Closing as this has been fixed by the navigation reboot!

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 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests