You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "lyndsiWilliams (via GitHub)" <gi...@apache.org> on 2023/01/24 00:36:38 UTC
[GitHub] [superset] lyndsiWilliams opened a new pull request, #22835: feat: Enable new dataset creation flow II
lyndsiWilliams opened a new pull request, #22835:
URL: https://github.com/apache/superset/pull/22835
<!---
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 create chart view when the create dataset button is clicked.
This PR also fixes all the issues introduced in the original iteration of this PR: #22610
### 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 create chart view with the newly created dataset selected
### 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
- [x] 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] codecov[bot] commented on pull request #22835: feat: Enable new dataset creation flow II
Posted by codecov.
codecov[bot] commented on PR #22835:
URL: https://github.com/apache/superset/pull/22835#issuecomment-1401212475
# [Codecov](https://codecov.io/gh/apache/superset/pull/22835?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 [#22835](https://codecov.io/gh/apache/superset/pull/22835?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92c7da4) into [master](https://codecov.io/gh/apache/superset/commit/577ac81686a9bf2074320e2eeb84bfa4d5562822?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (577ac81) will **decrease** coverage by `11.67%`.
> The diff coverage is `70.45%`.
```diff
@@ Coverage Diff @@
## master #22835 +/- ##
===========================================
- Coverage 67.29% 55.63% -11.67%
===========================================
Files 1877 1877
Lines 71939 73097 +1158
Branches 7877 7877
===========================================
- Hits 48412 40666 -7746
- Misses 21499 30403 +8904
Partials 2028 2028
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.50% <0.00%> (+<0.01%)` | :arrow_up: |
| python | `57.49% <0.00%> (-24.40%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.37% <0.00%> (-0.17%)` | :arrow_down: |
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/22835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ols/DndColumnSelectControl/ColumnSelectPopover.tsx](https://codecov.io/gh/apache/superset/pull/22835?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%> (ø)` | |
| [...uperset-frontend/src/pages/ChartCreation/index.tsx](https://codecov.io/gh/apache/superset/pull/22835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3BhZ2VzL0NoYXJ0Q3JlYXRpb24vaW5kZXgudHN4) | `61.11% <ø> (ø)` | |
| [...D/data/dataset/AddDataset/DatasetPanel/fixtures.ts](https://codecov.io/gh/apache/superset/pull/22835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvRGF0YXNldFBhbmVsL2ZpeHR1cmVzLnRz) | `100.00% <ø> (ø)` | |
| [...RUD/data/dataset/AddDataset/DatasetPanel/index.tsx](https://codecov.io/gh/apache/superset/pull/22835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvRGF0YXNldFBhbmVsL2luZGV4LnRzeA==) | `29.03% <0.00%> (ø)` | |
| [...ontend/src/views/CRUD/data/dataset/DatasetList.tsx](https://codecov.io/gh/apache/superset/pull/22835?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% <ø> (ø)` | |
| [superset-frontend/src/views/CRUD/data/hooks.ts](https://codecov.io/gh/apache/superset/pull/22835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9ob29rcy50cw==) | `57.69% <0.00%> (ø)` | |
| [...perset-frontend/src/views/components/RightMenu.tsx](https://codecov.io/gh/apache/superset/pull/22835?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==) | `77.48% <ø> (ø)` | |
| [superset/views/datasource/views.py](https://codecov.io/gh/apache/superset/pull/22835?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==) | `52.84% <0.00%> (-38.43%)` | :arrow_down: |
| [...d/src/views/CRUD/data/dataset/AddDataset/index.tsx](https://codecov.io/gh/apache/superset/pull/22835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvaW5kZXgudHN4) | `51.61% <66.66%> (ø)` | |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/22835?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==) | `44.46% <75.00%> (ø)` | |
| ... and [318 more](https://codecov.io/gh/apache/superset/pull/22835?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
[GitHub] [superset] yousoph commented on pull request #22835: feat: Enable new dataset creation flow II
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #22835:
URL: https://github.com/apache/superset/pull/22835#issuecomment-1407186517
/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 #22835: feat: Enable new dataset creation flow II
Posted by "lyndsiWilliams (via GitHub)" <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22835:
URL: https://github.com/apache/superset/pull/22835#discussion_r1092554581
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,13 +100,13 @@ 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();
+ history.push(`/chart/add/?dataset=${datasetObject.table_name}`);
}
});
}
};
- const CREATE_DATASET_TEXT = t('Create Dataset');
+ const CREATE_DATASET_TEXT = t('Create Dataset and Create Chart');
Review Comment:
Good thinking on the text, and good catch on the tooltip in the footer! Fixed in [`this commit`](https://github.com/apache/superset/pull/22835/commits/76b3590962bea5fd457cb8bf790ad811d147c316).
--
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 #22835: feat: Enable new dataset creation flow II
Posted by "lyndsiWilliams (via GitHub)" <gi...@apache.org>.
lyndsiWilliams merged PR #22835:
URL: https://github.com/apache/superset/pull/22835
--
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 #22835: feat: Enable new dataset creation flow II
Posted by "codyml (via GitHub)" <gi...@apache.org>.
codyml commented on code in PR #22835:
URL: https://github.com/apache/superset/pull/22835#discussion_r1092357804
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -640,6 +642,14 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onHide();
};
+ const redirectURL = (url: string) => {
Review Comment:
Would you mind adding a `TODO` comment noting that this check and passing `history` as a prop can be removed once SQL Lab is in the SPA, as `history` will then be globally available? Assuming that describes the situation correctly.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,13 +100,13 @@ 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();
+ history.push(`/chart/add/?dataset=${datasetObject.table_name}`);
}
});
}
};
- const CREATE_DATASET_TEXT = t('Create Dataset');
+ const CREATE_DATASET_TEXT = t('Create Dataset and Create Chart');
Review Comment:
Even though this appears in all caps, can we make it sentence case in the code for consistency and in case that CSS style ever gets changed?
```suggestion
const CREATE_DATASET_TEXT = t('Create dataset and create chart');
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,13 +100,13 @@ 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();
+ history.push(`/chart/add/?dataset=${datasetObject.table_name}`);
}
});
}
};
- const CREATE_DATASET_TEXT = t('Create Dataset');
+ const CREATE_DATASET_TEXT = t('Create Dataset and Create Chart');
Review Comment:
Also, I noticed that the tooltip nub is covered by the footer element when the button is disabled. A quick fix if it doesn't break anything else would be to add `z-index: 0;` to the `FooterRow` styles in `styles.ts`.
--
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 #22835: feat: Enable new dataset creation flow II
Posted by "lyndsiWilliams (via GitHub)" <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22835:
URL: https://github.com/apache/superset/pull/22835#discussion_r1092554797
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -640,6 +642,14 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onHide();
};
+ const redirectURL = (url: string) => {
Review Comment:
Good idea! Added the comment in [`this commit`](https://github.com/apache/superset/pull/22835/commits/76b3590962bea5fd457cb8bf790ad811d147c316).
--
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 #22835: feat: Enable new dataset creation flow II
Posted by github-actions.
github-actions[bot] commented on PR #22835:
URL: https://github.com/apache/superset/pull/22835#issuecomment-1407187437
@yousoph Ephemeral environment spinning up at http://54.186.237.99: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] github-actions[bot] commented on pull request #22835: feat: Enable new dataset creation flow II
Posted by github-actions.
github-actions[bot] commented on PR #22835:
URL: https://github.com/apache/superset/pull/22835#issuecomment-1412283796
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