fix: preventing save button from flickering in SQL Lab #25106

fisjac · 2023-08-28T20:56:43Z

SUMMARY

The save button was flickering from a non-split save button to a split save button due to a variable database.allows_virtual_table_explore being undefined after component mounts. Added logic to prevent this flickering behavior by blocking the save button from rendering while allows_virtual_table_explore is undefined. If allows_virtual_table_explore === false, then the non-split save button will still render.

Adjusted testsuite to test for this behavior.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
https://www.loom.com/share/6eaf1679a6ac480daf675d367e561488

After:
https://www.loom.com/share/a378b25defda424689f9aa6a16ff3aed?sid=37b2a345-3d49-4ff2-9e8a-078ad6f92282

TESTING 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

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

john-bodley · 2023-08-29T16:51:24Z

@fisjac thanks for the change. Would you mind adding a more descriptive PR description as well as a before and after screenshot? This would significantly help the review process.

eschutho · 2023-09-01T17:27:43Z

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

@@ -98,6 +98,8 @@ const SaveQuery = ({
  const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
  const isSaved = !!query.remoteId;
  const canExploreDatabase = !!database?.allows_virtual_table_explore;
+  const canShowSaveButton =
+    database?.allows_virtual_table_explore !== undefined;


what should the value of canShowSaveButton be if database is null or database.allows_virtual_table_explore is False?

Basically, what's the difference between canExploreDatabase and canShowSaveButton? Also, just a naming nit, I would change canShowSaveButton to something like shouldShowSaveButton, to signify that 1) it's a boolean like you have it, but also 2) that it's not related to permissions but rather state.

database should always be set since the parent component returns an component if database is not defined.
The allows_virtual_table_explore property is added to the store when dispatch(getDatabases) action is called inside of the , and then the component gets rerendered with databases.allows_virtual_table_explore as a boolean.

While the getDatabases action is awaiting a response databases.allows_virtual_table_explore will be undefined, and canShowSaveButton prevents the save button from being rendered. Once the additional databases data has been added to the redux store, the save button will either render a split button, or the original button based upon the value canExploreDatabase.

betodealmeida · 2023-09-25T20:00:44Z

We need to merge #25399 and rebase this PR so the tests can pass.

…atabase

…ore is undefined rather than falsey. Adjusting tests for this behavior.

fisjac · 2023-09-26T16:34:38Z

@betodealmeida I've merged and rebased this PR to the latest from master.

(cherry picked from commit 296ff17)

* fix: is_select with UNION (apache#25290) (cherry picked from commit bb002d6) * fix: Add explicit ON DELETE CASCADE for dashboard_roles (apache#25320) (cherry picked from commit d54e827) * fix(chart): Supporting custom SQL as temporal x-axis column with filter (apache#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> * fix: Use RLS clause instead of ID for cache key (apache#25229) (cherry picked from commit fba66c6) * fix: Improve the reliability of alerts & reports (apache#25239) (cherry picked from commit f672d5d) * fix: DashboardRoles cascade operation (apache#25349) (cherry picked from commit a971a28) * fix: datetime with timezone excel export (apache#25318) Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> (cherry picked from commit 5ebcd2a) * fix: Workaround for Cypress ECONNRESET error (apache#25399) (cherry picked from commit d76ff39) * fix(sqllab): invalid persisted tab state (apache#25308) (apache#25398) * fix: Rename on_delete parameter to ondelete (apache#25424) (cherry picked from commit 893b45f) * fix: preventing save button from flickering in SQL Lab (apache#25106) (cherry picked from commit 296ff17) * fix: chart import (apache#25425) (cherry picked from commit a4d8f36) * fix: swagger UI CSP error (apache#25368) (cherry picked from commit 1716b9f) * fix: smarter date formatter (apache#25404) (cherry picked from commit f0080f9) * fix(sqllab): invalid start date (apache#25437) * fix(nativeFilters): Speed up native filters by removing unnecessary rerenders (apache#25282) Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> (cherry picked from commit a0eeb4d) * fix(SqlLab): make icon placement even (apache#25372) (cherry picked from commit 11b49a6) * fix: Duplicate items when pasting into Select (apache#25447) (cherry picked from commit 7cf96cd) * fix: update the SQLAlchemy model definition at json column for Log table (apache#25445) (cherry picked from commit e83a76a) * fix(helm chart): set chart appVersion to 3.0.0 (apache#25373) * fix(mysql): handle string typed decimal results (apache#24241) (cherry picked from commit 7eab59a) * fix: Styles not loading because of faulty CSP setting (apache#25468) (cherry picked from commit 0cebffd) * fix(sqllab): error with lazy_gettext for tab titles (apache#25469) (cherry picked from commit ddde178) * fix: Address Mypy issue which is causing CI to fail (apache#25494) (cherry picked from commit 36ed617) * chore: Adds 3.0.1 CHANGELOG * fix: Unable to sync columns when database or dataset name contains `+` (apache#25390) (cherry picked from commit dbe0838) * fix(sqllab): Broken query containing 'children' (apache#25490) (cherry picked from commit b92957e) * chore: Expand error detail on screencapture (apache#25519) (cherry picked from commit ba541e8) * fix: tags permissions error message (apache#25516) (cherry picked from commit 50b0816) * fix: Apply normalization to all dttm columns (apache#25147) (cherry picked from commit 58fcd29) * fix: REST API CSRF exempt list (apache#25590) (cherry picked from commit 549abb5) * fix(RLS): Fix Info Tooltip + Button Alignment on RLS Modal (apache#25400) (cherry picked from commit a6d0e6f) * fix: thubmnails loading - Talisman default config (apache#25486) (cherry picked from commit 52f631a) * fix(Presto): catch DatabaseError when testing Presto views (apache#25559) Co-authored-by: Rui Zhao <zhaorui@dropbox.com> (cherry picked from commit be3714e) * fix(Charts): Set max row limit + removed the option to use an empty row limit value (apache#25579) (cherry picked from commit f556ef5) * fix(window): unavailable localStorage and sessionStorage (apache#25599) * fix: finestTemporalGrainFormatter (apache#25618) (cherry picked from commit 62bffaf) * fix: revert fix(sqllab): Force trino client async execution (apache#24859) (apache#25541) (cherry picked from commit e56e0de) * chore: Updates 3.0.1 CHANGELOG * fix(sqllab): Mistitled for new tab after rename (apache#25523) (cherry picked from commit a520124) * fix(sqllab): template validation error within comments (apache#25626) (cherry picked from commit b370c66) * fix: avoid 500 errors with SQLLAB_BACKEND_PERSISTENCE (apache#25553) (cherry picked from commit 99f79f5) * fix(import): Make sure query context is overwritten for overwriting imports (apache#25493) (cherry picked from commit a0a0d80) * fix: permalink save/overwrites in explore (apache#25112) Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> (cherry picked from commit e58a3ab) * fix(header navlinks): link navlinks to path prefix (apache#25495) (cherry picked from commit 51c56dd) * fix: improve upload ZIP file validation (apache#25658) * fix: warning of nth-child (apache#23638) (cherry picked from commit 16cc089) * fix(dremio): Fixes issue with Dremio SQL generation for Charts with Series Limit (apache#25657) (cherry picked from commit be82657) --------- Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com> Co-authored-by: Zef Lin <zef@preset.io> Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> Co-authored-by: Jack Fragassi <jfragassi98@gmail.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> Co-authored-by: Jack <41238731+fisjac@users.noreply.github.com> Co-authored-by: Daniel Vaz Gaspar <danielvazgaspar@gmail.com> Co-authored-by: Stepan <66589759+Always-prog@users.noreply.github.com> Co-authored-by: Corbin Bullard <corbindbullard@gmail.com> Co-authored-by: Gyuil Han <cnabro91@gmail.com> Co-authored-by: Celalettin Calis <celalettin1286@gmail.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Co-authored-by: ʈᵃᵢ <tdupreetan@gmail.com> Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com> Co-authored-by: mapledan <mapledan829@gmail.com> Co-authored-by: Igor Khrol <khroliz@gmail.com> Co-authored-by: Rui Zhao <105950525+zhaorui2022@users.noreply.github.com> Co-authored-by: Fabien <18534166+frassinier@users.noreply.github.com> Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com> Co-authored-by: OskarNS <soerensen.oskar@gmail.com>

pull-request-size bot added the size/S label Aug 28, 2023

hughhhh reviewed Aug 29, 2023

View reviewed changes

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

john-bodley changed the title ~~fix: preventing save button from flickering in sqlLab~~ fix: preventing save button from flickering in SQL Lab database Aug 29, 2023

john-bodley changed the title ~~fix: preventing save button from flickering in SQL Lab database~~ fix: preventing save button from flickering in SQL Lab Aug 29, 2023

pull-request-size bot added size/XS size/S size/M and removed size/S size/XS labels Aug 30, 2023

eschutho reviewed Sep 1, 2023

View reviewed changes

fisjac added 6 commits September 26, 2023 09:20

preventing save button from rendering prior to evaluating canExploreD…

9b95ca8

…atabase

changing null to false

cc6ee44

adding back hide logic after erroneously removed

2aa0080

adjusting logic to prevent render only when allows_virtual_table_expl…

97a92a4

…ore is undefined rather than falsey. Adjusting tests for this behavior.

fixing conditional expect error in test case

513f8eb

changing canShowSaveButton to shouldShowSaveButton

57327bf

fisjac force-pushed the save-button-polish branch from 0e1f954 to 57327bf Compare September 26, 2023 14:26

betodealmeida approved these changes Sep 26, 2023

View reviewed changes

betodealmeida merged commit 296ff17 into apache:master Sep 26, 2023
26 checks passed

michael-s-molina requested a review from justinpark September 26, 2023 22:09

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Sep 28, 2023

michael-s-molina pushed a commit that referenced this pull request Sep 28, 2023

fix: preventing save button from flickering in SQL Lab (#25106)

4a65d41

(cherry picked from commit 296ff17)

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

fix: preventing save button from flickering in SQL Lab (apache#25106)

5563701

mistercrunch added 🍒 3.0.1 🍒 3.0.2 labels Mar 8, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: preventing save button from flickering in SQL Lab #25106

fix: preventing save button from flickering in SQL Lab #25106

fisjac commented Aug 28, 2023 •

edited

Loading

john-bodley commented Aug 29, 2023

eschutho Sep 1, 2023

eschutho Sep 1, 2023

fisjac Sep 5, 2023

betodealmeida commented Sep 25, 2023

fisjac commented Sep 26, 2023

fix: preventing save button from flickering in SQL Lab #25106

fix: preventing save button from flickering in SQL Lab #25106

Conversation

fisjac commented Aug 28, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

john-bodley commented Aug 29, 2023

eschutho Sep 1, 2023

Choose a reason for hiding this comment

eschutho Sep 1, 2023

Choose a reason for hiding this comment

fisjac Sep 5, 2023

Choose a reason for hiding this comment

betodealmeida commented Sep 25, 2023

fisjac commented Sep 26, 2023

fisjac commented Aug 28, 2023 •

edited

Loading