Skip to content

Commit

Permalink
Remove legacy unpaginated list-group-members API
Browse files Browse the repository at this point in the history
  • Loading branch information
@seanh
seanh committed Dec 12, 2024
1 parent 7f8c0f4 commit 0bf36c2
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 242 deletions.
29 changes: 8 additions & 21 deletions docs/_extra/api-reference/hypothesis-v1.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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'
Expand Down
29 changes: 8 additions & 21 deletions docs/_extra/api-reference/hypothesis-v2.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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'
Expand Down
44 changes: 8 additions & 36 deletions h/views/api/group_members.py
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import logging

from pyramid.config import not_
from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound

from h.presenters import GroupMembershipJSONPresenter
Expand All @@ -15,41 +14,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[offset]"), **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[offset]", **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")
Expand Down
150 changes: 35 additions & 115 deletions tests/functional/api/groups/members_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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[offset]": 1, "page[limit]": 2},
headers={"User-Agent": "test_user_agent", "Referer": "test_referer"},
)

Expand All @@ -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))[1:3]
for second, membership in list(enumerate(group.memberships))
],
}

Expand Down Expand Up @@ -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[offset]": 0},
headers=token_authorization_header(token),
)

Expand Down Expand Up @@ -222,7 +110,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[offset]": 0},
headers=token_authorization_header(factories.DeveloperToken()),
expect_errors=True,
)
Expand All @@ -232,11 +119,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[offset]": 0},
)

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))[1:3]
]

def test_it_returns_an_error_if_offset_and_limit_are_invalid(
self, app, db_session, factories
):
Expand Down
49 changes: 0 additions & 49 deletions tests/unit/h/views/api/group_members_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,55 +13,6 @@
from h.views.api.exceptions import PayloadError


class TestListMembersLegacyLegacy:
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,
Expand Down

0 comments on commit 0bf36c2

Please sign in to comment.