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

[izarutskaya]

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:

1. Open any conversation have attachment
2. Click on download
3. 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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6474921_1715180171795.Untitled_Project.mp4

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]

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]

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

[devguest07]

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.

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Lines 495 to 501 in edec897

	shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID, isPinnedChat, isUnreadChat, isOffline): reportAction is ReportAction => {
	const isAttachment = ReportActionsUtils.isReportActionAttachment(reportAction);
	const messageHtml = getActionHtml(reportAction);
	return (
	isAttachment && messageHtml !== CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML && !!reportAction?.reportActionID && !ReportActionsUtils.isMessageDeleted(reportAction) && !isOffline
	);
	},

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.

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 40 in edec897

const isDownloading = download?.isDownloading ?? false;

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

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 56 in edec897

onLongPress={(event) => showContextMenuForReport(event, anchor, report?.reportID ?? '', action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report))}

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

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Lines 495 to 501 in edec897

	shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID, isPinnedChat, isUnreadChat, isOffline): reportAction is ReportAction => {
	const isAttachment = ReportActionsUtils.isReportActionAttachment(reportAction);
	const messageHtml = getActionHtml(reportAction);
	return (
	isAttachment && messageHtml !== CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML && !!reportAction?.reportActionID && !ReportActionsUtils.isMessageDeleted(reportAction) && !isOffline
	);
	},

What alternative solutions did you explore?

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

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

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 65 in c11ae2f

shouldShowLoadingSpinnerIcon={isDownloading}

It will show the loading icon if the attachment is downloading, it will also do not allow further download trigger

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 48 in c11ae2f

if (isDownloading || isOffline) {

However, we don't have any implementation to disable the Download button in the report action context menu when the attachment is downloading

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Line 504 in c11ae2f

onPress: (closePopover, {reportAction}) => {

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

  App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

  Line 515 in c11ae2f

  getDescription: () => {},

  , add a method shouldDisable for checking if the context menu item should be disabled

shouldDisable: (download) => {
    return download?.isDownloading ?? false
}

• In

  App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx

  Line 231 in c11ae2f
  , 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

  App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx

  Line 289 in c11ae2f

  onBlur={() => (index === filteredContextMenuActions.length - 1 || index === 1) && setFocusedIndex(-1)}

  , use shouldDisable

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

• Pass ContextMenuItem from ContextMenuItem to FocusableMenuItem

  App/src/components/ContextMenuItem.tsx

  Line 137 in c11ae2f

  onBlur={onBlur}

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 while the attachment is downloading. Like the UX of AnchorForAttachmentsOnly

Result:

Screen.Recording.2024-06-19.at.2.34.45.PM.mov

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.

[situchan]

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

[situchan]

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]

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]

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]

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]

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.

[melvin-bot]

@anmurali, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[situchan]

disable + replace icon with loading spinner

@nkdengineer's proposal looks good based on expected behavior we discussed.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

@madmax330 @anmurali @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[nkdengineer]

@situchan this PR is ready for review

[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 9.0.13-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: disabled download button when downloading #44760

If no regressions arise, payment will be issued on 2024-08-05. 🎊

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

• @situchan requires payment automatic offer (Reviewer)
• @nkdengineer 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:

• [@situchan] The PR that introduced the bug has been identified. Link to the PR:
• [@situchan] 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:
• [@situchan] 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:
• [@situchan] Determine if we should create a regression test for this bug.
• [@situchan] 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.
• [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

This issue has not been updated in over 15 days. @madmax330, @anmurali, @situchan, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

@madmax330, @anmurali, @situchan, @nkdengineer Whoops! This issue is 2 days overdue. Let's get this updated quick!

[situchan]

Waiting for payment
cc: @anmurali

[melvin-bot]

@madmax330, @anmurali, @situchan, @nkdengineer 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[anmurali]

@situchan can you please propose a regression test and otherwise complete the BZ checklist first?
@nkdengineer is paid.

[situchan]

This is not regression but improvement.
I think we can skip regression test as edge case.

[melvin-bot]

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

[melvin-bot]

@madmax330, @anmurali, @situchan, @nkdengineer 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

@madmax330, @anmurali, @situchan, @nkdengineer Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[melvin-bot]

@madmax330, @anmurali, @situchan, @nkdengineer 12 days overdue now... This issue's end is nigh!

[anmurali]

Paid.