feat(accounts): add default notification configuration to settings#19005
feat(accounts): add default notification configuration to settings#19005michael-smt wants to merge 4 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... |
michael-smt
force-pushed
the
feat/setting-default-notifications
branch
2 times, most recently
from
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. |
michael-smt
force-pushed
the
feat/setting-default-notifications
branch
2 times, most recently
from
f3ca2b5 to
a7dda2e
Compare
michael-smt
force-pushed
the
feat/setting-default-notifications
branch
from
a6e0dea to
eea1aa3
Compare
Co-authored-by: Michal Čihař <michal@cihar.com>
for more information, see https://pre-commit.ci
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.
| def create_default_notifications(user: User) -> None: | ||
| if settings.DEFAULT_NOTIFICATIONS: | ||
| for scope, frequency, notification in settings.DEFAULT_NOTIFICATIONS: | ||
| user.subscription_set.get_or_create( | ||
| scope=scope, | ||
| notification=notification, | ||
| defaults={"frequency": frequency}, | ||
| ) |
| default_notifications = get_env_tuples_or_none("WEBLATE_DEFAULT_NOTIFICATIONS") | ||
| if default_notifications is not None: | ||
| DEFAULT_NOTIFICATIONS = default_notifications | ||
| del default_notifications |
| """ | ||
| parsed_list = get_env_list_or_none(name) | ||
| if parsed_list is not None: | ||
| if parsed_list == [""]: |
| **Example:** | ||
|
|
||
| .. code-block:: yaml | ||
|
|
||
| environment: | ||
| WEBLATE_DEFAULT_NOTIFICATIONS: 0:1:MentionCommentNotificaton,10:1:LastAuthorCommentNotificaton |
|
|
||
| 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.
Allows configuring the default notification configuration for newly created users in settings.
Implements some of #5155