Update correct next approver with category/tag rules #52537
Conversation
|
I spent a lot of time today fixing the backend, PR is up - but I didn't get to test your front end changes, sorry - if you can't find a bug with the correct approver when there's multiple category approvers, that's great! I will try to reproduce the issue i found on Monday 🙏 |
|
Got it, no problem. |
src/libs/TransactionUtils/index.ts
Outdated
| * Return the sorted list transactions of an iou report | ||
| */ | ||
| function getAllSortedTransactions(iouReportID: string): Array<OnyxEntry<Transaction>> { | ||
| // We need sort all transactions by sorting the parent report actions because `created` of the transaction only has format `YYYY-MM-DD` which can cause the wrong sorting |
There was a problem hiding this comment.
Oooohhh this is interesting, and maaay be wrong - but let's see.
What we need to test is (in OldDot):
- create 2 new expenses in a report with different category approvers
- The 1st created expense should have an expense date AFTER the date of the 2nd expense
Then, when the submitter submits the report, does the report go to the category approver on the first expense or the category approver on the 2nd expense?
There was a problem hiding this comment.
// We need sort all transactions by sorting the parent report actions because
createdof the transaction only has formatYYYY-MM-DDwhich can cause the wrong sorting
@Beamanator Currently, we're going to the first expense. I think I need to update the comment a bit to
// We need to sort all transactions by sorting the parent report actions because `created` of the transaction doesn't mean the created time of the transaction.
There was a problem hiding this comment.
Ok well from my testing in OldDot, it honestly doesn't seem consistent, which order the category approvers show up when there's multiple 😅 So I'm discussing internally to figure out what we'll want to do in NewDot, as an expected order
There was a problem hiding this comment.
Ok Updated sort order is here: #52458 (comment)
There was a problem hiding this comment.
seems frontend also needs a tweak to achieve this sort order.
There was a problem hiding this comment.
Still investigate the sort order.
There was a problem hiding this comment.
@ntdiary Updated the sort order.
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari52537-mac-chrome-submit.mp452537-mac-chrome-approve-cat_1.mp452537-mac-chrome-approve-cat_2.mp452537-mac-chrome-approve-tag_3.mp452537-mac-chrome-approve-tag_4.mp4MacOS: Desktop
|
no.4.movMaybe this should be fixed as a frontend bug: When |
|
@ntdiary Is this reproducible on the latest main? If yes, I think we should fix it as a separate issue. |
The reason I'm asking here, is if we don't fix it, we can't achieve the expected behavior in the OP (step 12 offline):
Additionally, this problem is introduced by PR #51196, the execution chain is: BTW, when building the approval chain, I think const chain = new Set<String>()
approvers.forEach(el => chain.add(el))
chain.delete(submitterEmail)
a = new Set([1,3,2,4,3,2])
return [...a]Finally, if needed, maybe we could increase the bounty for this issue, since it really involves quite multiple cases that need to be considered. :) |
|
I definitely think it's fair to bump the bounty on this one, it's a pretty complicated issue that we're trying to get perfect 👍 @ntdiary are you waiting on me or @nkdengineer to look at your latest message? BTW I'm making sure a transaction |
@Beamanator, I’m waiting for the new sorting implementation, |
|
Cool, yeah @nkdengineer can now implement the correct sorting (with transaction
I think this makes sense too, to fix here 👍 i believe it should be a relatively quick fix 😅 |
|
Sure will give an update soon. |
|
BTW, it would be great if we can add some unit tests for this feature, since we've recently started prioritizing unit test. :) |
|
Ooh definitely agreed about tests 👍 though i wouldn't mind if that's in a follow-up just so we can get this out the door quicker 🤷 |
|
@Beamanator Let's wait a bit. I just found a case that we cannot submit the report when the approval flow is disabled. |
|
|
||
| // 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.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@ntdiary Clarify a bit, we need to return early if the approval mode is
OPTIONALthen when the user submits the report, backend will not return error about themanagerAccountIDis incorrect.
Interesting, @nkdengineer, so you tried this case:
- enable
Advanced Approvalmode - set category&tag rule approvers
- then switch to
Submit and Closemode directly.
right?
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 
. As for the unit tests, we’ve already confirmed before, If needed, can add them in a follow-up PR.
cc @Beamanator
There was a problem hiding this comment.
Ooh ok I will test that out in OldDot now to see what happens!
There was a problem hiding this comment.
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.
Interesting, @nkdengineer, so you tried this case:
- enable
Advanced Approvalmode- set category&tag rule approvers
- then switch to
Submit and Closemode 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?!?
There was a problem hiding this comment.
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
|
@nkdengineer conflicts 🙏 I'm doing a final run through today, but seems we might be able to get this merged today!! |
|
Update now. |
|
@Beamanator Resolved conflict. |
There was a problem hiding this comment.
Ok looooooking good, just a few comments:
- So far the
QA Stepsonly include tests for theAdvancedapproval mode. Please update to include tests forSubmit & ApproveandSubmit & Close🙏- I think it would be also smart to make a quick note that QA should test approval modes on Collect plans as well to make sure they're still working perfect
- There's actually another part which I think we are missing - which is basically: when building the approval chain, we should check if everyone in the chain has already approved or not - because it's possible for the order to get mixed up if someone approves, then the approval chain changes. This, however, I think I'll try to handle in a follow-up cuz it's a pretty annoying case & we should get this one out the door 😅
| return [...categoryAppovers, ...tagApprovers]; | ||
| } | ||
|
|
||
| function getManagerAccountID(policy: OnyxEntry<Policy> | SearchPolicy, expenseReport: OnyxEntry<Report>) { |
There was a problem hiding this comment.
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
|
|
||
| // 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.
| // Don't push submiiter to approve as a rule approver | |
| // Don't push submitter to approve as a rule approver |
|
@Beamanator looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Failing test was esLint errors unrelated to these changes, so not an |
1 similar comment
|
Failing test was esLint errors unrelated to these changes, so not an |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.78-0 🚀
|
2 similar comments
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.78-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.78-6 🚀
|
Explanation of Change
getApprovalChainto include category/tag rule approversgetNextApproverAccountIDalready works as expectedFixed Issues
$ #52458
PROPOSAL:
Tests
Precondition:
ADVANCECAT1andCAT2, tagsTAG1andTAG2Offline tests
Same as above
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Precondition:
ADVANCECAT1andCAT2, tagsTAG1andTAG2PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2024-11-26.at.12.43.40.mov
Android: mWeb Chrome
Screen.Recording.2024-11-20.at.14.28.34.mov
iOS: Native
Screen.Recording.2024-11-26.at.12.45.17.mov
iOS: mWeb Safari
Screen.Recording.2024-11-26.at.12.41.25.mov
MacOS: Chrome / Safari
Screen.Recording.2024-11-26.at.12.30.19.mov
Screen.Recording.2024-11-26.at.12.34.51.mov
Screen.Recording.2024-11-26.at.12.35.47.mov
Screen.Recording.2024-11-26.at.12.36.28.mov
Screen.Recording.2024-11-26.at.12.37.03.mov
MacOS: Desktop
Screen.Recording.2024-11-26.at.12.47.36.mov