feat(SqlLab): Change Save Dataset Button to Split Save Query Button IV #20852

lyndsiWilliams · 2022-07-25T17:23:48Z

SUMMARY

This changes the current save query button into a split save button if the database has "Allow this database to be explored" enabled. When the user clicks the caret section of the button, they can now save the query as a dataset.

Bonus: In order to create this button, I conveniently needed to fix the styling on the "Run" query split button. The split line now goes all the way through the button and the caret is centered.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

TESTING INSTRUCTIONS

From SqlLab, open a query for a database that has "Allow this database to be explored" enabled.
- Observe the new split save button
  - Click the "Save" section of the button to see the save query modal
  - Click the caret v section of the button, then click "Save dataset" to see the save dataset modal
  - Both of these modals should be fully functional
From SqlLab, open a query for a database that does NOT have "Allow this database to be explored" enabled.
- Observe that the save button is a normal button, not split
  - Clicking this button should show the save query modal

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

hughhhh · 2022-07-25T17:28:49Z

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

@@ -275,11 +250,19 @@ export default class ResultSet extends React.PureComponent<
              this.props.database?.allows_virtual_table_explore && (
                <ExploreResultsButton
                  database={this.props.database}
-                  onClick={() => this.setState({ showSaveDatasetModal: true })}


add back this line

Added back in this commit.

hughhhh · 2022-07-25T17:28:57Z

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

                />
-                // In order to use the new workflow for a query powered chart, replace the


add back these lines

more of a broader question, when do we make this functionality the default behavior?

Added back in this commit.

hughhhh · 2022-07-25T17:29:11Z

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

+                  onClick={() => {
+                    // There is currently redux / state issue where sometimes a query will have serverId
+                    // and other times it will not.  We need this attribute consistently for this to work
+                    // const qid = this.props?.query?.results?.query_id;
+                    // if (qid) {
+                    //   // This will open explore using the query as datasource
+                    //   window.location.href = `/explore/?dataset_type=query&dataset_id=${qid}`;
+                    // } else {
+                    //   this.setState({ showSaveDatasetModal: true });
+                    // }
+                    this.setState({ showSaveDatasetModal: true });
+                  }}


remove these lines

Removed in this commit.

hughhhh · 2022-07-25T17:30:08Z

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

-describe('SaveDatasetModal RTL', () => {
-  it('renders a "Save as new" field', () => {
-    render(<SaveDatasetModal {...mockedProps} />, { useRedux: true });
+const testDatasetList = {


can we put this in a separate fixture file?

Yeah, once I'm done fixing this test file I'll be cleaning it up.

hughhhh · 2022-07-25T17:31:08Z

superset-frontend/src/components/Chart/Chart.jsx

+        </ErrorBoundary>
+        {showSaveDatasetModal && (
+          <SaveDatasetModal
+            key={Math.random()}


why are we doing a Math.random() here for the key? couldn't we just set it as specific value?

I think the SaveDatasetModal can be deleted from this, we ended up invoking it in another portion of the code.

This was removed in this commit.

AAfghahi · 2022-07-25T17:47:03Z

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

                />
-                // In order to use the new workflow for a query powered chart, replace the


more of a broader question, when do we make this functionality the default behavior?

AAfghahi · 2022-07-25T17:51:19Z

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

-  const toggleSave = () => {
-    setShowSave(!showSave);
-  };
+  const toggleSave = () => setShowSave(!showSave);


nit: I think that this would be better taking in a boolean value that we set. Sometimes when connections are slower, the action doesn't immediately render and users click multiple times, causing it to reverse back to original value.

Good call, I cleaned this up in this commit.

AAfghahi · 2022-07-25T17:53:43Z

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

@@ -29,19 +29,14 @@ import {
  SaveDatasetModal,
 } from 'src/SqlLab/components/SaveDatasetModal';
 import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
-import { EXPLORE_CHART_DEFAULT } from 'src/SqlLab/types';


I think this was lost in a rebase potentially? Please revert this.

Yeah there seems to be some weird rebase stuff on this PR, thanks for catching all these! Re-added in this commit.

AAfghahi · 2022-07-25T17:53:54Z

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

@@ -218,26 +213,6 @@ export default class ResultSet extends React.PureComponent<
    }
  }

-  createExploreResultsOnClick = async () => {


Re-added in this commit.

AAfghahi · 2022-07-25T17:58:23Z

superset-frontend/src/components/Chart/Chart.jsx

@@ -25,6 +25,7 @@ import { PLACEHOLDER_DATASOURCE } from 'src/dashboard/constants';
 import Loading from 'src/components/Loading';
 import { EmptyStateBig } from 'src/components/EmptyState';
 import ErrorBoundary from 'src/components/ErrorBoundary';
+import { SaveDatasetModal } from 'src/SqlLab/components/SaveDatasetModal';


I believe that this is part of an older rebase, and is no longer in this portion of the code base.

Removed in this commit.

AAfghahi · 2022-07-25T17:58:28Z

superset-frontend/src/components/Chart/Chart.jsx

@@ -122,8 +123,10 @@ const MonospaceDiv = styled.div`
 class Chart extends React.PureComponent {
  constructor(props) {
    super(props);
+    this.state = { showSaveDatasetModal: false };


Removed in this commit.

AAfghahi · 2022-07-25T17:59:50Z

superset-frontend/src/components/Chart/Chart.jsx

+        </ErrorBoundary>
+        {showSaveDatasetModal && (
+          <SaveDatasetModal
+            key={Math.random()}


I think the SaveDatasetModal can be deleted from this, we ended up invoking it in another portion of the code.

AAfghahi · 2022-07-25T17:59:59Z

superset-frontend/src/components/Chart/Chart.jsx

@@ -136,6 +139,12 @@ class Chart extends React.PureComponent {
    }
  }

+  toggleSaveDatasetModal = () => {


Removed in this commit.

AAfghahi · 2022-07-25T18:00:16Z

superset-frontend/src/components/Chart/Chart.jsx

@@ -253,6 +262,7 @@ class Chart extends React.PureComponent {
      width,
    } = this.props;

+    const { showSaveDatasetModal } = this.state;


same as above

Removed in this commit.

AAfghahi · 2022-07-25T18:00:37Z

superset-frontend/src/components/Chart/ChartErrorMessage.tsx

@@ -42,6 +42,5 @@ export const ChartErrorMessage: React.FC<Props> = ({
    ...error,
    extra: { ...error.extra, owners },
  };
-


nit: add this space back?

Weird rebase stuff haha! Added back in this commit.

AAfghahi

LGTM

codecov · 2022-07-25T22:11:55Z

Codecov Report

Merging #20852 (548c804) into master (3a11856) will increase coverage by 0.05%.
The diff coverage is 77.14%.

@@            Coverage Diff             @@
##           master   #20852      +/-   ##
==========================================
+ Coverage   66.30%   66.36%   +0.05%     
==========================================
  Files        1759     1760       +1     
  Lines       67088    67096       +8     
  Branches     7127     7129       +2     
==========================================
+ Hits        44486    44528      +42     
+ Misses      20770    20744      -26     
+ Partials     1832     1824       -8

Flag	Coverage Δ
javascript	`52.12% <77.14%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
...frontend/src/SqlLab/components/ResultSet/index.tsx	`51.03% <ø> (+0.34%)`	⬆️
...frontend/src/SqlLab/components/SqlEditor/index.jsx	`51.36% <0.00%> (ø)`
...d/src/SqlLab/components/TabbedSqlEditors/index.jsx	`56.11% <ø> (ø)`
...et-frontend/src/SqlLab/reducers/getInitialState.js	`46.42% <ø> (ø)`
superset-frontend/src/SqlLab/reducers/sqlLab.js	`33.33% <ø> (ø)`
superset-frontend/src/SqlLab/types.ts	`57.14% <ø> (ø)`
superset-frontend/src/components/Chart/Chart.jsx	`52.54% <ø> (ø)`
superset-frontend/src/utils/datasourceUtils.js	`100.00% <ø> (ø)`
...d/src/SqlLab/components/SaveDatasetModal/index.tsx	`50.00% <40.00%> (+15.11%)`	⬆️
superset-frontend/src/SqlLab/actions/sqlLab.js	`60.44% <66.66%> (ø)`
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

lyndsiWilliams · 2022-07-28T16:46:28Z

/testenv up

github-actions · 2022-07-28T17:23:46Z

@lyndsiWilliams Ephemeral environment spinning up at http://52.25.149.118:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

lyndsiWilliams · 2022-07-29T22:24:27Z

/testenv up

github-actions · 2022-07-29T22:28:46Z

@lyndsiWilliams Ephemeral environment spinning up at http://54.202.3.230:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

lyndsiWilliams · 2022-07-29T22:49:25Z

/testenv up

github-actions · 2022-07-29T22:53:35Z

@lyndsiWilliams Ephemeral environment spinning up at http://54.185.46.90:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

lyndsiWilliams · 2022-07-29T23:24:22Z

/testenv up

github-actions · 2022-07-29T23:28:26Z

@lyndsiWilliams Ephemeral environment spinning up at http://35.165.194.84:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

…lit-save-btn

github-actions · 2022-08-01T19:36:57Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/XXL label Jul 25, 2022

lyndsiWilliams mentioned this pull request Jul 25, 2022

feat(SqlLab): Change Save Dataset Button to Split Save Query Button II #20570

Closed

9 tasks

hughhhh reviewed Jul 25, 2022

View reviewed changes

lyndsiWilliams mentioned this pull request Jul 25, 2022

feat(SqlLab): Change Save Dataset Button to Split Save Query Button #20515

Closed

9 tasks

hughhhh reviewed Jul 25, 2022

View reviewed changes

AAfghahi reviewed Jul 25, 2022

View reviewed changes

AAfghahi approved these changes Jul 25, 2022

View reviewed changes

pull-request-size bot added size/XL and removed size/XXL labels Jul 27, 2022

lyndsiWilliams mentioned this pull request Jul 28, 2022

feat(SqlLab): Change Save Dataset Button to Split Save Query Button III preset-io/superset#234

Closed

9 tasks

lyndsiWilliams added 11 commits July 29, 2022 17:42

Moving entire split save btn PR

c7ee008

Addressed review comments

19f96c1

Remove arbitrary div from ErrorBoundary in Chart

b370209

Added accidentally removed comment

f66ce82

Fix act errors in SaveQuery tests

5c3c9c2

Fix SaveDatasetActionButton test

47b275d

SaveDatasetModal test almost working

4ed58bc

SaveDatasetModal tests all passing

c415fe4

Clean SaveDatasetModal test

3fe3741

Fix create chart button and SaveDatasetModal text in SQL Lab

da90320

Fix untitled dataset name on SaveDatasetModal in explore

76375a8

lyndsiWilliams force-pushed the lyndsi/split-save-btn branch from 2f41613 to 76375a8 Compare July 29, 2022 22:42

Fix styling on split save button

a8ef2ea

hughhhh approved these changes Aug 1, 2022

View reviewed changes

Merge branch 'master' of github.com:preset-io/superset into lyndsi/sp…

548c804

…lit-save-btn

lyndsiWilliams merged commit 8a04536 into apache:master Aug 1, 2022

lyndsiWilliams deleted the lyndsi/split-save-btn branch August 1, 2022 19:37

AAfghahi added the logging Creates a UI or API endpoint that could benefit from logging. label Aug 5, 2022

justinpark mentioned this pull request Aug 29, 2022

fix(sqllab): sql editor's name has lost after reload #21246

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

		/>
		// In order to use the new workflow for a query powered chart, replace the

feat(SqlLab): Change Save Dataset Button to Split Save Query Button IV #20852

feat(SqlLab): Change Save Dataset Button to Split Save Query Button IV #20852

Conversation

lyndsiWilliams commented Jul 25, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

lyndsiWilliams Jul 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAfghahi left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 25, 2022 • edited

Codecov Report

lyndsiWilliams commented Jul 28, 2022

github-actions bot commented Jul 28, 2022

lyndsiWilliams commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

lyndsiWilliams commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

lyndsiWilliams commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

github-actions bot commented Aug 1, 2022

lyndsiWilliams commented Jul 25, 2022 •

edited

lyndsiWilliams Jul 25, 2022 •

edited

codecov bot commented Jul 25, 2022 •

edited