From f9310dd2ed3ff63c20d4d0c961cfa1286e26acd9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 10:13:10 +0000 Subject: [PATCH 01/10] Use template_type property directly from API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s included in the job/upload response now[1]. So we don’t need to fetch the template every time we want to access it. 1. https://github.com/alphagov/notifications-api/pull/2728 --- app/models/job.py | 5 +---- .../partials/jobs/notifications.html | 8 +++---- tests/__init__.py | 2 ++ tests/app/main/views/test_jobs.py | 15 +++++++------ tests/conftest.py | 22 +++++++++++++++++++ 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/app/models/job.py b/app/models/job.py index 5cd92c3a0..dfe44f45d 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -30,6 +30,7 @@ class Job(JSONModel): 'processing_started', 'notification_count', 'created_by', + 'template_type', } @classmethod @@ -132,10 +133,6 @@ class Job(JSONModel): version=self.template_version, )['data'] - @property - def template_type(self): - return self.template['template_type'] - @property def percentage_complete(self): return self.notifications_requested / self.notification_count * 100 diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 842a9d805..86c52fb57 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -25,21 +25,21 @@
{% endif %} - {% if template.template_type == 'letter' %} + {% if job.template_type == 'letter' %}
{% endif %} {% if job.still_processing %} -

+

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

{% elif notifications %} -

+

Download this report   {{ time_left }}

{% endif %} - {% if template.template_type == 'letter' %} + {% if job.template_type == 'letter' %}
{% endif %} diff --git a/tests/__init__.py b/tests/__init__.py index 70f04ed12..eaaf86886 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -354,6 +354,7 @@ def job_json( job_id=None, template_id=None, template_version=1, + template_type='sms', created_at=None, bucket_name='', original_file_name="thisisatest.csv", @@ -375,6 +376,7 @@ def job_json( 'service': service_id, 'template': template_id, 'template_version': template_version, + 'template_type': template_type, 'original_file_name': original_file_name, 'created_at': created_at, 'notification_count': notification_count, diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 2a9e7b69e..23e012b1f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -398,7 +398,7 @@ def test_should_show_old_job( def test_should_show_letter_job( client_request, mock_get_service_letter_template, - mock_get_job, + mock_get_letter_job, mock_get_service_data_retention, fake_uuid, mocker, @@ -460,7 +460,7 @@ def test_should_show_letter_job( def test_should_show_letter_job_with_banner_after_sending_before_1730( client_request, mock_get_service_letter_template, - mock_get_job, + mock_get_letter_job, mock_get_notifications, mock_get_service_data_retention, fake_uuid, @@ -483,7 +483,7 @@ def test_should_show_letter_job_with_banner_after_sending_before_1730( def test_should_show_letter_job_with_banner_when_there_are_multiple_CSV_rows( client_request, mock_get_service_letter_template, - mock_get_job_in_progress, + mock_get_letter_job_in_progress, mock_get_notifications, mock_get_service_data_retention, fake_uuid, @@ -506,7 +506,7 @@ def test_should_show_letter_job_with_banner_when_there_are_multiple_CSV_rows( def test_should_show_letter_job_with_banner_after_sending_after_1730( client_request, mock_get_service_letter_template, - mock_get_job, + mock_get_letter_job, mock_get_notifications, mock_get_service_data_retention, fake_uuid, @@ -601,7 +601,8 @@ def test_should_cancel_letter_job( active_user_with_permissions, job_id=job_id, created_at="2019-06-20T15:30:00.000001+00:00", - job_status="finished" + job_status="finished", + template_type="letter", ) mocker.patch('app.job_api_client.get_job', side_effect=[{"data": job}]) notifications_json = notification_json(SERVICE_ONE_ID, job=job, status="created", template_type="letter") @@ -819,7 +820,7 @@ def test_time_left(job_created_at, expected_message): def test_should_show_letter_job_with_first_class_if_notifications_are_first_class( client_request, mock_get_service_letter_template, - mock_get_job, + mock_get_letter_job, mock_get_service_data_retention, fake_uuid, mocker, @@ -840,7 +841,7 @@ def test_should_show_letter_job_with_first_class_if_notifications_are_first_clas def test_should_show_letter_job_with_first_class_if_no_notifications( client_request, service_one, - mock_get_job, + mock_get_letter_job, fake_uuid, mock_get_notifications_with_no_notifications, mock_get_service_data_retention, diff --git a/tests/conftest.py b/tests/conftest.py index 3b3cd7ed6..046d3f7ad 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1656,6 +1656,14 @@ 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_letter_job(mocker, api_user_active): + def _get_job(service_id, job_id): + return {"data": job_json(service_id, api_user_active, job_id=job_id, template_type='letter')} + + return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) + + @pytest.fixture def mock_get_job_doesnt_exist(mocker): def _get_job(service_id, job_id): @@ -1705,6 +1713,20 @@ def mock_get_job_in_progress(mocker, api_user_active): return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) +@pytest.fixture(scope='function') +def mock_get_letter_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_requested=5, + job_status='processing', + template_type='letter', + )} + + return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) + + @pytest.fixture(scope='function') def mock_has_jobs(mocker): mocker.patch('app.job_api_client.has_jobs', return_value=True) From 0d4c97e64a65456af98d8eb440b423618404bbad Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 10:39:02 +0000 Subject: [PATCH 02/10] Make sticky footer clear the previous/next nav MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we don’t do this then the sticky footer only takes up half the width of the column. --- .../stylesheets/components/vendor/previous-next-navigation.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/stylesheets/components/vendor/previous-next-navigation.scss b/app/assets/stylesheets/components/vendor/previous-next-navigation.scss index 8fc2213a0..5db0b9be8 100644 --- a/app/assets/stylesheets/components/vendor/previous-next-navigation.scss +++ b/app/assets/stylesheets/components/vendor/previous-next-navigation.scss @@ -34,6 +34,7 @@ $is-ie: false !default; margin-bottom: $gutter; margin-left: -$gutter-half; margin-right: -$gutter-half; + overflow: hidden; ul { margin: 0; From eed5e0fdd76be211edb41d50869f11d47549c42a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 10:49:23 +0000 Subject: [PATCH 03/10] Combine scheduled and already-sent jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’m hoping that if I can design something that clearly differentiates them then we won’t need to do so by putting them in separate tables, which then need labelling, which would clutter up the page. --- app/main/views/uploads.py | 10 ++++++---- app/models/__init__.py | 8 ++++++++ app/models/service.py | 6 +++--- tests/app/main/views/test_uploads.py | 6 +++--- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 65edfcfc2..961b1ae80 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -52,14 +52,16 @@ def uploads(service_id): if uploads.prev_page: next_page = generate_next_dict('main.uploads', service_id, uploads.current_page) + if uploads.current_page == 1: + listed_uploads = current_service.scheduled_jobs + uploads + else: + listed_uploads = uploads + return render_template( 'views/jobs/jobs.html', - jobs=uploads, + jobs=listed_uploads, prev_page=prev_page, next_page=next_page, - show_scheduled_jobs=( - uploads.current_page == 1 and current_service.scheduled_jobs - ), ) diff --git a/app/models/__init__.py b/app/models/__init__.py index 4f8850e9d..6034f4fb3 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -76,5 +76,13 @@ class ModelList(ABC, Sequence): return list(self) + list(other) +class EmptyModelList(ModelList): + + client_method = model = None + + def __init__(self, *args): + self.items = [] + + class InviteTokenError(Exception): pass diff --git a/app/models/service.py b/app/models/service.py index 132ac157c..0ee6217b0 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -5,7 +5,7 @@ from flask import abort, current_app from notifications_utils.timezones import local_timezone from werkzeug.utils import cached_property -from app.models import JSONModel +from app.models import EmptyModelList, JSONModel from app.models.job import ( ImmediateJobs, PaginatedJobs, @@ -123,13 +123,13 @@ class Service(JSONModel): @cached_property def immediate_jobs(self): if not self.has_jobs: - return [] + return EmptyModelList() return ImmediateJobs(self.id) @cached_property def scheduled_jobs(self): if not self.has_jobs: - return [] + return EmptyModelList() return ScheduledJobs(self.id) @cached_property diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index cbb7a5396..b36791533 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -643,15 +643,15 @@ def test_uploads_page_shows_scheduled_jobs( normalize_spaces(row.text) for row in page.select('tr') ] == [ ( - 'File Messages to be sent' + 'File Sending Delivered Failed' ), ( 'send_me_later.csv ' - 'Sending 1 January 2016 at 11:09am 1' + 'Sent 1 January 2016 at 11:09am 0 0 0' ), ( 'even_later.csv ' - 'Sending 1 January 2016 at 11:09pm 1' + 'Sent 1 January 2016 at 11:09pm 0 0 0' ), ] assert not page.select('.table-empty-message') From 2770e5013b9c7389713388645395e8e2302fcf73 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 10:58:04 +0000 Subject: [PATCH 04/10] Use smaller heading size for Uploads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because it’s a top-level page without a back link it should have a smaller heading size. --- app/templates/views/jobs/jobs.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/jobs/jobs.html b/app/templates/views/jobs/jobs.html index b7d7088c5..33c49f0ff 100644 --- a/app/templates/views/jobs/jobs.html +++ b/app/templates/views/jobs/jobs.html @@ -7,7 +7,7 @@ {% endblock %} {% block maincolumn_content %} -

Uploads

+

Uploads

{% if show_scheduled_jobs %} {% with hide_heading = True %} From ee8436ca859f03910f14b64c28ded4a724ba3fd8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 14:03:03 +0000 Subject: [PATCH 05/10] Differentiate between different kinds of uploads Knowing what kind of upload a thing is is useful. And the information that is useful to show about each upload depends on what kind of upload it is. --- app/assets/stylesheets/components/table.scss | 4 + app/assets/stylesheets/views/dashboard.scss | 12 +++ app/models/job.py | 5 ++ app/templates/views/dashboard/_jobs.html | 85 +++++++++++++++----- tests/app/main/views/test_dashboard.py | 7 +- tests/app/main/views/test_jobs.py | 30 +++---- tests/app/main/views/test_uploads.py | 12 +-- tests/conftest.py | 13 ++- 8 files changed, 123 insertions(+), 45 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 3b4a4a49b..d69b25af5 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -49,6 +49,10 @@ } } + .big-number-label { + padding-bottom: 0; + } + } .template-statistics-table { diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index b300d5be2..8faef80c7 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -56,6 +56,18 @@ margin-top: -10px; } + &-filename-large { + @include bold-24; + display: block; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + padding-bottom: 30px; + padding-top: 10px; + margin-bottom: -30px; + margin-top: -10px; + } + &-filename-unlinked { @include core-19; } diff --git a/app/models/job.py b/app/models/job.py index dfe44f45d..02d40159e 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -31,6 +31,7 @@ class Job(JSONModel): 'notification_count', 'created_by', 'template_type', + 'recipient', } @classmethod @@ -57,6 +58,10 @@ class Job(JSONModel): def upload_type(self): return self._dict.get('upload_type') + @property + def pdf_letter(self): + return self.upload_type == 'letter' + @property def processing_started(self): if not self._dict.get('processing_started'): diff --git a/app/templates/views/dashboard/_jobs.html b/app/templates/views/dashboard/_jobs.html index 3d4c9bba4..dfbaedb54 100644 --- a/app/templates/views/dashboard/_jobs.html +++ b/app/templates/views/dashboard/_jobs.html @@ -1,5 +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 -%}
{% call(item, row_number) list_table( @@ -11,37 +12,79 @@ ), field_headings=[ 'File', - 'Sending', - 'Delivered', - 'Failed' + 'Status' ], - field_headings_visible=True if jobs else False + field_headings_visible=False ) %} {% call row_heading() %}
{% if item.upload_type == 'letter' %} - {{ item.original_file_name }} + {{ item.original_file_name }} {% else %} - {{ item.original_file_name }} + {{ item.original_file_name }} {% endif %} - - Sent {{ - (item.scheduled_for or item.created_at)|format_datetime_relative - }} - + {% if item.scheduled %} + + Sending {{ + item.scheduled_for|format_datetime_relative + }} + + {% else %} + + Sent {{ + (item.scheduled_for or item.created_at)|format_datetime_relative + }} + + {% endif %} +
{% endcall %} {% call field() %} - {{ big_number( - item.notifications_sending, - smallest=True - ) }} - {% endcall %} - {% call field() %} - {{ big_number(item.notifications_delivered, smallest=True) }} - {% endcall %} - {% call field(status='error' if item.high_failure_rate else '') %} - {{ big_number(item.notifications_failed, smallest=True) }} + {% if item.scheduled %} + {{ big_number( + item.notification_count, + smallest=True, + label=message_count_label( + item.notification_count, + item.template_type, + suffix='waiting to send' + ) + ) }} + {% elif item.template_type == 'letter' %} + {{ big_number( + item.notification_count, + smallest=True, + label=message_count_label( + item.notification_count, + item.template_type, + suffix='' + ) + ) }} + {% elif item.pdf_letter %} +

+ {% for line in item.recipient.split(',') %} + {% if loop.index < 3 %} + {{ line }}
+ {% endif %} + {% endfor %} +

+ {% 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 %}
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 9e47e946e..b9fb1b01c 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -937,8 +937,11 @@ def test_should_show_recent_jobs_on_dashboard( )): assert filename in table_rows[index].find_all('th')[0].text assert 'Sent today at 11:09' in table_rows[index].find_all('th')[0].text - for column_index, count in enumerate((1, 0, 0)): - assert table_rows[index].find_all('td')[column_index].text.strip() == str(count) + assert normalize_spaces( + table_rows[index].select_one('td').text + ) == ( + '1 sending 0 delivered 0 failed' + ) @pytest.mark.parametrize('extra_permissions', ( diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 23e012b1f..9dfbd5b9a 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -21,23 +21,23 @@ from tests.conftest import ( @pytest.mark.parametrize('user, expected_rows', [ (create_active_user_with_permissions(), ( ( - 'File Sending Delivered Failed' + 'File Status' ), ( 'export 1/1/2016.xls ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'all email addresses.xlsx ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'applicants.ods ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'thisisatest.csv ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), )), (create_active_caseworking_user(), ( @@ -53,23 +53,23 @@ from tests.conftest import ( 'Sending 1 January 2016 at 11:09pm 1' ), ( - 'File Sending Delivered Failed' + 'File Status' ), ( 'export 1/1/2016.xls ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'all email addresses.xlsx ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'applicants.ods ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'thisisatest.csv ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), )), ]) @@ -123,23 +123,23 @@ def test_jobs_page_doesnt_show_scheduled_on_page_2( for index, row in enumerate(( ( - 'File Sending Delivered Failed' + 'File Status' ), ( 'export 1/1/2016.xls ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'all email addresses.xlsx ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'applicants.ods ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), ( 'thisisatest.csv ' - 'Sent today at 12:12pm 1 0 0' + 'Sent today at 12:12pm 1 sending 0 delivered 0 failed' ), )): assert normalize_spaces(page.select('tr')[index].text) == row diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index b36791533..229f68268 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -77,11 +77,11 @@ def test_get_upload_hub_page( ) assert page.find_all( - 'a', {'class': 'file-list-filename'} + 'a', {'class': 'file-list-filename-large'} )[0].attrs['href'] == '/services/{}/jobs/job_id_1'.format(SERVICE_ONE_ID) assert page.find_all( - 'a', {'class': 'file-list-filename'} + 'a', {'class': 'file-list-filename-large'} )[1].attrs['href'] == '/services/{}/notification/letter_id_1'.format(SERVICE_ONE_ID) @@ -643,15 +643,17 @@ def test_uploads_page_shows_scheduled_jobs( normalize_spaces(row.text) for row in page.select('tr') ] == [ ( - 'File Sending Delivered Failed' + 'File Status' ), ( 'send_me_later.csv ' - 'Sent 1 January 2016 at 11:09am 0 0 0' + 'Sending 1 January 2016 at 11:09am ' + '1 text message waiting to send' ), ( 'even_later.csv ' - 'Sent 1 January 2016 at 11:09pm 0 0 0' + 'Sending 1 January 2016 at 11:09pm ' + '1 text message waiting to send' ), ] assert not page.select('.table-empty-message') diff --git a/tests/conftest.py b/tests/conftest.py index 046d3f7ad..f01c9150c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1781,13 +1781,22 @@ def mock_get_uploads(mocker, api_user_active): 'notification_count': 10, 'created_at': '2016-01-01 11:09:00.061258', 'statistics': [{'count': 8, 'status': 'delivered'}, {'count': 2, 'status': 'temporary-failure'}], - 'upload_type': 'job'}, + 'upload_type': 'job', + 'template_type': 'sms', + 'recipient': None}, {'id': 'letter_id_1', 'original_file_name': 'some.pdf', 'notification_count': 1, 'created_at': '2016-01-01 11:09:00.061258', 'statistics': [{'count': 1, 'status': 'delivered'}], - 'upload_type': 'letter'} + 'upload_type': 'letter', + 'template_type': None, + 'recipient': ( + 'Firstname Lastname\n' + '123 Example Street\n' + 'City of Town\n' + 'XM4 5QQ' + )} ] return { 'data': uploads, From e638b68af1d02d0ac211d68f489672ac9424ee73 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 14:20:31 +0000 Subject: [PATCH 06/10] Add some more content to the empty page state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We think that we need to make it clear what the difference between uploading a letter and uploading a spreadsheet is, and where you go to do each. We get some confusion about uploading being behind the ‘Send’ button on the template page. There’s some concern that launching the upload page will increase this confusion, unless we head it off with some messaging. --- app/templates/views/jobs/jobs.html | 16 +++++++++++----- tests/app/main/views/test_uploads.py | 13 ++++++++----- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/app/templates/views/jobs/jobs.html b/app/templates/views/jobs/jobs.html index 33c49f0ff..0e026dd6e 100644 --- a/app/templates/views/jobs/jobs.html +++ b/app/templates/views/jobs/jobs.html @@ -18,11 +18,17 @@ {% include 'views/dashboard/_jobs.html' %} {% endif %} {% if not jobs and not show_scheduled_jobs %} - {# - `_jobs.html` will show the ‘You have no jobs’ message when - passed an empty list of jobs - #} - {% include 'views/dashboard/_jobs.html' %} +

+ You have not uploaded any files yet. +

+ {% if current_service.has_permission('upload_letters') %} +

+ Upload a letter and Notify will print, pack and post it for you. +

+ {% endif %} +

+ To upload a list of contact details, first choose a template. +

{% endif %} {{ previous_next_navigation(prev_page, next_page) }} {% if current_service.can_upload_letters and current_user.has_permissions('send_messages') %} diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 229f68268..561482436 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -39,10 +39,13 @@ def test_no_upload_letters_button_without_permission( @pytest.mark.parametrize('extra_permissions, expected_empty_message', ( (['letter'], ( - 'You have not uploaded any files yet' + 'You have not uploaded any files yet. ' + 'To upload a list of contact details, first choose a template.' )), (['letter', 'upload_letters'], ( - 'Upload a letter and Notify will print, pack and post it for you.' + 'You have not uploaded any files yet. ' + 'Upload a letter and Notify will print, pack and post it for you. ' + 'To upload a list of contact details, first choose a template.' )), )) def test_get_upload_hub_with_no_uploads( @@ -56,9 +59,9 @@ def test_get_upload_hub_with_no_uploads( mocker.patch('app.job_api_client.get_jobs', return_value={'data': []}) service_one['permissions'] += extra_permissions page = client_request.get('main.uploads', service_id=SERVICE_ONE_ID) - assert normalize_spaces( - page.select_one('.table-empty-message').text - ) == expected_empty_message + assert normalize_spaces(' '.join( + paragraph.text for paragraph in page.select('main p') + )) == expected_empty_message assert not page.select('.file-list-filename') From 63f704c64f92e151bbaa114da18f83c04643b2a4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 17:51:01 +0000 Subject: [PATCH 07/10] Fix accidental switch of previous and next We were showing the previous page link when we meant to show the next page link because the variables were swapped round in the code. --- app/main/views/uploads.py | 4 ++-- tests/app/main/views/test_uploads.py | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 961b1ae80..a6b32f28d 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -46,10 +46,10 @@ def uploads(service_id): uploads = current_service.get_page_of_uploads(page=request.args.get('page')) prev_page = None - if uploads.next_page: + if uploads.prev_page: prev_page = generate_previous_dict('main.uploads', service_id, uploads.current_page) next_page = None - if uploads.prev_page: + if uploads.next_page: next_page = generate_next_dict('main.uploads', service_id, uploads.current_page) if uploads.current_page == 1: diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 561482436..c9cf333ed 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -660,3 +660,21 @@ def test_uploads_page_shows_scheduled_jobs( ), ] assert not page.select('.table-empty-message') + + +def test_get_uploads_shows_pagination( + client_request, + active_user_with_permissions, + mock_get_jobs, + mock_get_uploads, +): + page = client_request.get('main.uploads', service_id=SERVICE_ONE_ID) + + assert normalize_spaces(page.select_one('.next-page').text) == ( + 'Next page ' + 'page 2' + ) + assert normalize_spaces(page.select_one('.previous-page').text) == ( + 'Previous page ' + 'page 0' + ) From 57c5c298d4e5f8c8ed86c74fd31cbe47f8cb0aa5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 2 Mar 2020 17:35:13 +0000 Subject: [PATCH 08/10] Add govuk-body class to paragraphs per https://design-system.service.gov.uk/styles/typography/#paragraphs --- app/templates/views/jobs/jobs.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/templates/views/jobs/jobs.html b/app/templates/views/jobs/jobs.html index 0e026dd6e..0c14dce42 100644 --- a/app/templates/views/jobs/jobs.html +++ b/app/templates/views/jobs/jobs.html @@ -18,15 +18,15 @@ {% include 'views/dashboard/_jobs.html' %} {% endif %} {% if not jobs and not show_scheduled_jobs %} -

+

You have not uploaded any files yet.

{% if current_service.has_permission('upload_letters') %} -

+

Upload a letter and Notify will print, pack and post it for you.

{% endif %} -

+

To upload a list of contact details, first choose a template.

{% endif %} From b1a97b0d69a640742443f6e0cef97c4a747ea4cd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 3 Mar 2020 10:11:42 +0000 Subject: [PATCH 09/10] Implement __radd__ on ModelList to make addition commutative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because ModelList implements `__add__` we can do the following: ```python ImmediateJobs() + ScheduledJobs() ImmediateJobs() + [] ``` Both of these call the `__add__` method of `ImmediateJobs`. What we can’t do is this: ```python [] + ScheduledJobs() ``` That tries to call the `__add__` method of list, which doesn’t know what to do with an instance of `ModelList`. The Pythonic way to deal with this is to implement `__radd__` (right add) which is invoked when our instance is on the right hand side of the addition operator. --- app/models/__init__.py | 9 ++------- app/models/service.py | 6 +++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/models/__init__.py b/app/models/__init__.py index 6034f4fb3..3a01e8a81 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -75,13 +75,8 @@ class ModelList(ABC, Sequence): def __add__(self, other): return list(self) + list(other) - -class EmptyModelList(ModelList): - - client_method = model = None - - def __init__(self, *args): - self.items = [] + def __radd__(self, other): + return list(other) + list(self) class InviteTokenError(Exception): diff --git a/app/models/service.py b/app/models/service.py index 0ee6217b0..132ac157c 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -5,7 +5,7 @@ from flask import abort, current_app from notifications_utils.timezones import local_timezone from werkzeug.utils import cached_property -from app.models import EmptyModelList, JSONModel +from app.models import JSONModel from app.models.job import ( ImmediateJobs, PaginatedJobs, @@ -123,13 +123,13 @@ class Service(JSONModel): @cached_property def immediate_jobs(self): if not self.has_jobs: - return EmptyModelList() + return [] return ImmediateJobs(self.id) @cached_property def scheduled_jobs(self): if not self.has_jobs: - return EmptyModelList() + return [] return ScheduledJobs(self.id) @cached_property From 7a780d115ed28b57c2c213da3d3aed4ebd21f09b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 3 Mar 2020 10:50:07 +0000 Subject: [PATCH 10/10] Test that addresses display on uploads page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We didn’t have a test that checked for the first two lines of the address being displayed when rendering one-off letters on the uploads page. I double checked in the database and we store addresses in the `to` field with newlines, not commas. --- app/templates/views/dashboard/_jobs.html | 2 +- tests/app/main/views/test_uploads.py | 31 +++++++++++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/app/templates/views/dashboard/_jobs.html b/app/templates/views/dashboard/_jobs.html index dfbaedb54..cc9f84ac6 100644 --- a/app/templates/views/dashboard/_jobs.html +++ b/app/templates/views/dashboard/_jobs.html @@ -62,7 +62,7 @@ ) }} {% elif item.pdf_letter %}

- {% for line in item.recipient.split(',') %} + {% for line in item.recipient.splitlines() %} {% if loop.index < 3 %} {{ line }}
{% endif %} diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index c9cf333ed..943e5b205 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -79,13 +79,32 @@ def test_get_upload_hub_page( 'main.upload_letter', service_id=SERVICE_ONE_ID ) - assert page.find_all( - 'a', {'class': 'file-list-filename-large'} - )[0].attrs['href'] == '/services/{}/jobs/job_id_1'.format(SERVICE_ONE_ID) + uploads = page.select('tbody tr') - assert page.find_all( - 'a', {'class': 'file-list-filename-large'} - )[1].attrs['href'] == '/services/{}/notification/letter_id_1'.format(SERVICE_ONE_ID) + assert normalize_spaces(uploads[0].text.strip()) == ( + 'some.csv ' + 'Sent 1 January 2016 at 11:09am ' + '0 sending 8 delivered 2 failed' + ) + assert uploads[0].select_one('a.file-list-filename-large')['href'] == ( + '/services/{}/jobs/job_id_1'.format(SERVICE_ONE_ID) + ) + + assert normalize_spaces(uploads[1].text.strip()) == ( + 'some.pdf ' + 'Sent 1 January 2016 at 11:09am ' + 'Firstname Lastname ' + '123 Example Street' + ) + assert normalize_spaces(str(uploads[1].select_one('.govuk-body'))) == ( + '

' + 'Firstname Lastname
' + '123 Example Street
' + '

' + ) + assert uploads[1].select_one('a.file-list-filename-large')['href'] == ( + '/services/{}/notification/letter_id_1'.format(SERVICE_ONE_ID) + ) def test_get_upload_letter(client_request):