From f30187b529867ac0a4f99ba6978945a0c1259135 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 6 May 2020 14:30:32 +0100 Subject: [PATCH 1/2] =?UTF-8?q?Don=E2=80=99t=20show=20pagination=20links?= =?UTF-8?q?=20when=20searching?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The search form makes a post request, so that phone numbers and email addresses don’t show up in logs or browser history. At most the API will return 50 results, with some pagination links. We can’t easily give you links to click in the admin app, because links can only perform get requests. Because the value of seeing more than 50 results feels quite low (users will probably make their search more specific before scrolling through all 50) let’s just show a message saying only the first 50 results are displayed. --- app/main/views/jobs.py | 4 ++- .../views/activity/notifications.html | 8 ++++- tests/app/main/views/test_activity.py | 29 +++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index fe60ee2b6..8dd95d582 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -211,6 +211,7 @@ def get_notifications(service_id, message_type, status_override=None): filter_args = parse_filter_args(request.args) filter_args['status'] = set_status_filters(filter_args) service_data_retention_days = None + search_term = request.form.get('to', '') if message_type is not None: service_data_retention_days = current_service.get_days_of_retention(message_type) @@ -235,7 +236,7 @@ def get_notifications(service_id, message_type, status_override=None): template_type=[message_type] if message_type else [], status=filter_args.get('status'), limit_days=service_data_retention_days, - to=request.form.get('to', ''), + to=search_term, ) url_args = { 'message_type': message_type, @@ -284,6 +285,7 @@ def get_notifications(service_id, message_type, status_override=None): limit_days=service_data_retention_days, prev_page=prev_page, next_page=next_page, + more_than_can_be_shown=(next_page and search_term), status=request.args.get('status'), message_type=message_type, download_link=download_link, diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index ed98307eb..a39d99f6a 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -33,6 +33,12 @@ {% endif %} - {{ previous_next_navigation(prev_page, next_page) }} + {% if more_than_can_be_shown %} +

+ Only showing the first 50 messages +

+ {% else %} + {{ previous_next_navigation(prev_page, next_page) }} + {% endif %} diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index c75aa90e3..6b92c7bec 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -527,6 +527,35 @@ def test_should_show_notifications_for_a_service_with_next_previous( assert 'page 1' in prev_page_link.text.strip() +def test_doesnt_show_pagination_with_search_term( + client_request, + service_one, + active_user_with_permissions, + mock_get_notifications_with_previous_next, + mock_get_service_statistics, + mock_get_service_data_retention, + mock_get_no_api_keys, + mocker, +): + page = client_request.post( + 'main.view_notifications', + service_id=service_one['id'], + message_type='sms', + _data={ + 'to': 'test@example.com', + }, + _expected_status=200, + ) + assert len(page.select('tbody tr')) == 50 + assert not page.find('a', {'rel': 'next'}) + assert not page.find('a', {'rel': 'previous'}) + assert normalize_spaces( + page.select_one('.table-show-more-link').text + ) == ( + 'Only showing the first 50 messages' + ) + + @pytest.mark.parametrize( "job_created_at, expected_message", [ ("2016-01-10 11:09:00.000000+00:00", "Data available for 7 days"), From 8655e519aab441d141b889bb8381918bc97ac052 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 May 2020 09:50:03 +0100 Subject: [PATCH 2/2] Change logic around to be clearer Think this is a clearer expression of what the intent it. --- app/main/views/jobs.py | 2 +- app/templates/views/activity/notifications.html | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 8dd95d582..e95c3d61f 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -285,7 +285,7 @@ def get_notifications(service_id, message_type, status_override=None): limit_days=service_data_retention_days, prev_page=prev_page, next_page=next_page, - more_than_can_be_shown=(next_page and search_term), + show_pagination=(not search_term), status=request.args.get('status'), message_type=message_type, download_link=download_link, diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index a39d99f6a..c38d3cee5 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -33,12 +33,12 @@ {% endif %} - {% if more_than_can_be_shown %} + {% if show_pagination %} + {{ previous_next_navigation(prev_page, next_page) }} + {% elif next_page %}

Only showing the first 50 messages

- {% else %} - {{ previous_next_navigation(prev_page, next_page) }} {% endif %}