Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

BUG: Receipt is deleted if app is killed while request is in queue #51761

Open neil-marcellini opened 2 weeks ago

neil-marcellini commented 2 weeks 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: v9.0.55-9 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: 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: See the linked issue below Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/434533 Issue reported by: @Neil Marcellini

Originally reported internally here

Slack conversation https://expensify.slack.com/archives/C049HHMV9SM/p1730315753190039

Action Performed:

  1. Go offline
  2. Submit an expense and scan a receipt with the in app camera, submit it to anyone
  3. Kill the app
  4. Re-open the app
  5. Go back online

Expected Result:

The receipt is uploaded and scanned correctly. If it fails for some reason, then the user is able to download the receipt.

Desired Result:

Store images captured with the in app camera to a non-temporary folder in the application folder within the file system. For example, store images in a folder Receipts-Pending-Upload under the New Expensify folder.

We're still discussing in the thread linked below, but maybe after a receipt has been uploaded on the server, delete it from this folder.

Actual Result:

There is an error submitting the expense and no option to download the receipt. If the user dismisses this error then they permanently lose the receipt image.

Notes

I think this happens because we store the receipts in a /tmp/ folder which is cleared when the app is killed (E.g. file:///private/var/mobile/Containers/Data/Application/C5B31E5E-C6BD-4372-B1DF-D1326BDA7B28/tmp/9C8642AF-C0FF-44F9-988A-10B3A752B7A2.jpeg). It's a common nervous habit of iOS users to kill apps whenever they go to the home screen, so it's pretty realistic that this can happen, especially if the SequentialQueue gets a bit backed up.

More discussion in #quality.

Looks like if we upgrade react-native-vision-camera we can specify a path when taking the photo. Or we could manually move the file to our desired location, if upgrading vision camera is really difficult.

Workaround:

Try to take screen shots of the receipt images before dismissing the error and try re-uploading later. Not a very good workaround especially when users trust our app not to lose data.

Platforms:

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

iOS Standalone, maybe others.

Screenshots/Videos

Add any screenshot/video evidence https://github.com/user-attachments/assets/7f982f57-92ff-48f6-8e78-80bebed7b534

View all open jobs on GitHub

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

ikevin127 commented 2 weeks ago

From taking a quick look at what happens to the RequestMoney call which was stored in the sequential queue and resumed once back online, I'm not entirely sure that the receipt is deleted from the API call, it looks like the receipt [Object] is still there and passed with the call upon returning back online when the call being made.

What I noticed instead is a 405 response from the BE regarding the RequestMoney call, mentioning that no amount was specified, which I think is what leads to the red error which if dismissed -> removes the expense entirely, giving the sense that the receipt was lose because of closing the app, when in fact the expense call failed and when dismissed, the expense goes away completely.

Note: I also noticed a Onyx merge error once the call is being made when returning back online, most likely coming from the BE response in onyx data.

I'd look into this from a BE perspective to see what happens there, if the receipt is indeed passed and why does the BE respond with error in this issue's case. I'm assuming it might have something to do with the scan being intrerupted by the user closing the app while offline, then getting back online and call being resumed from squential queue, since the BE response is error: no amount was passed.

neil-marcellini commented 1 week ago

Hi, thanks for taking a look @ikevin127. There's a lot more context that I forgot to add to the issue description. I'll start adding that now. I'll also work on doing some backend testing to verify that the receipt file is not received in this case. I'm having a bit of trouble getting the iOS simulator to connect to my dev environment, which is why I haven't been able to gather that info yet. However, I'm pretty sure that's what's happening.

neil-marcellini commented 1 week ago

The description is updated now.

ikevin127 commented 1 week ago

I deployed iOS: Native on a real device and was able to do some debugging, looks like this is a matter of the temporary file having a limited lifetime and being deleted if the app is closed for x amount of time (minute or so).

neil-marcellini commented 1 week ago

What were your conclusions from debugging? I see you crossed part out.

ikevin127 commented 1 week ago

@neil-marcellini It's confusing but here's what I found:

The root cause does not seem related to the device saving the file temporarely but rather to this line -> where we save the source here:

https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/libs/actions/IOU.ts#L458-L463

because in our case isDraft is always true meaning we always set the receipt source to ONYXKEYS.COLLECTION.TRANSACTION_DRAFT instead of ONYXKEYS.COLLECTION.TRANSACTION.

Then we have this part:

https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/libs/actions/IOU.ts#L840-L849

where we build the RequestMoney API call onyx failureData we're passing the transaction.receipt via IOU.setMoneyRequestReceipt.

What I noticed about setMoneyRequestReceipt is that if we change the code like this:

function setMoneyRequestReceipt(transactionID: string, source: string, filename: string, isDraft: boolean, type?: string) {
    if (isDraft) {
        Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {
            receipt: {source, type: type ?? ''},
            filename,
        });
    }   

    Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
        receipt: {source, type: type ?? ''},
        filename,
    });
}

Then when returning back online and transaction fails, we always get the Download receipt button which allows us to download the receipt - regardless of how long the app was killed for.

https://github.com/user-attachments/assets/e909bed4-8277-4782-b640-59a31d1b27d5

This is the confusing part as I was not able to figure out what happens BE wise when returning back online and why changing setMoneyRequestReceipt as mentioned above always returns the Download receipt error whereas with the current code that never happens on staging even if closing / reopening the app right away, but it can happen on dev sometimes.

allgandalf commented 1 week ago

@neil-marcellini thoughts on ^

neil-marcellini commented 1 week ago

Interesting, thanks for posting your findings. I managed to test this on dev with an iOS simulator. Of course I can't take a photo but choosing an image from the photo library had the same result. I had to leave the app killed for about a minute to reproduce the issue, which isn't shown in the video to save time.

I confirmed by debugging the backend that the receiptFile is empty. I will try your suggested solution and see if that fixes it.

https://github.com/user-attachments/assets/355d055d-b7ba-43a4-9171-d44e34e7663d

Code 2024-11-06 12 46 45

neil-marcellini commented 1 week ago

The root cause does not seem related to the device saving the file temporarely but rather to this line -> where we save the source here:

I'm not quite sure I'm following. When requestMoney is called on the confirmation page the receiptFile is passed. https://github.com/Expensify/App/blob/ab40264dff9a6c21372617c8744b1013c22dd2d2/src/pages/iou/request/step/IOURequestStepConfirmation.tsx#L500-L503

If you trace that down to where the failure data is generated I'm pretty sure it all works.

neil-marcellini commented 1 week ago

I added a suggestion about how we might store the photo in a non temp location.

rezkiy37 commented 1 week ago

Hi, I am Michael (Mykhailo) from Callstack, an expert agency and I can work on this issue.

neil-marcellini commented 1 week ago

Thanks so much. Please post a proposal for how you're planning to fix this.

allgandalf commented 6 days ago

@rezkiy37 when can we expect a proposal for the fix?

rezkiy37 commented 6 days ago

I am working on the proposal. I will post it next week.

neil-marcellini commented 5 days ago

Heads up, I'll OOO next week. If you want a faster review you can ask @johncschuster to get another internal reviewer.

allgandalf commented 3 days ago

not overdue sir melvin

johncschuster commented 2 days ago

Not overdue

rezkiy37 commented 2 days ago

I've created a draft implementation with the stable folder creation. Testing it and working on the proposal.

allgandalf commented 1 day ago

can you link that draft @rezkiy37 ?, would like to get my eyes through it

melvin-bot[bot] commented 1 day ago

@johncschuster @neil-marcellini @rezkiy37 @allgandalf 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!

rezkiy37 commented 1 day ago

can you link that draft @rezkiy37 ?, would like to get my eyes through it

I am going to link it today 🙂

rezkiy37 commented 1 day ago

Proposal

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

Submitting the expense causes an error, and there is no option to download the receipt. If the user dismisses this error, they will permanently lose the receipt image.

What is the root cause of that problem?

It happens because the app stores the receipts in a /tmp/ folder which is cleared when the app is killed.

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

Improve the app to store captured receipts in a new stable directory. We can call it /Receipts-Pending-Upload under the /New Expensify folder (the app's folder).

Video examples https://github.com/user-attachments/assets/e1adc09c-ecb8-4fdc-8996-c8959556d30a https://github.com/user-attachments/assets/ccd8eab9-ee92-4eb4-b041-2be8546ab641

Steps to do:

  1. Upgrade react-native-vision-camera to the latest version 4.6.1 where they have added path property to the takePhoto method.
Screenshot

Screenshot 2024-11-13 at 16 26 14

  1. Implement a function that returns a proper path for IOS and Android separately because there is a non-critical difference between IOS and Android: a. IOS allows the creation of custom directories in the project's root folder. Path: ⁨On My iPhone⁩/New Expensify Dev⁩/Receipts-Pending-Upload⁩/*.jpg. b. Android allows the creation of custom directories only inside some default ones like /Downloads. Path: Android/data/com.expensify.chat.dev/files/Download/Receipts-Pending-Upload/*.jpg.

  2. Check if the directory already exists and create if not yet like

ReactNativeBlobUtil.fs.isDir(path)
  1. Store captured receipts in the newly added directory.
ReactNativeBlobUtil.fs
.mkdir(path)
.then(() => {
    console.debug('[dev] Directory created successfully');
})
.catch((error) => {
    console.error('[dev] Error creating directory:', error);
});

I have a draft PR - https://github.com/Expensify/App/pull/52483.

What alternative solutions did you explore? (Optional)

N/A