[HOLD for payment 2023-11-22] [$500] Scan receipt - Crash when trying to download a receipt while in scanning process

[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.3.93-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Open staging.new.expensify or the mobile app
2. Navigate to the workspace chat as an employee
3. Select "Request money" > "Scan receipt"
4. Upload a receipt file from your device
5. Click "Request"
6. Navigate to the request conversation
7. Open the request preview and the attached receipt
8. Click "More" > "Download"

Expected Result:

When attempting to download a receipt while in scanning process, Expensify crashes

Actual Result:

It should be possible to download the receipt in all viable scenarios
Extra info: Crash occasionally happens with any type of request (Manual, Scan and Distance)
Uh-oh, something went wrong!
Please try closing and reopening the app or switching to web.
If the problem persists, reach out to [email protected]

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

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Bug6257345_1698699313430.Web_-_error.mp4

logs.txt

Bug6257345_1698699384771.iOS_-_error.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0128ad4c7b2932ce1f
• Upwork Job ID: 1719139466680426496
• Last Price Increase: 2023-10-30
• Automatic offers:
• aimane-chnaif | Reviewer | 27512074
• mkhutornyi | Contributor | 27512075

[melvin-bot]

Triggered auto assignment to @maddylewis (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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0128ad4c7b2932ce1f

[melvin-bot]

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

[wlegolas]

Proposal

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

Scan receipt - Crash when trying to download a receipt while in the scanning process

What is the root cause of that problem?

The AttachmentModal component doesn't receive the file name by the property "originalFileName" from the ReportAttachments component, by default "originalFileName" value is an empty string.

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

Lines 35 to 43 in ec7fb3c

	return (
	<AttachmentModal
	allowDownload
	defaultOpen
	report={report}
	source={source}
	onModalHide={() => Navigation.dismissModal()}
	onCarouselAttachmentChange={onCarouselAttachmentChange}
	/>

The property "originalFileName" is used to set the file structure file.name that is used when the user clicks on the button to download the file:

App/src/components/AttachmentModal.js

Lines 188 to 199 in 6ec9dd5

	const downloadAttachment = useCallback(() => {
	let sourceURL = source;
	if (isAuthTokenRequired) {
	sourceURL = addEncryptedAuthTokenToURL(sourceURL);
	}
	fileDownload(sourceURL, file.name);
	// At ios, if the keyboard is open while opening the attachment, then after downloading
	// the attachment keyboard will show up. So, to fix it we need to dismiss the keyboard.
	Keyboard.dismiss();
	}, [isAuthTokenRequired, source, file]);

As the "originalFileName" is an empty string, when the downloadAttachment method is called the file state is undefined, and an exception is thrown because it is trying to access the name from an undefined value.

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

We should change the ReportAttachments component to extract the file name from the source property and pass this value to the AttachmentModal component.

What alternative solutions did you explore? (Optional)

N/A

POC

poc-issue-30616.mp4

[dukenv0307]

Proposal

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

When attempting to download a receipt while in scanning process, Expensify crashes

What is the root cause of that problem?

In here, we're not passing the originalFileName to the AttachmentModal so the file is undefined and it crashes here when we try to access "name" of the file here.

The purpose of the original file name is that when the user downloads, we want it to have readable file name and recognizable to the user since it's the original name of the file the user uploaded.

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

1. We should add originalFileName to the ReportAttachments route params, and pass it to the AttachmentModal. The originalFileName should be added to the optimistic data (and likely back-end as well) when we upload the receipt, same thing we do with the workspace avatar here (and also user avatar)

2. Additionally, we should use lodashGet when retrieving the file name here to avoid any potential crash.

fileDownload(sourceURL, lodashGet(file, 'name', ''));

3. Also there's a bug in the fallback logic here. Earlier if the fileName is not available, it will fallback to the FileUtils.getAttachmentName(url). But after appendTimeToFileName is added, the fallback will never happen because even when the fileName is empty, appendTimeToFileName will return something like -2023-10-31 02_26_43.729 (without extension) and it will still be used as the file name.

We need to update it to:

fileName ? FileUtils.appendTimeToFileName(fileName) : FileUtils.getAttachmentName(url)

What alternative solutions did you explore? (Optional)

If we don't want to add original file name for receipt, the step 2 and 3 in the solution should be enough, since that is also the fallback logic we used earlier.

[mkhutornyi]

Proposal

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

App crashes when download attachment before being uploaded to server

What is the root cause of that problem?

App/src/components/AttachmentModal.js

Line 356 in 444a908

const threeDotsMenuItems = useMemo(() => {

Above function is memoized and not re-calculated when file is set below:

App/src/components/AttachmentModal.js

Line 153 in 444a908

setFile(attachment.file);

So even if file value exists, it's not updated and remains as undefined (initial state value) in downloadAttachment function below:

App/src/components/AttachmentModal.js

Lines 188 to 199 in 444a908

	const downloadAttachment = useCallback(() => {
	let sourceURL = source;
	if (isAuthTokenRequired) {
	sourceURL = addEncryptedAuthTokenToURL(sourceURL);
	}
	fileDownload(sourceURL, file.name);
	// At ios, if the keyboard is open while opening the attachment, then after downloading
	// the attachment keyboard will show up. So, to fix it we need to dismiss the keyboard.
	Keyboard.dismiss();
	}, [isAuthTokenRequired, source, file]);

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

Add file to this dependencies array:

App/src/components/AttachmentModal.js

Line 394 in 444a908

}, [isAttachmentReceipt, props.parentReport, props.parentReportActions, props.policy, props.transaction]);

Addon:
It's possible that use clicks Download button quickly before file is being set.
So we can show Download button only when file exists

App/src/components/AttachmentModal.js

Lines 378 to 382 in 444a908

	menuItems.push({
	icon: Expensicons.Download,
	text: props.translate('common.download'),
	onSelected: () => downloadAttachment(source),
	});

So wrap this with if (file) condition

[melvin-bot]

@maddylewis, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

[aimane-chnaif]

I debugged myself and it was indeed memo issue.
file is set correctly but undefined when download.

@dukenv0307 your solution will prevent crash but always generates file name even though real file name exists so workaround.

@mkhutornyi's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @aimane-chnaif 🎉 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]

📣 @mkhutornyi 🎉 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 📖

[kidroca]

It strikes me as counterintuitive and potentially problematic that users are able to initiate a download of an attachment before the upload process to the server is even finalized. Consider the following inconsistencies:

1. Premature Download Option: The current functionality presents the download button even before the upload is completed. This is technically paradoxical; how can one download an attachment that hasn't fully reached the server yet? Are users downloading the very data they're in the process of uploading?

2. Processing State Ambiguity: Additionally, the button remains active while the attachment is flagged as 'processing'. I'm very familiar with the Receipt feature but if the end state of a receipt can morph post-processing, say from a simple image to a structured PDF, shouldn't the download only be permissible post this transformation to ensure consistency and integrity of the document?

In my opinion, the download feature should be context-sensitive and only become available when the upload is conclusively complete and the file is in a stable, fully processed form.

[aimane-chnaif]

@kidroca this issue is not only tied to local download but also to remote download. Sometimes crashes even when download uploaded image.

"While in scanning process" has 2 cases:

• image being uploaded
• image uploaded successfully but no scan result yet

Download availability for local image can be out of scope. Should be separate issue

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

• fix crash when trying to download a receipt while in scanning process #30925

If no regressions arise, payment will be issued on 2023-11-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @aimane-chnaif requires payment offer (Reviewer)
• @mkhutornyi requires payment 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:

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

[melvin-bot]

@bondydaa, @maddylewis, @aimane-chnaif, @mkhutornyi Eep! 4 days overdue now. Issues have feelings too...

[aimane-chnaif]

• The PR that introduced the bug has been identified. Link to the PR: Hide replace option for the request that cannot edit #28373
• 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: https://github.com/Expensify/App/pull/28373/files#r1406406095
• 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: N/A
• Determine if we should create a regression test for this bug. This was caught by Applause team so I assume there's already regression test for this
• 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.

[maddylewis]

Payment Summary

• $500 @aimane-chnaif requires payment offer (Reviewer)
• $500 @mkhutornyi requires payment offer (Contributor)

[maddylewis]

both C+ and Contributor paid via Upwork!