feat: Flow for tables that already have a dataset #22136

lyndsiWilliams · 2022-11-16T01:04:53Z

SUMMARY

This implements a flow in the new dataset creation page for when a table already has a dataset. It uses a simplified dataset fetch in the AddDataset/index.tsx file to cross-reference with the table list. If the table has a dataset, it gets a warning icon in the left panel. If the table is selected, the table's columns are still displayed, but the "Create Dataset" button is disabled and it has an info banner at the top informing the user of the pre-existing dataset with a link to that dataset in the upper-left corner of the alert. Clicking the "View Dataset" button in this alert will bring the user to the explore view of the existing dataset in a new tab.

ANIMATED GIF / SCREENSHOT

Screenshot of selected table with existing dataset

Left panel warning icons

"View dataset"

TESTING INSTRUCTIONS

Go to http://localhost:9000/dataset/add/?testing
Select a database and a schema
Observe that any tables in the left panel with a pre-existing dataset will have a warning icon
Click a table with a dataset
Observe that the "Create dataset" button is disabled and there is an info banner at the top informing of the pre-existing dataset
Click "View dataset" in the alert
Observe that you are taken to the explore view of the existing dataset in a new tab

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 · 2022-11-16T01:26:15Z

Codecov Report

Merging #22136 (11e2409) into master (6f6cb18) will decrease coverage by 1.59%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master   #22136      +/-   ##
==========================================
- Coverage   66.98%   65.39%   -1.60%     
==========================================
  Files        1832     1851      +19     
  Lines       69918    72903    +2985     
  Branches     7570     8661    +1091     
==========================================
+ Hits        46838    47672     +834     
- Misses      21122    23069    +1947     
- Partials     1958     2162     +204

Flag	Coverage Δ
hive	`52.60% <ø> (ø)`
mysql	`78.15% <ø> (ø)`
postgres	`78.21% <ø> (ø)`
presto	`52.49% <ø> (ø)`
python	`78.69% <ø> (-2.69%)`	⬇️
sqlite	`76.67% <ø> (ø)`
unit	`?`

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

Impacted Files	Coverage Δ
...RUD/data/dataset/AddDataset/DatasetPanel/index.tsx	`29.03% <ø> (ø)`
...set-frontend/src/views/CRUD/data/dataset/styles.ts	`100.00% <ø> (ø)`
superset-frontend/src/views/CRUD/data/hooks.ts	`57.69% <25.00%> (-5.95%)`	⬇️
...d/src/views/CRUD/data/dataset/AddDataset/index.tsx	`51.61% <41.66%> (-8.39%)`	⬇️
...s/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx	`87.32% <78.57%> (+0.65%)`	⬆️
...a/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx	`86.95% <90.90%> (-4.72%)`	⬇️
...D/data/dataset/AddDataset/DatasetPanel/fixtures.ts	`100.00% <100.00%> (ø)`
...iews/CRUD/data/dataset/AddDataset/Footer/index.tsx	`39.39% <100.00%> (+1.89%)`	⬆️
...set/advanced_data_type/plugins/internet_address.py	`16.32% <0.00%> (-79.60%)`	⬇️
superset/utils/pandas_postprocessing/boxplot.py	`20.51% <0.00%> (-79.49%)`	⬇️
... and 191 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lyndsiWilliams · 2022-11-16T02:52:42Z

/testenv up

github-actions · 2022-11-16T02:57:03Z

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

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

eschutho · 2022-11-17T04:45:40Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

 }

+const renderExistingDatasetAlert = (linkedDataset: any) => (


Can we put a type on this linkeddataset?

Oops, good catch! Typed in this commit.

eric-briscoe

Overall looks good and works as described when testing on ephemeral. I left a few comments I think are worth addressing

eric-briscoe · 2022-11-16T01:30:16Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

+    message={t('This table already has a dataset')}
+    description={
+      <>
+        {t(


I recommend defining all text using t() as const outside of functions so that the text only gets translated once, not per render / function call. It also makes the text exportable for use in test files so you don't have to keep string literals defined in multiple files in sync manually.

I am commenting once but recommend for anywhere t() is being called inside a function / functional component.

Example can be seen on line 149, 150, 151

Thanks for the explanation on this, I didn't realize it was running on each render/function call but it definitely makes sense! I fixed these spots and some others that I found in this commit.

eric-briscoe · 2022-11-16T01:32:33Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx

@@ -110,7 +113,7 @@ const DatasetPanelWrapper = ({
    if (tableName && schema && dbId) {
      getTableMetadata({ tableName, dbId, schema });
    }
-    // getTableMetadata is a const and should not be independency array
+    // getTableMetadata is a const and should not be in dependency array


nice catch!

Thanks! 😁

eric-briscoe · 2022-11-16T01:43:46Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx

-      .catch(e => {
-        console.log('error', e);
-      });
+      .catch(error => console.log('There was an error fetching tables', error));


Should this be an error instead of log? Also there is actually a logging utility in Superset-frontend that I was introduced to lately that should be best practice instead of calling console.log | .error | etc directly

You can: import { logging } from '@superset-ui/core';

Then call the logging methods defined at: https://github.com/apache/superset/blob/3c41ff68a43b5ab6b871226a73de9f2129d64766/superset-frontend/packages/superset-ui-core/src/utils/logging.ts

This lets us avoid ts-ignore comments anywhere we legitimately need to log something, and will allow for enriching what we do with logging calls in future, such as send some of the errors for backend logging to capture in usability metrics

I actually didn't know about this logging utility, thanks for the info! Changed the catch in this commit.

eric-briscoe · 2022-11-16T20:59:22Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx

+        );
+      })
+      .catch(error =>
+        console.log('There was an error fetching dataset', error),


Please see other comment about using logging utility

Changed the catch in this commit.

eric-briscoe · 2022-11-16T21:03:34Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx

@@ -113,7 +115,11 @@ function Footer({
      <Button onClick={cancelButtonOnClick}>Cancel</Button>
      <Button
        buttonStyle="primary"
-        disabled={!datasetObject?.table_name || !hasColumns}
+        disabled={


This is a nit, but since this disabled check is getting kind of long it may be worth defining a variable doing this check before the return JSX block which will make the code easier to debug in future and keep the actual JSX portion cleaner.

Example:

const disabled = !datasetObject?.table_name || !hasColumns || linkedDatasets?.includes(datasetObject?.table_name); return ( <> <Button onClick={cancelButtonOnClick}>Cancel</Button> <Button buttonStyle="primary" disabled tooltip={!datasetObject?.table_name ? tooltipText : undefined} onClick={onSave} >

Oh yeah good call, much cleaner! Changed in this commit

eric-briscoe · 2022-11-16T21:28:42Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx

+  };
+
+  useEffect(() => {
+    if (dataset?.schema) getDatasetsList();


Although JavaScript allows for omitting curly braces around one line conditionals and functions, over the years it has become a widely recommended practice to always include curly braces for following reasons:

It is a common area bug bugs to appear when refactoring is done from single line to multiple lines

It can make code ambiguous based on line spacing
if (myVar > 3) myVar = 0; doSomething(myVar);
The above code can be confusing to identify if the author intended to always run doSomething(myVar); after the if statement, or they made a code mistake and only want that to run in context of the if statement and forgot to add braces. This can get a lot more confusing than this simple example when this syntax gets used frequently.

Later if additional logic is added the curly braces will need to be added back. If dev forgets this the logic will run, but second line with run outside of the conditional

Because it has been recommended to be avoided, this syntax is not common to see in JavasScript code and some developers may be confused by and /or misinterpret the code and change / refactor incorrectly.

Many tools have rules defined to track this syntax as a bug, for example: This is a common SonarQube rule: https://3layer.com.br/sonar/rules/show/javascript:CurlyBraces?layout=false

Consistency, code clarity, and avoiding bugs typically trumps saving extra characters. This may be worth discussing adding as a lint rule with wider group

agree on the lint rule as a follow up +1

Oh wow I didn't realize this was so risky! I added brackets in this commit and will avoid this in the future.

eric-briscoe

I found a theme anti-pattern we should adjust

eric-briscoe · 2022-11-17T22:21:46Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

+            title={tableName || ''}
+          >
+            {tableName && (
+              <Icons.Table iconColor={supersetTheme.colors.grayscale.base} />


This is where I commented earlier use of supersetTheme directly is an anit pattern we did not catch in initial creation of this file.

Above in the functional component we should have
const theme = useTheme();

Then here use
<Icons.Table iconColor={theme.colors.grayscale.base} />`

FIxed in this commit.

eric-briscoe · 2022-11-17T22:23:59Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

@@ -19,10 +19,12 @@
 import React from 'react';
 import { supersetTheme, t, styled } from '@superset-ui/core';


@lyndsiWilliams I missed this when we originally worked on DatasetPanel but using supersetTheme directly is an anti-pattern. We should remove this import, and instead bring in useTheme
import { useTheme, t, styled } from '@superset-ui/core';
At line 131-148, and line 288, anywhere else we directly use supersetTheme should be changed.

FIxed in this commit.

eric-briscoe · 2022-11-17T22:26:16Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

@@ -123,6 +128,24 @@ const StyledTable = styled(Table)`
  right: 0;
 `;

+const StyledAlert = styled(Alert)`


Related to coment on line 20, we shuld repelase use of supertThem with:
const StyledAlert = styled(Alert)( ({ theme }) =>

and replace all the supersetTheme with theme
for example:
`border: 1px solid ${theme.colors.info.base};

FIxed in this commit.

superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx

eschutho · 2022-11-18T00:06:19Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx

+      })
+    : undefined;
+
+  const getDatasetsList = () => {


I'd recommend moving any api calls outside of the view component into either a hook or redux action for separation of concerns.

Moved the API call into a hook in this commit.

EugeneTorap · 2022-11-18T20:30:25Z

@eschutho @eric-briscoe @lyndsiWilliams I have some nice suggestions!

This is the new dataset creation page and we have [SIP-61] - Improve the organization of front-end folders created by @michael-s-molina
We can move this page into src/pages/addDataset and it'll be first component in src/pages after that we can smoothly move others page to the folder or if we create a new page for superset we can immediately do it in src/pages.
Creating Virtual Dataset on this new page. It will be good if we run our SQL query here we can see all metadata: columns types, used tables in the query, labels/aliases of columns and so on. Because in SQL Lab if you run sql query you can only see result data without columns metadata and others additional info.

What do you think about it?

…ts to datasets

lyndsiWilliams · 2022-11-22T02:16:52Z

@eschutho @eric-briscoe @lyndsiWilliams I have some nice suggestions!

This is the new dataset creation page and we have [SIP-61] - Improve the organization of front-end folders created by @michael-s-molina
We can move this page into src/pages/addDataset and it'll be first component in src/pages after that we can smoothly move others page to the folder or if we create a new page for superset we can immediately do it in src/pages.

Creating Virtual Dataset on this new page. It will be good if we run our SQL query here we can see all metadata: columns types, used tables in the query, labels/aliases of columns and so on. Because in SQL Lab if you run sql query you can only see result data without columns metadata and others additional info.

What do you think about it?

Hey @EugeneTorap!

This is a good suggestion, but it is a pretty substantial change for this PR. It will be better to do this in a future cleanup PR.
We currently don't have plans to add a virtual dataset from this flow. Now that we can create charts from a query in SQL Lab, the left panel in Explore should show you the column metadata info.

AAfghahi · 2022-11-22T15:08:58Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
   * Boolean indicating if the component is in a loading state
   */
  loading: boolean;
+  datasets?: DatasetObject[] | undefined;


nit: I kind of want this to have a better name, maybe existingDatasets? or something along those lines.

I was going off of what @eschutho suggested in this review comment. I'm not really settled on the best name here but I'd be fine changing it, would like to hear what Elizabeth thinks of existingDatasets as well.

Makes sense!

Does this need to be a list or can it be boolean? From my, limited, understanding of this feature you're seeing if there already is a dataset linked, so we don't need to be passing in a list of datasets, we could just check to see if there is an associated one.

We need the whole object so we can cross-reference the list of table names with the table names in the list of datasets here:

superset/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

Lines 292 to 295 in d77263d

{datasetNames?.includes(tableName) &&

renderExistingDatasetAlert(

datasets?.find(dataset => dataset.table_name === tableName),

)}

And when it's passed into renderExistingDatasetAlert() we need to be able to pull out the explore_url from the dataset object here:

superset/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

Lines 224 to 230 in 0bed85a

onClick={() => {

window.open(

dataset?.explore_url,

'_blank',

'noreferrer noopener popup=false',

);

}}

got it! Makes sense

AAfghahi · 2022-11-22T15:11:40Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

 }

+const EXISTING_DATASET_DESCRIPTION = t(
+  'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',


Hmm, does this need to be preset specific language? If so, then I think we need to add that in on the Preset side and have a more generic description in superset.

That makes sense, but I'm not sure about Preset-specific language here. I think we'd need @yousoph 's input on this one.

Updated text: This table already has a dataset associated with it. You can only associate one dataset with a table.

I spoke with Sophie and Karan in Slack and we discussed changing the text to be more general and not include Superset or Preset. Changed in this commit.

AAfghahi · 2022-11-22T15:13:44Z

/testenv up

github-actions · 2022-11-22T15:17:41Z

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

lyndsiWilliams · 2022-11-22T22:21:33Z

/testenv up

github-actions · 2022-11-22T22:25:48Z

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

eric-briscoe · 2022-12-02T22:38:50Z

superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

@@ -94,7 +94,7 @@ const LoaderContainer = styled.div`
  display: flex;
  align-items: center;
  justify-content: center;
-  height: 100%;
+  height: 98%;


@lyndsiWilliams typically we we see scrolling with something where width / height is set to 100% due to box-sizing logic. By default CSS does not subtract padding and border thickness from the value of the parent elements size. This makes the child set its size to 100% of the parent element plus the child's padding and border dimensions (100% of parent + child's padding and borders making the child larger than the parents visible area). Getting rid of the scrollbar should be achievable with:

height: 100%; box-sizing: border-box;

The CSS box-sizing property allows us to include the padding and border in an element's total width and height calculation

The reason we might want to try this instead of reducing the height to 98% is that 2% of the total height in actual pixels varies on the browser window's height. The higher the screen resolution and taller the browser window is, the more that 2% will cause an offset of actual vertical center alignment. For example on a HD screen the offset would be ~ 17px (850px of usable screen after browsers UI. 850 * 0.02 = ~17px). On a 4k screen the offset could be significantly larger. In other words as the browser grows in height, the more non-centered aligned the item becomes. Alternatively, as the screen size reduces it becomes possible that 2% is less than the height - padding+border and scroll bar still ends up appearing. Using 100% with the box-sizing ensures we remain center aligned and void scroll regardless of browser height and screen resolution.

good explanation with interactive examples of box-sizing here: https://www.w3schools.com/css/css3_box-sizing.asp

Oh that makes sense, I didn't know about box-sizing, thank you! But setting height to 100% and box-sizing: border-box did not get rid of the scrollbar on my end. Did it work on your end?

I see the reasoning for wanting to include 100% of the element, but I was thinking it might be acceptable to do 98% height for this since it's just the loading gif that shows in the center, so that 2% will never cut any visual elements off. What do you think?

Implemented the border-box fix we discussed on Slack in this commit.

eric-briscoe

@lyndsiWilliams Latest looks good! There is one more CSS height issue we discussed that is already fixed in follow on PR to this so approving as is. thanks!

eschutho

This is great @lyndsiWilliams!!

github-actions · 2022-12-05T21:44:22Z

Ephemeral environment shutdown and build artifacts deleted.

lyndsiWilliams added 6 commits November 15, 2022 14:45

User is now notified if a table already has a dataset

f5b01d5

Tables with dataset flow complete, leftPanel tested

16f1f6e

Re-added banner to dataset panel post-rebase

d697bbf

DatasetPanel and Footer tested

73c5950

Hover styles added to info banner

4281a00

Set view dataset button to open in new tab

b58e689

pull-request-size bot added the size/L label Nov 16, 2022

Adjust View Dataset button hover color

8f47e55

eschutho reviewed Nov 17, 2022

View reviewed changes

eric-briscoe suggested changes Nov 17, 2022

View reviewed changes

eschutho reviewed Nov 18, 2022

View reviewed changes

superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx Outdated Show resolved Hide resolved

eschutho reviewed Nov 18, 2022

View reviewed changes

superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx Outdated Show resolved Hide resolved

eschutho reviewed Nov 18, 2022

View reviewed changes

lyndsiWilliams added 2 commits November 21, 2022 14:44

Fixed theming calls, bracket formation, logging, and typing issues

8654966

Move getDatasetList API call to a hook, change naming of linkedDatase…

ad13ff4

…ts to datasets

optimized setDatasets call

955ffc0

AAfghahi reviewed Nov 22, 2022

View reviewed changes

lyndsiWilliams added 4 commits November 22, 2022 10:11

Remove console.log

01f4ffa

Change banner text to be more general and not include Preset or Superset

f4a42c1

Fix DatasetPanel test

d77263d

Fixed spelling of noreferrer

0bed85a

Fix loader height so no vertical scrollbar shows

5a3f188

eric-briscoe reviewed Dec 2, 2022

View reviewed changes

Implement Eric's scrollbar fix

11e2409

eric-briscoe approved these changes Dec 5, 2022

View reviewed changes

eschutho approved these changes Dec 5, 2022

View reviewed changes

lyndsiWilliams merged commit 04b7a26 into master Dec 5, 2022

lyndsiWilliams deleted the lyndsi/tables-with-preexisting-datasets branch December 5, 2022 21:43

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

		}

		const renderExistingDatasetAlert = (linkedDataset: any) => (

		@@ -19,10 +19,12 @@
		import React from 'react';
		import { supersetTheme, t, styled } from '@superset-ui/core';

	{datasetNames?.includes(tableName) &&
	renderExistingDatasetAlert(
	datasets?.find(dataset => dataset.table_name === tableName),
	)}

	onClick={() => {
	window.open(
	dataset?.explore_url,
	'_blank',
	'noreferrer noopener popup=false',
	);
	}}

feat: Flow for tables that already have a dataset #22136

feat: Flow for tables that already have a dataset #22136

Conversation

lyndsiWilliams commented Nov 16, 2022

SUMMARY

ANIMATED GIF / SCREENSHOT

Screenshot of selected table with existing dataset

Left panel warning icons

"View dataset"

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Nov 16, 2022 • edited Loading

Codecov Report

lyndsiWilliams commented Nov 16, 2022

github-actions bot commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-briscoe left a 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

eric-briscoe Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-briscoe left a 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

EugeneTorap commented Nov 18, 2022 • edited Loading

lyndsiWilliams commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyndsiWilliams Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyndsiWilliams Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAfghahi commented Nov 22, 2022

github-actions bot commented Nov 22, 2022

lyndsiWilliams commented Nov 22, 2022

github-actions bot commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-briscoe left a comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2022

codecov bot commented Nov 16, 2022 •

edited

Loading

eric-briscoe Nov 16, 2022 •

edited

Loading

eschutho Nov 18, 2022 •

edited

Loading

EugeneTorap commented Nov 18, 2022 •

edited

Loading

lyndsiWilliams Nov 22, 2022 •

edited

Loading

lyndsiWilliams Nov 22, 2022 •

edited

Loading