From fae18f300883d42804626f191edaf3bb4e7482bd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Dec 2019 10:16:35 +0000 Subject: [PATCH 1/9] Put CSV extension on filename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This follows our pattern for other downloadable reports, and gives people who know/care about stuff like file types some indication of what they’re about to download. --- app/main/views/returned_letters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/returned_letters.py b/app/main/views/returned_letters.py index 6d6b24d12..09f3db4ab 100644 --- a/app/main/views/returned_letters.py +++ b/app/main/views/returned_letters.py @@ -17,7 +17,7 @@ def returned_letter_summary(service_id): ) -@main.route("/services//returned-letters-csv/", methods=["GET"]) +@main.route("/services//returned-letters/.csv", methods=["GET"]) @user_has_permissions('view_activity') def returned_letters_report(service_id, reported_at): returned_letters = service_api_client.get_returned_letters(service_id, reported_at) From 8bcfb2fcde9e7f6b2fee9d40d54003d185c74ac4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Dec 2019 10:20:09 +0000 Subject: [PATCH 2/9] Use file list pattern for list of files This means the page has the same appearance as other lists of stuff like the notifications page. --- app/templates/views/returned-letter-summary.html | 9 ++++++--- tests/app/main/views/test_returned_letters.py | 16 ++++++++++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/app/templates/views/returned-letter-summary.html b/app/templates/views/returned-letter-summary.html index 9c50d1c40..5d4f45d91 100644 --- a/app/templates/views/returned-letter-summary.html +++ b/app/templates/views/returned-letter-summary.html @@ -18,10 +18,13 @@ empty_message='If you have returned letter reports they will be listed here' ) %} {% call field() %} - Returned letters reported on {{ item.reported_at | format_date}} - {{ item.returned_letter_count}} {{ message_count_label(item.returned_letter_count, 'letter', suffix='')}} + {{ item.reported_at | format_date_normal }} +

+ {{ item.returned_letter_count}} {{ message_count_label(item.returned_letter_count, 'letter', suffix='')}} +

{% endcall %} {% endcall %} -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/tests/app/main/views/test_returned_letters.py b/tests/app/main/views/test_returned_letters.py index eb373b07d..1b205f1be 100644 --- a/tests/app/main/views/test_returned_letters.py +++ b/tests/app/main/views/test_returned_letters.py @@ -15,9 +15,13 @@ def test_returned_letter_summary( mock.assert_called_once_with(SERVICE_ONE_ID) - expected_text = "Returned letters reported on Tuesday 24 December 2019 - 30 letters" assert page.h1.string.strip() == 'Returned letters' - assert normalize_spaces(page.select('.table-field-left-aligned')[0].text) == expected_text + assert normalize_spaces( + page.select_one('.table-field-left-aligned').text + ) == ( + '24 December 2019 ' + '30 letters' + ) assert page.select_one('.table-field-left-aligned a')['href'] == url_for('.returned_letters_report', service_id=SERVICE_ONE_ID, reported_at='2019-12-24') @@ -35,9 +39,13 @@ def test_returned_letter_summary_with_one_letter( mock.assert_called_once_with(SERVICE_ONE_ID) - expected_text = "Returned letters reported on Tuesday 24 December 2019 - 1 letter" assert page.h1.string.strip() == 'Returned letters' - assert normalize_spaces(page.select('.table-field-left-aligned')[0].text) == expected_text + assert normalize_spaces( + page.select_one('.table-field-left-aligned').text + ) == ( + '24 December 2019 ' + '1 letter' + ) def test_returned_letters_reports( From 57ca75f5bca34666ab0f6164482056e98d67c332 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Dec 2019 10:24:01 +0000 Subject: [PATCH 3/9] Add download attribute to link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This forces the file to download, rather than open in the user’s browser. We do this in other places where we let people download CSV and PDF files. --- app/templates/views/returned-letter-summary.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/returned-letter-summary.html b/app/templates/views/returned-letter-summary.html index 5d4f45d91..573069513 100644 --- a/app/templates/views/returned-letter-summary.html +++ b/app/templates/views/returned-letter-summary.html @@ -18,7 +18,7 @@ empty_message='If you have returned letter reports they will be listed here' ) %} {% call field() %} - {{ item.reported_at | format_date_normal }}

{{ item.returned_letter_count}} {{ message_count_label(item.returned_letter_count, 'letter', suffix='')}} From 9c3315f194fc5ec3f165f4ca6eca7bd05425d133 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Dec 2019 10:24:39 +0000 Subject: [PATCH 4/9] Add field headings to table Headings are good for accessibility. They also are what adds the grey border to the top of the first row in the table. --- app/templates/views/returned-letter-summary.html | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/templates/views/returned-letter-summary.html b/app/templates/views/returned-letter-summary.html index 573069513..38106526f 100644 --- a/app/templates/views/returned-letter-summary.html +++ b/app/templates/views/returned-letter-summary.html @@ -15,7 +15,9 @@ data, caption="Returned letters report", caption_visible=False, - empty_message='If you have returned letter reports they will be listed here' + empty_message='If you have returned letter reports they will be listed here', + field_headings=['Report'], + field_headings_visible=False ) %} {% call field() %} Date: Tue, 31 Dec 2019 13:27:27 +0000 Subject: [PATCH 5/9] Add a page for each report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s useful to get some kind of preview of the report before you download it. And if there’s only a few letters in there then you might not even need to download it at all. For teams with lots of letters we don’t want the page to load too slowly so let’s cap the number of displayed items to 50, same as previewing a spreadsheet. --- app/assets/stylesheets/views/dashboard.scss | 4 + app/main/views/returned_letters.py | 18 ++++ app/navigation.py | 4 + .../views/returned-letter-summary.html | 4 +- app/templates/views/returned-letters.html | 57 +++++++++++ tests/app/main/views/test_returned_letters.py | 98 ++++++++++++++++++- 6 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 app/templates/views/returned-letters.html diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index 39d8e82ee..76a5e834c 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -62,6 +62,10 @@ margin-top: -10px; } + &-filename-unlinked { + @include core-19; + } + &-hint { @include core-16; display: block; diff --git a/app/main/views/returned_letters.py b/app/main/views/returned_letters.py index 09f3db4ab..a381b1312 100644 --- a/app/main/views/returned_letters.py +++ b/app/main/views/returned_letters.py @@ -17,6 +17,24 @@ def returned_letter_summary(service_id): ) +@main.route("/services//returned-letters/", methods=["GET"]) +@user_has_permissions('view_activity') +def returned_letters(service_id, reported_at): + + page_size = 50 + returned_letters = service_api_client.get_returned_letters(service_id, reported_at) + count_of_returned_letters = len(returned_letters) + + return render_template( + 'views/returned-letters.html', + returned_letters=returned_letters[:page_size], + reported_at=reported_at, + more_than_one_page=(count_of_returned_letters > page_size), + page_size=page_size, + count_of_returned_letters=count_of_returned_letters, + ) + + @main.route("/services//returned-letters/.csv", methods=["GET"]) @user_has_permissions('view_activity') def returned_letters_report(service_id, reported_at): diff --git a/app/navigation.py b/app/navigation.py index fe2f2092b..354483c7d 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -243,6 +243,7 @@ class HeaderNavigation(Navigation): 'resend_email_verification', 'resume_service', 'returned_letter_summary', + 'returned_letters', 'returned_letters_report', 'revoke_api_key', 'robots', @@ -567,6 +568,7 @@ class MainNavigation(Navigation): 'resend_email_verification', 'resume_service', 'returned_letter_summary', + 'returned_letters', 'returned_letters_report', 'roadmap', 'robots', @@ -806,6 +808,7 @@ class CaseworkNavigation(Navigation): 'resend_email_verification', 'resume_service', 'returned_letter_summary', + 'returned_letters', 'returned_letters_report', 'revoke_api_key', 'roadmap', @@ -1084,6 +1087,7 @@ class OrgNavigation(Navigation): 'resend_email_verification', 'resume_service', 'returned_letter_summary', + 'returned_letters', 'returned_letters_report', 'revoke_api_key', 'roadmap', diff --git a/app/templates/views/returned-letter-summary.html b/app/templates/views/returned-letter-summary.html index 38106526f..4071b30c3 100644 --- a/app/templates/views/returned-letter-summary.html +++ b/app/templates/views/returned-letter-summary.html @@ -20,8 +20,8 @@ field_headings_visible=False ) %} {% call field() %} - {{ item.reported_at | format_date_normal }} + {{ item.reported_at | format_date_normal }}

{{ item.returned_letter_count}} {{ message_count_label(item.returned_letter_count, 'letter', suffix='')}}

diff --git a/app/templates/views/returned-letters.html b/app/templates/views/returned-letters.html new file mode 100644 index 000000000..98e71e94c --- /dev/null +++ b/app/templates/views/returned-letters.html @@ -0,0 +1,57 @@ +{% from "components/table.html" import list_table, field %} +{% from "components/message-count-label.html" import message_count_label %} +{% from "components/page-header.html" import page_header %} +{% extends "withnav_template.html" %} + +{% block service_page_title %} + Returned letters for {{ reported_at|format_date_normal }} +{% endblock %} + +{% block maincolumn_content %} + +{{ page_header( + 'Returned letters for {}'.format(reported_at|format_date_normal), + back_link=url_for('main.returned_letter_summary', service_id=current_service.id) +) }} + +

+ Download this report +

+ +
+ {% call(item, row_number) list_table( + returned_letters, + caption="Returned letters for {}".format(today), + caption_visible=False, + empty_message='If you have returned letter reports they will be listed here', + field_headings=['Template name', 'Originally sent'], + field_headings_visible=False + ) %} + {% call field() %} + {{ item.template_name or item.uploaded_letter_file_name }} + + {% if item.client_reference %} + Reference {{ item.client_reference }} + {% elif item.original_file_name %} + Sent from {{ item.original_file_name }} + {% else %} + No reference provided + {% endif %} + + {% endcall %} + {% call field(align='right') %} + + + Originally sent {{ item.created_at|format_date_normal }} + + + {% endcall %} + {% endcall %} + {% if more_than_one_page %} +

+ Only showing the first {{ page_size }} of {{ count_of_returned_letters }} rows +

+ {% endif %} +
+ +{% endblock %} diff --git a/tests/app/main/views/test_returned_letters.py b/tests/app/main/views/test_returned_letters.py index 1b205f1be..d1039a833 100644 --- a/tests/app/main/views/test_returned_letters.py +++ b/tests/app/main/views/test_returned_letters.py @@ -1,3 +1,5 @@ +import uuid + from flask import url_for from tests.conftest import SERVICE_ONE_ID, normalize_spaces @@ -22,9 +24,11 @@ def test_returned_letter_summary( '24 December 2019 ' '30 letters' ) - assert page.select_one('.table-field-left-aligned a')['href'] == url_for('.returned_letters_report', - service_id=SERVICE_ONE_ID, - reported_at='2019-12-24') + assert page.select_one('.table-field-left-aligned a')['href'] == url_for( + '.returned_letters', + service_id=SERVICE_ONE_ID, + reported_at='2019-12-24', + ) def test_returned_letter_summary_with_one_letter( @@ -48,6 +52,94 @@ def test_returned_letter_summary_with_one_letter( ) +def test_returned_letters_page( + client_request, + mocker +): + data = [ + { + 'notification_id': uuid.uuid4(), + 'client_reference': client_reference, + 'created_at': '2019-12-24 13:30', + 'email_address': 'test@gov.uk', + 'template_name': template_name, + 'template_id': uuid.uuid4(), + 'template_version': None, + 'original_file_name': original_file_name, + 'job_row_number': None, + 'uploaded_letter_file_name': 'test_letter.pdf', + } + for client_reference, template_name, original_file_name, uploaded_letter_file_name in ( + ('ABC123', 'Example template', None, None), + (None, 'Example template', 'Example spreadsheet.xlsx', None), + (None, 'Example template', None, None), + ('DEF456', None, None, 'Example precompiled.pdf'), + (None, None, None, 'Example one-off.pdf'), + ) + ] + mocker.patch('app.service_api_client.get_returned_letters', return_value=data) + + page = client_request.get( + 'main.returned_letters', + service_id=SERVICE_ONE_ID, + reported_at='2019-12-24', + ) + + assert [ + 'Template name Originally sent', + 'Example template Reference ABC123 Originally sent 24 December 2019', + 'Example template Sent from Example spreadsheet.xlsx Originally sent 24 December 2019', + 'Example template No reference provided Originally sent 24 December 2019', + 'test_letter.pdf Reference DEF456 Originally sent 24 December 2019', + 'test_letter.pdf No reference provided Originally sent 24 December 2019', + ] == [ + normalize_spaces(row.text) for row in page.select('tr') + ] + + +def test_returned_letters_page_with_many_letters( + client_request, + mocker +): + data = [ + { + 'notification_id': uuid.uuid4(), + 'client_reference': None, + 'created_at': '2019-12-24 13:30', + 'email_address': 'test@gov.uk', + 'template_name': 'Example template', + 'template_id': uuid.uuid4(), + 'template_version': None, + 'original_file_name': None, + 'job_row_number': None, + 'uploaded_letter_file_name': None, + } + ] * 51 + mocker.patch('app.service_api_client.get_returned_letters', return_value=data) + + page = client_request.get( + 'main.returned_letters', + service_id=SERVICE_ONE_ID, + reported_at='2019-12-24', + ) + + assert len(data) == 51 + assert len(page.select('tbody tr')) == 50 + assert normalize_spaces( + page.select_one('.table-show-more-link').text + ) == ( + 'Only showing the first 50 of 51 rows' + ) + assert page.select_one('a[download]').text == ( + 'Download this report' + ) + assert page.select_one('a[download]')['href'] == url_for( + '.returned_letters_report', + service_id=SERVICE_ONE_ID, + reported_at='2019-12-24', + ) + + def test_returned_letters_reports( client_request, mocker From 227fca6263d884d4a3ea150dd4e9c1733f8c8cda Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Dec 2019 13:36:18 +0000 Subject: [PATCH 6/9] =?UTF-8?q?Make=20appearance=20of=20=E2=80=98empty?= =?UTF-8?q?=E2=80=99=20table=20rows=20even?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Our table rows take up 65px vertical space. We also have things that look like rows that say: - there are no rows - there are more rows than can be shown on screen This commit makes them appear the same height. --- app/assets/stylesheets/components/table.scss | 9 +++++---- app/templates/views/returned-letter-summary.html | 8 ++++---- tests/app/main/views/test_returned_letters.py | 6 +++--- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 7be4fb25f..d8572e776 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -324,11 +324,12 @@ border-bottom: 1px solid $border-colour; } -.table-empty-message { - @include core-16; +.table-empty-message, +td.table-empty-message { + @include core-19; color: $secondary-text-colour; border-bottom: 1px solid $border-colour; - padding: 0.75em 0 0.5625em 0; + padding: 20px 0 20px 0; } .table-show-more-link { @@ -337,7 +338,7 @@ color: $secondary-text-colour; margin-bottom: $gutter * 1.3333; border-bottom: 1px solid $border-colour; - padding: 10px 0 10px 0; + padding: 35px 0 10px 0; text-align: center; .table + & { diff --git a/app/templates/views/returned-letter-summary.html b/app/templates/views/returned-letter-summary.html index 4071b30c3..5be1fc0b3 100644 --- a/app/templates/views/returned-letter-summary.html +++ b/app/templates/views/returned-letter-summary.html @@ -1,4 +1,4 @@ -{% from "components/table.html" import list_table, field %} +{% from "components/table.html" import list_table, row_heading %} {% from "components/message-count-label.html" import message_count_label %} {% extends "withnav_template.html" %} @@ -10,7 +10,7 @@

Returned letters

-
+
{% call(item, row_number) list_table( data, caption="Returned letters report", @@ -18,8 +18,8 @@ empty_message='If you have returned letter reports they will be listed here', field_headings=['Report'], field_headings_visible=False - ) %} - {% call field() %} + ) %} + {% call row_heading() %} {{ item.reported_at | format_date_normal }}

diff --git a/tests/app/main/views/test_returned_letters.py b/tests/app/main/views/test_returned_letters.py index d1039a833..19779ae3c 100644 --- a/tests/app/main/views/test_returned_letters.py +++ b/tests/app/main/views/test_returned_letters.py @@ -19,12 +19,12 @@ def test_returned_letter_summary( assert page.h1.string.strip() == 'Returned letters' assert normalize_spaces( - page.select_one('.table-field-left-aligned').text + page.select_one('.table-field').text ) == ( '24 December 2019 ' '30 letters' ) - assert page.select_one('.table-field-left-aligned a')['href'] == url_for( + assert page.select_one('.table-field a')['href'] == url_for( '.returned_letters', service_id=SERVICE_ONE_ID, reported_at='2019-12-24', @@ -45,7 +45,7 @@ def test_returned_letter_summary_with_one_letter( assert page.h1.string.strip() == 'Returned letters' assert normalize_spaces( - page.select_one('.table-field-left-aligned').text + page.select_one('.table-field').text ) == ( '24 December 2019 ' '1 letter' From 7059e475c161a036f1f0bae3318b23b8aac5f50a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Dec 2019 14:44:15 +0000 Subject: [PATCH 7/9] Make URLs consistent/hackable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do `/things` and `/things/` elsewhere; let’s be consistent here. Means you don’t have to remember the word ‘summary’. --- app/main/views/returned_letters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/returned_letters.py b/app/main/views/returned_letters.py index a381b1312..a977257ff 100644 --- a/app/main/views/returned_letters.py +++ b/app/main/views/returned_letters.py @@ -7,7 +7,7 @@ from app.main import main from app.utils import Spreadsheet, user_has_permissions -@main.route("/services//returned-letter-summary", methods=["GET"]) +@main.route("/services//returned-letters", methods=["GET"]) @user_has_permissions('view_activity') def returned_letter_summary(service_id): summary = service_api_client.get_returned_letter_summary(service_id) From 317aa53b6a95f088dbe2bb9f690f123ab82d3368 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Dec 2019 14:49:51 +0000 Subject: [PATCH 8/9] =?UTF-8?q?Don=E2=80=99t=20specify=20that=20routes=20a?= =?UTF-8?q?re=20GET=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > By default a route only answers to `GET` requests https://flask.palletsprojects.com/en/1.1.x/quickstart/#http-methods --- app/main/views/returned_letters.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/returned_letters.py b/app/main/views/returned_letters.py index a977257ff..cdc06e464 100644 --- a/app/main/views/returned_letters.py +++ b/app/main/views/returned_letters.py @@ -7,7 +7,7 @@ from app.main import main from app.utils import Spreadsheet, user_has_permissions -@main.route("/services//returned-letters", methods=["GET"]) +@main.route("/services//returned-letters") @user_has_permissions('view_activity') def returned_letter_summary(service_id): summary = service_api_client.get_returned_letter_summary(service_id) @@ -17,7 +17,7 @@ def returned_letter_summary(service_id): ) -@main.route("/services//returned-letters/", methods=["GET"]) +@main.route("/services//returned-letters/") @user_has_permissions('view_activity') def returned_letters(service_id, reported_at): @@ -35,7 +35,7 @@ def returned_letters(service_id, reported_at): ) -@main.route("/services//returned-letters/.csv", methods=["GET"]) +@main.route("/services//returned-letters/.csv") @user_has_permissions('view_activity') def returned_letters_report(service_id, reported_at): returned_letters = service_api_client.get_returned_letters(service_id, reported_at) From 07f2ffca25cc3d71a3b0497754d71629918573c7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 2 Jan 2020 10:08:18 +0000 Subject: [PATCH 9/9] Add govuk-link class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So it’s one less link we have to do when moving to Design System. Co-Authored-By: Tom Byers --- app/templates/views/returned-letters.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/returned-letters.html b/app/templates/views/returned-letters.html index 98e71e94c..7f29035ee 100644 --- a/app/templates/views/returned-letters.html +++ b/app/templates/views/returned-letters.html @@ -15,7 +15,7 @@ ) }}

- Download this report + Download this report