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

[charts] New, REST API #8917

Merged
merged 27 commits into from Jan 21, 2020
Merged

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Jan 3, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

New charts REST API.

  • Took the chance to make a more generic solution for Models that implement a owners field and security.
  • Improved field validation for owners and dashboards

image

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@dpgaspar dpgaspar changed the title [charts] New REST API [charts] New, REST API Jan 3, 2020
@codecov-io
Copy link

codecov-io commented Jan 3, 2020

Codecov Report

Merging #8917 into master will increase coverage by 0.17%.
The diff coverage is 67.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8917      +/-   ##
==========================================
+ Coverage   58.97%   59.15%   +0.17%     
==========================================
  Files         359      367       +8     
  Lines       11333    11681     +348     
  Branches     2787     2863      +76     
==========================================
+ Hits         6684     6910     +226     
- Misses       4471     4592     +121     
- Partials      178      179       +1
Impacted Files Coverage Δ
.../assets/src/SqlLab/components/AceEditorWrapper.jsx 55.69% <ø> (-4.08%) ⬇️
superset/assets/src/welcome/App.jsx 0% <ø> (ø) ⬆️
superset/assets/src/dashboard/reducers/index.js 100% <ø> (ø) ⬆️
...ssets/src/dashboard/containers/DashboardHeader.jsx 100% <ø> (ø) ⬆️
...set/assets/src/dashboard/actions/dashboardState.js 40.96% <0%> (ø) ⬆️
...t/assets/src/dashboard/reducers/dashboardLayout.js 93% <0%> (-1.9%) ⬇️
.../src/explore/components/AdhocMetricEditPopover.jsx 62.35% <100%> (ø) ⬆️
...explore/components/AdhocMetricEditPopoverTitle.jsx 70% <100%> (ø) ⬆️
superset/assets/src/SqlLab/constants.js 100% <100%> (ø) ⬆️
...src/dashboard/util/getFilterConfigsFromFormdata.js 88.46% <100%> (+1.5%) ⬆️
... and 22 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 c9f333f...8bb704f. Read the comment docs.

Copy link
Member

@villebro villebro 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 on first pass

superset/views/api.py Outdated Show resolved Hide resolved
superset/views/chart/api.py Outdated Show resolved Hide resolved
Comment on lines 108 to 111
from .chart import views as chart_views
from .dashboard import views as dash_views
from .dashboard.filters import DashboardFilter
from .database import views as in_views
from .utils import (
Copy link
Member

Choose a reason for hiding this comment

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

Same here regarding absolute vs relative imports.

Copy link
Member Author

@dpgaspar dpgaspar Jan 6, 2020

Choose a reason for hiding this comment

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

Will make a separate PR to address all relative imports on views (non charts related)

@dpgaspar dpgaspar marked this pull request as ready for review January 6, 2020 15:45
@dpgaspar dpgaspar changed the title [charts] New, REST API [WiP] [charts] New, REST API Jan 6, 2020
Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

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

A couple of thoughts for you :)

superset/views/base.py Outdated Show resolved Hide resolved
superset/views/base.py Outdated Show resolved Hide resolved
superset/views/chart/api.py Outdated Show resolved Hide resolved
superset/views/dashboard/api.py Outdated Show resolved Hide resolved
@dpgaspar dpgaspar changed the title [WiP] [charts] New, REST API [charts] New, REST API Jan 8, 2020
@dpgaspar dpgaspar requested a review from villebro January 8, 2020 16:28
Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM

superset/views/chart/api.py Show resolved Hide resolved
@mistercrunch mistercrunch merged commit 7415869 into apache:master Jan 21, 2020
@mistercrunch mistercrunch deleted the feature/charts-api branch January 21, 2020 18:04
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 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 size/XXL 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants