Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Approval button is missing after payment of the report with hold expense #51503

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Approval button is missing after payment of the report with hold expense #51503

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7035,9 +7035,19 @@ function canApproveIOU(iouReport: OnyxTypes.OnyxInputOrEntry<OnyxTypes.Report>,
const iouSettled = ReportUtils.isSettled(iouReport?.reportID);
const reportNameValuePairs = ReportUtils.getReportNameValuePairs(iouReport?.reportID);
const isArchivedReport = ReportUtils.isArchivedRoom(iouReport, reportNameValuePairs);
const unheldTotalIsZero = iouReport && iouReport.unheldTotal === 0;
let isTransactionBeingScanned = false;
const reportTransactions = TransactionUtils.getAllReportTransactions(iouReport?.reportID);
for (const transaction of reportTransactions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Screenshot 2024-10-29 at 15 14 54

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 ?

Screenshot 2024-10-29 at 15 19 26

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

  1. 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.
  2. 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:

  1. 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.
  2. 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.

Copy link
Contributor

@jjcoffee jjcoffee Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thank you both for the productive back-n-forth 🙏
@marcochavezf I think we're ready for your final review and merge! 🚀

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;
}
}

return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !unheldTotalIsZero;
return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !isTransactionBeingScanned;
}

function canIOUBePaid(
Expand Down
Loading