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: Verify when null value should be undefined in Select #17013

Merged
merged 3 commits into from
Oct 8, 2021
Merged

fix: Verify when null value should be undefined in Select #17013

merged 3 commits into from
Oct 8, 2021

Conversation

geido
Copy link
Member

@geido geido commented Oct 7, 2021

SUMMARY

This PR fixes an edge case with the Select component. With dynamic controls there might be the case where a default value is being passed as null when it was intended to be undefined. This change makes sure that a null value that is not available in the options is considered asundefined instead.

BEFORE

Screen Shot 2021-10-07 at 17 35 15

AFTER

Screen Shot 2021-10-07 at 17 33 45

TESTING INSTRUCTIONS

  1. Create a Histogram chart
  2. Observe the Columns control

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

@geido
Copy link
Member Author

geido commented Oct 7, 2021

@michael-s-molina for clarity, this is just a safety check for now. Ideally, we should go through each configuration object of the SelectControl in superset and superset-ui and remove the default value when it's null but it's meant to be undefined. I can do that in separate PRs.

@michael-s-molina
Copy link
Member

michael-s-molina commented Oct 8, 2021

@michael-s-molina for clarity, this is just a safety check for now. Ideally, we should go through each configuration object of the SelectControl in superset and superset-ui and remove the default value when it's null but it's meant to be undefined. I can do that in separate PRs.

@geido Can't this check/conversion be made in SelectControl?

@geido
Copy link
Member Author

geido commented Oct 8, 2021

For reference, we have agreed with Michael that this safety check will be done in SelectControl and SelectAsyncControl as those are the components where this problem might happen more often. I am about to update this PR.

@pull-request-size pull-request-size bot added size/M and removed size/XS labels Oct 8, 2021
@geido
Copy link
Member Author

geido commented Oct 8, 2021

@michael-s-molina I just updated the PR. Please have a look

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #17013 (3be175a) into master (9e980b6) will decrease coverage by 0.00%.
The diff coverage is 76.92%.

❗ Current head 3be175a differs from pull request most recent head a5aabfb. Consider uploading reports for the commit a5aabfb to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17013      +/-   ##
==========================================
- Coverage   76.94%   76.93%   -0.01%     
==========================================
  Files        1030     1030              
  Lines       55088    55098      +10     
  Branches     7480     7484       +4     
==========================================
+ Hits        42385    42392       +7     
- Misses      12452    12455       +3     
  Partials      251      251              
Flag Coverage Δ
javascript 70.90% <76.92%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
.../src/explore/components/controls/SelectControl.jsx 77.61% <75.00%> (-1.08%) ⬇️
...e/components/controls/SelectAsyncControl/index.tsx 80.48% <80.00%> (-0.60%) ⬇️

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 9e980b6...a5aabfb. Read the comment docs.

@geido geido merged commit a57ae35 into apache:master Oct 8, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* Check for null value

* Safety chek SelectControl and SelectAsyncControl
@AAfghahi
Copy link
Member

🏷 v1.4

eschutho pushed a commit that referenced this pull request Nov 22, 2021
* Check for null value

* Safety chek SelectControl and SelectAsyncControl
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* Check for null value

* Safety chek SelectControl and SelectAsyncControl
@mistercrunch mistercrunch added 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.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 size/M v1.4 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants