[$500] Web - Admin can submit and approve their own report when "Prevent Self-Approval" is on

[kbecciv]

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: v1.4.40-4
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: Applause - Internal Team
Slack conversation:

Issue found when executing PR #34450

Action Performed:

Precondition:

• Admin is in the Collect workspace.
• "Prevent Self-Approval" is enabled.

1. On OD, create a report and add an expense to it.
2. Copy the report ID n OD, and paste it after https://staging.new.expensify.com/r/
3. On ND, submit and approve the report.

Expected Result:

Admin will be prevented from submitting and approving the report as indicated by this line " Looks like you're submitting to yourself. Approving your own reports is forbidden by your policy." in Next step message.

Actual Result:

Admin is able to submit and approve their own report when "Prevent Self-Approval" is enabled.

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

Bug6378275_1707840606490.20240214_000634.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011beabd7ddef2a97c
• Upwork Job ID: 1757449822623985664
• Last Price Increase: 2024-02-20
• Automatic offers:
• alitoshmatov | Reviewer | 0
• barros001 | Contributor | 0

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[kbecciv]

We think that this bug might be related #wave7-collect-approvers
CC @RachCHopkins

[jeremy-croff]

Proposal

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

We are able to approve reports in newdot when we configured on old dot to now allow this.

What is the root cause of that problem?

We are not using the logic
isPreventSelfApprovalEnabled && isSelfApproval to decide whether or not to disable to approval action. We are using this condition however to decide whethhr or not to show the information that its not allowed.

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

We can use the condition that determines isPreventSelfApprovalEnabled && isSelfApproval to disable to approval of the report. The back end is catching this error however we should prevent the user action.

We can additionally decide to just remove the approval button as well if this condition is met.

The conditions that determine the rendering of the button is found here:

App/src/components/SettlementButton.tsx

Line 36 in 19e2849

type SettlementButtonProps = SettlementButtonOnyxProps & {

What alternative solutions did you explore? (Optional)

[barros001]

Proposal

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

Admin is allowed to approve it's own expense report even when "Prevent Self-Approval" is enabled.

What is the root cause of that problem?

The FE does not check is the user is self approving and always display the submit button. This can be seen here:

App/src/components/MoneyReportHeader.tsx

Line 81 in d50d603

const shouldShowSubmitButton = isDraft && reimbursableSpend !== 0;

App/src/components/MoneyReportHeader.tsx

Lines 133 to 142 in d50d603

	{shouldShowSubmitButton && !isSmallScreenWidth && (
	<View style={styles.pv2}>
	<Button
	medium
	success={isWaitingForSubmissionFromCurrentUser}
	text={translate('common.submit')}
	style={[styles.mnw120, styles.pv2, styles.pr0]}
	onPress={() => IOU.submitReport(moneyRequestReport)}
	/>
	</View>

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

We create a new method on ReportUtils.ts called isAllowedToSumitDraftExpenseReport and use it as part of the shouldShowSubmitButton logic.

function isAllowedToSumitDraftExpenseReport(report: OnyxEntry<Report>): boolean {
    const policy = getPolicy(report?.policyID);
    const {submitsTo, isPreventSelfApprovalEnabled} = policy;

    // return isPreventSelfApprovalEnabled && isSelfApproval
    const isSelfApproval = currentUserAccountID === submitsTo;

    return !isPreventSelfApprovalEnabled || !isSelfApproval;
}

We then enable/disable the Submit button based on the value of isAllowedToSumitDraftExpenseReport , as follows:

                        <Button
                            medium
                            success={isWaitingForSubmissionFromCurrentUser}
                            text={translate('common.submit')}
                            style={[styles.mnw120, styles.pv2, styles.pr0]}
                            onPress={() => IOU.submitReport(moneyRequestReport)}
                            isDisabled={!ReportUtils.isAllowedToSumitDraftExpenseReport(chatReport)}
                        />

Note that there are two buttons on the same component (mobile/desktop) and the logic must be applied to both.

Similar logic needs to be implemented on ReportPreview.tsx to hide/show or disable/enable the Submit button from the report preview.

Here's a couple of screenshots:

There's also another issue here with the APPROVAL. Currently the front end will show the "Oops, you can't submit to yourself" message but if it's already submitted, it will not show a similar message when the next action is "Approve". See screenshot.

If you click approve the backend won't let you because you can't approve your own reports.

I propose we apply a similar logic to check for self-approval here:

App/src/libs/NextStepUtils.ts

Lines 188 to 243 in d50d603

	case CONST.REPORT.STATUS_NUM.SUBMITTED: {
	const verb = isManager ? 'review' : 'approve';
	// Another owner
	optimisticNextStep = {
	type,
	title: 'Next Steps:',
	message: [
	{
	text: ownerLogin,
	type: 'strong',
	},
	{
	text: ' is waiting for ',
	},
	{
	text: 'you',
	type: 'strong',
	},
	{
	text: ' to ',
	},
	{
	text: verb,
	type: 'strong',
	},
	{
	text: ' these %expenses.',
	},
	],
	};
	// Self review & another reviewer
	if (isOwner) {
	optimisticNextStep.message = [
	{
	text: 'Waiting for ',
	},
	{
	text: managerDisplayName,
	type: 'strong',
	},
	{
	text: ' to ',
	},
	{
	text: verb,
	type: 'strong',
	},
	{
	text: ' %expenses.',
	},
	];
	}
	break;

Using similar logic we have here:

App/src/libs/NextStepUtils.ts

Lines 163 to 182 in d50d603

	if (isPreventSelfApprovalEnabled && isSelfApproval) {
	optimisticNextStep.message = [
	{
	text: "Oops! Looks like you're submitting to ",
	},
	{
	text: 'yourself',
	type: 'strong',
	},
	{
	text: '. Approving your own reports is ',
	},
	{
	text: 'forbidden',
	type: 'strong',
	},
	{
	text: ' by your policy. Please submit this report to someone else or contact your admin to change the person you submit to.',
	},
	];

For the sake of DRY, make the logic above use the new isAllowedToSumitDraftExpenseReport (open to better name here as it's gonna be used for both submit and approve). method as well

Once this part is taken care of we also have to disable the approve button on the FE. We can do this by passing isDisabled="true" to the <SettlementButton> component on both ReportPreview.tsx and MoneyReportHeader.tsx.

There might also be yet another issue where the FE is allowing us to Pay before we approve:

If this is really not intentional, we need to update the shouldShowPayButton logic here:

App/src/components/ReportActionItem/ReportPreview.tsx

Lines 219 to 222 in 9c4c4ac

	const shouldShowPayButton = useMemo(
	() => isPayer && !isDraftExpenseReport && !iouSettled && !iouReport?.isWaitingOnBankAccount && reimbursableSpend !== 0 && !iouCanceled && !isAutoReimbursable,
	[isPayer, isDraftExpenseReport, iouSettled, reimbursableSpend, iouCanceled, isAutoReimbursable, iouReport],
	);

And here:

App/src/components/MoneyReportHeader.tsx

Lines 79 to 82 in 9c4c4ac

	const shouldShowPayButton = useMemo(
	() => isPayer && !isDraft && !isSettled && !moneyRequestReport.isWaitingOnBankAccount && reimbursableSpend !== 0 && !ReportUtils.isArchivedRoom(chatReport) && !isAutoReimbursable,
	[isPayer, isDraft, isSettled, moneyRequestReport, reimbursableSpend, chatReport, isAutoReimbursable],
	);

To check if report is approved or not.

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.

[alitoshmatov]

@jeremy-croff Thank you for your proposal. There is not much details in your proposal and you are only suggesting to disable approve button whereas in OP expected result is to prevent user from submitting proposal to themself when self approving is disabled.

[alitoshmatov]

@barros001 Thank you for your proposal. Did you mean to say isAllowedToSumitDraftExpenseReport instead of isAllowedToApproveDraftExpenseReport? Other than that I do agree with your approach, but still we have to also disable approve button for cases where prevent self approval is disabled after submitting report.

[barros001]

@barros001 Thank you for your proposal. Did you mean to say isAllowedToSumitDraftExpenseReport instead of isAllowedToApproveDraftExpenseReport? Other than that I do agree with your approach, but still we have to also disable approve button for cases where prevent self approval is disabled after submitting report.

Yes, that was an oversight of mine. I'll update the proposal to reflect that. We can apply the same/similar logic to disable the approve button.

[jeremy-croff]

So it looks like I have the steps for approval, and @barros001 for submit. And we need a combination of both.

[melvin-bot]

@kevinksullivan, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[barros001]

Proposal

Updated

Added other scenarios that need to be accounted for such as displaying a message saying that user can't approve their own report, disabling the Approve button in this scenario and also removing the payment buttons if report is not yet approved (pending determination is that's really how it should behave).

[alitoshmatov]

We can go with @barros001 's proposal

@barros001 One note, can't we just not show approve button?

C+ reviewed 🎀 👀 🎀

[melvin-bot]

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

[barros001]

@barros001 One note, can't we just not show approve button?

Do you mean hiding the approve button on this drop-down?

We could, but I don't think any of these options should really be available if the report is not approved. If I try clicking Pay, backend won't let me (BE doesn't return any specific message but I'm assuming it's because you can't pay it until it's approved), so disabling the main button so the entire drop-down is disabled sounds like the right thing here.

Unless you mean instead of disabling, just hiding the button (both the settlement dropdown - which contains the approve - and the submit button). That could work as well and was in fact my first thought, but then I thought this would look weird without a button:

Let me know what you think is best and we can adjust it on the PR.

[alitoshmatov]

I was meaning whole button with dropdown. But let's see what @lakchote thinks about it

[melvin-bot]

This issue has not been updated in over 15 days. @barros001, @lakchote, @kevinksullivan, @alitoshmatov eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[alitoshmatov]

The PR went into production in 27th of march, only payment is left.
#38889 - This issue was also related. Not sure if we are going to treat it as regression and apply penalty here

[barros001]

Not sure if we are going to treat it as regression and apply penalty here

I was wondering the same as it didn’t technically break something but rather was a missed edge case during implementation.

[barros001]

Friendly bump.

[lakchote]

I'd say it's not a regression and more of an edge case that wasn't properly handled.

Thoughts @kevinksullivan?

[alitoshmatov]

@kevinksullivan Friendly bump

[barros001]

@kevinksullivan @lakchote @alitoshmatov looks like this bug slip through the cracks. Just wanted to bump it one more time.

[lakchote]

DMed @kevinksullivan to make sure this is not missed.

[kevinksullivan]

Sorry for the delay, I missed this.

[kevinksullivan]

had to create a new job @barros001 @alitoshmatov . Lmk when you accept

https://www.upwork.com/ab/applicants/1793302649633525760/job-details

[barros001]

@kevinksullivan accepted, thanks!

[kevinksullivan]

paid @barros001

[alitoshmatov]

Accepted the offer

[alitoshmatov]

@kevinksullivan Bump

[mallenexpensify]

Contributor: @barros001 paid $500 via Upwork (by Kev, as mentioned above)
Contributor+: @alitoshmatov paid $500 via Upwork

This looks like a bug we'd want a regression test for, do you agree @alitoshmatov ? IF so, please fill out the BZ checklist below.

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@] The PR that introduced the bug has been identified. Link to the PR:
• [@] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@] Determine if we should create a regression test for this bug.
• [@] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: [NoQA] feat: show submit button in MoneyReportHeader when report is in draft #28998
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/28998/files#r1650175146
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No need
• Determine if we should create a regression test for this bug. No need

[alitoshmatov]

@mallenexpensify Completed the checklist

[mallenexpensify]

Are we should we don't want a regression test for this? It was a bug that didn't come from an existing regression test.
@alitoshmatov can you provide reasoning why you don't think we need?
@lakchote , what do you think? Here's the PR

• Disable Submit/Approve buttons when self approving is disabled and user is trying to self approve

[kevinksullivan]

Waiting for @alitoshmatov response here

[alitoshmatov]

@mallenexpensify I think regression tests created in this #28771 should be sufficient.

@mallenexpensify Yep, we'll have to create a new one. The test steps given in the PR description #34450 look comprehensive enough to me. Let me know if you need any help with those.

Based on this comment I think existing regression test steps are the same as test steps in #34450 which caught this issue as OP states:

Issue found when executing PR #34450

But we can create a new regression test specifically for this case if needed, let me know your thoughts

[mallenexpensify]

Thanks @alitoshmatov , I agree, closing
TR GH is here - https://github.com/Expensify/Expensify/issues/372499

Section is likely this one (there are many)