-
Notifications
You must be signed in to change notification settings - Fork 3k
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
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 |
---|---|---|
|
@@ -921,6 +921,7 @@ | |
return validateString(value); | ||
case 'created': | ||
case 'modifiedCreated': | ||
case 'inserted': | ||
case 'posted': | ||
return validateDate(value); | ||
case 'isLoading': | ||
|
@@ -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', | ||
); | ||
|
@@ -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
|
||
|
||
return Number(transactionID) > 0 | ||
? transactionID | ||
|
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)) { | ||||||
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 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 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 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! 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 🙏 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?!? 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 | ||||||
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 |
---|---|---|
|
@@ -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
|
||
}, | ||
}); | ||
|
||
|
@@ -189,6 +189,7 @@ | |
billable, | ||
reimbursable, | ||
attendees, | ||
inserted: DateUtils.getDBTime(), | ||
}; | ||
} | ||
|
||
|
@@ -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
|
||
} | ||
|
||
/** | ||
|
@@ -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
|
||
} | ||
const defaultExternalID = policy?.taxRates?.defaultExternalID; | ||
const foreignTaxDefault = policy?.taxRates?.foreignTaxDefault; | ||
|
@@ -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
|
||
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); | ||
|
@@ -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
|
||
const isMultiLevelTags = PolicyUtils.isMultiLevelTags(policyTags); | ||
if (isMultiLevelTags) { | ||
if (areAllFieldsEqualForKey || !policy?.areTagsEnabled) { | ||
|
@@ -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
|
||
const IOUTransactionID = ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction)?.IOUTransactionID ?? '-1' : '-1'; | ||
Check failure on line 1207 in src/libs/TransactionUtils/index.ts
|
||
|
||
return IOUTransactionID; | ||
} | ||
|
@@ -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; | ||
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 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 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 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. 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? 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 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 @@ | |
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
& renamegetManagerAccountEmail
to 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