From fc1ca3ab2fea8eb0f7625e7d32c49461bb2799d8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 17:06:23 +0100 Subject: [PATCH 1/9] Refactor to make pagination reusable The responses we get to paginated queries from the API are fairly consistent, so we should be able to reuse the code that takes JSON from the API and turns it into Python objects. This commits factors out that code so that it is reusable (by inheriting from it). --- app/models/__init__.py | 19 +++++++++++++++++++ app/models/job.py | 19 ++++++------------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/app/models/__init__.py b/app/models/__init__.py index 791c4a0c8..0acd9a31a 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -41,3 +41,22 @@ class ModelList(SerialisedModelCollection): def __init__(self, *args): self.items = self.client_method(*args) + + +class PaginatedModelList(ModelList): + + response_key = 'data' + + def __init__(self, *args, page=None, **kwargs): + try: + self.current_page = int(page) + except TypeError: + self.current_page = 1 + response = self.client_method( + *args, + **kwargs, + page=self.current_page, + ) + self.items = response[self.response_key] + self.prev_page = response.get('links', {}).get('prev', None) + self.next_page = response.get('links', {}).get('next', None) diff --git a/app/models/job.py b/app/models/job.py index 65679f3aa..e5443262e 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -9,7 +9,7 @@ from notifications_utils.letter_timings import ( 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 import JSONModel, ModelList, PaginatedModelList from app.notify_client.job_api_client import job_api_client from app.notify_client.notification_api_client import notification_api_client from app.notify_client.service_api_client import service_api_client @@ -25,6 +25,7 @@ class Job(JSONModel): ALLOWED_PROPERTIES = { 'id', 'service', + 'template_name', 'template_version', 'original_file_name', 'created_at', @@ -230,20 +231,12 @@ class ScheduledJobs(ImmediateJobs): client_method = job_api_client.get_scheduled_jobs -class PaginatedJobs(ImmediateJobs): - +class PaginatedJobs(PaginatedModelList, ImmediateJobs): client_method = job_api_client.get_page_of_jobs - def __init__(self, service_id, page=None): - try: - self.current_page = int(page) - except TypeError: - self.current_page = 1 - response = self.client_method(service_id, page=self.current_page) - self.items = response['data'] - self.prev_page = response.get('links', {}).get('prev', None) - self.next_page = response.get('links', {}).get('next', None) + def __init__(self, service_id, *, page=None): + super().__init__(service_id, page=page) -class PaginatedUploads(PaginatedJobs): +class PaginatedUploads(PaginatedModelList, ImmediateJobs): client_method = job_api_client.get_uploads From 236ddf053db35322f3bd3a2616e26313fdbe9dbd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 17:08:13 +0100 Subject: [PATCH 2/9] Make created_at a property of contact lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The view layer shouldn’t be having to deal with converting dates as strings. This is an artefact of how we send data from the API to the admin app. The model layer should be responsible for turning JSON into richer types, where it can. --- app/models/contact_list.py | 6 +++++- tests/app/models/test_contact_list.py | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 tests/app/models/test_contact_list.py diff --git a/app/models/contact_list.py b/app/models/contact_list.py index 9d4dfaaa7..355991c61 100644 --- a/app/models/contact_list.py +++ b/app/models/contact_list.py @@ -4,6 +4,7 @@ from os import path from flask import abort, current_app from notifications_utils.formatters import strip_whitespace from notifications_utils.recipients import RecipientCSV +from notifications_utils.timezones import utc_string_to_aware_gmt_datetime from werkzeug.utils import cached_property from app.models import JSONModel, ModelList @@ -21,7 +22,6 @@ class ContactList(JSONModel): ALLOWED_PROPERTIES = { 'id', - 'created_at', 'created_by', 'service_id', 'original_file_name', @@ -112,6 +112,10 @@ class ContactList(JSONModel): contact_list_id=self.id, ) + @property + def created_at(self): + return utc_string_to_aware_gmt_datetime(self._dict['created_at']) + @property def contents(self): return self.download(self.service_id, self.id) diff --git a/tests/app/models/test_contact_list.py b/tests/app/models/test_contact_list.py new file mode 100644 index 000000000..fb1f01c11 --- /dev/null +++ b/tests/app/models/test_contact_list.py @@ -0,0 +1,9 @@ +from datetime import datetime + +from app.models.contact_list import ContactList + + +def test_created_at(): + created_at = ContactList({'created_at': '2016-05-06T07:08:09.061258'}).created_at + assert isinstance(created_at, datetime) + assert created_at.isoformat() == '2016-05-06T08:08:09.061258+01:00' From 8f5c07336d54221d4cdd8ac599724ab408c7aaa8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 17:09:13 +0100 Subject: [PATCH 3/9] Add method to get jobs for a contact list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because the API lets us query jobs by contact list now, let’s add the model and client layer code that will let us display them in the admin app. --- app/models/contact_list.py | 8 ++++++++ app/models/job.py | 4 ++-- app/notify_client/job_api_client.py | 7 +++++-- tests/app/models/test_contact_list.py | 19 +++++++++++++++++++ tests/conftest.py | 2 +- 5 files changed, 35 insertions(+), 5 deletions(-) diff --git a/app/models/contact_list.py b/app/models/contact_list.py index 355991c61..0261ac28f 100644 --- a/app/models/contact_list.py +++ b/app/models/contact_list.py @@ -8,6 +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.notify_client.contact_list_api_client import contact_list_api_client from app.s3_client.s3_csv_client import ( get_csv_metadata, @@ -134,6 +135,13 @@ class ContactList(JSONModel): file_name, extention = path.splitext(self.original_file_name) return f'{file_name}.csv' + def get_jobs(self, *, page): + return PaginatedJobs( + self.service_id, + contact_list_id=self.id, + page=page, + ) + class ContactLists(ModelList): diff --git a/app/models/job.py b/app/models/job.py index e5443262e..af196c0f1 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -234,8 +234,8 @@ class ScheduledJobs(ImmediateJobs): class PaginatedJobs(PaginatedModelList, ImmediateJobs): client_method = job_api_client.get_page_of_jobs - def __init__(self, service_id, *, page=None): - super().__init__(service_id, page=page) + def __init__(self, service_id, *, contact_list_id=None, page=None): + super().__init__(service_id, contact_list_id=contact_list_id, page=page) class PaginatedUploads(PaginatedModelList, ImmediateJobs): diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 9e514af6f..caea3d40d 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -24,12 +24,14 @@ class JobApiClient(NotifyAdminAPIClient): return job - def get_jobs(self, service_id, limit_days=None, statuses=None, page=1): + def get_jobs(self, service_id, *, limit_days=None, contact_list_id=None, statuses=None, page=1): params = {'page': page} if limit_days is not None: params['limit_days'] = limit_days if statuses is not None: params['statuses'] = ','.join(statuses) + if contact_list_id is not None: + params['contact_list_id'] = contact_list_id return self.get(url='/service/{}/job'.format(service_id), params=params) @@ -50,11 +52,12 @@ class JobApiClient(NotifyAdminAPIClient): if job['job_status'] != 'cancelled' ) - def get_page_of_jobs(self, service_id, page): + def get_page_of_jobs(self, service_id, *, page, contact_list_id=None): return self.get_jobs( service_id, statuses=self.NON_SCHEDULED_JOB_STATUSES, page=page, + contact_list_id=contact_list_id, ) def get_immediate_jobs(self, service_id): diff --git a/tests/app/models/test_contact_list.py b/tests/app/models/test_contact_list.py index fb1f01c11..24c60ac3a 100644 --- a/tests/app/models/test_contact_list.py +++ b/tests/app/models/test_contact_list.py @@ -1,9 +1,28 @@ from datetime import datetime from app.models.contact_list import ContactList +from app.models.job import PaginatedJobs def test_created_at(): created_at = ContactList({'created_at': '2016-05-06T07:08:09.061258'}).created_at assert isinstance(created_at, datetime) assert created_at.isoformat() == '2016-05-06T08:08:09.061258+01:00' + + +def test_get_jobs(mock_get_jobs): + contact_list = ContactList({'id': 'a', 'service_id': 'b'}) + assert isinstance(contact_list.get_jobs(page=123), PaginatedJobs) + mock_get_jobs.assert_called_once_with( + 'b', + contact_list_id='a', + statuses={ + 'finished', + 'sending limits exceeded', + 'ready to send', + 'sent to dvla', + 'pending', + 'in progress', + }, + page=123, + ) diff --git a/tests/conftest.py b/tests/conftest.py index e7046e7d5..55c031677 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1808,7 +1808,7 @@ def mock_has_no_jobs(mocker): @pytest.fixture(scope='function') def mock_get_jobs(mocker, api_user_active): - def _get_jobs(service_id, limit_days=None, statuses=None, page=1): + 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'] From 149456b73ac86859ea925ae155133a9b9697223a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 17:30:42 +0100 Subject: [PATCH 4/9] Add some more macros for counting things --- .../components/message-count-label.html | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/app/templates/components/message-count-label.html b/app/templates/components/message-count-label.html index a4a9078de..b85d8f420 100644 --- a/app/templates/components/message-count-label.html +++ b/app/templates/components/message-count-label.html @@ -61,3 +61,19 @@ {%- endif -%} {%- endif %} {%- endmacro %} + + +{% macro recipient_count(count, template_type, prefix='') -%} + {{ count|format_thousands }} {{ prefix }} {{ recipient_count_label(count, template_type)}} +{% endmacro %} + + +{% macro iteration_count(count) -%} + {% if count == 1 %} + once + {% elif count == 2 %} + twice + {% else %} + {{ count }} times + {% endif %} +{% endmacro %} From 423875011c47bc38e842e96a6836037660c72203 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 17:31:01 +0100 Subject: [PATCH 5/9] 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( From 45b60e955588aa8d103aeddae17dfa78c02234b2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 17:52:13 +0100 Subject: [PATCH 6/9] Show usage count on uploads page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because we’re be grouping jobs under their parent contact lists it’s good to have some information ‘scent’ to help people find their jobs, ie by clicking into a contact list. It also lets you see which list have been used more than others, maybe because the update hasn’t been sent to that group of people yet. The hint text under uploads always says when they were used. For contact lists this is a bit more complicated, since they can: - never have been used - been used multiple times This commit makes use of the new fields being returned by the API to say determine when these messages are relevant. They also let us differentiate between a contact list that’s never been used, and one that has been used, but not recently enough to show any jobs against it. --- app/models/contact_list.py | 2 ++ app/templates/views/dashboard/_jobs.html | 18 +++++++++++++----- tests/app/main/views/test_send.py | 17 +++++++++-------- .../views/uploads/test_upload_contact_list.py | 2 +- .../app/main/views/uploads/test_upload_hub.py | 10 ++++++++-- tests/conftest.py | 16 ++++++++++++++++ 6 files changed, 49 insertions(+), 16 deletions(-) diff --git a/app/models/contact_list.py b/app/models/contact_list.py index e1bbebbca..294c53165 100644 --- a/app/models/contact_list.py +++ b/app/models/contact_list.py @@ -24,6 +24,8 @@ class ContactList(JSONModel): ALLOWED_PROPERTIES = { 'id', 'created_by', + 'has_jobs', + 'recent_job_count', 'service_id', 'original_file_name', 'row_count', diff --git a/app/templates/views/dashboard/_jobs.html b/app/templates/views/dashboard/_jobs.html index d65e3573f..7a8e0f9a2 100644 --- a/app/templates/views/dashboard/_jobs.html +++ b/app/templates/views/dashboard/_jobs.html @@ -1,6 +1,6 @@ {% from "components/table.html" import list_table, field, right_aligned_field_heading, row_heading %} {% from "components/big-number.html" import big_number -%} -{% 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_label, iteration_count -%}
{% call(item, row_number) list_table( @@ -35,10 +35,18 @@ {% elif item.upload_type == 'contact_list' %} - Uploaded {{ - item.created_at|format_datetime_relative - }} - + {% if item.recent_job_count %} + Used {{ iteration_count(item.recent_job_count) }} + in the last + {{ current_service.get_days_of_retention(item.template_type) }} + days + {% elif item.has_jobs %} + Not used in the last + {{ current_service.get_days_of_retention(item.template_type) }} + days + {% else %} + Not used yet + {% endif %} {% elif item.upload_type == 'letter_day' %} {{ item.letter_printing_statement }} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 875c9206f..cc906312f 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -4174,21 +4174,21 @@ def test_redirects_to_template_if_job_exists_already( @pytest.mark.parametrize(( 'template_type, ' 'expected_list_id, ' - 'expected_filename, ' + 'expected_filenames, ' 'expected_time, ' 'expected_count' ), ( ( 'email', '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6', - 'EmergencyContactList.xls', + ['EmergencyContactList.xls'], 'Uploaded today at 10:59am', '100 email addresses', ), ( 'sms', 'd7b0bd1a-d1c7-4621-be5c-3c1b4278a2ad', - 'phone number list.csv', + ['phone number list.csv', 'UnusedList.tsv'], 'Uploaded today at 1:00pm', '123 phone numbers', ), @@ -4201,7 +4201,7 @@ def test_choose_from_contact_list( fake_uuid, template_type, expected_list_id, - expected_filename, + expected_filenames, expected_time, expected_count, ): @@ -4215,10 +4215,11 @@ def test_choose_from_contact_list( service_id=SERVICE_ONE_ID, template_id=fake_uuid, ) - assert len(page.select('.file-list-filename-large')) == 1 - assert normalize_spaces(page.select_one('.file-list-filename-large').text) == ( - expected_filename - ) + assert [ + normalize_spaces(filename.text) + for filename in page.select('.file-list-filename-large') + ] == expected_filenames + assert page.select_one('a.file-list-filename-large')['href'] == url_for( 'main.send_from_contact_list', service_id=SERVICE_ONE_ID, 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 364db9f20..6f932e854 100644 --- a/tests/app/main/views/uploads/test_upload_contact_list.py +++ b/tests/app/main/views/uploads/test_upload_contact_list.py @@ -478,7 +478,7 @@ def test_view_contact_list( 'Uploaded by Test User today at 10:59am.' ) assert normalize_spaces(page.select('main p')[1].text) == ( - 'Not used yet.' + 'Not used in the last 7 days.' ) assert normalize_spaces(page.select_one('main h2').text) == ( '51 saved email addresses' diff --git a/tests/app/main/views/uploads/test_upload_hub.py b/tests/app/main/views/uploads/test_upload_hub.py index fae8de5d8..f9e44fafe 100644 --- a/tests/app/main/views/uploads/test_upload_hub.py +++ b/tests/app/main/views/uploads/test_upload_hub.py @@ -338,6 +338,7 @@ def test_uploads_page_shows_contact_lists_first( mock_get_no_uploads, mock_get_jobs, mock_get_contact_lists, + mock_get_service_data_retention, ): page = client_request.get('main.uploads', service_id=SERVICE_ONE_ID) @@ -349,14 +350,19 @@ def test_uploads_page_shows_contact_lists_first( ), ( 'phone number list.csv ' - 'Uploaded 13 March at 1:00pm ' + 'Used twice in the last 7 days ' '123 saved phone numbers' ), ( 'EmergencyContactList.xls ' - 'Uploaded 13 March at 10:59am ' + 'Not used in the last 7 days ' '100 saved email addresses' ), + ( + 'UnusedList.tsv ' + 'Not used yet ' + '1 saved phone number' + ), ( 'even_later.csv ' 'Sending 1 January 2016 at 11:09pm ' diff --git a/tests/conftest.py b/tests/conftest.py index f4ce31281..a283d3b82 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2075,6 +2075,8 @@ def mock_get_contact_lists(mocker, api_user_active, fake_uuid): 'id': fake_uuid, 'original_file_name': 'EmergencyContactList.xls', 'row_count': 100, + 'recent_job_count': 0, + 'has_jobs': True, 'service_id': service_id, 'template_type': 'email', }, { @@ -2083,6 +2085,18 @@ def mock_get_contact_lists(mocker, api_user_active, fake_uuid): 'id': 'd7b0bd1a-d1c7-4621-be5c-3c1b4278a2ad', 'original_file_name': 'phone number list.csv', 'row_count': 123, + 'recent_job_count': 2, + 'has_jobs': True, + 'service_id': service_id, + 'template_type': 'sms', + }, { + 'created_at': '2020-02-02 02:00:00', + 'created_by': 'Test User', + 'id': fake_uuid, + 'original_file_name': 'UnusedList.tsv', + 'row_count': 1, + 'recent_job_count': 0, + 'has_jobs': False, 'service_id': service_id, 'template_type': 'sms', }] @@ -2102,6 +2116,8 @@ def mock_get_contact_list(mocker, api_user_active, fake_uuid): 'id': fake_uuid, 'original_file_name': 'EmergencyContactList.xls', 'row_count': 100, + 'recent_job_count': 0, + 'has_jobs': True, 'service_id': service_id, 'template_type': 'email', } From 94c713e1b466bca1bd222c48cad8650bbf844201 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Nov 2020 14:14:26 +0000 Subject: [PATCH 7/9] Make dates line up in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Having a contact list uploaded in 2020 but used in 2015 isn’t possible, and so makes the tests confusing to read. --- .../views/uploads/test_upload_contact_list.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) 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 6f932e854..9b5afd112 100644 --- a/tests/app/main/views/uploads/test_upload_contact_list.py +++ b/tests/app/main/views/uploads/test_upload_contact_list.py @@ -512,11 +512,24 @@ def test_view_contact_list( 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.contact_list_api_client.get_contact_list', + return_value={ + 'created_at': '2015-12-31 12:12:12', + 'created_by': 'Test User', + 'id': fake_uuid, + 'original_file_name': 'EmergencyContactList.xls', + 'row_count': 100, + 'recent_job_count': 0, + 'has_jobs': True, + 'service_id': SERVICE_ONE_ID, + 'template_type': 'email', + }, + ) mocker.patch('app.models.contact_list.s3download', return_value='\n'.join( ['email address'] + ['test@example.com'] * 51 )) @@ -529,7 +542,7 @@ def test_view_jobs_for_contact_list( 'EmergencyContactList.xls' ) assert normalize_spaces(page.select('main p')[0].text) == ( - 'Uploaded by Test User on 13 March 2020 at 10:59am.' + 'Uploaded by Test User today at 12:12pm.' ) assert normalize_spaces(page.select('main p')[1].text) == ( 'Used 6 times in the last 7 days.' From a9783f790734d9d4990728d982401341b57dea87 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Nov 2020 14:20:53 +0000 Subject: [PATCH 8/9] Test for old but unused contact lists Old contact lists can be: - never used - used, but so long ago we no longer have data about the jobs due to retention settings We show different messages in each of these cases. This commit parametrizes the tests to ensure that both cases are covered. Also makes the job a bit older so that both cases are logically possible with the test data. --- .../views/uploads/test_upload_contact_list.py | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) 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 9b5afd112..e3bfd34cb 100644 --- a/tests/app/main/views/uploads/test_upload_contact_list.py +++ b/tests/app/main/views/uploads/test_upload_contact_list.py @@ -452,6 +452,10 @@ def test_cant_save_bad_contact_list( assert mock_create_contact_list.called is False +@pytest.mark.parametrize('has_jobs, expected_empty_message', [ + (False, 'Not used yet.'), + (True, 'Not used in the last 7 days.'), +]) @freeze_time('2020-03-13 16:51:56') def test_view_contact_list( mocker, @@ -460,7 +464,23 @@ def test_view_contact_list( mock_get_no_jobs, mock_get_service_data_retention, fake_uuid, + has_jobs, + expected_empty_message, ): + mocker.patch( + 'app.models.contact_list.contact_list_api_client.get_contact_list', + return_value={ + 'created_at': '2020-03-03 12:12:12', + 'created_by': 'Test User', + 'id': fake_uuid, + 'original_file_name': 'EmergencyContactList.xls', + 'row_count': 100, + 'recent_job_count': 0, + 'has_jobs': has_jobs, + 'service_id': SERVICE_ONE_ID, + 'template_type': 'email', + }, + ) mocker.patch('app.models.contact_list.s3download', return_value='\n'.join( ['email address'] + [ f'test-{i}@example.com' for i in range(51) @@ -475,10 +495,10 @@ 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 3 March at 12:12pm.' ) assert normalize_spaces(page.select('main p')[1].text) == ( - 'Not used in the last 7 days.' + expected_empty_message ) assert normalize_spaces(page.select_one('main h2').text) == ( '51 saved email addresses' From c898b68fa8d81963b7a5282e67ecb28764bbd0a3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Nov 2020 14:24:40 +0000 Subject: [PATCH 9/9] Add comment explaining mocking > I'd find it useful to know mock_get_jobs mocks > app.job_api_client.get_jobs here, perhaps through a comment. Reading > it, I associated it with contact_list.get_jobs above. --- tests/app/models/test_contact_list.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/app/models/test_contact_list.py b/tests/app/models/test_contact_list.py index 99ce7d3fb..95a93126c 100644 --- a/tests/app/models/test_contact_list.py +++ b/tests/app/models/test_contact_list.py @@ -13,6 +13,8 @@ def test_created_at(): def test_get_jobs(mock_get_jobs): contact_list = ContactList({'id': 'a', 'service_id': 'b'}) assert isinstance(contact_list.get_jobs(page=123), PaginatedJobs) + # mock_get_jobs mocks the underlying API client method, not + # contact_list.get_jobs mock_get_jobs.assert_called_once_with( 'b', contact_list_id='a',