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.

Sign up for GitHub

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

Jump to bottom

fix: Don't allow duplicated tag values in the Select #19283

Merged

michael-s-molina merged 2 commits into apache:master from michael-s-molina:fix-duplicated-tags-select

Mar 22, 2022

Member

michael-s-molina commented Mar 21, 2022

SUMMARY

This PR fixes two bugs in the Select component:

Duplicated tags were allowed
When pasting multiple values, only the last value was being added

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-03-21.at.11.19.43.AM.mov

Screen.Recording.2022-03-21.at.11.16.48.AM.mov

TESTING INSTRUCTIONS

Check the videos for instructions.

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


          fix: Don't allow duplicated tag values in the Select

323f43f

michael-s-molina requested review from ktmud and rusackas

March 21, 2022 14:26

michael-s-molina requested a review from geido as a code owner

March 21, 2022 14:26

pull-request-size bot added the size/M label

codecov bot commented Mar 21, 2022 •

edited

Loading

Codecov Report

Merging #19283 (05602ca) into master (783168e) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master   #19283   +/-   ##
=======================================
  Coverage   66.57%   66.57%           
=======================================
  Files        1667     1667           
  Lines       64418    64429   +11     
  Branches     6503     6503           
=======================================
+ Hits        42886    42896   +10     
  Misses      19849    19849           
- Partials     1683     1684    +1

Flag	Coverage Δ
javascript	`51.37% <80.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/components/Select/Select.tsx	`86.91% <80.00%> (+0.53%)`	⬆️
...nd/src/components/MessageToasts/ToastPresenter.tsx	`93.33% <0.00%> (-6.67%)`	⬇️
superset-frontend/src/embedded/index.tsx	`0.00% <0.00%> (ø)`
...ontend/src/components/MessageToasts/withToasts.tsx	`100.00% <0.00%> (ø)`
...rset-ui-core/src/connection/SupersetClientClass.ts	`100.00% <0.00%> (ø)`
...nd/src/components/MessageToasts/ToastContainer.jsx
...nd/src/components/MessageToasts/ToastContainer.tsx	`100.00% <0.00%> (ø)`
superset-frontend/src/views/components/Menu.tsx	`60.00% <0.00%> (+1.02%)`	⬆️

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 783168e...05602ca. Read the comment docs.

ktmud reviewed

View reviewed changes

Member

ktmud left a comment

Thanks for fixing this! Added a couple of comments.

superset-frontend/src/components/Select/Select.tsx Outdated

+                const valueAsArray = <T,>(value: any): T[] => {
+                  const array = value ? (Array.isArray(value) ? value : [value]) : [];
+                  return array as T[];
+                };

Member

ktmud Mar 21, 2022

There is a ensureIsArray function in @superset-ui/core

superset-frontend/src/components/Select/Select.tsx Outdated

+                    setSelectValue(previousState => {
+                      const primitiveArray = valueAsArray<string | number>(previousState);
+                      // Tokenized values can contain duplicated values
+                      if (!primitiveArray.some(value => value === selectedValue)) {

Member

ktmud Mar 21, 2022

Suggested change

      
                    if (!primitiveArray.some(value => value === selectedValue)) {
          
                    if (!primitiveArray.includes(selectedValue)) {

superset-frontend/src/components/Select/Select.tsx Outdated

+                    setSelectValue(previousState => {
+                      const objectArray = valueAsArray<AntdLabeledValue>(previousState);
+                      // Tokenized values can contain duplicated values
+                      if (!objectArray.some(object => object.value === selectedValue.label)) {

Member

ktmud Mar 21, 2022

Why are we comparing value to labels?

Member Author

michael-s-molina Mar 21, 2022

🙈 Lack of coffee

superset-frontend/src/components/Select/Select.tsx

+                  const array = value ? (Array.isArray(value) ? value : [value]) : [];
+                  return array as T[];
+                };
                 const handleOnSelect = (
                   selectedValue: string | number | AntdLabeledValue,

Member

ktmud Mar 21, 2022

Can we rename this local selectedValue to selectedItem so not to confuse with selectValue in the parent context which may also be an array?

superset-frontend/src/components/Select/Select.tsx Outdated

+                  } else if (
+                    typeof selectedValue === 'number' ||
+                    typeof selectedValue === 'string'
+                  ) {

Member

ktmud Mar 21, 2022

I believe these two if/else can be rewritten with hasOption in Select/utils.


          Addresses comments and adds test

05602ca

Member Author

michael-s-molina commented Mar 21, 2022

@ktmud I addressed all your comments. Thanks for the tips, they were valuable! I also added a test.

ktmud approved these changes

View reviewed changes

Member

ktmud left a comment

LGTM!

superset-frontend/src/components/Select/Select.test.tsx

+                expect(values[0]).toHaveTextContent('a');
+                expect(values[1]).toHaveTextContent('b');
+                expect(values[2]).toHaveTextContent('c');
+                expect(values[3]).toHaveTextContent('d');

Member

ktmud Mar 21, 2022

Nice!

geido approved these changes

View reviewed changes

Member

geido left a comment

LGTM!

michael-s-molina merged commit d3ce398 into apache:master

diegomedina248 mentioned this pull request

fix: regression on Select component when handling null values #19326

Merged

9 tasks

michael-hoffman-26 pushed a commit to nielsen-oss/superset that referenced this pull request


          fix: Don't allow duplicated tag values in the Select (apache#19283)

c5b61e8

* fix: Don't allow duplicated tag values in the Select

* Addresses comments and adds test

villebro added the lts-v1 label

villebro pushed a commit that referenced this pull request


          fix: Don't allow duplicated tag values in the Select (#19283)

46dbf6c

* fix: Don't allow duplicated tag values in the Select

* Addresses comments and adds test

(cherry picked from commit d3ce398)

mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot 🚢 2.0.0 labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot lts-v1 size/M 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0