Expensify
Hold expense - Hold option is available for failed scan expense but does not work as intended
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: 9.0.6-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Action Performed:
- Go to staging.new.expensify.com
- Go to DM with any user
- Submit one manual expense
- Submit a scan expense with an invalid receipt
- Wait for the scanning to fail
- After the scanning fails, go to transaction thread of the failed scan expense
- Click on the report header
- Click Hold
- Enter a reason and save it
- Revisit the transaction thread
Expected Result:
The failed scan expense will remain in held status
Actual Result:
The failed scan expense becomes unheld after revisiting the report
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
- [x] Android: Native
- [x] Android: mWeb Chrome
- [x] iOS: Native
- [x] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [x] MacOS: Desktop
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/43a1a21f-e364-4209-9574-de09017b79d8
Triggered auto assignment to @kevinksullivan (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.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@kevinksullivan FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
Proposal
Please re-state the problem that we are trying to solve in this issue.
Hold option is available for failed scan expense but does not work as intended
What is the root cause of that problem?
canHoldUnholdReportAction returns true for hold when scan state is SCANFAILED. This is because we are only checking if isScanning to determine if should display hold option;
https://github.com/Expensify/App/blob/28e5e4553454503180a64103bb35fae743424b6b/src/libs/ReportUtils.ts#L2826
What changes do you think we should make in order to solve the problem?
We should check if scanning failed and return false if it does.
- Check if scanning failed
const isScanFailed = TransactionUtils.hasReceipt(transaction) && transaction?.receipt?.state === CONST.IOU.RECEIPT_STATE.SCANFAILED;
- return false when it is true
const canHoldRequest = canHoldOrUnholdRequest && !isOnHold && (isRequestHoldCreator || (!isRequestIOU && canModifyStatus)) && !isScanning && !!transaction?.reimbursable && !isScanFailed;
We can apply the same for canUnholdRequest to put an extra guard
Also we can create TransactionUtils.isScanFailed utility function that returns to true if scan failed.
What alternative solutions did you explore? (Optional)
@kevinksullivan Huh... This is 4 days overdue. Who can take care of this?
@kevinksullivan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
@kevinksullivan 10 days overdue. Is anyone even seeing these? Hello?
Job added to Upwork: https://www.upwork.com/jobs/~0119279654a77b0111
Triggered auto assignment to Contributor-plus team member for initial proposal review - @c3024 (External)
@kevinksullivan, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!
The canHoldOrUnholdRequest function only considers the scanning status of the receipt and not the failed status. When the receipt scanning fails, canHoldOrUnholdRequest shows the Hold option. Clicking on the Hold option adds a hold field to the comment of the transaction, and the backend does not throw an error. The Unhold option then appears in place of Hold on the ReportDetailsPage.
However, when we go back to the parent report and revisit the thread, it calls OpenReport and there is no hold field in the comment of the transaction. Consequently, the Hold option appears again on the ReportDetailsPage.
Since we do not allow holding an expense while scanning, it is logical to also prevent holding the expense when the scan fails. Therefore, the proposal here by @etCoderDysto looks good to me.
🎀 👀 🎀 C+ Reviewed
Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
I'm not sure what the expected behavior is here. @robertjchen could you please confirm? Should we be able to hold expenses with a failed scan? I don't see anything in the backend preventing that, so I'm not sure why the hold action is not persisted in this case.
There is no check on the backend to prevent even holding a request that is being scanned. This restriction is only on the frontend. If we remove the isScanning condition in canHoldOrUnHoldRequest on the front-end and click on Hold in the ReportDetailsPage, the backend does not throw any error.
Yes, I noticed that when implementing the feature on the Search page as well. There are several checks on the frontend that are not enforced by the API. @robertjchen is that something that we should be enforcing on the API side too?
Should we be able to hold expenses with a failed scan?
Yes.
Is the issue with the fact that the report status is not being optimistically updated after the held action?
Ok, so we should be able to hold the expense. @etCoderDysto @c3024 I think the RCA is incorrect in this case. Now why does that not work properly if the API request doesn't fail?
Is the issue with the fact that the report status is not being optimistically updated after the held action?
Not sure yet. We need to check if the action is persisted or not
Ok, so we should be able to hold the expense. @etCoderDysto @c3024 I think the RCA is incorrect in this case. Now why does that not work properly if the API request doesn't fail?
I will investigate why it is failing and get back to you.
@luacmartins, @kevinksullivan, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!
I think this should be checked from the backend.
HoldRequest adds a hold field to the comment in the transaction, but after visiting the parent report and then revisiting the transaction thread, the OpenReport call does not return the transaction with the hold in its comment field.
https://github.com/user-attachments/assets/2be2ff1d-56f1-4d00-b708-f011128254c7
The expense status is shown as held correctly as long as we are on the report or on the report details page but when we revisit this report after visiting some other reports, the OpenReport response from backend resets this field that determines if the expense if held or not.
@c3024 I'm unable to reproduce that behavior in dev. Loading the report with the held transaction still returns the hold action. What am I missing?
https://github.com/user-attachments/assets/84754adf-7af5-4b29-8e3a-2f1d0328e656
I can repro this:
https://github.com/user-attachments/assets/f908506e-0a0a-4964-9f76-15b42c38964b
Yea, I'm not sure why I can't reproduce this. I wonder if we're not persisting the data or if OpenReport is just not returning the hold action. @trjExpensify @c3024 do you have a reportID for the problematic transactions?
@luacmartins, @kevinksullivan, @c3024 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!