[HOLD for payment 2024-09-06] [$250] Workflows - "Approver" field disappears after unselecting approver

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v9.0.22-5
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace settings > Workflows.
3. Click on the modal under Add approvals.
4. Click Approver
5. Unselect the approver and click Save.

Expected Result:

There should be "Approver" field in "Edit approval workflow" page after removing the approver so that approver can be reselected.

Actual Result:

There is no "Approver" field in "Edit approval workflow" page after removing the approver.
The other field "Additional approver" requires an upgrade to access.

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6576985_1724154419888.20240820_194200.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ac1b073e9e5491ad
• Upwork Job ID: 1826615705245101757
• Last Price Increase: 2024-08-22

Issue Owner

Current Issue Owner: @abekkala

[melvin-bot]

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

[IuliiaHerets]

We think that this bug might be related to #wave-collect - Release 2

[IuliiaHerets]

@abekkala FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[bernhardoj]

Proposal

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

The approver field disappears when unselecting the approver.

What is the root cause of that problem?

The approver field is shown based on the approver array.

App/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx

Lines 130 to 134 in c7ec59b

	{approvalWorkflow.approvers.map((approver, approverIndex) => {
	const errorText = approverErrorMessage(approver, approverIndex);
	const hintText = !errorText && approverHintMessage(approver, approverIndex);
	return (
	<MenuItemWithTopDescription

When we unselect the approver, the array is empty, thus, nothing is shown.

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

If the approver is empty, we can render a menu item with minimum details.

{approvalWorkflow.approvers.length > 0 ? (
    approvalWorkflow.approvers.map((approver, approverIndex) => {
        return ...
    })
) : (
    <MenuItemWithTopDescription
        wrapperStyle={styles.sectionMenuItemTopDescription}
        description={translate('workflowsPage.approver')}
        onPress={() => editApprover(0)}
        shouldShowRightIcon
    />
)}

We will need to do this for create approval flow too.

[daledah]

Proposal

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

Workflows - "Approver" field disappears after unselecting approver

What is the root cause of that problem?

We allow user to unselect the first approver and save it

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

The first approver should always be mandatory in all cases. We should allow the user to update the approver but prevent them from deselecting the selected approver.

App/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsApproverPage.tsx

Line 198 in 685bb4c

isDisabled={!selectedApproverEmail && isInitialCreationFlow}

We should disable the submit button if the user doesn't select an approver on the first approver selection page

isDisabled={!selectedApproverEmail && (isInitialCreationFlow || approverIndex === 0)}

What alternative solutions did you explore? (Optional)

Do nothing if user unselect the first approver (we still allow user change the first approver, we only prevent user from unselecting)

    const toggleApprover = (approver: SelectionListApprover) => {
        if (selectedApproverEmail === approver.login) {
             if (approverIndex === 0) {
                 return;
             }
            setSelectedApproverEmail(undefined);
            return;
        }
        setSelectedApproverEmail(approver.login);
    };

[daledah]

@DylanDylann Do you take care this issue? Because I see you reviewed a couple of issues that relates to this feature

There should be "Approver" field in "Edit approval workflow" page after removing the approver so that approver can be reselected.

If yes, please confirm this expected. TIA

[DylanDylann]

@blazejkustra Oh, we've got a new little one!

In this case, If the user continues to click on the Save button. An error will be displayed

If we move forward with this issue with an external contributor, I suggest going with @daledah's proposal

The first approver should always be mandatory in all cases. We should allow the user to update the approver but prevent them from deselecting the selected approver.

[melvin-bot]

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

[melvin-bot]

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

[jjcoffee]

I think we're good to go with @bernhardoj's proposal here, as we already have an error to display if we try and save without an approver.

@daledah's proposal would result in a lack of feedback to the user.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[bernhardoj]

PR is ready

cc: @jjcoffee

[DylanDylann]

@daledah's #47723 (comment) would result in a lack of feedback to the user.

@jjcoffee Ah see your point, I gave my opinion previously to make it consistent with "Expense from" section (editing flow)

Screen.Recording.2024-08-25.at.17.46.48.mov

And the creation flow (we disable the button if there are no selected option)

If we go with @bernhardoj's solution, please also update the "Expense from" section (in the editing flow) to make it consistent. Thanks

[bernhardoj]

@jjcoffee @mountiny do you agree to change "Expense from" behavior to allow it to save without any user selected so it's the same with the Approver selection page?

[DylanDylann]

cc @JmillsExpensify

[blazejkustra]

Hey guys! Let's not overcomplicate this, we just need to change the copy on the additionalApprover input, and make sure admins can edit the default approver without the need to update the workspace to Control plan.

Not a Proposal 😆

diff --git a/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx b/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx
index 9864241a8e8..f232e971b24 100644
--- a/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx
+++ b/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx
@@ -97,7 +97,7 @@ function ApprovalWorkflowEditor({approvalWorkflow, removeApprovalWorkflow, polic
 
     // User should be allowed to add additional approver only if they upgraded to Control Plan, otherwise redirected to the Upgrade Page
     const addAdditionalApprover = useCallback(() => {
-        if (!PolicyUtils.isControlPolicy(policy)) {
+        if (!PolicyUtils.isControlPolicy(policy) && approvalWorkflow.approvers.length > 0) {
             Navigation.navigate(ROUTES.WORKSPACE_UPGRADE.getRoute(policyID, CONST.UPGRADE_FEATURE_INTRO_MAPPING.approvals.alias, Navigation.getActiveRoute()));
             return;
         }
@@ -151,7 +151,7 @@ function ApprovalWorkflowEditor({approvalWorkflow, removeApprovalWorkflow, polic
                 })}
 
                 <MenuItemWithTopDescription
-                    description={translate('workflowsCreateApprovalsPage.additionalApprover')}
+                    description={approvalWorkflow.approvers.length === 0 ? translate('workflowsPage.approver') : translate('workflowsCreateApprovalsPage.additionalApprover')}
                     onPress={addAdditionalApprover}
                     shouldShowRightIcon
                     wrapperStyle={styles.sectionMenuItemTopDescription}

cc @jjcoffee @DylanDylann @bernhardoj

[blazejkustra]

We should disable the submit button if the user doesn't select an approver on the first approver selection page

@daledah I believe it is against Expensify's guidelines, we don't want to disable the submit button and instead allow user to make an action and display an error.

@jjcoffee Ah see your point, I gave my opinion previously to make it consistent with "Expense from" section (editing flow)

Maybe we should do the other way around and make Expenses From page consistent with Approver and Confirm screen? Meaning: allow the user to create an invalid state in the form and display an error when clicking on the submit button.

[jjcoffee]

we just need to change the copy on the additionalApprover input, and make sure admins can edit the default approver without the need to update the workspace to Control plan.

@blazejkustra That doesn't sound quite related to this issue, at least I don't think the (not a) proposal would do anything to fix the issue of the Approver field disappearing. Unless I'm missing something obvious, of course!

Regarding the Expenses From flow, I think we can make it consistent with the Approver screen. It makes sense to block tapping the Next button whilst we're in the creation flow as it would be annoying to wait until the end to display an error, but it seems awkward UX to do it when editing a field (even if in this case it's a bit of a case of dumb user behaviour 😄).

[blazejkustra]

@blazejkustra That doesn't sound quite related to this issue, at least I don't think the (not a) proposal would do anything to fix the issue of the Approver field disappearing. Unless I'm missing something obvious, of course!

@jjcoffee Check out the logic in ApprovalWorkflowEditor component, it displays a bunch of menu items that link to Approver and Expenses From screens:

• first: for updating members
• n-menu items: for updating existing approvers
• last: for adding a new approver

The idea behind this setup is to always have one empty menu item displayed (add additional approver), no matter how many approvers there are currently. If all approvers are removed we want to display only one menu item, this way it's less confusing for the user (they don't have to guess if they need to click on add additional approver or approver menu item).

Here is the video of how it behaves with my fix:

Screen.Recording.2024-08-26.at.15.05.18.mov

[jjcoffee]

@blazejkustra Ahh gotcha, now it makes sense! I thought that having the Additional approver field always there was the intended behaviour, so that's why I was confused by your proposal 😄

I'm a bit on the fence on hiding it if you deselect the default approver, since the Additional approver field is always there otherwise so represents the "default" UI state in my mind, and it feels a bit jarring for that field to disappear when I've only interacted with the default approver field. Having said that I'm not sure how much it matters either way 😄 @bernhardoj or @mountiny Do you have any strong feelings?

[bernhardoj]

I think what @blazejkustra suggested makes sense to me. The additional approver is shown to allow the user to add another approver, but if we unselect the current approver, then the additional approver field seems unneeded.

[mountiny]

Ah alright so thanks for chiming in @blazejkustra I agree the solution that Blazej implemented is the desired behaviour here

[jjcoffee]

Sounds like a consensus to me! @bernhardoj Can you go ahead and implement @blazejkustra's suggestion in your PR?

[bernhardoj]

@jjcoffee Great, updated the PR. Please check!

[abekkala]

merged to staging yesterday

[jjcoffee]

Deployed to production Aug 30th, so should be due for payment 2024-09-06.

[abekkala]

PAYMENT SUMMARY FOR SEPT 06

• Fix: @bernhardoj [$250] payment via NewDot
• PR Review: @jjcoffee [$250] OFFER

[jjcoffee]

@abekkala Accepted, thanks! 🙏

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj

[abekkala]

@jjcoffee payment sent and contract ended - thank you! 🎉