-
Notifications
You must be signed in to change notification settings - Fork 13.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add certifiedby & certification details fields to the edit dataset columns fields #16454
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, thanks for extending this feature to apply to columns!
can you run @betodealmeida's migration perf testing script on this PR? unfortunately we're (at airbnb) still using MySQL and a column add will result in locking the table for us (and we have ~1.7 million rows in the table_colums table).
This PR will need a message in UPDATING.md, and folks on MySQL will either need to schedule downtime or use the percona toolkit (or similar) to perform the migration.
I'll review the rest of the JS/python once it's ready for review, but you'll want to add the documentation alongside here. I also don't know if a migration like this requires waiting for v1.4 or not... i hope not, but i don't remember the requirements. @eschutho could certainly comment
Codecov Report
@@ Coverage Diff @@
## master #16454 +/- ##
==========================================
- Coverage 76.99% 76.82% -0.17%
==========================================
Files 1007 1007
Lines 54180 54204 +24
Branches 7463 7473 +10
==========================================
- Hits 41715 41644 -71
- Misses 12225 12320 +95
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments, but only one major one.
Does this also need a change in superset-ui so that everywhere you select columns the certification (or warning) icon is displayed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM! Just some nits to follow our conventions
/testenv up |
@geido Ephemeral environment spinning up at http://34.220.226.194:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkdotson below my feedback after manual testing:
- Metrics use a tooltip for both fields. The tooltip is missing for both fields in Columns and Calculated columns. I think we should have it there too for consistency
- The certified icon should show also in the Dataset panel. However, this is happening only for the Metrics currently
Co-authored-by: Geido <[email protected]>
Co-authored-by: Geido <[email protected]>
Co-authored-by: Geido <[email protected]>
…dd-certified-columns
Hey @geido the change has already been made in superset-ui. I just pulled in the latest code should have change. I'll spin up new test env. |
❗ Please consider rebasing your branch to avoid db migration conflicts. |
/testenv up |
/testenv up |
@pkdotson Ephemeral environment spinning up at http://52.12.101.216:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love it! thanks for adding the Mixin and reusing code
@etr2460 thanks for the review. tested locally. certification feature is added column and calculated columns ✅
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see comment. 🙏
cc @jinghua-qa please add to our test case
/testenv up |
Fixes for editing in datasetlist: fixes alignment issues: |
/testenv up |
@pkdotson Ephemeral environment spinning up at http://54.245.12.35:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Product sign-off! thanks for the improvement!
Ephemeral environment shutdown and build artifacts deleted. |
…set columns fields (apache#16454) * add migration * add backend and frontend for certified * update migration with batch * fix integration test and update Updating.md * Update superset-frontend/src/datasource/DatasourceEditor.jsx Co-authored-by: Geido <[email protected]> * Update superset-frontend/src/datasource/DatasourceEditor.jsx Co-authored-by: Geido <[email protected]> * Update superset-frontend/src/datasource/DatasourceEditor.jsx Co-authored-by: Geido <[email protected]> * change method name * add tooltip info * add mixin * merge heads * address comments * fix select label styles * add extra field * fix test? * add extra field to put schema Co-authored-by: Geido <[email protected]>
…set columns fields (apache#16454) * add migration * add backend and frontend for certified * update migration with batch * fix integration test and update Updating.md * Update superset-frontend/src/datasource/DatasourceEditor.jsx Co-authored-by: Geido <[email protected]> * Update superset-frontend/src/datasource/DatasourceEditor.jsx Co-authored-by: Geido <[email protected]> * Update superset-frontend/src/datasource/DatasourceEditor.jsx Co-authored-by: Geido <[email protected]> * change method name * add tooltip info * add mixin * merge heads * address comments * fix select label styles * add extra field * fix test? * add extra field to put schema Co-authored-by: Geido <[email protected]>
SUMMARY
This Pr adds certifedby and certification details fields to the edit dataset columns and calculated columns fields.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2021-08-25.at.4.50.00.PM.mov
TESTING INSTRUCTIONS
Go to edit dataset and edit the certified by and certification details fields and saved modal. Modal should persist the fields on reload.
ADDITIONAL INFORMATION