Send flag notification emails to moderators #9116
Conversation
seanh
force-pushed
the
send-flag-notification-emails-to-moderators
branch
from
77ae1a8
to
5f632e2
Compare
| @requires(authenticated_user, group_found) | ||
| def group_created_by_user(identity, context): | ||
| return context.group.creator and context.group.creator.id == identity.user.id |
There was a problem hiding this comment.
No longer used: being a group's creator no longer grants you any privileges.
seanh
force-pushed
the
send-flag-notification-emails-to-moderators
branch
from
5f632e2
to
b88d390
Compare
h/views/api/flags.py
Outdated
| memberships = group_members_service.get( | ||
| annotation.group, roles=list(GROUP_MODERATE_PREDICATES.keys()) | ||
| ) |
There was a problem hiding this comment.
I originally wanted this to go through the permissions system to get the list of users who have the Permission.Group.MODERATE permission in the context of this annotation's group: create a context object for the group, then for each member create an identity object and call identity_permits(identity, context, Permission.Group.MODERATE) to see if that member should get a moderation email. But there are two problems with that:
- We'd have to fetch all the group's members from the DB and iterate over them, doing a permissions check for each individual member to find out which ones to email
- Creating an identity for a user with
Identity.from_models(user)iterates over all the user's memberships (of all groups that the user is a member of). So we don't want to iterate over all a group's memberships callingIdentity.from_models(membership.user)for each of them
I think we want to avoid a solution that scales poorly as the number of plain members (not even moderators) of the group increases, so I think the solution in this PR that duplicates some logic is better.
There was a problem hiding this comment.
The list() is actually need to get an assert_called_once_with() in the tests to pass.
| group_members_service = request.find_service(name="group_members") | ||
|
|
||
| memberships = group_members_service.get( | ||
| annotation.group, roles=list(GROUP_MODERATE_PREDICATES.keys()) |
There was a problem hiding this comment.
Not sure how often/ how flexible this will be in the future but maybe listing the [ GroupMembershipRoles.OWNER, GroupMembershipRoles.ADMIN, GroupMembershipRoles.MODERATOR]
here will just read better.
There was a problem hiding this comment.
I'm trying to avoid duplication because there's a security predicate that says "owners, admins and moderators can moderate (hide) annotations" and then there's this view code here that says "owners, admins and moderators receive flag notification emails" and we want those two lists of roles to always be in sync: the people who receive the emails should always be the same people who can moderate the annotations. So implementing it in this indirect way to avoid any duplication / any potential for the logic in two places to get out of sync.
h/services/group_members.py
Outdated
| @@ -24,6 +24,29 @@ def __init__(self, db, user_fetcher, publish): | |||
| self.user_fetcher = user_fetcher | |||
| self.publish = publish | |||
|
|
|||
| def get(self, group: Group, roles: list[GroupMembershipRoles] | None = None): | |||
There was a problem hiding this comment.
I made the same comment in a similar PR, this might be better named get_memberships or similar, small extra typing but much better greppeability.
There was a problem hiding this comment.
Yep, renamed this to get_memberships() so we now have get_membership(group, user) to get a single membership and get_memberships(group, roles=None) to get all of a group's memberships.
h/services/group_members.py
Outdated
| query = select(GroupMembership).where(GroupMembership.group == group) | ||
|
|
||
| if roles: | ||
| if len(roles) == 1: |
There was a problem hiding this comment.
Is this necessary? Test seem to pass without it, looks like or_ with just one element renders to just the condition (without OR).
https://docs.sqlalchemy.org/en/20/core/sqlelement.html#sqlalchemy.sql.expression.or_
There was a problem hiding this comment.
👍 I checked this and if we just allow the query to contain an or_() with only one predicate in it SQLAlchemy compiles that into an SQL query without an OR. So yes we can remove this
I think we can go ahead and merge this because currently every group creator is an owner and there's no way for anyone to become a moderator: when you create a group you become an owner, and anyone who joins a group is just a plain member. Also we did a migration to make pre-existing group creators into owners. So merging this won't make any difference right now: only group creators will get these emails. But in future when we release the UI for changing members roles to admin or moderator, the emails will just work for those users as well. (Technically this does make one difference: if the group's creator leaves a group they will no longer get flag notification emails, whereas on |
Send flag notification emails to all of a group's moderators rather than just to the group's creator. Also allow all moderators to moderate (hide) flagged annotations, not just the creator.
seanh
force-pushed
the
send-flag-notification-emails-to-moderators
branch
from
b88d390
to
768fff1
Compare
Send flag notification emails to all of a group's moderators rather than just to the group's creator.
Also allow all moderators to moderate (hide) flagged annotations, not just the creator.