Open Beamanator opened 2 weeks ago
Job added to Upwork: https://www.upwork.com/jobs/~021882791476183455793
Current assignee @muttmuure is eligible for the NewFeature assigner, not assigning anyone new.
:warning: 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 :warning:
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External
)
Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature
)
Contributor details Expensify account email: omar.koueifi@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~016c1a735b187cb976
๐ฃ @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:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Contributor details Your Expensify account email: omar.koueifi@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~016c1a735b187cb976
โ Contributor details stored successfully. Thank you for contributing to Expensify!
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".
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.
handleDeeplinkNavigation
logic inside waitForUserSignIn().then
to include a pre-check for report access:
const report = getReport(reportID);
to fetch the report details.report
is undefined
, it indicates the user does not have access to the report.NA
Prevent the "Hmm... It's not here" message during login by promptly navigating the user to a report they can access
New requirement
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 https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/components/BlockingViews/FullPageNotFoundView.tsx#L64
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- https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/pages/home/ReportScreen.tsx#L793
redirectLastAccessedReport = {shouldShowNotFoundPage}
<FullPageNotFoundView shouldShow={shouldShowNotFoundPage} redirectLastAccessedReport = {shouldShowNotFoundPage}>
redirectLastAccessedReport = {shouldShowNotFoundPage}
redirectLastAccessedReport = {shouldShow}
<FullPageNotFoundView shouldShow={shouldShowBlockingView} redirectLastAccessedReport = {shouldShowBlockingView}>
<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.
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.
Videos:
Before:
https://github.com/user-attachments/assets/3a922fa7-e325-4308-8136-25fbf6000b30
After:
https://github.com/user-attachments/assets/939646a3-b44b-44a3-8151-4bd730652530
@rushatgabhane looks like you've got a few proposals to review :D
@shawnborton, @Beamanator, @rushatgabhane, @muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
โ ๏ธ This issue has had its price increased by 4x or more. Please review the issue and ensure the price is correct.
Upwork job price has been updated to $500
hmm... let me clarify expected outcome here on slack
hellooou! i don't think the above proposals are exactly what we want.
this is the expected outcome -
new.expensify.com/r/1234
. this will redirect you to login page.r/1234
. OpenReport
command has returned 404.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
@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?
i could be wrong, but we only store last visited route. it is stored as a NVP (name value pair) Last_Visited_Path
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
@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:
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.
waitForUserSignIn()
resolves (indicating the user has signed in), the handleDeeplinkNavigation
function is invoked.canAccessReport
function to check if the user has access:
if (!canAccessReportPage(reportID)) {
Navigation.navigate(ROUTES.HOME);
return;
}
https://github.com/user-attachments/assets/0784ea20-19ef-416a-a6c9-5b8fd263971b
https://github.com/user-attachments/assets/a1ed0534-c598-4aff-b180-429f32cf0bfd
https://github.com/user-attachments/assets/542191a8-92e7-4e97-86c6-108ba15a2005
https://github.com/user-attachments/assets/1fa3228e-5a68-4ef7-9cfe-da912454c47f
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:
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.
give @OmarKoueifi the ๐ฐ already! @Beamanator their proposal LGTM
๐ ๐ ๐
Current assignee @Beamanator is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
@OmarKoueifi i think we should also add Concierge as a fallback. If findLastAccessedReport
is null, then we should take them to concierge.
Sounds good, will do.
๐ฃ @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 ๐
The PR will be ready tomorrow end of day or Monday morning, depending on the test results.
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.95-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2025-02-18. :confetti_ball:
For reference, here are some details about the assignees on this issue:
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:
This is a new feature. Regression steps:
Expected Result: The app should redirect to your most recently accessed report (or the Concierge report) rather than displaying "Hmm... it's not here".
Expected Result: The app should display "Hmm... it's not here" to indicate access is not permitted.
Expected Result: The report opens normally without any redirection or error messages.
If you havenโt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: ? Slack conversation #quality:
Action Performed:
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"
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @muttmuure