feat: Sqllab to Explore UX improvements #11755

hughhhh · 2020-11-19T22:10:36Z

SUMMARY

Allow users the option to name their dataset or overwrite a previously created dataset they own. This a new flow to allow users to update datasets they currently own vs. only being able to create new ones.

Creating a new dataset

https://www.loom.com/share/52e624667090477f91db69addc42fccb

Overwriting a dataset

https://www.loom.com/share/4e77b441c4894dc69ccfb655804cc6ae

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

…tor-superset into hugh/SO-1117-modal

superset-frontend/src/SqlLab/components/ExploreResultsButton.jsx

betodealmeida · 2020-11-19T22:32:47Z

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

  }

  componentDidMount() {
    // only do this the first time the component is rendered/mounted
    this.reRunQueryIfSessionTimeoutErrorOnMount();
+
+    // Todo: figure out how to get user information to properly query datasets they own


I think if you query the API on /api/v1/dataset/ it should return only datasets owned by the user (cc: @dpgaspar).

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

eschutho · 2020-12-08T00:34:36Z

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

+      }),
+    );
+
+    this.setState({ userDatasetsOwned });


is this data loaded into redux when the app loads?

The database_ids is actually living in the bootstap-data, but is behind a featureFlag. @mistercrunch has a PR to fix

https://github.com/apache/incubator-superset/pull/11934/files

eschutho · 2020-12-08T00:36:34Z

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

+
+    if (
+      Object.keys(datasetToOverwrite).length === 0 &&
+      datasetToOverwrite.constructor === Object


will Object.keys(datasetToOverwrite).length === 0 ever be true and datasetToOverwrite.constructor === Object be false? Seems like if the former is true, then the constructor will always be an Object?

Good catch i actually figured out how to prevent this via onChangeAutocomplete. Users have to choose a value from the dropdown before progressing to the overwrite page. So we don't need this condition.

eschutho · 2020-12-08T00:40:51Z

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

@@ -357,6 +658,7 @@ export default class ResultSet extends React.PureComponent<
    if (query.progress > 0) {
      progressBar = (
        <ProgressBar
+          // @ts-ignore


are you running into issues where a value is missing on these types/interfaces?

Yea wasn't able to figure out what the issue is

eschutho · 2020-12-08T00:43:50Z

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

+      } = this.state;
+      const disableSaveAndExploreBtn =
+        (saveDatasetRadioBtnState === 1 && newSaveDatasetName.length === 0) ||
+        (saveDatasetRadioBtnState === 2 &&


do you want to use the enum DatasetRadioState here?

graceguo-supercat · 2020-12-08T02:43:33Z

Hi @hughhhh Thanks for making this PR. I have a question: Is there any permission check when one user overwriting an existed dataset? If not, i feel it is a very dangerous feature. It looks like anyone can overwrite my chart without my acknowledge, even they are not owner.

graceguo-supercat · 2020-12-08T02:51:59Z

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

+    this.setState({ datasetToOverwrite: {} });
+  };
+
+  handleOverwriteDataset = async () => {


async/await usually used as a pair, to avoid nested callback hells. Could you please replace .then block with await?

…into hugh/SO-1117-modal

hughhhh · 2020-12-08T04:35:43Z

Hi @hughhhh Thanks for making this PR. I have a question: Is there any permission check when one user overwriting an existed dataset? If not, i feel it is a very dangerous feature. It looks like anyone can overwrite my chart without my acknowledge, even they are not owner.

On the dataset.update we check for the ownership on every request.
https://github.com/apache/incubator-superset/blob/master/superset/datasets/commands/update.py#L88

I'm also only exposing datasets the user owns via getByUser
https://github.com/apache/incubator-superset/pull/11755/files#diff-4563e37b508d681c4d3470358bb89edf6a7f0577849e9f0c9dea92ee37fdd544R176

@graceguo-supercat

graceguo-supercat · 2020-12-08T06:59:40Z

It's good that users only see dataset list from they owned, and can only overwrite dataset that he owned.
But there is a can_access_all_datasources permission: Users with can_access_all_datasources can add anyone as owner of a dataset. For these users, they can still overwrite others dataset (after they add them as dataset owners).
Can you confirm this is expected behavior? There is no extra protection in can_access_all_datasources users?
Thanks @hughhhh!

ktmud · 2020-12-08T07:10:58Z

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

              >
-                Ok
+                Save &amp; Explore


i18n needed

ktmud · 2020-12-08T07:14:47Z

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

+        'MM/DD/YYYY HH:mm:ss',
+      )}`,
+      userDatasetsOwned: [],
+      saveDatasetRadioBtnState: DatasetRadioState.SAVE_NEW,


This is getting too specific. I would just call it "shouldSaveAsNewDataset" or something aline. Imagine what if a future design changes the radio button to a dropdown select?

ktmud · 2020-12-08T07:17:05Z

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

+      shouldOverwriteDataSet: false,
+      datasetToOverwrite: {},
+      saveModalAutocompleteValue: '',
+      userDatasetOptions: [],


I feel you may be passing too many states between components unnecessarily. Can these be combined into one construct as the modal's local states and only be passed up to the parent component when users hit "Save"?

ktmud · 2020-12-08T07:22:26Z

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

+      shouldOverwriteDataSet: false,
+      datasetToOverwrite: {},
+      newSaveDatasetName: `${this.props.query.tab} ${moment().format(
+        'MM/DD/YYYY HH:mm:ss',


This is US English only. No need to resolve in this PR, but we should really start thinking about an i18n strategy for things like date formatting.

cc @villebro @rusackas

+1 i18n strategy

"YYYY-MM-DD HH:mm:ss" It's a more common way for i18n.

+1 on using YYYY-MM-DD HH:mm:ss by default before we add proper internalization/localization support.

ktmud · 2020-12-08T07:23:57Z

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

+      newSaveDatasetName: `${this.props.query.tab} ${moment().format(
+        'MM/DD/YYYY HH:mm:ss',
+      )}`,
+    });


There's seem to be some repetitive code between Override and Save as New. Is there a chance for abstraction?

hughhhh · 2020-12-08T21:17:21Z

It's good that users only see dataset list from they owned, and can only overwrite dataset that he owned.
But there is a can_access_all_datasources permission: Users with can_access_all_datasources can add anyone as owner of a dataset. For these users, they can still overwrite others dataset (after they add them as dataset owners).
Can you confirm this is expected behavior? There is no extra protection in can_access_all_datasources users?
Thanks @hughhhh!

@graceguo-supercat So the only way for the users to overwrite a dataset is if they are in the owner list via check_ownership function. So if a user has access to all datasource they must first add themselves as an owner then they'll be able to overwrite.

…into hugh/SO-1117-modal

hughhhh added 23 commits October 31, 2020 13:33

create boiler modal component

fc914fd

hello world modal

bfac037

setup modal flow

b2a8d6b

setup savemodal for components

04e1216

flake8

6d2a61b

fix onclick reference

3170382

working create datasource boiler

268052c

saving spot for callback on text input

74348a1

working dataset with input box

711c5ac

working redirect on completion

a734442

get data for owners dropdown

0c7810a

fix merge conflicts

b8fe70e

fix build with pull from master

c356796

fix the filteroption

d3cd168

💯

e72d882

move state to upper component

4c7364f

add overwrite state

dba206a

hacked overwrite process

398bfee

linting

07e7388

fix filter

203aa15

cleaning up the coe

f8a34be

Delete preset.code-workspace

066054e

Merge branch 'hugh/SO-1117-modal' of https://github.com/apache/incuba…

f35b59b

…tor-superset into hugh/SO-1117-modal

pull-request-size bot added the size/L label Nov 19, 2020

hughhhh added 2 commits November 19, 2020 14:11

remove unused code

482353f

remove visualize

cc5dfff

betodealmeida reviewed Nov 19, 2020

View reviewed changes

superset-frontend/src/SqlLab/components/ExploreResultsButton.jsx Show resolved Hide resolved

betodealmeida reviewed Nov 19, 2020

View reviewed changes

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

update default value

616480b

hughhhh added 2 commits December 7, 2020 13:36

reverting CTAS btn flow

70166d0

remove onclick

b4c53b6

eschutho reviewed Dec 8, 2020

View reviewed changes

hughhhh added 3 commits December 7, 2020 17:07

save frontend work

250fd99

allow for database id to be passed as param in body

c09950c

use enum

0bae866

pull-request-size bot added size/L and removed size/XL labels Dec 8, 2020

fix linting

d9a6434

graceguo-supercat reviewed Dec 8, 2020

View reviewed changes

Merge branch 'master' of https://github.com/apache/incubator-superset …

e3af417

…into hugh/SO-1117-modal

ktmud reviewed Dec 8, 2020

View reviewed changes

hughhhh added 4 commits December 8, 2020 12:30

style alignment

d151c1b

get rid of .then

2ec480a

add function to compute default value with tiem

1d8686a

lit

88ba7a1

hughhhh added 3 commits December 8, 2020 13:45

remove ts-error

1aff0f1

fix typing

2953258

Merge branch 'master' of https://github.com/apache/incubator-superset …

d10f0da

…into hugh/SO-1117-modal

hughhhh merged commit cc44a2c into master Dec 9, 2020

amitmiran137 deleted the hugh/SO-1117-modal branch March 29, 2021 18:11

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Sqllab to Explore UX improvements #11755

feat: Sqllab to Explore UX improvements #11755

hughhhh commented Nov 19, 2020 •

edited

Loading

betodealmeida Nov 19, 2020

eschutho Dec 8, 2020

hughhhh Dec 8, 2020 •

edited

Loading

eschutho Dec 8, 2020

hughhhh Dec 8, 2020

eschutho Dec 8, 2020

hughhhh Dec 8, 2020

eschutho Dec 8, 2020

graceguo-supercat commented Dec 8, 2020 •

edited

Loading

graceguo-supercat Dec 8, 2020

hughhhh commented Dec 8, 2020 •

edited

Loading

graceguo-supercat commented Dec 8, 2020

ktmud Dec 8, 2020

ktmud Dec 8, 2020

ktmud Dec 8, 2020 •

edited

Loading

ktmud Dec 8, 2020

junlincc Dec 8, 2020 •

edited

Loading

zhaoyongjie Dec 8, 2020

ktmud Dec 8, 2020

ktmud Dec 8, 2020

hughhhh commented Dec 8, 2020

feat: Sqllab to Explore UX improvements #11755

feat: Sqllab to Explore UX improvements #11755

Conversation

hughhhh commented Nov 19, 2020 • edited Loading

SUMMARY

Creating a new dataset

Overwriting a dataset

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughhhh Dec 8, 2020 • edited Loading

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

graceguo-supercat commented Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

hughhhh commented Dec 8, 2020 • edited Loading

graceguo-supercat commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junlincc Dec 8, 2020 • edited Loading

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 Dec 8, 2020

hughhhh commented Nov 19, 2020 •

edited

Loading

hughhhh Dec 8, 2020 •

edited

Loading

graceguo-supercat commented Dec 8, 2020 •

edited

Loading

hughhhh commented Dec 8, 2020 •

edited

Loading

ktmud Dec 8, 2020 •

edited

Loading

junlincc Dec 8, 2020 •

edited

Loading