Skip to content
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

Send flag notification emails to moderators #9116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Nov 23, 2024

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 seanh requested a review from marcospri November 23, 2024 07:12
@seanh seanh force-pushed the send-flag-notification-emails-to-moderators branch from 77ae1a8 to 5f632e2 Compare November 23, 2024 08:52
Comment on lines -140 to -142
@requires(authenticated_user, group_found)
def group_created_by_user(identity, context):
return context.group.creator and context.group.creator.id == identity.user.id
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer used: being a group's creator no longer grants you any privileges.

@seanh seanh marked this pull request as ready for review November 23, 2024 09:07
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 seanh force-pushed the send-flag-notification-emails-to-moderators branch from 5f632e2 to b88d390 Compare November 23, 2024 12:10
Comment on lines +34 to +36
memberships = group_members_service.get(
annotation.group, roles=list(GROUP_MODERATE_PREDICATES.keys())
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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
  2. 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 calling Identity.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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list() is actually need to get an assert_called_once_with() in the tests to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant