[HOLD for payment 2024-08-05] [$250] Web - Attachment - Downloads can be requested continuously while the file is still loading

izarutskaya commented 2 months ago

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

Version Number: v1.4.82-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): nhut.nguyenminh.it+dt002@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Open any conversation have attachment
Click on download
Right click > Click on download multi times

Expected Result:

When the file is being downloaded, the download button inside the right-click menu must also be disabled to avoid users being able to click repeatedly to download the file.

Actual Result:

When clicking download, the download button will change to a loading button and the user cannot click on it, however, the download button in the right-click menu is not disabled and the user can click continuously and download repeatedly.

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[ ] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/54cea47a-d9dc-4eaa-a69b-37724419d589

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015a422bd93974d7e4
Upwork Job ID: 1803174728471774019
Last Price Increase: 2024-06-25
Automatic offers:

situchan | Reviewer | 102939368
nkdengineer | Contributor | 102939370

Issue Owner

Current Issue Owner: @anmurali

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

izarutskaya commented 2 months ago

We think this issue might be related to the #vip-vsb

devguest07 commented 2 months ago

Proposal

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

The user is able to trigger a download when the file is already downloading.

What is the root cause of that problem?

In the Context menu, we do not display the Download button based on the download state. The file can be downloaded, and the download link will still appear on the context menu.

https://github.com/Expensify/App/blob/edec89787051305c5ac23c21779f65c53f43c1fe/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L495-L501

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

In BaseAnchorForAttachmentsOnly, we have an isDownloading variable used to prevent the click action on the file when downloading.

https://github.com/Expensify/App/blob/edec89787051305c5ac23c21779f65c53f43c1fe/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx#L40

We can use the same isDownloading variable and pass it to showContextMenuForReport.

https://github.com/Expensify/App/blob/edec89787051305c5ac23c21779f65c53f43c1fe/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx#L56

In ContextMenuActions, we can add another condition to shouldShow to prevent the display when isDownloading.

https://github.com/Expensify/App/blob/edec89787051305c5ac23c21779f65c53f43c1fe/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L495-L501

What alternative solutions did you explore?

melvin-bot[bot] commented 2 months ago

@anmurali Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

nkdengineer commented 2 months ago

Proposal

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

The download button in the right-click menu is not disabled and the user can click continuously and download repeatedly.

What is the root cause of that problem?

In AnchorForAttachmentsOnly as shown in the OP video https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx#L65 It will show the loading icon if the attachment is downloading, it will also do not allow further download trigger https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx#L48

However, we don't have any implementation to disable the Download button in the report action context menu when the attachment is downloading https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L504

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

Add implementation to disable the Download button in the report action context menu when the attachment is downloading.

In https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L515, add a method shouldDisable for checking if the context menu item should be disabled
```
shouldDisable: (download) => {
return download?.isDownloading ?? false
}
```
In https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L231, connect to the download key of the source of the attachment
```
const html = getActionHtml(reportAction);
const {sourceURL} = getAttachmentDetails(html);
const sourceID = (sourceURL?.match(CONST.REGEX.ATTACHMENT_ID) ?? [])[1];
```

const [download] = useOnyx(${ONYXKEYS.COLLECTION.DOWNLOAD}${sourceID});

(`getActionHtml` should be exported from `src/pages/home/report/ContextMenu/ContextMenuActions.tsx`)
- In https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L289, use `shouldDisable`

disabled={contextAction?.shouldDisable ? contextAction?.shouldDisable(download) : false}

- Pass `ContextMenuItem` from `ContextMenuItem` to `FocusableMenuItem` https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/components/ContextMenuItem.tsx#L137

disabled={disabled}


- Optional: Customize the cursor to be `cursor: wait` so it's clear it's disabled because it is loading. Or we can go further by customizing the icon of the `Download` context menu item to [use loading indicator](https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/components/Attachments/AttachmentView/DefaultAttachmentView/index.tsx#L54) while the attachment is downloading. Like the [UX of AnchorForAttachmentsOnly](https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx#L48)

Result:

https://github.com/Expensify/App/assets/161821005/2e285dc9-003f-4589-b17c-602696eee01f

### What alternative solutions did you explore? (Optional)
Other UXs can be suggested: Changing the action menu text to `Downloading` when it's downloading, ...

The problem could exist in similar case such as mini context menu, ... can be fixed like described.

<!---
ATTN: Contributor+

You are the first line of defense in making sure every proposal has a clear and easily understood problem with a "root cause". Do not approve any proposals that lack a satisfying explanation to the first two prompts. It is CRITICALLY important that we understand the root cause at a minimum even if the solution doesn't directly address it. When we avoid this step we can end up solving the wrong problems entirely or just writing hacks and workarounds.

Instructions for how to review a proposal:

1. Address each contributor proposal one at a time and address each part of the question one at a time e.g. if a solution looks acceptable, but the stated problem is not clear then you should provide feedback and make suggestions to improve each prompt before moving on to the next. Avoid responding to all sections of a proposal at once. Move from one question to the next each time asking the contributor to "Please update your original proposal and tag me again when it's ready for review".

2. Limit excessive conversation and moderate issues to keep them on track. If someone is doing any of the following things please kindly and humbly course-correct them:

- Posting PRs.
- Posting large multi-line diffs (this is basically a PR).
- Skipping any of the required questions.
- Not using the proposal template at all.
- Suggesting that an existing issue is related to the current issue before a problem or root cause has been established.
- Excessively wordy explanations.

3. Choose the first proposal that has a reasonable answer to all the required questions.
-->

situchan commented 2 months ago

@devguest07 can you please share demo video after applying your solution?

situchan commented 2 months ago

If I understand correctly, @devguest07 suggested to hide Download button, while @nkdengineer suggested to disable Download button.

@Expensify/design which pattern is correct? hide or disable?

shawnborton commented 2 months ago

Hmm I would think we would replace the icon in the right click menu's Download row with a loading spinner as well.

That being said, this is an edge-case, I doubt real users are going to be doing this action.

dannymcclain commented 2 months ago

Hmm I would think we would replace the icon in the right click menu's Download row with a loading spinner as well.

Yeah I'd say disable + replace icon with loading spinner. But I also agree with that this is an edge-case.

nkdengineer commented 2 months ago

Yeah I'd say disable + replace icon with loading spinner.

@situchan Looks like everyone agrees to go with my suggestion, I think we're good to move forward here

Add implementation to disable the Download button in the report action context menu when the attachment is downloading.

Or we can go further by customizing the icon of the Download context menu item to use loading indicator while the attachment is downloading. Like the UX of AnchorForAttachmentsOnly

shawnborton commented 2 months ago

That being said, if we do decide to keep this one open, let's lower the bug bounty because it's such an edge case that not a single customer has ever reported yet.