chore: Changes the DatabaseSelector and TableSelector to use the new Select component #16334

michael-s-molina · 2021-08-18T20:34:57Z

SUMMARY

Changes the DatabaseSelector and TableSelector to use the new Select component

Make the selects more similar to the rest of the application with the labels on top and no tags
Adds pagination and lazy loading to the database select
Adds lazy loading to the schemas select
Adds lazy loading to the tables select and sort the options
Removes the number of items in a select due to lazy loading
Improves the layout of the virtual dataset
Migrates the table and view icons to the new Icons component
Adjusts the size of the table, view, certification, and warning icons
Cancels the import of icons when unmounting
Adjusts the tests

@junlincc @rusackas @villebro @jinghua-qa @rosemarie-chiu

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-08-18.at.5.13.22.PM.mov

Screen.Recording.2021-08-18.at.5.09.49.PM.mov

PS: There's an error that is also happening on master when we try to change the table of a saved dataset. It raises "An error has occurred". On the server logs, we can see a 401. @villebro This is not related to this PR but if it's something simple we can add the fix here.

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

codecov · 2021-08-19T15:04:17Z

Codecov Report

Merging #16334 (00b99b5) into master (3faf653) will decrease coverage by 0.01%.
The diff coverage is 58.88%.

@@            Coverage Diff             @@
##           master   #16334      +/-   ##
==========================================
- Coverage   76.57%   76.55%   -0.02%     
==========================================
  Files        1000     1000              
  Lines       53489    53479      -10     
  Branches     6816     6816              
==========================================
- Hits        40957    40943      -14     
- Misses      12296    12298       +2     
- Partials      236      238       +2

Flag	Coverage Δ
hive	`81.22% <27.27%> (-0.02%)`	⬇️
javascript	`70.69% <56.98%> (-0.19%)`	⬇️
mysql	`81.52% <90.90%> (+0.04%)`	⬆️
postgres	`81.51% <90.90%> (-0.04%)`	⬇️
presto	`81.37% <27.27%> (?)`
python	`82.06% <90.90%> (+0.14%)`	⬆️
sqlite	`81.19% <90.90%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx	`70.05% <ø> (ø)`
superset-frontend/src/components/Select/Select.tsx	`76.57% <25.00%> (+3.15%)`	⬆️
...erset-frontend/src/datasource/DatasourceEditor.jsx	`74.81% <50.00%> (ø)`
...et-frontend/src/components/TableSelector/index.tsx	`61.29% <53.48%> (-22.97%)`	⬇️
...frontend/src/components/DatabaseSelector/index.tsx	`66.66% <69.81%> (-26.05%)`	⬇️
superset/views/core.py	`75.30% <90.00%> (+0.05%)`	⬆️
...et-frontend/src/components/CertifiedIcon/index.tsx	`100.00% <100.00%> (ø)`
superset-frontend/src/components/Icons/Icon.tsx	`100.00% <100.00%> (ø)`
...nd/src/components/WarningIconWithTooltip/index.tsx	`100.00% <100.00%> (ø)`
...re/components/controls/DatasourceControl/index.jsx	`79.06% <100.00%> (ø)`
... and 8 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 3faf653...00b99b5. Read the comment docs.

villebro · 2021-08-20T08:56:43Z

PS: There's an error that is also happening on master when we try to change the table of a saved dataset. It raises "An error has occurred". On the server logs, we can see a 401. @villebro This is not related to this PR but if it's something simple we can add the fix here.

This now fixed on master as of #16369

villebro

Great improvement! One last problem related to always force refreshing the data from the db, other than that looks great!

villebro · 2021-08-20T09:02:32Z

superset-frontend/src/components/DatabaseSelector/index.tsx

+        // The endpoint currently does not support pagination
+        return SupersetClient.get({ endpoint }).then(({ json }) => {


Could potentially change this to a TODO: if you have to push commits to the PR (would be nice to add pagination in a follow-up)

villebro · 2021-08-20T09:17:33Z

superset-frontend/src/components/TableSelector/index.tsx

+        const encodedSchema = encodeURIComponent(dbSchema);
+        const encodedSubstr = encodeURIComponent(search || 'undefined');
+        const endpoint = encodeURI(
+          `/superset/tables/${currentDbId}/${encodedSchema}/${encodedSubstr}/true/`,


This should also default to not force updating the metadata from the db

Suggested change

`/superset/tables/${currentDbId}/${encodedSchema}/${encodedSubstr}/true/`,

`/superset/tables/${currentDbId}/${encodedSchema}/${encodedSubstr}/false/`,

villebro · 2021-08-20T09:30:01Z

superset-frontend/src/components/TableSelector/index.tsx

    }
  }

-  function changeSchema(schemaOpt: any, force = false) {
-    const value = schemaOpt ? schemaOpt.value : null;
+  function onRefresh() {


I think we need to add a force param here that the force refresh calls. Something like

function onRefresh(forceRefresh: boolean) {

villebro · 2021-08-20T09:32:14Z

superset-frontend/src/components/TableSelector/index.tsx

+  const loadTable = useMemo(
+    () => async (dbId: number, schema: string, tableName: string) => {
      const endpoint = encodeURI(
-        `/superset/tables/${actualDbId}/${encodedSchema}/${encodedSubstr}/${!!forceRefresh}/`,
+        `/superset/tables/${dbId}/${schema}/${encodeURIComponent(
+          tableName,
+        )}/true/true`,
      );


This will force table metadata to be refetched from the db as opposed to the cache. We should probably retain the forceRefresh variable and pass it along here, like

const loadTable = useMemo( () => async (dbId: number, schema: string, tableName: string, forceRefresh: boolean) => { const endpoint = encodeURI( `/superset/tables/${dbId}/${schema}/${encodeURIComponent( tableName, )}/${forceRefresh}/true`, );```

michael-s-molina · 2021-08-21T16:31:27Z

@villebro I changed the loadTable (singular) to run only once with forceRefresh always false. Then I added a forceRefresh parameter to loadTables (plural) that will only be true when the user clicks on the refresh button.

michael-s-molina · 2021-08-22T16:14:38Z

/testenv up

github-actions · 2021-08-22T16:16:40Z

@michael-s-molina Ephemeral environment spinning up at http://52.32.213.248:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro

LGTM, thanks for all the hard work here!

github-actions · 2021-08-23T18:41:23Z

Ephemeral environment shutdown and build artifacts deleted.

* master: (582 commits) feat: config to customize bootstrap data overrides (apache#16386) fix: regex for multi-region IPs (apache#16410) chore: Changes the DatabaseSelector to use the new Select component (apache#16334) chore: Displays the dataset description in a tooltip in the datasets list (apache#16392) fix(pylint): Fix master (apache#16405) chore(pylint): Enable useless-suppression check (apache#16388) feat: Add extraVolumes and extraVolumeMounts to all main containers (apache#16361) initial commit (apache#16366) fix: big number default date format (apache#16383) initial commit (apache#16380) fix: import dashboard w/o metadata (apache#16360) test: Functional RTL for email report modal II (apache#16148) fix: update table ID in query context on chart import (apache#16374) docs: document FLASK_APP_MUTATOR (apache#16286) feat: Add new dev commands to Makefile (apache#16327) fix: call external metadata endpoint with correct rison object (apache#16369) fix: Fix parsing onSaving reports toast when user hasn't saved chart (apache#16330) fix: columns/index rebuild (apache#16355) chore(viz): bump deckgl plugin to 0.4.11 (apache#16353) fix: Blank space in Change dataset modal without warning message (apache#16324) ... # Conflicts: # superset/app.py # superset/models/dashboard.py # tests/integration_tests/charts/filter_sets/__init__.py # tests/integration_tests/charts/filter_sets/conftest.py # tests/integration_tests/charts/filter_sets/consts.py # tests/integration_tests/charts/filter_sets/create_api_tests.py # tests/integration_tests/charts/filter_sets/delete_api_tests.py # tests/integration_tests/charts/filter_sets/get_api_tests.py # tests/integration_tests/charts/filter_sets/update_api_tests.py # tests/integration_tests/charts/filter_sets/utils.py # tests/integration_tests/superset_test_config.py

AAfghahi · 2021-08-26T18:53:18Z

superset-frontend/src/components/TableSelector/index.tsx

+          database_name: string;
+          backend: string;
+        }
+      | undefined,


Hey Michael, why is this set to undefined?

Oh, I think it's just a leftover from the whole refactoring. I touched so many files and this was probably a loose end.

ok, sounds good.

…ponent (#16334)" This reverts commit c768941.

…ponent (#16334)" (#16478) This reverts commit c768941.

* upstream/master: fix: create example DB if needed (apache#16451) fix(native-filters): add handle undefined control value gracefully (apache#16468) Revert "chore: Changes the DatabaseSelector to use the new Select component (apache#16334)" (apache#16478) fix(explore): JS error for creating new metrics from columns (apache#16477) fix: queryEditor bug (apache#16452) docs: make code snippet usable with required imports (apache#16473) perf(dashboard): decouple redux props from dashboard components (apache#16421) perf(dashboard): reduce number of rerenders of Charts (apache#16444)

…ponent (apache#16334)" (apache#16478) This reverts commit c768941. (cherry picked from commit 8adc31d)

…pache#16334)

…ponent (apache#16334)" (apache#16478) This reverts commit c768941.

…pache#16334)

…ponent (apache#16334)" (apache#16478) This reverts commit c768941.

michael-s-molina requested review from rusackas, pkdotson, villebro and geido August 18, 2021 20:34

pull-request-size bot added the size/XXL label Aug 18, 2021

michael-s-molina force-pushed the change-database-selector-select branch from 82d5770 to f72be89 Compare August 19, 2021 13:15

michael-s-molina marked this pull request as ready for review August 19, 2021 13:19

michael-s-molina force-pushed the change-database-selector-select branch 3 times, most recently from 5ca149c to 77825ed Compare August 19, 2021 14:41

michael-s-molina force-pushed the change-database-selector-select branch 4 times, most recently from 5306809 to f465414 Compare August 19, 2021 19:44

villebro requested changes Aug 20, 2021

View reviewed changes

chore: Changes the DatabaseSelector to use the new Select component

00b99b5

michael-s-molina force-pushed the change-database-selector-select branch from f465414 to 00b99b5 Compare August 21, 2021 16:28

michael-s-molina requested a review from villebro August 21, 2021 16:31

villebro approved these changes Aug 23, 2021

View reviewed changes

michael-s-molina merged commit c768941 into apache:master Aug 23, 2021

AAfghahi reviewed Aug 26, 2021

View reviewed changes

etr2460 pushed a commit that referenced this pull request Aug 26, 2021

Revert "chore: Changes the DatabaseSelector to use the new Select com…

cce34a5

…ponent (#16334)" This reverts commit c768941.

etr2460 mentioned this pull request Aug 26, 2021

fix: Revert "chore: Changes the DatabaseSelector and TableSelector to use the new Select component" #16478

Merged

michael-s-molina pushed a commit that referenced this pull request Aug 27, 2021

Revert "chore: Changes the DatabaseSelector to use the new Select com…

8adc31d

…ponent (#16334)" (#16478) This reverts commit c768941.

michael-s-molina mentioned this pull request Aug 27, 2021

refactor: Changes the DatabaseSelector and TableSelector to use the new Select component #16483

Merged

9 tasks

stevenuray pushed a commit to preset-io/superset that referenced this pull request Aug 31, 2021

Revert "chore: Changes the DatabaseSelector to use the new Select com…

9068dd3

…ponent (apache#16334)" (apache#16478) This reverts commit c768941. (cherry picked from commit 8adc31d)

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

chore: Changes the DatabaseSelector to use the new Select component (a…

6ea88fb

…pache#16334)

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

Revert "chore: Changes the DatabaseSelector to use the new Select com…

d0fd2dd

…ponent (apache#16334)" (apache#16478) This reverts commit c768941.

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021

chore: Changes the DatabaseSelector to use the new Select component (a…

733bc32

…pache#16334)

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021

Revert "chore: Changes the DatabaseSelector to use the new Select com…

616e72d

…ponent (apache#16334)" (apache#16478) This reverts commit c768941.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Changes the DatabaseSelector and TableSelector to use the new Select component #16334

chore: Changes the DatabaseSelector and TableSelector to use the new Select component #16334

michael-s-molina commented Aug 18, 2021 •

edited

codecov bot commented Aug 19, 2021 •

edited

villebro commented Aug 20, 2021

villebro left a comment

villebro Aug 20, 2021

villebro Aug 20, 2021

villebro Aug 20, 2021

villebro Aug 20, 2021

michael-s-molina commented Aug 21, 2021

michael-s-molina commented Aug 22, 2021

github-actions bot commented Aug 22, 2021

villebro left a comment

github-actions bot commented Aug 23, 2021

AAfghahi Aug 26, 2021

michael-s-molina Aug 26, 2021

AAfghahi Aug 26, 2021

		// The endpoint currently does not support pagination
		return SupersetClient.get({ endpoint }).then(({ json }) => {

	`/superset/tables/${currentDbId}/${encodedSchema}/${encodedSubstr}/true/`,
	`/superset/tables/${currentDbId}/${encodedSchema}/${encodedSubstr}/false/`,

chore: Changes the DatabaseSelector and TableSelector to use the new Select component #16334

chore: Changes the DatabaseSelector and TableSelector to use the new Select component #16334

Conversation

michael-s-molina commented Aug 18, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Aug 19, 2021 • edited

Codecov Report

villebro commented Aug 20, 2021

villebro left a comment

Choose a reason for hiding this comment

villebro Aug 20, 2021

Choose a reason for hiding this comment

villebro Aug 20, 2021

Choose a reason for hiding this comment

villebro Aug 20, 2021

Choose a reason for hiding this comment

villebro Aug 20, 2021

Choose a reason for hiding this comment

michael-s-molina commented Aug 21, 2021

michael-s-molina commented Aug 22, 2021

github-actions bot commented Aug 22, 2021

villebro left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 23, 2021

AAfghahi Aug 26, 2021

Choose a reason for hiding this comment

michael-s-molina Aug 26, 2021

Choose a reason for hiding this comment

AAfghahi Aug 26, 2021

Choose a reason for hiding this comment

michael-s-molina commented Aug 18, 2021 •

edited

codecov bot commented Aug 19, 2021 •

edited