-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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(dashboard): dashboard/id/datasets endpoint #13523
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13523 +/- ##
==========================================
- Coverage 77.45% 71.49% -5.97%
==========================================
Files 906 824 -82
Lines 45825 41189 -4636
Branches 5530 4261 -1269
==========================================
- Hits 35495 29448 -6047
- Misses 10202 11741 +1539
+ Partials 128 0 -128
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good left a couple of comments
) | ||
dashboard = query.one_or_none() | ||
if not dashboard: | ||
raise DashboardNotFoundError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels kind of strange that a DAO
raises a Command
Exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I never noticed that. It also feels kind of strange to create multiple different exception classes with the exact same meaning. I think it's reasonable to share them.
superset/dashboards/api.py
Outdated
type: object | ||
properties: | ||
result: | ||
type: object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to declare a marshmallow schema and reference it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but please check over my schema for any issues because it's a big one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just one minor improvement proposal for tests.
tests/dashboards/api_tests.py
Outdated
dataset_ids = set([s.datasource_id for s in dashboard.slices]) | ||
self.assertEqual(len(data["result"]), len(dataset_ids)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could probably compare a set of id
s from data["result"]
with dataset_ids
here to make sure the ids are identical.
* feat(dashboard) dashboard/id/datasets endpoint * schema for dashboard datasets * list instead of map * finish dashboard dataset schema * description * better test * add the dataset schema to the schema list * lint
SUMMARY
Adding a new endpoint that gets the dataset metadata for a dashboard.
This is intended to replace the template-loaded bootstrap data on the dashboard page. In keeping with the precedent set by the existing bootstrap data, I have opted to return data in the same shape (a map of datasets) rather than a list, and to copy the bootstrap data's "trimming" behavior where any data unnecessary for rendering the dashboard is stripped out of the result. I don't believe these design decisions make for the ideal endpoint, but they do reduce potential regressions in the Single Page App project.
TEST PLAN
Wrote unit tests, tested manually
ADDITIONAL INFORMATION