From 47e7d418eb81a95852baa16929d30b9e36c375e4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 1 Aug 2016 10:57:05 +0100 Subject: [PATCH 01/11] Remove redundant if statement We check for the `None` case in the parent `if` statement. --- app/main/views/jobs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index f043ddfca..648fb5c21 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -50,7 +50,7 @@ def _set_status_filters(filter_args): all_failure_statuses = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] all_statuses = ['sending', 'delivered'] + all_failure_statuses if filter_args.get('status'): - if 'processed' in filter_args.get('status') or not filter_args.get('status'): + if 'processed' in filter_args.get('status'): filter_args['status'] = all_statuses elif 'failed' in filter_args.get('status'): filter_args['status'].extend(all_failure_statuses[1:]) From 3fcfdc1bce7fcfeaff4c5ae36a10eb76a5328e9b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 1 Aug 2016 09:54:30 +0100 Subject: [PATCH 02/11] Make sending count down from total rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s weird when the sending number ramps up to ~200 or so and then just floats around as new rows are being added and older ones are being marked as delivered/failed. It’s also not great that you don’t know how many rows are in a file, if you haven’t uploaded it yourself. But the only reason you want to know this is to know how much work Notify has remaining to do. So ‘sending’ should start from the total number of rows in the file and count down. --- app/main/views/jobs.py | 2 +- app/templates/views/dashboard/_jobs.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 648fb5c21..1fc897f9a 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -293,7 +293,7 @@ def _get_job_counts(job, help_argument): [ 'Sending', 'sending', ( - job.get('notifications_sent', 0) - + job.get('notification_count', 0) - job.get('notifications_delivered', 0) - job.get('notifications_failed', 0) ) diff --git a/app/templates/views/dashboard/_jobs.html b/app/templates/views/dashboard/_jobs.html index b414e4456..0a2b3a2f7 100644 --- a/app/templates/views/dashboard/_jobs.html +++ b/app/templates/views/dashboard/_jobs.html @@ -23,7 +23,7 @@ {% endcall %} {% call field() %} {{ big_number( - item.get('notifications_sent', 0) - item.get('notifications_delivered', 0) - item.get('notifications_failed', 0), + item.get('notification_count', 0) - item.get('notifications_delivered', 0) - item.get('notifications_failed', 0), smallest=True ) }} {% endcall %} From 51a4ab806060892c5a51ffe75e1d0ee76d066bec Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 3 Aug 2016 09:47:27 +0100 Subject: [PATCH 03/11] =?UTF-8?q?Change=20=E2=80=98processed=E2=80=99=20to?= =?UTF-8?q?=20=E2=80=98total=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Processed is not an easy to understand thing, and the overlap with sending/delivered/failed is not easy to intuit. ‘Total’ is a much easier concept to grasp (it relates directly to your file), and it’s less distracting because it doesn’t change. --- app/main/views/jobs.py | 12 ++++++------ tests/app/main/views/test_jobs.py | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 1fc897f9a..5dbdc0d97 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -250,7 +250,7 @@ def get_status_filters(service, message_type, statistics): filters = [ # key, label, option - ('requested', 'processed', 'sending,delivered,failed'), + ('requested', 'total', 'sending,delivered,failed'), ('sending', 'sending', 'sending'), ('delivered', 'delivered', 'delivered'), ('failed', 'failed', 'failed'), @@ -287,8 +287,8 @@ def _get_job_counts(job, help_argument): count ) for label, query_param, count in [ [ - 'Processed', '', - job.get('notifications_sent', 0) + 'total', '', + job.get('notification_count', 0) ], [ 'Sending', 'sending', @@ -299,12 +299,12 @@ def _get_job_counts(job, help_argument): ) ], [ - 'Delivered', 'delivered', + 'delivered', 'delivered', job.get('notifications_delivered', 0) ], [ - 'Failed', 'failed', - job.get('notifications_failed') + 'failed', 'failed', + job.get('notifications_failed', 0) ] ] ] diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 7e3378e63..c5c2bc7fa 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -179,7 +179,7 @@ def test_should_show_updates_for_one_job_as_json( @pytest.mark.parametrize( "status_argument, expected_api_call", [ ( - 'processed', + '', ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] ), ( @@ -335,7 +335,7 @@ def test_get_status_filters_calculates_stats(app_): ret = get_status_filters({'id': 'foo'}, 'sms', STATISTICS) assert {label: count for label, _option, _link, count in ret} == { - 'processed': 6, + 'total': 6, 'sending': 3, 'failed': 2, 'delivered': 1 @@ -347,7 +347,7 @@ def test_get_status_filters_in_right_order(app_): ret = get_status_filters({'id': 'foo'}, 'sms', STATISTICS) assert [label for label, _option, _link, _count in ret] == [ - 'processed', 'sending', 'delivered', 'failed' + 'total', 'sending', 'delivered', 'failed' ] From 45ae43d987f0056842f48d9dba23f7af4cb79680 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 3 Aug 2016 09:47:12 +0100 Subject: [PATCH 04/11] =?UTF-8?q?Include=20=E2=80=98created=E2=80=99=20in?= =?UTF-8?q?=20=E2=80=98sending=E2=80=99=20bucket?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The difference between created and sending isn’t something a user should have to care about. So this commit: - counts created and sending as the same thing - displays and notifications which have a status of created as sending --- app/__init__.py | 9 ++++++--- app/main/views/jobs.py | 17 ++++++++--------- tests/app/main/views/test_jobs.py | 12 ++++-------- 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 9463305c3..2f39d2bee 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -243,7 +243,8 @@ def format_notification_status(status, template_type): 'temporary-failure': 'Inbox not accepting messages right now', 'permanent-failure': 'Email address doesn’t exist', 'delivered': 'Delivered', - 'sending': 'Sending' + 'sending': 'Sending', + 'created': 'Sending' }, 'sms': { 'failed': 'Failed', @@ -251,7 +252,8 @@ def format_notification_status(status, template_type): 'temporary-failure': 'Phone not accepting messages right now', 'permanent-failure': 'Phone number doesn’t exist', 'delivered': 'Delivered', - 'sending': 'Sending' + 'sending': 'Sending', + 'created': 'Sending' } }.get(template_type).get(status, status) @@ -263,7 +265,8 @@ def format_notification_status_as_field_status(status): 'temporary-failure': 'error', 'permanent-failure': 'error', 'delivered': None, - 'sending': 'default' + 'sending': 'default', + 'created': 'default' }.get(status, 'error') diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 5dbdc0d97..b623917e9 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -48,10 +48,11 @@ def _parse_filter_args(filter_dict): def _set_status_filters(filter_args): all_failure_statuses = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] - all_statuses = ['sending', 'delivered'] + all_failure_statuses + all_sending_statuses = ['created', 'sending'] + all_statuses = all_sending_statuses + ['delivered'] + all_failure_statuses if filter_args.get('status'): - if 'processed' in filter_args.get('status'): - filter_args['status'] = all_statuses + if 'sending' in filter_args.get('status'): + filter_args['status'].extend(all_sending_statuses[:1]) elif 'failed' in filter_args.get('status'): filter_args['status'].extend(all_failure_statuses[1:]) else: @@ -291,12 +292,10 @@ def _get_job_counts(job, help_argument): job.get('notification_count', 0) ], [ - 'Sending', 'sending', - ( - job.get('notification_count', 0) - - job.get('notifications_delivered', 0) - - job.get('notifications_failed', 0) - ) + 'sending', 'sending', + job.get('notification_count', 0) - + job.get('notifications_delivered', 0) - + job.get('notifications_failed', 0) ], [ 'delivered', 'delivered', diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index c5c2bc7fa..c13a42519 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -32,15 +32,11 @@ def test_should_return_list_of_all_jobs(app_, "status_argument, expected_api_call", [ ( '', - ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] - ), - ( - 'processed', - ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ['created', 'sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] ), ( 'sending', - ['sending'] + ['sending', 'created'] ), ( 'delivered', @@ -180,11 +176,11 @@ def test_should_show_updates_for_one_job_as_json( "status_argument, expected_api_call", [ ( '', - ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ['created', 'sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] ), ( 'sending', - ['sending'] + ['sending', 'created'] ), ( 'delivered', From e621dddd6d68ef92434fd65d0ada331171e7cc98 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 2 Aug 2016 21:27:23 +0100 Subject: [PATCH 05/11] =?UTF-8?q?Don=E2=80=99t=20show=20the=20download=20u?= =?UTF-8?q?ntil=20the=20job=20is=20complete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CSV report isn’t very useful until it has all the rows from your original file. So we shouldn’t show you the link until all notifications have been created. Until this point, it’s useful to know how much longer you need to wait, so this commit adds a percentage count of how much of the file has been processed. --- app/main/views/jobs.py | 9 +++++--- .../partials/jobs/notifications.html | 18 ++++++++++----- tests/app/main/views/test_jobs.py | 23 +++++++++++++++++++ tests/conftest.py | 12 ++++++++++ 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index b623917e9..1fe6d93aa 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -312,6 +312,9 @@ def _get_job_counts(job, help_argument): def get_job_partials(job): filter_args = _parse_filter_args(request.args) _set_status_filters(filter_args) + notifications = notification_api_client.get_notifications_for_service( + job['service'], job['id'], status=filter_args.get('status') + ) return { 'counts': render_template( 'partials/jobs/count.html', @@ -321,9 +324,9 @@ def get_job_partials(job): ), 'notifications': render_template( 'partials/jobs/notifications.html', - notifications=notification_api_client.get_notifications_for_service( - job['service'], job['id'], status=filter_args.get('status') - )['notifications'], + notifications=notifications['notifications'], + more_than_one_page=bool(notifications.get('links', {}).get('next')), + percentage_complete=(job['notifications_sent'] / job['notification_count'] * 100), download_link=url_for( '.view_job_csv', service_id=current_service['id'], diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 27243bfbb..20dca0c4f 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -4,12 +4,18 @@
{% endif %} - {% if notifications and not help %} -

- Download this report -   - {{ time_left }} -

+ {% if not help %} + {% if percentage_complete < 100 %} +

+ Report is {{ "{:.0f}%".format(percentage_complete) }} complete… +

+ {% elif notifications %} +

+ Download this report +   + {{ time_left }} +

+ {% endif %} {% endif %} {% call(item, row_number) list_table( diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index c13a42519..d33c85511 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -102,6 +102,29 @@ def test_should_show_page_for_one_job( ) +def test_should_show_job_in_progress( + app_, + service_one, + active_user_with_permissions, + mock_get_service_template, + mock_get_job_in_progress, + mocker, + mock_get_notifications, + fake_uuid +): + with app_.test_request_context(), app_.test_client() as client: + client.login(active_user_with_permissions, mocker, service_one) + response = client.get(url_for( + 'main.view_job', + service_id=service_one['id'], + job_id=fake_uuid + )) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.find('p', {'class': 'hint'}).text.strip() == 'Report is 50% complete…' + + def test_should_show_not_show_csv_download_in_tour( app_, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index 2ace067a9..47fd6e1b7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -858,6 +858,18 @@ def mock_get_job(mocker, api_user_active): return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) +@pytest.fixture(scope='function') +def mock_get_job_in_progress(mocker, api_user_active): + def _get_job(service_id, job_id): + return {"data": job_json( + service_id, api_user_active, job_id=job_id, + notification_count=10, + notifications_sent=5 + )} + + return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) + + @pytest.fixture(scope='function') def mock_get_jobs(mocker, api_user_active): def _get_jobs(service_id, limit_days=None): From 940815b33ca4b471031b66230b58441d3d2f90e6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 2 Aug 2016 16:43:47 +0100 Subject: [PATCH 06/11] Tighten up spacing around download link --- app/assets/stylesheets/_grids.scss | 4 ++++ app/templates/partials/jobs/notifications.html | 4 ++-- app/templates/views/notifications.html | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index 5231b67ae..183a85630 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -31,6 +31,10 @@ margin-bottom: $gutter-two-thirds; } +.bottom-gutter-1-2 { + margin-bottom: $gutter-half; +} + .align-with-heading { display: block; text-align: center; diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 20dca0c4f..6ed2b61b3 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -6,11 +6,11 @@ {% if not help %} {% if percentage_complete < 100 %} -

+

Report is {{ "{:.0f}%".format(percentage_complete) }} complete…

{% elif notifications %} -

+

Download this report   {{ time_left }} diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 69d8ed40e..f55d3c6a1 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -34,7 +34,7 @@

{% if notifications %} -

+

Download as a CSV file   Delivery information is available for 7 days From 9e6111fd005652ffde4ec8b3d21c10e13a5d46c4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 2 Aug 2016 16:51:31 +0100 Subject: [PATCH 07/11] =?UTF-8?q?Make=20=E2=80=98download=E2=80=99=20wordi?= =?UTF-8?q?ng=20consistent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We changed this on the jobs page in 9a5fbca707f35c2cee388c66f059b08f29e6087f Missed doing it on the notifications page. --- app/templates/views/notifications.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index f55d3c6a1..087722456 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -35,9 +35,9 @@ {% if notifications %}

- Download as a CSV file + Download this report   - Delivery information is available for 7 days + Data available for 7 days

{% endif %} From d0ef9135202ccfdae4f310b42dcc3d31b811b091 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 2 Aug 2016 10:34:27 +0100 Subject: [PATCH 08/11] Add a message if there are more than 50 rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Similar to how we do it on the check page, we should indicate if there are more results than we can show. No-one’s really complained about the absence of this, but it can’t hurt. --- app/templates/partials/jobs/notifications.html | 6 ++++++ tests/app/main/views/test_jobs.py | 1 + tests/conftest.py | 6 ++++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 6ed2b61b3..f915e6a11 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -50,6 +50,12 @@ {% endcall %} {% endcall %} + {% if more_than_one_page %} +

+ Only showing the first 50 rows +

+ {% endif %} + {% if notifications %} {% endif %} diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index d33c85511..c68a42068 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -95,6 +95,7 @@ def test_should_show_page_for_one_job( ) assert csv_link.text == 'Download this report' assert page.find('span', {'id': 'time-left'}).text == 'Data available for 7 days' + assert page.find('p', {'class': 'table-show-more-link'}).text.strip() == 'Only showing the first 50 rows' mock_get_notifications.assert_called_with( service_one['id'], fake_uuid, diff --git a/tests/conftest.py b/tests/conftest.py index 47fd6e1b7..1535d0080 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -910,14 +910,16 @@ def mock_get_notifications(mocker, api_user_active): template={'template_type': set_template_type, 'name': 'name', 'id': 'id', 'version': 1}, rows=rows, status=set_status, - job=job + job=job, + with_links=True ) else: return notification_json( service_id, rows=rows, status=set_status, - job=job + job=job, + with_links=True ) return mocker.patch( From 01bb2ada0a893e56071945d568de83250107d661 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Aug 2016 10:18:27 +0100 Subject: [PATCH 09/11] =?UTF-8?q?Don=E2=80=99t=20let=20=5Fset=5Fstatus=5Ff?= =?UTF-8?q?ilters=20mutate=20original=20list?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutating stuff is scary and prone to problems. Better for it to return a new list instead. --- app/main/views/jobs.py | 34 +++++++++++++------------- app/templates/views/notifications.html | 6 ++--- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 1fe6d93aa..34d6dacb4 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -1,8 +1,9 @@ # -*- coding: utf-8 -*- +import ago import time import dateutil from datetime import datetime, timedelta, timezone -import ago +from itertools import chain from flask import ( render_template, @@ -47,16 +48,15 @@ def _parse_filter_args(filter_dict): def _set_status_filters(filter_args): + status_filters = filter_args.get('status', []) all_failure_statuses = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] all_sending_statuses = ['created', 'sending'] all_statuses = all_sending_statuses + ['delivered'] + all_failure_statuses - if filter_args.get('status'): - if 'sending' in filter_args.get('status'): - filter_args['status'].extend(all_sending_statuses[:1]) - elif 'failed' in filter_args.get('status'): - filter_args['status'].extend(all_failure_statuses[1:]) - else: - filter_args['status'] = all_statuses + return list(chain( + (status_filters or all_statuses), + all_sending_statuses[:1] if 'sending' in status_filters else [], + all_failure_statuses[1:] if 'failed' in status_filters else [] + )) @main.route("/services//jobs") @@ -76,7 +76,7 @@ def view_job(service_id, job_id): job = job_api_client.get_job(service_id, job_id)['data'] filter_args = _parse_filter_args(request.args) - _set_status_filters(filter_args) + filter_args['status'] = _set_status_filters(filter_args) return render_template( 'views/jobs/job.html', @@ -118,7 +118,7 @@ def view_job_csv(service_id, job_id): version=job['template_version'] )['data'] filter_args = _parse_filter_args(request.args) - _set_status_filters(filter_args) + filter_args['status'] = _set_status_filters(filter_args) return ( generate_notifications_csv( @@ -162,7 +162,7 @@ def view_notifications(service_id, message_type): abort(404) filter_args = _parse_filter_args(request.args) - _set_status_filters(filter_args) + filter_args['status'] = _set_status_filters(filter_args) notifications = notification_api_client.get_notifications_for_service( service_id=service_id, @@ -172,7 +172,7 @@ def view_notifications(service_id, message_type): limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) view_dict = dict( message_type=message_type, - status=request.args.get('status') + status=filter_args['status'] ) prev_page = None if notifications['links'].get('prev', None): @@ -211,7 +211,7 @@ def view_notifications(service_id, message_type): page=page, prev_page=prev_page, next_page=next_page, - request_args=request.args, + status=request.args.get('status'), message_type=message_type, download_link=url_for( '.view_notifications_csv', @@ -311,16 +311,16 @@ def _get_job_counts(job, help_argument): def get_job_partials(job): filter_args = _parse_filter_args(request.args) - _set_status_filters(filter_args) + filter_args['status'] = _set_status_filters(filter_args) notifications = notification_api_client.get_notifications_for_service( - job['service'], job['id'], status=filter_args.get('status') + job['service'], job['id'], status=filter_args['status'] ) return { 'counts': render_template( 'partials/jobs/count.html', job=job, counts=_get_job_counts(job, request.args.get('help', 0)), - status=request.args.get('status', '') + status=filter_args['status'] ), 'notifications': render_template( 'partials/jobs/notifications.html', @@ -331,7 +331,7 @@ def get_job_partials(job): '.view_job_csv', service_id=current_service['id'], job_id=job['id'], - status=request.args.get('status', '') + status=request.args.get('status') ), help=get_help_argument(), time_left=get_time_left(job['created_at']) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 087722456..78b285549 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -14,9 +14,9 @@

@@ -29,7 +29,7 @@ {{ pill( 'Status', status_filters, - request_args.get('status', '') + status ) }} From b85b929b11eb4e82536dab2b41e02ef0c2aa7cb7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Aug 2016 10:59:36 +0100 Subject: [PATCH 10/11] Lose the confusing list slicing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Slicing was to avoid duplicate items in the list. A more idomatic way to avoid duplicate items in a list is to use a `set` instead. The order of the list doesn’t really matter, but it’s a lot easier to test for if the order is consistent. --- app/main/views/jobs.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 34d6dacb4..352ff5445 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -2,6 +2,7 @@ import ago import time import dateutil +from orderedset import OrderedSet from datetime import datetime, timedelta, timezone from itertools import chain @@ -51,12 +52,11 @@ def _set_status_filters(filter_args): status_filters = filter_args.get('status', []) all_failure_statuses = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] all_sending_statuses = ['created', 'sending'] - all_statuses = all_sending_statuses + ['delivered'] + all_failure_statuses - return list(chain( - (status_filters or all_statuses), - all_sending_statuses[:1] if 'sending' in status_filters else [], - all_failure_statuses[1:] if 'failed' in status_filters else [] - )) + return list(OrderedSet(chain( + (status_filters or all_sending_statuses + ['delivered'] + all_failure_statuses), + all_sending_statuses if 'sending' in status_filters else [], + all_failure_statuses if 'failed' in status_filters else [] + ))) @main.route("/services//jobs") From 652534533b1e7114b925cd9b7cebaf8e1362dac2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Aug 2016 11:33:32 +0100 Subject: [PATCH 11/11] Rewrite prev/next test to use BeautifulSoup --- app/main/views/jobs.py | 2 +- tests/app/main/views/test_jobs.py | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 352ff5445..749fbfa31 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -172,7 +172,7 @@ def view_notifications(service_id, message_type): limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) view_dict = dict( message_type=message_type, - status=filter_args['status'] + status=request.args.get('status') ) prev_page = None if notifications['links'].get('prev', None): diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index c68a42068..7bfbcefb2 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -295,10 +295,21 @@ def test_should_show_notifications_for_a_service_with_next_previous( )) assert response.status_code == 200 content = response.get_data(as_text=True) - assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=3) in content - assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=1) in content - assert 'Previous page' in content - assert 'Next page' in content + 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.view_notifications', service_id=service_one['id'], message_type='sms', 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.view_notifications', service_id=service_one['id'], message_type='sms', 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() @freeze_time("2016-01-01 11:09:00.061258")