feat: Flow for tables that already have a dataset #22136
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
/testenv up |
|
@lyndsiWilliams Ephemeral environment spinning up at http://35.85.146.196:8080. Credentials are |
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const renderExistingDatasetAlert = (linkedDataset: any) => ( |
There was a problem hiding this comment.
Can we put a type on this linkeddataset?
There was a problem hiding this comment.
Oops, good catch! Typed in this commit.
| message={t('This table already has a dataset')} | ||
| description={ | ||
| <> | ||
| {t( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -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 | |||
| .catch(e => { | ||
| console.log('error', e); | ||
| }); | ||
| .catch(error => console.log('There was an error fetching tables', error)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I actually didn't know about this logging utility, thanks for the info! Changed the catch in this commit.
| ); | ||
| }) | ||
| .catch(error => | ||
| console.log('There was an error fetching dataset', error), |
There was a problem hiding this comment.
Please see other comment about using logging utility
There was a problem hiding this comment.
Changed the catch in this commit.
| @@ -113,7 +115,11 @@ function Footer({ | |||
| <Button onClick={cancelButtonOnClick}>Cancel</Button> | |||
| <Button | |||
| buttonStyle="primary" | |||
| disabled={!datasetObject?.table_name || !hasColumns} | |||
| disabled={ | |||
There was a problem hiding this comment.
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}
>
There was a problem hiding this comment.
Oh yeah good call, much cleaner! Changed in this commit
| }; | ||
|
|
||
| useEffect(() => { | ||
| if (dataset?.schema) getDatasetsList(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
agree on the lint rule as a follow up +1
There was a problem hiding this comment.
Oh wow I didn't realize this was so risky! I added brackets in this commit and will avoid this in the future.
| title={tableName || ''} | ||
| > | ||
| {tableName && ( | ||
| <Icons.Table iconColor={supersetTheme.colors.grayscale.base} /> |
There was a problem hiding this comment.
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} />`
There was a problem hiding this comment.
FIxed in this commit.
| @@ -19,10 +19,12 @@ | |||
| import React from 'react'; | |||
| import { supersetTheme, t, styled } from '@superset-ui/core'; | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
FIxed in this commit.
| @@ -123,6 +128,24 @@ const StyledTable = styled(Table)` | |||
| right: 0; | |||
| `; | |||
|
|
|||
| const StyledAlert = styled(Alert)` | |||
There was a problem hiding this comment.
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};
There was a problem hiding this comment.
FIxed in this commit.
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx
Outdated
Show resolved
Hide resolved
| }) | ||
| : undefined; | ||
|
|
||
| const getDatasetsList = () => { |
There was a problem hiding this comment.
I'd recommend moving any api calls outside of the view component into either a hook or redux action for separation of concerns.
There was a problem hiding this comment.
Moved the API call into a hook in this commit.
|
@eschutho @eric-briscoe @lyndsiWilliams I have some nice suggestions!
What do you think about it? |
Hey @EugeneTorap!
|
| @@ -168,15 +202,52 @@ export interface IDatasetPanelProps { | |||
| * Boolean indicating if the component is in a loading state | |||
| */ | |||
| loading: boolean; | |||
| datasets?: DatasetObject[] | undefined; | |||
There was a problem hiding this comment.
nit: I kind of want this to have a better name, maybe existingDatasets? or something along those lines.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
And when it's passed into renderExistingDatasetAlert() we need to be able to pull out the explore_url from the dataset object here:
| } | ||
|
|
||
| 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', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That makes sense, but I'm not sure about Preset-specific language here. I think we'd need @yousoph 's input on this one.
There was a problem hiding this comment.
Updated text: This table already has a dataset associated with it. You can only associate one dataset with a table.
There was a problem hiding this comment.
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.
|
/testenv up |
|
@AAfghahi Ephemeral environment spinning up at http://52.42.126.171:8080. Credentials are |
|
/testenv up |
|
@lyndsiWilliams Ephemeral environment spinning up at http://54.202.36.48:8080. Credentials are |
| @@ -94,7 +94,7 @@ const LoaderContainer = styled.div` | |||
| display: flex; | |||
| align-items: center; | |||
| justify-content: center; | |||
| height: 100%; | |||
| height: 98%; | |||
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Implemented the border-box fix we discussed on Slack in this commit.
There was a problem hiding this comment.
@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!
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
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.tsxfile 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
http://localhost:9000/dataset/add/?testingADDITIONAL INFORMATION