feat(sql lab): Save Dataset Modal Autocomplete should display list when overwritting #20512

Antonio-RiveroMartnez · 2022-06-27T20:42:12Z

SUMMARY

Use our Select component as substitute of the Autocomplete one so options are loaded without the user having to trigger a search and also we are more consistent with the rest of the app
Changing datasetId to lowercase so when custom props get into the DOM we don't get warning related to invalid formatting
We extracted the dropdown out of the radio because it causes invalid click handling when an option is selected
Add getPopupContainer as prop for Select component

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

In SQL Editor, enter valid query
Result tab should show
Click on Explore in Result tab
Select overwrite existing and save & explore
User should be able to see a list of virtual datasets to choose from to overwrite without typing

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

superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx

stephenLYZ

Thanks for the fix! Left some comments

superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx

stephenLYZ · 2022-06-28T15:20:29Z

superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx

@@ -139,7 +143,7 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
  const handleOverwriteDataset = async () => {
    await updateDataset(
      query.dbId,
-      datasetToOverwrite.datasetId,


Lower camel case naming makes sense, curious about the reason for the change here is.

This custom prop gets into the DOM so react throws a warning for invalid format. That's why it was changed to all lowercase.

diegomedina248 · 2022-06-28T18:04:28Z

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

+   * Customize who the container node is.
+   * Parent node by default.
+   */
+  getPopupContainer?: (triggerNode?: any) => any;


I think the return should be HTMLElement

Yeah, I had it like that, but SelectFilterPlugin is also customizing such prop and I didn't want to impact other non-related component.

codecov · 2022-06-29T17:13:24Z

Codecov Report

Merging #20512 (c0c78a5) into master (7f70de3) will increase coverage by 0.00%.
The diff coverage is 26.66%.

@@           Coverage Diff           @@
##           master   #20512   +/-   ##
=======================================
  Coverage   66.79%   66.79%           
=======================================
  Files        1754     1754           
  Lines       65561    65557    -4     
  Branches     6933     6933           
=======================================
- Hits        43793    43791    -2     
+ Misses      20014    20011    -3     
- Partials     1754     1755    +1

Flag	Coverage Δ
javascript	`51.82% <26.66%> (+<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	`62.82% <0.00%> (-1.29%)`	⬇️
...d/src/SqlLab/components/SaveDatasetModal/index.tsx	`34.61% <25.00%> (+0.46%)`	⬆️
superset-frontend/src/components/Select/Select.tsx	`87.44% <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 7f70de3...c0c78a5. Read the comment docs.

rusackas

LGTM

rusackas · 2022-06-30T21:26:06Z

Just needs a rebase, and I'll happily merge :)

jinghua-qa · 2022-07-01T16:57:53Z

/testenv up

github-actions · 2022-07-01T16:59:45Z

@jinghua-qa Ephemeral environment spinning up at http://54.212.127.246:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

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

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

- Use our Select component as substitute of the Autocomplete one so options are loaded initially without the user having to trigger a search and we are mosre consistent with the rest of the app - Changing datasetId to lowercase so when custom props get into the DOM we don't get warning related to invalid formatting - We extracted the dropdown out of the radio because it causes invalid click handling when an option is selected - Updated tests

- Update missing test for DatasourceControl

- Remove conditional from load options function since only guest users dont have userId, and if that is the case they wont reach this part of the application

- Remove unused comment

- Add getPopupContainer as prop for Select component

- Add tests for our new getPopupContainer prop in Select component. So if passed it gets called.

- use lowercased property when calling post form data

- Update tests so there is no need to define a null returning func

- Including getPopupContainer from PickedSelectProps instead - Updating definition in SelectFilterPlugin

github-actions · 2022-07-01T20:40:50Z

Ephemeral environment shutdown and build artifacts deleted.

…list when overwritting (apache#20512)" This reverts commit 8a57a71.

…n overwritting (apache#20512) * Save Dataset Modal: - Use our Select component as substitute of the Autocomplete one so options are loaded initially without the user having to trigger a search and we are mosre consistent with the rest of the app - Changing datasetId to lowercase so when custom props get into the DOM we don't get warning related to invalid formatting - We extracted the dropdown out of the radio because it causes invalid click handling when an option is selected - Updated tests * Save Dataset Modal: - Update missing test for DatasourceControl * Save Dataset Modal: - Remove conditional from load options function since only guest users dont have userId, and if that is the case they wont reach this part of the application * Save Dataset Modal: - Remove unused comment * Save Dataset Modal: - Add getPopupContainer as prop for Select component * Save Dataset Modal: - Add tests for our new getPopupContainer prop in Select component. So if passed it gets called. * Save Dataset Modal: - use lowercased property when calling post form data * Save Dataset Modal: - Update tests so there is no need to define a null returning func * Save Dataset Modal: - Including getPopupContainer from PickedSelectProps instead - Updating definition in SelectFilterPlugin

pull-request-size bot added the size/L label Jun 27, 2022

michael-s-molina reviewed Jun 27, 2022

View reviewed changes

superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx Show resolved Hide resolved

michael-s-molina reviewed Jun 27, 2022

View reviewed changes

superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx Outdated Show resolved Hide resolved

Antonio-RiveroMartnez force-pushed the FIX-50967 branch from d630d76 to 40923fb Compare June 28, 2022 09:00

stephenLYZ reviewed Jun 28, 2022

View reviewed changes

Antonio-RiveroMartnez force-pushed the FIX-50967 branch from 40923fb to c91a92b Compare June 28, 2022 15:42

Antonio-RiveroMartnez requested review from geido and ktmud as code owners June 28, 2022 17:39

diegomedina248 reviewed Jun 28, 2022

View reviewed changes

diegomedina248 approved these changes Jun 28, 2022

View reviewed changes

Antonio-RiveroMartnez force-pushed the FIX-50967 branch from f92a68e to 2be6dc1 Compare June 28, 2022 18:40

Antonio-RiveroMartnez force-pushed the FIX-50967 branch from 2be6dc1 to bfddc93 Compare June 30, 2022 10:59

rusackas approved these changes Jun 30, 2022

View reviewed changes

Antonio-RiveroMartnez force-pushed the FIX-50967 branch 2 times, most recently from 3f99091 to 82351b9 Compare July 1, 2022 11:31

michael-s-molina requested changes Jul 1, 2022

View reviewed changes

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

michael-s-molina reviewed Jul 1, 2022

View reviewed changes

superset-frontend/src/components/Select/Select.test.tsx Outdated Show resolved Hide resolved

michael-s-molina reviewed Jul 1, 2022

View reviewed changes

superset-frontend/src/components/Select/Select.test.tsx Outdated Show resolved Hide resolved

Antonio-RiveroMartnez added 8 commits July 1, 2022 15:41

Save Dataset Modal:

4072789

- Update missing test for DatasourceControl

Save Dataset Modal:

d065536

- Remove conditional from load options function since only guest users dont have userId, and if that is the case they wont reach this part of the application

Save Dataset Modal:

f590149

- Remove unused comment

Save Dataset Modal:

24a631d

- Add getPopupContainer as prop for Select component

Save Dataset Modal:

e80f4b4

- Add tests for our new getPopupContainer prop in Select component. So if passed it gets called.

Save Dataset Modal:

77722b1

- use lowercased property when calling post form data

Save Dataset Modal:

ada7231

- Update tests so there is no need to define a null returning func

Antonio-RiveroMartnez force-pushed the FIX-50967 branch from 82351b9 to ada7231 Compare July 1, 2022 18:41

Save Dataset Modal:

c0c78a5

- Including getPopupContainer from PickedSelectProps instead - Updating definition in SelectFilterPlugin

michael-s-molina approved these changes Jul 1, 2022

View reviewed changes

rusackas merged commit 8a57a71 into apache:master Jul 1, 2022

michael-s-molina changed the title ~~fix(sql lab): Save Dataset Modal Autocomplete should display list when overwritting~~ feat(sql lab): Save Dataset Modal Autocomplete should display list when overwritting Jul 6, 2022

Antonio-RiveroMartnez deleted the FIX-50967 branch July 8, 2022 12:19

jinghua-qa added a commit to preset-io/superset that referenced this pull request Jul 8, 2022

Revert "fix(sql lab): Save Dataset Modal Autocomplete should display …

8b0d28d

…list when overwritting (apache#20512)" This reverts commit 8a57a71.

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sql lab): Save Dataset Modal Autocomplete should display list when overwritting #20512

feat(sql lab): Save Dataset Modal Autocomplete should display list when overwritting #20512

Antonio-RiveroMartnez commented Jun 27, 2022 •

edited

Loading

stephenLYZ left a comment

stephenLYZ Jun 28, 2022

Antonio-RiveroMartnez Jun 28, 2022

diegomedina248 Jun 28, 2022

Antonio-RiveroMartnez Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 29, 2022 •

edited

Loading

rusackas left a comment

rusackas commented Jun 30, 2022

jinghua-qa commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

feat(sql lab): Save Dataset Modal Autocomplete should display list when overwritting #20512

feat(sql lab): Save Dataset Modal Autocomplete should display list when overwritting #20512

Conversation

Antonio-RiveroMartnez commented Jun 27, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

stephenLYZ left a comment

Choose a reason for hiding this comment

stephenLYZ Jun 28, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jun 28, 2022

Choose a reason for hiding this comment

diegomedina248 Jun 28, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 29, 2022 • edited Loading

Codecov Report

rusackas left a comment

Choose a reason for hiding this comment

rusackas commented Jun 30, 2022

jinghua-qa commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

Antonio-RiveroMartnez commented Jun 27, 2022 •

edited

Loading

Antonio-RiveroMartnez Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 29, 2022 •

edited

Loading