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/views/api/group_members.py

@@ -1,8 +1,9 @@
 import logging
 
 from pyramid.config import not_
-from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound
+from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound
 
+from h.models import GroupMembershipRoles
 from h.presenters import GroupMembershipJSONPresenter
 from h.schemas.api.group_membership import EditGroupMembershipAPISchema
 from h.schemas.pagination import PaginationQueryParamsSchema
@@ -103,16 +104,22 @@ def add_member(context: GroupMembershipContext, request):
         appstruct = EditGroupMembershipAPISchema().validate(json_payload(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.
-        roles = None
+        roles = [GroupMembershipRoles.MEMBER]
+
+    kwargs = {"roles": roles}
 
     group_members_service = request.find_service(name="group_members")
 
     membership = group_members_service.member_join(
-        context.group, context.user.userid, roles
+        context.group, context.user.userid, **kwargs
     )
 
+    for key, value in kwargs.items():
+        if getattr(membership, key) != value:
+            raise HTTPConflict(
+                "The user is already a member of the group, with conflicting membership attributes"
+            )
+
     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/views/api/group_members_test.py

@@ -2,11 +2,11 @@
 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.models import GroupMembership, GroupMembershipRoles
 from h.schemas.base import ValidationError
 from h.security.identity import Identity, LongLivedGroup, LongLivedMembership
 from h.traversal import GroupContext, GroupMembershipContext
@@ -156,7 +156,11 @@ def test_it(
             sentinel.json_body
         )
         group_members_service.member_join.assert_called_once_with(
-            context.group, context.user.userid, roles=sentinel.roles
+            context.group,
+            context.user.userid,
+            roles=EditGroupMembershipAPISchema.return_value.validate.return_value[
+                "roles"
+            ],
         )
         GroupMembershipJSONPresenter.assert_called_once_with(
             pyramid_request, group_members_service.member_join.return_value
@@ -171,15 +175,32 @@ def test_it_with_no_request_body(
         context,
         EditGroupMembershipAPISchema,
     ):
+        group_members_service.member_join.return_value.roles = [
+            GroupMembershipRoles.MEMBER
+        ]
+
         pyramid_request.body = b""
 
         views.add_member(context, pyramid_request)
 
         EditGroupMembershipAPISchema.assert_not_called()
         group_members_service.member_join.assert_called_once_with(
-            context.group, context.user.userid, roles=None
+            context.group, context.user.userid, roles=[GroupMembershipRoles.MEMBER]
         )
 
+    def test_it_when_a_conflicting_membership_already_exists(
+        self, pyramid_request, group_members_service, context
+    ):
+        group_members_service.member_join.return_value.roles = [
+            GroupMembershipRoles.ADMIN
+        ]
+
+        with pytest.raises(
+            HTTPConflict,
+            match="^The user is already a member of the group, with conflicting membership attributes$",
+        ):
+            views.add_member(context, pyramid_request)
+
     def test_it_errors_if_the_request_isnt_valid_JSON(
         self, context, pyramid_request, mocker
     ):
@@ -217,10 +238,17 @@ def pyramid_request(self, pyramid_request):
     @pytest.fixture
     def EditGroupMembershipAPISchema(self, EditGroupMembershipAPISchema):
         EditGroupMembershipAPISchema.return_value.validate.return_value = {
-            "roles": sentinel.roles
+            "roles": [GroupMembershipRoles.MODERATOR]
         }
         return EditGroupMembershipAPISchema
 
+    @pytest.fixture
+    def group_members_service(self, group_members_service):
+        group_members_service.member_join.return_value.roles = [
+            GroupMembershipRoles.MODERATOR
+        ]
+        return group_members_service
+
 
 class TestEditMember:
     def test_it(