feat: Create Chart onClick Functionality #20809

AAfghahi · 2022-07-21T16:10:58Z

SUMMARY

This changes the functionality of the create chart button to be in line with the new feature of a query powered chart. Essentially when the create chart button is now clicked the user will have a new tab open with a table chart using all their current data.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-07-21.at.11.46.42.AM.mov

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

AAfghahi · 2022-07-21T16:14:38Z

/testenv up

hughhhh · 2022-07-21T16:15:58Z

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

@@ -223,7 +248,7 @@ export default class ResultSet extends React.PureComponent<
      // Added compute logic to stop user from being able to Save & Explore
      const { showSaveDatasetModal } = this.state;
      const { query } = this.props;
-
+      console.log('this is query', query);


remove this console.log

@AAfghahi maybe it would be a good idea to get warning on console in linter

hughhhh · 2022-07-21T16:17:31Z

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

-                    // }
-                    this.setState({ showSaveDatasetModal: true });
-                  }}
+                  onClick={this.createExploreResultsOnClick}


we still want to hide this feature so leave the original code in place, and add a comment saying to enable chart source to query change the onClick to onClick={this.createExploreResultsOnClick}

yeah that makes sense, just did that, though will reverse it after this has gone through code review so that QA can test the new flow.

eric-briscoe · 2022-07-21T17:00:58Z

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

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

+  async createExploreResultsOnClick() {
+    const { results } = this.props.query;
+    // There is currently redux / state issue where sometimes a query will have serverId


This comment is probably no longer relevant - did you encounter this issue at all in this workflow?

No I didn't, but wasn't sure so kept it in. Will delete.

hughhhh · 2022-07-21T17:44:50Z

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

+    const { results } = this.props.query;
+
+    if (results.query_id) {
+      const key = await postFormData(results.query_id, 'table', {


the type should be a query

hughhhh · 2022-07-21T17:45:03Z

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

+    if (results.query_id) {
+      const key = await postFormData(results.query_id, 'table', {
+        ...EXPLORE_CHART_DEFAULT,
+        datasource: `${results.query_id}__table`,


type should be query

hughhhh · 2022-07-21T17:46:11Z

Looking at the video it seems like you are registering the query as a dataset not a query

codecov · 2022-07-21T18:33:08Z

Codecov Report

Merging #20809 (8e0deff) into master (84b852c) will increase coverage by 0.02%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##           master   #20809      +/-   ##
==========================================
+ Coverage   66.28%   66.30%   +0.02%     
==========================================
  Files        1757     1761       +4     
  Lines       66796    67060     +264     
  Branches     7052     7131      +79     
==========================================
+ Hits        44276    44465     +189     
- Misses      20721    20794      +73     
- Partials     1799     1801       +2

Flag	Coverage Δ
hive	`53.24% <ø> (ø)`
javascript	`52.05% <11.11%> (+0.11%)`	⬆️
mysql	`80.98% <ø> (ø)`
postgres	`81.05% <ø> (ø)`
presto	`53.10% <ø> (ø)`
python	`81.49% <ø> (ø)`
sqlite	`79.54% <ø> (ø)`
unit	`50.24% <ø> (ø)`

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.74% <11.11%> (-2.33%)`	⬇️
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <0.00%> (ø)`
...et-ui-chart-controls/src/components/labelUtils.tsx	`100.00% <0.00%> (ø)`
...rset-frontend/src/dashboard/actions/datasources.ts	`0.00% <0.00%> (ø)`
...set-frontend/src/dashboard/reducers/datasources.ts	`20.00% <0.00%> (ø)`
...frontend/src/explore/actions/datasourcesActions.ts	`100.00% <0.00%> (ø)`
...rontend/src/explore/reducers/datasourcesReducer.ts	`60.00% <0.00%> (ø)`
superset-frontend/src/views/store.ts	`70.58% <0.00%> (+1.83%)`	⬆️
.../controls/DndColumnSelectControl/OptionWrapper.tsx	`65.85% <0.00%> (+2.43%)`	⬆️
...et-frontend/src/explore/reducers/exploreReducer.js	`42.99% <0.00%> (+3.64%)`	⬆️
... and 6 more

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

eric-briscoe · 2022-07-21T17:03:57Z

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

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

+  async createExploreResultsOnClick() {


if we use arrow function syntax here, you don't have to add the .bind call on line 141,142:
async createExploreResultsOnClick = () => {
Arrow functions automatically bind this to where the function is defined which has the same effect as calling this.createExploreResultsOnClick = this.createExploreResultsOnClick.bind(this);
I have been updating files I work in to convert to arrow functions and remove the bind calls because it removes the second step and extra code.

eric-briscoe · 2022-07-21T17:56:20Z

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

+      });
+      window.open(url, '_blank', 'noreferrer');
+    } else {
+      this.setState({ showSaveDatasetModal: true });


I think we can remove this this.setState({ showSaveDatasetModal: true });. Initially I was trying to use serverId which was not always present and just defaulted to the original behavior. with use of queryId, it should always be there and if it is not we have a bug. We may to change the else to show an error toast indicating the query has no id and cannot be used to create a chart. This should never happen and provides visual indication if something is broken

eric-briscoe · 2022-07-21T18:02:36Z

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

+    const { results } = this.props.query;
+
+    if (results.query_id) {
+      const key = await postFormData(results.query_id, 'table', {


One thing I noticed with this await is that if you set your network speed in dev tools to slow 3G to represent a poor connection, the UI looks unresponsive after clicking the button. We could consider setting state before the await to trigger a re-render and show UI that indicates the user's button clicked is being processed. Not critical, and maybe a separate ticket, but these types of things often get missed in our local dev environment where responses are near instant and have no real world latency / bandwidth restrictions

github-actions · 2022-07-21T22:40:33Z

@AAfghahi Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-07-21T22:40:34Z

@AAfghahi Ephemeral environment creation failed. Please check the Actions logs for details.

explore results button

9039880

pull-request-size bot added the size/M label Jul 21, 2022

AAfghahi requested a review from hughhhh July 21, 2022 16:12

AAfghahi mentioned this pull request Jul 21, 2022

fix: Create Chart opening in new tab #20777

Closed

9 tasks

hughhhh reviewed Jul 21, 2022

View reviewed changes

AAfghahi force-pushed the arash.afghahi/sc-52999/explore-should-have-a-working-table-when branch from fd8fc41 to 8ea1742 Compare July 21, 2022 16:34

eric-briscoe reviewed Jul 21, 2022

View reviewed changes

AAfghahi force-pushed the arash.afghahi/sc-52999/explore-should-have-a-working-table-when branch from 8ea1742 to 4f878c7 Compare July 21, 2022 17:10

addresing hugh comments

9c2f323

AAfghahi force-pushed the arash.afghahi/sc-52999/explore-should-have-a-working-table-when branch from 4f878c7 to 9c2f323 Compare July 21, 2022 17:26

hughhhh reviewed Jul 21, 2022

View reviewed changes

changed datatype to query

30c99ee

eric-briscoe reviewed Jul 21, 2022

View reviewed changes

addresing eric comments

8e0deff

hughhhh approved these changes Jul 22, 2022

View reviewed changes

AAfghahi merged commit 7495cdb into master Jul 22, 2022

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

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

mistercrunch deleted the arash.afghahi/sc-52999/explore-should-have-a-working-table-when branch March 26, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create Chart onClick Functionality #20809

feat: Create Chart onClick Functionality #20809

AAfghahi commented Jul 21, 2022

AAfghahi commented Jul 21, 2022

hughhhh Jul 21, 2022

daryazata Jul 27, 2022

hughhhh Jul 21, 2022

AAfghahi Jul 21, 2022

eric-briscoe Jul 21, 2022

AAfghahi Jul 21, 2022

AAfghahi Jul 21, 2022

hughhhh Jul 21, 2022

hughhhh Jul 21, 2022

hughhhh commented Jul 21, 2022

codecov bot commented Jul 21, 2022 •

edited

Loading

eric-briscoe Jul 21, 2022

eric-briscoe Jul 21, 2022

eric-briscoe Jul 21, 2022

github-actions bot commented Jul 21, 2022

github-actions bot commented Jul 21, 2022

feat: Create Chart onClick Functionality #20809

feat: Create Chart onClick Functionality #20809

Conversation

AAfghahi commented Jul 21, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

AAfghahi commented Jul 21, 2022

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

hughhhh commented Jul 21, 2022

codecov bot commented Jul 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 21, 2022

github-actions bot commented Jul 21, 2022

codecov bot commented Jul 21, 2022 •

edited

Loading