Fix custom report field disappearing for submitter after report approval#95337
Fix custom report field disappearing for submitter after report approval#95337MelvinBot wants to merge 5 commits into
Conversation
Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
I looked into the failing checks. No code fix is needed — every functional check passes. The only failing check is
This isn't a code issue and pushing a commit won't resolve it — it just needs you (the PR author) to review and check the boxes in the PR description. Once they're filled in, this check will pass. All other checks are green: |
There was a problem hiding this comment.
| !sortedPolicyReportFields.every(shouldHideSingleReportField) |
@MelvinBot can we simplify these conditions?
There was a problem hiding this comment.
(!isCombinedReport || !isOnlyTitleFieldEnabled) && !sortedPolicyReportFields.every(shouldHideSingleReportField);
I mean, we may not need two conditions as a single condition looks to cover both cases(combine or not-combine reports)
There was a problem hiding this comment.
Yes — your suggestion is correct, it's the absorption law. Since isOnlyTitleFieldEnabled is defined as sortedPolicyReportFields.every(shouldHideSingleReportField), the last two clauses of shouldShowReportField are:
(!isCombinedReport || !isOnlyTitleFieldEnabled) && !isOnlyTitleFieldEnabled
which collapses to just !isOnlyTitleFieldEnabled ((A || B) && B === B). So rather than re-inlining the .every(...), I'd reuse the named variable:
| !isOnlyTitleFieldEnabled; |
giving:
const shouldShowReportField =
!isClosedExpenseReportWithNoExpenses &&
(isGroupPolicyExpenseReport || isInvoiceReport) &&
!!policy?.areReportFieldsEnabled &&
!isOnlyTitleFieldEnabled;Bonus — the sibling can be simplified further. In MoneyRequestViewReportFields.tsx, sortedPolicyReportFields is already .filter(field => !shouldHideSingleReportField(field)), so isOnlyTitleFieldEnabled is true only when the array is empty — i.e. it's identical to !sortedPolicyReportFields.length, which is already checked in the early return. Working through the same absorption, the isCombinedReport term and isOnlyTitleFieldEnabled are both redundant, and it reduces to:
const shouldDisplayReportFields = (isGroupPolicyExpenseReport || isInvoiceReport) && !!policy?.areReportFieldsEnabled;
if (!shouldDisplayReportFields || !sortedPolicyReportFields.length) {
return null;
}letting you drop isOnlyTitleFieldEnabled entirely there.
Want me to push these? Reply @MelvinBot implement this and I'll apply both.
There was a problem hiding this comment.
Done — pushed in ff93af8.
MoneyReportView.tsx:shouldShowReportFieldnow ends in!isOnlyTitleFieldEnabled(the(!isCombinedReport || …)clause was absorbed).MoneyRequestViewReportFields.tsx: droppedisOnlyTitleFieldEnabledentirely — sincesortedPolicyReportFieldsis pre-filtered, the empty-list early return already covers it. No caller ever passedisCombinedReport, so I removed that dead prop too.
Verified: oxfmt, eslint (both files), typecheck-tsgo, React Compiler compliance, and tests/ui/MoneyReportViewTest.tsx (13/13 passing).
|
I re-checked — the situation is unchanged. The only failing check is still This check fails because the author checklist boxes in the PR description aren't ticked ("You completed 0 out of 41 checklist items"). It will keep failing on every re-run until you (the PR author) review and check the boxes in the PR description. Every functional check is green: |
|
🤖 Done — PR #95337 is now marked Ready for review. PullerBear will assign the engineering reviewer. A heads-up for the reviewer: the human-only sections of the PR body still have unchecked boxes (the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // and list fields with no enabled option, so if it's true for every field then only the title field remains. | ||
| const isOnlyTitleFieldEnabled = sortedPolicyReportFields.every(shouldHideSingleReportField); | ||
| const isClosedExpenseReportWithNoExpenses = isClosedExpenseReportWithNoExpensesReportUtils(report); | ||
| const isGroupPolicyExpenseReport = isGroupPolicyExpenseReportUtils(report); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
After this change isOnlyTitleFieldEnabled is defined as sortedPolicyReportFields.every(shouldHideSingleReportField), which is exactly the expression still repeated inside shouldShowReportField on the line !sortedPolicyReportFields.every(shouldHideSingleReportField). That term now duplicates the derived isOnlyTitleFieldEnabled — and the same shouldShowReportField expression already includes !isOnlyTitleFieldEnabled, so the .every(...) call recomputes an identical predicate over the same array.
Reuse the derived value instead of recomputing it. Since shouldShowReportField already contains (!isCombinedReport || !isOnlyTitleFieldEnabled), the extra .every(...) term collapses to !isOnlyTitleFieldEnabled and can be dropped:
const isOnlyTitleFieldEnabled = sortedPolicyReportFields.every(shouldHideSingleReportField);
// ...
const shouldShowReportField =
!isClosedExpenseReportWithNoExpenses &&
(isGroupPolicyExpenseReport || isInvoiceReport) &&
!!policy?.areReportFieldsEnabled &&
(!isCombinedReport || !isOnlyTitleFieldEnabled) &&
!isOnlyTitleFieldEnabled;(the last two lines can be merged into a single !isOnlyTitleFieldEnabled).
Reviewed at: 42604eb | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Collapse the redundant report-field-visibility clauses. In MoneyReportView, (!isCombinedReport || !isOnlyTitleFieldEnabled) && !every(shouldHideSingleReportField) reduces to !isOnlyTitleFieldEnabled by absorption. In MoneyRequestViewReportFields, sortedPolicyReportFields is already filtered by shouldHideSingleReportField and isCombinedReport is never passed, so isOnlyTitleFieldEnabled and the unused isCombinedReport prop are dropped; the empty-list early return covers the case. Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-06.at.16.03.59.mov |
| // Whether the only report field configured for this report is the title field. This must be based on which fields are | ||
| // present/displayable, not on which are currently editable: after approval every field becomes read-only for the | ||
| // submitter, but a still-present custom field should keep showing. `shouldHideSingleReportField` hides the title field | ||
| // and list fields with no enabled option, so if it's true for every field then only the title field remains. |
There was a problem hiding this comment.
@MelvinBot remove the comment from here and also from MoneyRequestViewReportFields as I think it is not needed
There was a problem hiding this comment.
Done — removed both comments in 7ddf814.
MoneyReportView.tsx— dropped the comment aboveisOnlyTitleFieldEnabled.MoneyRequestViewReportFields.tsx— dropped the comment aboveshouldDisplayReportFields.
Comment-only change, no logic touched.
Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
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. |
1 similar comment
|
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. |
|
@Pujan92 please complete author checklist |
3 participants
Explanation of Change
A custom report field disappeared for the submitter once a single-expense report was approved.
For a single-expense report,
MoneyReportViewis mounted withisCombinedReport = true, and the whole report-fields block is gated by(!isCombinedReport || !isOnlyTitleFieldEnabled). The problem was thatisOnlyTitleFieldEnabledwas derived fromenabledReportFields, which filtered fields by editability (isReportFieldDisabled) rather than presence:isReportFieldDisabledreturnstruefor every field for a non-admin submitter (theisApprovedbranch), so the custom (non-formula) field was dropped fromenabledReportFields.FORMULAfield, which survives via the|| type === FORMULAclause, soenabledReportFieldscollapsed to just the title →isOnlyTitleFieldEnabledflipped totrue→ the combined-view gate hid the entire section, including the still-present custom field.The field data was never lost — this was purely a display gate that conflated "field is editable" with "field should be shown". A field that merely became read-only after approval should still display.
Fix: compute
isOnlyTitleFieldEnabledfrom field presence using the already-importedshouldHideSingleReportFieldpredicate (which hides the title field and list fields with no enabled option) instead of from editability. This is the same predicate the render loop and the existingshouldShowReportFielddivider check already use, so the section now stays visible with a still-present custom field (shown read-only) after approval, while a workspace whose only configured field is the title still correctly hides the redundant section in the combined view. The same change is applied to the siblingMoneyRequestViewReportFieldsfor consistency. Removing the editability dependency also made the|| type === FORMULAspecial-case and theisReportFieldDisabled/isReportFieldOfTypeTitleimports unnecessary in these components.Added regression tests in
tests/ui/MoneyReportViewTest.tsxthat assert a custom field stays visible for a non-admin submitter on an approved combined report, and that a title-only workspace still hides the section in the combined view.Fixed Issues
$ #93824
PROPOSAL: #93824 (comment)
Tests
Offline tests
Same as tests.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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