feat(accounts): add default notification configuration to settings#19005
feat(accounts): add default notification configuration to settings#19005michael-smt wants to merge 3 commits into
Conversation
|
The configuration syntax is probably not ideal, and it might be a bit too much documentation 😉 |
|
I'm not really sure about this. This used to be an implementation detail, and we should probably think about this more before exposing it as configuration. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
I'm also not so sure about this 🙂 But I thought better something than nothing... Maybe we could keep it undocumented, that would allow coming up with a better way to expose it as configuration later... |
eb801c8 to
0e956ec
Compare
|
I wasn't able to come to a better conclusion here, so let's make this happen for the next milestone. The only thing I don't like here is manually written documentation for possible values, which is super easy to become outdated. There are definitely better and more complex approaches to this, but we know we're going to change the notifications subsystem heavily in the not-so-distant future (at least to support web notifications), so let's not spend much time on something that will have to change anyway. |
|
@michael-smt Can you rebase this on current main? If you have time to add documentation generation (see |
|
Yes, will do. Thanks for the pointer with the docs generation. |
a6e0dea to
eea1aa3
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new DEFAULT_NOTIFICATIONS setting (and Docker env var support) to configure which notification subscriptions are created for newly registered users, addressing part of #5155 (admin presets for notifications).
Changes:
- Introduces
DEFAULT_NOTIFICATIONSas a first-class setting (defaulted viaWeblateAccountsConf) and uses it when auto-creating user subscriptions. - Adds Docker env var support (
WEBLATE_DEFAULT_NOTIFICATIONS) plus an env parsing helper and tests. - Adds documentation + an autogenerated snippet and a management command to list available scopes/frequencies/handlers.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| weblate/utils/tests/test_environment.py | Adds unit tests for the new env tuple parsing helper. |
| weblate/utils/environment.py | Adds get_env_tuples_or_none helper for tuple-list env parsing. |
| weblate/settings_docker.py | Allows overriding DEFAULT_NOTIFICATIONS via WEBLATE_DEFAULT_NOTIFICATIONS. |
| weblate/accounts/tests/test_notifications.py | Updates tests to reference the new default-notifications source. |
| weblate/accounts/models.py | Moves default notification presets into WeblateAccountsConf and applies settings.DEFAULT_NOTIFICATIONS on user creation. |
| weblate/accounts/management/commands/list_notification_config.py | Adds doc-generator command to list notification scopes/frequencies/handlers. |
| weblate/accounts/data.py | Removes old module containing defaults + creation helper (logic moved into models/settings). |
| docs/snippets/notifications-config.rst | Adds autogenerated snippet content for notification scopes/frequencies/handlers. |
| docs/Makefile | Updates update-docs target to regenerate the notifications snippet. |
| docs/changes.rst | Mentions the new DEFAULT_NOTIFICATIONS setting in changelog. |
| docs/admin/management.rst | Documents the new list_notification_config command. |
| docs/admin/install/docker.rst | Documents WEBLATE_DEFAULT_NOTIFICATIONS env var with example. |
| docs/admin/config.rst | Documents the new DEFAULT_NOTIFICATIONS setting and includes the generated snippet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| MAXIMAL_PASSWORD_LENGTH = defaults.DEFAULT_MAXIMAL_PASSWORD_LENGTH | ||
|
|
||
| DEFAULT_NOTIFICATIONS: ClassVar[ |
There was a problem hiding this comment.
The default value should go to the defaults module and it can be then reused as default value in the Docker settings.
There was a problem hiding this comment.
I tried, but it means we can't use Django's IntegerChoices anymore to define the defaults, only raw values. Because when they are used in the Docker settings, the Django apps aren't loaded yet. Maybe there is another way to solve this?
abdd0df to
ff4b2df
Compare
ccd7182 to
8f4d2f3
Compare
Co-authored-by: Michal Čihař <michal@cihar.com>
8f4d2f3 to
87d4bcd
Compare
| except ValueError as error: | ||
| msg = f"{name}: not an integer: {error}" | ||
| raise ImproperlyConfigured(msg) from error | ||
|
|
There was a problem hiding this comment.
I would have liked to extend the validation here with something like this:
if frequency not in NotificationFrequency.values:
msg = f"{name}: invalid notification frequency {frequency}"
raise ImproperlyConfigured(msg)
if scope not in NotificationScope.values:
msg = f"{name}: invalid notification scope {scope}"
raise ImproperlyConfigured(msg)
if handler not in [n.__name__ for n in NOTIFICATIONS]:
msg = f"{name}: unknown handler {handler}"
raise ImproperlyConfigured(msg)But can't use these classes here because django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.
Maybe there is a better place for the validation?
Allows configuring the default notification configuration for newly created users in settings.
Implements some of #5155