[HOLD for payment 2023-12-06] [HOLD for payment 2023-12-04] Distance - Wrong error message is shown when creating an invalid distance request

[kbecciv]

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.3.0
Reproducible in staging?: y
Reproducible in production?: n
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. Navigate to staging.new.expensify.com.
2. Go offline.
3. Go to workspace chat > + > Request money.
4. Go to Distance.
5. Enter vague addresses - SF for Start and LA for Finish point.
6. Create the Distance expense.
7. Go online.

Expected Result:

The error message that will be shown is "Unexpected error requesting money, please try again later"

Actual Result:

The error message is "The receipt didn't upload. Download the file or dismiss this error and lose it" instead of the usual "Unexpected error requesting money, please try again later".

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

Bug6289525_1700832005026.bandicam_2023-11-24_15-34-26-832.mp4

View all open jobs on GitHub

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

📣 @github-actions[bot]! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot]

Triggered auto assignment to @iwiznia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[dummy-1111]

Looks like a regression of #29790

[iwiznia]

It indeed looks like a regression of that PR.. @Gonals can you take a look? Otherwise we can revert...
Going to wait till monday since most people are ooo today.

[luacmartins]

TBH I think we should revert given the comments on the PR after it was merged. That being said, a clean revert is no longer possible

[iwiznia]

Yeah, going to wait for monday so that Alberto can take a look. It's not like we want to deploy today...

[luacmartins]

Haha I was trying to deploy or at least leave the checklist clear for monday morning deploy, but there are so many open blockers 😅

[iwiznia]

I was trying to deploy

You crazy man, it's friday and most of the company is OOO and/or traveling 😄

[luacmartins]

Anyways, I got the revert PR ready if we decide to go that way - #31854

[ishpaul777]

@luacmartins @iwiznia if i am not wrong the receipt for the distance request is created on Backend and we use placeholder image as optimistic data so if request fails in any case a receipt is never generated for the distance request, Right? In thats the case we can just skip using "Download file" message if file is never generated for failure scenario, and just use the genericCreateFailureMessage?

[situchan]

@ishpaul777 no, it's local file download. Not from server. Just to help user not re-scan receipt in case upload failed for some reason.
Here's scenario:

• user scans receipt using device camera
• upload to server and forget the receipt
• after a while, failed to upload
• user has no way to upload receipt again

With this local download feature, user can download scanned receipt to device and re-upload importing from gallery.
So first time user scans from camera, 2nd time imports from device gallery.
Makes sense?

[iwiznia]

For distance receipts, there's nothing to download, so that message does not make sense.

[situchan]

yeah, I am talking about scan receipt, not distance receipt. So this is bug anyway

[ishpaul777]

For distance receipts, there's nothing to download, so that message does not make sense.

This is what i was meant to say, I can raise a quick PR with fix then we CP to staging instead of reverting

Changes

just add below condition here and pass transaction to getReceiptError call

_.isEmpty(receipt) || TransactionUtils.isDistanceRequest(transaction)

[luacmartins]

I don't think the implementation of this error is correct. If there was an error uploading the receipt, that's a server side error and we should be sending an errorField: receipt Onyx update from the server and checking that instead. I don't think we should optimistically set this on the client in failureData if a money request fails because it could have failed for other reasons other than a receipt upload.

[ishpaul777]

I don't think we should optimistically set this on the client in failureData if a money request fails because it could have failed for other reasons other than a receipt upload.

In any case a request fails, we want the user to have option to download receipt if he added otherwise it will be lost no matter the reason request fails as far as i understand, so i think its fine to use failuredata here

[iwiznia]

In any case a request fails, we want the user to have option to download receipt if he added otherwise it will be lost no matter the reason request fails as far as i understand, so i think its fine to use failuredata here

Correct. If the user uploaded a file, we want to ensure the file is not lost and they can download it.

[shubham1206agra]

@luacmartins For receipt money requests, shouldn't we allow the user to download the file irrespective of the error, as we will remove the request after clicking the dismiss button?

I think the error message structure can be:

Error message from the backend
Due to the above error, the receipt didn't upload. You can download the file or dismiss this error and lose it.

Something on the line of this.

On the second line, we can put the condition for the non-empty receipt. If you want to go this way, I can try to resolve this.

[shubham1206agra]

Another thing, backend doesn't send anything except error status code when I send corrupted file as receipt. I think backend can send error for onyx in this case.

[ishpaul777]

The issue here as i understand is is specific to Distance request only, "Download file" message does not makes sense in any case as user can add(replace) a receipt only request is successful and initail receipt is a created on backend and use a placeholder image till the request is successful, that is root cause we see a "download file" message for distance request as receipt is not empty, soultion to this can be as simple as to not set error message to "download file" for distance request.

backend doesn't send anything except error status code when I send corrupted file as receipt.

if there is a error and request is failed, we show the user to download the file (corrupted in this case) why do we want BE to send a error for currupted receipt?

[shubham1206agra]

The issue here as i understand is is specific to Distance request only, "Download file" message does not makes sense in any case as user can add(replace) a receipt only request is successful and initail receipt is a created on backend and use a placeholder image till the request is successful, that is root cause we see a "download file" message for distance request as receipt is not empty, soultion to this can be as simple as to not set error message to "download file" for distance request as t=

I think there is a issue already created which will remove the optimistic receipt from Distance request.

backend doesn't send anything except error status code when I send corrupted file as receipt.

if there is a error and request is failed, we show the user to download the file (corrupted in this case) why do we want BE to send a error for currupted receipt?

To differentiate from other errors.
@luacmartins Do we have cases where scan request will fail other than corrupted file?

[shubham1206agra]

This is the issue #28965. Here, we will replace the placeholder image to optimistic map component.

[ishpaul777]

Okay, as i skimming through issue we will use a snapshot image of route as initial receipt unless request is successful,still the question is same why we show download file message when user haven't added one?

[shubham1206agra]

Okay, as i skimming through issue we will use a snapshot image of route as initial receipt unless request is successful,still the question is same why we show download file message when user haven't added one?

Nope we are not using an image there

[mountiny]

Assigning @0xmiroslav as he was C+ on the PR and created a follow up partial revert

[ishpaul777]

Okay! 👍 makes sense , after linked issue is resolved then there is no optimistic receipt object so it fallbacks to generic error message, my proposed changes does achieve the same as it uses generic error message for every distance request which i thought is fine for now as this was a Deploy blocker and that feature is holding for other GH, anyways if we are going to revert offending PR then i think holding for the linked issue might be a good idea

[mountiny]

This is fixed in staging, given @0xmiroslav has been C+ on the offending PR which will have to be completed I think payments will be handled there and we can close this one
https://github.com/Expensify/App/assets/36083550/fee29b4d-e49d-480e-a2f4-7cc5a264b153

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

• [CP Staging] revert optimistic receipt errors #31911

If no regressions arise, payment will be issued on 2023-12-04. 🎊

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:

• @0xmiroslav requires payment

[melvin-bot]

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

• [CP Staging] revert optimistic receipt errors #31911

If no regressions arise, payment will be issued on 2023-12-06. 🎊

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:

• @0xmiroslav requires payment