[C+ Checklist Needs Completion] [$500] IOU - Approve button appears while approvals is disabled

izarutskaya commented 5 months ago

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: 1.4.57-2 Reproducible in staging?: Y Reproducible in production?: Y Found when executing PR : https://github.com/Expensify/App/pull/38253 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Pre-requisites: Have a collect WS and make chat enabled on ND Have 2 accounts (one admin, one employee) Having `Make or track payments enabled Toggle on delay submissions on ND (Logged in as the admin) Toggle off Add approvals On OD Make reimbursement direct with a VBBA

[Employee] Request money from the workspace chat and submit it
[Admin] Click on the IOU preview and observe the header

Expected Result:

"Pay with expensify" button should show up since this is the case where Delay submissions is enabled and approvals is disabled

Actual Result:

"Approve" button appears on the header

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/4c0144dc-f6e3-4455-9c33-2db66596918b

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~014fc34bb4b2bfbb10
Upwork Job ID: 1773511332097155072
Last Price Increase: 2024-03-29
Automatic offers:

DylanDylann | Reviewer | 103218707

Issue Owner

Current Issue Owner: @greg-schroeder

melvin-bot[bot] commented 5 months ago

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

izarutskaya commented 5 months ago

We think this issue might be related to the #collect project.

allgandalf commented 5 months ago

Proposal

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

Approve button appears even when approvals are not required

What is the root cause of that problem?

In canApproveIOU we do not check if the approvalMode and autoReportingFrequency are optional: https://github.com/Expensify/App/blob/4fbd2184458a9cbfee0380b571fc95bff042c0af/src/libs/actions/IOU.ts#L4500 https://github.com/Expensify/App/blob/4fbd2184458a9cbfee0380b571fc95bff042c0af/src/libs/actions/IOU.ts#L4523-L4524

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

Add a check in canApproveIOU to check if the approval mode and delay submittion are Optional and if it is then return the value:

diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts
index b67e44fcf5..c361ed3e5e 100644
--- a/src/libs/actions/IOU.ts
+++ b/src/libs/actions/IOU.ts
@@ -4519,8 +4519,10 @@ function canApproveIOU(iouReport: OnyxEntry<OnyxTypes.Report> | EmptyObject, cha
     const isOpenExpenseReport = isPolicyExpenseChat && ReportUtils.isOpenExpenseReport(iouReport);
     const isApproved = ReportUtils.isReportApproved(iouReport);
     const iouSettled = ReportUtils.isSettled(iouReport?.reportID);
+    const isApprovalOptional = policy?.approvalMode === CONST.POLICY.APPROVAL_MODE.OPTIONAL;
+    const canDelaySubmissions = policy?.autoReportingFrequency !== CONST.POLICY.AUTO_REPORTING_FREQUENCY.IMMEDIATE &&  policy?.autoReportingFrequency !== CONST.POLICY.AUTO_REPORTING_FREQUENCY.INSTANT;

-    return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled;
+    return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !(isApprovalOptional && canDelaySubmissions);
 }

What alternative solutions did you explore? (Optional)

N/A

bernhardoj commented 5 months ago

Proposal

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

The approve button is still displayed even though the add approvals option is disabled.

What is the root cause of that problem?

We already have a condition here to not show the approve button if the approval is disabled (isOnSubmitAndClosePolicy). https://github.com/Expensify/App/blob/4fbd2184458a9cbfee0380b571fc95bff042c0af/src/libs/actions/IOU.ts#L4509-L4513

However, the auto-reporting frequency needs to be instant (isOnInstantSubmitPolicy) to not show the approval button. In our case, the frequency is set to WEEKLY, so the condition is never met.

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

Change the condition to accept either isOnInstantSubmitPolicy or isOnSubmitAndClosePolicy.

if (isOnInstantSubmitPolicy || isOnSubmitAndClosePolicy) {

Or we can remove isOnInstantSubmitPolicy

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

DylanDylann commented 5 months ago

@bernhardoj's proposal looks good to me

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 5 months ago

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

stitesExpensify commented 5 months ago

@bernhardoj do you know why we initially implemented it to only be disabled when isOnInstantSubmitPolicy? That seems like it was done intentionally, and I just want to make sure we aren't going to break something

bernhardoj commented 5 months ago

It was first implemented in https://github.com/Expensify/App/pull/35470, but looks like in this PR https://github.com/Expensify/App/pull/38253 (where the QA found the issue), the test step expect the approve button to not show.

cc: @luacmartins @lakchote

luacmartins commented 5 months ago

We should not show the approve button if approvals are disabled

DylanDylann commented 5 months ago

@luacmartins As mentioned here

Approver shouldn't be able to "Approve" an expense report IFF the report's policy is a Collect Policy, on "Instant Submit" reporting frequency, with "Submit & Close" approval mode.

Do we want to display the Approve button if users are in approval mode and not in "Instant Submit" reporting frequency

cc @Beamanator

Beamanator commented 5 months ago

Approver shouldn't be able to "Approve" an expense report IFF the report's policy is a Collect Policy, on "Instant Submit" reporting frequency, with "Submit & Close" approval mode.

Agreed (I wrote that lol)

Do we want to display the Approve button if users are in approval mode and not in "Instant Submit" reporting frequency

To be clear, you're asking "if users are in approval mode Submit & Close", right? I believe in that case we wouldn't want to show the "Approve" button either

Because "Submit & Close" basically means: After a submitter submits a report, what next? We close the report, we don't wait for the admin to Approve it

bernhardoj commented 5 months ago

Because "Submit & Close" basically means: After a submitter submits a report, what next? We close the report, we don't wait for the admin to Approve it

That makes sense, but currently, the condition expects the instant submit is on too to hide the approve button. https://github.com/Expensify/App/blob/ebc8de8229fa700e5a7d08f4589db934a79d4f1d/src/libs/actions/IOU.ts#L4599-L4601

I think it makes sense if we remove the instant submit condition because whether it's an instant or manual submit, the report will be closed immediately, right?

Beamanator commented 5 months ago

I tentatively think that definitely makes sense 😅 👍

greg-schroeder commented 5 months ago

Discussion continues here - @stitesExpensify @Beamanator we cool with @bernhardoj's approach to solve this one?

Beamanator commented 5 months ago

I say let's move forward with always disabling / hiding the Approve button for Submit & Close approval mode, carlos agreed earlier too 👍

rojiphil commented 5 months ago

I say let's move forward with always disabling/hiding the Approve button for Submit & Close approval mode

Oh! If this is where we are heading to, then there is already a PR here for this. Not sure of the specifics of this issue but would the PR solve the issue here?

Beamanator commented 5 months ago

Ooh good catch! @greg-schroeder do you mind checking if this bug still exists? Maybe that linked PR ^ already fixed this!

greg-schroeder commented 5 months ago

Okay that was just deployed to prod. Asked QA to re-test here: https://expensify.slack.com/archives/C9YU7BX5M/p1712613747421269

melvin-bot[bot] commented 4 months ago

@greg-schroeder @stitesExpensify @DylanDylann 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!

stitesExpensify commented 4 months ago

Waiting for a re-test

mvtglobally commented 4 months ago

Issue not reproducible during KI retests. (First week)

greg-schroeder commented 4 months ago

Aha, looks like it's no longer a thing! Closing accordingly per the above. Please reopen if you disagree and explain your reasoning!

lanitochka17 commented 1 month ago

Issue is still reproducible on the latest build 9.0.9-2

https://github.com/user-attachments/assets/c1731e4c-7d7c-43df-888a-8acd3cc99890

wildan-m commented 1 month ago

Proposal

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

IOU - Approve button appears while approvals is disabled

What is the root cause of that problem?

In canApproveIOU function we are not checking if the policy isSubmitAndClose

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

Add !PolicyUtils.isSubmitAndClose(policy) to canApproveIOU result.

    const result = isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !PolicyUtils.isSubmitAndClose(policy);

What alternative solutions did you explore? (Optional)

greg-schroeder commented 1 month ago

Back into proposal review

DylanDylann commented 1 month ago

@bernhardoj's proposal looks good to me

🎀 👀 🎀 C+ reviewed

Note that: The mentioned PR is another case and It doesn't fix this bug

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @DylanDylann 🎉 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

bernhardoj commented 1 month ago

PR is ready

cc: @DylanDylann

greg-schroeder commented 1 month ago

PR is being reviewed

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/46044

If no regressions arise, payment will be issued on 2024-08-09. :confetti_ball:

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

@bernhardoj requires payment through NewDot Manual Requests
@DylanDylann requires payment automatic offer (Reviewer)

melvin-bot[bot] commented 1 month ago

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:

[ ] [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@DylanDylann] 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:
[ ] [@DylanDylann] 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:
[ ] [@DylanDylann] Determine if we should create a regression test for this bug.
[ ] [@DylanDylann] 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.
[ ] [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

greg-schroeder commented 4 weeks ago

Payment summary:

Contributor: @bernhardoj - $500 - You can make a manual request C+: @DylanDylann - $500 - Paying through Upwork

greg-schroeder commented 4 weeks ago

Please complete the checklist @DylanDylann when you're able!

bernhardoj commented 4 weeks ago

Requested in ND.

JmillsExpensify commented 4 weeks ago

$500 approved for @bernhardoj

DylanDylann commented 4 weeks ago

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:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/35470 [@DylanDylann] 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: The author explained here [@DylanDylann] 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: NA [@DylanDylann] Determine if we should create a regression test for this bug. Yes [@DylanDylann] 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.

Regression Test Proposal

Prerequisite: Admin has a collect workspace and turns off the approval and sets the Delayed submission to MANUAL (or other than INSTANT)

As an employee, add a money request and submit it to the workspace expense chat
As the admin, open the workspace chat (refresh or reopen the app if needed to get the latest data)
Verify the pay button shows instead of the approve button on the report preview

Do we agree 👍 or 👎

greg-schroeder commented 3 weeks ago

Filed regression test, closing

Expensify / App