Disabled list-type report fields remain visible in the expense report#76713
Disabled list-type report fields remain visible in the expense report#76713neil-marcellini merged 11 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I think the lint-changed test is irrelevant to this PR. |
melvin-bot
bot
requested review from
heyjennahay and
ikevin127
and removed request for
a team and
heyjennahay
|
@ikevin127 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 fields = mergedFieldIds.map((id) => { | ||
| const field = report?.fieldList?.[getReportFieldKey(id)]; | ||
| const policyReportField = policyReportFields.find(({fieldID}) => fieldID === id); |
There was a problem hiding this comment.
❌ PERF-2 (docs)
This .find() call is now executed for every field in mergedFieldIds, even when field exists and we might not need policyReportField in all cases. Previously, this .find() was only called when field was falsy, which was more efficient.
The expensive .find() operation should be performed after simple property checks or moved inside conditional branches where it's actually needed.
Suggested fix:
const fields = mergedFieldIds.map((id) => {
const field = report?.fieldList?.[getReportFieldKey(id)];
if (field) {
const policyReportField = policyReportFields.find(({fieldID}) => fieldID === id);
return {
...field,
...(policyReportField
? {
disabledOptions: policyReportField.disabledOptions,
values: policyReportField.values,
}
: {}),
};
}
const policyReportField = policyReportFields.find(({fieldID}) => fieldID === id);
if (policyReportField) {
return policyReportField;
}
return null;
});This approach calls .find() separately in each branch where it's needed, which duplicates the call but ensures it only runs when necessary. If the field exists in the report but doesn't have a corresponding policy field, we skip the second lookup entirely.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios-hybrid.moviOS: mWeb Safariios-mweb.movMacOS: Chrome / Safariweb.mov |
|
@mkzie2 🟢 Change looks good and fixed the issue but only for a single-expense report, Note Please add this case in the PR description Tests section to make sure the multiple-expenses case is covered as well 👍 regression.mov♻️ Continuing review, working on completing the checklist. Will approve once the issue described above is addressed and the fix will pass the test. |
|
@ikevin127 I updated. |
|
Failed jest test from main. |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Thanks for addressing the issue and DRY-ing up the code.
The failing test might be resolved after a sync with main.
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Looks good, thank you.
I imagine that changed files lint check is failing because we recently added a new rule for this, and this is the first time somebody has changed this file since then. You could try to figure out who added the rule and whether there is a plan to fix all errors; or it may just be easiest to fix it yourself here.
Also, please add a unit test covering the bug that was fixed here. You can also do that in a follow up, but it will be required to receive payment for the issue.
Looks like this PR has missed adding deprecation notice to here and here. Do you want me to add it or let the PR author handle it instead?
I added unit tests. |
|
Fixed spell check, typecheck fail should be fixed in #77197. |
|
@neil-marcellini Looks like the missing deprecated comments is added on main. Test looks good now. |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Good to go, thanks
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.2.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.78-8 🚀
|
5 participants
Explanation of Change
Fixed Issues
$ #74906
PROPOSAL: #74906 (comment)
Tests
Test 1:
Test 2: Same as test 1, but add 2 expenses to the report.
Offline tests
QA Steps
Test 1:
Test 2: Same as test 1, but add 2 expenses to the report.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Screen.Recording.2025-12-04.at.23.05.10.mov
Android: mWeb Chrome
Screen.Recording.2025-12-04.at.23.06.42.mov
iOS: Native
Screen.Recording.2025-12-04.at.23.07.45.mov
iOS: mWeb Safari
Screen.Recording.2025-12-04.at.23.08.34.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-04.at.23.09.16.mov