From c649dc92cf5a3596fb50958ec5b0de65ca54fec8 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 12 Dec 2024 17:56:42 +0000 Subject: [PATCH] Add response body to add-member API Change the add-group-membership API's response from a 204 No Content to a 200 OK with a JSON representation of the added membership as the body. Fixes https://github.com/hypothesis/h/issues/9141 --- h/services/group_members.py | 6 ++++-- h/views/api/group_members.py | 8 ++++---- tests/functional/api/groups/members_test.py | 2 +- tests/unit/h/services/group_members_test.py | 9 ++++++--- tests/unit/h/views/api/group_members_test.py | 14 ++++++++++++-- 5 files changed, 27 insertions(+), 12 deletions(-) diff --git a/h/services/group_members.py b/h/services/group_members.py index f9f0a2f0278..22f326e06fd 100644 --- a/h/services/group_members.py +++ b/h/services/group_members.py @@ -111,7 +111,7 @@ def update_members(self, group, userids): for userid in userids_for_removal: self.member_leave(group, userid) - def member_join(self, group, userid): + def member_join(self, group, userid) -> GroupMembership: """Add `userid` to the member list of `group`.""" user = self.user_fetcher(userid) @@ -119,7 +119,7 @@ def member_join(self, group, userid): if existing_membership: # The user is already a member of the group. - return + return existing_membership membership = GroupMembership(group=group, user=user) self.db.add(membership) @@ -130,6 +130,8 @@ def member_join(self, group, userid): log.info("Added group membership: %r", membership) self.publish("group-join", group.pubid, userid) + return membership + def member_leave(self, group, userid): """Remove `userid` from the member list of `group`.""" user = self.user_fetcher(userid) diff --git a/h/views/api/group_members.py b/h/views/api/group_members.py index e04b0059871..0aab42d904b 100644 --- a/h/views/api/group_members.py +++ b/h/views/api/group_members.py @@ -96,14 +96,14 @@ def remove_member(context: GroupMembershipContext, request): permission=Permission.Group.MEMBER_ADD, ) def add_member(context: GroupMembershipContext, request): - group_members_service = request.find_service(name="group_members") - if context.user.authority != context.group.authority: raise HTTPNotFound() - group_members_service.member_join(context.group, context.user.userid) + group_members_service = request.find_service(name="group_members") - return HTTPNoContent() + membership = group_members_service.member_join(context.group, context.user.userid) + + return GroupMembershipJSONPresenter(request, membership).asdict() @api_config( diff --git a/tests/functional/api/groups/members_test.py b/tests/functional/api/groups/members_test.py index 5240cb5bfdf..2307df73efb 100644 --- a/tests/functional/api/groups/members_test.py +++ b/tests/functional/api/groups/members_test.py @@ -357,7 +357,7 @@ def test_me_alias_without_forwarded_user(self, do_request): @pytest.fixture def do_request(self, db_session, app, group, user, headers): - def do_request(pubid=group.pubid, userid=user.userid, status=204): + def do_request(pubid=group.pubid, userid=user.userid, status=200): db_session.commit() return app.post_json( f"/api/groups/{pubid}/members/{userid}", headers=headers, status=status diff --git a/tests/unit/h/services/group_members_test.py b/tests/unit/h/services/group_members_test.py index ccb42f0a6d8..a7d9a6e873b 100644 --- a/tests/unit/h/services/group_members_test.py +++ b/tests/unit/h/services/group_members_test.py @@ -183,7 +183,7 @@ def test_it_adds_user_to_group( caplog.set_level(logging.INFO) user = factories.User() group = factories.Group() - group_members_service.member_join(group, user.userid) + returned = group_members_service.member_join(group, user.userid) membership = db_session.scalars( select(GroupMembership) @@ -191,14 +191,17 @@ def test_it_adds_user_to_group( .where(GroupMembership.group == group) ).one_or_none() assert membership + assert returned == membership assert caplog.messages == [f"Added group membership: {membership!r}"] def test_it_is_idempotent(self, group_members_service, factories): user = factories.User() group = factories.Group() - group_members_service.member_join(group, user.userid) - group_members_service.member_join(group, user.userid) + existing_membership = group_members_service.member_join(group, user.userid) + + returned = group_members_service.member_join(group, user.userid) + assert returned == existing_membership assert group.members.count(user) == 1 def test_it_publishes_join_event(self, group_members_service, factories, publish): diff --git a/tests/unit/h/views/api/group_members_test.py b/tests/unit/h/views/api/group_members_test.py index 0b1f67af093..0555c6209b3 100644 --- a/tests/unit/h/views/api/group_members_test.py +++ b/tests/unit/h/views/api/group_members_test.py @@ -141,13 +141,23 @@ def context(self, factories): @pytest.mark.usefixtures("group_members_service") class TestAddMember: - def test_it(self, pyramid_request, group_members_service, context): + def test_it( + self, + pyramid_request, + group_members_service, + context, + GroupMembershipJSONPresenter, + ): response = views.add_member(context, pyramid_request) group_members_service.member_join.assert_called_once_with( context.group, context.user.userid ) - assert isinstance(response, HTTPNoContent) + GroupMembershipJSONPresenter.assert_called_once_with( + pyramid_request, group_members_service.member_join.return_value + ) + GroupMembershipJSONPresenter.return_value.asdict.assert_called_once_with() + assert response == GroupMembershipJSONPresenter.return_value.asdict.return_value def test_it_with_authority_mismatch(self, pyramid_request, context): context.group.authority = "other"