Repository navigation
fix: disable report preview Submit button when strict workflow rules block submission - #97946
Conversation
…ubmission Mirror the report header's SubmitPrimaryAction gate in the report preview's SubmitActionButton: compute shouldBlockSubmit from shouldBlockSubmitDueToStrictPolicyRules and shouldBlockSubmitDueToPreventSelfApproval, disable the button, and early-return in handleSubmit before the submit-to popover branch so no doomed SubmitReport call reaches the backend. Previously only the header button was gated, so the preview button on the same report stayed enabled and clicking it surfaced the generic "Unexpected error. Please try again later." Issue: Expensify#92367 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Bb6ztR6SVJyk85SaHhL1a
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@mananjadhav 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7fe6a8b73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isBlockSubmitDueToStrictPolicyRules = shouldBlockSubmitDueToStrictPolicyRules( | ||
| iouReport?.reportID, | ||
| transactionViolations, |
There was a problem hiding this comment.
When strict rules are enabled, this passes the preview context’s raw transactionViolations slice into shouldBlockSubmitDueToStrictPolicyRules, whereas the header uses the filtered violations returned by useTransactionsAndViolationsForReport. That matters for dismissed violations that require report/policy context to ignore (for example instant-submit RTER dismissals by another participant): the helper’s hasAnyViolations call receives no report or policy here, so it treats those dismissed violations as still blocking and leaves the preview Submit button disabled even though the report header can submit. Please feed the same filtered violations as the header or extend the helper so this path filters dismissals with the report, submitter, and policy context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cfbfe86 — the strict-rules gate now receives dismissal-filtered violations, replicating useTransactionsAndViolationsForReport's filtering as a pure computation over the preview context's own data (no additional Onyx subscriptions per preview card), so the preview and header gates receive semantically identical input. Covered by a new unit test asserting the gate gets the filtered collection with the full report/owner/policy context.
Replicate useTransactionsAndViolationsForReport's dismissal filtering as a pure computation over the preview context's data, and feed the filtered collection to shouldBlockSubmitDueToStrictPolicyRules. The header's gate receives pre-filtered violations, so without this the two Submit buttons disagree on violations whose dismissal is only detectable with report/owner/policy context. Addresses the Codex review P2 on the pull request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Bb6ztR6SVJyk85SaHhL1a
|
Reviewing the PR. |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
| // The header's gate receives violations pre-filtered by useTransactionsAndViolationsForReport, which drops dismissals | ||
| // that are only detectable with report/owner/policy context (e.g. RTER violations dismissed under instant submit). The | ||
| // preview context exposes the raw slice, so apply the same filter here or the two Submit buttons disagree on dismissed | ||
| // violations. This stays a pure computation over data already in scope; calling the hook instead would duplicate its |
There was a problem hiding this comment.
This loop is a copy of useTransactionsAndViolationsForReport. The comment explains why we can’t call the hook (extra Onyx per preview card). I think it's worth extracting a shared pure helper later so the two can’t drift
There was a problem hiding this comment.
Agreed — the duplication is the one liability of this approach, and a shared pure helper (e.g. a filterDismissedTransactionViolations(transactions, violations, email, accountID, report, ownerLogin, policy) in TransactionUtils) would let both call sites use it without adding Onyx subscriptions. Happy to do it whichever way you prefer: extract it in this PR, or keep this diff focused and handle it as a follow-up since useTransactionsAndViolationsForReport has many consumers. Your call.
| }); | ||
|
|
||
| it('disables the button and blocks submission when the workspace prevents self-approval', () => { | ||
| // Given a workspace that prevents self-approval on a report whose next approver is the submitter |
There was a problem hiding this comment.
May be we can add expect(mockedShouldBlockSubmitDueToPreventSelfApproval).toHaveBeenCalledWith(iouReport, /* policy */ …), wdyt?
There was a problem hiding this comment.
Done in e3b882f — added expect(mockedShouldBlockSubmitDueToPreventSelfApproval).toHaveBeenCalledWith(iouReport, undefined). The second argument is undefined because the policy is never hydrated in this fully-mocked environment; left a comment in the test noting that so it doesn't read like a typo.
| mockedShouldBlockSubmitDueToStrictPolicyRules.mockReturnValue(true); | ||
|
|
||
| // When the button renders and its press handler runs anyway, because a disabled button swallows taps but the | ||
| // handler stays reachable through the keyboard shortcut and any in-flight press |
There was a problem hiding this comment.
The early return is good to test, but the comment about keyboard shortcuts might not be correct. This button doesn’t use pressOnEnter. Something like “call onPress directly to confirm it no-ops when blocked” would be clearer.
There was a problem hiding this comment.
Good catch — you're right, this button doesn't use pressOnEnter. Reworded in e3b882f: the comment now says onPress is called directly to confirm the handler no-ops on its own (the disabled prop is the first layer of defense, the early return in the handler is the second).
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
Minor comments @kamronbek04. Testing it out.
|
@kamronbek04 One thing I want to highlight is that we need builds for all the platforms. I am requesting adhoc builds for now. I hope you're able to run the tests on those builds. |
|
@lakchote Can you help with the adhoc build here? |
Assert shouldBlockSubmitDueToPreventSelfApproval receives the report and policy the component reads, and reword an inaccurate test comment: this button has no keyboard shortcut, the direct onPress call exercises the handler's early return as the second layer behind the disabled prop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwjvYBYjgntCP6ZeKgEfga
|
Thanks @mananjadhav — both test comments addressed in e3b882f (9/9 passing, lint/format/typecheck clean), and replied inline on the helper extraction. On the adhoc builds: once they're posted I'll run the tests on the Android build on a physical device (same device as the mWeb Chrome screenshots) and add native screenshots to the PR body. I don't have iOS/macOS hardware, so I'd appreciate verification on the iOS build during your review. |
|
Reviewing the changes again. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
|
@kamronbek04 Please resolve conflicts. |
|
@mananjadhav conflicts resolved — Both conflicts were additive collisions rather than genuine disagreements:
Nothing else in either file was hand-edited; the rest ( Verified locally on the merged head: ESLint clean, oxfmt clean, React Compiler compiles and memoizes on both Babel and OXC with no divergence, and typecheck reports no errors in either file. The |
|
@kamronbek04 there's this action failing https://github.com/Expensify/App/actions/runs/32455546132/job/97498218301?pr=97946, can you take a look? |
|
@lakchote Thanks for the ping — that finding isn't coming from this PR's diff. This PR changes exactly two files, It was branch staleness. Fixed by merging current One thing worth flagging: the new Knip run is sitting at On the merged head I verified that the PR's two files came through byte-identical, that the helpers they call are unchanged in signature (
|
|
@kamronbek04 perf tests are failing |
|
@lakchote Thank you for flagging this — I took a close look at the run, and I believe the failure isn't caused by this PR's changes. It appears to be the same baseline-skew issue we saw with the Knip check. Please allow me to walk through the evidence. The clearest signal comes from comparing the green Aug 21 run with the failing one, per scenario (baseline → branch):
The branch's render counts are identical in both runs, so the branch itself didn't regress. What changed is the baseline: I was able to trace the cause:
For completeness: none of the failing suites render I've addressed it by merging current One small observation that may be useful to the workflow maintainers: Thanks again for the ping, and sorry for the extra review round-trips this is causing. |
|
@lakchote Gentle bump — all checks are green on |
|
Quick bump @lakchote |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.4.74-0 🚀
|
|
Yes — help site changes are required, and I opened a draft PR for them: #100635 WhyThis PR changes what a member sees: the Submit button on the report preview card in chat is now grayed out when a domain group enforces workspace rules on a report with violations, or when Prevent self-approvals blocks the submission. Previously the button looked active and the submission failed with a generic "Unexpected error. Please try again later." The help site documented neither the old error nor the new grayed-out state. What changed in the docs PR
UI label verification and what I deliberately left aloneVerified against the live app on web:
Not changed:
Both files follow One thing to flag: GitHub wouldn't let me set you as assignee on the docs PR because assignees need write access on @kamronbek04, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
@jponikarchuk Double-checked — #97946 can be ruled out. It only changes the report preview's Submit button ( |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀
|
Explanation of Change
There are two Submit buttons for an expense report: the report header button (
SubmitPrimaryAction) and the report preview card button in chat (SubmitActionButton). Only the header was wired to the strict-policy-rules gate, so on a domain with Strictly Enforce Workflow Rules a violating report showed a disabled header button but an enabled preview button. Pressing the preview button firedSubmitReport, the backend rejected it, and the user only saw the generic "Unexpected error. Please try again later."This PR mirrors the header's existing two-layer gate inside
SubmitActionButtonContent:shouldBlockSubmitfrom the same helpers the header uses —shouldBlockSubmitDueToStrictPolicyRules(withareStrictPolicyRulesEnabledfromuseStrictPolicyRules) ORshouldBlockSubmitDueToPreventSelfApproval— with identical argument order and variable naming, so agit grep shouldBlockSubmitparity audit passes acrossSubmitPrimaryAction.tsx,useLifecycleActions.tsx, and this file;isDisabled={shouldBlockSubmit}toAnimatedSubmitButton(which already forwards it toButton);handleSubmit, above theisSubmitPolicypopover branch — the submit-to popover runs the submission itself, so a lower guard would leave that path unblocked. This matches the guard position in today'sSubmitPrimaryAction.handleSubmit.Violations come from the preview's own context (
useReportPreviewTransactionViolations) — the same source the component's existinghasViolations/hasAnyPendingRTERViolationcalls use.No copy changes (
src/languages/en.tsuntouched); the fix prevents the doomed call rather than rewording its error.Notes for the reviewer (scope decisions):
iou.error.otherinsubmitReport'sfailureData). That is deliberately not included: it depends onSubmitReportreturning violation detail inresponse.onyxData, which needs backend confirmation first (asked in the issue).useTransactionsAndViolationsForReport's filtering as a pure computation over data already in the preview context (no extra Onyx subscriptions per preview card) — so the preview and header gates receive semantically identical input, including for dismissals that need report/owner/policy context (this addresses the Codex review's P2). The component's pre-existinghasViolations/hasAnyPendingRTERViolationcalls keep receiving the raw slice, as they did before this PR — changing them is out of scope.Fixed Issues
$ #92367
PROPOSAL: #92367 (comment)
Tests
Strict workflow rules (main path):
Prevent self-approval (second operand):
7. On a workspace with Workflows > Add approvals where the approver is the submitter themselves, enable Prevent self-approvals (Workflows > Advanced) → verify the preview Submit button is disabled; disable the setting → it re-enables.
Offline tests
QA Steps
Same as Tests (requires a domain with Strictly Enforce Workflow Rules on staging).
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.Note on platform coverage: the change is platform-agnostic shared logic (a boolean gate +
isDisabledon an existing shared component) with no platform-specific code or styling. I don't have access to a Mac or iOS hardware; native platforms can be verified via an adhoc test build or during C+ review — happy to follow up on any platform on request. The blocked state in the screenshots reproduces the domain security-group flag and server violation locally (the real "Strictly Enforce Workflow Rules" setting requires a private-domain admin), which drives the exact same Onyx-derived gate the fix reads.Screenshots/Videos
Android: Native
No local HybridApp build environment available — requesting verification via adhoc test build / C+ review. The gate is shared JS with no native-specific code.
Android: mWeb Chrome
Real device (Xiaomi Redmi Note, Android Chrome). Blocked: violation shown, Submit disabled — while the unaffected report above keeps its active Approve button. Then violation cleared: Submit re-enables.
iOS: Native
No macOS/iOS hardware available — requesting verification via adhoc test build / C+ review. The gate is shared JS with no native-specific code.
iOS: mWeb Safari
No iOS hardware available; captures below are the identical responsive bundle at a mobile viewport (the same code path mobile Safari loads). Happy to re-verify on a real device via adhoc/C+ if needed.
MacOS: Chrome / Safari
Desktop Chrome. Blocked preview (violation + disabled Submit), then the same report open — header and preview Submit both disabled (parity with
SubmitPrimaryAction), then state cleared — both re-enabled (no over-blocking).