409 if a conflicting membership already exists

Change the add-group-membership API to return a 409 Conflict if a
conflicting membership (one with the same user and group but different
roles) already exists in the DB:

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["owner"]}
    -> 200 OK

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["admin"]}
    -> 409 Conflict

Previously this would have resulted in a 200 OK even though the
membership's role in the DB would *not* have been updated with the new
role from the second request.

For backwards-compatibility it will still respond 200 OK if the
subsequent request's role *does* match the existing role in the DB:

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["owner"]}
    -> 200 OK

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["owner"]}
    -> 200 OK

This also works if requests are made with no JSON body (which creates
memberships with the default role of "member"):

    POST /api/groups/{groupid}/members/{userid}
    (no body content)
    -> 200 OK

    POST /api/groups/{groupid}/members/{userid}
    (no body content)
    -> 200 OK

h/services/group_members.py

@@ -9,6 +9,10 @@
 log = logging.getLogger(__name__)
 
 
+class ConflictError(Exception):
+    """A conflicting group membership already exists in the DB."""
+
+
 class GroupMembersService:
     """A service for manipulating group membership."""
 
@@ -112,22 +116,33 @@ def update_members(self, group, userids):
             self.member_leave(group, userid)
 
     def member_join(self, group, userid, roles=None) -> GroupMembership:
-        """Add `userid` to the member list of `group`."""
+        """
+        Add `userid` to `group` with `roles` and return the resulting membership.
 
-        user = self.user_fetcher(userid)
+        If `roles=None` it will default to `[GroupMembershipRoles.MEMBER]`.
 
-        existing_membership = self.get_membership(group, user)
+        If a membership matching `group`, `userid` and `roles` already exists
+        in the DB it will just be returned.
 
-        if existing_membership:
-            # The user is already a member of the group.
-            return existing_membership
+        :raise ConflictError: if a membership already exists with the given
+            group and userid but different roles
+        """
+        roles = roles or [GroupMembershipRoles.MEMBER]
+
+        user = self.user_fetcher(userid)
 
-        kwargs = {"group": group, "user": user}
+        kwargs = {"roles": roles}
 
-        if roles is not None:
-            kwargs["roles"] = roles
+        if existing_membership := self.get_membership(group, user):
+            for key, value in kwargs.items():
+                if getattr(existing_membership, key) != value:
+                    raise ConflictError(
+                        "The user is already a member of the group, with conflicting membership attributes"
+                    )
+
+            return existing_membership
 
-        membership = GroupMembership(**kwargs)
+        membership = GroupMembership(group=group, user=user, **kwargs)
         self.db.add(membership)
 
         # Flush the DB to generate SQL defaults for `membership` before logging it.

h/views/api/group_members.py

@@ -1,13 +1,14 @@
 import logging
 
 from pyramid.config import not_
-from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound
+from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound
 
 from h.presenters import GroupMembershipJSONPresenter
 from h.schemas.api.group_membership import EditGroupMembershipAPISchema
 from h.schemas.pagination import PaginationQueryParamsSchema
 from h.schemas.util import validate_query_params
 from h.security import Permission
+from h.services.group_members import ConflictError
 from h.traversal import EditGroupMembershipContext, GroupContext, GroupMembershipContext
 from h.views.api.config import api_config
 from h.views.api.helpers.json_payload import json_payload
@@ -104,14 +105,17 @@ def add_member(context: GroupMembershipContext, request):
         roles = appstruct["roles"]
     else:
         # This doesn't mean the membership will be created with no roles:
-        # there's a server_default in the DB schema that will be applied.
+        # default roles will be applied by the service.
         roles = None
 
     group_members_service = request.find_service(name="group_members")
 
-    membership = group_members_service.member_join(
-        context.group, context.user.userid, roles
-    )
+    try:
+        membership = group_members_service.member_join(
+            context.group, context.user.userid, roles
+        )
+    except ConflictError as err:
+        raise HTTPConflict(str(err)) from err
 
     return GroupMembershipJSONPresenter(request, membership).asdict()
 

tests/functional/api/groups/members_test.py

@@ -287,6 +287,22 @@ def test_it_does_nothing_if_the_user_is_already_a_member_of_the_group(
 
         assert user in group.members
 
+    def test_it_when_a_conflicting_membership_already_exists(
+        self, do_request, group, user
+    ):
+        group.memberships.append(
+            GroupMembership(user=user, roles=[GroupMembershipRoles.MEMBER])
+        )
+
+        response = do_request(
+            json={"roles": [GroupMembershipRoles.MODERATOR]}, status=409
+        )
+
+        assert (
+            response.json["reason"]
+            == "The user is already a member of the group, with conflicting membership attributes"
+        )
+
     def test_it_errors_if_the_pubid_is_unknown(self, do_request):
         do_request(pubid="UNKNOWN_PUBID", status=404)
 

tests/unit/h/services/group_members_test.py

@@ -7,7 +7,11 @@
 from sqlalchemy import select
 
 from h.models import GroupMembership, GroupMembershipRoles, User
-from h.services.group_members import GroupMembersService, group_members_factory
+from h.services.group_members import (
+    ConflictError,
+    GroupMembersService,
+    group_members_factory,
+)
 
 
 class TestGetMembership:
@@ -210,7 +214,9 @@ def test_it_with_no_role(self, group_members_service, factories, db_session):
         ).one_or_none()
         assert membership.roles == [GroupMembershipRoles.MEMBER]
 
-    def test_it_is_idempotent(self, group_members_service, factories):
+    def test_it_when_a_matching_membership_already_exists(
+        self, group_members_service, factories
+    ):
         user = factories.User()
         group = factories.Group()
         existing_membership = group_members_service.member_join(group, user.userid)
@@ -220,6 +226,20 @@ def test_it_is_idempotent(self, group_members_service, factories):
         assert returned == existing_membership
         assert group.members.count(user) == 1
 
+    def test_it_when_a_conflicting_membership_already_exists(
+        self, group_members_service, factories
+    ):
+        user = factories.User()
+        group = factories.Group()
+        existing_membership = group_members_service.member_join(
+            group, user.userid, roles=[GroupMembershipRoles.MEMBER]
+        )
+
+        with pytest.raises(ConflictError):
+            group_members_service.member_join(
+                group, user.userid, roles=[GroupMembershipRoles.MODERATOR]
+            )
+
     def test_it_publishes_join_event(self, group_members_service, factories, publish):
         group = factories.Group()
         user = factories.User()

tests/unit/h/views/api/group_members_test.py

@@ -2,13 +2,14 @@
 from unittest.mock import PropertyMock, call, create_autospec, sentinel
 
 import pytest
-from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound
+from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound
 
 import h.views.api.group_members as views
 from h import presenters
 from h.models import GroupMembership
 from h.schemas.base import ValidationError
 from h.security.identity import Identity, LongLivedGroup, LongLivedMembership
+from h.services.group_members import ConflictError
 from h.traversal import GroupContext, GroupMembershipContext
 from h.views.api.exceptions import PayloadError
 
@@ -180,6 +181,16 @@ def test_it_with_no_request_body(
             context.group, context.user.userid, roles=None
         )
 
+    def test_it_when_a_conflicting_membership_already_exists(
+        self, pyramid_request, group_members_service, context
+    ):
+        group_members_service.member_join.side_effect = ConflictError(
+            "test_error_message"
+        )
+
+        with pytest.raises(HTTPConflict, match="^test_error_message$"):
+            views.add_member(context, pyramid_request)
+
     def test_it_errors_if_the_request_isnt_valid_JSON(
         self, context, pyramid_request, mocker
     ):