You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2023/01/05 22:20:08 UTC
[GitHub] [superset] lyndsiWilliams opened a new pull request, #22610: feat: Enable new dataset creation flow
lyndsiWilliams opened a new pull request, #22610:
URL: https://github.com/apache/superset/pull/22610
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR enables the new create dataset flow in dataset view, the `+ -> Data` menu in the app header, chart creation, and database creation. On the create dataset page (`/dataset/add/`), user is brought to their previous page when the cancel button is clicked and the dataset list view when the create dataset button is clicked.
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- Check all areas in Superset where user can create a dataset, it should bring you to the new create dataset page: `/dataset/add`
- All creation spots: `+ Dataset` button in dataset view, the `+ -> Data` menu in the app header, chart creation, and database creation.
- There should no longer be an Add Dataset modal in any area of the app
- On the new create dataset page, ensure that:
- Cancel button brings you to the previous page you were on
- Create Dataset button brings you to the dataset list view, showing the newly created dataset
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1063399952
##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -296,10 +293,6 @@ const RightMenu = ({
setShowDatabaseModal(false);
};
- const handleOnHideDatasetModalModal = () => {
Review Comment:
I think this is another situation where previously users were supposed to be able to create a dataset in the middle of doing something else – if we could maintain that behavior it would be awesome. I just checked though and I think the current implementation is broken: it seem to always redirect to the add chart page on save no matter what 🤪
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1064853186
##########
superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx:
##########
@@ -603,34 +590,16 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
});
}
- const CREATE_HASH = '#create';
- const location = useLocation();
- const history = useHistory();
-
- // Sync Dataset Add modal with #create hash
- useEffect(() => {
- const modalOpen = location.hash === CREATE_HASH && canCreate;
- setDatasetAddModalOpen(modalOpen);
- }, [canCreate, location.hash]);
-
- // Add #create hash
- const openDatasetAddModal = useCallback(() => {
- history.replace(`${location.pathname}${location.search}${CREATE_HASH}`);
- }, [history, location.pathname, location.search]);
-
- // Remove #create hash
- const closeDatasetAddModal = useCallback(() => {
- history.replace(`${location.pathname}${location.search}`);
- }, [history, location.pathname, location.search]);
-
if (canCreate) {
buttonArr.push({
name: (
<>
<i className="fa fa-plus" /> {t('Dataset')}{' '}
</>
),
- onClick: openDatasetAddModal,
+ onClick: () => {
+ window.location.href = '/dataset/add/';
Review Comment:
Yeah that works! Changed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/99751f95d12f2c3fb6266ddf4492a2270e10e738).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1063574041
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,7 +100,8 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
- goToPreviousUrl();
+ window.location.href =
Review Comment:
I feel like when you finish creating a dataset, I think that the figma had you going to the edit flow. Which is obviously not created yet, maybe for the time being it should redirect you back to where you where?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1386363349
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1386184580
> Hi, found an issues, an error occurs when you go to the "create dataset" page after you have deleted the newly connected database, also, the deleted database is displayed in the dropdown list
>
> error.message.mp4
Hey @andrey-zayats , thanks for the review! I've just pushed up fixes for this issue and the "already existing" issue ([here](https://github.com/apache/superset/pull/22610#issuecomment-1386027957)). The other issues (slow-loading SQL Lab and disappearing top nav in SQL Lab) are unrelated to my changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1065051402
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -373,7 +373,9 @@ export default function DataSourcePanel({
<span
role="button"
tabIndex={0}
- onClick={() => setShowSaveDatasetModal(true)}
+ onClick={() => {
Review Comment:
Confirmed: virtual only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] andrey-zayats commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
andrey-zayats commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1385940520
SQL lab page takes forever to load when navigating to it via the top nav bar
https://user-images.githubusercontent.com/90777564/212994093-42a2e117-d105-4367-be1b-0b3a4e9eaf51.mp4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1067442874
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
+ }
+ }, []);
Review Comment:
I'm getting a missing dependency lint warning here for `setDatabase`. As I understand it, missing dependencies in hook dependency arrays always means the code is dangerous even if it works in the moment ([here's one reference](https://github.com/facebook/create-react-app/issues/6880#issuecomment-485912528)), so I'd always recommend jumping through whatever hoops React tells you to to make those warnings go away. Looks like in this case you may need to add a `useCallback` on `setDatabase` to keep it stable.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
+ }
+ }, []);
+
useEffect(() => {
Review Comment:
I'm also getting a missing dependency error in the dependency array of this effect, but since this wasn't changed in this PR, up to you if you want to fix it here.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -192,9 +193,10 @@ export default function LeftPanel({
setResetTables(false);
setRefresh(false);
})
- .catch(error =>
- logging.error('There was an error fetching tables', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching tables');
Review Comment:
```suggestion
addDangerToast(t('There was an error fetching tables'));
```
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1231,7 +1231,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onClick={() => {
setLoading(true);
fetchAndSetDB();
- window.location.href = '/tablemodelview/list#create';
+ window.location.href = '/dataset/add/';
Review Comment:
```suggestion
history.push('/dataset/add/');
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -94,8 +95,10 @@ const DatasetPanelWrapper = ({
setColumnList([]);
setHasColumns?.(false);
setHasError(true);
- // eslint-disable-next-line no-console
- console.error(
+ addDangerToast(
+ `The API response from ${path} does not match the IDatabaseTable interface.`,
Review Comment:
```suggestion
t('The API response from %s does not match the IDatabaseTable interface.', path),
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -96,9 +97,10 @@ export default function AddDataset() {
.then(json => {
setDatasets(json?.result);
})
- .catch(error =>
- logging.error('There was an error fetching dataset', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching dataset');
Review Comment:
```suggestion
addDangerToast(t('There was an error fetching dataset'));
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
Review Comment:
I tried clicking the "Create Dataset" button after creating a database and wasn't able to get the database to auto-populate on my machine – is it working on yours? Haven't looked deeply into it, but I think the problem might be that even though this effect calls `setDatabase` correctly to update the selected DB in the `dataset` state object of the parent `AddDataset` component, the selected DB isn't sent back down to the child `DatabaseSelector` component so it never updates. To fix this, you may need to pass more of the database object (or the whole dataset object) through `LeftPanel` so you can send it to `DatabaseSelector`.
https://user-images.githubusercontent.com/13007381/211922005-11e9c9b0-d3ba-41d4-9991-8bb125acdd50.mov
##########
superset-frontend/src/views/CRUD/data/hooks.ts:
##########
@@ -80,6 +81,7 @@ export const UseGetDatasetsList = (queryParams: string | undefined) =>
endpoint: `/api/v1/dataset/?q=${queryParams}`,
})
.then(({ json }) => json)
- .catch(error =>
- logging.error('There was an error fetching dataset', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching dataset');
Review Comment:
```suggestion
addDangerToast(t('There was an error fetching dataset'));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1382397114
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1063891312
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,7 +100,8 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
- goToPreviousUrl();
+ window.location.href =
Review Comment:
Oops sorry, I missed that in the designs – if we're intentionally changing that flow so it always ends you at the Edit Dataset view no matter where the user started then please disregard. I'm fine with how it is now (redirect to the dataset list) as a temporary solution but @AAfghahi's suggestion works too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068552219
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
+ }
+ }, []);
+
useEffect(() => {
Review Comment:
I fixed it anyways! 😁 Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1063399952
##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -296,10 +293,6 @@ const RightMenu = ({
setShowDatabaseModal(false);
};
- const handleOnHideDatasetModalModal = () => {
Review Comment:
I think this is another situation where previously users were supposed to be able to create a dataset in the middle of doing something else – if we could maintain that behavior it would be awesome. I just checked though and I think the current implementation is broken: it seems to always redirect to the add chart page on save no matter what 🤪
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1064812755
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,7 +100,8 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
- goToPreviousUrl();
+ window.location.href =
Review Comment:
I think originally we were rerouting to the dataset list so the user can see the dataset they just created. I'm gonna bring in @yousoph for this one, should the user be redirected to the dataset list or their previous page after creating a dataset?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1064832540
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1231,7 +1231,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onClick={() => {
setLoading(true);
fetchAndSetDB();
- window.location.href = '/tablemodelview/list#create';
+ window.location.href = '/dataset/add/';
Review Comment:
I think it should still do that, right? This only changes where the user is redirected when they click "CREATE DATASET" on the final step of the create database modal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1063388821
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -231,7 +231,7 @@ const ColumnSelectPopover = ({
}, []);
const setDatasetAndClose = () => {
- if (setDatasetModal) setDatasetModal(true);
+ window.location.href = '/dataset/add/';
Review Comment:
Ditto above about this being a virtual dataset creation action.
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1231,7 +1231,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onClick={() => {
setLoading(true);
fetchAndSetDB();
- window.location.href = '/tablemodelview/list#create';
+ window.location.href = '/dataset/add/';
Review Comment:
The modal would auto-populate the database field with whatever database was just created – do we want to maintain that behavior?
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,7 +100,8 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
- goToPreviousUrl();
+ window.location.href =
Review Comment:
Previously, users could create a dataset as part of the adding a chart flow and still be on that page when the modal was closed, but with this change they'll need to re-navigate there. This feels like maybe too big of a UX regression to me – could we implement a more flexible system for redirecting on submit/cancel?
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx:
##########
@@ -388,7 +388,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
tabIndex={0}
role="button"
onClick={() => {
- this.props.handleDatasetModal(true);
+ window.location.href = '/dataset/add/';
Review Comment:
Ditto above about this being a virtual dataset creation action.
##########
superset-frontend/src/explore/components/ExploreChartPanel.jsx:
##########
@@ -324,7 +324,9 @@ const ExploreChartPanel = ({
<span
role="button"
tabIndex={0}
- onClick={() => setShowDatasetModal(true)}
+ onClick={() => {
Review Comment:
Ditto above about this being a virtual dataset creation action.
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -373,7 +373,9 @@ export default function DataSourcePanel({
<span
role="button"
tabIndex={0}
- onClick={() => setShowSaveDatasetModal(true)}
+ onClick={() => {
Review Comment:
Isn't the Add Dataset page only for adding physical datasets, and this modal is for creating a virtual dataset from a query? If so, should it be left as-is?
##########
superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx:
##########
@@ -603,34 +590,16 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
});
}
- const CREATE_HASH = '#create';
- const location = useLocation();
- const history = useHistory();
-
- // Sync Dataset Add modal with #create hash
- useEffect(() => {
- const modalOpen = location.hash === CREATE_HASH && canCreate;
- setDatasetAddModalOpen(modalOpen);
- }, [canCreate, location.hash]);
-
- // Add #create hash
- const openDatasetAddModal = useCallback(() => {
- history.replace(`${location.pathname}${location.search}${CREATE_HASH}`);
- }, [history, location.pathname, location.search]);
-
- // Remove #create hash
- const closeDatasetAddModal = useCallback(() => {
- history.replace(`${location.pathname}${location.search}`);
- }, [history, location.pathname, location.search]);
-
if (canCreate) {
buttonArr.push({
name: (
<>
<i className="fa fa-plus" /> {t('Dataset')}{' '}
</>
),
- onClick: openDatasetAddModal,
+ onClick: () => {
+ window.location.href = '/dataset/add/';
Review Comment:
Unless it doesn't work here, could we use the `useHistory` hook and call `history.push()` here instead of directly mutating the global `location` object, just to keep API usage consistent?
##########
superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx:
##########
@@ -1,172 +0,0 @@
-/**
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-import React, { FunctionComponent, useState, useEffect } from 'react';
-import { styled, t } from '@superset-ui/core';
-import { useSingleViewResource } from 'src/views/CRUD/hooks';
-import Modal from 'src/components/Modal';
-import TableSelector from 'src/components/TableSelector';
-import withToasts from 'src/components/MessageToasts/withToasts';
-import { DatabaseObject } from 'src/components/DatabaseSelector';
-import {
- getItem,
- LocalStorageKeys,
- setItem,
-} from 'src/utils/localStorageHelpers';
-import { isEmpty } from 'lodash';
-
-type DatasetAddObject = {
- id: number;
- database: number;
- schema: string;
- table_name: string;
-};
-interface DatasetModalProps {
- addDangerToast: (msg: string) => void;
- addSuccessToast: (msg: string) => void;
- onDatasetAdd?: (dataset: DatasetAddObject) => void;
- onHide: () => void;
- show: boolean;
- history?: any; // So we can render the modal when not using SPA
-}
-
-const TableSelectorContainer = styled.div`
- padding-bottom: 340px;
- width: 65%;
-`;
-
-const DatasetModal: FunctionComponent<DatasetModalProps> = ({
- addDangerToast,
- onDatasetAdd,
- onHide,
- show,
- history,
-}) => {
- const [currentDatabase, setCurrentDatabase] = useState<
- DatabaseObject | undefined
- >();
- const [currentSchema, setSchema] = useState<string | undefined>('');
- const [currentTableName, setTableName] = useState('');
- const [disableSave, setDisableSave] = useState(true);
- const {
- createResource,
- state: { loading },
- } = useSingleViewResource<Partial<DatasetAddObject>>(
- 'dataset',
- t('dataset'),
- addDangerToast,
- );
-
- useEffect(() => {
- setDisableSave(currentDatabase === undefined || currentTableName === '');
- }, [currentTableName, currentDatabase]);
-
- useEffect(() => {
- const currentUserSelectedDb = getItem(
Review Comment:
I think this is the system previously used to track the selected DB. Could we use something similar in the new views, or maybe a query param or history state?
##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -296,10 +293,6 @@ const RightMenu = ({
setShowDatabaseModal(false);
};
- const handleOnHideDatasetModalModal = () => {
Review Comment:
I think this is another situation where previously users were supposed to be able to create a dataset in the middle of doing something else – if we could maintain that behavior it would be awesome. I just checked though and I think the current implementation is broken: it seem to always redirects to the add chart page no matter what 🤪
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1064851374
##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -296,10 +293,6 @@ const RightMenu = ({
setShowDatabaseModal(false);
};
- const handleOnHideDatasetModalModal = () => {
Review Comment:
The plans are to remove the old modal entirely, what other flow is this used in? Definitely could have missed some spots but I'd need to discuss with design since we won't be using the old modal anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1065055249
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -231,7 +231,7 @@ const ColumnSelectPopover = ({
}, []);
const setDatasetAndClose = () => {
- if (setDatasetModal) setDatasetModal(true);
+ window.location.href = '/dataset/add/';
Review Comment:
Confirmed: virtual only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1381165299
> One (last, I swear) thing I just noticed is that in the previous modal, saving a dataset would redirect you to `/chart/add` with a `?dataset=table_name` query string, which would autoselect the just-created dataset and also show a toast on the Add Chart page. If this behavior is easy to reproduce it should probably be maintained, but feel free to do it in a separate PR. Database autoselect now works great and everything else LGTM!
Easy fix, thanks for that catch! Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/5941d037e9c2a7958a3d1d6f82827ab4ce3c9143).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] EugeneTorap commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1387055453
Hi @lyndsiWilliams! Can you move `DatasetCreation` and `DatabaseList` to `src/pages` folder?
#22330 - src/pages structure proposal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1065040873
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,7 +100,8 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
- goToPreviousUrl();
+ window.location.href =
Review Comment:
Confirmed from @yousoph: for this step, redirect to Add Chart page on successful dataset create. May possibly change button text as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1063390181
##########
superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx:
##########
@@ -1,172 +0,0 @@
-/**
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-import React, { FunctionComponent, useState, useEffect } from 'react';
-import { styled, t } from '@superset-ui/core';
-import { useSingleViewResource } from 'src/views/CRUD/hooks';
-import Modal from 'src/components/Modal';
-import TableSelector from 'src/components/TableSelector';
-import withToasts from 'src/components/MessageToasts/withToasts';
-import { DatabaseObject } from 'src/components/DatabaseSelector';
-import {
- getItem,
- LocalStorageKeys,
- setItem,
-} from 'src/utils/localStorageHelpers';
-import { isEmpty } from 'lodash';
-
-type DatasetAddObject = {
- id: number;
- database: number;
- schema: string;
- table_name: string;
-};
-interface DatasetModalProps {
- addDangerToast: (msg: string) => void;
- addSuccessToast: (msg: string) => void;
- onDatasetAdd?: (dataset: DatasetAddObject) => void;
- onHide: () => void;
- show: boolean;
- history?: any; // So we can render the modal when not using SPA
-}
-
-const TableSelectorContainer = styled.div`
- padding-bottom: 340px;
- width: 65%;
-`;
-
-const DatasetModal: FunctionComponent<DatasetModalProps> = ({
- addDangerToast,
- onDatasetAdd,
- onHide,
- show,
- history,
-}) => {
- const [currentDatabase, setCurrentDatabase] = useState<
- DatabaseObject | undefined
- >();
- const [currentSchema, setSchema] = useState<string | undefined>('');
- const [currentTableName, setTableName] = useState('');
- const [disableSave, setDisableSave] = useState(true);
- const {
- createResource,
- state: { loading },
- } = useSingleViewResource<Partial<DatasetAddObject>>(
- 'dataset',
- t('dataset'),
- addDangerToast,
- );
-
- useEffect(() => {
- setDisableSave(currentDatabase === undefined || currentTableName === '');
- }, [currentTableName, currentDatabase]);
-
- useEffect(() => {
- const currentUserSelectedDb = getItem(
Review Comment:
I think this is how we currently track the selected DB. Could we use something similar in the new views, or maybe a query param or history state?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1065055639
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx:
##########
@@ -388,7 +388,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
tabIndex={0}
role="button"
onClick={() => {
- this.props.handleDatasetModal(true);
+ window.location.href = '/dataset/add/';
Review Comment:
Confirmed: virtual only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1065046088
##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -296,10 +293,6 @@ const RightMenu = ({
setShowDatabaseModal(false);
};
- const handleOnHideDatasetModalModal = () => {
Review Comment:
Resolving because it will always redirect to Add Chart in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068756176
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -94,8 +95,10 @@ const DatasetPanelWrapper = ({
setColumnList([]);
setHasColumns?.(false);
setHasError(true);
- // eslint-disable-next-line no-console
- console.error(
+ addDangerToast(
+ `The API response from ${path} does not match the IDatabaseTable interface.`,
Review Comment:
Ohhh nice! Good to know, and much cleaner! Thanks! 😁
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1381311105
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068551394
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
+ }
+ }, []);
Review Comment:
Thanks for explaining this to me! I didn't realize the warnings were that dangerous, I'll watch out for these going forward. Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams merged pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams merged PR #22610:
URL: https://github.com/apache/superset/pull/22610
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1387707870
> Hi @lyndsiWilliams! Can you move `DatasetCreation` and `DatabaseList` to `src/pages` folder? #22330 - src/pages structure proposal
Can do! I'll be moving all dataset files to the new structure within the next couple of weeks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] andrey-zayats commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
andrey-zayats commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1386027957
after we have created the dataset, it should be marked as "already exists" and the button should be disabled on "create dataset" page, but this does not happen
Repro steps:
1. Go to "Datasets" -> "+Dataset"
2. Select examples db, select main schema
3. Select a database table and click on "create dataset and create chart" button
4. Select the same database, schema and table
5. Ensure that dataset is marked as "already exists" and the button is disabled
https://user-images.githubusercontent.com/90777564/213008530-39dfd867-c4f7-48b5-8832-3fcd0841eef3.mp4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1064844436
##########
superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx:
##########
@@ -1,172 +0,0 @@
-/**
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-import React, { FunctionComponent, useState, useEffect } from 'react';
-import { styled, t } from '@superset-ui/core';
-import { useSingleViewResource } from 'src/views/CRUD/hooks';
-import Modal from 'src/components/Modal';
-import TableSelector from 'src/components/TableSelector';
-import withToasts from 'src/components/MessageToasts/withToasts';
-import { DatabaseObject } from 'src/components/DatabaseSelector';
-import {
- getItem,
- LocalStorageKeys,
- setItem,
-} from 'src/utils/localStorageHelpers';
-import { isEmpty } from 'lodash';
-
-type DatasetAddObject = {
- id: number;
- database: number;
- schema: string;
- table_name: string;
-};
-interface DatasetModalProps {
- addDangerToast: (msg: string) => void;
- addSuccessToast: (msg: string) => void;
- onDatasetAdd?: (dataset: DatasetAddObject) => void;
- onHide: () => void;
- show: boolean;
- history?: any; // So we can render the modal when not using SPA
-}
-
-const TableSelectorContainer = styled.div`
- padding-bottom: 340px;
- width: 65%;
-`;
-
-const DatasetModal: FunctionComponent<DatasetModalProps> = ({
- addDangerToast,
- onDatasetAdd,
- onHide,
- show,
- history,
-}) => {
- const [currentDatabase, setCurrentDatabase] = useState<
- DatabaseObject | undefined
- >();
- const [currentSchema, setSchema] = useState<string | undefined>('');
- const [currentTableName, setTableName] = useState('');
- const [disableSave, setDisableSave] = useState(true);
- const {
- createResource,
- state: { loading },
- } = useSingleViewResource<Partial<DatasetAddObject>>(
- 'dataset',
- t('dataset'),
- addDangerToast,
- );
-
- useEffect(() => {
- setDisableSave(currentDatabase === undefined || currentTableName === '');
- }, [currentTableName, currentDatabase]);
-
- useEffect(() => {
- const currentUserSelectedDb = getItem(
Review Comment:
We could definitely move that into the modal, what is this user for? I'm not exactly sure what this is doing but I can mirror the logic once I understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1064841948
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -373,7 +373,9 @@ export default function DataSourcePanel({
<span
role="button"
tabIndex={0}
- onClick={() => setShowSaveDatasetModal(true)}
+ onClick={() => {
Review Comment:
I saw your other comments about virtual dataset creation, are those all solely for virtual dataset creation or is it also for physical?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1065053740
##########
superset-frontend/src/explore/components/ExploreChartPanel.jsx:
##########
@@ -324,7 +324,9 @@ const ExploreChartPanel = ({
<span
role="button"
tabIndex={0}
- onClick={() => setShowDatasetModal(true)}
+ onClick={() => {
Review Comment:
Confirmed: virtual only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] andrey-zayats commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
andrey-zayats commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1385908014
Hi, found an issues, an error occurs when you go to the "create dataset" page after you have deleted the newly connected database, also, the deleted database is displayed in the dropdown list
https://user-images.githubusercontent.com/90777564/212987580-05683fb2-4f6a-4b6a-ab24-e42c77fc2c7e.mp4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068553583
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -192,9 +193,10 @@ export default function LeftPanel({
setResetTables(false);
setRefresh(false);
})
- .catch(error =>
- logging.error('There was an error fetching tables', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching tables');
Review Comment:
Good catch! Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63).
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -96,9 +97,10 @@ export default function AddDataset() {
.then(json => {
setDatasets(json?.result);
})
- .catch(error =>
- logging.error('There was an error fetching dataset', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching dataset');
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068553317
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -94,8 +95,10 @@ const DatasetPanelWrapper = ({
setColumnList([]);
setHasColumns?.(false);
setHasError(true);
- // eslint-disable-next-line no-console
- console.error(
+ addDangerToast(
+ `The API response from ${path} does not match the IDatabaseTable interface.`,
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63). Just out of curiosity, what does `%s` do here? Does it take the first variable it receives?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1067390133
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,7 +100,8 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
- goToPreviousUrl();
+ window.location.href =
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/d2903e307228270c40270088b7a83d64ae13e545).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1067390133
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,7 +100,8 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
- goToPreviousUrl();
+ window.location.href =
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/d2903e307228270c40270088b7a83d64ae13e545);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1065045492
##########
superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx:
##########
@@ -1,172 +0,0 @@
-/**
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-import React, { FunctionComponent, useState, useEffect } from 'react';
-import { styled, t } from '@superset-ui/core';
-import { useSingleViewResource } from 'src/views/CRUD/hooks';
-import Modal from 'src/components/Modal';
-import TableSelector from 'src/components/TableSelector';
-import withToasts from 'src/components/MessageToasts/withToasts';
-import { DatabaseObject } from 'src/components/DatabaseSelector';
-import {
- getItem,
- LocalStorageKeys,
- setItem,
-} from 'src/utils/localStorageHelpers';
-import { isEmpty } from 'lodash';
-
-type DatasetAddObject = {
- id: number;
- database: number;
- schema: string;
- table_name: string;
-};
-interface DatasetModalProps {
- addDangerToast: (msg: string) => void;
- addSuccessToast: (msg: string) => void;
- onDatasetAdd?: (dataset: DatasetAddObject) => void;
- onHide: () => void;
- show: boolean;
- history?: any; // So we can render the modal when not using SPA
-}
-
-const TableSelectorContainer = styled.div`
- padding-bottom: 340px;
- width: 65%;
-`;
-
-const DatasetModal: FunctionComponent<DatasetModalProps> = ({
- addDangerToast,
- onDatasetAdd,
- onHide,
- show,
- history,
-}) => {
- const [currentDatabase, setCurrentDatabase] = useState<
- DatabaseObject | undefined
- >();
- const [currentSchema, setSchema] = useState<string | undefined>('');
- const [currentTableName, setTableName] = useState('');
- const [disableSave, setDisableSave] = useState(true);
- const {
- createResource,
- state: { loading },
- } = useSingleViewResource<Partial<DatasetAddObject>>(
- 'dataset',
- t('dataset'),
- addDangerToast,
- );
-
- useEffect(() => {
- setDisableSave(currentDatabase === undefined || currentTableName === '');
- }, [currentTableName, currentDatabase]);
-
- useEffect(() => {
- const currentUserSelectedDb = getItem(
Review Comment:
Anywhere where that `LocalStorageKeys.db` key is saved should probably still work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1064841097
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -373,7 +373,9 @@ export default function DataSourcePanel({
<span
role="button"
tabIndex={0}
- onClick={() => setShowSaveDatasetModal(true)}
+ onClick={() => {
Review Comment:
Is this spot only for virtual datasets or is it also for physical? We currently don't have plans to add a virtual dataset from the new flow, the user can create them through the SQL Lab route by creating a chart from a query, then creating a dataset off of that in explore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068757410
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -94,8 +95,10 @@ const DatasetPanelWrapper = ({
setColumnList([]);
setHasColumns?.(false);
setHasError(true);
- // eslint-disable-next-line no-console
- console.error(
+ addDangerToast(
+ `The API response from ${path} does not match the IDatabaseTable interface.`,
Review Comment:
Also I just realized I forgot to add `path` at the end when I changed it, whoops! Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/09cc3bfa9638a3676f0be969d273105ecb954351).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068554617
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1231,7 +1231,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onClick={() => {
setLoading(true);
fetchAndSetDB();
- window.location.href = '/tablemodelview/list#create';
+ window.location.href = '/dataset/add/';
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63). I found another `window.location.href` a little further down so I changed that to use `history.push()` as well.
##########
superset-frontend/src/views/CRUD/data/hooks.ts:
##########
@@ -80,6 +81,7 @@ export const UseGetDatasetsList = (queryParams: string | undefined) =>
endpoint: `/api/v1/dataset/?q=${queryParams}`,
})
.then(({ json }) => json)
- .catch(error =>
- logging.error('There was an error fetching dataset', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching dataset');
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] andrey-zayats commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
andrey-zayats commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1385979801
Not sure if this is a bug, but if we have created a dataset, then we try to create the same one, it is not marked as already exists on the "create dataset" page (perhaps this is caused by the issue described above)
https://user-images.githubusercontent.com/90777564/212999526-2d93fe30-c721-4fb5-9896-ae071bf04c30.mp4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] andrey-zayats commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
andrey-zayats commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1385964152
Top nav bar disappears in SQL lab (the same in latest master)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1067390552
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -373,7 +373,9 @@ export default function DataSourcePanel({
<span
role="button"
tabIndex={0}
- onClick={() => setShowSaveDatasetModal(true)}
+ onClick={() => {
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/d2903e307228270c40270088b7a83d64ae13e545).
##########
superset-frontend/src/explore/components/ExploreChartPanel.jsx:
##########
@@ -324,7 +324,9 @@ const ExploreChartPanel = ({
<span
role="button"
tabIndex={0}
- onClick={() => setShowDatasetModal(true)}
+ onClick={() => {
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/d2903e307228270c40270088b7a83d64ae13e545).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068555114
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
Review Comment:
Thanks for pairing with me to fix this! Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/a72d05829371f6a375e165dd2d3739474587fa63).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1381311977
@lyndsiWilliams Ephemeral environment spinning up at http://18.237.97.3:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1068648724
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -94,8 +95,10 @@ const DatasetPanelWrapper = ({
setColumnList([]);
setHasColumns?.(false);
setHasError(true);
- // eslint-disable-next-line no-console
- console.error(
+ addDangerToast(
+ `The API response from ${path} does not match the IDatabaseTable interface.`,
Review Comment:
Yep! You can add multiple too, so you can do `t('First var is %s, second var is %s', firstVar, secondVar)`. I believe it comes from C's `printf` format string minilanguage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1067390692
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -231,7 +231,7 @@ const ColumnSelectPopover = ({
}, []);
const setDatasetAndClose = () => {
- if (setDatasetModal) setDatasetModal(true);
+ window.location.href = '/dataset/add/';
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/d2903e307228270c40270088b7a83d64ae13e545).
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx:
##########
@@ -388,7 +388,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
tabIndex={0}
role="button"
onClick={() => {
- this.props.handleDatasetModal(true);
+ window.location.href = '/dataset/add/';
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/d2903e307228270c40270088b7a83d64ae13e545).
##########
superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx:
##########
@@ -1,172 +0,0 @@
-/**
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-import React, { FunctionComponent, useState, useEffect } from 'react';
-import { styled, t } from '@superset-ui/core';
-import { useSingleViewResource } from 'src/views/CRUD/hooks';
-import Modal from 'src/components/Modal';
-import TableSelector from 'src/components/TableSelector';
-import withToasts from 'src/components/MessageToasts/withToasts';
-import { DatabaseObject } from 'src/components/DatabaseSelector';
-import {
- getItem,
- LocalStorageKeys,
- setItem,
-} from 'src/utils/localStorageHelpers';
-import { isEmpty } from 'lodash';
-
-type DatasetAddObject = {
- id: number;
- database: number;
- schema: string;
- table_name: string;
-};
-interface DatasetModalProps {
- addDangerToast: (msg: string) => void;
- addSuccessToast: (msg: string) => void;
- onDatasetAdd?: (dataset: DatasetAddObject) => void;
- onHide: () => void;
- show: boolean;
- history?: any; // So we can render the modal when not using SPA
-}
-
-const TableSelectorContainer = styled.div`
- padding-bottom: 340px;
- width: 65%;
-`;
-
-const DatasetModal: FunctionComponent<DatasetModalProps> = ({
- addDangerToast,
- onDatasetAdd,
- onHide,
- show,
- history,
-}) => {
- const [currentDatabase, setCurrentDatabase] = useState<
- DatabaseObject | undefined
- >();
- const [currentSchema, setSchema] = useState<string | undefined>('');
- const [currentTableName, setTableName] = useState('');
- const [disableSave, setDisableSave] = useState(true);
- const {
- createResource,
- state: { loading },
- } = useSingleViewResource<Partial<DatasetAddObject>>(
- 'dataset',
- t('dataset'),
- addDangerToast,
- );
-
- useEffect(() => {
- setDisableSave(currentDatabase === undefined || currentTableName === '');
- }, [currentTableName, currentDatabase]);
-
- useEffect(() => {
- const currentUserSelectedDb = getItem(
Review Comment:
Fixed in [`this commit`](https://github.com/apache/superset/pull/22610/commits/d2903e307228270c40270088b7a83d64ae13e545).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1382401293
@lyndsiWilliams Ephemeral environment spinning up at http://35.89.232.0:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1376230091
One more comment from @yousoph: errors in the new dataset flow should show up as toasts for now; will circle back if that should change for new dataset flow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22610: feat: Enable new dataset creation flow
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22610:
URL: https://github.com/apache/superset/pull/22610#issuecomment-1372868576
# [Codecov](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22610](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1460b78) into [master](https://codecov.io/gh/apache/superset/commit/3761694d72ba77332d9af68ec67fb178a25b1292?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3761694) will **increase** coverage by `0.00%`.
> The diff coverage is `25.00%`.
> :exclamation: Current head 1460b78 differs from pull request most recent head 0090c5b. Consider uploading reports for the commit 0090c5b to get more accurate results
```diff
@@ Coverage Diff @@
## master #22610 +/- ##
=======================================
Coverage 67.01% 67.02%
=======================================
Files 1859 1859
Lines 71036 71033 -3
Branches 7766 7766
=======================================
Hits 47608 47608
+ Misses 21406 21403 -3
Partials 2022 2022
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.48% <0.00%> (+<0.01%)` | :arrow_up: |
| mysql | `78.02% <0.00%> (+<0.01%)` | :arrow_up: |
| postgres | `78.09% <0.00%> (+<0.01%)` | :arrow_up: |
| presto | `52.37% <0.00%> (+<0.01%)` | :arrow_up: |
| python | `81.35% <0.00%> (+<0.01%)` | :arrow_up: |
| sqlite | `76.47% <0.00%> (+<0.01%)` | :arrow_up: |
| unit | `51.47% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...perset-frontend/src/addSlice/AddSliceContainer.tsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLnRzeA==) | `59.61% <ø> (ø)` | |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `69.73% <0.00%> (ø)` | |
| [...ols/DndColumnSelectControl/ColumnSelectPopover.tsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL0NvbHVtblNlbGVjdFBvcG92ZXIudHN4) | `3.37% <0.00%> (ø)` | |
| [...ols/MetricControl/AdhocMetricEditPopover/index.jsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9NZXRyaWNDb250cm9sL0FkaG9jTWV0cmljRWRpdFBvcG92ZXIvaW5kZXguanN4) | `79.78% <0.00%> (ø)` | |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `43.76% <0.00%> (ø)` | |
| [...ontend/src/views/CRUD/data/dataset/DatasetList.tsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0RhdGFzZXRMaXN0LnRzeA==) | `57.73% <ø> (ø)` | |
| [...perset-frontend/src/views/components/RightMenu.tsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvUmlnaHRNZW51LnRzeA==) | `78.23% <ø> (ø)` | |
| [superset/views/datasource/views.py](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS92aWV3cy5weQ==) | `93.49% <0.00%> (+2.22%)` | :arrow_up: |
| [...iews/CRUD/data/dataset/AddDataset/Footer/index.tsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvRm9vdGVyL2luZGV4LnRzeA==) | `39.39% <50.00%> (ø)` | |
| [...d/src/explore/components/DatasourcePanel/index.tsx](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhc291cmNlUGFuZWwvaW5kZXgudHN4) | `66.66% <100.00%> (ø)` | |
| ... and [6 more](https://codecov.io/gh/apache/superset/pull/22610?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org