Skip to content

Conversation

@Simrayz
Copy link
Contributor

@Simrayz Simrayz commented Oct 8, 2025

Scope and purpose

Resolves #3572 , continuation of #2344.

EDIT 15.10.25:
This PR has replaced #3585, and replaces the create_default_dashboard database trigger instead of using django signals.

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_default field on the AccountDashboard model, to using a join model AccountDefaultDashboard to 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 #3585 the 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_default field 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 the is_default value until we are sure that the new system works as intended.

This pull request

  • Changes Account post_save signal to use new default dashboard logic
  • Changes dashboard utilities to include default dashboards regardless of owner for the current account
  • Changes dashboard actions to include a settings popover for shared dashboards, which enables setting the default dashboard and export.
  • Removed separate export button, as this is now found in the shared dashboard settings.

Screenshots

A shared dashboard settings menu has been added

image

A default shared dashboard is included in the dashboard nav, with both a default and shared icon

image

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.

  • Added a changelog fragment for towncrier
  • Added/amended tests for new/changed code
  • Added/changed documentation
  • Linted/formatted the code with ruff, easiest by using pre-commit
  • Wrote the commit message so that the first line continues the sentence "If applied, this commit will ...", starts with a capital letter, does not end with punctuation and is 50 characters or less long. See https://cbea.ms/git-commit/
  • Based this pull request on the correct upstream branch: For a patch/bugfix affecting the latest stable version, it should be based on that version's branch (<major>.<minor>.x). For a new feature or other additions, it should be based on master.
  • If applicable: Created new issues if this PR does not fix the issue completely/there is further work to be done
  • If it's not obvious from a linked issue, described how to interact with NAV in order for a reviewer to observe the effects of this change first-hand (commands, URLs, UI interactions)
  • If this results in changes in the UI: Added screenshots of the before and after
  • If this adds a new Python source code file: Added the boilerplate header to that file

@Simrayz Simrayz self-assigned this Oct 8, 2025
@github-actions
Copy link

github-actions bot commented Oct 8, 2025

Test results

    27 files      27 suites   45m 20s ⏱️
 2 626 tests  2 626 ✅ 0 💤 0 ❌
19 398 runs  19 398 ✅ 0 💤 0 ❌

Results for commit 15d67a0.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Oct 8, 2025

Codecov Report

❌ Patch coverage is 98.03922% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 62.53%. Comparing base (f35cca2) to head (15d67a0).
⚠️ Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
python/nav/web/navlets/__init__.py 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Simrayz Simrayz force-pushed the feat/3572-support-shared-dashboard-as-default branch 4 times, most recently from 19f39b6 to 2c1ca40 Compare October 13, 2025 09:55
@Simrayz Simrayz changed the base branch from master to chore/replace-default-dashboard-trigger October 13, 2025 11:38
@Simrayz Simrayz force-pushed the feat/3572-support-shared-dashboard-as-default branch 2 times, most recently from 79572d1 to 3361c7e Compare October 13, 2025 12:01
@Simrayz Simrayz requested a review from a team October 14, 2025 07:43
@Simrayz Simrayz marked this pull request as ready for review October 14, 2025 07:43
@Simrayz Simrayz force-pushed the feat/3572-support-shared-dashboard-as-default branch from 3361c7e to 7df5c62 Compare October 14, 2025 08:29
@Simrayz Simrayz linked an issue Oct 14, 2025 that may be closed by this pull request
@Simrayz Simrayz force-pushed the feat/3572-support-shared-dashboard-as-default branch from 7df5c62 to b132774 Compare October 15, 2025 13:48
@Simrayz Simrayz changed the base branch from chore/replace-default-dashboard-trigger to master October 15, 2025 13:49
Copy link
Member

@lunkwill42 lunkwill42 left a 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:

  1. 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?

  2. 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.

@Simrayz Simrayz force-pushed the feat/3572-support-shared-dashboard-as-default branch from b132774 to 0054015 Compare October 16, 2025 08:50
@Simrayz
Copy link
Contributor Author

Simrayz commented Oct 16, 2025

This is nice, but it also leaves me with some thoughts that aren't directly related to this specific PR:

  1. There wasn't much of an actual discussion in Replace create default dashboard trigger with django signal #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?
  2. 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.
  1. I agree that we could have discussed the PR before closing it, but since it was a suggested change to make my feature work as intended, I figured that I might as well just go for the solution that makes the least amount of change in how the system operates. I still don't think it is obvious that these things happen somewhere in a sql file, and we spent some time looking without getting any wiser. My suggestion was an attempt to make these effects less opaque, by bringing it into python code. Adding a JIT-solution could work well, I'm just wondering if it would affect any tests or functionality that depends on the existence of a default dashboard - i.e. the default dashboard fix shouldn't depend on loading the webfront-index view.
    The delete_dashboard view still does not allow deleting your last dashboard, and does not take any subscribed or shared dashboards into account. As such, it should not be possible for the user to end up with no dashboards at all. I agree that a JIT solution could work well here :) I'm happy to take any suggestions on how to best implement a JIT-solution, or we could find a time for a short design discussion.

  2. I agree that this is strange. The reason is that the AccountDefaultDashboard link is not deleted, when a dashboard is unshared. It was my intention to delete these mappings along with the subscriptions, but I must have forgotten to do it. As I see it, we could solve this in two ways. We can keep existing default mappings, but show a message to the user that "This dashboard has been unshared by its owner, please select a new default dashboard". Alternatively, default dashboard mappings are removed, and a fallback is shown for the account. Preferably the UI should warn the user that they have set no default, and prompt them to select from their existing dashboards.
    Regarding the 404, I think we should improve the UX by returning an empty dashboard with a "Dashboard not found" type message. This way the user can still navigate to an existing dashboard. Similarly, a 403 should inform the user that the dashboard exists, but it is not accessible. It should also perhaps include information about the dashboard owner - e.g. if someone shares a link to a dashboard, so they know who to notify.

@johannaengland johannaengland requested a review from a team October 22, 2025 16:47
@johannaengland
Copy link
Contributor

2. I agree that this is strange. The reason is that the `AccountDefaultDashboard` link is not deleted, when a dashboard is unshared. It was my intention to delete these mappings along with the subscriptions, but I must have forgotten to do it. As I see it, we could solve this in two ways. We can keep existing default mappings, but show a message to the user that "This dashboard has been unshared by its owner, please select a new default dashboard". Alternatively, default dashboard mappings are removed, and a fallback is shown for the account. Preferably the UI should warn the user that they have set no default, and prompt them to select from their existing dashboards.

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

@Simrayz Simrayz force-pushed the feat/3572-support-shared-dashboard-as-default branch from 0054015 to a376c5d Compare October 23, 2025 08:41
@johannaengland johannaengland requested a review from hmpf October 25, 2025 15:09
@johannaengland
Copy link
Contributor

2. I agree that this is strange. The reason is that the `AccountDefaultDashboard` link is not deleted, when a dashboard is unshared. It was my intention to delete these mappings along with the subscriptions, but I must have forgotten to do it. As I see it, we could solve this in two ways. We can keep existing default mappings, but show a message to the user that "This dashboard has been unshared by its owner, please select a new default dashboard". Alternatively, default dashboard mappings are removed, and a fallback is shown for the account. Preferably the UI should warn the user that they have set no default, and prompt them to select from their existing dashboards.

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

Comment on lines +373 to +375
AccountDefaultDashboard.objects.update_or_create(
account_id=self.id, defaults={'dashboard_id': dashboard_id}
)
Copy link
Contributor

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!

Comment on lines +80 to +86
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)
Copy link
Contributor

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.

Comment on lines +51 to +60
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

@johannaengland johannaengland requested a review from a team October 25, 2025 15:40
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support setting shared dashboard as default

4 participants