Skip to content
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.

Sign up for GitHub

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

Update getSubmitToAccountID with category and tag approver #51196

Merged
merged 16 commits into from
Nov 11, 2024
Merged

Update getSubmitToAccountID with category and tag approver #51196

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2af0515
update getSubmitToAccountID with category and tag approver
nkdengineer Oct 21, 2024
c93c19d
revert change
nkdengineer Oct 21, 2024
7d36a2b
fix lint
nkdengineer Oct 21, 2024
3682b9f
revert pod file
nkdengineer Oct 21, 2024
7b22022
Merge branch 'main' into fix/51001
nkdengineer Oct 22, 2024
bf5d6f5
merge main
nkdengineer Oct 23, 2024
2de4fee
Merge branch 'main' into fix/51001
nkdengineer Oct 24, 2024
b75c056
Update src/libs/PolicyUtils.ts
nkdengineer Oct 24, 2024
de7e6a6
update comment
nkdengineer Oct 24, 2024
cbaee21
Merge branch 'main' into fix/51001
nkdengineer Oct 25, 2024
4dbb1a4
fix test
nkdengineer Oct 25, 2024
9a789f3
fix test
nkdengineer Oct 25, 2024
62c0db1
fix cycle dependen
nkdengineer Oct 25, 2024
4faba7f
Merge branch 'main' into fix/51001
nkdengineer Oct 29, 2024
0e6c35a
Merge branch 'main' into fix/51001
nkdengineer Oct 31, 2024
e2c5d24
Merge branch 'main' into fix/51001
nkdengineer Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions src/libs/PolicyUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import INPUT_IDS from '@src/types/form/NetSuiteCustomFieldForm';
import type {OnyxInputOrEntry, Policy, PolicyCategories, PolicyEmployeeList, PolicyTagLists, PolicyTags, TaxRate} from '@src/types/onyx';
import type {OnyxInputOrEntry, Policy, PolicyCategories, PolicyEmployeeList, PolicyTagLists, PolicyTags, Report, TaxRate} from '@src/types/onyx';
import type {CardFeedData} from '@src/types/onyx/CardFeeds';
import type {ErrorFields, PendingAction, PendingFields} from '@src/types/onyx/OnyxCommon';
import type {
Expand All @@ -31,10 +31,12 @@ import type {
import type PolicyEmployee from '@src/types/onyx/PolicyEmployee';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import {hasSynchronizationErrorMessage} from './actions/connections';
import {getCategoryApproverRule} from './CategoryUtils';
import * as Localize from './Localize';
import Navigation from './Navigation/Navigation';
import * as NetworkStore from './Network/NetworkStore';
import {getAccountIDsByLogins, getLoginsByAccountIDs, getPersonalDetailByEmail} from './PersonalDetailsUtils';
import {getAllReportTransactions, getCategory, getTag} from './TransactionUtils';

type MemberEmailsToAccountIDs = Record<string, number>;

Expand Down Expand Up @@ -518,12 +520,37 @@ function getDefaultApprover(policy: OnyxEntry<Policy>): string {
}

/**
* Returns the accountID to whom the given employeeAccountID submits reports to in the given Policy.
* Returns the accountID to whom the given expenseReport submits reports to in the given Policy.
*/
function getSubmitToAccountID(policy: OnyxEntry<Policy>, employeeAccountID: number): number {
function getSubmitToAccountID(policy: OnyxEntry<Policy>, expenseReport: OnyxEntry<Report>): number {
const employeeAccountID = expenseReport?.ownerAccountID ?? -1;
const employeeLogin = getLoginsByAccountIDs([employeeAccountID]).at(0) ?? '';
const defaultApprover = getDefaultApprover(policy);

let categoryAppover;
let tagApprover;
const allTransactions = getAllReportTransactions(expenseReport?.reportID).sort((transA, transB) => (transA.created < transB.created ? -1 : 1));

// Before submitting to their `submitsTo` (in a policy on Advanced Approvals), submit to category/tag approvers.
// Category approvers are prioritized, then tag approvers.
for (let i = 0; i < allTransactions.length; i++) {
const transaction = allTransactions.at(i);
const tag = getTag(transaction);
const category = getCategory(transaction);
categoryAppover = getCategoryApproverRule(policy?.rules?.approvalRules ?? [], category)?.approver;
if (categoryAppover) {
return getAccountIDsByLogins([categoryAppover]).at(0) ?? -1;
}

if (!tagApprover && getTagApproverRule(policy?.id ?? '-1', tag)?.approver) {
tagApprover = getTagApproverRule(policy?.id ?? '-1', tag)?.approver;
}
}

if (tagApprover) {
return getAccountIDsByLogins([tagApprover]).at(0) ?? -1;
}

// For policy using the optional or basic workflow, the manager is the policy default approver.
if (([CONST.POLICY.APPROVAL_MODE.OPTIONAL, CONST.POLICY.APPROVAL_MODE.BASIC] as Array<ValueOf<typeof CONST.POLICY.APPROVAL_MODE>>).includes(getApprovalWorkflow(policy))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah I was looking for this! @nkdengineer - shouldn't we move this up above the new logic since none of the loops
& transaction-fetching are needed for non-advanced-approval policies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Beamanator When I tested by adding the category/tag rules and didn't upgrade to advanced-approval, the category/tag rules is still applied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great! But still isn't it better to return early via this check BEFORE looping through transactions? Not fixing a bug, but more efficient to return early

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Beamanator I mean that the category/tag rule should still apply for non-advanced-approval policies. If we move this after this condition, the error appears when we submit the report of non-advanced-approval policies that has transaction match category/tag approver rule.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hmmmm i assumed category/tag approvals were only supported w/ Advanced approval settings, do you know if that's incorrect @garrettmknight ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I guessed wrong 😅 You're right @nkdengineer - category/tag rule approvers should still apply for non-advanced-approval Control policies

return getAccountIDsByLogins([defaultApprover]).at(0) ?? -1;
Expand All @@ -537,8 +564,8 @@ function getSubmitToAccountID(policy: OnyxEntry<Policy>, employeeAccountID: numb
return getAccountIDsByLogins([employee.submitsTo ?? defaultApprover]).at(0) ?? -1;
}

function getSubmitToEmail(policy: OnyxEntry<Policy>, employeeAccountID: number): string {
const submitToAccountID = getSubmitToAccountID(policy, employeeAccountID);
function getSubmitToEmail(policy: OnyxEntry<Policy>, expenseReport: OnyxEntry<Report>): string {
const submitToAccountID = getSubmitToAccountID(policy, expenseReport);
return getLoginsByAccountIDs([submitToAccountID]).at(0) ?? '';
}

Expand Down
11 changes: 6 additions & 5 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1295,7 +1295,7 @@
return false;
}

const submitsToAccountID = PolicyUtils.getSubmitToAccountID(getPolicy(report.policyID), report.ownerAccountID ?? -1);

Check failure on line 1298 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / typecheck 

Argument of type 'number' is not assignable to parameter of type 'OnyxEntry<{ avatarUrl?: string | undefined; avatarFileName?: string | undefined; chatType?: ValueOf<{ readonly POLICY_ANNOUNCE: "policyAnnounce"; readonly POLICY_ADMINS: "policyAdmins"; readonly TRIP_ROOM: "tripRoom"; ... 6 more ...; readonly SYSTEM: "system"; }> | undefined; ... 71 more ...; visibleChatMemberAccoun...'.

return isProcessingReport(report) && submitsToAccountID === report.managerID;
}
Expand Down Expand Up @@ -3096,7 +3096,7 @@
}

if (policy?.type === CONST.POLICY.TYPE.CORPORATE && moneyRequestReport && isSubmitted && isCurrentUserSubmitter(moneyRequestReport.reportID)) {
const isForwarded = PolicyUtils.getSubmitToAccountID(policy, moneyRequestReport.ownerAccountID ?? -1) !== moneyRequestReport.managerID;
const isForwarded = PolicyUtils.getSubmitToAccountID(policy, moneyRequestReport) !== moneyRequestReport.managerID;
return !isForwarded;
}

Expand Down Expand Up @@ -4612,7 +4612,7 @@
};

// Get the approver/manager for this report to properly display the optimistic data
const submitToAccountID = PolicyUtils.getSubmitToAccountID(policy, payeeAccountID);
const submitToAccountID = PolicyUtils.getSubmitToAccountID(policy, expenseReport);
if (submitToAccountID) {
expenseReport.managerID = submitToAccountID;
}
Expand Down Expand Up @@ -7895,7 +7895,7 @@

function isAllowedToSubmitDraftExpenseReport(report: OnyxEntry<Report>): boolean {
const policy = getPolicy(report?.policyID);
const submitToAccountID = PolicyUtils.getSubmitToAccountID(policy, report?.ownerAccountID ?? -1);
const submitToAccountID = PolicyUtils.getSubmitToAccountID(policy, report);

return isAllowedToApproveExpenseReport(report, submitToAccountID);
}
Expand Down Expand Up @@ -8276,15 +8276,16 @@
return Object.values(reportActions).some((action) => ReportActionsUtils.isExportIntegrationAction(action));
}

function getApprovalChain(policy: OnyxEntry<Policy>, employeeAccountID: number, reportTotal: number): string[] {
function getApprovalChain(policy: OnyxEntry<Policy>, expenseReport: OnyxEntry<Report>): string[] {
const approvalChain: string[] = [];
const reportTotal = expenseReport?.total ?? 0;

// If the policy is not on advanced approval mode, we should not use the approval chain even if it exists.
if (!PolicyUtils.isControlOnAdvancedApprovalMode(policy)) {
return approvalChain;
}

let nextApproverEmail = PolicyUtils.getSubmitToEmail(policy, employeeAccountID);
let nextApproverEmail = PolicyUtils.getSubmitToEmail(policy, expenseReport);

while (nextApproverEmail && !approvalChain.includes(nextApproverEmail)) {
approvalChain.push(nextApproverEmail);
Expand Down
9 changes: 4 additions & 5 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7127,10 +7127,9 @@ function isLastApprover(approvalChain: string[]): boolean {
}

function getNextApproverAccountID(report: OnyxEntry<OnyxTypes.Report>) {
const ownerAccountID = report?.ownerAccountID ?? -1;
const policy = PolicyUtils.getPolicy(report?.policyID);
const approvalChain = ReportUtils.getApprovalChain(policy, ownerAccountID, report?.total ?? 0);
const submitToAccountID = PolicyUtils.getSubmitToAccountID(policy, ownerAccountID);
const approvalChain = ReportUtils.getApprovalChain(policy, report);
const submitToAccountID = PolicyUtils.getSubmitToAccountID(policy, report);

if (approvalChain.length === 0) {
return submitToAccountID;
Expand Down Expand Up @@ -7158,7 +7157,7 @@ function approveMoneyRequest(expenseReport: OnyxEntry<OnyxTypes.Report>, full?:
}
const optimisticApprovedReportAction = ReportUtils.buildOptimisticApprovedReportAction(total, expenseReport?.currency ?? '', expenseReport?.reportID ?? '-1');

const approvalChain = ReportUtils.getApprovalChain(PolicyUtils.getPolicy(expenseReport?.policyID), expenseReport?.ownerAccountID ?? -1, expenseReport?.total ?? 0);
const approvalChain = ReportUtils.getApprovalChain(PolicyUtils.getPolicy(expenseReport?.policyID), expenseReport);

const predictedNextStatus = isLastApprover(approvalChain) ? CONST.REPORT.STATUS_NUM.APPROVED : CONST.REPORT.STATUS_NUM.SUBMITTED;
const predictedNextState = isLastApprover(approvalChain) ? CONST.REPORT.STATE_NUM.APPROVED : CONST.REPORT.STATE_NUM.SUBMITTED;
Expand Down Expand Up @@ -7519,7 +7518,7 @@ function submitReport(expenseReport: OnyxTypes.Report) {

const parameters: SubmitReportParams = {
reportID: expenseReport.reportID,
managerAccountID: PolicyUtils.getSubmitToAccountID(policy, expenseReport.ownerAccountID ?? -1) ?? expenseReport.managerID,
managerAccountID: PolicyUtils.getSubmitToAccountID(policy, expenseReport) ?? expenseReport.managerID,
reportActionID: optimisticSubmittedReportAction.reportActionID,
};

Expand Down
Loading