feat(dashboard): API to get a dashboard's charts #12978
Conversation
suddjian
changed the title
|
Requesting review from @dpgaspar to make sure I got the security related code and doc strings correct :) edit: CI says there's something wrong with my doc string but I'm not sure how to fix it! 😖 |
|
@suddjian You can probably get some logs if you visit |
Codecov Report
@@ Coverage Diff @@
## master #12978 +/- ##
===========================================
+ Coverage 53.06% 66.74% +13.67%
===========================================
Files 489 491 +2
Lines 17314 28972 +11658
Branches 4482 0 -4482
===========================================
+ Hits 9187 19336 +10149
- Misses 8127 9636 +1509
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.
I think tests readability can be improved.
maybe structure them in an AAA (arrange act assert ) like :
https://github.com/apache/superset/blob/master/tests/dashboards/security/security_rbac_tests.py#L47:L64
and a bit cleanup by extracting to common reusable method can make tests fairly easy to understand
|
Thanks for the QA @junlincc, but I don't think that error could possibly have anything to do with this PR. No existing functionality has been changed here. |
|
got it, that's strange. i also checked on master and other branches which don't have this issue. please disregard if it's not related. |
| ) | ||
| if cx < half_dash_count: | ||
| chart = self.insert_chart(f"slice{cx}", [admin.id], 1, params="{}") |
There was a problem hiding this comment.
Non blocking. Having one or a bunch of dashboards with more then 1 chart could make a better GET test
| @@ -216,6 +219,53 @@ def __init__(self) -> None: | |||
| self.include_route_methods = self.include_route_methods | {"thumbnail"} | |||
| super().__init__() | |||
|
|
|||
| @expose("/<pk>/charts", methods=["GET"]) | |||
| @protect() | |||
| @safe | |||
There was a problem hiding this comment.
Should additionally use has_dashboard access decorator to enforce access for the dashboard
There was a problem hiding this comment.
right! good point @amitmiran137, we can use dashboard = self.datamodel.get(pk, self._base_filters)
mistercrunch
added
🏷️ bot
SUMMARY
Adds a new API to get the chart definitions for a specific dashboard. This is going to support future efforts to turn Superset's frontend into a single page app by allowing removal of the Dashboard app's bootstrap data.
Alternative considered: Using
GETdashboard/:id/related/slices, and iterating over the results to fetch all the slices from/charts/:id.I rejected this approach because it would require changing the
/related/slicesendpoint (currently the endpoint returns the names of charts rather than ids, which is pretty useless for this purpose), and also because it would result in a large number of API requests. The chosen method of a single convenient API endpoint is much simpler.Thanks to @pkdotson for pairing ❤️
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
Wrote tests
ADDITIONAL INFORMATION