From 6d7d5a4e463e505a93a8c5ad7a67ce6ca50fd129 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 8 Jun 2016 16:34:26 +0100 Subject: [PATCH] Make jobs filterable by notification status We can filter all notifications by status already. This commit reuses the same code to filter the notifications for a job by status. This means that, visually we can show the count on a job the same as we do for all notifications, which is similar to how we show the counts on the dashboard, so hopefully it feels like a bit more of a solid thing. This also applies to CSV downloads and AJAX updates, which will inherit any filtering that their parent page has applied. --- app/main/views/jobs.py | 67 +++++++++++++++++-- app/notify_client/job_api_client.py | 7 +- app/templates/partials/jobs/count.html | 24 ++----- .../partials/jobs/notifications.html | 10 +-- app/templates/partials/jobs/status.html | 4 +- tests/app/main/views/test_jobs.py | 51 +++++++++++++- 6 files changed, 128 insertions(+), 35 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 0fd18f1c1..6bbcb7b5d 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -48,7 +48,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'): + if 'processed' in filter_args.get('status') or not filter_args.get('status'): filter_args['status'] = all_statuses elif 'failed' in filter_args.get('status'): filter_args['status'].extend(all_failure_statuses[1:]) @@ -75,7 +75,12 @@ def view_job(service_id, job_id): template = service_api_client.get_service_template(service_id=service_id, template_id=job['template'], version=job['template_version'])['data'] - notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + + filter_args = _parse_filter_args(request.args) + _set_status_filters(filter_args) + notifications = notification_api_client.get_notifications_for_service( + service_id, job_id, status=filter_args.get('status'), + ) finished = job['status'] == 'finished' return render_template( 'views/jobs/job.html', @@ -95,7 +100,9 @@ def view_job(service_id, job_id): template=Template( template, prefix=current_service['name'] - ) + ), + counts=_get_job_counts(job, request.args.get('help', 0)), + status=request.args.get('status', '') ) @@ -109,12 +116,15 @@ def view_job_csv(service_id, job_id): template_id=job['template'], version=job['template_version'] )['data'] + filter_args = _parse_filter_args(request.args) + _set_status_filters(filter_args) return ( generate_notifications_csv( notification_api_client.get_notifications_for_service( service_id, job_id, + status=filter_args.get('status'), page_size=job['notification_count'] )['notifications'] ), @@ -134,7 +144,11 @@ def view_job_csv(service_id, job_id): @user_has_permissions('view_activity', admin_override=True) def view_job_updates(service_id, job_id): job = job_api_client.get_job(service_id, job_id)['data'] - notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + filter_args = _parse_filter_args(request.args) + _set_status_filters(filter_args) + notifications = notification_api_client.get_notifications_for_service( + service_id, job_id, status=filter_args.get('status') + ) finished = ( job.get('notifications_sent', 0) - job.get('notifications_delivered', 0) - @@ -144,13 +158,16 @@ def view_job_updates(service_id, job_id): 'counts': render_template( 'partials/jobs/count.html', job=job, - finished=finished + finished=finished, + counts=_get_job_counts(job, request.args.get('help', 0)), + status=request.args.get('status', '') ), 'notifications': render_template( 'partials/jobs/notifications.html', job=job, notifications=notifications['notifications'], - finished=finished + finished=finished, + status=request.args.get('status', '') ), 'status': render_template( 'partials/jobs/status.html', @@ -269,3 +286,41 @@ def view_notification(service_id, job_id, notification_id): uploaded_at=now, job_id=job_id ) + + +def _get_job_counts(job, help_argument): + return [ + ( + label, + query_param, + url_for( + ".view_job", + service_id=job['service'], + job_id=job['id'], + status=query_param, + help=help_argument + ), + count + ) for label, query_param, count in [ + [ + 'Processed', '', + job.get('notifications_sent', 0) + ], + [ + 'Sending', 'sending', + ( + job.get('notifications_sent', 0) - + job.get('notifications_delivered', 0) - + job.get('notifications_failed', 0) + ) + ], + [ + 'Delivered', 'delivered', + job.get('notifications_delivered', 0) + ], + [ + 'Failed', 'failed', + job.get('notifications_failed') + ] + ] + ] diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 77d0d1b01..c3d45c5f5 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -14,9 +14,12 @@ class JobApiClient(BaseAPIClient): self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] self.secret = app.config['ADMIN_CLIENT_SECRET'] - def get_job(self, service_id, job_id=None, limit_days=None): + def get_job(self, service_id, job_id=None, limit_days=None, status=None): if job_id: - return self.get(url='/service/{}/job/{}'.format(service_id, job_id)) + params = {} + if status is not None: + params['status'] = status + return self.get(url='/service/{}/job/{}'.format(service_id, job_id), params=params) params = {} if limit_days is not None: params['limit_days'] = limit_days diff --git a/app/templates/partials/jobs/count.html b/app/templates/partials/jobs/count.html index 38ea4e5e7..a1ad9a0af 100644 --- a/app/templates/partials/jobs/count.html +++ b/app/templates/partials/jobs/count.html @@ -1,30 +1,16 @@ -{% from "components/big-number.html" import big_number %} +{% from "components/pill.html" import pill %}
- +
+ {{ pill('Status', counts, status) }} +
diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index c8dfb9a56..c045d7bae 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -1,10 +1,12 @@ {% from "components/table.html" import list_table, field, right_aligned_field_heading, date_field, row_heading %}
- Download as a CSV file + Download as a CSV file   Delivery information is available for 7 days

@@ -22,7 +24,7 @@ notifications, caption=uploaded_file_name, caption_visible=False, - empty_message="No messages to show yet", + empty_message="No messages to show", field_headings=[ 'Recipient', 'Time', diff --git a/app/templates/partials/jobs/status.html b/app/templates/partials/jobs/status.html index 2ef0016c7..ec4f0a308 100644 --- a/app/templates/partials/jobs/status.html +++ b/app/templates/partials/jobs/status.html @@ -1,12 +1,12 @@
-

+

Uploaded by {{ job.created_by.name }} on {{ job.created_at|format_datetime_short }}

diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 8d666a2ab..d6922f0a3 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -26,6 +26,30 @@ def test_should_return_list_of_all_jobs(app_, assert len(jobs) == 5 +@pytest.mark.parametrize( + "status_argument, expected_api_call", [ + ( + '', + ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ), + ( + 'processed', + ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ), + ( + 'sending', + ['sending'] + ), + ( + 'delivered', + ['delivered'] + ), + ( + 'failed', + ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ) + ] +) @freeze_time("2016-01-01 11:09:00.061258") def test_should_show_page_for_one_job( app_, @@ -36,13 +60,20 @@ def test_should_show_page_for_one_job( mock_get_job, mocker, mock_get_notifications, - fake_uuid + fake_uuid, + status_argument, + expected_api_call ): file_name = mock_get_job(service_one['id'], fake_uuid)['data']['original_file_name'] with app_.test_request_context(): with 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)) + response = client.get(url_for( + 'main.view_job', + service_id=service_one['id'], + job_id=fake_uuid, + status=status_argument + )) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -50,6 +81,22 @@ def test_should_show_page_for_one_job( assert file_name in content assert 'Delivered' in content assert '11:10' in content + assert url_for( + 'main.view_job_updates', + service_id=service_one['id'], + job_id=fake_uuid, + status=status_argument, + ) in content + assert url_for( + 'main.view_job_csv', + service_id=service_one['id'], + job_id=fake_uuid + ) in content + mock_get_notifications.assert_called_with( + service_one['id'], + fake_uuid, + status=expected_api_call + ) @freeze_time("2016-01-01 11:09:00.061258")