Stop showing the LHN RBR, Fix badge and To-do for companyCardRequired after submit - #100190
Conversation
…uired after submit Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
@codex review |
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
Two checks were failing. I fixed one and confirmed the other is unrelated. Fixed — The ternary at Not fixed —
This PR touches Local verification
view run · no recording available |
![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: 55f4dc2259
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| hasViolation(transaction, violations, currentUserEmailParam, currentUserAccountIDParam, report, currentUserEmailParam, policy, true) || | ||
| hasWarningTypeViolation(transaction, violations, currentUserEmailParam, currentUserAccountIDParam, report, currentUserEmailParam, policy, true) || |
There was a problem hiding this comment.
When a submitted expense has companyCardRequired alongside a currently hidden hard violation—for example, missingCategory while auto-categorization is pending—hasVisibleViolationsForUser() passes because the company-card violation itself is visible. After that violation is removed here, hasViolation() still accepts missingCategory because it does not call shouldShowViolation() (which explicitly hides that violation while isCategoryBeingAnalyzed() is true). The function therefore continues returning the report ID, leaving the LHN RBR, Fix badge, and To-do visible even though none of the remaining issues are actionable; apply the same per-violation visibility predicate before these type checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex is right, and I've fixed it in 2a9d573.
The premise checks out: of the three helpers I now OR together, only hasNoticeTypeViolation calls shouldShowViolation. hasViolation and hasWarningTypeViolation only check type, showInReview and dismissal. So the two operands of the && were reading different lists — hasVisibleViolationsForUser the raw one, my helper the name-filtered one — which let a visible companyCardRequired vouch for a hidden missingCategory that survived the filter. The dot stayed lit, which is the exact outcome this PR exists to prevent.
The fix moves shouldShowViolation into the same filter as the name exclusion, so both operands agree on what counts:
Lines 9939 to 9947 in 2a9d573
One correction to the framing, since it affects how you weigh this: it is not a regression. On main the same report shows the same red dot — just for a different reason (companyCardRequired itself satisfied the unfiltered hasViolations). main has the identical hole for its own modifiedAmount exclusion. The PR's considered violation set is a subset of main's at every branch, so it could only ever remove dots, never add one. What Codex found is a pre-existing gap that this PR would have inherited and left visible in one case, not something it broke.
Reachability, test, and verification
How you'd actually hit it. shouldShowViolation hides missingCategory while isCategoryBeingAnalyzed is true — either pendingAction === ADD or within a 60-second pendingAutoCategorizationTime window. Both violations are backend-generated, so pendingAction === ADD is largely mutually exclusive with having them. The realistic path is instant-submit: the report is born SUBMITTED, the backend returns both violations, and for up to a minute the category chip reads "analyzing" while the LHN dot stays on. Narrow and transient — but the same argument applies to any other hidden violation, and the fix covers all of them rather than special-casing this pair.
Test. Added should not surface RBR when the violation left next to companyCardRequired is hidden while the category is being analyzed. I confirmed it actually reproduces the bug: stashing the ReportUtils.ts change and re-running gives 1 failed, 5 passed on the companyCardRequired block; with the fix restored it passes.
Checks on 2a9d573:
| Check | Result |
|---|---|
npm test -- tests/unit/ReportUtilsTest.ts |
✅ 1309 passed |
npm test -- ViolationUtilsTest SidebarFilterTest SidebarOrderTest OptionsListUtilsTest navigateAfterOnboardingTest |
✅ 614 passed |
npm run typecheck |
✅ passed |
npm run lint -- src/libs/ReportUtils.ts tests/unit/ReportUtilsTest.ts |
✅ passed |
npm run fmt |
✅ no diff beyond the intended change |
npm run spell |
✅ 0 issues in the changed files |
npm run lint-changed and spell-changed still can't run in this checkout — no merge base with origin/main — so I ran lint and spell against the files directly instead.
|
🤖 Android test steps verification results: ❌ fail I could not run the testing steps. The app crashed on the Android emulator with a fatal RedBox Fabric error (
Two things worth flagging:
Retriggering the |
|
Oxfmt check is now green — my earlier commit Nothing left to push. The three still-red checks are all outside this PR:
The
It will pass on its own tomorrow. Per policy I'm not touching an unrelated flake from this PR, but the off-by-one in Evidence it isn't this PR
Local checks on the pushed commit: |
|
@MelvinBot Take a step back and make an adversarial review of this PR |
|
The remaining red checks are not this PR's code:
Every other PR running CI today hits it too — for example #100189, an unrelated PR, fails the identical assertion in the identical job. This PR only touches Fixing it means changing Local verification of the Oxfmt fix
On CI, |
|
The three remaining red checks are all outside this PR's scope:
On Why it fails only todayThe test derives its own expected value from This PR changes Once #100134 is fixed on |
hasViolation and hasWarningTypeViolation never applied shouldShowViolation, so once companyCardRequired was filtered out by name a hidden violation left behind it could keep the red dot lit. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
Nothing left to fix on this PR —
The test derives its own expected value from The other two red checks are review gates, not code failures: Current check state
I reproduced the |
|
Production builds the auto-submit ETA at let etaDateTime = setDate(new Date(), policy?.autoReportingOffset);
if (isPast(etaDateTime)) {
etaDateTime = addMonths(etaDateTime, 1);
}
I confirmed this by running that single test twice back to back on the current head — one pass, one fail, same commit, same minute. It should stop failing on its own tomorrow. This is a real (if cosmetic) bug in Full check status and local verificationFixed earlier in Verified on the current head
The other two red checks are human gates, not code failures: view run · no recording available |
Adversarial reviewThe If you land only the name exclusion, this is a clean one-line fix. The 1. 🔴
|
| Violation | Why it now returns false | Scope |
|---|---|---|
receiptNotSmartScanned |
isPolicyMember && !isSubmitter && !isReportOpen — always false for the submitter |
open and submitted |
missingAttendees |
isAttendeeTrackingEnabledForPolicy(policy) is false on every non-Control policy, and when policy hasn't loaded yet |
open and submitted |
futureDate |
re-derived from the local date, so a stale back-end violation is dropped once the date passes | open and submitted |
overAutoApprovalLimit |
needs isPolicyAdmin && (!isSubmitter || isReportManager) |
open and submitted |
missingCategory |
during the 60s analyze window or pendingAction === ADD |
the only one tested |
Note the scope column: excludedViolationNamesForLHN is empty on open reports, but the shouldShowViolation filter still runs there. So this reaches reports the fix explicitly says it doesn't touch. The missingAttendees case is the sharpest — that's a hard violation dropped for every Collect/Team workspace.
Your inline comment argues the filter is needed because hasVisibleViolationsForUser reads the raw list. That's true, but it justifies filtering the leftovers after a name exclusion — it doesn't justify applying it on open reports where nothing was excluded.
Ask: either scope the shouldShowViolation filter to excludedViolationNames.length > 0, or split it into its own PR with tests for the five names above.
2. 🔴 The modifiedAmount exclusion silently widened from notice-only to all three types
Old code passed excludedNoticeNamesForLHN only into the notice branch. Line 9893 now applies it to all three. modifiedAmount is known to arrive as violation as well as notice, so on a processing report a violation-typed modifiedAmount stops driving the RBR where it previously did. Not in the title, not in the description, no test.
3. 🟡 The stated criterion doesn't match the precedent it cites
The description says companyCardRequired qualifies because it "maps to no editable field (useViolations.ts maps it to none)... the same mechanism already used for modifiedAmount." But useViolations.ts:39 maps modifiedAmount to 'amount' — an editable field. The real rule behind the precedent is "not actionable once submitted", not "maps to none".
That matters because it leaves "is the two-name list complete?" unanswered. The none mapping isn't the discriminator: the only other none is hold, which is resolvable. Please state the actual rule.
4. 🟡 The central safety claim and two of the three named surfaces are untested
- "Keeps the violation message visible to everyone on the expense" — the whole justification for scoping this to the RBR — has no test. It exists only as a code comment. This is the one I'd block on.
- The title names three surfaces. All five new assertions call
getViolatingReportIDForRBRInLHNdirectly; nothing covers the Fix badge or the Inbox To-do. That's not cosmetic — the To-do path reaches the helper viahasErrorsOtherThanFailedReceiptand bypasses thewillShowGreenSubmitandisChildOfAccessiblePolicyExpenseChatguards that gate the dot and badge. - Minor framing: the red dot and the Fix badge aren't two fixes.
ErrorBadge.tsx:34-45renders one or the other from the samebrickRoadIndicator === ERROR.
5. 🟡 "The plural wrappers are unchanged, since they have many other callers" is only half true
True for hasViolations (~33 call sites). False for hasWarningTypeViolations — it has exactly one caller left, the private sibling hasAnyViolations at ReportUtils.ts:10078, and it isn't exported. Verified by grep across src/ and tests/. Not a defect, but the description shouldn't lump them together.
Lower-severity findings
6. hasVisibleViolationsForUser may now be a dead AND-term. The new helper applies the same shouldShowViolation(...) with the same arguments, and the singular helpers apply the same isViolationDismissed. So hasViolationOfAnyTypeForRBRInLHN === true implies hasVisibleViolationsForUser === true, and the gate at L9897-L9904 can no longer change the outcome. Your comment at L9944-L9945 justifies the filter using a condition the filter itself made unreachable.
7. The last test depends on the wall clock. ReportUtilsTest.ts:17499 builds pendingAutoCategorizationTime from new Date() to land inside a 60s window. Timers are real here — jest/setupAfterEnv.ts:11 calls jest.useRealTimers() for every suite. TransactionUtilsTest.ts:2350 already uses the jest.setSystemTime idiom. Practical flake risk is low (the 5s per-test timeout fires long before 60s elapses), but the same test also asserts a negative with no positive control on the same fixture — if the fixture silently broke, it would still pass.
8. isProcessingReport(potentialReport) at L9893 is exactly !isOpenReport(potentialReport) given the isOpenOrProcessingReport guard 13 lines above at L9879-L9882. The ternary advertises a lifecycle discrimination it can't exercise, and the comment reads as if it handles post-approval states that can never reach it. (I checked: the fix is not incomplete for APPROVED/REIMBURSED — those return false at the guard.)
9. Double work. L9946 evaluates shouldShowViolation for every violation, then hasNoticeTypeViolation re-evaluates it internally on the already-filtered list. The old excludedViolationNames.length > 0 ? filter : raw short-circuit was also dropped, so the filter now always runs on the LHN hot path.
Things I checked and cleared: argument order and positional correctness of all the new call sites (the true at position 6 of shouldShowViolation is shouldShowRterForSettledReport, which is also its default — not showInReview); the ViolationName typing (sound, though the old string[] also type-checked, so this is a strictness upgrade, not a fix); hasNoticeTypeViolationsForRBRInLHN is fully removed with nothing exported-but-unused; the submitter gating is real and doubly enforced (the guard at L9862-L9865 and the owner filter on the report cache at L1152), so no admin/approver leak is possible; and companyCardRequired genuinely has no Mark as cash and no dismiss affordance anywhere.
Not fixed, worth stating in the description: Search still renders a red dot and the violation text on the report row post-submit — shouldShowViolationDescription is explicitly isOpenExpenseReport || isProcessingReport. You call this intentional, and I agree with the reasoning, but the reporter on #99793 will still see a red dot somewhere.
CI: test (job 3) is red, but it's unrelated — NextStepUtilsTest.ts:314, case "monthly on the 2nd", failing on the 2nd of the month. A pre-existing date flake, and an argument for finding 7. Your new tests pass in CI (jobs 1, 2, 4–8 green; job 3's only failure is that one).
Basis: static analysis of the working tree at 2a9d573 plus CI logs. I could not execute jest or typecheck locally in this environment, so I did not independently reproduce the five suppressed-violation cases at runtime — they are read off shouldShowViolation's branches, not observed.
|
@MelvinBot Take a look at this review. What should we fix? I'm not sure about the first one, but we don't want to change behaviour for others violations. In other words, we need to leave the logic for all violations as is and change it only for |
Drop the shouldShowViolation pre-filter and keep modifiedAmount notice-only, so companyCardRequired is the only violation whose behaviour changes. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
You were right to push back, and your rule resolves finding 1 for us. I've pushed Two things needed fixing, and your rule fixes both with one change: drop the On the first finding you weren't sure about — the mechanism is real, but the pre-filter was never needed for The reviewer also overstated its blast radius, which is worth recording so nobody re-adds it thinking it fixed five things:
Finding 2 was the real defect — and worse than "untested". What changed in
|
Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
Same two checks as before. Fixed the Oxfmt one; the test failure is still the unrelated date flake. Fixed — The two commits added after my last fix ( Not fixed — Still Local verification on 5e3b071
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
Nothing to push this time — the one real failure is already tracked elsewhere.
The test builds its expected value from Two ways forward: land a fix on #100134 first, or re-run
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e3b0716b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| hasViolation(transaction, violations, currentUserEmailParam, currentUserAccountIDParam, report, currentUserEmailParam, policy, true) || | ||
| hasWarningTypeViolation(transaction, violations, currentUserEmailParam, currentUserAccountIDParam, report, currentUserEmailParam, policy, true) || |
There was a problem hiding this comment.
When a submitted expense contains a visible companyCardRequired violation alongside missingCategory while auto-categorization is pending, the outer hasVisibleViolationsForUser() check succeeds because the company-card violation is visible. After that violation is excluded, hasViolation() and hasWarningTypeViolation() still accept the hidden violation because neither applies shouldShowViolation(), so the LHN RBR, Fix badge, and To-do remain despite there being no actionable issue. Fresh evidence since the existing thread is that follow-up commit bb26bc5 removed the shouldShowViolation() prefilter from the final tree; retain per-violation visibility filtering while preserving the intended notice-only treatment of modifiedAmount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex is right on the facts, and I've fixed it in 2f927ce — though not by restoring what bb26bc5 removed.
The finding checks out. bb26bc5 did drop the shouldShowViolation prefilter that 2a9d573 added, and the hole came back with it. On a submitted report carrying a visible companyCardRequired next to a hidden missingCategory, hasVisibleViolationsForUser passed on the raw list while the type checks read the name-filtered one, so the excluded violation vouched for the hidden one and the dot stayed lit.
But restoring the prefilter is the wrong fix, which is why it was removed. It applied shouldShowViolation to the violation and warning buckets, which previously only the notice check gated — and it ran on open reports too, where the exclusion list is empty. That silently dropped receiptNotSmartScanned, missingAttendees, futureDate and overAutoApprovalLimit from the RBR on reports this PR is not supposed to touch. Putting it back would trade one narrow bug for four broad ones.
What I did instead: pass the same exclusion list to the visibility check, so both operands of the && judge the same set.
Lines 9900 to 9913 in 2f927ce
This closes the vouching hole with no collateral scope. Nothing about which violation types drive the dot changes, so the four violations above are untouched. On open reports excludedViolationNamesForLHN is empty, making the new argument a literal no-op — behaviour there is identical to main. On submitted reports the only delta is companyCardRequired itself, which is the entire point of the PR. Both operands can now only shrink relative to main, so this can remove dots but never add one.
One correction to Codex's framing, unchanged from the earlier thread: this is not a regression. main lights the same dot on the same report, just because companyCardRequired itself satisfied the unfiltered hasViolations. It's a pre-existing gap this PR would have inherited, not one it introduced.
Reachability, test, and verification
How you'd hit it. shouldShowViolation hides missingCategory while isCategoryBeingAnalyzed is true — pendingAction === ADD, or inside a 60-second pendingAutoCategorizationTime window. Both violations are backend-generated, so pendingAction === ADD is largely mutually exclusive with having them. The realistic path is instant-submit: the report is born SUBMITTED, the backend returns both violations, and for up to a minute the category chip reads "analyzing" while the dot stays on. Narrow and transient — but the fix is name-generic, so it covers any hidden violation, not just this pair.
Test. Re-added should not surface RBR when the violation left next to companyCardRequired is hidden while the category is being analyzed (it was dropped in bb26bc5 along with the prefilter). I confirmed it actually pins the behaviour: with the new argument removed it is the only failure in the block —
● should not surface RBR when the violation left next to companyCardRequired is hidden while the category is being analyzed
expect(received).toBeNull()
Received: "expense-rbr-company-card-alongside-hidden"
Tests: 1 failed, 8 passed
— and with it restored all 9 pass. Notably the "should still surface RBR on an open expense report" test passes in both configurations, which is the open-report no-op claim above holding empirically.
Checks on 2f927ce:
| Check | Result |
|---|---|
npm test -- tests/unit/ReportUtilsTest.ts |
✅ 1312 passed |
npm test -- tests/unit/ViolationUtilsTest.ts |
✅ 186 passed |
npm run typecheck |
✅ passed |
npm run lint -- src/libs/ReportUtils.ts src/libs/Violations/ViolationsUtils.ts tests/unit/ReportUtilsTest.ts |
✅ passed |
npm run fmt |
✅ no diff |
hasVisibleViolationsForUser gained a defaulted trailing parameter, so the change is backwards-compatible: ReportUtils.ts is its only production caller, and all existing ViolationUtilsTest call sites are unchanged.
Both operands of the && in getViolatingReportIDForRBRInLHN have to judge the same set of violations. Passing the exclusion list only to the type checks let a visible companyCardRequired vouch for a violation hidden from the submitter, keeping the red dot lit with nothing behind it. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
@MelvinBot update PR Explanation of changes section to reflect PR changes |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp100190-android-native.movAndroid: mWeb Chrome100190-android-web.moviOS: HybridApp100190-ios-native.moviOS: mWeb Safari100190-ios-web.movMacOS: Chrome / Safari100190-web.mov |
|
Updated the description to match the diff as of
I also added the caveat that the back end's type bucket for The Checks re-run on
|
| Check | Result |
|---|---|
npm test -- tests/unit/ReportUtilsTest.ts |
✅ 1311 passed |
npm test -- tests/unit/ViolationUtilsTest.ts tests/unit/SidebarUtilsTest.ts tests/unit/SidebarFilterTest.ts tests/unit/SidebarOrderTest.ts tests/unit/TransactionUtilsTest.ts |
✅ 738 passed, 144 skipped |
npm run typecheck |
✅ passed |
| ESLint on the three changed files | ✅ 0 errors |
| cspell on the three changed files | ✅ 0 issues |
The 144 skipped are all of SidebarFilterTest, which is xdescribed on main — not something this PR did. lint-changed and spell-changed still can't run here because this checkout is shallow and their git merge-base step fails, so I ran ESLint and cspell against the changed files directly. CI's own ESLint check, spellcheck, Oxfmt check, typecheck and all eight test jobs are green on this commit.
DetailsExplanation of ChangeAfter a report is submitted, a This PR keeps the violation message visible to everyone on the expense, and stops it from driving the RBR / Fix / To-do for the submitter once the report is submitted.
Fixed Issues$#99793 TestsPreconditions
Offline testsN/A QA Steps// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safari@MelvinBot please set the PR body to the content in the details section above |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Done — the PR body is now the content from your details block, verbatim. One thing to check before this merges: the |
|
@Eskalifer1 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}
| const transactions = getReportTransactions(potentialReport.reportID).filter((transaction) => !isTransactionPendingDelete(transaction)); | ||
|
|
||
| const excludedNoticeNamesForLHN = isProcessingReport(potentialReport) ? [CONST.VIOLATIONS.MODIFIED_AMOUNT] : []; | ||
| // A submitted `companyCardRequired` is not actionable by the submitter, so it must not drive the RBR; it is excluded by name because the back end owns its type. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
Comments should read as plain, natural sentences. This comment uses a semicolon to join two independent clauses instead of writing two separate sentences, which the plain-comment-style guideline flags.
Split the two clauses into separate sentences:
// A submitted `companyCardRequired` is not actionable by the submitter, so it must not drive the RBR. It is excluded by name because the back end owns its type.Reviewed at: fc02644 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Correct — that's a semicolon joining two independent clauses, which CONSISTENCY-16 flags. Fixed in 7141e8e, split into two sentences and wrapped to match the neighbouring comments: src/libs/ReportUtils.ts:9888-9889.
The same commit fixes the second CONSISTENCY-16 hit on the hasViolationOfAnyTypeForRBRInLHN JSDoc. Comment text only, no logic touched — npm run typecheck passed, ESLint on the file 0 errors, tests/unit/ReportUtilsTest.ts 1311 passed.
There was a problem hiding this comment.
Already handled — this landed in 7141e8e a few minutes ago (my reply just above), so this is only a confirmation that it's in and green.
The comment now reads as two sentences, exactly as suggested: src/libs/ReportUtils.ts:9888-9889. The sibling CONSISTENCY-16 hit on the hasViolationOfAnyTypeForRBRInLHN JSDoc went in the same commit: src/libs/ReportUtils.ts:9921.
Comment text only, no logic touched. On 7141e8e every check passes except Check independent approval, which is the human-approval gate, not a code failure.
| /** | ||
| * Whether any transaction on the report carries a violation that should drive the LHN RBR, checking all three violation | ||
| * types. Names in `excludedViolationNames` are dropped from every type check, so the exclusion holds regardless of which | ||
| * bucket the back end assigns the violation to; names in `excludedNoticeNames` are dropped from the notice check only. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This JSDoc line uses a semicolon to join two independent clauses ("...so the exclusion holds regardless of which bucket the back end assigns the violation to; names in excludedNoticeNames are dropped from the notice check only."). Per the plain-comment-style guideline, use two separate sentences instead of a semicolon.
Rewrite as two sentences:
* bucket the back end assigns the violation to. Names in `excludedNoticeNames` are dropped from the notice check only.Reviewed at: fc02644 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Correct — same CONSISTENCY-16 issue, a semicolon standing in for a sentence break. Fixed in 7141e8e exactly as suggested: src/libs/ReportUtils.ts:9921.
Comment text only, no logic touched — npm run typecheck passed, ESLint on the file 0 errors, tests/unit/ReportUtilsTest.ts 1311 passed.
CONSISTENCY-16 flagged both the inline comment above excludedViolationNamesForLHN and the hasViolationOfAnyTypeForRBRInLHN JSDoc for joining two independent clauses with a semicolon. Comment text only. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
@chuckdries 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] |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 chuckdries 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! 🧪🧪
|
4 participants
Explanation of Change
After a report is submitted, a
companyCardRequiredviolation still drove the submitter's LHN red dot, the Fix action badge and the Inbox To-do - but clicking Fix opened a report with nothing to act on. Unlike every other Rules > Expenses violation,companyCardRequiredmaps to no editable field (useViolations.tsmaps it tonone), has no Mark as cash and no dismiss, so only an admin turning the rule off can clear it.This PR keeps the violation message visible to everyone on the expense, and stops it from driving the RBR / Fix / To-do for the submitter once the report is submitted.
src/libs/ReportUtils.tsgetViolatingReportIDForRBRInLHNbuilds a newexcludedViolationNamesForLHNlist — [COMPANY_CARD_REQUIRED] on processing reports, empty otherwise — and passes it to both operands of the && that decides the RBR. The pre-existingexcludedNoticeNamesForLHN([MODIFIED_AMOUNT]) is unchanged and still reaches the notice check only.The exclusion is applied by violation name, before the type checks, so it holds no matter which bucket (violation / warning / notice) the back end assigns. To make that possible,
hasNoticeTypeViolationsForRBRInLHNbecame the type-agnostichasViolationOfAnyTypeForRBRInLHN: it takes both exclusion lists and folds the three previously-ORed calls into a single pass over the report transactions.src/libs/Violations/ViolationsUtils.ts
hasVisibleViolationsForUsergains an optionalexcludedViolationNamesparameter defaulting to [], so every existing caller is unaffected. Both operands of the&&above have to judge the same set of violations — without this, the visibility check still sawcompanyCardRequiredand let it vouch for a violation hidden from the submitter (missingCategory while auto-categorization is still running, for example), leaving the red dot lit with nothing behind it.Fixed Issues
$#99793
PROPOSAL:#99793 (comment)
Tests
Preconditions
Offline tests
N/A
QA Steps
Same as tests
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