-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: Increments charts endpoint with related dashboards #21518
feat: Increments charts endpoint with related dashboards #21518
Conversation
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.
Can you change test_get_chart
and add dashboards
to the expected_results
. Or do a slight revamp of that test.
Would also be really nice to have a test for the new dashboard filter.
I'll checkout this PR to make sure it will not generate N+1 queries
Codecov Report
@@ Coverage Diff @@
## master #21518 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 1793 1793
Lines 68493 68495 +2
Branches 7275 7275
=======================================
+ Hits 45665 45667 +2
Misses 20966 20966
Partials 1862 1862
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Everything looks good, there is a small risk that https://github.com/dpgaspar/Flask-AppBuilder/blob/449afe47b17298b57272762332f9c96ea6af0449/flask_appbuilder/api/__init__.py#L1517 may be ineffective in fetching the relationships.
If that will turn out to be the case, sqlalchemy has ways of addressing it with loading strategies: https://docs.sqlalchemy.org/en/14/tutorial/orm_related_objects.html#tutorial-orm-loader-strategies
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.
Checked for N+1 queries found nothing. All good!
SUMMARY
We're currently working on a new feature called cross-linking. As one of the examples of the feature, we want to show the dashboards related to a specific chart in the charts list and enable navigation and filtering. The screenshot below shows the concept:
To do that, this PR changes the charts list endpoint to also return the associated dashboards. The
superset/models/Dashboard
model already contains aslices
property with abackref
calleddashboards
which allows us to reference the dashboards from theSlice
model.This is the modified result:
I also added the
dashboards
property to the list of searchable columns to allow queries in the form of:This will allow us to add a Dashboards filter in the charts list screen to find all the charts of a particular dashboard.
TESTING INSTRUCTIONS
1 - Check that the dashboards are returned as expected.
2 - Check that you can the charts list by a particular dashboard.
ADDITIONAL INFORMATION