[$250] Hide current user in approver selector when 'prevent self approvals' is enabled

[marcaaron]

cc @JmillsExpensify coming from our DM

While we have the ability to set "Prevent self approval" on a workspace. There are some ways in which this feature works unexpectedly in NewDot. Specifically, the following scenarios seem possible and should not be:

Incorrect workflow setup restrictions

Problem

• User tries to create a new workflow with themselves as approver when prevent self approval is enabled.
• This should not be possible. The OldDot behavior is to show a "problem" that needs to be fixed like this:

In NewDot, we just let it happen:

Solution:

• Ideally, we would just remove this person from the list of approvers when giving the option to select an approver

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021866263734682899164
• Upwork Job ID: 1866263734682899164
• Last Price Increase: 2024-12-09
• Automatic offers:
• brunovjk | Reviewer | 105298681
• Krishna2323 | Contributor | 105298682

Issue Owner

Current Issue Owner: @brunovjk

[melvin-bot]

Triggered auto assignment to @anmurali (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[melvin-bot]

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

[marcaaron]

I think this part: Incorrect workflow setup restrictions can be external as it's straightforward enough.

How to handle the users who are already self-approvers is less clear to me and probably requires a non-trivial backend change and maybe better to discuss in #product first. I feel like this is an edge case related to Workflows feature (so cc'ing @tgolen in case he has thoughts on this one as well).

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-12-05 00:52:32 UTC.

Proposal

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

Figure out how Prevent Self Approvals should work in NewDot and implement it

What is the root cause of that problem?

• Improvement -- currently we do not filter out the members when policy?.preventSelfApproval is true and approvalWorkflow?.members include the approver (employee)
  

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

  Lines 79 to 141 in 0e28aa4

  	const sections: ApproverSection[] = useMemo(() => {
  	const approvers: SelectionListApprover[] = [];
  	if (employeeList) {
  	const availableApprovers = Object.values(employeeList)
  	.map((employee): SelectionListApprover | null => {
  	const isAdmin = employee?.role === CONST.REPORT.ROLE.ADMIN;
  	const email = employee.email;
  	if (!email) {
  	return null;
  	}
  	// Do not allow the same email to be added twice
  	const isEmailAlreadyInApprovers = approversFromWorkflow?.some((approver, index) => approver?.email === email && index !== approverIndex);
  	if (isEmailAlreadyInApprovers && selectedApproverEmail !== email) {
  	return null;
  	}
  	// Do not allow the default approver to be added as the first approver
  	if (!isDefault && approverIndex === 0 && defaultApprover === email) {
  	return null;
  	}
  	const policyMemberEmailsToAccountIDs = PolicyUtils.getMemberAccountIDsForWorkspace(employeeList);
  	const accountID = Number(policyMemberEmailsToAccountIDs[email] ?? '');
  	const {avatar, displayName = email} = personalDetails?.[accountID] ?? {};
  	return {
  	text: displayName,
  	alternateText: email,
  	keyForList: email,
  	isSelected: selectedApproverEmail === email,
  	login: email,
  	icons: [{source: avatar ?? FallbackAvatar, type: CONST.ICON_TYPE_AVATAR, name: displayName, id: accountID}],
  	rightElement: isAdmin ? <Badge text={translate('common.admin')} /> : undefined,
  	};
  	})
  	.filter((approver): approver is SelectionListApprover => !!approver);
  	approvers.push(...availableApprovers);
  	// eslint-disable-next-line react-compiler/react-compiler
  	setAllApprovers(approvers);
  	}
  	const filteredApprovers =
  	debouncedSearchTerm !== ''
  	? approvers.filter((option) => {
  	const searchValue = OptionsListUtils.getSearchValueForPhoneOrEmail(debouncedSearchTerm);
  	const isPartOfSearchTerm = !!option.text?.toLowerCase().includes(searchValue) || !!option.login?.toLowerCase().includes(searchValue);
  	return isPartOfSearchTerm;
  	})
  	: approvers;
  	const data = OptionsListUtils.sortAlphabetically(filteredApprovers, 'text');
  	return [
  	{
  	title: undefined,
  	data,
  	shouldShow: true,
  	},
  	];
  	}, [approversFromWorkflow, isDefault, approverIndex, debouncedSearchTerm, defaultApprover, personalDetails, employeeList, selectedApproverEmail, translate]);

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

• We can add another condition to filter out the logins which are included in the members list and policy?.preventSelfApproval is true.

Pseudocode

    const membersEmail = useMemo(() => approvalWorkflow?.members.map((mem) => mem.email), [approvalWorkflow?.members]);
    const sections: ApproverSection[] = useMemo(() => {
        const approvers: SelectionListApprover[] = [];

        if (employeeList) {
            const availableApprovers = Object.values(employeeList)
                .map((employee): SelectionListApprover | null => {
                    const isAdmin = employee?.role === CONST.REPORT.ROLE.ADMIN;
                    const email = employee.email;

                    if (!email) {
                        return null;
                    }

                    if (policy?.preventSelfApproval && membersEmail?.includes(email)) {
                        return null;
                    }
                    // Do not allow the same email to be added twice
                    const isEmailAlreadyInApprovers = approversFromWorkflow?.some((approver, index) => approver?.email === email && index !== approverIndex);
                    if (isEmailAlreadyInApprovers && selectedApproverEmail !== email) {
                        return null;
                    }

                    // Do not allow the default approver to be added as the first approver
                    if (!isDefault && approverIndex === 0 && defaultApprover === email) {
                        return null;
                    }

                    const policyMemberEmailsToAccountIDs = PolicyUtils.getMemberAccountIDsForWorkspace(employeeList);
                    const accountID = Number(policyMemberEmailsToAccountIDs[email] ?? '');
                    const {avatar, displayName = email} = personalDetails?.[accountID] ?? {};

                    return {
                        text: displayName,
                        alternateText: email,
                        keyForList: email,
                        isSelected: selectedApproverEmail === email,
                        login: email,
                        icons: [{source: avatar ?? FallbackAvatar, type: CONST.ICON_TYPE_AVATAR, name: displayName, id: accountID}],
                        rightElement: isAdmin ? <Badge text={translate('common.admin')} /> : undefined,
                    };
                })
                .filter((approver): approver is SelectionListApprover => !!approver);

            approvers.push(...availableApprovers);
            // eslint-disable-next-line react-compiler/react-compiler
            setAllApprovers(approvers);
        }

        const filteredApprovers =
            debouncedSearchTerm !== ''
                ? approvers.filter((option) => {
                      const searchValue = OptionsListUtils.getSearchValueForPhoneOrEmail(debouncedSearchTerm);
                      const isPartOfSearchTerm = !!option.text?.toLowerCase().includes(searchValue) || !!option.login?.toLowerCase().includes(searchValue);
                      return isPartOfSearchTerm;
                  })
                : approvers;

        const data = OptionsListUtils.sortAlphabetically(filteredApprovers, 'text');
        return [
            {
                title: undefined,
                data,
                shouldShow: true,
            },
        ];
    }, [
        approversFromWorkflow,
        isDefault,
        approverIndex,
        debouncedSearchTerm,
        defaultApprover,
        personalDetails,
        employeeList,
        selectedApproverEmail,
        translate,
        policy?.preventSelfApproval,
        membersEmail,
    ]);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?


• We can render the App and WorkspaceWorkflowsApprovalsApproverPage and set the onyx data with few fake policies and provide some employees list and dummy approvalWorkflow data.
• Then we can verify that if policy?.preventSelfApproval is true in a policy, it filters out the selected members (expense from) from approvers list. It can be verified by checking the length of the options or verifying each email which should be there according to the members list value.

What alternative solutions did you explore? (Optional)

[marcaaron]

@Krishna2323 looks good, but not ready for proposals just yet.

[Krishna2323]

Hid the proposal 😅. Will add it back when we're ready for proposals.

[JmillsExpensify]

Ideally, we would just remove this person from the list of approvers when giving the option to select an approver here

I would think for this case, we:

• Show a centered modal clarifying the workspace is configured with workflows that violate this configuration, and as a result, they will be reset to the workspace owner.
• Then we potentially highlight the specific workflows that have changed (and if there are several, then we make sure at least one is within the scroll view and is highlighted.

[marcaaron]

Ah, so, maybe it is a bit easier to have them set back to whatever the default approver here is?

Or are we wanting to update each custom workflow to the workspace owner with some kind of note?

Either way, I think we may need a backend change since all the affected reports would need to have their managerID set to a new approver (and potentially Onyx updates sent out for this change).

potentially highlight the specific workflows that have changed (and if there are several, then we make sure at least one is within the scroll view and is highlighted

The user updating the prevent self approval setting would be on the "Rules" page. I think it would be easier to update them without having any kind of highlighting, but I might be missing why that's important.

[JmillsExpensify]

Yeah great call! Let's set them back to the default approver. In that case, also no need for highlighting.

[JmillsExpensify]

@marcaaron do you think we're ready to open up to proposals following our discussion last week?

[Krishna2323]

Show a centered modal clarifying the workspace is configured with workflows that violate this configuration, and as a result, they will be reset to the workspace owner.

@JmillsExpensify, do we want to show the modal or we just want to exclude the members from the approvers list?

[marcaaron]

Yeah. Ok, I think for clarity I am going to split this into two separate issues and we will make this one External and only focus on the exclusion of the user from the person selector page.

@JmillsExpensify, do we want to show the modal or we just want to exclude the members from the approvers list?

Let's chat about this in the next issue.

[melvin-bot]

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

[melvin-bot]

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

[brunovjk]

only focus on the exclusion of the user from the person selector page

With that I think we can go with @Krishna2323's proposal

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @brunovjk 🎉 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]

📣 @Krishna2323 🎉 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 📖

[Krishna2323]

@brunovjk PR ready for review ^

[brunovjk]

I think the automation failed to update the status. The PR was deployed to production on December 18th.