Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(select): render when empty multiselect #19612

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

villebro
Copy link
Member

@villebro villebro commented Apr 8, 2022

SUMMARY

PR #19341 introduced a regression that caused the adhoc filter value selector to disappear if a multioperator (IN, NOT IN) was chosen and there were no options. It is typical for the options to be missing if the "populate autocomplete filters options" option has been disabled in the dataset properties. I believe the reason for the bug that #19341 attempted to address was due to only checking if a selected item was typeof 'object'. This is true for null, which caused the logic to assume the value was in fact a LabeledValue (=object with keys value and label). Here we introduce a typeguard to make the checking even more robust, and remove the logic that delays rendering of SelectWithLabel.

I checked that the original issue that #19341 set out to fix still works after this change. However, deselecting a NULL value doesn't seem to work properly if there are other falsy values (for this we'll need to use LabeledValue and use a custom key as do make sure each entry is unique). However, I consider this a separate issue which should be addressed in the native filter, so will address that in a separate PR.

AFTER

Now the dropdown renders correctly:

filter-after.mp4

BEFORE

Previously the dropdown disappeared if there were no options:

filter-before2.mp4

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Disable "populate autocomplete filters options" in a dataset
  2. Explore the dataset and add an adhoc filter
  3. choose any column and choose the "IN" operator
  4. see the dropdown disappear

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #19612 (0d8d080) into master (761d5c4) will decrease coverage by 0.15%.
The diff coverage is 87.50%.

❗ Current head 0d8d080 differs from pull request most recent head c16c534. Consider uploading reports for the commit c16c534 to get more accurate results

@@            Coverage Diff             @@
##           master   #19612      +/-   ##
==========================================
- Coverage   66.68%   66.53%   -0.16%     
==========================================
  Files        1681     1681              
  Lines       64316    64319       +3     
  Branches     6565     6564       -1     
==========================================
- Hits        42888    42793      -95     
- Misses      19733    19853     +120     
+ Partials     1695     1673      -22     
Flag Coverage Δ
javascript 51.21% <87.50%> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx 65.60% <ø> (+0.52%) ⬆️
superset-frontend/src/components/Select/Select.tsx 87.28% <83.33%> (+0.36%) ⬆️
superset-frontend/src/components/Select/utils.ts 65.00% <100.00%> (+1.84%) ⬆️
...ts/nativeFilters/FilterBar/FilterControls/state.ts 0.00% <0.00%> (-75.00%) ⬇️
...veFilters/FilterBar/FilterControls/FilterValue.tsx 6.59% <0.00%> (-53.85%) ⬇️
...tend/src/filters/components/Time/transformProps.ts 0.00% <0.00%> (-50.00%) ⬇️
...oard/components/nativeFilters/FilterCard/index.tsx 10.00% <0.00%> (-50.00%) ⬇️
...d/src/filters/components/Time/TimeFilterPlugin.tsx 0.00% <0.00%> (-47.06%) ⬇️
...Filters/FilterBar/FilterControls/FilterControl.tsx 29.03% <0.00%> (-41.94%) ⬇️
...rset-frontend/src/filters/components/Time/index.ts 66.66% <0.00%> (-33.34%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 761d5c4...c16c534. Read the comment docs.

@villebro villebro merged commit 1ad82af into apache:master Apr 8, 2022
@villebro villebro deleted the villebro/fix-empty-select branch April 8, 2022 13:40
villebro added a commit that referenced this pull request Apr 8, 2022
* fix(select): render when empty multiselect

* disable flaky test

(cherry picked from commit 1ad82af)
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* fix(select): render when empty multiselect

* disable flaky test
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 preset-io size/M 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
4 participants