-
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
fix(dashboard): Get dashboard by slug #13352
Conversation
query, None | ||
) | ||
dashboard = query.one_or_none() | ||
if not dashboard: |
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.
this if
doesn't trigger when I enter a bogus slug or id and I'm not sure why yet
Codecov Report
@@ Coverage Diff @@
## master #13352 +/- ##
==========================================
+ Coverage 77.21% 77.23% +0.02%
==========================================
Files 872 872
Lines 45140 45189 +49
Branches 5435 5435
==========================================
+ Hits 34854 34901 +47
- Misses 10163 10165 +2
Partials 123 123
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@suddjian sorry I may have mislead you on a conversation. You can get a dashboard by slug by using the get list endpoint
You can also create a new filter and add it to the list of filters, that filters by id or slug |
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.
Evaluate if we really need the override
@dpgaspar Using the list endpoint isn't ideal as it isn't a very RESTful approach and also doesn't return all the dashboard fields we need. Do you think it would be too much work to get this endpoint working? |
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.
Looking good, just a couple of comments
@@ -361,15 +364,16 @@ def export_dashboards( # pylint: disable=too-many-locals | |||
@classmethod | |||
def get(cls, id_or_slug: str) -> Dashboard: | |||
session = db.session() |
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.
Unrelated, but seems strange that this is creating a new session 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.
It does. I didn't feel entirely comfortable removing it though. Not sure why it was put there.
Comments addressed |
401: | ||
$ref: '#/components/responses/401' | ||
404: | ||
$ref: '#/components/responses/404' |
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 return 500 also from the @safe
decorator
* refactor out id_or_slug filter logic * fix(dashboard): accept slug in place of id in url * remove unnecessary show fields * fixes and tests * linting * linter compliance * change requests * names
SUMMARY
Need get dashboard by slug. Can't get dashboard by slug. Override default FAB endpoint to get dashboard by slug.
Also add field
datasources
, two birds one stone.TEST PLAN
Tests to be written.
ADDITIONAL INFORMATION