From 6c27a17e1ee16d7cdb2d5df8347ddbcf333e32e0 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 3 Jul 2018 10:54:33 +0100 Subject: [PATCH 1/2] Paginate complaints page The API now returns paginated complaint data, so the `/platform-admin/complaints` page can now be paginated. --- app/main/views/platform_admin.py | 22 ++++++++++-- app/notify_client/complaint_api_client.py | 5 +-- .../views/platform-admin/complaints.html | 4 ++- tests/app/main/views/test_platform_admin.py | 35 ++++++++++++++++++- .../notify_client/test_compliant_client.py | 11 +++++- 5 files changed, 69 insertions(+), 8 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index c4164ce10..30cba5208 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -12,7 +12,11 @@ from app import ( from app.main import main from app.main.forms import DateFilterForm from app.statistics_utils import get_formatted_percentage -from app.utils import user_is_platform_admin +from app.utils import ( + generate_next_dict, + generate_previous_dict, + user_is_platform_admin, +) COMPLAINT_THRESHOLD = 0.02 FAILURE_THRESHOLD = 3 @@ -190,11 +194,23 @@ def platform_admin_services(): @login_required @user_is_platform_admin def platform_admin_list_complaints(): - complaints = complaint_api_client.get_all_complaints() + page = int(request.args.get('page', 1)) + response = complaint_api_client.get_all_complaints(page=page) + + prev_page = None + if response['links'].get('prev'): + prev_page = generate_previous_dict('main.platform_admin_list_complaints', None, page) + next_page = None + if response['links'].get('next'): + next_page = generate_next_dict('main.platform_admin_list_complaints', None, page) + return render_template( 'views/platform-admin/complaints.html', - complaints=complaints, + complaints=response['complaints'], page_title='All Complaints', + page=page, + prev_page=prev_page, + next_page=next_page, ) diff --git a/app/notify_client/complaint_api_client.py b/app/notify_client/complaint_api_client.py index 72760d0b7..2d51f3ebc 100644 --- a/app/notify_client/complaint_api_client.py +++ b/app/notify_client/complaint_api_client.py @@ -7,8 +7,9 @@ class ComplaintApiClient(NotifyAdminAPIClient): def __init__(self): super().__init__("a" * 73, "b") - def get_all_complaints(self): - return self.get('/complaint') + def get_all_complaints(self, page=1): + params = {'page': page} + return self.get('/complaint', params=params) def get_complaint_count(self, params_dict=None): return self.get('/complaint/count-by-date-range', params=params_dict) diff --git a/app/templates/views/platform-admin/complaints.html b/app/templates/views/platform-admin/complaints.html index ff7c90a4a..a4e08465c 100644 --- a/app/templates/views/platform-admin/complaints.html +++ b/app/templates/views/platform-admin/complaints.html @@ -1,5 +1,6 @@ {% extends "views/platform-admin/_base_template.html" %} {% from "components/page-footer.html" import page_footer %} +{% from "components/previous-next-navigation.html" import previous_next_navigation %} {% from "components/table.html" import list_table, field, text_field, link_field, right_aligned_field_heading, hidden_field_heading %} {% block per_page_title %} @@ -28,9 +29,10 @@ {{ text_field(item.complaint_type) }} - {{ text_field(item.complaint_date|format_datetime_short) }} + {{ text_field(item.complaint_date|format_datetime_short if item.complaint_date else None) }} {% endcall %} + {{ previous_next_navigation(prev_page, next_page) }} {% endblock %} diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 8c587a23e..a4f5ba518 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -662,7 +662,7 @@ def test_platform_admin_list_complaints( 'created_at': '2018-06-05T13:50:30.012354', } mock = mocker.patch('app.complaint_api_client.get_all_complaints', - return_value=[complaint]) + return_value={'complaints': [complaint], 'links': {}}) client.login(platform_admin_user) response = client.get(url_for('main.platform_admin_list_complaints')) @@ -673,6 +673,39 @@ def test_platform_admin_list_complaints( assert mock.called +def test_should_show_complaints_with_next_previous(mocker, client, platform_admin_user, service_one, fake_uuid): + mock_get_user(mocker, user=platform_admin_user) + client.login(platform_admin_user) + + api_response = { + 'complaints': [{'complaint_date': None, + 'complaint_type': None, + 'created_at': '2017-12-18T05:00:00.000000Z', + 'id': fake_uuid, + 'notification_id': fake_uuid, + 'service_id': service_one['id'], + 'service_name': service_one['name'], + 'ses_feedback_id': 'None'}], + 'links': {'last': '/complaint?page=3', 'next': '/complaint?page=3', 'prev': '/complaint?page=1'} + } + + mocker.patch('app.complaint_api_client.get_all_complaints', return_value=api_response) + + response = client.get(url_for('main.platform_admin_list_complaints', page=2)) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + next_page_link = page.find('a', {'rel': 'next'}) + prev_page_link = page.find('a', {'rel': 'previous'}) + assert (url_for('main.platform_admin_list_complaints', page=3) in next_page_link['href']) + assert 'Next page' in next_page_link.text.strip() + assert 'page 3' in next_page_link.text.strip() + assert (url_for('main.platform_admin_list_complaints', page=1) in prev_page_link['href']) + assert 'Previous page' in prev_page_link.text.strip() + assert 'page 1' in prev_page_link.text.strip() + + @pytest.mark.parametrize('number, total, threshold, result', [ (0, 0, 0, False), (1, 1, 0, True), diff --git a/tests/app/notify_client/test_compliant_client.py b/tests/app/notify_client/test_compliant_client.py index 2665a14a7..f845725f7 100644 --- a/tests/app/notify_client/test_compliant_client.py +++ b/tests/app/notify_client/test_compliant_client.py @@ -7,7 +7,16 @@ def test_get_all_complaints(mocker): mock = mocker.patch('app.notify_client.complaint_api_client.ComplaintApiClient.get') client.get_all_complaints() - mock.assert_called_once_with('/complaint') + mock.assert_called_once_with('/complaint', params={'page': 1}) + + +def test_get_all_complaints_with_a_page_number_specified(mocker): + client = ComplaintApiClient() + + mock = mocker.patch('app.notify_client.complaint_api_client.ComplaintApiClient.get') + + client.get_all_complaints(page=3) + mock.assert_called_once_with('/complaint', params={'page': 3}) def test_get_complaint_count(mocker): From b27d29805b2075c32f18e94f75779d078d007768 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 4 Jul 2018 10:38:46 +0100 Subject: [PATCH 2/2] Raise error if getting complaints page with invalid page param If the `page` parameter passed to the `platform-admin/complaints` pageis not an integer, we should return a `404` error page. --- app/main/views/jobs.py | 2 +- app/main/views/platform_admin.py | 8 ++++++-- tests/app/main/views/test_platform_admin.py | 11 +++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 2549df1f7..fe2c3bc6d 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -199,7 +199,7 @@ def get_notifications(service_id, message_type, status_override=None): # TODO get the api to return count of pages as well. page = get_page_from_request() if page is None: - abort(404, "Invalid page argument ({}) reverting to page 1.".format(request.args['page'], None)) + abort(404, "Invalid page argument ({}).".format(request.args.get('page'))) if message_type not in ['email', 'sms', 'letter']: abort(404) filter_args = parse_filter_args(request.args) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 30cba5208..584fdfcf8 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -1,7 +1,7 @@ import itertools from datetime import datetime -from flask import render_template, request, url_for +from flask import abort, render_template, request, url_for from flask_login import login_required from app import ( @@ -15,6 +15,7 @@ from app.statistics_utils import get_formatted_percentage from app.utils import ( generate_next_dict, generate_previous_dict, + get_page_from_request, user_is_platform_admin, ) @@ -194,7 +195,10 @@ def platform_admin_services(): @login_required @user_is_platform_admin def platform_admin_list_complaints(): - page = int(request.args.get('page', 1)) + page = get_page_from_request() + if page is None: + abort(404, "Invalid page argument ({}).".format(request.args.get('page'))) + response = complaint_api_client.get_all_complaints(page=page) prev_page = None diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index a4f5ba518..047affe81 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -706,6 +706,17 @@ def test_should_show_complaints_with_next_previous(mocker, client, platform_admi assert 'page 1' in prev_page_link.text.strip() +def test_platform_admin_list_complaints_returns_404_with_invalid_page(mocker, client, platform_admin_user): + mock_get_user(mocker, user=platform_admin_user) + client.login(platform_admin_user) + + mocker.patch('app.complaint_api_client.get_all_complaints', return_value={'complaints': [], 'links': {}}) + + response = client.get(url_for('main.platform_admin_list_complaints', page='invalid')) + + assert response.status_code == 404 + + @pytest.mark.parametrize('number, total, threshold, result', [ (0, 0, 0, False), (1, 1, 0, True),