Approval button is missing after payment of the report with hold expense #51503
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-app-2024-10-28_11.18.41.mp4Android: mWeb Chromeandroid-chrome-2024-10-28_11.19.56.mp4iOS: Nativeios-app-2024-10-28_12.17.11.mp4iOS: mWeb Safariios-safari-2024-10-28_12.14.19.mp4MacOS: Chrome / Safaridesktop-chrome-2024-10-28_10.24.59.mp4MacOS: Desktopdesktop-app-2024-10-28_12.19.16.mp4 |
| const unheldTotalIsZero = iouReport && iouReport.unheldTotal === 0; | ||
| let isTransactionBeingScanned = false; | ||
| const reportTransactions = TransactionUtils.getAllReportTransactions(iouReport?.reportID); | ||
| for (const transaction of reportTransactions) { |
There was a problem hiding this comment.
Ah there's a problem here, this still will allow approvals of 0-value scans (e.g. if you upload a junk receipt where the amount can't be read).
desktop-chrome-0-value-2024-10-28_10.28.09.mp4
There was a problem hiding this comment.
@jjcoffee That's true and expected given my solution. As stated in the other issue which implemented the unheldTotalIsZero logic, the issue's (#48590) the expected result states:
Approver should not be able to approve scanning expense
None of the issues state that we shouldn't allow Approve when the transaction amount is 0, but that we shouldn't allow Approve while a scan transaction is currently scanning.
To confirm this I asked the C+ / CME in the other issue #48590 (comment) but didn't get a response so I assumed that the above expected result is the way to go.
Why should we allow Approve when amount is 0 ?
Because when a scan transaction with blurry image fails and amount is 0, you get an error on the field that you should add an amount, which allows you to add an amount if current is 0 then Approve - otherwise you deliberately Approve with 0 amount, on purpose.
If you have more context that I'm missing here which confirms that we shouldn't show the Approve button if the transaction amount is 0, please let me know and I'll add that to the PRs logic - otherwise, if there are no other issues with the logic I think we're good to move forward with this.
There was a problem hiding this comment.
otherwise you deliberately Approve with 0 amount, on purpose.
@ikevin127 I can't think of any reason why you'd want to approve an expense with a 0 amount. There's nothing explicitly wrong with it but it just feels a bit messy that it's even possible, I think!
I'm also assuming we don't want to allow this both from the title of #48590 as well as the PR's test step:
verify that approver can't approve the scan expense as 0.00 amount
There was a problem hiding this comment.
@jjcoffee The problem and pitfall in that PRs logic is that canApproveIOU is used in MoneyReportHeader and ReportPreview to show the Approve button for multiple expenses, because of this we can't hide Approve only for a transaction with amount 0 while showing it for the other transaction which is approvable (has all required fields completed).
That's why the unheldTotalIsZero logic added by the other PR was hiding the Approve button even for transactions what were not of type scan with 0 amount (our issue).
Say we implement this logic:
let isAmountMissing = false;
let isTransactionBeingScanned = false;
const reportTransactions = TransactionUtils.getAllReportTransactions(iouReport?.reportID);
for (const transaction of reportTransactions) {
const hasReceipt = TransactionUtils.hasReceipt(transaction);
const isReceiptBeingScanned = TransactionUtils.isReceiptBeingScanned(transaction);
// If transaction has receipt (scan) and its receipt is being scanned, we shouldn't be able to Approve
if (hasReceipt && isReceiptBeingScanned) {
isTransactionBeingScanned = true;
}
// Change isAmountMissing to true only if transaction amount is 0 (eg. failed scan)
if (TransactionUtils.isAmountMissing(transaction)) {
isAmountMissing = true;
}
}
return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !isTransactionBeingScanned && !isAmountMissing;And we have a report with 2 transactions, 1 is manual and has amount and 1 is a failed scan (amount 0), with the logic above we won't see the Approve button at all.
Whereas with the current logic of this PR, say the manual transaction is on hold and we also have 1 failed scan transaction (amount 0), we will see the Approve button on ReportPreview and MoneyReportHeader, which when clicked will ask the user if they want to approve amount 0 (only the failed scan transaction) or both (failed scan and the held manual $7), which nobody would do, meaning to approve only the $0, in a real-world scenario right ?
To change this logic to your expectations would require rethinking how canApproveIOU works in MoneyReportHeader and ReportPreview, instead of showing Approve on a group of expenses (ReportPreview / MoneyReportHeader) we would have to implement logic to only show the button per individual transaction, which is definitely out of scope for our issue.
Please let me know what do you think we should do with this!
There was a problem hiding this comment.
Thanks for your thoughts @ikevin127!
we have a report with 2 transactions, 1 is manual and has amount and 1 is a failed scan (amount 0), with the logic above we won't see the Approve button at all.
Agree that we wouldn't want this, in this case and if the manual transaction is held I guess we'd ideally want to hide the Approve only $0.00 button.
@marcochavezf Bringing you in here to chime in on whether we're okay to keep showing the Approve button for a failed scan (e.g. blurry image) with a $0.00 amount.
The main issue is that if you have another expense that's held, after tapping the Approve button, the first option shows as Approve only $0.00. That feels like dodgy UX to me, but I agree with @ikevin127 that fixing it is probably out of scope for this issue, since we wouldn't want to disable approving entirely as that would block approving the entire report (the second option in the screenshot below) and this would require a bit of a rethink of how to display that popup.
Just to note that the issue that introduced the regression had this as a test step:
verify that approver can't approve the scan expense as 0.00 amount
So wanted to be sure we can go ahead despite that.
There was a problem hiding this comment.
Thanks for bringing this up, guys! I agree that approving a report with an amount of 0 after scanning is not a good UX. Also, understandably it can be considered out-of-scope because it's an edge case we couldn't foresee.
Given that we'd need to wait until this PR is deployed to staging/production to report it as a bug, what would be the solution to fix it here? We can adjust the price of the original issue by justifying the out-of-scope edge case.
There was a problem hiding this comment.
@marcochavezf I appreciate the input 🙏
I agree that approving a report with an amount of 0 after scanning is not a good UX. Also, understandably it can be considered out-of-scope because it's an edge case we couldn't foresee.
- Approving a report with an amount of 0 after scanning was allowed, present and possible on prod / staging even before PRs Fix Scanning expense can be approved as 0.00 amount and can not be edited #50817 changes and wasn't reported as an issue. Therefore the fact that the current PR fixes our issue while maintaining PRs Fix Scanning expense can be approved as 0.00 amount and can not be edited #50817 changes its a good thing which I could've left out as our issue's scope wasnt to improve or correct the other PRs logic but I did because otherwise that issue would've been reintroduced.
- To reinforce point (1.), it is for sure out of scope since up until me bringing up the fact that this PRs fix handles both our issue and maintaining the other PRs logic, this wasn't even an issue (never reported as one).
Given all this, here are the 2 roads I'm willing to move forward with right now regarding this PR:
- Merge this PR as is since it fixes both our issue while maintaining the expected reult fix for issue [$250] Hold - Scanning expense can be approved as 0.00 amount and can not be edited #48590.
- Drop everything, close the PR and leave it to somebody else to handle this + any out of scope work.
Reasoning
I'm not willing to go out of scope here regardless of extra compensation simply because the issue of approving a report with an amount of 0 after scanning was present on prod / staging even before PR #50817, therefore it should not be a problem or reported as a regression of this PR - IMO it should be a New Feature issue since it requires lots of changes throughout the app.
There was a problem hiding this comment.
Given that we'd need to wait until this PR is deployed to staging/production to report it as a bug, what would be the solution to fix it here?
@marcochavezf Given that it's a bit of an edge case, and it was also previously possible (to approve a $0 amount), I think we can just proceed with this PR and report the bug after.
There was a problem hiding this comment.
Got it, thanks for the additional context @ikevin127! Yeah, if approving a report with an amount of 0 after scanning was already allowed then it makes sense to report it as a bug and continue with this PR as-is
There was a problem hiding this comment.
Cool, thank you both for the productive back-n-forth 🙏
@marcochavezf I think we're ready for your final review and merge! 🚀
This comment was marked as outdated.
This comment was marked as outdated.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.0.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.59-3 🚀
|
Details
We're replacing the logic added by PR #50817 by actually checking if the transaction has a receipt (scan) and is currently being scanned when it comes to hiding the
Approvebutton because the previous logic was hiding the button regardless of the transaction being of type scan or not which shouldn't happen. The improved logic ensures we fix our issue while keeping the fix for the other issue for which the logic was written in the first place.Fixed Issues
$ #50932
PROPOSAL: #50932 (comment)
Tests
Same as QA Steps.
Offline tests
Same as QA Steps.
QA Steps
Prerequisite: A workspace with an Employee and a general Approver (non-admin, can approve of any Employee).
Approvebutton.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop