-
Notifications
You must be signed in to change notification settings - Fork 2
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.
We didn't capture this in any of our tests. Can we create a simple test for the functions? A simple request with the right status code might be enough - though this possibly means that we have to create a dashboard page in lana and describe it as a test fixture in the tests' README.md
.
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.
Some minor comments, otherwise looks fine. And we should test out black for code formatting.
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.
The test tests/test_shiny_dashboards.py::TestDashboardAPI::test_shiny_dashboard_management
fails. It seems like another tests creates more and dashboards, so that this test finds more than one and raises. Maybe there is some cleaning up after the tests missing?
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.
Some minor changes and a question about the test readme.
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 to merge
This PR contains a fix for the sharing of dashboards and shiny dashboards.
The endpoint used for sharing was changed to
/api/{resource_id}/sharing
.In addition a fix for the
describe_dashboard_items()
function was included where a non-existing function was referenced and some doc-strings were updated for accuracy.