From 5d44145360f7500849beeaf0cea711184c7d0351 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Sat, 23 Nov 2024 07:11:05 +0000 Subject: [PATCH] Send flag notification emails to moderators 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. --- h/security/permission_map.py | 14 +- h/security/predicates.py | 5 - h/services/group_members.py | 26 +++- h/views/api/flags.py | 21 +-- tests/functional/api/moderation_test.py | 12 +- tests/functional/moderation_test.py | 61 +++----- tests/unit/h/security/permits_test.py | 8 +- tests/unit/h/security/predicates_test.py | 10 -- tests/unit/h/services/group_members_test.py | 72 +++++++++- tests/unit/h/views/api/flags_test.py | 149 ++++++++++++-------- 10 files changed, 240 insertions(+), 138 deletions(-) diff --git a/h/security/permission_map.py b/h/security/permission_map.py index 5b3e8bfe0ba..f8285bc40d9 100644 --- a/h/security/permission_map.py +++ b/h/security/permission_map.py @@ -13,9 +13,21 @@ """ import h.security.predicates as p +from h.models import GroupMembershipRoles from h.security.permissions import Permission from h.security.predicates import resolve_predicates +# The logic for who is allowed to moderate annotations in a group unfortunately +# has to be duplicated in the PERMISSION_MAP below and elsewhere when sending +# email notifications to moderators. To help keep the two implementations in +# sync both are derived from this constant. +GROUP_MODERATE_PREDICATES = { + GroupMembershipRoles.OWNER: [p.group_has_user_as_owner], + GroupMembershipRoles.ADMIN: [p.group_has_user_as_admin], + GroupMembershipRoles.MODERATOR: [p.group_has_user_as_moderator], +} + + PERMISSION_MAP = { # Admin pages Permission.AdminPage.HIGH_RISK: [[p.user_is_admin]], @@ -56,7 +68,7 @@ ], Permission.Group.MEMBER_ADD: [[p.group_matches_authenticated_client_authority]], Permission.Group.MEMBER_REMOVE: [[p.group_member_remove]], - Permission.Group.MODERATE: [[p.group_created_by_user]], + Permission.Group.MODERATE: GROUP_MODERATE_PREDICATES.values(), # --------------------------------------------------------------------- # # Annotations Permission.Annotation.CREATE: [[p.authenticated]], diff --git a/h/security/predicates.py b/h/security/predicates.py index a5aa199925f..1729e96c407 100644 --- a/h/security/predicates.py +++ b/h/security/predicates.py @@ -138,11 +138,6 @@ def group_joinable_by_authority(_identity, context): return context.group.joinable_by == JoinableBy.authority -@requires(authenticated_user, group_found) -def group_created_by_user(identity, context): - return context.group.creator and context.group.creator.id == identity.user.id - - @requires(authenticated_user, group_found) def group_has_user_as_owner(identity, context): return _group_has_user_as_role(identity, context, GroupMembershipRoles.OWNER) diff --git a/h/services/group_members.py b/h/services/group_members.py index 08ee28a8301..2a5e671d94a 100644 --- a/h/services/group_members.py +++ b/h/services/group_members.py @@ -1,10 +1,10 @@ import logging from functools import partial -from sqlalchemy import select +from sqlalchemy import or_, select from h import session -from h.models import GroupMembership +from h.models import Group, GroupMembership, GroupMembershipRoles log = logging.getLogger(__name__) @@ -32,6 +32,28 @@ def get_membership(self, group, user) -> GroupMembership | None: .where(GroupMembership.user == user) ) + def get_memberships( + self, group: Group, roles: list[GroupMembershipRoles] | None = None + ): + """ + Return `group`'s memberships. + + If `roles` is None return all of `group`'s memberships. + + If `roles` is not None return only those memberships matching the given role(s). + + If multiple roles are given return all memberships matching *any* of + the given roles. + """ + query = select(GroupMembership).where(GroupMembership.group == group) + + if roles: + query = query.where( + or_(GroupMembership.roles.contains(role) for role in roles) + ) + + return self.db.scalars(query) + def add_members(self, group, userids): """ Add the users indicated by userids to this group's members. diff --git a/h/views/api/flags.py b/h/views/api/flags.py index 5b616db052b..02aaf326f4c 100644 --- a/h/views/api/flags.py +++ b/h/views/api/flags.py @@ -3,6 +3,7 @@ from h import links from h.emails import flag_notification from h.security import Permission +from h.security.permission_map import GROUP_MODERATE_PREDICATES from h.tasks import mailer from h.views.api.config import api_config @@ -18,19 +19,23 @@ def create(context, request): request.find_service(name="flag").create(request.user, context.annotation) - _email_group_admin(request, context.annotation) + _email_group_moderators(request, context.annotation) return HTTPNoContent() -def _email_group_admin(request, annotation): +def _email_group_moderators(request, annotation): incontext_link = links.incontext_link(request, annotation) if incontext_link is None: incontext_link = annotation.target_uri - group = annotation.group - if group.creator and group.creator.email: - send_params = flag_notification.generate( - request, group.creator.email, incontext_link - ) - mailer.send.delay(*send_params) + group_members_service = request.find_service(name="group_members") + + memberships = group_members_service.get_memberships( + annotation.group, roles=list(GROUP_MODERATE_PREDICATES.keys()) + ) + + for membership in memberships: + if email := membership.user.email: + send_params = flag_notification.generate(request, email, incontext_link) + mailer.send.delay(*send_params) diff --git a/tests/functional/api/moderation_test.py b/tests/functional/api/moderation_test.py index 7bc9183bfdb..7c6e26f5b38 100644 --- a/tests/functional/api/moderation_test.py +++ b/tests/functional/api/moderation_test.py @@ -1,8 +1,10 @@ import pytest +from h.models import GroupMembership, GroupMembershipRoles + class TestPutHide: - def test_it_returns_http_204_for_group_creator( + def test_it_returns_http_204_for_group_moderator( self, app, group_annotation, user_with_token ): _, token = user_with_token @@ -10,7 +12,6 @@ def test_it_returns_http_204_for_group_creator( res = app.put(f"/api/annotations/{group_annotation.id}/hide", headers=headers) - # The creator of a group has moderation rights over the annotations in that group assert res.status_code == 204 def test_it_returns_http_404_if_annotation_is_in_world_group( @@ -51,7 +52,7 @@ def test_it_returns_http_404_if_annotation_is_private( class TestDeleteHide: - def test_it_returns_http_204_for_group_creator( + def test_it_returns_http_204_for_group_moderator( self, app, group_annotation, user_with_token ): _, token = user_with_token @@ -61,7 +62,6 @@ def test_it_returns_http_204_for_group_creator( f"/api/annotations/{group_annotation.id}/hide", headers=headers ) - # The creator of a group has moderation rights over the annotations in that group assert res.status_code == 204 def test_it_returns_http_404_if_annotation_is_in_world_group( @@ -117,7 +117,9 @@ def other_user(db_session, factories): @pytest.fixture def group(user, db_session, factories): - group = factories.Group(creator=user) + group = factories.Group( + memberships=[GroupMembership(user=user, roles=[GroupMembershipRoles.MODERATOR])] + ) db_session.commit() return group diff --git a/tests/functional/moderation_test.py b/tests/functional/moderation_test.py index fa17a350733..05e4d63b1bb 100644 --- a/tests/functional/moderation_test.py +++ b/tests/functional/moderation_test.py @@ -1,44 +1,23 @@ -import pytest +from h.models import GroupMembership, GroupMembershipRoles class TestModeration: - def test_moderator_flag_listing( - self, app, flagged_annotation, moderator_with_token - ): - _, token = moderator_with_token - - headers = {"Authorization": f"Bearer {token.value}"} - annotation_url = f"/api/annotations/{flagged_annotation.id}" - res = app.get(annotation_url, headers=headers) - - assert "moderation" in res.json - assert res.json["moderation"]["flagCount"] > 0 - - -@pytest.fixture -def group(db_session, factories, moderator): - group = factories.OpenGroup(creator=moderator) - db_session.commit() - return group - - -@pytest.fixture -def flagged_annotation(group, db_session, factories): - ann = factories.Annotation(groupid=group.pubid, shared=True) - factories.Flag(annotation=ann) - db_session.commit() - return ann - - -@pytest.fixture -def moderator(db_session, factories): - user = factories.User() - db_session.commit() - return user - - -@pytest.fixture -def moderator_with_token(moderator, db_session, factories): - token = factories.DeveloperToken(user=moderator) - db_session.commit() - return (moderator, token) + def test_it(self, app, db_session, factories): + group = factories.OpenGroup() + annotation = factories.Annotation(group=group, shared=True) + factories.Flag(annotation=annotation) + moderator = factories.User( + memberships=[ + GroupMembership(group=group, roles=[GroupMembershipRoles.MODERATOR]) + ] + ) + token = factories.DeveloperToken(user=moderator) + db_session.commit() + + response = app.get( + f"/api/annotations/{annotation.id}", + headers={"Authorization": f"Bearer {token.value}"}, + ) + + assert "moderation" in response.json + assert response.json["moderation"]["flagCount"] > 0 diff --git a/tests/unit/h/security/permits_test.py b/tests/unit/h/security/permits_test.py index 0b598946b5f..07f89bce309 100644 --- a/tests/unit/h/security/permits_test.py +++ b/tests/unit/h/security/permits_test.py @@ -3,7 +3,7 @@ import pytest from pyramid.security import Allowed, Denied -from h.models import GroupMembership +from h.models import GroupMembership, GroupMembershipRoles from h.security import Identity, Permission from h.security.permits import PERMISSION_MAP, identity_permits from h.traversal import AnnotationContext @@ -67,7 +67,7 @@ def PERMISSION_MAP(self): class TestIdentityPermitsIntegrated: - def test_it(self, user, group, annotation): + def test_it(self, user, annotation): # We aren't going to go bonkers here, but a couple of tests to show # this actually holds together. This isn't really to inform us of any # particular failure, but just give us sensitivity if this doesn't work @@ -79,11 +79,11 @@ def test_it(self, user, group, annotation): # A user can delete their own annotation assert identity_permits(identity, anno_context, Permission.Annotation.DELETE) - # Once a user is the creator of a group they can moderate + # Once a user is an owner of a group they can moderate assert not identity_permits( identity, anno_context, Permission.Annotation.MODERATE ) - group.creator = user + identity.user.memberships[0].roles = [GroupMembershipRoles.OWNER] assert identity_permits(identity, anno_context, Permission.Annotation.MODERATE) # Once a user is an admin they can do admin things diff --git a/tests/unit/h/security/predicates_test.py b/tests/unit/h/security/predicates_test.py index 6bbc29ddbc1..96ec6d6122b 100644 --- a/tests/unit/h/security/predicates_test.py +++ b/tests/unit/h/security/predicates_test.py @@ -165,16 +165,6 @@ def test_group_joinable_by_authority(self, group_context, joinable_by): assert result == (joinable_by == JoinableBy.authority) - def test_group_created_by_user(self, identity, group_context, factories): - group_context.group.creator = None - assert not predicates.group_created_by_user(identity, group_context) - - group_context.group.creator = factories.User.build(id="different_user") - assert not predicates.group_created_by_user(identity, group_context) - - group_context.group.creator.id = identity.user.id - assert predicates.group_created_by_user(identity, group_context) - @pytest.mark.parametrize( "role,expected_result", [ diff --git a/tests/unit/h/services/group_members_test.py b/tests/unit/h/services/group_members_test.py index 335b9ab6543..76ad6a15aeb 100644 --- a/tests/unit/h/services/group_members_test.py +++ b/tests/unit/h/services/group_members_test.py @@ -4,7 +4,7 @@ import pytest from sqlalchemy import select -from h.models import GroupMembership, User +from h.models import GroupMembership, GroupMembershipRoles, User from h.services.group_members import GroupMembersService, group_members_factory @@ -30,6 +30,76 @@ def test_it_when_theres_no_matching_membership( assert result is None +class TestGetMemberships: + def test_it(self, group_members_service, db_session, factories): + group, other_group = factories.Group.build_batch(size=2) + users = factories.User.build_batch(size=2) + memberships = [GroupMembership(group=group, user=user) for user in users] + db_session.add_all( + [*memberships, GroupMembership(group=other_group, user=users[0])] + ) + + assert list(group_members_service.get_memberships(group)) == memberships + + def test_roles(self, group_members_service, db_session, factories): + group = factories.Group.build() + admins = factories.User.build_batch(size=2) + moderator = factories.User.build() + memberships = [ + GroupMembership(group=group, user=user, roles=[GroupMembershipRoles.ADMIN]) + for user in admins + ] + db_session.add_all( + [ + *memberships, + GroupMembership( + group=group, user=moderator, roles=[GroupMembershipRoles.MODERATOR] + ), + ] + ) + + assert ( + list( + group_members_service.get_memberships( + group, roles=[GroupMembershipRoles.ADMIN] + ) + ) + == memberships + ) + + def test_multiple_roles(self, group_members_service, db_session, factories): + group = factories.Group.build() + admin = factories.User.build() + moderator = factories.User.build() + member = factories.User.build() + memberships = [ + GroupMembership( + group=group, user=admin, roles=[GroupMembershipRoles.ADMIN] + ), + GroupMembership( + group=group, user=moderator, roles=[GroupMembershipRoles.MODERATOR] + ), + ] + db_session.add_all( + [ + *memberships, + GroupMembership( + group=group, user=member, roles=[GroupMembershipRoles.MEMBER] + ), + ] + ) + + assert ( + list( + group_members_service.get_memberships( + group, + roles=[GroupMembershipRoles.ADMIN, GroupMembershipRoles.MODERATOR], + ) + ) + == memberships + ) + + class TestMemberJoin: def test_it_adds_user_to_group( self, group_members_service, factories, caplog, db_session diff --git a/tests/unit/h/views/api/flags_test.py b/tests/unit/h/views/api/flags_test.py index 57c579d92d4..f859d322bc7 100644 --- a/tests/unit/h/views/api/flags_test.py +++ b/tests/unit/h/views/api/flags_test.py @@ -1,86 +1,113 @@ -from unittest import mock +from unittest.mock import ANY, call, sentinel import pytest from pyramid.httpexceptions import HTTPNoContent +from h.models import GroupMembership, GroupMembershipRoles from h.traversal import AnnotationContext -from h.views.api import flags as views +from h.views.api import flags -@pytest.mark.usefixtures("flag_service") +@pytest.mark.usefixtures("flag_service", "group_members_service") class TestCreate: - def test_it(self, annotation_context, pyramid_request, flag_service): - response = views.create(annotation_context, pyramid_request) - - assert isinstance(response, HTTPNoContent) - flag_service.create.assert_called_once_with( - pyramid_request.user, annotation_context.annotation - ) - - @pytest.mark.parametrize("incontext_returns", (True, False)) - def test_it_sends_notification_email( + def test_it( self, - annotation_context, + annotation, + context, pyramid_request, - flag_notification, + flag_service, + links, + group_members_service, mailer, - incontext_link, - incontext_returns, + flag_notification, + moderators, ): - if not incontext_returns: - incontext_link.return_value = None - - views.create(annotation_context, pyramid_request) - - flag_notification.generate.assert_called_once_with( - request=pyramid_request, - email=annotation_context.annotation.group.creator.email, - incontext_link=( - incontext_link.return_value - if incontext_returns - else annotation_context.annotation.target_uri - ), + response = flags.create(context, pyramid_request) + + flag_service.create.assert_called_once_with(pyramid_request.user, annotation) + links.incontext_link.assert_called_once_with(pyramid_request, annotation) + group_members_service.get_memberships.assert_called_once_with( + annotation.group, + roles=[ + GroupMembershipRoles.OWNER, + GroupMembershipRoles.ADMIN, + GroupMembershipRoles.MODERATOR, + ], ) + assert flag_notification.generate.call_args_list == [ + call(pyramid_request, user.email, links.incontext_link.return_value) + for user in moderators + ] + assert mailer.send.delay.call_args_list == [ + call(sentinel.email1, sentinel.subject1, sentinel.text1, sentinel.html1), + call(sentinel.email2, sentinel.subject2, sentinel.text2, sentinel.html2), + ] + assert isinstance(response, HTTPNoContent) - mailer.send.delay.assert_called_once_with( - *flag_notification.generate.return_value - ) + def test_when_the_annotation_has_no_incontext_link( + self, context, pyramid_request, links, annotation, flag_notification + ): + links.incontext_link.return_value = None + + flags.create(context, pyramid_request) + + assert flag_notification.generate.call_args[0][2] == annotation.target_uri + + def test_when_a_moderator_has_no_email( + self, context, pyramid_request, moderators, flag_notification + ): + moderators[0].email = None + + flags.create(context, pyramid_request) + + assert flag_notification.generate.call_args_list == [ + call(ANY, moderators[1].email, ANY) + ] - @pytest.mark.parametrize("blank_field", ("creator", "creator_email")) - def test_doesnt_send_email_if_group_has_no_creator_or_email( - self, annotation_context, pyramid_request, mailer, blank_field + def test_when_there_are_no_moderators( + self, context, pyramid_request, group_members_service, flag_notification, mailer ): - if blank_field == "creator": - annotation_context.annotation.group.creator = None - else: - annotation_context.annotation.group.creator.email = None + group_members_service.get_memberships.return_value = [] - views.create(annotation_context, pyramid_request) + flags.create(context, pyramid_request) - assert not mailer.send.delay.called + flag_notification.generate.assert_not_called() + mailer.send.delay.assert_not_called() + + @pytest.fixture(autouse=True) + def moderators(self, factories, group_members_service): + moderators = factories.User.build_batch(2) + group_members_service.get_memberships.return_value = [ + GroupMembership(user=user) for user in moderators + ] + return moderators @pytest.fixture - def annotation_context(self, factories): - return mock.create_autospec( - AnnotationContext, - instance=True, - annotation=factories.Annotation(group=factories.Group()), - ) + def annotation(self, factories): + return factories.Annotation.build() @pytest.fixture - def pyramid_request(self, factories, pyramid_request, annotation_context): - pyramid_request.user = factories.User() - pyramid_request.json_body = {"annotation": annotation_context.annotation.id} - return pyramid_request + def context(self, annotation): + return AnnotationContext(annotation) - @pytest.fixture(autouse=True) - def flag_notification(self, patch): - return patch("h.views.api.flags.flag_notification") - @pytest.fixture(autouse=True) - def mailer(self, patch): - return patch("h.views.api.flags.mailer") +@pytest.fixture(autouse=True) +def links(mocker): + return mocker.patch("h.views.api.flags.links", autospec=True) - @pytest.fixture(autouse=True) - def incontext_link(self, patch): - return patch("h.views.api.flags.links.incontext_link") + +@pytest.fixture(autouse=True) +def flag_notification(mocker): + flag_notification = mocker.patch( + "h.views.api.flags.flag_notification", autospec=True + ) + flag_notification.generate.side_effect = [ + (sentinel.email1, sentinel.subject1, sentinel.text1, sentinel.html1), + (sentinel.email2, sentinel.subject2, sentinel.text2, sentinel.html2), + ] + return flag_notification + + +@pytest.fixture(autouse=True) +def mailer(mocker): + return mocker.patch("h.views.api.flags.mailer", autospec=True)