[HOLD for payment 2024-10-30] [$250] 2nd-level+ Non-admin approver should NOT be able to Submit draft reports on behalf of their submitters

[garrettmknight]

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: N/A Reproducible in staging?: Y Reproducible in production?: Y Logs: logs-2024-09-13 16_55_08.340.txt Expensify/Expensify Issue URL: Issue reported by: @saracouto Slack conversation: https://expensify.slack.com/archives/C06ML6X0W9L/p1725864497268339

Action Performed:

1. Create a workspace
2. Invite a submitter and 2 approvers (both NON-ADMINS)
3. Enable approvals setting Submitter -> submitsTo approver 1 -> forwardsTo approver 2
4. Enable Delay submissions
5. Create an expense as the submitter
6. As the approver 1, submit & approve the report
7. As the approver 2, navigate to the submitter's workspace chat

Expected Result:

As a second-level+ non-admin approver, you should NOT see the 'submit' button.

Actual Result:

As a non-admin approver, you see the 'submit' button. When you tap/click it, you get an error.

Workaround:

There is no workaround.

Platforms:

All

Screenshots/Videos

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021833163296746944359
• Upwork Job ID: 1833163296746944359
• Last Price Increase: 2024-10-10
• Automatic offers:
• ikevin127 | Reviewer | 104458910
• cretadn22 | Contributor | 104458913

Issue Owner

Current Issue Owner: @greg-schroeder

[melvin-bot[bot]]

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

[NJ-2020]

Edited by proposal-police: This proposal was edited at 2024-09-09 12:14:39 UTC.

Proposal

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

Non-admin approver shouldn't see the Submit option for submitters

What is the root cause of that problem?

Right here https://github.com/Expensify/App/blob/498d45cc55bae4dde9f85036c75d5a421e0469e9/src/components/ReportActionItem/ReportPreview.tsx#L187 We show the submit button if the expense is open and the reimbursable spend is not equal to zero and doesn't have any violation message

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

We can check if the report owner is equal to currentAccountID i.e iouReport.ownerAccountID is equal to currentAccountID then we will show the submit button

Or we will show the submit button if it's the admin by using PolicyUtils.isPolicyAdmin

What alternative solutions did you explore? (Optional)

[ChavdaSachin]

Edited by proposal-police: This proposal was edited at 2024-09-09 16:40:04 UTC.

Proposal

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

Submit button is enabled for non-admin approver to submit expanse on behalf of member

What is the root cause of that problem?

• When checking condition, whether submit button should be disabled here, ReportUtils.isAllowedToSubmitDraftExpenseReport function is used to check permissions. https://github.com/Expensify/App/blob/498d45cc55bae4dde9f85036c75d5a421e0469e9/src/libs/ReportUtils.ts#L7380-L7385
• So this function allows all approver(includes admin and owner) to submit on behalf of employee.

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

• disable submit button for non-admin non-policyOwner approver
• Include a new function isAdminOwnerOrReportOwner(similar to isAdminOwnerApproverOrReportOwner under ReportUtils.ts to check whether user is reportOwner or policyAdmin or policyOwner.

  function isAdminOwnerOrReportOwner(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>): boolean {
  return PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPolicyOwner(policy, currentUserAccountID ?? -1) || isReportOwner(report);
  }

• Replace isAllowedToApproveExpenseReport with isAdminOwnerOrReportOwner and remove redundant line.

  function isAllowedToSubmitDraftExpenseReport(report: OnyxEntry<Report>): boolean {
  const policy = getPolicy(report?.policyID);
  
  return isAdminOwnerOrReportOwner(report, policy);
  }

  https://github.com/Expensify/App/blob/498d45cc55bae4dde9f85036c75d5a421e0469e9/src/libs/ReportUtils.ts#L7380-L7385

  Result

  

What alternative solutions did you explore? (Optional)

• If we agree upon hiding submit button rather then disabling the button -
• From shouldDisableSubmitButton check ReportUtils.isAllowedToSubmitDraftExpenseReport(moneyRequestReport) could be migrated to shouldShowSubmitButton https://github.com/Expensify/App/blob/474d15e886f950a7aa9513bdcc12f57bfd96b612/src/components/MoneyReportHeader.tsx#L126 https://github.com/Expensify/App/blob/474d15e886f950a7aa9513bdcc12f57bfd96b612/src/components/MoneyReportHeader.tsx#L133

[nyomanjyotisa]

Edited by proposal-police: This proposal was edited at 2024-10-09 00:10:21 UTC.

Proposal

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

Second-level and above approvers that are not admin shouldn't see the Submit option for submitters

What is the root cause of that problem?

We don't hide the submit button if the current user is second-level or above approvers and not admin https://github.com/Expensify/App/blob/498d45cc55bae4dde9f85036c75d5a421e0469e9/src/components/ReportActionItem/ReportPreview.tsx#L187

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

Only show the submit button if the current user is policy admin or report owner or first approver

    const [approvalWorkflow] = useOnyx(ONYXKEYS.APPROVAL_WORKFLOW);
    const session = useSession();
    const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
    const firstApprover = approvalWorkflow.approvers.at(0);
    const shouldShowSubmitButton = isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage && (iouReport?.ownerAccountID == session?.accountID || isAdmin || firstApprover);

We should completely hide it, instead of disabling it

What alternative solutions did you explore? (Optional)

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[Beamanator]

Assigning External for a C+ to help with this one 🙏

[Beamanator]

oops solution is still being discussed in slack (here), so plz no more proposals for now 🙏

[cretadn22]

Before moving forward with the proposal, I want to verify our expectations first. It seems that the isAllowedToApproveExpenseReport and isAllowedToSubmitDraftExpenseReport functions might be incorrect following the implementation of the advanced approval workflow.

Please review the following points to confirm:

1. The isAllowedToSubmitDraftExpenseReport function should return true in one of the following cases:

• If the current user is the request creator.
• If the current user is either a policy owner or a policy admin.

2. For the isAllowedToApproveExpenseReport function:

Note: The approve button only appears if the "Add approvals" option is enabled in the workflow.

• If the current user is listed as an approver for the submitter (see in workflow > Add approvals), the function should return true.
• If preventSelfApproval is set to true and the current user is the submitter, the function should return false.

[cretadn22]

@Beamanator thanks for the information! I also plan to confirm the expectations with you.

[ChavdaSachin]

Proposal

updated Added alternate solution.

[NJ-2020]

Proposal Updated

[melvin-bot[bot]]

@Beamanator, @greg-schroeder, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[greg-schroeder]

@Beamanator @garrettmknight where do you think we've landed here - should we still hold on proposals?

[Beamanator]

Good question, looks like garrett might be looking into creating new issues / updating existing issues for the agreed upon solution - see https://expensify.slack.com/archives/C06ML6X0W9L/p1725978630280069?thread_ts=1725864497.268339&cid=C06ML6X0W9L

[garrettmknight]

I've updated the OP with the new expected behavior. This is back open for proposals.

[garrettmknight]

I've also added logs to the OP.

[aldo-expensify]

As a non-admin approver, you see the 'submit' button. When you tap/click it, you get an error.

We think this is the error: https://github.com/Expensify/Expensify/issues/428170. This feels like something we have to fix in the backend so it is allowed and doesn't throw an error (Internal, not External).

[Beamanator]

this will probably have a bit of internal and a bit of external stuff

[greg-schroeder]

Okay, so we're into proposal review still

[Beamanator]

Goood question @greg-schroeder , can you tell if the Submit button shows for all approvers of a report?

[greg-schroeder]

Hmm. It's actually a bit difficult to test all cases of this, but I think it does show for all approvers from what I can see in my test account

[garrettmknight]

@Beamanator Submit has been showing for first and second-level approvers for me.

[Beamanator]

Groovy, so still the possible external component could be: If first and second-level approvers click Submit offline, what happens

[greg-schroeder]

Okay, understood

[melvin-bot[bot]]

@Beamanator @greg-schroeder @ikevin127 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[greg-schroeder]

This is still awaiting someone to take it on I believe

[Beamanator]

Ok so what I'm seeing so far is this... Say we have Submitter -> Approver A -> Approver B.

1. When Approver A submits, everything works fine 👍 (all the way through reimbursing)
2. ^ Same when an admin submits
3. When Approver B tries to submit...
   1. Offline everything seems to work well -> So I think we can remove External here 👍
   2. Online, we throw an exception... This is because we CURRENTLY only allow admins or current approvers to "force submit"
   3. Even in OldDot we hide the Submit button for non-admin non-manager approvers:

https://github.com/user-attachments/assets/7d17997d-1235-4f5c-9f67-d8c8a8dc3dec

[Beamanator]

cc @garrettmknight ^ So it looks like if we want to keep NewDot consistent with OldDot, the only change we'd have to make is: _For non-admin, non-manager approvers (a.k.a. for second-level approvers & up who aren't admins), HIDE Submit button from NewDot. Thoughts? Otherwise we'd have to update how OldDot works too

[greg-schroeder]

@garrettmknight What do you think about the above?

[greg-schroeder]

gentle bump @garrettmknight!

[cretadn22]

cc @garrettmknight ^ So it looks like if we want to keep NewDot consistent with OldDot, the only change we'd have to make is: _For non-admin, non-manager approvers (a.k.a. for second-level approvers & up who aren't admins), HIDE Submit button from NewDot. Thoughts? Otherwise we'd have to update how OldDot works too

@Beamanator To align with the OldDot process, I believe we should also allow the creator of the money request (report.ownerAccountID) to submit their expenses:

1. Admin of policy (Checking by PolicyUtils.isPolicyAdmin function)
2. The first approver (Checking by report.managerID)
3. The money request creator (Checking by report.ownerAccountID)

[melvin-bot[bot]]

@Beamanator @greg-schroeder @ikevin127 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[Beamanator]

@cretadn22 true, but isn't that already allowed right now? The creator of the money request = the submitter, who definitely should currently be able to submit

[cretadn22]

@Beamanator Could you provide the final expected outcome? Or are we waiting on @garrettmknight’s input?

[cretadn22]

I haven't seen a reply from him in the last two weeks. Not sure what is the next step here? Could I post a new proposal?

[Beamanator]

Yeah i think @garrettmknight was OOO, should be coming back in the next day or two

[garrettmknight]

Sorry for the wait here - I'd agree that all we really need to do is to ensure the Submit button shows and works for the submitter, first approver and workspace admins. Said differently, we want to hide it for second-level and above submitters, but it's probably easier to focus on who should be able to see the button imo.

[Beamanator]

No prob at all, thanks for confirming!

And just b/c i'm super picky, I have to ask: you're ok if we do "hide it [the submit button] for second-level and above submitters THAT ARE NOT ADMINS" right?

B/c technically right now, in OldDot, all admins are able to submit, I believe (even second-level and above)

[garrettmknight]

Good nitpick - I agree that we're only hiding from non-admin, second-level and above approvers.

[Beamanator]

Amazalicious, thank you sir 🙏

[Beamanator]

Ok so @ikevin127 I thinkkkkk we're 100% good to go externally here, can you confirm & if you agree, see if any existing proposals satisfy the new requirements? I'm happy to help answer any questions if you've got any! :D

[ikevin127]

@Beamanator Thanks! I noticed all previous proposals were set as Outdated (retracted), so given the new requirements discussed above, we're open for new proposals that satisfy the requirements.

cc @cretadn22 @nyomanjyotisa @NJ-2020 If still interested 🥇

[Beamanator]

Love it 👍 And if we don't get updated proposals by EOD, tomorrow I'll probably add Help Wanted back 👍

[nyomanjyotisa]

Proposal

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

Second-level and above approvers that are not admin shouldn't see the Submit option for submitters

What is the root cause of that problem?

We don't hide the submit button if the current user is second-level or above approvers that not admin https://github.com/Expensify/App/blob/498d45cc55bae4dde9f85036c75d5a421e0469e9/src/components/ReportActionItem/ReportPreview.tsx#L187

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

Only show the submit button if the current user is policy admin or report owner or first approver

    const [approvalWorkflow] = useOnyx(ONYXKEYS.APPROVAL_WORKFLOW);
    const session = useSession();
    const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
    const firstApprover = approvalWorkflow.approvers.at(0);
    const shouldShowSubmitButton =
        isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage && (iouReport?.ownerAccountID == session?.accountID || isAdmin || firstApprover?.email == session?.email);

We should completely hide it, instead of disabling it

What alternative solutions did you explore? (Optional)

[ikevin127]

If what I've been able to put together from the new requirements is the following:

1. The roles which should see the Submit button are:
   • Workspace Owner
   • Workspace Admin(s)
   • Submitter (aka Report Owner)
   • First Approver
2. The Second Approver should NOT see the Submit button anymore.

... then @nyomanjyotisa's proposal looks good to me. The RCA is correct, the proposed solution fixes the issue.

Here's a video showing how it works with the fix:

Video

https://github.com/user-attachments/assets/10c0f490-18bb-4618-b91f-8d8b570a2a61

Note: I know there's a lot of roles to keep in mind but, the only difference that one should notice is that the Second Approver does NOT see the Submit button anymore, which currently happens on staging after the expense is submitted.

^ Related to the staging behaviour, I noticed that if we logout and login again, we won't even see the report details in the report preview anymore, just a blank report preview saying [workspaceName] owes: 🤔 (see staging video below)

Here's a video showing how things look and behave on staging vs dev (with this fix):

Videos

| Staging | Dev (with this fix) | | ------------- | ------------- | |

[!caution] If any of the looks / behaviours shown in the videos above are off or I misuderstood the issue's expected result or anything else, please feel free to correct me or hold on assignment.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @Beamanator is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[cretadn22]

Edited by proposal-police: This proposal was edited at 2024-10-09 03:49:36 UTC.

Proposal

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

Approvers at the second level and above who are not admins should not have access to the Submit option

What is the root cause of that problem?

The current code does not prevent that

https://github.com/Expensify/App/blob/498d45cc55bae4dde9f85036c75d5a421e0469e9/src/components/ReportActionItem/ReportPreview.tsx#L187

https://github.com/Expensify/App/blob/498d45cc55bae4dde9f85036c75d5a421e0469e9/src/components/MoneyReportHeader.tsx#L126

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

We need to adjust the condition in both locations.

    const currentUserAccountID = getCurrentUserAccountID()
    const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
    const shouldShowSubmitButton =
        isOpenExpenseReport &&
        reimbursableSpend !== 0 &&
        !showRTERViolationMessage &&
        (iouReport?.ownerAccountID === currentUserAccountID || isAdmin || iouReport?.managerID === currentUserAccountID);

What alternative solutions did you explore? (Optional)

To check first approver, beside using managerID as I mentioned on the main solution we also can use getApprovalChain function and get the first element

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.

[cretadn22]

@ikevin127 The chosen proposal doesn't seem to work for me

    const [approvalWorkflow] = useOnyx(ONYXKEYS.APPROVAL_WORKFLOW);

The approvalWorkflow is utilized during the creation or editing of the approval workflow. In this case, approvalWorkflow will always be null. Could you please test it again?

[cretadn22]

@ikevin127 Your video is effective because the selected proposal only changes the reportPreview, not the MoneyReportHeader. However, your test was conducted on the MoneyReportHeader.