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: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: 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 For this bug, I see it's expected here that will show the |
|
Ok IDK bout y'all, but I think y'all need to be suuuuuper clear on the setup you're using to confirm / deny if the policy owner is expected to be the "final approver" in your cases. @nkdengineer the PR you linked doesn't have anything to do with category/tag approvers, it looks like a super simple setup - does that apply here? Do you both understand each other's setups or should you both lay out the setup (policy type, |
I think we both have a clear understanding of each other's setups, it's just that we haven't reached an agreement on what should be displayed after the last I have the following setup:
|
|
Thanks @ntdiary that's very helpful! Can you also please show what the approval workflow looks like? I see you mention "the final approver is policy owner (no.25)" but I would love to see how that's set up in your |
|
@Beamanator, this 
For easier testing, my account name matches the email number. e.g., approval flow: submitter( submitter-no.1-no.2.mp4no.2-no.3-no.4-no.25.mp4 |
|
Thanks @ntdiary ! 👍 Ok ya so it IS expected that, in your case, the policy owner is the final approver - this is because the submitter's |
|
@Beamanator So it's not a bug, right? |
|
Right, in @ntdiary 's case at least, it's not a backend bug, it is expected for the |
@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
…On Mon, Dec 9, 2024 at 5:44 PM wentao ***@***.***> wrote:
Right, in @ntdiary <https://github.com/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 <https://github.com/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. :)
—
Reply to this email directly, view it on GitHub
<#52537 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5UTPZDNMPK5NODB2IK3KD2EY2PDAVCNFSM6AAAAABRYPSOSOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRZHEZDOOJYGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
BTW, @Beamanator, curious, can we have multiple tag approvers for 1 single expense? So far, I’ve only seen that a regular tag can have just one approver. Although we have multi-level tags, it seems like approvers can’t be assigned to them yet? 
|
|
@ntdiary yikes, yeah in OldDot I'm struggling to even get multi-level tags set up in a policy 🙈 I guess NewDot doesn't support multiple tags even at the moment, so probably won't support multiple tag approvers for a bit, so I would say we can skip that part of tests / sorting & add it in later whenever that's added to the app |
|
So what's the status of this PR? @nkdengineer are you waiting on anyone for anything? |
I think this is the last major problem that needs to be addressed. :D |
| return 1; | ||
| } | ||
|
|
||
| return (transA.inserted ?? '') < (transB.inserted ?? '') ? -1 : 1; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think we can make it optional and update it in buildOptimisticTransaction because the draft transaction don't need to add this field.
There was a problem hiding this comment.
@ntdiary I updated.
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