Shared dashboards: Support loading shared and subscribed dashboards#3553
Conversation
Test results 27 files 27 suites 45m 8s ⏱️ Results for commit 67424dc. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/2344-add-shareable-dashboards #3553 +/- ##
======================================================================
+ Coverage 62.12% 62.20% +0.08%
======================================================================
Files 611 611
Lines 44900 44939 +39
Branches 43 43
======================================================================
+ Hits 27893 27955 +62
+ Misses 16997 16974 -23
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0c69b79 to
6549854
Compare
2493ac3 to
00e542a
Compare
6549854 to
1821850
Compare
00e542a to
72e875f
Compare
1821850 to
cde7e99
Compare
72e875f to
d3ee6c1
Compare
hmpf
left a comment
There was a problem hiding this comment.
What happens if you subscribe to multiple dashboards from the same user when you try to subscribe/unsubscribe?
@hmpf Just what is suspected :) You are (un)subscribed only to the dashboard you're viewing currently. |
lunkwill42
left a comment
There was a problem hiding this comment.
I have not done a deep-dive into the code, but instead focused on testing the new functionality, and I have found some issues:
Authorization issues
-
With this PR, the widget controls are displayed to an anonymous user (
default) when viewing the system default dashboard. This is not good.- Attempts to delete widgets fails with an error message. Phew.
- Attempts to move widgets around on the dashboard work, but the change does not seem persistent. Everything is restored on a page reload.
- Some widgets allow for editing their title, like the "Status" widget. The anonymous user is allowed to change the widget title permanently under this PR, whereas it previously was not.
-
Related to this seems to be: If I am logged in as user A and share my dashboard D, then sudo to user B (using the "operate as this user" button in the user admin panel), then try to view dashboard D, I am also shown the widget control buttons. I am now even allowed to enter the widget configuration forms. Fortunately most forms respond with a failure if I try to save my configuration changes (one of them has a snappy error message about Oompa Loompas not willing to do their job).
- If I, however, log in as user B without the sudo function, none of the widget control are displayed to me when I view dashboard D.
Usability thoughts
-
I find it somewhat difficult to deduce whether I am just watching someone else's dashboard transiently, or whether I am in fact subscribed to the dashboard I'm looking at. I think maybe the dashboard selector button should have some type of indication that this is a transient view. Dotted-line frame around the button, perhaps? (although, what about UU?) Having to click the dashboard owner's name to the right of the screen to see whether I am subscribed or not seems a bit excessive.
-
Similarly, I find myself wanting to have a better indication of whether one of my own dashboards are shared with others - without the need to use the gear button (dashboard config form) to see the share status.
Other thoughts
This is a nice piece of work, and I think we are headed in the right direction with this :)
0c679f8 to
bb94025
Compare
e7822ed to
a697519
Compare
bb94025 to
f3078e4
Compare
|
And another thought that just occurred to me: Can you make someone else's dashboard your default dashboard? And should you be able to? Because I assume that some user might want to do that - and what happens then if that one is deleted? |
It is not possible to make a shared dashboard your default in the current system. The default dashboard is set directly on the dashboard table, while subscriptions are using a separate join table. It is possible to add a default attribute to the subscription table, and then check both for an owned default and a subscribed default. Whether it should be possible, I'm not sure, but it makes sense for those who don't want to create their own dashboards. In either case, since this was not in the list of requirements, I would prefer that this is done at a later time, either based on user demand or as a follow-up if the team thinks it is essential. |
5340d39 to
edd62b1
Compare
Yes, good idea. Depending on what @lunkwill42 and @hmpf say we can make a follow up issue. Or just forget about this |
I think we should make a followup issue. Setting a subscribed dashboard as your default makes sense, and I'm sure many users will want to do this. I never liked the model where the "default" is an attribute on the dashboard model itself. It would be much nicer to have it be part of the user preferences, so that only a single entry could ever be made (I guess @johannaengland still remembers the weird "no-default-dashboard" or "multiple-default-dashboards"-debaucle). |
lunkwill42
left a comment
There was a problem hiding this comment.
Tested from #3557, but things seem better now :-)
| <div class="row navlet-header"> | ||
| <div class="small-12 column"> | ||
| {% if not current_user_data.account.is_default_account or current_user_data.sudoer %} | ||
| {% if navlet.can_edit or current_user_data.sudoer %} |
There was a problem hiding this comment.
As described in #3571 - this no longer works anyway. Not sure why. But it means that this check is partly useless atm.
There was a problem hiding this comment.
Will look into allowing admins to edit default account dashboards :)
2dfd09c to
f4b50b2
Compare
|
Made a follow-up issue #3572 |
Scope and purpose
Resolves #3555
Dependent on #3551. Broken into its own PR to reduce review time.
Large PR, but 74% (493 lines) is added tests, as there was no existing coverage for the existing
find_dashboardorindexview (in terms of dashboard response).This PR adds support for loading a shared dashboard and for the current account to toggle their subscription to it.
No-News because the parent PR has the fragment.
This pull request
find_dashboardutility to include a shared dashboard.get_dashboards_for_accountutility that gets dashboards the current account owns or is subscribed to.'dashboard-index'and'get-user-navlet'to return a response if the currentdashboard.idis either owner by the current account oris_shared=True.Screenshots
Modifications to the dashboard navigation
A new Owner popover with subscribtion controls has been added to the header. Only visible when viewing a dashboard shared by another account.Owner info with subscription controls
Modifications to dashboard and navlet actions
When viewing a dashboard shared by another account, the:
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.
<major>.<minor>.x). For a new feature or other additions, it should be based onmaster.