Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add response body to add-member API #9188

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading