-
Notifications
You must be signed in to change notification settings - Fork 46
Replace create default dashboard trigger with django signal #3585
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
Conversation
13116e5 to
29c0c34
Compare
|
Test results 27 files 27 suites 45m 43s ⏱️ Results for commit 29c0c34. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3585 +/- ##
==========================================
- Coverage 62.21% 62.21% -0.01%
==========================================
Files 611 612 +1
Lines 44939 44951 +12
Branches 43 43
==========================================
+ Hits 27960 27967 +7
- Misses 16969 16974 +5
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lunkwill42
left a comment
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 would be just fine in a pure Django-world. But even with this, there may be non-obvious pitfalls in the future (which is to say, I disagree with your statement that the database trigger was non-obvious, but our mileages do vary :-) )
The standard users are created using pure SQL, not using Django migrations or Python code. The next time we squash the migrations into a new SQL schema baseline, we would need to remember to also create default dashboards for the default users in pure SQL - and if we were to add new users as part of a migration at some point, we must also remember to add their dashboards, since this signal would never be triggered.
I'm not against this, I'm just saying it potentially creates different pitfalls that we still need to be aware of...
| @@ -0,0 +1,32 @@ | |||
| """ | |||
| signals.py | |||
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.
Repeating the filename in the docstring is wholly unnecessary. However, the file lacks the license boilerplate, as mentioned in https://nav.readthedocs.io/en/latest/hacking/hacking.html#python-boilerplate-headers
|
We could perhaps add a safeguard against future pitfalls in the form of a test that asserts that all registered accounts has at least one dashboard. |
| verbose_name = 'NAV models' | ||
|
|
||
| def ready(self): | ||
| import nav.models.signals # noqa |
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.
| import nav.models.signals # noqa | |
| import nav.models.signals # noqa: F401 |
just to be specific which rule you're intentionally breaking
| Defines Django signal handlers for model events within the application. | ||
| Handles actions such as creating default dashboards and navlets when new Accounts | ||
| are created. |
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.
| are created. | |
| are created. |
| """ | ||
| if created: | ||
| dashboard = AccountDashboard.objects.create( | ||
| name='My dashboard', account=instance, num_columns=4, is_default=True |
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.
| name='My dashboard', account=instance, num_columns=4, is_default=True | |
| name='My dashboard', account=instance, num_columns=3, is_default=True |
| dashboard = AccountDashboard.objects.create( | ||
| name='My dashboard', account=instance, num_columns=4, is_default=True | ||
| ) | ||
| default_navlets = AccountNavlet.objects.filter(account_id=0) |
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.
| default_navlets = AccountNavlet.objects.filter(account_id=0) | |
| default_navlets = AccountNavlet.objects.filter(account_id=Account.DEFAULT_ACCOUNT) |
|
This PR has been closed, as it has been made obsolete by a sql trigger update in #3579. |



Scope and purpose
When adding support for setting shared dashboards as a user default (#3572), I discovered that the creation of a default dashboard is handled by a SQL trigger, i.e.
nav/python/nav/models/sql/changes/sc.04.06.0054.sql
Lines 8 to 20 in 69825a3
This is not optimal for two reasons:
This PR proposes to replace the SQL trigger with a django
post_savesignal on the account model. This moves the logic from a static SQL trigger that has to be updated with migrations, to a function that we can maintain with a simple commit and changelog.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.