-
Notifications
You must be signed in to change notification settings - Fork 45
Support shared dashboard as default #3579
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
base: master
Are you sure you want to change the base?
Conversation
Test results 27 files 27 suites 45m 20s ⏱️ Results for commit 15d67a0. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3579 +/- ##
==========================================
+ Coverage 62.50% 62.53% +0.02%
==========================================
Files 611 611
Lines 45093 45129 +36
Branches 43 43
==========================================
+ Hits 28186 28221 +35
- Misses 16897 16898 +1
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
19f39b6 to
2c1ca40
Compare
79572d1 to
3361c7e
Compare
3361c7e to
7df5c62
Compare
7df5c62 to
b132774
Compare
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 is nice, but it also leaves me with some thoughts that aren't directly related to this specific PR:
-
There wasn't much of an actual discussion in #3585 before it was closed. Looking at this PR, I'm thinking an alternative approach to both the database trigger and the signal is a "just-in-time" creation of a user's first dashboard: I.e. when the front page is visited by a user that doesn't have any dashboards configured, the user's default dashboard could be created "just-in-time", by copying whatever is currently considered the system default dashboard. That would cover both the case of an account being inserted directly using SQL and and account created using Django's ORM. Also, not sure from the recent changes + this PR, whether a user can end up with no dashboards of their own, just a subscribed one as their default. If their single subscribed dashboard went away (deleted or unshared), this could cover that case as well. Thoughts?
-
I noticed that if a user unshares a dashboard, it is still on the list of my subscribed dashboards (but when visiting /, it at least shows a different dashboard, even if the now-unshared dashboard is set as my default). When I attempt to navigate to the unshared dashboard, I get a 404 error in my face. I guess that's not really part of this PR, but it should be fixed separately.
b132774 to
0054015
Compare
|
I am in favor of showing an explicit message, that way the user is not surprised and can even go to the user that originally shared the dashboard and complain :D |
0054015 to
a376c5d
Compare
One thing I also noticed when testing is that there is different behavior when one sets another user's dashboard as their default dashboard and then the dashboard gets unshared/deleted: When it is unshared, then it still shows up in the listing and is still marked as default (as described by @lunkwill42 above) and when it is deleted, then it completely disappears As long as both behaviors are the same I am both happy with the dashboard simply disappearing or a message to inform the user along the lines of "This dashboard was unshared/deleted" being shown |
| AccountDefaultDashboard.objects.update_or_create( | ||
| account_id=self.id, defaults={'dashboard_id': dashboard_id} | ||
| ) |
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, this is beautifully done!
| default_dashboard = account.default_dashboard | ||
| default_dashboard_id = default_dashboard.id if default_dashboard else None | ||
| dashboards = ( | ||
| AccountDashboard.objects.filter( | ||
| Q(account=account) | Q(subscribers__account=account) | ||
| Q(account=account) | ||
| | Q(subscribers__account=account) | ||
| | Q(pk=default_dashboard_id) |
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.
To answer your question if setting a dashboard as default should also subscribe you to it, I would expect that to be the case. Because I would be surprised that if I set a dashboard as default and then make something else my default if that previous default dashboard would completely disappear from my feed.
Which if we decide for this would make this change superfluous.
| dashboard_id = ( | ||
| account.default_dashboard.pk if account.has_default_dashboard else None | ||
| ) | ||
|
|
||
| if dashboard_id: | ||
| dashboard = AccountDashboard.objects.filter( | ||
| Q(account=account) | Q(is_shared=True), pk=dashboard_id | ||
| ).first() | ||
| if dashboard: | ||
| return 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.
| dashboard_id = ( | |
| account.default_dashboard.pk if account.has_default_dashboard else None | |
| ) | |
| if dashboard_id: | |
| dashboard = AccountDashboard.objects.filter( | |
| Q(account=account) | Q(is_shared=True), pk=dashboard_id | |
| ).first() | |
| if dashboard: | |
| return dashboard | |
| if account.has_default_dashboard: | |
| return account.default_dashboard |
|



Scope and purpose
Resolves #3572
, continuation of #2344.This PR adds support for setting a shared dashboard as the user default. In order to achieve this, I had to adjust the schema by moving from an
is_defaultfield on theAccountDashboardmodel, to using a join modelAccountDefaultDashboardto store an (account_id, dashboard_id) pair. I also had to adjust the existing logic for handling default dashboard creation for new accounts, which can be seen in#3585the first commit, where the default dashboard trigger is updated.In the current implementation, setting a shared dashboard as your default and subscribing to it are two separate actions. As such, you can set a dashboard as your default without subscribing first. However, it might be desirable to automatically subscribe to a shared dashboard when setting it as your default. This would ensure that the dashboard does not disappear from the dashboard nav after setting a different dashboard as the our default. However, it could be confusing to the user that setting a dashboard as your default also subscribes to it. I'll leave it up to the reviewers whether to implement this or not.
Follow-up task (#3586): Remove
is_defaultfield from database schema. This has to be done after this PR is merged, to avoid breaking other PRs or the master branch, as removing the field will cause an error on the dashboard pages. We might also want to keep theis_defaultvalue until we are sure that the new system works as intended.This pull request
Screenshots
A shared dashboard settings menu has been added
A default shared dashboard is included in the dashboard nav, with both a default and shared icon
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.