From 989ef9c21a2dc65bc607b6dcf3cabd8fcb3de54a Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 2 Dec 2021 09:44:43 +0000 Subject: [PATCH] Remove `last` and `total` keys from pagination links These don't appear to be used anywhere in the admin app and this route is only used by the admin app. Therefore it is safe to remove them. We remove them because the calculate the total number of notifications or the final page number of results can be particularly slow for services with many many notifications, for example 100 seconds for a service with 500k notifications sent in the past 7 days. Given neither are being used, this will give us the potential in the next commit to reduce the number of slow queries and improve page load times. Note, I've kept the scope small by only introducing the new pagination function for this one endpoint but there could be scope in future to get all pagination using the next function if appropriate. --- app/service/rest.py | 17 ++++++++++++++--- tests/app/service/test_rest.py | 8 -------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index a22373b02..4acd04bb3 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1,7 +1,7 @@ import itertools from datetime import datetime -from flask import Blueprint, current_app, jsonify, request +from flask import Blueprint, current_app, jsonify, request, url_for from notifications_utils.letter_timings import ( letter_can_be_cancelled, too_late_to_cancel_letter, @@ -441,11 +441,22 @@ def get_all_notifications_for_service(service_id): notifications = [notification.serialize_for_csv() for notification in pagination.items] else: notifications = notification_with_template_schema.dump(pagination.items, many=True).data + + + def get_prev_next_pagination_links(pagination, endpoint, **kwargs): + if 'page' in kwargs: + kwargs.pop('page', None) + links = {} + if pagination.has_prev: + links['prev'] = url_for(endpoint, page=pagination.prev_num, **kwargs) + if pagination.has_next: + links['next'] = url_for(endpoint, page=pagination.next_num, **kwargs) + return links + return jsonify( notifications=notifications, page_size=page_size, - total=pagination.total, - links=pagination_links( + links=get_prev_next_pagination_links( pagination, '.get_all_notifications_for_service', **kwargs diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index b63b27d66..020348918 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1881,11 +1881,9 @@ def test_get_notifications_for_service_without_page_count( count_pages=False ) assert len(resp['notifications']) == 1 - assert resp['total'] is None assert resp['notifications'][0]['id'] == str(without_job.id) assert 'prev' not in resp['links'] assert 'next' not in resp['links'] - assert 'last' not in resp['links'] def test_get_notifications_for_service_pagination_links( @@ -1902,10 +1900,8 @@ def test_get_notifications_for_service_pagination_links( service_id=sample_template.service_id ) - assert resp['total'] == 101 assert 'prev' not in resp['links'] assert '?page=2' in resp['links']['next'] - assert '?page=3' in resp['links']['last'] resp = admin_request.get( 'service.get_all_notifications_for_service', @@ -1913,10 +1909,8 @@ def test_get_notifications_for_service_pagination_links( page=2 ) - assert resp['total'] == 101 assert '?page=1' in resp['links']['prev'] assert '?page=3' in resp['links']['next'] - assert '?page=3' in resp['links']['last'] resp = admin_request.get( 'service.get_all_notifications_for_service', @@ -1924,10 +1918,8 @@ def test_get_notifications_for_service_pagination_links( page=3 ) - assert resp['total'] == 101 assert '?page=2' in resp['links']['prev'] assert 'next' not in resp['links'] - assert 'last' not in resp['links'] @pytest.mark.parametrize('should_prefix', [