Repository navigation
Show list report fields when disabledOptions is shorter than values - #103060
Conversation
Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
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. |
|
@codex review |
|
The failing check is unrelated to this PR, so I didn't push a fix. Please rerun the failed Analysis: Only Evidence: This PR changes only If the rerun fails the same way, the test needs its API call mocked on |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
Eskalifer1
left a comment
There was a problem hiding this comment.
availableListValuesLength still counts only the false entries in disabledOptions, so a field that comes with disabledOptions: [] reports 0 available values here while hasEnabledListValue treats every value as enabled. This is the same check this PR moved to hasEnabledListValue in ReportUtils and FieldsSettingsPage, and this copy now disagrees with them.
| * Checks whether a list report field has at least one enabled value. | ||
| * A value without a matching `disabledOptions` entry is treated as enabled. |
There was a problem hiding this comment.
The comment explains what the function does but not why a missing disabledOptions entry counts as enabled. Fields created outside NewDot come with an empty disabledOptions array, and without that context this looks safe to "simplify" back to disabledOptions.some(...), which would bring the bug back. Let's mention that in the comment.
There was a problem hiding this comment.
Done in 99f828d. The comment now says that fields created outside NewDot can arrive with an empty disabledOptions array. It also says that switching back to disabledOptions.some(...) would hide those fields again.
|
🤖 Web test steps verification results: Verified on dev NewDot web: a List report field "Department" renders on the report when it has an enabled value and disappears once all values are disabled; the JS console could not be read, so that step is unverified. The PR's Tests section is still a TODO, so these steps come from the linked issue's repro plus a fully-disabled regression check; the exact empty
|
|
This PR doesn't cause the What's failing: All 3 tests in Evidence: The same error appears in Jest runs on Next step: Someone needs to fix this test on The |
|
The failing check is still unrelated to this PR, so I didn't push a fix. Analysis: The only real failure is still Evidence: This PR changes only Next step: Rerun |
|
@MelvinBot Resolve this comments: |
|
@MelvinBot please set the PR body to the content in the details section below DetailsExplanation of ChangeList report fields that were not created in NewDot can arrive with an empty This PR adds Unit tests cover an empty Fixed Issues$ #102788 Tests// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
Offline testsN/A QA Steps// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
PR Author Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safari |
…eldsInitialValuePage Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
I fixed both review comments in 99f828d:
Typecheck and the |
|
🤖 I updated the PR body to the content you provided, word for word. The two |
…dsWithEmptyDisabledOptions Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
I fixed the failing check by merging My earlier comment called this flaky. That was wrong. The test was broken on Details
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp103060-android-native.movAndroid: mWeb Chrome103060-android-web.moviOS: HybridApp103060-ios-native.moviOS: mWeb Safari103060-ios-web.movMacOS: Chrome / Safari103060-web.mov |
|
@MelvinBot Mark this PR as ready for review |
|
I couldn't mark this PR as ready for review. My environment blocked the You can do it yourself with the Ready for review button at the bottom of the PR. Before you do, fill in the Tests and QA Steps sections. They still contain the TODO placeholders for the human co-author. |
|
@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] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 luacmartins 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.5.3-0 🚀
|
|
No help site update is needed: this bug fix makes configured list report fields appear on reports as users already expect, and it doesn't make any sentence in |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.3-4 🚀
Bundle Size Analysis (Sentry): |














Explanation of Change
List report fields that were not created in NewDot can arrive with an empty
disabledOptionsarray, even whenvalueshas many entries.shouldHideSingleReportFieldonly looked at the contents ofdisabledOptions, so[].some(...)returnedfalseand the field was hidden. Because the same check drives the report field violation RBR, thefieldRequiredviolation was also hidden, leaving the user with a required field they couldn't see.This PR adds
hasEnabledListValueinReportUtils. It iterates overvaluesand treats a value without a matchingdisabledOptionsentry as enabled, which matches howFieldsListValuesPageandDynamicEditReportFieldPagealready read the array.shouldHideSingleReportFieldandisListFieldEmptyinFieldsSettingsPagenow use it. A field whose values are all explicitly disabled ([true, true, ...]) is still hidden, so the original "hide fully disabled list fields" behavior is unchanged.Unit tests cover an empty
disabledOptions, adisabledOptionsshorter thanvalues, and a fully disabled list.Fixed Issues
$ #102788
PROPOSAL: #102788 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Listtype and add few valuesOffline tests
N/A
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
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