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

Remove legacy unpaginated list-group-members API #9183

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

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Dec 4, 2024

Before merging this we should:

  • Check if anyone appears to be using the API and consider if we want to contact them to let them know about the change. We should be able to use this log message to find usages

@@ -116,7 +22,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},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the pagination query params from these functests, so they're now testing that the default behaviour of the API (without these query params) is paginated. Added a separate test for pagination below.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming that removing the legacy API can be done without causing significant disruption.

@seanh seanh force-pushed the remove-legacy-unpaginated-api branch from ac6c469 to 0bf36c2 Compare December 12, 2024 17:54
@seanh seanh changed the base branch from main to update-api-docs December 12, 2024 17:55
@seanh seanh requested a review from Copilot December 12, 2024 18:04

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 5 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • docs/_extra/api-reference/hypothesis-v1.yaml: Language not supported
  • docs/_extra/api-reference/hypothesis-v2.yaml: Language not supported
Comments skipped due to low confidence (1)

tests/functional/api/groups/members_test.py:47

  • The use of list(enumerate(group.memberships)) is unnecessary. It should be enumerate(group.memberships).
for second, membership in list(enumerate(group.memberships))
@seanh seanh force-pushed the update-api-docs branch 2 times, most recently from 117e029 to 6b20525 Compare January 7, 2025 17:25
Base automatically changed from update-api-docs to main January 7, 2025 17:35
@seanh seanh force-pushed the remove-legacy-unpaginated-api branch from 0bf36c2 to c8e062e Compare January 7, 2025 17:46
@seanh
Copy link
Contributor Author

seanh commented Jan 7, 2025

As far as I'm concerned this PR is ready to go as soon as we're satisfied that no one is still using the legacy API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants