[Payment due @marufsharifi] [CP Staging] Stop nudging approvers to submit reports#96741
Conversation
|
@thesahindia 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
asked for an urgent review since this fixes a DB |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Thanks for jumping on this, @rushatgabhane.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-22.at.8.34.23.PM.movAndroid: mWeb ChromeRecording_20260722_193230.mp4iOS: HybridAppiOS: mWeb SafariScreen.Recording.2026-07-22.at.8.23.52.PM.movMacOS: Chrome / SafariScreencast.From.2026-07-22.17-35-32.webm |
marufsharifi
left a comment
marufsharifi
left a comment
There was a problem hiding this comment.
Looks good overall! I just have a few minor suggestions for your consideration.
| isChatReportArchived: false, | ||
| }), | ||
| ).toBe(CONST.REPORT.PRIMARY_ACTIONS.SUBMIT); | ||
| ).toBe(''); |
There was a problem hiding this comment.
'' is the genuine "no primary action" sentinel returned by getReportPrimaryAction, so this assertion is consistent with the source — 👍.
Optional: to lock both directions of this change, consider adding a sibling case asserting the owner still gets CONST.REPORT.PRIMARY_ACTIONS.SUBMIT on the same OPEN report, right next to this one. That guards against a future edit to the shared isSubmitAction silently breaking the owner path.
There was a problem hiding this comment.
sorry, i don't understand what you mean by the optional case
There was a problem hiding this comment.
No worries, let me be concrete.
Your changed test proves the negative direction: a workflow approver who doesn't own the report gets '' (no primary Submit). What it doesn't prove is that the same setup still gives the owner a Submit — i.e. that the new isCurrentUserSubmitter(...) guard discriminates by owner rather than just switching Submit off for everyone in an advanced-approval policy.
"Sibling case" just meant a second it(...) right after this one, reusing this exact fixture but flipping the current user to be the owner, asserting SUBMIT:
it('should return SUBMIT as the primary action for the owner of an OPEN expense report', async () => {
// same policy/report setup as the test above, but current user IS the owner
const report = createMock<Report>({
reportID: REPORT_ID,
type: CONST.REPORT.TYPE.EXPENSE,
ownerAccountID: CURRENT_USER_ACCOUNT_ID, // <- owner is the current user
stateNum: CONST.REPORT.STATE_NUM.OPEN,
statusNum: CONST.REPORT.STATUS_NUM.OPEN,
});
// ...same advanced-approval policy + transaction as the approver test...
expect(getReportPrimaryAction({ /* ...same params... */ }))
.toBe(CONST.REPORT.PRIMARY_ACTIONS.SUBMIT);
});The value is regression protection: if someone later inverts the guard or edits the shared isSubmitAction, the negative test alone wouldn't catch the owner losing Submit — the positive one would.
That said — feel free to skip it. I just noticed the file already has 'should return SUBMIT for open report in instant submit policy with no approvers' (and the one above it) covering the owner→SUBMIT direction, so the positive path isn't unguarded. The only thing a sibling would add is coverage of that direction within the advanced-approval fixture specifically, which is marginal. Non-blocking either way.
There was a problem hiding this comment.
same setup still gives the owner a Submit
but we already have tests for submitter should see Submit button. like, it won't cover more functions. if test breaks for advanced approval, they will break for instant submit too
rushatgabhane
changed the title
melvin-bot
Bot
changed the title
|
🎯 @marufsharifi, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
[Payment due @marufsharifi] [CP Staging] Stop nudging approvers to submit reports (cherry picked from commit 5fba1a0) (cherry-picked to staging by mountiny)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.42-1 🚀
|
|
🤖 I reviewed this PR against the help site articles under Why: This change only adjusts where the Submit action surfaces for a non-owner approver viewing a draft report:
Submit remains the green primary action for the report owner/submitter. I checked the two articles that document report actions: Both already describe Submit as the Submitter's primary action (the "Primary report actions" table lists Since there are no user-facing documented behaviors that this change contradicts, I did not open a docs PR (so there's no linked help site PR to review). @rushatgabhane, if you'd prefer to explicitly document that a non-owner approver can still force-submit from the More menu, reply and I'll draft that addition as a |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🤖 Payment issue created: #96790 |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.42-1 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.43-0 🚀
|
|
🤖 I reviewed this PR against the help site articles under Why: This change refines where the Submit action appears for a workflow approver viewing a draft report they don't own — moving it from the green primary button / report-preview Submit button to the More menu (header) and View (preview). It doesn't change any documented, user-facing capability. The relevant articles already describe Submit as fundamentally a submitter action, with force-submit available as a secondary action via More — which is exactly what this PR now enforces:
None of the articles document the previous behavior of nudging a non-owner approver with a primary Submit button, so there's nothing to correct. The generic descriptions of the primary vs. More buttons remain accurate after this change. Since no changes are needed, I did not open a draft docs PR. @rushatgabhane, please confirm you agree no help site update is needed here. If you'd like me to document approver-specific force-submit behavior explicitly (e.g., in |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.43-1 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
Slack: https://expensify.slack.com/archives/C07HPDRELLD/p1784685321619489
Submit is the primary CTA only for the report owner. For an approver viewing a draft they don't own, force-submitting is a secondary action, so:
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/662756
PROPOSAL:
Tests
Offline tests
Submit action visibility is computed locally from Onyx data, so the behavior is identical offline.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-07-22.at.16.56.38.mov