[HOLD for payment 2024-06-13] [$500] Infinite loading spinner when trying to open a link to an Image that is in a room I have no access to.

[kavimuru]

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. Open any chat room and send an image
2. Open the image and copy the link of the image
3. Login as a user who DOESN'T have access to that room
4. Navigate to the link you copied

Expected Result:

An error page should appear for the user that he doesn't have access to that attachment because he doesn't have access to its room.

Actual Result:

An infinite spinner for the attachment loading appears, and nothing to tell the user that he can't open that attachment.
After closing it, the user got "Hmm... it's not here You don't have access to this chat'', and if he navigate to the image link again he got the "Uh-oh, something went wrong!" page

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.44-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screencast.from.19-07-23.21_28_35.webm

Recording.1317.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Ahmed-Abdella
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689814964693879

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01440acd5fcf557a24
• Upwork Job ID: 1683569021878947840
• Last Price Increase: 2024-05-08
• Automatic offers:
• Ahmed-Abdella | Contributor | 0

Issue Owner

Current Issue Owner: @greg-schroeder

[melvin-bot]

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

[melvin-bot]

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

[StevenKKC]

Proposal

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

Infinite loading spinner when trying to open a link to an Image that is in a room I have no access to.

What is the root cause of that problem?

For the attachment that have no access to, app pass empty file.name prop to AttachmentView.
But in AttachmentView, there is not check whether file.name is empty.
So the infinite loading spinner is shown.

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

We should add checking whether file.name is empty in AttachmentView component, and if is empty, return NotFoundView.

function AttachmentView(props) {
    ...
    if (props.file.name === '') {
        return <NotFoundPage />;
    }
    ...

What alternative solutions did you explore? (Optional)

None.

[greg-schroeder]

Will review

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[MitchExpensify]

I believe my auto-assignment was buggy here. Greg is already on it so unassigning myself 👍

[DanutGavrus]

@greg-schroeder
First Proposal from #23068 fixes this issue too.

[Ahmed-Abdella]

@greg-schroeder @DanutGavrus They are two different cases, The other issue here (#23068) is just the second half of this issue, I mentioned two issues here. The first one is Trying to open an attachment from a room that you don't have access to which leads to infinite Loading. This issue isn't mentioned there as it covers the attachment from a room you have access to, which takes you to the "something went wrong" page. And this is the second part of this issue, which happens when you try to open the link again after closing the infinite-loading page

[daanish-shaikh]

Proposal

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

Infinite loading spinner visible on attachment modal, when a chat attachment url is directly used and the user logged in at that moment doesn't have access to that chat.

What is the root cause of that problem?

There is no chat access check or handling at AttachmentModal.js.

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

Inorder to resolve this we can utilize FullPageNotFoundView wrapper component.

1. Add import FullPageNotFoundView from ../components/BlockingViews/FullPageNotFoundView';, here.
2. Modify shouldShowDownloadButton props of HeaderWithBackButton to not show download icon, here.
3. Wraps following content from Line 320 to Line 358 with FullPageNotFoundView wrapper component.

<FullPageNotFoundView
    shouldShow={!props.report.reportID && !props.report.isLoadingReportActions}
    subtitleKey="notFound.noAccessToAttachment"
    shouldShowCloseButton={false}
    shouldShowBackButton={props.isSmallScreenWidth}
>
...
</FullPageNotFoundView>

4. Add "You don't have access to this attachment" text in translation files.
   src/languages/en.js

notFound: {
    chatYouLookingForCannotBeFound: 'The chat you are looking for cannot be found.',
    getMeOutOfHere: 'Get me out of here',
    iouReportNotFound: 'The payment details you are looking for cannot be found.',
    notHere: "Hmm... it's not here",
    pageNotFound: 'Oops, this page cannot be found',
    noAccess: "You don't have access to this chat",
    noAccessToAttachment: "You don't have access to this attachment",
    goBackHome: 'Go back to Home page',
}

src/languages/es.js

notFound: {
    chatYouLookingForCannotBeFound: 'El chat que estás buscando no se pudo encontrar.',
    getMeOutOfHere: 'Sácame de aquí',
    iouReportNotFound: 'Los detalles del pago que estás buscando no se pudieron encontrar.',
    notHere: 'Hmm… no está aquí',
    pageNotFound: 'Ups, no deberías estar aquí',
    noAccess: 'No tienes acceso a este chat',
    noAccessToAttachment: 'No tienes acceso a este adjunto',
    goBackHome: 'Volver a la página principal',
},

[parasharrajat]

We should be putting this on hold @greg-schroeder based on the conversation on #23068 (comment). Right?

[greg-schroeder]

Yes, doing

[parasharrajat]

Opened the same question on slack https://expensify.slack.com/archives/C01GTK53T8Q/p1716284156418899.

[greg-schroeder]

Yeah, I'm really not sure so I'll defer to that conversation in Slack

[greg-schroeder]

It seems so far people think the behavior is fine

[parasharrajat]

@greg-schroeder Let's see if we can push the discussion on slack to get a few more votes.

[parasharrajat]

I didn't get good participation on the discussion but I think we are fine with this behaviour so I am moving forward with the PR>

[melvin-bot]

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

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.79-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Display blocking view and remove download button when user have no access to the report #42312

If no regressions arise, payment will be issued on 2024-06-13. 🎊

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

• @parasharrajat requires payment through NewDot Manual Requests
• @Ahmed-Abdella requires payment automatic offer (Contributor)
• @Ahmed-Abdella requires payment automatic offer (Contributor)

[melvin-bot]

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

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[parasharrajat]

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

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Present fro the start.
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: NA
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not needed
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

1. Open any chat room and send an image
2. Open the image and copy the URL.
3. Login as a different user who DOESN'T have access to that room
4. Navigate to the link you copied.
5. Verify that you see Not Found page for the attachment too.

Do you agree 👍 or 👎 ?

[kidroca]

@parasharrajat I don't agree that the PR which introduced the bug was identified correctly. Additionally, you didn't provide an explanation for your conclusion.

The endless loading bug was occurring long before the linked PR. The issue lies within the AttachmentView component used in the Carousel items, which is configured to render endless loading when no fallbackSource is provided.

App/src/components/Attachments/AttachmentView/index.tsx

Lines 217 to 218 in 569b7c1

	<AttachmentViewImage
	url={imageError && fallbackSource ? (fallbackSource as string) : (source as string)}

fallbackSource is optional and there's no default value. In its absence, nothing changes, leading to an endless spinner instead of error handling.

The endless loading bug would still occur for any network error.

I believe addressing this underlying issue will provide a more robust solution.

[parasharrajat]

This issue is related to inaccessible reports. So when you don't have access to the report, we shouldn't render the attachment at all. Thus I don't see how providing a fallback to the attachmentView component will help with this issue.

I saw that you added a new route for attachment and thus I thought this is a valid use case to be handled with routing.

But you might be correct, I can be wrong with the root cause. I will try to find the older PRs responsible.

[kidroca]

I don't think it was possible to have a link to a specific attachment in the past and while the recent PR may have surfaced the issue, it's more accurate to address the underlying component behavior.

I believe it would be more accurate to render the attachment carousel regardless of the end result. Route parameters match the attachment route, and the user probably expects to see an attachment. In case of an error, like when they don't have access, they'll see the attachment frame and the fallback/error content. The fallback can be further enhanced to display a message based on the network error.

[parasharrajat]

That's what we are showing so far. The attachment modal is shown but instead of an image, it shows not found paragraph. Not found content is the fallback here.

[parasharrajat]

Updated the checklist to remove the PR responsible. This issue seemed to be present from the start. The app was either crashing or showing a continuous loader when the image was not loaded anyhow.

[greg-schroeder]

Processing

[greg-schroeder]

Payment summary:

Contributor: @Ahmed-Abdella - $500 - Paid via Upwork
C+: @parasharrajat - $500 - You can make a ND manual request

[greg-schroeder]

Filed regression test, closing

[parasharrajat]

Payment requested as per #23374 (comment)

[JmillsExpensify]

$500 approved for @parasharrajat