[Needs regression test steps][$1000] Restricted access - The crash when navigate to the link https://staging.new.expensify.com/r/6821704960193694/details

[kbecciv]

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 chat
3. Navigate to the link https://staging.new.expensify.com/r/6821704960193694/details

Expected Result:

You're redirected to the concierge conversation

Actual Result:

System is crashed

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.38.0

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5858104_Recording__3246.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01342b9d79a0c15641
• Upwork Job ID: 1602317515226468352

[melvin-bot]

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

[situchan]

Proposal

This is a regression from #13398
The crash happens when report is empty json: {} so report.participants is undefined

App/src/libs/ReportUtils.js

Line 476 in 10dd188

for (let i = 0; i < report.participants.length; i++) {

Solution:
https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.js#L438

function getIcons(report, personalDetails, policies, defaultIcon = null) {
-   if (!report) {
+   if (_.isEmpty(report)) {
        return [defaultIcon || getDefaultAvatar()];
    }

[CortneyOfstad]

Was able to recreate on my side 👍

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[chiragsalian]

Removing external and help wanted labels. Normally for deploy blockers issues we try to handle them internally for speediness. If we use your solution @situchan we will compensate you.

[aldo-expensify]

Proposal

This is a regression from #13398 The crash happens when report is empty json: {} so report.participants is undefined

App/src/libs/ReportUtils.js

Line 476 in 10dd188

for (let i = 0; i < report.participants.length; i++) {

Solution: https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.js#L438

function getIcons(report, personalDetails, policies, defaultIcon = null) {
-   if (!report) {
+   if (_.isEmpty(report)) {
        return [defaultIcon || getDefaultAvatar()];
    }

Thanks @situchan !, I'm wondering why there is a report {}, maybe we should fix that?

[situchan]

Thanks @situchan !, I'm wondering why there is a report {}, maybe we should fix that?

App/src/pages/ReportDetailsPage.js

Line 123 in 1c60740

icons={ReportUtils.getIcons(this.props.report, this.props.personalDetails, this.props.policies)}

The crash happens on this line when user navigates to report detail page directly that they don't have access.
So ReportDetailsPage is rendered first before loading report data and quickly redirects to main page.
This is not avoidable case (anyone can visit any link on any browser).

Another solution is to render null before loading report data on ReportDetailsPage but I prefer above solution for safety (in case ReportUtils.getIcons is called in other pages).

[youssef-lr]

App/src/pages/ReportDetailsPage.js

Line 123 in 1c60740

icons={ReportUtils.getIcons(this.props.report, this.props.personalDetails, this.props.policies)}

The crash happens on this line when user navigates to report detail page directly that they don't have access.

I'm not sure this is true, the crash seems to happen inside ReportUtils and specifically in this line as report is set to an empty object {}. I agree with @aldo-expensify, this seems like a regression and we need to find out why is the report object empty. Maybe before, the code did not reach the stage where it renders ReportDetailsPage?

[aldo-expensify]

I think this HOC is passing the {} report:

App/src/pages/home/report/withReportOrNavigateHome.js

Line 19 in 3ef5d88

Maybe render() shouldn't render if the report was not found, specially if this is going to redirect the user "home". Investigating further.

[aldo-expensify]

Still it may be worth to change the !report for safer _.isEmpty(report) checks, we have this in a few functions

[MonilBhavsar]

I think I partially agree with @situchan on the cause. The root cause is correct but I don't think the proposed solution works. I feel like if empty report json is passed, we should just return early instead of rendering the component.

[mallenexpensify]

Sorry @eVoloshchak , I think I saw your name in the upper right of the PR but didn't look for the ✔️ showing that you had reviewed the PR.

@situchan , Aldo is out this week, I pinged our engineers who aren't out to see if anyone can give me a 👍 to pay. Hopefully it won't be long

[youssef-lr]

@mallenexpensify I can confirm we used @situchan solution in the PR.

[mallenexpensify]

Thanks @youssef-lr ! I hired you for $1000 @situchan , can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01342b9d79a0c15641

[situchan]

@mallenexpensify accepted. Thank you

[mallenexpensify]

Paid @situchan , thanks! Leaving issue open and weekly for regression test steps, cc @CortneyOfstad and @eVoloshchak (I think you should be the one doing the engineering parts Eugene). I'll try to help later this week @CortneyOfstad

[eVoloshchak]

Leaving issue open and weekly for regression test steps, cc @CortneyOfstad and @eVoloshchak (I think you should be the one doing the engineering parts Eugene)

@mallenexpensify, not sure I understand correctly how this works for internal issues, I should be here in case some regressions arise, correct?

[mallenexpensify]

@eVoloshchak , I'm unsure too, thanks for commenting. I hadn't taken into consideration Internal issues and what engineer is responsible for those. I'm going to address in #contributor-plus, it's here

[mallenexpensify]

@eVoloshchak @MonilBhavsar @aldo-expensify This one slipped through the cracks cuz the Reviewing label kept it from going overdue. Can one of you complete the top three steps here plz #13509 (comment)

I'll get to the fourth, hopefully, today.

[aldo-expensify]

Completed the first two, I'm not sure how to go about 3.

About the tests update, I'm missing if this was happened with any report / details, or if it happened only when accessing the specific report details: https://staging.new.expensify.com/r/6821704960193694/details (going to test this)

Update: from the steps in the PR #13531 it seems that we need to use a URL of a report that doesn't exists. Considering this, we need a test that opens a URL to the report's details of such report.

[aldo-expensify]

@mvtglobally 👋 do you know if we have a test that opens the url of a report's details, but of a report that doesn't exists? It should be as simple as:

1. Open https://staging.new.expensify.com/r/6666666666666/details
2. Make sure that you get redirected to a conversation and the app doesn't crash

If we don't we can add it as a test 🤷 , what do you think @mallenexpensify ?

[mallenexpensify]

@aldo-expensify is it "report that doesn't exist", one a user user doesn't have access to or both?
I did a quick search in TestRail for report redirect but couldn't find any relevant steps. Any other terms I should check?

[aldo-expensify]

@aldo-expensify is it "report that doesn't exist", one a user user doesn't have access to or both?

I honestly don't know at the moment, but I think I can test and see if it matters.

[mallenexpensify]

I founds this TestRail that likely covers it if it's an access issue. Under the top link I could add https://staging.new.expensify.com/r/6666666666666/details if we know it's a non-existent report vs one a user doesn't have access to .

1. Navigate to the following link

• https://staging.new.expensify.com/r/6821704960193694

1. Verify you're redirected to a blank conversation with a message indicating that the user doesn't have access in web and to LHN in mobile view
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

1. Verify you're redirected to the concierge conversation in Web and to the LHN in mobile view in each

[aldo-expensify]

if we know it's a non-existent report vs one a user doesn't have access to .

I tested this a bit, an a report where the user doesn't have access and a report that doesn't exists leads to the same report = {}. So it can be either.

The described tests should cover it. If the bug was there, it would crash when you try to check https://staging.new.expensify.com/r/6821704960193694/details

[mallenexpensify]

Thanks @aldo-expensify , after reviewing again, I'm unsure if we need/want to update any of the steps. Checking in #qa
https://expensify.slack.com/archives/C9YU7BX5M/p1674611981896969

[aldo-expensify]

Thanks @aldo-expensify , after reviewing again, I'm unsure if we need/want to update any of the steps. Checking in #qa
https://expensify.slack.com/archives/C9YU7BX5M/p1674611981896969

I think the test without changes is fine to cover the case.

[mallenexpensify]

Agreed, I'm going to close.
Put another way... I think the existing steps would catch the bug if it occurred.