-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#2707] Fix display of notification preferences in profile #1374
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1374 +/- ##
========================================
Coverage 95.21% 95.21%
========================================
Files 1005 1005
Lines 37185 37203 +18
========================================
+ Hits 35404 35423 +19
+ Misses 1781 1780 -1 ☔ View full report in Codecov by Sentry. |
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.
Optional nitpick only!
src/open_inwoner/accounts/models.py
Outdated
if ( | ||
config.notifications_messages_enabled | ||
and self.messages_notifications | ||
and inbox_page_is_published() | ||
): | ||
enabled.append(_("messages")) | ||
if self.plans_notifications and collaborate_page_is_published(): | ||
if ( | ||
config.notifications_plans_enabled | ||
and self.plans_notifications | ||
and collaborate_page_is_published() | ||
): |
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.
Nitpick, but we can express the grouped nature with:
if config.notifications_messages_enabled:
if self.messages_notifications and inbox_page_is_published():
enabled.append(_("messages"))
if self.plans_notifications and collaborate_page_is_published():
enabled.append(_("plans"))
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.
Good idea to make the hierarchy between the options explicit. I'll adopt it.
- The notification section in the profile did not reflect the admin's choice to enable/disable the relevant notifications. If the admin disabled notifications about "Samenwerken", they would still be incorrectly listed.
11d0480
to
0bd87ab
Compare
The notification section in the profile did not the admin's choice to enable/disable the relevant notifications. If the admin disabled notifications about "Samenwerken", they would still be listed.
Taiga: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2707