Skip to content
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

Merged
merged 22 commits into from
Sep 22, 2021

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented Aug 25, 2021

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

  • [ x] Has associated issue: close: [editdataset]certify columns and calculated columns  #16349
  • Required feature flags:
  • Changes UI
  • [ x] Includes DB Migration (follow approval process in SIP-59)
    • [ x] Migration is atomic, supports rollback & is backwards-compatible
    • [x ] Confirm DB migration upgrade and downgrade tested
    • [x ] Runtime estimates and downtime expectations provided: 0.51s for 1m entries
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Member

@etr2460 etr2460 left a 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

@pkdotson pkdotson requested a review from etr2460 August 30, 2021 22:02
@pkdotson pkdotson marked this pull request as ready for review August 31, 2021 16:18
@pkdotson pkdotson requested a review from a team as a code owner August 31, 2021 16:18
@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #16454 (490004c) into master (2a25e2d) will decrease coverage by 0.16%.
The diff coverage is 65.38%.

❗ Current head 490004c differs from pull request most recent head 92b2552. Consider uploading reports for the commit 92b2552 to get more accurate results
Impacted file tree graph

@@            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              
Flag Coverage Δ
hive ?
javascript 71.27% <25.00%> (-0.04%) ⬇️
mysql ?
postgres 81.81% <100.00%> (+<0.01%) ⬆️
presto 81.70% <100.00%> (+0.04%) ⬆️
python 82.03% <100.00%> (-0.29%) ⬇️
sqlite 81.42% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
.../src/explore/components/controls/SelectControl.jsx 89.38% <ø> (ø)
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx 65.92% <0.00%> (-3.49%) ⬇️
superset/connectors/sqla/views.py 64.01% <ø> (ø)
superset/datasets/api.py 91.92% <ø> (ø)
...erset-frontend/src/datasource/DatasourceEditor.jsx 73.57% <12.50%> (-1.25%) ⬇️
...perset-frontend/src/datasource/DatasourceModal.tsx 75.00% <71.42%> (+0.80%) ⬆️
superset/connectors/sqla/models.py 87.51% <100.00%> (-0.44%) ⬇️
superset/datasets/schemas.py 96.52% <100.00%> (+0.06%) ⬆️
superset/models/helpers.py 90.55% <100.00%> (+0.76%) ⬆️
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a25e2d...92b2552. Read the comment docs.

Copy link
Member

@etr2460 etr2460 left a 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?

UPDATING.md Outdated Show resolved Hide resolved
superset-frontend/src/datasource/DatasourceModal.tsx Outdated Show resolved Hide resolved
superset/connectors/sqla/models.py Outdated Show resolved Hide resolved
Copy link
Member

@geido geido left a 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

superset-frontend/src/datasource/DatasourceEditor.jsx Outdated Show resolved Hide resolved
superset-frontend/src/datasource/DatasourceEditor.jsx Outdated Show resolved Hide resolved
superset-frontend/src/datasource/DatasourceEditor.jsx Outdated Show resolved Hide resolved
@geido
Copy link
Member

geido commented Sep 9, 2021

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

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

geido
geido previously requested changes Sep 9, 2021
Copy link
Member

@geido geido left a 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

Screen Shot 2021-09-09 at 14 53 51

  • The certified icon should show also in the Dataset panel. However, this is happening only for the Metrics currently

Screen Shot 2021-09-09 at 14 56 19

pkdotson and others added 5 commits September 9, 2021 08:08
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
@pkdotson
Copy link
Member Author

pkdotson commented Sep 9, 2021

@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

Screen Shot 2021-09-09 at 14 53 51

  • The certified icon should show also in the Dataset panel. However, this is happening only for the Metrics currently

Screen Shot 2021-09-09 at 14 56 19

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.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

⚠️ @pkdotson Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@pkdotson
Copy link
Member Author

pkdotson commented Sep 9, 2021

/testenv up

@pkdotson
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

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

Copy link
Member

@etr2460 etr2460 left a 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 :shipit:

@junlincc
Copy link
Member

junlincc commented Sep 15, 2021

@etr2460 thanks for the review. tested locally.

certification feature is added column and calculated columns ✅
certified items goes on top of the list in each section ✅
edited other uncertified fields did not affect the certified ones ✅
2 issues need to be addressed before merging (?)

  1. icon alignment Screen Shot 2021-09-15 at 3 24 59 PM

Screen Shot 2021-09-15 at 3 24 51 PM

  1. when user click on the pencil icon from dataset crud view and open edit dataset modal, certification details are not showing.

Copy link
Member

@junlincc junlincc left a 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

@pkdotson
Copy link
Member Author

/testenv up

@pkdotson
Copy link
Member Author

Fixes for editing in datasetlist:
https://user-images.githubusercontent.com/17326228/134069819-150028c6-337b-4c03-a51a-395dfb01cc74.mov

fixes alignment issues:
https://user-images.githubusercontent.com/17326228/134070612-068431c4-9428-4e6c-8b0c-a14337c20266.mov

@apache apache deleted a comment from github-actions bot Sep 20, 2021
@pkdotson
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

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

Copy link
Member

@junlincc junlincc left a 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!

@junlincc
Copy link
Member

Follow up tasks @pkdotson

  1. add tooltip as @geido pointed out
    2)certified calculated columns should go to the top of the list.
Screen.Recording.2021-09-22.at.2.29.28.PM.mov

@junlincc junlincc dismissed geido’s stale review September 22, 2021 21:35

let's address it in a separate pr.

@pkdotson pkdotson merged commit a198dbb into apache:master Sep 22, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
…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 <60598000+geido@users.noreply.github.com>

* Update superset-frontend/src/datasource/DatasourceEditor.jsx

Co-authored-by: Geido <60598000+geido@users.noreply.github.com>

* Update superset-frontend/src/datasource/DatasourceEditor.jsx

Co-authored-by: Geido <60598000+geido@users.noreply.github.com>

* 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 <60598000+geido@users.noreply.github.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
…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 <60598000+geido@users.noreply.github.com>

* Update superset-frontend/src/datasource/DatasourceEditor.jsx

Co-authored-by: Geido <60598000+geido@users.noreply.github.com>

* Update superset-frontend/src/datasource/DatasourceEditor.jsx

Co-authored-by: Geido <60598000+geido@users.noreply.github.com>

* 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 <60598000+geido@users.noreply.github.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[editdataset]certify columns and calculated columns
5 participants