[Change Approver] Add support to bypass approver on an expense report #66899
[Change Approver] Add support to bypass approver on an expense report #66899marcaaron merged 37 commits intoExpensify:mainfrom
Conversation
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I will have the checklist completed tomorrow morning. Sorry about that. |
|
Status is not updated immediately upon taking control. It requires refresh of report. @marcaaron
|
|
@shubham1206agra What is the standard way to generate translations in all languages? |
I am using |
|
@parasharrajat Have you got this fixed #66899 (comment)? |
|
Yes, the issue was fixed. #66899 (comment) is an unrelated problem that I noticed in code so I raised on Slack. |
| // @todo we will remove checking whether current manager is admin in PR #68353 | ||
| // When report manager is not the policy admin and current user is policy admin, allow changing the approver | ||
| if ( | ||
| !isMemberPolicyAdmin(policy, getLoginByAccountID(report.managerID ?? CONST.DEFAULT_NUMBER_ID)) && |
There was a problem hiding this comment.
@marcaaron @parasharrajat What should happen here if the self-approval is disabled and we try to switch approver to ourselves for my own expense report?
There was a problem hiding this comment.
Totally fine to take control as long as you are admin.
There was a problem hiding this comment.
Self approval prevention I think maybe does not apply to "Take Control" case. In any case, it's the admin who has control over this feature and can turn it off so I think that's maybe why we allow it. Not entirely sure, but that's how it works on OD today so we can keep it that way for ND.
There was a problem hiding this comment.
Ideally, we'd limit the list of eligible approvers to remove the submitter if self-approvals is disabled.
There was a problem hiding this comment.
I don't think we have to do anything here on this PR for this. Bypass option is only applicable to admins who is not the manager themselves so self-approval concept does not apply in this case.
There was a problem hiding this comment.
I don't think we have to do anything here on this PR for this. Bypass option is only applicable to admins who is not the manager themselves so self-approval concept does not apply in this case.
Self-approval will apply if the report we are approving is ours only. Logically, we should not allow them to change approver to self here. But we can leave it here for now.
There was a problem hiding this comment.
I think we should follow up on this as a polish item. Let's get the core flow out.
| */ | ||
| type OriginalMessageTakeControl = { | ||
| /** Whether the action was taken on newDot or oldDot */ | ||
| isNewDot: boolean; |
There was a problem hiding this comment.
@parasharrajat Is this property really in use anywhere?
There was a problem hiding this comment.
If not, @marcaaron can we stop sending this in payload?
There was a problem hiding this comment.
It is coming from the backend, so I matched the data structure.
There was a problem hiding this comment.
Good callout. I may have sent this unintentionally. I think if we are not referencing it in the App code (not sure why we would). Then it's fine to remove this.
There was a problem hiding this comment.
I will remove that in the next PR.
|
Let's pull out the stops @shubham1206agra those last comments are not blockers so let's get this tested and merged! |
|
@shubham1206agra, please let me know if you have noticed any issues. Thanks. |
Yes |
|
@shubham1206agra what's your ETA on the checklist? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-08-22.at.9.07.20.PM.movAndroid: mWeb ChromeScreen.Recording.2025-08-22.at.8.41.33.PM.moviOS: HybridAppScreen.Recording.2025-08-22.at.9.01.07.PM.moviOS: mWeb SafariScreen.Recording.2025-08-22.at.8.33.50.PM.movMacOS: Chrome / SafariScreen.Recording.2025-08-22.at.6.46.47.PM.movScreen.Recording.2025-08-22.at.6.56.46.PM.movMacOS: DesktopScreen.Recording.2025-08-22.at.8.49.38.PM.mov |
|
@parasharrajat can you fix the conflicts? |
|
On it. |
|
@marcaaron I updated the manual test sheet to add some additional tests. |
|
All set @marcaaron |
|
✋ 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/marcaaron in version: 9.1.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.99-11 🚀
|
| onSelectRow={(option) => setSelectedApproverType(option.keyForList)} | ||
| showConfirmButton | ||
| confirmButtonText={translate('iou.changeApprover.title')} | ||
| onConfirm={changeApprover} |
There was a problem hiding this comment.
We forgot to pass prop shouldUpdateFocusedIndex here which caused an issue
| !isMemberPolicyAdmin(policy, getLoginByAccountID(report.managerID ?? CONST.DEFAULT_NUMBER_ID)) && | ||
| isExpenseReportUtils(report) && | ||
| isProcessingReportUtils(report) && | ||
| isPolicyAdmin(policy) |
There was a problem hiding this comment.
We forgot to check if the approval is enabled or not #76655.

Explanation of Change
Desing doc: https://docs.google.com/document/d/1XKNwFO_-hDU9IUa9f8oWYpwMy5BltHSYDdmdvJigi40/edit?tab=t.0#heading=h.lf9ovssvs77q
Fixed Issues
$ #66193
PROPOSAL: As per design doc.
Tests
Prerequisites
Morein the header button.Change Approverbutton.Change Approverbutton on the report from member A.Bypass Approver.Changed the approver @youis posted on the report by you.Offline tests
Morein the header button.Change Approverbutton.Change Approverbutton on the report from member A.Bypass Approver.Changed the approver @youis posted on the report by you.QA Steps
Same as tests
PR 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
12.08.2025_16.46.40_REC.mp4
Android: mWeb Chrome
12.08.2025_16.32.53_REC.mp4
iOS: Native
12.08.2025_16.43.01_REC.mp4
iOS: mWeb Safari
12.08.2025_16.35.53_REC.mp4
MacOS: Chrome / Safari
12.08.2025_16.20.34_REC.mp4
MacOS: Desktop
12.08.2025_16.25.09_REC.mp4