Show the auto-approval limit notice on the expense preview - #99796
Draft
KioCoan wants to merge 4 commits into
Draft
Show the auto-approval limit notice on the expense preview#99796KioCoan wants to merge 4 commits into
KioCoan wants to merge 4 commits into
Conversation
Contributor
Author
|
@arosiclair do we need a C+ to review this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Follow-up to Show the auto-approval limit notice when the submitter is also the approver, as requested in review.
That PR let a submitter who is also the report's approver see the
overAutoApprovalLimitnotice, so they finally learn why their report was held for manual approval. It landed on the expense detail Amount row and in Search, but not on the expense preview card, which is what this issue reports.The preview is the only surface that gates on the
showInReviewflag that comes down with the violation.createTransactionPreviewConditionalsaskshasNoticeTypeViolation(..., showInReview = true), which counts a notice only when the payload literally carriesshowInReview: true. The backend never sets it for this viewer: it decides that flag from whether the requester owns the report, and the violations query does not look atmanagerID, so it cannot tell that this submitter is also the approver. I confirmed the payload on a live held report, and it arrives with noshowInReviewkey at all:{"name": "overAutoApprovalLimit", "type": "notice", "data": {"amount": 10000, "currency": "USD", "formattedLimit": "$100"}}The expense detail (
useViolations) and the Search transaction row apply noshowInReviewfilter, which is why they show it today and the preview does not.Rather than change that flag in the backend, this adds a narrow exception on the client, next to the distance
MODIFIED_AMOUNTexception that already sits in the same two functions.shouldShowViolationis the single rule for who should see this notice, so the new helper asks it directly. It re-applies that rule rather than trusting the incoming array, becauseTransactionPreviewpasses violations already filtered byuseTransactionViolationswhileMoneyRequestReportTransactionListpasses raw Onyx violations, where a bare name check would light up the row for a plain submitter who must not see it.The helper is called from both
createTransactionPreviewConditionalsandtransactionHasRBR, which the file documents as mirrors of each other, so the report transaction list keeps sorting RBR rows the same way the preview flags them.The preview shows the dot and "Review required" rather than the full sentence. "Expense exceeds auto-approval limit of $100.00" is 46 characters against the 40 character
RBR_MESSAGE_MAX_CHARACTERS_FOR_PREVIEWcap, so it takes the samereviewRequiredfallback every other long violation message takes on a preview card. The full sentence still shows on the expense detail and in Search.Fixed Issues
$#99613
Tests
Unit tests
tests/unit/TransactionPreviewUtils.test.ts, two forcreateTransactionPreviewConditionalsand two fortransactionHasRBR, using anoverAutoApprovalLimitnotice with noshowInReviewkey, which is the shape the backend actually sends. Every violation fixture already in that file stampsshowInReview: true, which is why this gap was never caught.mainfirst and confirmed the two positive cases fail, so the tests prove the fix rather than the fixture.npx jest tests/unit/TransactionPreviewUtils.test.ts tests/unit/TransactionUtilsTest.ts tests/unit/useTransactionViolationsTest.ts tests/ui/TransactionPreviewContentTest.tsx tests/ui/MoneyRequestReportPreview.test.tsxand all pass. TheTransactionUtilsTestanduseTransactionViolationsTestruns guard theshouldShowViolationbehavior from PR 99016.Browser, against a local backend
Set up a workspace where I am the admin and submit to myself, with auto-approval enabled and a limit of $100, then created four reports on it: $175 held, $101 held, $150 approved and $99 auto-approved.
isProcessingReporthalf of the rule holding.Onyx.merge('report_<reportID>', {managerID: <some other accountID>})makes the dot and message disappear and flips the card button from Approve to View, and merging the real approver back restores both. That is theisReportManagerhalf of the rule holding, without needing a second account.Offline tests
N/A
QA Steps
Every expense below needs a merchant and a category, otherwise a missing field violation blocks auto-approval on its own and hides the behavior being tested.
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
Before: the $175 expense is correctly held for manual approval and the card offers an Approve button, but nothing on it says why it was not auto-approved. The approved $150 report above it is the control.

After: the same screen, same scroll position. The held report now explains itself with the dot and "Review required", and the approved report above is untouched, so the change does not leak outside reports awaiting approval.
Control, a different approver: the same held report with the approver set to someone else. No dot, no message, and the button reads View rather than Approve. This is the behavior we are deliberately preserving, since a submitter who cannot approve the report has nothing to act on.