Configuration step for notifications config and subscriptions #22
Conversation
swrichards
force-pushed
the
feature/setup-configuration-with-subscriptions
branch
4 times, most recently
from
267a42c to
1bd8e55
Compare
swrichards
force-pushed
the
feature/setup-configuration-with-subscriptions
branch
2 times, most recently
from
3ad1c63 to
34b525c
Compare
swrichards
force-pushed
the
feature/setup-configuration-with-subscriptions
branch
from
fa58cc7 to
59b4d2b
Compare
| # Note the order: NotificationSubscriptionConfigurationStep expects a notification service | ||
| # to have been configured by NotificationConfigurationStep | ||
| "notifications_api_common.contrib.setup_configuration.steps.NotificationConfigurationStep" | ||
| "notifications_api_common.contrib.setup_configuration.steps.NotificationSubscriptionConfigurationStep" |
There was a problem hiding this comment.
| "notifications_api_common.contrib.setup_configuration.steps.NotificationSubscriptionConfigurationStep" | |
| "notifications_api_common.contrib.setup_configuration.steps.NotificationSubscriptionConfigurationStep" # Optional |
There was a problem hiding this comment.
I am not sure it makes sense to add this. Steps are inherently optional (they're ignored unless explicitly enabled). Configuring this is just about having the capability to use the step. More generally, all steps are inherently optional, you're not forced to use them at all (and with dependencies you can take care of dependencies manually as well).
There was a problem hiding this comment.
I think in the context of setting up notifications for your project through django-setup-configuration (e.g not manually providing data or loading in fixtures) the NotificationSubscriptionConfigurationStep step is optional (but requires NotificationConfigurationStep) and the NotificationConfigurationStep is required (again, assuming you only want to use django-setup-configuration). Not all projects will use the NotificationSubscriptionConfigurationStep and the current documentation might give the impression that they will have to.
If this discussion is preventing you from merging the PR, then I would say go ahead and merge it 👍
swrichards
force-pushed
the
feature/setup-configuration-with-subscriptions
branch
from
59b4d2b to
57363d4
Compare
No description provided.