Skip to content

Commit

Permalink
Add response body to add-member API
Browse files Browse the repository at this point in the history
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 #9141
  • Loading branch information
seanh committed Dec 12, 2024
1 parent fb09be7 commit c649dc9
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 12 deletions.
6 changes: 4 additions & 2 deletions h/services/group_members.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,15 @@ 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)

existing_membership = self.get_membership(group, user)

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)
Expand All @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions h/views/api/group_members.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/api/groups/members_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions tests/unit/h/services/group_members_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,22 +183,25 @@ 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)
.where(GroupMembership.user == user)
.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):
Expand Down
14 changes: 12 additions & 2 deletions tests/unit/h/views/api/group_members_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit c649dc9

Please sign in to comment.