fix: allow to select <NULL> in a native filter single mode #19076

diegomedina248 · 2022-03-09T14:09:58Z

SUMMARY

The native filter used in the Dashboard filters values in single mode, which is a valid state.
The filter needs to be made only on undefined specifically.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Fix demo:

Screen.Recording.2022-03-09.at.11.07.53.mov

TESTING INSTRUCTIONS

go to FCC survey dashboard
create value filter for bootcamp name
unselect ' Can select multiple values'
Select <'NULL'> in the bootcamp name

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-03-09T16:30:18Z

Codecov Report

Merging #19076 (93480f9) into master (8d53db1) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head 93480f9 differs from pull request most recent head d796c65. Consider uploading reports for the commit d796c65 to get more accurate results

@@            Coverage Diff             @@
##           master   #19076      +/-   ##
==========================================
- Coverage   66.54%   66.54%   -0.01%     
==========================================
  Files        1646     1646              
  Lines       63630    63630              
  Branches     6475     6476       +1     
==========================================
- Hits        42343    42342       -1     
  Misses      19607    19607              
- Partials     1680     1681       +1

Flag	Coverage Δ
javascript	`51.29% <50.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...c/filters/components/Select/SelectFilterPlugin.tsx	`64.47% <0.00%> (-1.32%)`	⬇️
...et-frontend/src/filters/components/Select/types.ts	`100.00% <ø> (ø)`
superset-frontend/src/components/Select/utils.ts	`52.94% <100.00%> (ø)`

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 8d53db1...d796c65. Read the comment docs.

rusackas

LGTM!

rusackas · 2022-03-09T21:37:11Z

@michael-s-molina @geido could one of you approve this as codeowners?

michael-s-molina · 2022-03-10T10:39:58Z

@michael-s-molina @geido could one of you approve this as codeowners?

@rusackas Before approving this PR we need to discuss how are we going to deal with null values in the Select component. Right now, Select values don't accept a null value as you can see by checking Typescript constraints.

export function getValue(option: string | number | { value: string | number }) {
  - return typeof option === 'object' ? option.value : option;
  + return option && typeof option === 'object' ? option.value : option;
}

This change, for instance, shouldn't be valid because option can only be a string, number or object. The problem is that in FilterSelectPlugin we have:

<Select
   // @ts-ignore
   options={options}
>

I already talked to @geido about this we will schedule a meeting to discuss the best approach.

ktmud

I think it's ok to allow option.value to be of arbitrary type, as long as there is a option.label string for display and search.

ktmud · 2022-03-10T14:47:14Z

superset-frontend/src/components/Select/utils.ts

@@ -61,7 +61,7 @@ export function findValue<OptionType extends OptionTypeBase>(
 }

 export function getValue(option: string | number | { value: string | number }) {
- return typeof option === 'object' ? option.value : option;
+ return option && typeof option === 'object' ? option.value : option;


If we are allowing option to be null then we need to update the signature of getValue as well (add | null).

diegomedina248 · 2022-03-10T16:26:00Z

@michael-s-molina @ktmud yeah, agreed.
Some other libraries store the selectedOptions instead of the value directly, so you will have: {value: null, label: 'x'} instead of plain null, which could lead to errors if treated like undefined

superset-frontend/src/components/Select/utils.ts

michael-s-molina · 2022-03-15T12:52:52Z

@diegomedina248 We discussed null values and we're already supporting it in the Select, so you just need to apply the suggestions to getValue and we're good to go.

michael-s-molina · 2022-03-15T12:59:33Z

superset-frontend/src/components/Select/utils.ts

@@ -61,7 +61,7 @@ export function findValue<OptionType extends OptionTypeBase>(
 }

 export function getValue(option: string | number | { value: string | number }) {
- return typeof option === 'object' ? option.value : option;
+ return option && typeof option === 'object' ? option.value : option;


Suggested change

return option && typeof option === 'object' ? option.value : option;

return typeof option === 'object' ? option.value : option;

typeof option === 'object' returns true when the value is null, that's why the change.
Repl.it: https://replit.com/@DiegoMedina6/BaggyGratefulAdware#index.js

The option shouldn't be null because we need a label to describe the null value. The value of an object option is null.

The getValue is called with the selectValue in which we store the value itself, not the option (for multi select, an array of the values)

@diegomedina248 Please disregard my previous message. To support Ant Design's different behavior depending on whether labelInValue is active, we need to also account for an option being null. You can keep your last commit where getValue(option: string | number | { value: string | number | null } | null)

michael-s-molina

LGTM. Thanks for the fix @diegomedina248!

We currently have a broken frontend test related to timezone issues. People are already looking into it. As soon as they fix it you will need to rebase your PR.

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

sadpandajoe · 2022-03-18T19:13:55Z

🏷️ preset:2022.11

) * fix: allow to select <NULL> in a native filter single mode * fix lint issue * Update superset-frontend/src/components/Select/utils.ts Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * fix Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> (cherry picked from commit 19fcd03)

* fix: allow to select <NULL> in a native filter single mode * fix lint issue * Update superset-frontend/src/components/Select/utils.ts Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * fix Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> (cherry picked from commit 19fcd03)

diegomedina248 requested review from michael-s-molina and geido as code owners March 9, 2022 14:09

pull-request-size bot added the size/M label Mar 9, 2022

rusackas approved these changes Mar 9, 2022

View reviewed changes

michael-s-molina mentioned this pull request Mar 10, 2022

fix(native-filters): Error when selecting filter value "null" #19096

Closed

9 tasks

ktmud reviewed Mar 10, 2022

View reviewed changes

michael-s-molina reviewed Mar 15, 2022

View reviewed changes

superset-frontend/src/components/Select/utils.ts Outdated Show resolved Hide resolved

michael-s-molina reviewed Mar 15, 2022

View reviewed changes

michael-s-molina approved these changes Mar 15, 2022

View reviewed changes

diegomedina248 force-pushed the fix/native-filter-allow-to-select-null-in-single-mode branch from 8546d83 to 7a324df Compare March 15, 2022 16:20

diegomedina248 and others added 4 commits March 16, 2022 19:52

fix: allow to select <NULL> in a native filter single mode

2b667e7

fix lint issue

5178f64

Update superset-frontend/src/components/Select/utils.ts

ddeb275

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

fix

d796c65

diegomedina248 force-pushed the fix/native-filter-allow-to-select-null-in-single-mode branch from 7a324df to d796c65 Compare March 16, 2022 22:52

michael-s-molina merged commit 19fcd03 into apache:master Mar 17, 2022

superset-github-bot bot added the preset:2022.11 label Mar 18, 2022

villebro added the lts-v1 label Mar 29, 2022

jinghua-qa mentioned this pull request Mar 30, 2022

[Native Filter] User can not select <'NULL'> value in single select mode #19000

Closed

3 tasks

mistercrunch added 🍒 1.5.0 🍒 1.5.1 labels Mar 13, 2024

mistercrunch added 🍒 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow to select <NULL> in a native filter single mode #19076

fix: allow to select <NULL> in a native filter single mode #19076

diegomedina248 commented Mar 9, 2022 •

edited

Loading

codecov bot commented Mar 9, 2022 •

edited

Loading

rusackas left a comment

rusackas commented Mar 9, 2022

michael-s-molina commented Mar 10, 2022

ktmud left a comment

ktmud Mar 10, 2022

diegomedina248 commented Mar 10, 2022

michael-s-molina commented Mar 15, 2022

michael-s-molina Mar 15, 2022

diegomedina248 Mar 15, 2022

michael-s-molina Mar 15, 2022 •

edited

Loading

diegomedina248 Mar 15, 2022

michael-s-molina Mar 15, 2022 •

edited

Loading

michael-s-molina left a comment •

edited

Loading

sadpandajoe commented Mar 18, 2022

	return option && typeof option === 'object' ? option.value : option;
	return typeof option === 'object' ? option.value : option;

fix: allow to select <NULL> in a native filter single mode #19076

fix: allow to select <NULL> in a native filter single mode #19076

Conversation

diegomedina248 commented Mar 9, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 9, 2022 • edited Loading

Codecov Report

rusackas left a comment

Choose a reason for hiding this comment

rusackas commented Mar 9, 2022

michael-s-molina commented Mar 10, 2022

ktmud left a comment

Choose a reason for hiding this comment

ktmud Mar 10, 2022

Choose a reason for hiding this comment

diegomedina248 commented Mar 10, 2022

michael-s-molina commented Mar 15, 2022

michael-s-molina Mar 15, 2022

Choose a reason for hiding this comment

diegomedina248 Mar 15, 2022

Choose a reason for hiding this comment

michael-s-molina Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

diegomedina248 Mar 15, 2022

Choose a reason for hiding this comment

michael-s-molina Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

michael-s-molina left a comment • edited Loading

Choose a reason for hiding this comment

sadpandajoe commented Mar 18, 2022

diegomedina248 commented Mar 9, 2022 •

edited

Loading

codecov bot commented Mar 9, 2022 •

edited

Loading

michael-s-molina Mar 15, 2022 •

edited

Loading

michael-s-molina Mar 15, 2022 •

edited

Loading

michael-s-molina left a comment •

edited

Loading