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.

Sign up for GitHub

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

Remove legacy unpaginated list-group-members API #9183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove legacy unpaginated list-group-members API #9183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 8 additions & 21 deletions docs/_extra/api-reference/hypothesis-v1.yaml
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 HTTPConflict, HTTPNoContent, HTTPNotFound

from h.presenters import GroupMembershipJSONPresenter
Expand All @@ -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")
Expand Down
150 changes: 35 additions & 115 deletions tests/functional/api/groups/members_test.py
View file
Open in desktop
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[number]": 2, "page[size]": 3},
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))[3:6]
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[number]": 1},
headers=token_authorization_header(token),
)

Expand Down Expand Up @@ -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,
)
Expand All @@ -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
):
Expand Down
49 changes: 0 additions & 49 deletions tests/unit/h/views/api/group_members_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading