[$250] "Approve" button appears even if the report has a violation on it

[m-natarajan]

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: 9.0.46-2
Reproducible in staging?: y
Reproducible in production?: y
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: @coleaeason
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1728406913463479

Action Performed:

1. Submit an expense report with violation
2. Sign in as a approver

Expected Result:

Should not see a "approve" button since receipt has a violation

Actual Result:

"Approve" button with GBR appears

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021845657010504568512
• Upwork Job ID: 1845657010504568512
• Last Price Increase: 2024-10-14
• Automatic offers:
• hoangzinh | Reviewer | 104460235
• abzokhattab | Contributor | 104460238

Issue Owner

Current Issue Owner: @adelekennedy

[melvin-bot]

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

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-10-08 20:05:37 UTC.

Proposal

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

The "Approve" button appears even when the report has a violation.

What is the root cause of that problem?

The canApprove function does not consider whether the report has violations.

App/src/libs/actions/IOU.ts

Lines 6905 to 6926 in fd32223

	function canApproveIOU(iouReport: OnyxTypes.OnyxInputOrEntry<OnyxTypes.Report>, policy: OnyxTypes.OnyxInputOrEntry<OnyxTypes.Policy>) {
	// Only expense reports can be approved
	const isPaidGroupPolicy = policy && PolicyUtils.isPaidGroupPolicy(policy);
	if (!isPaidGroupPolicy) {
	return false;
	}
	const isOnSubmitAndClosePolicy = PolicyUtils.isSubmitAndClose(policy);
	if (isOnSubmitAndClosePolicy) {
	return false;
	}
	const managerID = iouReport?.managerID ?? -1;
	const isCurrentUserManager = managerID === userAccountID;
	const isOpenExpenseReport = ReportUtils.isOpenExpenseReport(iouReport);
	const isApproved = ReportUtils.isReportApproved(iouReport);
	const iouSettled = ReportUtils.isSettled(iouReport?.reportID);
	const reportNameValuePairs = ReportUtils.getReportNameValuePairs(iouReport?.reportID);
	const isArchivedReport = ReportUtils.isArchivedRoom(iouReport, reportNameValuePairs);
	return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport;
	}

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

We should update the return statement to add a condition to check that there are no violations:

return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !hasReportViolations(iouReport?.reportID)

the same change can be done in other iou actions if needed.

What alternative solutions did you explore? (Optional)

optionally we can useReportUtils.hasViolations(iouReport?.reportID ?? "", transactionsViolations) instead of hasReportViolations(iouReport?.reportID) where the transactionsViolations can either be passed to the function using Onyx:

const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);

Or, it can be declared as a global variable within the file, with the value fetched using Onyx.connect to the ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS key.

[ChavdaSachin]

Edited by proposal-police: This proposal was edited at 2024-10-08 20:21:18 UTC.

Proposal

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

"Approve" button appears even if the report has a violation on it

What is the root cause of that problem?

We do not check for Violations to display or disable approve button.

App/src/components/ReportActionItem/ReportPreview.tsx

Lines 334 to 336 in c1fa2fd

	const shouldShowApproveButton = useMemo(() => IOU.canApproveIOU(iouReport, policy), [iouReport, policy]);
	const shouldDisableApproveButton = shouldShowApproveButton && !ReportUtils.isAllowedToApproveExpenseReport(iouReport);

App/src/components/MoneyReportHeader.tsx

Lines 126 to 128 in f30b745

	const shouldShowApproveButton = useMemo(() => IOU.canApproveIOU(moneyRequestReport, policy), [moneyRequestReport, policy]);
	const shouldDisableApproveButton = shouldShowApproveButton && !ReportUtils.isAllowedToApproveExpenseReport(moneyRequestReport);

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

Add violation check for approve button same as submit button here.

App/src/components/ReportActionItem/ReportPreview.tsx

Line 182 in c1fa2fd

const shouldShowSubmitButton = isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage;

Append && !showRTERViolationMessage here(based of if we want to hide or disable the button)

App/src/components/ReportActionItem/ReportPreview.tsx

Lines 334 to 336 in c1fa2fd

	const shouldShowApproveButton = useMemo(() => IOU.canApproveIOU(iouReport, policy), [iouReport, policy]);
	const shouldDisableApproveButton = shouldShowApproveButton && !ReportUtils.isAllowedToApproveExpenseReport(iouReport);

And Append && !hasAllPendingRTERViolations here(based of if we want to hide or disable the button)

App/src/components/MoneyReportHeader.tsx

Lines 126 to 128 in f30b745

	const shouldShowApproveButton = useMemo(() => IOU.canApproveIOU(moneyRequestReport, policy), [moneyRequestReport, policy]);
	const shouldDisableApproveButton = shouldShowApproveButton && !ReportUtils.isAllowedToApproveExpenseReport(moneyRequestReport);

Note

by using this check we would only stop approval if violationType==='RTER' same as submit button ,
if we want to stop user for violationType==='Notice' use additional check using TransactionUtils.hasNoticeTypeViolation()
for violationType==='Warning' use additional check using TransactionUtils.hasWarningTypeViolation()
for violationType==='Violation' use additional check using TransactionUtils.hasViolation()
IMO these we should add check for violationType==='Violation' and ignore warningType and noticeType.

Optionally we could also implement the same for the submit button so employee could not submit the expense with the violation in the first place.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[ChavdaSachin]

Updated proposal
added important info in Note section

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021845657010504568512

[melvin-bot]

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

[hoangzinh]

Thank you for your proposals, everyone. I think @abzokhattab's proposal looks good to me. Both proposals pointed out root cause exactly, but @abzokhattab's solution (alternative) avoids repeat condition check violations (DRY)

Link to proposal #50479 (comment)

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[ChavdaSachin]

@hoangzinh I agree we could make changes to canApproveIOU function if we only want to make changes to approve button and still let user submit without any restriction.
Curious, do we want to only prevent approvals? or should we prevent submissions as well?

[hoangzinh]

Curious, do we want to only prevent approvals? or should we prevent submissions as well?

It's a good call. I will let @Gonals decide on it but in my opinion, because we haven't had an issue with the "submit" button so I think it's fine with the current violation checking for that button 👀 (I'm unsure besides RTERViolation, do we have any violations that can occur before a requester submit an expense)

[Gonals]

Curious, do we want to only prevent approvals? or should we prevent submissions as well?

It's a good call. I will let @Gonals decide on it but in my opinion, because we haven't had an issue with the "submit" button so I think it's fine with the current violation checking for that button 👀 (I'm unsure besides RTERViolation, do we have any violations that can occur before a requester submit an expense)

Agreed 👍

[melvin-bot]

📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @abzokhattab 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[ChavdaSachin]

One more point I missed to inform - the date violation shown in video does not fall under RTERViolations.

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

• Hide approve/pay buttons if the report contains violations #51133

If no regressions arise, payment will be issued on 2024-12-05. 🎊

For reference, here are some details about the assignees on this issue:

• @hoangzinh requires payment automatic offer (Reviewer)
• @abzokhattab requires payment automatic offer (Contributor)

[melvin-bot]

@hoangzinh @adelekennedy @hoangzinh The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[melvin-bot]

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

• Hide approve/pay buttons if the report contains violations #51133

If no regressions arise, payment will be issued on 2024-12-07. 🎊

For reference, here are some details about the assignees on this issue:

• @hoangzinh requires payment automatic offer (Reviewer)
• @abzokhattab requires payment automatic offer (Contributor)

[melvin-bot]

@hoangzinh @adelekennedy @hoangzinh The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[melvin-bot]

@hoangzinh, @Gonals, @abzokhattab, @adelekennedy Huh... This is 4 days overdue. Who can take care of this?

[hoangzinh]

Confirming on slack https://expensify.slack.com/archives/C049HHMV9SM/p1734346655468849?thread_ts=1728406913.463479&cid=C049HHMV9SM

[adelekennedy]

I think we want to close here - but payment is still due

[hoangzinh]

Agree @adelekennedy

[adelekennedy]

Upwork payments made - closing