From 423875011c47bc38e842e96a6836037660c72203 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 17:31:01 +0100 Subject: [PATCH] Show jobs on contact list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a bit unintuitive that starting a job from a contact list makes a copy of the file, which has no relationship to the list it was copied from. This is more of an implementation detail, rather than something that comes from people’s mental models of what is going on. Or at least that’s what I hypothesise. I think it’s clearer to show jobs that come from contact lists within the lists that they were created from. By naming the jobs by template this gives a clearer view of what messages have been sent to the group over time. --- app/main/views/uploads.py | 4 +- app/models/contact_list.py | 4 +- app/models/job.py | 12 +- app/notify_client/job_api_client.py | 5 +- .../uploads/contact-list/contact-list.html | 140 ++++++++++++++---- tests/__init__.py | 2 + .../views/uploads/test_upload_contact_list.py | 119 ++++++++++++--- tests/app/models/test_contact_list.py | 1 + tests/conftest.py | 31 ++-- 9 files changed, 257 insertions(+), 61 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index ef9d53da6..5d8809dd9 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -513,9 +513,11 @@ def save_contact_list(service_id, upload_id): @main.route("/services//contact-list/", methods=['GET']) @user_has_permissions('send_messages') def contact_list(service_id, contact_list_id): + contact_list = ContactList.from_id(contact_list_id, service_id=service_id) return render_template( 'views/uploads/contact-list/contact-list.html', - contact_list=ContactList.from_id(contact_list_id, service_id=service_id), + contact_list=contact_list, + jobs=contact_list.get_jobs(page=1), ) diff --git a/app/models/contact_list.py b/app/models/contact_list.py index 0261ac28f..e1bbebbca 100644 --- a/app/models/contact_list.py +++ b/app/models/contact_list.py @@ -8,7 +8,7 @@ from notifications_utils.timezones import utc_string_to_aware_gmt_datetime from werkzeug.utils import cached_property from app.models import JSONModel, ModelList -from app.models.job import PaginatedJobs +from app.models.job import PaginatedJobsAndScheduledJobs from app.notify_client.contact_list_api_client import contact_list_api_client from app.s3_client.s3_csv_client import ( get_csv_metadata, @@ -136,7 +136,7 @@ class ContactList(JSONModel): return f'{file_name}.csv' def get_jobs(self, *, page): - return PaginatedJobs( + return PaginatedJobsAndScheduledJobs( self.service_id, contact_list_id=self.id, page=page, diff --git a/app/models/job.py b/app/models/job.py index af196c0f1..66a33ac38 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -233,9 +233,19 @@ class ScheduledJobs(ImmediateJobs): class PaginatedJobs(PaginatedModelList, ImmediateJobs): client_method = job_api_client.get_page_of_jobs + statuses = None def __init__(self, service_id, *, contact_list_id=None, page=None): - super().__init__(service_id, contact_list_id=contact_list_id, page=page) + super().__init__( + service_id, + contact_list_id=contact_list_id, + statuses=self.statuses, + page=page, + ) + + +class PaginatedJobsAndScheduledJobs(PaginatedJobs): + statuses = job_api_client.NON_CANCELLED_JOB_STATUSES class PaginatedUploads(PaginatedModelList, ImmediateJobs): diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index caea3d40d..e94a09dbe 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -16,6 +16,7 @@ class JobApiClient(NotifyAdminAPIClient): } SCHEDULED_JOB_STATUS = 'scheduled' CANCELLED_JOB_STATUS = 'cancelled' + NON_CANCELLED_JOB_STATUSES = JOB_STATUSES - {CANCELLED_JOB_STATUS} NON_SCHEDULED_JOB_STATUSES = JOB_STATUSES - {SCHEDULED_JOB_STATUS, CANCELLED_JOB_STATUS} def get_job(self, service_id, job_id): @@ -52,10 +53,10 @@ class JobApiClient(NotifyAdminAPIClient): if job['job_status'] != 'cancelled' ) - def get_page_of_jobs(self, service_id, *, page, contact_list_id=None): + def get_page_of_jobs(self, service_id, *, page, statuses=None, contact_list_id=None): return self.get_jobs( service_id, - statuses=self.NON_SCHEDULED_JOB_STATUSES, + statuses=statuses or self.NON_SCHEDULED_JOB_STATUSES, page=page, contact_list_id=contact_list_id, ) diff --git a/app/templates/views/uploads/contact-list/contact-list.html b/app/templates/views/uploads/contact-list/contact-list.html index eee292712..4fbd5397f 100644 --- a/app/templates/views/uploads/contact-list/contact-list.html +++ b/app/templates/views/uploads/contact-list/contact-list.html @@ -1,9 +1,10 @@ {% extends "withnav_template.html" %} {% from "components/banner.html" import banner_wrapper %} +{% from "components/big-number.html" import big_number %} {% from "components/radios.html" import radio_select %} -{% from "components/table.html" import list_table, field, text_field, index_field, hidden_field_heading %} +{% from "components/table.html" import list_table, field, text_field, index_field, hidden_field_heading, row, row_heading %} {% from "components/page-header.html" import page_header %} -{% from "components/message-count-label.html" import message_count_label, recipient_count_label %} +{% from "components/message-count-label.html" import message_count_label, recipient_count, recipient_count_label, iteration_count %} {% from "components/button/macro.njk" import govukButton %} {% block service_page_title %} @@ -18,41 +19,124 @@ ) }}

- Uploaded by {{ contact_list.created_by }} {{ contact_list.created_at|format_datetime_human }} + Uploaded by {{ contact_list.created_by }} {{ contact_list.created_at|format_datetime_human }}.

-

- Download this list  - {{ contact_list.recipients|length|format_thousands }} - {{ recipient_count_label(contact_list.recipients|length, contact_list.recipients.template_type) }} -

+ {% if jobs %} +

+ Used {{ iteration_count(jobs|length) }} + in the last {{ current_service.get_days_of_retention(contact_list.template_type) }} + days. +

+
+ {% call(item, row_number) list_table( + jobs, + caption="Messages sent from this contact list", + caption_visible=False, + empty_message='', + field_headings=[ + 'Template', + 'Status' + ], + field_headings_visible=False + ) %} + {% call row_heading() %} +
+ {{ item.template_name }} + {% if item.scheduled %} + + Sending {{ + item.scheduled_for|format_datetime_relative + }} + + {% else %} + + Sent {{ + (item.scheduled_for or item.created_at)|format_datetime_relative + }} + + {% endif %} - {% set recipient_column = contact_list.recipients.column_headers[0] %} - - {% call(item, row_number) list_table( - contact_list.recipients.displayed_rows, - caption=recipient_count_label(contact_list.recipients|length, contact_list.template_type)|capitalize, - caption_visible=False, - field_headings=['1', recipient_column], - ) %} - {{ index_field(row_number) }} - {{ text_field(item[recipient_column].data) }} - {% endcall %} - - {% if contact_list.recipients.displayed_rows|list|length < contact_list.recipients|length %} -

- Only showing the first {{ contact_list.recipients.displayed_rows|list|length }} rows +

+ {% endcall %} + {% call field() %} + {% if item.scheduled %} + {{ big_number( + item.notification_count, + smallest=True, + label=message_count_label( + item.notification_count, + item.template_type, + suffix='waiting to send' + ) + ) }} + {% else %} +
+
+ {{ big_number( + item.notifications_sending, + smallest=True, + label='sending', + ) }} +
+
+ {{ big_number(item.notifications_delivered, smallest=True, label='delivered') }} +
+
+ {{ big_number(item.notifications_failed, smallest=True, label='failed') }} +
+
+ {% endif %} + {% endcall %} + {% endcall %} +
+ {% else %} +

+ {% if contact_list.has_jobs %} + Not used in the last {{ current_service.get_days_of_retention(contact_list.template_type) }} days. + {% else %} + Not used yet. + {% endif %}

{% endif %} - {% if not confirm_delete_banner %} -
-
+

+ {{ recipient_count(contact_list.recipients|length, contact_list.template_type, prefix='saved') }} +

+ + {% set recipient_column = contact_list.recipients.column_headers[0] %} + +
+ {% call(item, row_number) list_table( + contact_list.recipients.displayed_rows, + caption=recipient_count_label(contact_list.recipients|length, contact_list.template_type)|capitalize, + caption_visible=False, + field_headings=[recipient_column], + field_headings_visible=False, + ) %} + {% if not loop.first %} + {{ text_field(item[recipient_column].data) }} + {% endif %} + {% endcall %} + + {% if contact_list.recipients.displayed_rows|list|length < contact_list.recipients|length %} +

+ Only showing the first {{ contact_list.recipients.displayed_rows|list|length }} rows +

+ {% endif %} +
+ + +
+
+ {% if not confirm_delete_banner %} Delete this contact list -
+ {% endif %} + Download this contact list
- {% endif %} +
+ {% endblock %} diff --git a/tests/__init__.py b/tests/__init__.py index 144b545f1..ff4866012 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -361,6 +361,7 @@ def job_json( template_id=None, template_version=1, template_type='sms', + template_name='Example template', created_at=None, bucket_name='', original_file_name="thisisatest.csv", @@ -381,6 +382,7 @@ def job_json( 'id': job_id, 'service': service_id, 'template': template_id, + 'template_name': template_name, 'template_version': template_version, 'template_type': template_type, 'original_file_name': original_file_name, diff --git a/tests/app/main/views/uploads/test_upload_contact_list.py b/tests/app/main/views/uploads/test_upload_contact_list.py index ba2e0fa1a..364db9f20 100644 --- a/tests/app/main/views/uploads/test_upload_contact_list.py +++ b/tests/app/main/views/uploads/test_upload_contact_list.py @@ -457,6 +457,64 @@ def test_view_contact_list( mocker, client_request, mock_get_contact_list, + mock_get_no_jobs, + mock_get_service_data_retention, + fake_uuid, +): + mocker.patch('app.models.contact_list.s3download', return_value='\n'.join( + ['email address'] + [ + f'test-{i}@example.com' for i in range(51) + ] + )) + page = client_request.get( + 'main.contact_list', + service_id=SERVICE_ONE_ID, + contact_list_id=fake_uuid, + ) + assert normalize_spaces(page.select_one('h1').text) == ( + 'EmergencyContactList.xls' + ) + assert normalize_spaces(page.select('main p')[0].text) == ( + 'Uploaded by Test User today at 10:59am.' + ) + assert normalize_spaces(page.select('main p')[1].text) == ( + 'Not used yet.' + ) + assert normalize_spaces(page.select_one('main h2').text) == ( + '51 saved email addresses' + ) + assert page.select_one('.js-stick-at-bottom-when-scrolling a[download]')['href'] == url_for( + 'main.download_contact_list', + service_id=SERVICE_ONE_ID, + contact_list_id=fake_uuid, + ) + assert len(page.select('tbody tr')) == 50 + assert [ + normalize_spaces(page.select('tbody tr')[0].text), + normalize_spaces(page.select('tbody tr')[1].text), + + normalize_spaces(page.select('tbody tr')[48].text), + normalize_spaces(page.select('tbody tr')[49].text), + ] == [ + 'test-0@example.com', + 'test-1@example.com', + + 'test-48@example.com', + 'test-49@example.com', + ] + assert 'test-50@example.com' not in page.select_one('tbody').text + assert normalize_spaces(page.select_one('.table-show-more-link').text) == ( + 'Only showing the first 50 rows' + ) + + +@freeze_time('2015-12-31 16:51:56') +def test_view_jobs_for_contact_list( + mocker, + client_request, + mock_get_contact_list, + mock_get_jobs, + mock_get_service_data_retention, fake_uuid, ): mocker.patch('app.models.contact_list.s3download', return_value='\n'.join( @@ -471,28 +529,51 @@ def test_view_contact_list( 'EmergencyContactList.xls' ) assert normalize_spaces(page.select('main p')[0].text) == ( - 'Uploaded by Test User today at 10:59am' + 'Uploaded by Test User on 13 March 2020 at 10:59am.' ) assert normalize_spaces(page.select('main p')[1].text) == ( - 'Download this list 51 email addresses' + 'Used 6 times in the last 7 days.' ) - assert page.select_one('a[download]')['href'] == url_for( - 'main.download_contact_list', + assert [ + normalize_spaces(row.text) + for row in page.select_one('table').select('tr') + ] == [ + 'Template Status', + ( + 'Template Y ' + 'Sending tomorrow at 11:09pm ' + '1 text message waiting to send' + ), + ( + 'Template Z ' + 'Sending tomorrow at 11:09am ' + '1 text message waiting to send' + ), + ( + 'Template A ' + 'Sent today at 4:51pm ' + '1 sending 0 delivered 0 failed' + ), + ( + 'Template B ' + 'Sent today at 4:51pm ' + '1 sending 0 delivered 0 failed' + ), + ( + 'Template C ' + 'Sent today at 4:51pm ' + '1 sending 0 delivered 0 failed' + ), + ( + 'Template D ' + 'Sent today at 4:51pm ' + '1 sending 0 delivered 0 failed' + ), + ] + assert page.select_one('table a')['href'] == url_for( + 'main.view_job', service_id=SERVICE_ONE_ID, - contact_list_id=fake_uuid, - ) - assert normalize_spaces(page.select_one('table').text).startswith( - 'Email addresses ' - '1 email address ' - '2 test@example.com ' - '3 test@example.com ' - ) - assert normalize_spaces(page.select_one('table').text).endswith( - '50 test@example.com ' - '51 test@example.com' - ) - assert normalize_spaces(page.select_one('.table-show-more-link').text) == ( - 'Only showing the first 50 rows' + job_id=fake_uuid, ) @@ -543,6 +624,8 @@ def test_confirm_delete_contact_list( mocker, client_request, fake_uuid, + mock_get_jobs, + mock_get_service_data_retention, mock_get_contact_list, ): mocker.patch( diff --git a/tests/app/models/test_contact_list.py b/tests/app/models/test_contact_list.py index 24c60ac3a..99ce7d3fb 100644 --- a/tests/app/models/test_contact_list.py +++ b/tests/app/models/test_contact_list.py @@ -20,6 +20,7 @@ def test_get_jobs(mock_get_jobs): 'finished', 'sending limits exceeded', 'ready to send', + 'scheduled', 'sent to dvla', 'pending', 'in progress', diff --git a/tests/conftest.py b/tests/conftest.py index 55c031677..f4ce31281 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1807,7 +1807,7 @@ def mock_has_no_jobs(mocker): @pytest.fixture(scope='function') -def mock_get_jobs(mocker, api_user_active): +def mock_get_jobs(mocker, api_user_active, fake_uuid): def _get_jobs(service_id, limit_days=None, statuses=None, contact_list_id=None, page=1): if statuses is None: statuses = ['', 'scheduled', 'pending', 'cancelled', 'finished'] @@ -1816,19 +1816,21 @@ def mock_get_jobs(mocker, api_user_active): job_json( service_id, api_user_active, + job_id=fake_uuid, original_file_name=filename, scheduled_for=scheduled_for, job_status=job_status, template_version=template_version, + template_name=template_name, ) - for filename, scheduled_for, job_status, template_version in ( - ('export 1/1/2016.xls', '', 'finished', 1), - ('all email addresses.xlsx', '', 'pending', 1), - ('applicants.ods', '', 'finished', 1), - ('thisisatest.csv', '', 'finished', 2), - ('send_me_later.csv', '2016-01-01 11:09:00.061258', 'scheduled', 1), - ('even_later.csv', '2016-01-01 23:09:00.061258', 'scheduled', 1), - ('full_of_regret.csv', '2016-01-01 23:09:00.061258', 'cancelled', 1) + for filename, scheduled_for, job_status, template_name, template_version in ( + ('full_of_regret.csv', '2016-01-01 23:09:00.061258', 'cancelled', 'Template X', 1), + ('even_later.csv', '2016-01-01 23:09:00.061258', 'scheduled', 'Template Y', 1), + ('send_me_later.csv', '2016-01-01 11:09:00.061258', 'scheduled', 'Template Z', 1), + ('export 1/1/2016.xls', '', 'finished', 'Template A', 1), + ('all email addresses.xlsx', '', 'pending', 'Template B', 1), + ('applicants.ods', '', 'finished', 'Template C', 1), + ('thisisatest.csv', '', 'finished', 'Template D', 2), ) ] return { @@ -2030,6 +2032,17 @@ def mock_get_no_uploads(mocker, api_user_active): ) +@pytest.fixture(scope='function') +def mock_get_no_jobs(mocker, api_user_active): + mocker.patch( + 'app.models.job.PaginatedJobs.client_method', + return_value={ + 'data': [], + 'links': {}, + } + ) + + @pytest.fixture(scope='function') def mock_create_contact_list(mocker, api_user_active): def _create(