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.

Sign up for GitHub

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

Jump to bottom

feat: Certify Charts and Dashboards #17335

Merged
merged 25 commits into from
Nov 24, 2021
Merged

feat: Certify Charts and Dashboards #17335

merged 25 commits into from
Nov 24, 2021

Conversation

geido
Copy link
Member

@geido geido commented Nov 3, 2021 •
edited by betodealmeida
Loading

SUMMARY

This PR:

  • Allows to certify Charts and Dashboard and filter them out
  • Refactors the Explore/PropertiesModal to use the built-in Antd form functionalities instead of polluting the state

AFTER

DEV.Superset.2.mp4

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@geido geido changed the title feat: Certify charts feat: Certify Charts and Dashboards Nov 5, 2021
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Nov 5, 2021
@geido geido marked this pull request as ready for review November 5, 2021 17:58
@geido geido requested a review from a team as a code owner November 5, 2021 17:58
@codecov
Copy link

codecov bot commented Nov 9, 2021 •
edited
Loading

Codecov Report

Merging #17335 (ee6e7ab) into master (c54027a) will decrease coverage by 0.19%.
The diff coverage is 89.53%.

❗ Current head ee6e7ab differs from pull request most recent head 082ef5a. Consider uploading reports for the commit 082ef5a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17335      +/-   ##
==========================================
- Coverage   76.94%   76.75%   -0.20%     
==========================================
  Files        1042     1042              
  Lines       56299    56364      +65     
  Branches     7793     7815      +22     
==========================================
- Hits        43322    43262      -60     
- Misses      12721    12846     +125     
  Partials      256      256
Flag Coverage Δ
hive ?
javascript 71.06% <84.78%> (+0.02%) ⬆️
presto ?

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

Impacted Files Coverage Δ
...perset-frontend/src/views/CRUD/chart/ChartCard.tsx 75.60% <ø> (ø)
...rontend/src/views/CRUD/dashboard/DashboardCard.tsx 77.77% <ø> (ø)
superset-frontend/src/views/CRUD/hooks.ts 52.76% <ø> (ø)
superset-frontend/src/views/CRUD/types.ts 100.00% <ø> (ø)
superset/dashboards/api.py 93.03% <ø> (ø)
...frontend/src/dashboard/components/Header/index.jsx 68.64% <50.00%> (-0.21%) ⬇️
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 71.14% <50.00%> (-1.09%) ⬇️
...set-frontend/src/components/ListViewCard/index.tsx 98.18% <66.66%> (-1.82%) ⬇️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 74.21% <66.66%> (-0.15%) ⬇️
...rc/explore/components/ExploreChartHeader/index.jsx 57.29% <83.33%> (+0.76%) ⬆️
... and 19 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 c54027a...082ef5a. Read the comment docs.

@geido
Copy link
Member Author

geido commented Nov 10, 2021

/testenv up

@jinghua-qa
Copy link
Member

The only issue I found is in the dashboard properties, the instruction for certified by should be changed to dashboard related?
Screen Shot 2021-11-11 at 9 03 03 AM

zhaoyongjie
zhaoyongjie approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codes LGTM, except certificated_by column type.

betodealmeida
betodealmeida requested changes Nov 12, 2021
View reviewed changes
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a script to benchmark DB migations, can you run it with your migration and post the results?

https://github.com/apache/superset/blob/master/scripts/benchmark_migration.py

superset/migrations/versions/aea15018d53b_add_certifications_columns_to_dashboard.py Outdated

def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.add_column("dashboards", sa.Column("certified_by", sa.Text(), nullable=True))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use op.batch_alter_table here and in the downgrade? Some databases don't support adding/removing columns, so you need to use op.batch_alter_table to make the migration work with them. You can find examples in the other migration scripts.

@betodealmeida betodealmeida added the risk:db-migration PRs that require a DB migration label Nov 12, 2021
@github-actions
Copy link
Contributor

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

❗ Please consider rebasing your branch to avoid db migration conflicts.

@geido
Copy link
Member Author

geido commented Nov 17, 2021 •
edited
Loading

Hey @betodealmeida I applied your suggested changes and ran the benchmarks as follows:

aea15018d53b_add_certifications_columns_to_dashboard.py

Current: 0.22 s
10+: 0.25 s
100+: 0.52 s
1000+: 0.21 s

f9847149153d_add_certifications_columns_to_slice.py

Current: 0.25 s
10+: 0.23 s
100+: 0.47 s
1000+: 0.22 s

betodealmeida
betodealmeida approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the migration benchmark!

@geido geido merged commit 83e49fc into apache:master Nov 24, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

eschutho pushed a commit that referenced this pull request Dec 21, 2021
@geido @eschutho
feat: Certify Charts and Dashboards (#17335)
8057582 
* Certify charts

* Format

* Certify dashboards

* Format

* Refactor card certification

* Clear details when certified by empty

* Show certification in detail page

* Add RTL tests

* Test charts api

* Enhance integration tests

* Lint

* Fix dashboards count

* Format

* Handle empty value

* Handle empty slice

* Downgrade migration

* Indent

* Use alter

* Fix revision

* Fix revision
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
@geido @AAfghahi
feat: Certify Charts and Dashboards (#17335)
2cf0c28 
* Certify charts

* Format

* Certify dashboards

* Format

* Refactor card certification

* Clear details when certified by empty

* Show certification in detail page

* Add RTL tests

* Test charts api

* Enhance integration tests

* Lint

* Fix dashboards count

* Format

* Handle empty value

* Handle empty slice

* Downgrade migration

* Indent

* Use alter

* Fix revision

* Fix revision
@siegfriedweber siegfriedweber mentioned this pull request Feb 9, 2022
Closed
@mistercrunch mistercrunch added 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhaoyongjie zhaoyongjie zhaoyongjie approved these changes

@betodealmeida betodealmeida betodealmeida approved these changes

@pkdotson pkdotson Awaiting requested review from pkdotson

@rusackas rusackas Awaiting requested review from rusackas

@villebro villebro Awaiting requested review from villebro

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels risk:db-migration PRs that require a DB migration size/XL v1.4 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🚢 1.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@geido @jinghua-qa @betodealmeida @zhaoyongjie @mistercrunch @amitmiran137