diff --git a/docs/_extra/api-reference/hypothesis-v1.yaml b/docs/_extra/api-reference/hypothesis-v1.yaml index ef31dfbf8c6..05d061ee144 100644 --- a/docs/_extra/api-reference/hypothesis-v1.yaml +++ b/docs/_extra/api-reference/hypothesis-v1.yaml @@ -889,13 +889,6 @@ paths: To get the memberships of a private group the authenticated user must be a member of the group. Getting the memberships of an open or restricted group does not require authentication. The returned memberships are sorted by joining date, oldest first. - - If the `page[number]` query param is included in the request then the response will be paginated. - If there's no `page[number]` query param in the request then a legacy, - un-paginated response format will be used. In the future this legacy - un-paginated response format will be removed and the paginated response - format will be used even when the `page[number]` query param is not - present. parameters: - $ref: '#/components/parameters/PageNumber' - $ref: '#/components/parameters/PageSize' @@ -909,22 +902,16 @@ paths: content: application/*json: schema: - oneOf: - - title: "Paginated" + type: object + properties: + meta: + description: "Metadata about this response." type: object properties: - meta: - description: "Metadata about this response." - type: object - properties: - page: - $ref: '#/components/schemas/PaginationMeta' - data: - description: "The list of memberships for the requested page." - type: array - items: - $ref: '#/components/schemas/Membership' - - title: "Unpaginated (deprecated)" + page: + $ref: '#/components/schemas/PaginationMeta' + data: + description: "The list of memberships for the requested page." type: array items: $ref: '#/components/schemas/Membership' diff --git a/docs/_extra/api-reference/hypothesis-v2.yaml b/docs/_extra/api-reference/hypothesis-v2.yaml index a8c16d78318..7c106f93d07 100644 --- a/docs/_extra/api-reference/hypothesis-v2.yaml +++ b/docs/_extra/api-reference/hypothesis-v2.yaml @@ -887,13 +887,6 @@ paths: To get the memberships of a private group the authenticated user must be a member of the group. Getting the memberships of an open or restricted group does not require authentication. The returned memberships are sorted by joining date, oldest first. - - If the `page[number]` query param is included in the request then the response will be paginated. - If there's no `page[number]` query param in the request then a legacy, - un-paginated response format will be used. In the future this legacy - un-paginated response format will be removed and the paginated response - format will be used even when the `page[number]` query param is not - present. parameters: - $ref: '#/components/parameters/PageNumber' - $ref: '#/components/parameters/PageSize' @@ -907,22 +900,16 @@ paths: content: application/*json: schema: - oneOf: - - title: "Paginated" + type: object + properties: + meta: + description: "Metadata about this response." type: object properties: - meta: - description: "Metadata about this response." - type: object - properties: - page: - $ref: '#/components/schemas/PaginationMeta' - data: - description: "The list of memberships for the requested page." - type: array - items: - $ref: '#/components/schemas/Membership' - - title: "Unpaginated (deprecated)" + page: + $ref: '#/components/schemas/PaginationMeta' + data: + description: "The list of memberships for the requested page." type: array items: $ref: '#/components/schemas/Membership' diff --git a/h/views/api/group_members.py b/h/views/api/group_members.py index eb85e90c5ed..0dc51e72519 100644 --- a/h/views/api/group_members.py +++ b/h/views/api/group_members.py @@ -1,6 +1,5 @@ import logging -from pyramid.config import not_ from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound from h.presenters import GroupMembershipJSONPresenter @@ -21,41 +20,14 @@ log = logging.getLogger(__name__) -LIST_MEMBERS_API_CONFIG = { - "versions": ["v1", "v2"], - "route_name": "api.group_members", - "request_method": "GET", - "link_name": "group.members.read", - "description": "Fetch a list of all members of a group", - "permission": Permission.Group.READ, -} - - -@api_config(request_param=not_("page[number]"), **LIST_MEMBERS_API_CONFIG) -def list_members_legacy(context: GroupContext, request): - """Legacy version of the list-members API, maintained for backwards-compatibility.""" - - log.info( - "list_members_legacy() was called. User-Agent: %s, Referer: %s, pubid: %s", - request.headers.get("User-Agent"), - request.headers.get("Referer"), - context.group.pubid, - ) - - # Get the list of memberships from GroupMembersService instead of just - # accessing `context.memberships` because GroupMembersService returns the - # memberships explictly sorted by creation date then username whereas - # `context.memberships` is unsorted. - group_members_service = request.find_service(name="group_members") - memberships = group_members_service.get_memberships(context.group) - - return [ - GroupMembershipJSONPresenter(request, membership).asdict() - for membership in memberships - ] - - -@api_config(request_param="page[number]", **LIST_MEMBERS_API_CONFIG) +@api_config( + versions=["v1", "v2"], + route_name="api.group_members", + request_method="GET", + link_name="group.members.read", + description="Fetch a list of all members of a group", + permission=Permission.Group.READ, +) def list_members(context: GroupContext, request): group = context.group group_members_service = request.find_service(name="group_members") diff --git a/tests/functional/api/groups/members_test.py b/tests/functional/api/groups/members_test.py index ae607b8f847..011a99f88fc 100644 --- a/tests/functional/api/groups/members_test.py +++ b/tests/functional/api/groups/members_test.py @@ -9,116 +9,6 @@ from h.models.auth_client import AuthClient, GrantType -class TestListMembersLegacy: - def test_it_returns_list_of_members_for_restricted_group_without_authn( - self, app, factories, db_session, caplog - ): - group = factories.RestrictedGroup( - memberships=[ - GroupMembership( - user=user, - created=datetime(1970, 1, 1, 0, 0, second), - updated=datetime(1970, 1, 2, 0, 0, second), - ) - for second, user in enumerate(factories.User.create_batch(size=3)) - ] - ) - db_session.commit() - - res = app.get( - "/api/groups/{pubid}/members".format(pubid=group.pubid), - headers={"User-Agent": "test_user_agent", "Referer": "test_referer"}, - ) - - assert caplog.messages == [ - f"list_members_legacy() was called. User-Agent: test_user_agent, Referer: test_referer, pubid: {group.pubid}" - ] - assert res.status_code == 200 - assert res.json == [ - { - "authority": membership.group.authority, - "userid": membership.user.userid, - "username": membership.user.username, - "display_name": membership.user.display_name, - "roles": membership.roles, - "actions": [], - "created": f"1970-01-01T00:00:{second:02}.000000+00:00", - "updated": f"1970-01-02T00:00:{second:02}.000000+00:00", - } - for second, membership in enumerate(group.memberships) - ] - - def test_it_returns_list_of_members_if_user_has_access_to_private_group( - self, app, factories, db_session - ): - group = factories.Group() - user, other_user = factories.User.create_batch(size=2) - token = factories.DeveloperToken(user=user) - group.memberships.extend( - [ - GroupMembership( - user=user, - created=datetime(1970, 1, 1, 0, 0, 0), - updated=datetime(1970, 1, 1, 0, 0, 1), - ), - GroupMembership( - user=other_user, - created=datetime(1971, 1, 2, 0, 0, 0), - updated=datetime(1971, 1, 2, 0, 0, 1), - ), - ] - ) - db_session.commit() - - res = app.get( - "/api/groups/{pubid}/members".format(pubid=group.pubid), - headers=token_authorization_header(token), - ) - - assert res.status_code == 200 - assert res.json == [ - { - "authority": group.authority, - "userid": user.userid, - "username": user.username, - "display_name": user.display_name, - "roles": [GroupMembershipRoles.MEMBER], - "actions": ["delete"], - "created": "1970-01-01T00:00:00.000000+00:00", - "updated": "1970-01-01T00:00:01.000000+00:00", - }, - { - "authority": group.authority, - "userid": other_user.userid, - "username": other_user.username, - "display_name": other_user.display_name, - "roles": [GroupMembershipRoles.MEMBER], - "actions": [], - "created": "1971-01-02T00:00:00.000000+00:00", - "updated": "1971-01-02T00:00:01.000000+00:00", - }, - ] - - def test_it_returns_404_if_user_does_not_have_read_access_to_group( - self, app, db_session, factories - ): - group = factories.Group() - db_session.commit() - - res = app.get( - "/api/groups/{pubid}/members".format(pubid=group.pubid), - headers=token_authorization_header(factories.DeveloperToken()), - expect_errors=True, - ) - - assert res.status_code == 404 - - def test_it_returns_empty_list_if_no_members_in_group(self, app): - res = app.get("/api/groups/__world__/members") - - assert res.json == [] - - class TestListMembers: def test_it_returns_list_of_members_for_restricted_group_without_auth( self, app, factories, db_session @@ -137,7 +27,6 @@ def test_it_returns_list_of_members_for_restricted_group_without_auth( res = app.get( "/api/groups/{pubid}/members".format(pubid=group.pubid), - params={"page[number]": 2, "page[size]": 3}, headers={"User-Agent": "test_user_agent", "Referer": "test_referer"}, ) @@ -155,7 +44,7 @@ def test_it_returns_list_of_members_for_restricted_group_without_auth( "created": f"1970-01-01T00:00:{second:02}.000000+00:00", "updated": f"1970-01-02T00:00:{second:02}.000000+00:00", } - for second, membership in list(enumerate(group.memberships))[3:6] + for second, membership in list(enumerate(group.memberships)) ], } @@ -183,7 +72,6 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group( res = app.get( "/api/groups/{pubid}/members".format(pubid=group.pubid), - params={"page[number]": 1}, headers=token_authorization_header(token), ) @@ -246,7 +134,6 @@ def test_it_returns_404_if_user_does_not_have_read_access_to_group( res = app.get( "/api/groups/{pubid}/members".format(pubid=group.pubid), - params={"page[number]": 1}, headers=token_authorization_header(factories.DeveloperToken()), expect_errors=True, ) @@ -256,11 +143,44 @@ def test_it_returns_404_if_user_does_not_have_read_access_to_group( def test_it_returns_empty_list_if_no_members_in_group(self, app): res = app.get( "/api/groups/__world__/members", - params={"page[number]": 1}, ) assert res.json == {"meta": {"page": {"total": 0}}, "data": []} + def test_pagination(self, app, factories, db_session): + group = factories.RestrictedGroup( + memberships=[ + GroupMembership( + user=user, + created=datetime(1970, 1, 1, 0, 0, second), + updated=datetime(1970, 1, 2, 0, 0, second), + ) + for second, user in enumerate(factories.User.create_batch(size=4)) + ] + ) + db_session.commit() + + res = app.get( + "/api/groups/{pubid}/members".format(pubid=group.pubid), + params={"page[offset]": 1, "page[limit]": 2}, + headers={"User-Agent": "test_user_agent", "Referer": "test_referer"}, + ) + + assert res.json["meta"]["page"]["total"] == 4 + assert res.json["data"] == [ + { + "authority": membership.group.authority, + "userid": membership.user.userid, + "username": membership.user.username, + "display_name": membership.user.display_name, + "roles": membership.roles, + "actions": [], + "created": f"1970-01-01T00:00:{second:02}.000000+00:00", + "updated": f"1970-01-02T00:00:{second:02}.000000+00:00", + } + for second, membership in list(enumerate(group.memberships)) + ] + def test_it_returns_an_error_if_number_and_size_are_invalid( self, app, db_session, factories ): diff --git a/tests/unit/h/views/api/group_members_test.py b/tests/unit/h/views/api/group_members_test.py index 25021b3611e..75f58e7dc02 100644 --- a/tests/unit/h/views/api/group_members_test.py +++ b/tests/unit/h/views/api/group_members_test.py @@ -20,55 +20,6 @@ from h.views.api.exceptions import PayloadError -class TestListMembersLegacy: - def test_it( - self, - context, - pyramid_request, - GroupMembershipJSONPresenter, - group_members_service, - caplog, - ): - pyramid_request.headers["User-Agent"] = sentinel.user_agent - pyramid_request.headers["Referer"] = sentinel.referer - group_members_service.get_memberships.return_value = [ - sentinel.membership_1, - sentinel.membership_2, - ] - - presenter_instances = GroupMembershipJSONPresenter.side_effect = [ - create_autospec( - presenters.GroupMembershipJSONPresenter, instance=True, spec_set=True - ), - create_autospec( - presenters.GroupMembershipJSONPresenter, instance=True, spec_set=True - ), - ] - - response = views.list_members_legacy(context, pyramid_request) - - assert caplog.messages == [ - f"list_members_legacy() was called. User-Agent: {sentinel.user_agent}, Referer: {sentinel.referer}, pubid: {context.group.pubid}" - ] - group_members_service.get_memberships.assert_called_once_with(context.group) - assert GroupMembershipJSONPresenter.call_args_list == [ - call(pyramid_request, sentinel.membership_1), - call(pyramid_request, sentinel.membership_2), - ] - presenter_instances[0].asdict.assert_called_once_with() - presenter_instances[1].asdict.assert_called_once_with() - assert response == [ - presenter_instances[0].asdict.return_value, - presenter_instances[1].asdict.return_value, - ] - - @pytest.fixture - def context(self, factories): - return create_autospec( - GroupContext, instance=True, spec_set=True, group=factories.Group() - ) - - class TestListMembers: def test_it( self,