Change from offset & limit to number and size

h/schemas/pagination.py

@@ -2,15 +2,15 @@
 
 
 class PaginationQueryParamsSchema(Schema):
-    offset = SchemaNode(
+    page = SchemaNode(
         Integer(),
-        name="page[offset]",
-        validator=Range(min=0),
-        missing=0,
+        name="page[number]",
+        validator=Range(min=1),
+        missing=1,
     )
-    limit = SchemaNode(
+    size = SchemaNode(
         Integer(),
-        name="page[limit]",
+        name="page[size]",
         validator=Range(min=1, max=100),
         missing=20,
     )

h/static/scripts/group-forms/components/EditGroupMembersForm.tsx

@@ -84,17 +84,17 @@ async function fetchMembers(
   currentUserid: string,
   options: {
     signal: AbortSignal;
-    offset: number;
-    limit: number;
+    pageNumber: number;
+    pageSize: number;
   },
 ): Promise<{ total: number; members: MemberRow[] }> {
-  const { offset, limit, signal } = options;
+  const { pageNumber, pageSize, signal } = options;
   const { url, method, headers } = api;
   const { meta, data }: GroupMembersResponse = await callAPI(url, {
     headers,
-    limit,
+    pageSize,
     method,
-    offset,
+    pageNumber,
     signal,
   });
 
@@ -191,7 +191,7 @@ export default function EditGroupMembersForm({
   const config = useContext(Config)!;
   const currentUserid = config.context.user.userid;
 
-  const [pageIndex, setPageIndex] = useState(0);
+  const [pageNumber, setPageNumber] = useState(1);
   const [totalMembers, setTotalMembers] = useState<number | null>(null);
   const totalPages =
     totalMembers !== null ? Math.ceil(totalMembers / pageSize) : null;
@@ -216,8 +216,8 @@ export default function EditGroupMembersForm({
     const abort = new AbortController();
     setErrorMessage(null);
     fetchMembers(config.api.readGroupMembers, currentUserid, {
-      offset: pageIndex * pageSize,
-      limit: pageSize,
+      pageNumber,
+      pageSize,
       signal: abort.signal,
     })
       .then(({ total, members }) => {
@@ -230,7 +230,7 @@ export default function EditGroupMembersForm({
     return () => {
       abort.abort();
     };
-  }, [config.api.readGroupMembers, currentUserid, pageIndex, setError]);
+  }, [config.api.readGroupMembers, currentUserid, pageNumber, setError]);
 
   const columns: TableColumn<MemberRow>[] = [
     {
@@ -399,8 +399,8 @@ export default function EditGroupMembersForm({
         {typeof totalPages === 'number' && totalPages > 1 && (
           <div className="mt-4 flex justify-center">
             <Pagination
-              currentPage={pageIndex + 1}
-              onChangePage={page => setPageIndex(page - 1)}
+              currentPage={pageNumber}
+              onChangePage={setPageNumber}
               totalPages={totalPages}
             />
           </div>

h/static/scripts/group-forms/components/test/EditGroupMembersForm-test.js

@@ -307,8 +307,8 @@ describe('EditGroupMembersForm', () => {
       fakeCallAPI,
       '/api/groups/1234/members',
       sinon.match({
-        offset: pageSize,
-        limit: pageSize,
+        pageNumber: 2,
+        pageSize: pageSize,
       }),
     );
 

h/static/scripts/group-forms/utils/api.ts

@@ -112,11 +112,11 @@ export type APIOptions = {
   headers?: Record<PropertyKey, unknown>;
   signal?: AbortSignal;
 
-  /** Index of first item to return in paginated APIs. */
-  offset?: number;
+  /** 1-based number of first page to return in paginated APIs. */
+  pageNumber?: number;
 
   /** Maximum number of items to return in response for a paginated API. */
-  limit?: number;
+  pageSize?: number;
 };
 
 /** Make an API call and return the parsed JSON body or throw APIError. */
@@ -125,9 +125,9 @@ export async function callAPI<R = unknown>(
   {
     headers = {},
     json = null,
-    limit,
+    pageSize,
     method = 'GET',
-    offset,
+    pageNumber,
     signal,
   }: APIOptions = {},
 ): Promise<R> {
@@ -145,11 +145,11 @@ export async function callAPI<R = unknown>(
   }
 
   const queryParams: Record<string, string | number> = {};
-  if (typeof offset === 'number') {
-    queryParams['page[offset]'] = offset;
+  if (typeof pageNumber === 'number') {
+    queryParams['page[number]'] = pageNumber;
   }
-  if (typeof limit === 'number') {
-    queryParams['page[limit]'] = limit;
+  if (typeof pageSize === 'number') {
+    queryParams['page[size]'] = pageSize;
   }
 
   const requestURL = new URL(url);

h/static/scripts/group-forms/utils/test/api-test.js

@@ -126,14 +126,14 @@ describe('callAPI', () => {
 
   it('adds pagination params to URL', async () => {
     const paginatedURL = new URL(url);
-    const offset = 5;
-    const limit = 10;
-    paginatedURL.searchParams.set('page[offset]', offset);
-    paginatedURL.searchParams.set('page[limit]', limit);
+    const pageNumber = 5;
+    const pageSize = 10;
+    paginatedURL.searchParams.set('page[number]', pageNumber);
+    paginatedURL.searchParams.set('page[size]', pageSize);
     const response = new Response(JSON.stringify({}), { status: 200 });
     fakeFetch.resolves(response);
 
-    await callAPI(url, { offset, limit });
+    await callAPI(url, { pageNumber, pageSize });
 
     assert.calledWith(fakeFetch, paginatedURL.toString());
   });

h/views/api/group_members.py

@@ -31,7 +31,7 @@
 }
 
 
-@api_config(request_param=not_("page[offset]"), **LIST_MEMBERS_API_CONFIG)
+@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."""
 
@@ -55,14 +55,16 @@ def list_members_legacy(context: GroupContext, request):
     ]
 
 
-@api_config(request_param="page[offset]", **LIST_MEMBERS_API_CONFIG)
+@api_config(request_param="page[number]", **LIST_MEMBERS_API_CONFIG)
 def list_members(context: GroupContext, request):
     group = context.group
     group_members_service = request.find_service(name="group_members")
 
     params = validate_query_params(PaginationQueryParamsSchema(), request.params)
-    offset = params["page[offset]"]
-    limit = params["page[limit]"]
+    page_number = params["page[number]"]
+    page_size = params["page[size]"]
+    offset = page_size * (page_number - 1)
+    limit = page_size
 
     total = group_members_service.count_memberships(group)
     memberships = group_members_service.get_memberships(

tests/functional/api/groups/members_test.py

@@ -130,20 +130,20 @@ def test_it_returns_list_of_members_for_restricted_group_without_auth(
                     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))
+                for second, user in enumerate(factories.User.create_batch(size=9))
             ]
         )
         db_session.commit()
 
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
-            params={"page[offset]": 1, "page[limit]": 2},
+            params={"page[number]": 2, "page[size]": 3},
             headers={"User-Agent": "test_user_agent", "Referer": "test_referer"},
         )
 
         assert res.status_code == 200
         assert res.json == {
-            "meta": {"page": {"total": 4}},
+            "meta": {"page": {"total": 9}},
             "data": [
                 {
                     "authority": membership.group.authority,
@@ -155,7 +155,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))[3:6]
             ],
         }
 
@@ -183,7 +183,7 @@ 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},
+            params={"page[number]": 1},
             headers=token_authorization_header(token),
         )
 
@@ -214,6 +214,30 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group(
             ],
         }
 
+    def test_it_returns_empty_list_if_page_number_beyond_last_page(
+        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=2))
+            ]
+        )
+        db_session.commit()
+
+        res = app.get(
+            "/api/groups/{pubid}/members".format(pubid=group.pubid),
+            params={"page[number]": 2, "page[size]": 10},
+            headers={"User-Agent": "test_user_agent", "Referer": "test_referer"},
+        )
+
+        assert res.json["meta"]["page"]["total"] == 2
+        assert res.json["data"] == []
+
     def test_it_returns_404_if_user_does_not_have_read_access_to_group(
         self, app, db_session, factories
     ):
@@ -222,7 +246,7 @@ 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},
+            params={"page[number]": 1},
             headers=token_authorization_header(factories.DeveloperToken()),
             expect_errors=True,
         )
@@ -232,12 +256,12 @@ 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},
+            params={"page[number]": 1},
         )
 
         assert res.json == {"meta": {"page": {"total": 0}}, "data": []}
 
-    def test_it_returns_an_error_if_offset_and_limit_are_invalid(
+    def test_it_returns_an_error_if_number_and_size_are_invalid(
         self, app, db_session, factories
     ):
         group = factories.Group()
@@ -248,14 +272,14 @@ def test_it_returns_an_error_if_offset_and_limit_are_invalid(
 
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
-            params={"page[offset]": -1, "page[limit]": 0},
+            params={"page[number]": 0, "page[size]": 0},
             headers=token_authorization_header(token),
             expect_errors=True,
         )
 
         assert res.status_code == 400
         assert res.json == {
-            "reason": "page[offset]: -1 is less than minimum value 0\npage[limit]: 0 is less than minimum value 1",
+            "reason": "page[number]: 0 is less than minimum value 1\npage[size]: 0 is less than minimum value 1",
             "status": "failure",
         }
 

tests/unit/h/schemas/pagination_test.py

@@ -10,10 +10,10 @@ class TestPaginationQueryParamsSchema:
     @pytest.mark.parametrize(
         "input_,output",
         [
-            ({}, {"page[offset]": 0, "page[limit]": 20}),
+            ({}, {"page[number]": 1, "page[size]": 20}),
             (
-                {"page[offset]": 150, "page[limit]": 50},
-                {"page[offset]": 150, "page[limit]": 50},
+                {"page[number]": 150, "page[size]": 50},
+                {"page[number]": 150, "page[size]": 50},
             ),
         ],
     )
@@ -26,17 +26,17 @@ def test_valid(self, schema, input_, output):
         "input_,message",
         [
             (
-                {"page[offset]": -1},
-                r"^page\[offset\]: -1 is less than minimum value 0$",
+                {"page[number]": 0},
+                r"^page\[number\]: 0 is less than minimum value 1$",
             ),
-            ({"page[limit]": 0}, r"^page\[limit\]: 0 is less than minimum value 1$"),
+            ({"page[size]": 0}, r"^page\[size\]: 0 is less than minimum value 1$"),
             (
-                {"page[limit]": 101},
-                r"^page\[limit\]: 101 is greater than maximum value 100$",
+                {"page[size]": 101},
+                r"^page\[size\]: 101 is greater than maximum value 100$",
             ),
             (
-                {"page[offset]": "foo", "page[limit]": "bar"},
-                r'^page\[offset\]: "foo" is not a number\npage\[limit\]: "bar" is not a number$',
+                {"page[number]": "foo", "page[size]": "bar"},
+                r'^page\[number\]: "foo" is not a number\npage\[size\]: "bar" is not a number$',
             ),
         ],
     )

tests/unit/h/views/api/group_members_test.py

@@ -20,7 +20,7 @@
 from h.views.api.exceptions import PayloadError
 
 
-class TestListMembersLegacyLegacy:
+class TestListMembersLegacy:
     def test_it(
         self,
         context,
@@ -80,8 +80,8 @@ def test_it(
         validate_query_params,
     ):
         pyramid_request.params = validate_query_params.return_value = {
-            "page[offset]": sentinel.offset,
-            "page[limit]": sentinel.limit,
+            "page[number]": 3,
+            "page[size]": 2,
         }
         group_members_service.get_memberships.return_value = [
             sentinel.membership_1,
@@ -104,7 +104,7 @@ def test_it(
         )
         group_members_service.count_memberships.assert_called_once_with(context.group)
         group_members_service.get_memberships.assert_called_once_with(
-            context.group, offset=sentinel.offset, limit=sentinel.limit
+            context.group, offset=4, limit=2
         )
         assert GroupMembershipJSONPresenter.call_args_list == [
             call(pyramid_request, sentinel.membership_1),