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

[dashboards] New, export api #8941

Merged
merged 4 commits into from
Jan 15, 2020

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Jan 9, 2020

CATEGORY

Choose one

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

SUMMARY

Implements a dashboard export API endpoint. To have the exact same functionality on the API and on MVC ModelView. This will be used by the new React view for dashboards.

Added enhanced security for the export, applying the filter that's applied for list and get

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

@nytai

@dpgaspar dpgaspar changed the title Feature/dashboards export api [dashboards] feat: export api Jan 9, 2020
@codecov-io
Copy link

codecov-io commented Jan 9, 2020

Codecov Report

Merging #8941 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8941      +/-   ##
==========================================
+ Coverage   58.97%   58.98%   +0.01%     
==========================================
  Files         359      359              
  Lines       11333    11336       +3     
  Branches     2787     2788       +1     
==========================================
+ Hits         6684     6687       +3     
  Misses       4471     4471              
  Partials      178      178
Impacted Files Coverage Δ
...set/assets/src/dashboard/actions/dashboardState.js 40.96% <0%> (ø) ⬆️
...explore/components/AdhocMetricEditPopoverTitle.jsx 70% <0%> (ø) ⬆️
...src/dashboard/util/getFilterConfigsFromFormdata.js 88.46% <0%> (+1.5%) ⬆️

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 2d456e8...0ad95a7. Read the comment docs.

@dpgaspar dpgaspar marked this pull request as ready for review January 9, 2020 16:26
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 9, 2020
@dpgaspar dpgaspar requested a review from villebro January 9, 2020 16:27
@dpgaspar dpgaspar changed the title [dashboards] feat: export api [dashboards] New, export api Jan 9, 2020
Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for implementing this :)

@@ -44,8 +44,7 @@ class DashboardModelView(
datamodel = SQLAInterface(models.Dashboard)

@action("mulexport", __("Export"), __("Export dashboards?"), "fa-database")
@staticmethod
def mulexport(items):
def mulexport(self, items): # pylint: disable=no-self-use
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's related with this already merged PR: #8942

the @action decorator creates a methods attribute on the method, this is defeated by making the method static

"""
self.login(username="admin")
argument = [1, 2]
uri = f"api/v1/dashboard/export/?q={prison.dumps(argument)}"
Copy link
Member

Choose a reason for hiding this comment

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

So the final URL here looks like api/v1/dashboard/export/?q=!(1,2)?

Copy link
Member Author

Choose a reason for hiding this comment

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

exactly

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.

LGTM!

Copy link
Member

@suddjian suddjian left a comment

Choose a reason for hiding this comment

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

yas

@dpgaspar dpgaspar merged commit 123246f into apache:master Jan 15, 2020
@dpgaspar dpgaspar deleted the feature/dashboards-export-api branch January 15, 2020 18:10
@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/L 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants