-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[No QA][HR Import] Generalize Workflows page for any HR connection #91324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
dd48583
8d8ffe9
b2db357
98dc455
01990a9
9ce37d8
198deb4
19047fc
1ae7267
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,18 +30,37 @@ type ApprovalWorkflowSectionProps = { | |
|
|
||
| /** Whether the workflow should be shown as read-only */ | ||
| isDisabled?: boolean; | ||
|
|
||
| /** HR provider display name, used in manager mode to show "Manager (from {provider})" */ | ||
| hrProviderName?: string; | ||
|
|
||
| /** When true, uses HR manager mode labels: "Manager (from {provider})" then "Final approver" */ | ||
| isHRManagerMode?: boolean; | ||
| }; | ||
|
|
||
| function ApprovalWorkflowSection({approvalWorkflow, onPress, currency = CONST.CURRENCY.USD, isDisabled = false}: ApprovalWorkflowSectionProps) { | ||
| function ApprovalWorkflowSection({approvalWorkflow, onPress, currency = CONST.CURRENCY.USD, isDisabled = false, hrProviderName, isHRManagerMode = false}: ApprovalWorkflowSectionProps) { | ||
| const icons = useMemoizedLazyExpensifyIcons(['ArrowRight', 'Lightbulb', 'Users', 'UserCheck']); | ||
| const styles = useThemeStyles(); | ||
| const theme = useTheme(); | ||
| const {translate, toLocaleOrdinal, localeCompare} = useLocalize(); | ||
| const {convertToDisplayString} = useCurrencyListActions(); | ||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
|
|
||
| const approverTitle = (index: number) => | ||
| approvalWorkflow.approvers.length > 1 ? `${toLocaleOrdinal(index + 1, true)} ${translate('workflowsPage.approver').toLowerCase()}` : `${translate('workflowsPage.approver')}`; | ||
| const fromProviderSuffix = hrProviderName ? ` (${translate('workflowsPage.approverFromProvider', {provider: hrProviderName})})` : ''; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should define this right before it's being used here |
||
|
|
||
| const approverTitle = (index: number) => { | ||
| if (isHRManagerMode) { | ||
| if (approvalWorkflow.approvers.length <= 1) { | ||
| return translate('workflowsPage.approver'); | ||
| } | ||
| const isLastApprover = index === approvalWorkflow.approvers.length - 1; | ||
| if (isLastApprover && approvalWorkflow.approvers.length > 1) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return translate('workflowsPage.finalApprover'); | ||
| } | ||
|
Comment on lines
+56
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In HR manager mode this unconditionally labels the last row as Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| return `${translate('workflowsPage.manager')}${fromProviderSuffix}`; | ||
| } | ||
| return approvalWorkflow.approvers.length > 1 ? `${toLocaleOrdinal(index + 1, true)} ${translate('workflowsPage.approver').toLowerCase()}` : translate('workflowsPage.approver'); | ||
| }; | ||
|
|
||
| const sortedMembers = approvalWorkflow.isDefault ? [] : sortAlphabetically(approvalWorkflow.members, 'displayName', localeCompare); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1300,6 +1300,26 @@ function getApprovalWorkflow(policy: OnyxEntry<Policy>): ValueOf<typeof CONST.PO | |
| return policy?.approvalMode ?? CONST.POLICY.APPROVAL_MODE.ADVANCED; | ||
| } | ||
|
|
||
| /** Returns the Merge HR finalApprover when the integration is in "basic" approval mode, or null otherwise. */ | ||
| function getMergeHRBasicModeFinalApprover(policy: OnyxEntry<Policy>): string | null { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB: this file has become so large that syntax highlighting has stopped working. Put Merge.dev / HR sync stuff in a new dedicated file. |
||
| const mergeConfig = policy?.connections?.merge_hris?.config; | ||
| if (mergeConfig?.approvalMode === CONST.MERGE_HR.APPROVAL_MODE.BASIC && mergeConfig.finalApprover) { | ||
| return mergeConfig.finalApprover; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** Returns the Merge HR finalApprover when the integration is in basic or manager mode, or null otherwise. */ | ||
| function getMergeHRFinalApprover(policy: OnyxEntry<Policy>): string | null { | ||
| const mergeConfig = policy?.connections?.merge_hris?.config; | ||
| if ((mergeConfig?.approvalMode === CONST.MERGE_HR.APPROVAL_MODE.BASIC || mergeConfig?.approvalMode === CONST.MERGE_HR.APPROVAL_MODE.MANAGER) && mergeConfig?.finalApprover) { | ||
| return mergeConfig.finalApprover; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function getDefaultApprover(policy: OnyxEntry<Policy>): string { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| return policy?.approver || policy?.owner || ''; | ||
|
|
@@ -2485,6 +2505,8 @@ export { | |
| getCurrentConnectionName, | ||
| getCustomersOrJobsLabelNetSuite, | ||
| getDefaultApprover, | ||
| getMergeHRBasicModeFinalApprover, | ||
| getMergeHRFinalApprover, | ||
| getApprovalWorkflow, | ||
| getReimburserAccountID, | ||
| isControlPolicy, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ import type Policy from '@src/types/onyx/Policy'; | |
| import type PolicyEmployee from '@src/types/onyx/PolicyEmployee'; | ||
| import type {PolicyEmployeeList} from '@src/types/onyx/PolicyEmployee'; | ||
| import {isBankAccountPartiallySetup} from './BankAccountUtils'; | ||
| import {getDefaultApprover, isExpensifyTeam, shouldFilterExpensifyTeam} from './PolicyUtils'; | ||
| import {getDefaultApprover, getMergeHRFinalApprover, isExpensifyTeam, shouldFilterExpensifyTeam} from './PolicyUtils'; | ||
|
|
||
| const INITIAL_APPROVAL_WORKFLOW: ApprovalWorkflowOnyx = { | ||
| members: [], | ||
|
|
@@ -150,7 +150,7 @@ function findFirstNonExpensifyApprover(employees: PolicyEmployeeList, startEmail | |
| /** Convert a list of policy employees to a list of approval workflows */ | ||
| function convertPolicyEmployeesToApprovalWorkflows({policy, personalDetails, firstApprover, localeCompare, currentUserLogin}: PolicyConversionParams): PolicyConversionResult { | ||
| const employees = policy?.employeeList ?? {}; | ||
| const defaultApprover = getDefaultApprover(policy); | ||
| const defaultApprover = getMergeHRFinalApprover(policy) ?? getDefaultApprover(policy); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Using Useful? React with 👍 / 👎. |
||
| const approvalWorkflows: Record<string, ApprovalWorkflow> = {}; | ||
| const shouldFilterOutExpensifyTeam = shouldFilterExpensifyTeam(policy?.owner, currentUserLogin); | ||
|
|
||
|
|
@@ -161,6 +161,7 @@ function convertPolicyEmployeesToApprovalWorkflows({policy, personalDetails, fir | |
| personalDetailsByEmail[value?.login ?? key] = value; | ||
| } | ||
| const availableMembers: Member[] = []; | ||
| const isMergeHRManagerMode = policy?.connections?.merge_hris?.config?.approvalMode === CONST.MERGE_HR.APPROVAL_MODE.MANAGER; | ||
|
|
||
| for (const employee of Object.values(employees)) { | ||
| const {email, submitsTo, pendingAction} = employee; | ||
|
|
@@ -179,19 +180,30 @@ function convertPolicyEmployeesToApprovalWorkflows({policy, personalDetails, fir | |
| availableMembers.push(member); | ||
| } | ||
|
|
||
| if (!submitsTo || !employees[submitsTo]) { | ||
| if (!submitsTo) { | ||
| continue; | ||
| } | ||
|
|
||
| // If submitsTo is an Expensify team member, find the first non-Expensify approver in the chain | ||
| const effectiveSubmitsTo = shouldFilterOutExpensifyTeam ? (findFirstNonExpensifyApprover(employees, submitsTo) ?? submitsTo) : submitsTo; | ||
|
|
||
| if (!employees[effectiveSubmitsTo]) { | ||
| if (!employees[submitsTo] && !isMergeHRManagerMode) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB: we have two back-to-back if statements that just do |
||
| continue; | ||
| } | ||
|
|
||
| // If submitsTo is an Expensify team member, find the first non-Expensify approver in the chain | ||
| const effectiveSubmitsTo = shouldFilterOutExpensifyTeam && employees[submitsTo] ? (findFirstNonExpensifyApprover(employees, submitsTo) ?? submitsTo) : submitsTo; | ||
|
|
||
| if (!approvalWorkflows[effectiveSubmitsTo]) { | ||
| let approvers = calculateApprovers({employees, firstEmail: effectiveSubmitsTo, personalDetailsByEmail}); | ||
| if (approvers.length === 0) { | ||
| approvers = [ | ||
| { | ||
| email: effectiveSubmitsTo, | ||
| forwardsTo: undefined, | ||
| avatar: personalDetailsByEmail[effectiveSubmitsTo]?.avatar, | ||
| displayName: personalDetailsByEmail[effectiveSubmitsTo]?.displayName ?? effectiveSubmitsTo, | ||
| isCircularReference: false, | ||
| }, | ||
| ]; | ||
| } | ||
| if (shouldFilterOutExpensifyTeam) { | ||
| approvers = approvers.filter((approver) => !isExpensifyTeam(approver.email)); | ||
| } | ||
|
|
@@ -225,6 +237,25 @@ function convertPolicyEmployeesToApprovalWorkflows({policy, personalDetails, fir | |
| } | ||
| } | ||
|
|
||
| // In Merge HR manager mode, append the finalApprover to each chain if not already present | ||
| const mergeConfig = policy?.connections?.merge_hris?.config; | ||
| if (isMergeHRManagerMode && mergeConfig?.finalApprover) { | ||
| const finalApproverEmail = mergeConfig.finalApprover; | ||
| for (const workflow of Object.values(approvalWorkflows)) { | ||
| const lastApprover = workflow.approvers.at(-1); | ||
| if (lastApprover && lastApprover.email !== finalApproverEmail) { | ||
| workflow.approvers.push({ | ||
| email: finalApproverEmail, | ||
| forwardsTo: undefined, | ||
| avatar: personalDetailsByEmail[finalApproverEmail]?.avatar, | ||
| displayName: personalDetailsByEmail[finalApproverEmail]?.displayName ?? finalApproverEmail, | ||
| isCircularReference: false, | ||
| }); | ||
| usedApproverEmails.add(finalApproverEmail); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Sort the workflows by the first approver's name (default workflow has priority) | ||
| const sortedApprovalWorkflows = Object.values(approvalWorkflows).sort((a, b) => { | ||
| if (a.isDefault) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "manager mode" is kind of confusing because there are three canonical approval modes in our backend:
imo it would be better to use the same terms in the front-end and the back-end in the code, even though the product shows the modes as Basic, Manager, and Custom.