Conversation
Test results 21 files 21 suites 25m 15s ⏱️ Results for commit a03160f. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3579 +/- ##
==========================================
- Coverage 63.14% 63.14% -0.01%
==========================================
Files 614 614
Lines 45502 45485 -17
Branches 43 43
==========================================
- Hits 28734 28721 -13
+ Misses 16758 16754 -4
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
lunkwill42
left a comment
There was a problem hiding this comment.
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 |
|
15d67a0 to
4e34b2e
Compare
acbd551 to
d6bbea6
Compare
|
I have added a few more changes:
|
4730824 to
08307fb
Compare
johannaengland
left a comment
There was a problem hiding this comment.
Follow up issue (let's get this merged): When I don't have a default dashboard and click on the "Set this dashboard as default" button, then I don't get the visual feedback of a star being added to that dashboard, only when I reload I see it being updated
Wooh, finally 😄 I also added a fixup commit for the default icon update, as it was a quick fix. |
fcc9d2c to
a03160f
Compare
|
lunkwill42
left a comment
There was a problem hiding this comment.
My peeves were fixed, and if this works for @johannaengland I'm not going to argue :-)
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.