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

[Due for payment 2025-02-18] [$500][not here] Prevent "Hmm... It's not here" when logging in by immediately navigating to a report user has access to #55721

Open
Beamanator opened this issue Jan 24, 2025 · 30 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@Beamanator
Copy link
Contributor

Beamanator commented Jan 24, 2025

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


Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: ?
Slack conversation #quality:

Action Performed:

  1. Get a report ID from an existing account A (can be concierge report, or chat, expense, whatever)
  2. Log out of Account A
  3. Navigate to the NewDot report link from above while logged out
  4. Log in with Account B that doesn't have access to that report ^

Expected Result:

I'd expect your most recently accessed report, or maybe even Concierge report, would be open instead of being greeted with "Hmm... it's not here"

Actual Result:

See that after you log in, you immediately are greeted with "Hmm... it's not here"
- Example logs from my expensifail account

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021882791476183455793
  • Upwork Job ID: 1882791476183455793
  • Last Price Increase: 2025-01-28
  • Automatic offers:
    • OmarKoueifi | Contributor | 105922405
Issue OwnerCurrent Issue Owner: @muttmuure
@Beamanator Beamanator added Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. labels Jan 24, 2025
Copy link

melvin-bot bot commented Jan 24, 2025

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

@melvin-bot melvin-bot bot changed the title Prevent "Hmm... It's not here" when logging in [$250] Prevent "Hmm... It's not here" when logging in Jan 24, 2025
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 24, 2025
Copy link

melvin-bot bot commented Jan 24, 2025

Current assignee @muttmuure is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 24, 2025
Copy link

melvin-bot bot commented Jan 24, 2025

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

Copy link

melvin-bot bot commented Jan 24, 2025

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 24, 2025
@Beamanator Beamanator changed the title [$250] Prevent "Hmm... It's not here" when logging in Prevent "Hmm... It's not here" when logging in by immediately navigating to a report you have access to Jan 24, 2025
Copy link

melvin-bot bot commented Jan 24, 2025

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

@Beamanator Beamanator changed the title Prevent "Hmm... It's not here" when logging in by immediately navigating to a report you have access to Prevent "Hmm... It's not here" when logging in by immediately navigating to a report user has access to Jan 24, 2025
@OmarKoueifi
Copy link
Contributor

Contributor details
Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~016c1a735b187cb976

@melvin-bot melvin-bot bot added the Overdue label Jan 27, 2025
Copy link

melvin-bot bot commented Jan 27, 2025

📣 @OmarKoueifi! 📣
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. Make sure you've read and understood the contributing guidelines.
  2. 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.
  3. 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.
  4. 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>

@OmarKoueifi
Copy link
Contributor

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~016c1a735b187cb976

Copy link

melvin-bot bot commented Jan 27, 2025

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@OmarKoueifi
Copy link
Contributor

Proposal

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

The issue arises when a user navigates to a report they do not have access to (e.g., due to permission issues or the report being restricted). Instead of a smooth fallback mechanism, the user currently sees a message that says: "Hmm... it's not here. You don't have access to this chat".

What is the root cause of that problem?

The root cause lies in the openReportFromDeepLink function in src/libs/actions/Report.ts. After sign-in/sign-up, this function attempts to navigate to the report without first verifying the user's access. Consequently, the app loads the report page, and only then checks permissions, resulting in the error message being displayed.

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

  1. Modify the handleDeeplinkNavigation logic inside waitForUserSignIn().then to include a pre-check for report access:
    • Use const report = getReport(reportID); to fetch the report details.
    • If report is undefined, it indicates the user does not have access to the report.
  2. Implement a fallback mechanism:
    • If the user lacks access, navigate them to a more appropriate screen, such as:
      • The Home Screen
      • The "Recently Accessed Reports" screen
      • The "Concierge Report" (depending on team preference).
    • Ensure valid deep links continue to function as expected for users with the appropriate permissions.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  1. Public Report Access:
    • Verify that users signing in with a public room link are able to access the report without issues.
  2. Private Report Access:
    • Verify that users signing in with a private room link, which they have permission to access, are able to open the report successfully.
  3. Restricted Access:
    • Verify that users signing in with a private room link, which they do not have permission to access, are redirected to the appropriate fallback screen (e.g., Home Screen or Concierge Report) instead of seeing the error message: "Hmm... it's not here. You don't have access to this chat."

What alternative solutions did you explore? (Optional)

NA

@ijmalik
Copy link
Contributor

ijmalik commented Jan 27, 2025

Proposal

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

Prevent the "Hmm... It's not here" message during login by promptly navigating the user to a report they can access

What is the root cause of that problem?

New requirement

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

Introduce a new prop, redirectLastAccessedReport, in the FullPageNotFoundView component. This prop will handle redirection to the most recently accessed report when enabled.

add following code at

    if (redirectLastAccessedReport) {
        const lastAccessedReport = findLastAccessedReport(false, false, '');
        const lastAccessedReportID = lastAccessedReport?.reportID;
        const lastAccessedReportRoute = ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID);
        Navigation.navigate(lastAccessedReportRoute);
        return null;
    }

And when calling FullPageNotFoundView, pass redirectLastAccessedReport as true for redirection where required

Examples:
pass redirectLastAccessedReport prop at
1-

shouldShow={shouldShowNotFoundPage}

redirectLastAccessedReport = {shouldShowNotFoundPage}

2-

<FullPageNotFoundView shouldShow={shouldShowNotFoundPage}>

<FullPageNotFoundView shouldShow={shouldShowNotFoundPage} redirectLastAccessedReport = {shouldShowNotFoundPage}>

3-

redirectLastAccessedReport = {shouldShowNotFoundPage}

4-

redirectLastAccessedReport = {shouldShow}

5-

<FullPageNotFoundView shouldShow={shouldShowBlockingView}>

<FullPageNotFoundView shouldShow={shouldShowBlockingView} redirectLastAccessedReport = {shouldShowBlockingView}>

6-

            <FullPageNotFoundView
                shouldShow
                redirectLastAccessedReport

Extend to Other Components
Similarly, we can analyze and optimize other components, such as AccessOrNotFoundWrapper, using the same approach applied to FullPageNotFoundView, if necessary.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Automated Test Scenarios
To ensure the issue does not reoccur, automated tests should comprehensively cover the following scenario:

Validate that upon login:
The application opens either:
The most recently accessed report available to the user.
Or the Concierge report (as a fallback).
Ensure that the user is not greeted with the "Hmm... It's not here" message.
This testing ensures proper redirection behavior and eliminates the risk of inaccessible reports disrupting the user experience.

Note:
The code will be further optimized during the PR process.

I have tested the following links, and they are functioning correctly after the changes.

55721-testCases-1.txt

Videos:

Before:

55721-Before-1.mp4

After:

55721-After-1.mp4

@Beamanator
Copy link
Contributor Author

@rushatgabhane looks like you've got a few proposals to review :D

@melvin-bot melvin-bot bot removed the Overdue label Jan 28, 2025
@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 28, 2025

hellooou! i don't think the above proposals are exactly what we want.

this is the expected outcome -

  1. In incognito, go to any report that is invalid - new.expensify.com/r/1234. this will redirect you to login page.
  2. login
  3. you will now be redirected to r/1234. OpenReport command has returned 404.
  4. so we should now redirect the user to last visited report/path or concierge.

Login is the only case where you should be redirected. If you're already logged in and visit an invalid report, then you should see the 404 page.

cc @ijmalik @OmarKoueifi

@ranjan-purbey
Copy link

@rushatgabhane how is the "last visited route" determined? Do we store the current route/screen on each navigation for logged-in users?
Or did you mean "last visited report"? If so, do we store the last-accessed timestamp for each report?

Also, should this behavior only be applied in case where the user doesn't have permission to view a valid report ID? Or should it also apply for invalid/non-existent report IDs?

@rushatgabhane
Copy link
Member

i could be wrong, but we only store last visited route. it is stored as a NVP (name value pair) Last_Visited_Path

@ranjan-purbey
Copy link

i could be wrong, but we only store last visited route. it is stored as a NVP (name value pair) Last_Visited_Path

I believe we can also get the last accessed report using ReportUtils.findLastAccessedReport

@OmarKoueifi
Copy link
Contributor

@rushatgabhane

Thanks for clarifying the expected behavior! I believe my proposed solution already aligns with the desired outcome. Let me explain how my changes ensure this:

1. What I did:

I introduced the canAccessReport function within the openReportFromDeepLink logic. This function checks whether the user has access to the report by fetching the report using getReport(reportID) it will returns a boolean.

2. How the logic flows:

  • After waitForUserSignIn() resolves (indicating the user has signed in), the handleDeeplinkNavigation function is invoked.
  • Before navigating to the requested report, I use the canAccessReport function to check if the user has access:
if (!canAccessReportPage(reportID)) {
    Navigation.navigate(ROUTES.HOME);
    return;
}
  • If the report is inaccessible, the user is redirected to the Home screen. Otherwise, the app proceeds to navigate to the requested report.

3. Why this matches the expected behavior:

  • When signing in with a valid report link: If the report is valid and accessible, the user is navigated to the report as expected.
1.When.signing.in.with.a.valid.report.link.mov
  • When signing in with an invalid report link: If the report is invalid, the user is redirected to the fallback location (e.g., Home, Concierge, or recently accessed report).
2.When.signing.in.with.an.invalid.report.link.mov
  • When already signed in: The current flow does not interfere with logged-in users. If they navigate to an invalid report, they will still see the intended "Hmm... It's not here" message.
3.When.already.signed.in.mov
  • Public report
public.report.mov

Additional Clarification

Currently, it seems that the existing behavior (without my code changes) redirects the user to the last report they interacted with or sent a message to. This happens automatically as part of the app’s navigation logic, only when signing in.

Should we keep this behavior, or should we explicitly redirect the user to:

  • Concierge
  • Recently Accessed Reports

I also tested ReportUtils.findLastAccessedReport(), and it returned the last report the user interacted with, which matches the current behavior. Let me know if we should override this or keep it as is.

@rushatgabhane
Copy link
Member

give @OmarKoueifi the 💰 already! @Beamanator their proposal LGTM

🎀 👀 🎀

Copy link

melvin-bot bot commented Jan 29, 2025

Current assignee @Beamanator is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@rushatgabhane
Copy link
Member

@OmarKoueifi i think we should also add Concierge as a fallback. If findLastAccessedReport is null, then we should take them to concierge.

@OmarKoueifi
Copy link
Contributor

Sounds good, will do.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

📣 @OmarKoueifi 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@OmarKoueifi
Copy link
Contributor

The PR will be ready tomorrow end of day or Monday morning, depending on the test results.

OmarKoueifi added a commit to OmarKoueifi/App that referenced this issue Feb 3, 2025
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 3, 2025
Beamanator added a commit that referenced this issue Feb 5, 2025
@mallenexpensify mallenexpensify changed the title [$500] Prevent "Hmm... It's not here" when logging in by immediately navigating to a report user has access to [$500][not here] Prevent "Hmm... It's not here" when logging in by immediately navigating to a report user has access to Feb 10, 2025
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 11, 2025
@melvin-bot melvin-bot bot changed the title [$500][not here] Prevent "Hmm... It's not here" when logging in by immediately navigating to a report user has access to [Due for payment 2025-02-18] [$500][not here] Prevent "Hmm... It's not here" when logging in by immediately navigating to a report user has access to Feb 11, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Feb 11, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.95-6 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 2025-02-18. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Feb 11, 2025

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@rushatgabhane] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

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 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2
Projects
Status: CRITICAL
Development

No branches or pull requests

7 participants