Add per-workspace user opt-out from email notifications#6750
Add per-workspace user opt-out from email notifications#6750rowanseymour wants to merge 1 commit into
Conversation
|
Claude finished @rowanseymour's task in 3m 59s —— View job Code ReviewDesign is straightforward and the split between Only minor comments left inline; no blockers:
Security/correctness look fine:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # for optional email types, drop the email medium for users who have opted out in this workspace | ||
| if cls.MEDIUM_EMAIL in user_medium and email_optional: | ||
| membership = org.get_membership(user) | ||
| if membership and not membership.email_notifications: | ||
| user_medium = user_medium.replace(cls.MEDIUM_EMAIL, "") |
There was a problem hiding this comment.
Minor perf: org.get_membership(user) runs once per user in the loop and, when uncached, hits the DB (OrgMembership.objects.filter(org=self, user=user).first()). For an incident with N admins that's up to N sequential queries per notification. Since notifications aren't sent very often and admin counts are usually small this is unlikely to matter in practice, but a single OrgMembership.objects.filter(org=org, user__in=users).in_bulk(field_name="user_id") would collapse it to one query if you'd like to preempt it.
| # role changes are implemented as remove + create so user preferences on the membership must be carried over | ||
| email_notifications = True | ||
|
|
||
| if self.has_user(user): # remove user from any existing roles | ||
| if existing := OrgMembership.objects.filter(org=self, user=user).first(): | ||
| email_notifications = existing.email_notifications | ||
| self.remove_user(user) |
There was a problem hiding this comment.
The existing := ...first() walrus is defensive against a case that can't occur here — self.has_user(user) returning True already implies a matching OrgMembership row exists (that's the through table), so existing will always be truthy inside this branch. You could just do:
existing = OrgMembership.objects.get(org=self, user=user)
email_notifications = existing.email_notifications
self.remove_user(user)Not a bug, just clearer about the invariant.
| obj = super().post_save(obj) | ||
|
|
||
| for membership in self.form.memberships: | ||
| enabled = self.form.cleaned_data.get(f"email_notifications_{membership.org_id}", True) |
There was a problem hiding this comment.
The True default here is unreachable: the form's __init__ always adds an email_notifications_{org_id} BooleanField(required=False) for every membership in self.form.memberships, so cleaned_data will always contain the key (as True or False after unchecked-box coercion). A missing key would mean the workspace was added between GET and POST, in which case defaulting to True is a somewhat surprising choice — you'd silently flip the freshly-created membership's setting even though the user never saw the checkbox. Safer to just do self.form.cleaned_data[f"email_notifications_{membership.org_id}"] or default to membership.email_notifications to be a no-op.
1 participant
Adds a per-membership setting that lets users opt out of unsolicited notification emails from a workspace, whilst still receiving them as UI notifications.
OrgMembership.email_notificationsfield (default on). Role changes recreate memberships, soOrg.add_usernow carries the preference over; leaving and rejoining a workspace resets it.NotificationType.email_optionalattribute marks which types users can opt out of by email:incident:startedandinvitation:accepted. Solicited emails likeexport:finishedare unaffected.Notification.create_alldrops the email medium per-user for opted-out members, and skips creating the notification entirely when email was its only medium.Mailroom needs no changes since it only creates UI-medium notifications, but if it ever starts creating email-medium ones it will need to honor this preference.
Related to #6737 and #6738 as part of reducing notification email noise.