[HOLD for payment 2024-02-07] [$500] Chat - When user goes offline, the download button do not disappear for other file types

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.32-2
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Pre-requisite: the user must be logged in and have sent an attachment that is not an image or PDF, such as .mp4 or .doc files

1. Go to the chat where you have sent the attachment (.mp4, .doc, etc.)
2. Go offline

Expected Result:

The download button for other type of files should disappear, as it does for images and PDF

Actual Result:

The download button does not disappear when the user goes offline

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

Add any screenshot/video evidence

Bug6355191_1706220113373.Twhp0840_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c2f068b75b4b0950
• Upwork Job ID: 1750641360532508672
• Last Price Increase: 2024-01-25
• Automatic offers:
• ishpaul777 | Reviewer | 28123937
• Krishna2323 | Contributor | 28123938

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01c2f068b75b4b0950

[melvin-bot]

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

[melvin-bot]

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

[Krishna2323]

Proposal

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

Chat - When user goes offline, the download button do not disappear for other file types

What is the root cause of that problem?

We don't have logic to make download button disappear inside BaseAnchorForAttachmentsOnly

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

We need to pass shouldShowDownloadIcon={!isOffline} to AttachmentView and should return from onPress calback.

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Lines 41 to 44 in 8cc80e6

	onPress={() => {
	if (isDownloading) {
	return;
	}

To:

if (isDownloading || isOffline) {
    return;
}

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 59 in 8cc80e6

shouldShowDownloadIcon

To:
shouldShowDownloadIcon={!isOffline}

Note: Even if we decide to fix it in AttachmentView, we need to disable onPress from AttachmentViewPdf & AttachmentViewImage because just hiding the download button won't do anything. We also need to pass onPress to AttachmentView if we want to save if the image/pdf is clicked, like the current behaviour.

Also, we already handle HeaderWithBackButton correctly using shouldShowDownloadButton, so no need to do something there:

App/src/components/AttachmentModal.js

Line 447 in 919aadb

shouldShowDownloadButton = props.allowDownload && isDownloadButtonReadyToBeShown && !props.isReceiptAttachment && !isOffline;

App/src/components/AttachmentModal.js

Line 474 in 919aadb

shouldShowDownloadButton={shouldShowDownloadButton}

App/src/pages/wallet/WalletStatementPage.js

Line 104 in 919aadb

shouldShowDownloadButton={!props.network.isOffline || props.walletStatement.isGenerating}

We can also show default cursor when user is offline, we can do it by overriding the cursor style or by editing role
on PressableWithoutFeedback .

Result

Alternative

We can only disable download without hiding the download icon. We can wrap it with OfflineWithFeedback.

[abzokhattab]

Proposal

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

When user goes offline, the download button do not disappear for other file types

What is the root cause of that problem?

we dont disable download for the attachment view when offline here:

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Lines 55 to 61 in 9f6179a

	<AttachmentView
	// @ts-expect-error TODO: Remove this once AttachmentView (https://github.com/Expensify/App/issues/25150) is migrated to TypeScript.
	source={sourceURLWithAuth}
	file={{name: displayName}}
	shouldShowDownloadIcon
	shouldShowLoadingSpinnerIcon={isDownloading}
	/>

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

we need to use useNetwork to get the connection status .. then we need to modify the shouldShowDownloadIcon and the shouldShowLoadingSpinnerIcon as follow:

 <AttachmentView
                        // @ts-expect-error TODO: Remove this once AttachmentView (https://github.com/Expensify/App/issues/25150) is migrated to TypeScript.
                        source={sourceURLWithAuth}
                        file={{name: displayName}}
                        shouldShowDownloadIcon={!isOffline}
                        shouldShowLoadingSpinnerIcon={isDownloading && !isOffline}
                    />

Alternatively:

we can update the attachmentView itself so that it doesnt show the icon whenever the connection is offline

we need to change this line to

            {!shouldShowLoadingSpinnerIcon && shouldShowDownloadIcon && !isOffline && (

and this one as well to:

            {shouldShowLoadingSpinnerIcon &&  !isOffline  && (

[Krishna2323]

Proposal Updated

Added code snippets.

[tienifr]

Proposal

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

The download button does not disappear when the user goes offline

What is the root cause of that problem?

We do not hide/disable download button when the network is offline

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

This problem happens in many places that we need to fix all

1. For attachment in report, we should fix it in AttachmentView to avoid unexpected bugs like that in the future by adding isOffline
2. In HeaderWithBackButton, add isOffline to here
3. Do the same with ShareCodePage

What alternative solutions did you explore? (Optional)

NA

[Krishna2323]

Proposal Updated

Added a note if we decide to fix it in AttachmentView.

[ishpaul777]

Thank you everyone for proposal, its seems all cases are handled except for Message attachemnents.

@Krishna2323's Proposal looks good, just a quick question should the cursor still be pointer, if the component is not interactable when offline?

Screen.Recording.2024-01-26.at.7.22.23.PM.mov

[Krishna2323]

@ishpaul777, if we want, we can override the hand cursor by passing extra style or changing the role on PressableWithoutFeedback. We should set it to default.

[ishpaul777]

Cool, Can you please update you proposal to add that..

[Krishna2323]

Done.

[ishpaul777]

Thanks!

@Krishna2323 Proposal LGTM! #35200 (comment)

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[garrettmknight]

@Krishna2323 all you - let us know when your PR will be up!

[Krishna2323]

PR will be up today.

[Krishna2323]

@ishpaul777, PR 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 1.4.34-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: When user goes offline, the download button do not disappear for other file types. #35324

If no regressions arise, payment will be issued on 2024-02-07. 🎊

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

• @Krishna2323 requires payment automatic offer (Contributor)
• @ishpaul777 requires payment automatic offer (Reviewer)

[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:

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

[ishpaul777]

This was a issue since the anchor render introduced at first 2 years ago - #10144

Not a impactful bug so i dont think we need regression test for this.

[garrettmknight]

Paid out, closing.