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

Opening a unknown report from the URL displays a blank chat page and no error - Reported by: @parasharrajat #5897

Closed
isagoico opened this issue Oct 15, 2021 · 30 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2

Comments

@isagoico
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Log in with any account
  2. Navigate to a conversation
  3. In the URL, change the report number to something random

Expected Result:

There should be an error about being unable to navigate to that report and user should be redirected to last chat.

Actual Result:

Blank conversation is displayed.

Workaround:

N/A.

Platform:

Where is this issue occurring?

  • Web
  • Mobile Web

Version Number: 1.1.8-0

Reproducible in staging?: Yes
Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

image

Expensify/Expensify Issue URL:

Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1634244884298600

View all open jobs on GitHub

@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

Proposal

  1. Current code handles the empty param but it does not handle reports that do not exist.

    if (_.isNaN(reportID)) {

  2. We should check if the report prop contains data such as reportID. If it does not have that data it means that report does not exists and then we should run the same logic as it does now.

  3. I don't think a call to the backend is needed to check if the report exists are all the reports are synced on Onyx before the screen is loaded.

@NikkiWines
Copy link
Contributor

👍 Looks good for UpWork, I agree with @parasharrajat. I don't think we need a call to the backend for this.

@NikkiWines NikkiWines added the External Added to denote the issue can be worked on by a contributor label Oct 15, 2021
@MelvinBot
Copy link

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

@NikkiWines NikkiWines removed their assignment Oct 15, 2021
@MelvinBot
Copy link

@trjExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@trjExpensify
Copy link
Contributor

Oops, seemed to have missed this. Sorry! Removed the n6-hold. Upwork job is here.

@MelvinBot MelvinBot removed the Overdue label Oct 19, 2021
@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Oct 19, 2021
@MelvinBot
Copy link

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

@deetergp
Copy link
Contributor

Let's go with @parasharrajat proposal. The this.props.report.reportID property is present for valid reportIDs and not present for invalid ones so it seems like a solid check and the right place to do it.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 28, 2021
@MelvinBot MelvinBot removed the Overdue label Oct 28, 2021
@botify botify changed the title Opening a unknown report from the URL displays a blank chat page and no error - Reported by: @parasharrajat [HOLD for payment 2021-11-04] Opening a unknown report from the URL displays a blank chat page and no error - Reported by: @parasharrajat Oct 28, 2021
@parasharrajat
Copy link
Member

Ok. Thanks @Beamanator . I will look into different ways.

@parasharrajat
Copy link
Member

Haven't found anything on the first look. I will try to find one as soon as possible. Def, not a priority.

@deetergp
Copy link
Contributor

No updates to report.

@deetergp
Copy link
Contributor

deetergp commented Jan 5, 2022

No updates. Since this is apparently not a priority, I'm going to bump it down to Monthly.

@deetergp
Copy link
Contributor

deetergp commented Feb 7, 2022

Where are we at with this? Is it still an issue?

@MelvinBot MelvinBot removed the Overdue label Feb 7, 2022
@parasharrajat
Copy link
Member

parasharrajat commented Feb 7, 2022

I think it is. I will try to find a solution to this problem. Adding to my Todo...

@trjExpensify
Copy link
Contributor

Any movement here?

@parasharrajat
Copy link
Member

Nope. We should export it again. I don't see any way to fix it atm. You can remove the report by me from the issue details and just reopen it for the start price. I can review the proposals as C+.

@melvin-bot melvin-bot bot added the Overdue label Apr 7, 2022
@deetergp
Copy link
Contributor

deetergp commented Apr 8, 2022

@trjExpensify Let's put this one back in the Exported pool and see if we get another bite.

@melvin-bot melvin-bot bot removed the Overdue label Apr 8, 2022
@trjExpensify
Copy link
Contributor

👋 Sorry for the slow response, I've been OoO. Going back to the OP for a sec:

Action Performed:
Log in with any account
Navigate to a conversation
In the URL, change the report number to something random

Expected Result:
There should be an error about being unable to navigate to that report and user should be redirected to last chat.

I follow those steps and I get this error message when changing the reportID to something random. I'm navigated back to the reportID I was previously on before amending it:

image

So what's the bug here, this is the expected result no? If that's the case, then I think we close this issue out.

@parasharrajat
Copy link
Member

Yup, if this is working then we can close it.

@trjExpensify
Copy link
Contributor

So to confirm, that is the expected result here right? Do you want to just confirm the same result as I got above and then we can close it out.

@tomivs
Copy link
Contributor

tomivs commented Apr 23, 2022

So to confirm, that is the expected result here right? Do you want to just confirm the same result as I got above and then we can close it out.

That's right, @trjExpensify. If you check App/src/libs/actions/Reports.js (line 385-390), you'll see that this scenario is already being handled using handleInaccessibleReport().

.catch((err) => {
if (err.message !== CONST.REPORT.ERROR.INACCESSIBLE_REPORT) {
return;
}
// eslint-disable-next-line no-use-before-define
handleInaccessibleReport();
});

And that function is defined as follows (line 1500-1506):

App/src/libs/actions/Report.js

Lines 1500 to 1506 in ad637d3

/**
* Handle the navigation when report is inaccessible
*/
function handleInaccessibleReport() {
Growl.error(Localize.translateLocal('notFound.chatYouLookingForCannotBeFound'));
navigateToConciergeChat();
}

So, it will throw an error and redirect to Concierge (as you showed). In my opinion, this issue can be closed.

@trjExpensify
Copy link
Contributor

Awesome, sounds good! Closing it out 👍

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

No branches or pull requests

9 participants