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.

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 correct next approver with category/tag rules #52537

Merged
merged 29 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
81cf189
Update correct next approver with category/tag rules
nkdengineer Nov 14, 2024
af33694
merge main
nkdengineer Nov 15, 2024
ea5f4d8
Merge branch 'main' into fix/52458
nkdengineer Nov 20, 2024
64f7fbd
Merge branch 'main' into fix/52458
nkdengineer Nov 25, 2024
9fa35ab
sort all transactons correctly
nkdengineer Nov 25, 2024
1b26e17
Merge branch 'main' into fix/52458
nkdengineer Nov 26, 2024
7cee3cc
Merge branch 'main' into fix/52458
nkdengineer Nov 27, 2024
9a0f257
merge main
nkdengineer Dec 3, 2024
fe82c8d
merge main
nkdengineer Dec 5, 2024
a04d1c8
fix transaction order
nkdengineer Dec 5, 2024
c79a161
remove log
nkdengineer Dec 5, 2024
77eccd9
fix lint
nkdengineer Dec 5, 2024
6c0b0e6
Merge branch 'main' into fix/52458
nkdengineer Dec 12, 2024
ee64053
Merge branch 'main' into fix/52458
nkdengineer Dec 13, 2024
152fa1b
update inserted in buildOptimisticTransaction
nkdengineer Dec 13, 2024
08fbff2
Merge branch 'main' into fix/52458
nkdengineer Dec 16, 2024
54601b2
add submitsTo of owner to approver chain
nkdengineer Dec 16, 2024
5514599
revert hardcode
nkdengineer Dec 16, 2024
b7ec6e0
Merge branch 'main' into fix/52458
nkdengineer Dec 18, 2024
28b59d7
refactor function
nkdengineer Dec 18, 2024
0e45944
Update src/libs/PolicyUtils.ts
nkdengineer Dec 18, 2024
550a6ae
Update src/libs/TransactionUtils/index.ts
nkdengineer Dec 18, 2024
aa2d12f
Update src/libs/PolicyUtils.ts
nkdengineer Dec 18, 2024
272b7e3
Update src/libs/PolicyUtils.ts
nkdengineer Dec 18, 2024
6585ba8
Update src/libs/PolicyUtils.ts
nkdengineer Dec 18, 2024
91a6363
remove getSubmitToEmail
nkdengineer Dec 18, 2024
a7c0c80
return early if the policy is submit and close
nkdengineer Dec 18, 2024
a47aab0
Merge branch 'main' into fix/52458
nkdengineer Dec 19, 2024
cfaabb3
merge main
nkdengineer Dec 19, 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
2 changes: 2 additions & 0 deletions src/libs/DebugUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,7 @@
return validateString(value);
case 'created':
case 'modifiedCreated':
case 'inserted':
case 'posted':
return validateDate(value);
case 'isLoading':
Expand Down Expand Up @@ -1046,6 +1047,7 @@
cardNumber: CONST.RED_BRICK_ROAD_PENDING_ACTION,
managedCard: CONST.RED_BRICK_ROAD_PENDING_ACTION,
posted: CONST.RED_BRICK_ROAD_PENDING_ACTION,
inserted: CONST.RED_BRICK_ROAD_PENDING_ACTION,
},
'string',
);
Expand Down Expand Up @@ -1367,7 +1369,7 @@
}

function getTransactionID(report: OnyxEntry<Report>, reportActions: OnyxEntry<ReportActions>) {
const transactionID = TransactionUtils.getTransactionID(report?.reportID ?? '-1');

Check failure on line 1372 in src/libs/DebugUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check


return Number(transactionID) > 0
? transactionID
Expand Down
61 changes: 36 additions & 25 deletions src/libs/PolicyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ 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';
import {getAllSortedTransactions, getCategory, getTag} from './TransactionUtils';

type MemberEmailsToAccountIDs = Record<string, number>;

Expand Down Expand Up @@ -542,37 +542,35 @@ function getDefaultApprover(policy: OnyxEntry<Policy> | SearchPolicy): string {
return policy?.approver ?? policy?.owner ?? '';
}

/**
* Returns the accountID to whom the given expenseReport submits reports to in the given Policy.
*/
function getSubmitToAccountID(policy: OnyxEntry<Policy> | SearchPolicy, expenseReport: OnyxEntry<Report>): number {
const employeeAccountID = expenseReport?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID;
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));
function getRuleApprovers(policy: OnyxEntry<Policy> | SearchPolicy, expenseReport: OnyxEntry<Report>) {
const categoryAppovers: string[] = [];
const tagApprovers: string[] = [];
const allReportTransactions = getAllSortedTransactions(expenseReport?.reportID);

// 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);
for (let i = 0; i < allReportTransactions.length; i++) {
const transaction = allReportTransactions.at(i);
const tag = getTag(transaction);
const category = getCategory(transaction);
categoryAppover = getCategoryApproverRule(policy?.rules?.approvalRules ?? [], category)?.approver;
const categoryAppover = getCategoryApproverRule(policy?.rules?.approvalRules ?? [], category)?.approver;
const tagApprover = getTagApproverRule(policy, tag)?.approver;
if (categoryAppover) {
return getAccountIDsByLogins([categoryAppover]).at(0) ?? -1;
categoryAppovers.push(categoryAppover);
}

if (!tagApprover && getTagApproverRule(policy, tag)?.approver) {
tagApprover = getTagApproverRule(policy, tag)?.approver;
if (tagApprover) {
tagApprovers.push(tagApprover);
}
}

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

function getManagerAccountID(policy: OnyxEntry<Policy> | SearchPolicy, expenseReport: OnyxEntry<Report>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we're getting the NEXT manager account ID, right? Can we call this function something like getNextManagerAccountID & rename getManagerAccountEmail to something like getNextManagerAccountEmail?

The way I currently read these functions is that we should be getting the report's current manager account ID / email - which I don't believe is true

const employeeAccountID = expenseReport?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID;
const employeeLogin = getLoginsByAccountIDs([employeeAccountID]).at(0) ?? '';
const defaultApprover = getDefaultApprover(policy);

// 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))) {
Expand All @@ -587,9 +585,21 @@ function getSubmitToAccountID(policy: OnyxEntry<Policy> | SearchPolicy, expenseR
return getAccountIDsByLogins([employee.submitsTo ?? defaultApprover]).at(0) ?? -1;
}

function getSubmitToEmail(policy: OnyxEntry<Policy>, expenseReport: OnyxEntry<Report>): string {
const submitToAccountID = getSubmitToAccountID(policy, expenseReport);
return getLoginsByAccountIDs([submitToAccountID]).at(0) ?? '';
/**
* Returns the accountID to whom the given expenseReport submits reports to in the given Policy.
*/
function getSubmitToAccountID(policy: OnyxEntry<Policy> | SearchPolicy, expenseReport: OnyxEntry<Report>): number {
const ruleApprovers = getRuleApprovers(policy, expenseReport);
if (ruleApprovers.length > 0 && !isSubmitAndClose(policy)) {
return getAccountIDsByLogins([ruleApprovers.at(0) ?? '']).at(0) ?? -1;
}

return getManagerAccountID(policy, expenseReport);
}

function getManagerAccountEmail(policy: OnyxEntry<Policy>, expenseReport: OnyxEntry<Report>): string {
const managerAccountID = getManagerAccountID(policy, expenseReport);
return getLoginsByAccountIDs([managerAccountID]).at(0) ?? '';
}

/**
Expand Down Expand Up @@ -1263,7 +1273,6 @@ export {
getCurrentTaxID,
areSettingsInErrorFields,
settingsPendingAction,
getSubmitToEmail,
getForwardsToAccount,
getSubmitToAccountID,
getWorkspaceAccountID,
Expand All @@ -1279,6 +1288,8 @@ export {
getUserFriendlyWorkspaceType,
isPolicyAccessible,
areAllGroupPoliciesExpenseChatDisabled,
getManagerAccountEmail,
getRuleApprovers,
};

export type {MemberEmailsToAccountIDs};
29 changes: 25 additions & 4 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8472,20 +8472,41 @@ function isExported(reportActions: OnyxEntry<ReportActions>) {

function getApprovalChain(policy: OnyxEntry<Policy>, expenseReport: OnyxEntry<Report>): string[] {
const approvalChain: string[] = [];
const fullApprovalChain: string[] = [];
const reportTotal = expenseReport?.total ?? 0;
const submitterEmail = PersonalDetailsUtils.getLoginsByAccountIDs([expenseReport?.ownerAccountID ?? -1]).at(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)) {
if (PolicyUtils.isSubmitAndClose(policy)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ntdiary Clarify a bit, we need to return early if the approval mode is OPTIONAL then when the user submits the report, backend will not return error about the managerAccountID is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can test with the case the step-up is the same with the original step except the approval mode is OPTIONAL. Then no.24 creates an expense with tag/category match with rule and submit the report.

Copy link
Contributor

@ntdiary ntdiary Dec 18, 2024

Choose a reason for hiding this comment

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

@ntdiary Clarify a bit, we need to return early if the approval mode is OPTIONAL then when the user submits the report, backend will not return error about the managerAccountID is incorrect.

Interesting, @nkdengineer, so you tried this case:

  1. enable Advanced Approval mode
  2. set category&tag rule approvers
  3. then switch to Submit and Close mode directly.

right?

image

Based on comments in PR #51196, I thought category/tag rule approvers were also supported in S&C mode. 😂
If the backend doesn't support it, I think your new early return is fine!

The frontend should be ready :shipit:. As for the unit tests, we’ve already confirmed before, If needed, can add them in a follow-up PR.
cc @Beamanator

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh ok I will test that out in OldDot now to see what happens!

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I just tested in OldDot and found that Category Approvers are NOT supported on Control, Submit & Close policies 🙏 - I honestly had never tested that, but glad it's tested now & it sounds like that's working in this PR 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, @nkdengineer, so you tried this case:

  1. enable Advanced Approval mode
  2. set category&tag rule approvers
  3. then switch to Submit and Close mode directly.

FYI I don't think it's important to run the test in this order - a.k.a. it's not important to enable advanced approval first, then switch to submit & close... Right?!?

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI I don't think it's important to run the test in this order - a.k.a. it's not important to enable advanced approval first, then switch to submit & close... Right?!?

@Beamanator, yeah, you are right! In OD web page, even in S&C mode, it is still possible to set category/tag approvers, although they won't take effect. :D

return approvalChain;
}

let nextApproverEmail = PolicyUtils.getSubmitToEmail(policy, expenseReport);
// Get category/tag approver list
const ruleApprovers = PolicyUtils.getRuleApprovers(policy, expenseReport);

// Push rule approvers to approvalChain list before submitsTo/forwardsTo approvers
ruleApprovers.forEach((ruleApprover) => {
// Don't push submiiter to approve as a rule approver
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Don't push submiiter to approve as a rule approver
// Don't push submitter to approve as a rule approver

if (fullApprovalChain.includes(ruleApprover) || ruleApprover === submitterEmail) {
return;
}
fullApprovalChain.push(ruleApprover);
});

let nextApproverEmail = PolicyUtils.getManagerAccountEmail(policy, expenseReport);

while (nextApproverEmail && !approvalChain.includes(nextApproverEmail)) {
approvalChain.push(nextApproverEmail);
nextApproverEmail = PolicyUtils.getForwardsToAccount(policy, nextApproverEmail, reportTotal);
}
return approvalChain;

approvalChain.forEach((approver) => {
if (fullApprovalChain.includes(approver)) {
return;
}

fullApprovalChain.push(approver);
});
return fullApprovalChain;
}

/**
Expand Down
19 changes: 19 additions & 0 deletions src/libs/TransactionUtils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
key: ONYXKEYS.SESSION,
callback: (val) => {
currentUserEmail = val?.email ?? '';
currentUserAccountID = val?.accountID ?? -1;

Check failure on line 80 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

},
});

Expand Down Expand Up @@ -189,6 +189,7 @@
billable,
reimbursable,
attendees,
inserted: DateUtils.getDBTime(),
};
}

Expand Down Expand Up @@ -578,7 +579,7 @@
* Return the cardID from the transaction.
*/
function getCardID(transaction: Transaction): number {
return transaction?.cardID ?? -1;

Check failure on line 582 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

}

/**
Expand Down Expand Up @@ -963,7 +964,7 @@
if (isDistanceRequest(transaction)) {
const customUnitRateID = getRateID(transaction) ?? '';
const customUnitRate = getDistanceRateCustomUnitRate(policy, customUnitRateID);
return customUnitRate?.attributes?.taxRateExternalID ?? '';

Check failure on line 967 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

}
const defaultExternalID = policy?.taxRates?.defaultExternalID;
const foreignTaxDefault = policy?.taxRates?.foreignTaxDefault;
Expand Down Expand Up @@ -1158,7 +1159,7 @@
}
} else if (fieldName === 'category') {
const differentValues = getDifferentValues(transactions, keys);
const policyCategories = getPolicyCategoriesData(report?.policyID ?? '-1');

Check failure on line 1162 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

const availableCategories = Object.values(policyCategories)
.filter((category) => differentValues.includes(category.name) && category.enabled && category.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
.map((e) => e.name);
Expand All @@ -1169,7 +1170,7 @@
keep[fieldName] = firstTransaction?.[keys[0]] ?? firstTransaction?.[keys[1]];
}
} else if (fieldName === 'tag') {
const policyTags = getPolicyTagsData(report?.policyID ?? '-1');

Check failure on line 1173 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

const isMultiLevelTags = PolicyUtils.isMultiLevelTags(policyTags);
if (isMultiLevelTags) {
if (areAllFieldsEqualForKey || !policy?.areTagsEnabled) {
Expand Down Expand Up @@ -1202,8 +1203,8 @@

function getTransactionID(threadReportID: string): string {
const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${threadReportID}`];
const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '', report?.parentReportActionID ?? '');

Check failure on line 1206 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Check failure on line 1206 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

const IOUTransactionID = ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction)?.IOUTransactionID ?? '-1' : '-1';

Check failure on line 1207 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Check failure on line 1207 in src/libs/TransactionUtils/index.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check


return IOUTransactionID;
}
Expand Down Expand Up @@ -1239,6 +1240,23 @@
};
}

/**
* Return the sorted list transactions of an iou report
*/
function getAllSortedTransactions(iouReportID?: string): Array<OnyxEntry<Transaction>> {
return getAllReportTransactions(iouReportID).sort((transA, transB) => {
if (transA.created < transB.created) {
return -1;
}

if (transA.created > transB.created) {
return 1;
}

return (transA.inserted ?? '') < (transB.inserted ?? '') ? -1 : 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this field should be required? If it's optional, creating multiple transactions offline could lead to a sorting problem due to this field being missing. If it's required, the frontend might need to include inserted: DateUtils.getDBTime() in the buildOptimisticTransaction?
cc @nkdengineer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make senses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can make it optional and update it in buildOptimisticTransaction because the draft transaction don't need to add this field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ntdiary I updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, in @ntdiary 's case at least, it's not a backend bug, it is expected for the submitsTo to approve (in advanced approval case) AFTER all of the rule approvers

@Beamanator, yeah, as I mentioned earlier, the backend works well, it's just that the frontend didn't correctly add the policy owner to the approval chain, I will continue testing soon. If you also feel this isn’t a blocker, we can ignore it here, just hope it won't be treated as a regression or bug in future QA tests. :)

Ya thanks for getting us full circle 😂 I honestly do think this is something we need to fix in this issue - agreed we don’t want this to be counted as a regression later

@nkdengineer, after some discussion, we think this issue still needs to be addressed in this PR, can you please take another look at how to address it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nkdengineer, after some discussion, we think this issue still needs to be addressed in this PR, can you please take another look at how to address it?

@ntdiary The frontend logic is slightly different after looking at the backend logic here. I updated the logic to add the submitsTo of owner to the approver chain.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nkdengineer, thank you! I’ll provide feedback within the next 24 hours. It's too late now, so need to rest first (23:30). :)

});
}

export {
buildOptimisticTransaction,
calculateTaxAmount,
Expand Down Expand Up @@ -1322,6 +1340,7 @@
getCardName,
hasReceiptSource,
shouldShowAttendees,
getAllSortedTransactions,
getFormattedPostedDate,
};

Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/Transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,9 @@ type Transaction = OnyxCommon.OnyxValueWithOfflineFeedback<

/** The card transaction's posted date */
posted?: string;

/** The inserted time of the transaction */
inserted?: string;
},
keyof Comment | keyof TransactionCustomUnit | 'attendees'
>;
Expand Down
Loading