-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Update correct next approver with category/tag rules #52537
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
Changes from all commits
81cf189
af33694
ea5f4d8
64f7fbd
9fa35ab
1b26e17
7cee3cc
9a0f257
fe82c8d
a04d1c8
c79a161
77eccd9
6c0b0e6
ee64053
152fa1b
08fbff2
54601b2
5514599
b7ec6e0
28b59d7
0e45944
550a6ae
aa2d12f
272b7e3
6585ba8
91a6363
a7c0c80
a47aab0
cfaabb3
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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)) { | ||||||
|
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. @ntdiary Clarify a bit, we need to return early if the approval mode is
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. You can test with the case the step-up is the same with the original step except the approval mode is
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.
Interesting, @nkdengineer, so you tried this case:
right?
Based on comments in PR #51196, I thought category/tag rule approvers were also supported in The frontend should be ready
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. Ooh ok I will test that out in OldDot now to see what happens!
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. 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 🙏
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.
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?!?
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.
@Beamanator, yeah, you are right! In OD web page, even in |
||||||
| 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 | ||||||
|
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.
Suggested change
|
||||||
| 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; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,6 +189,7 @@ function buildOptimisticTransaction( | |
| billable, | ||
| reimbursable, | ||
| attendees, | ||
| inserted: DateUtils.getDBTime(), | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -1239,6 +1240,23 @@ function buildTransactionsMergeParams(reviewDuplicates: OnyxEntry<ReviewDuplicat | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * 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; | ||
|
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. 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
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. Make senses
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. I think we can make it optional and update it in
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. @ntdiary I updated.
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.
@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?
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.
@ntdiary The frontend logic is slightly different after looking at the backend logic here. I updated the logic to add the
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. @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, | ||
|
|
@@ -1322,6 +1340,7 @@ export { | |
| getCardName, | ||
| hasReceiptSource, | ||
| shouldShowAttendees, | ||
| getAllSortedTransactions, | ||
| getFormattedPostedDate, | ||
| }; | ||
|
|
||
|
|
||

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.
Here we're getting the NEXT manager account ID, right? Can we call this function something like
getNextManagerAccountID& renamegetManagerAccountEmailto something likegetNextManagerAccountEmail?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