From fdf74572b9408a1dfad7abe0b344adc308720c60 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 9 Oct 2019 11:23:02 +0100 Subject: [PATCH 1/9] Add radio button and convert from to WTForm --- app/main/forms.py | 13 +++++++++++++ app/main/views/uploads.py | 5 ++++- app/templates/views/uploads/preview.html | 6 ++++-- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 65877e997..ecd8af8a8 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -765,6 +765,19 @@ class LetterTemplatePostageForm(StripWhitespaceForm): ) +class LetterUploadPostageForm(StripWhitespaceForm): + postage = RadioField( + 'Choose the postage for this letter', + choices=[ + ('first', 'First class post'), + ('second', 'Second class post'), + ], + default='second', + validators=[DataRequired()] + ) + file_id = HiddenField() + + class ForgotPasswordForm(StripWhitespaceForm): email_address = email_address(gov_user=False) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 665fce5a5..0fd7dbe84 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -18,7 +18,7 @@ from requests import RequestException from app import current_service, notification_api_client, service_api_client from app.extensions import antivirus_client from app.main import main -from app.main.forms import PDFUploadForm +from app.main.forms import LetterUploadPostageForm, PDFUploadForm from app.main.views.jobs import view_jobs from app.s3_client.s3_letter_upload_client import ( get_letter_metadata, @@ -154,6 +154,8 @@ def uploaded_letter_preview(service_id, file_id): error = get_letter_validation_error(error_message, invalid_pages, page_count) template_dict = service_api_client.get_precompiled_template(service_id) + form = LetterUploadPostageForm() + template = get_template( template_dict, service_id, @@ -172,6 +174,7 @@ def uploaded_letter_preview(service_id, file_id): status=status, file_id=file_id, error=error, + form=form, ) diff --git a/app/templates/views/uploads/preview.html b/app/templates/views/uploads/preview.html index c448b3b4d..fc74ca0e2 100644 --- a/app/templates/views/uploads/preview.html +++ b/app/templates/views/uploads/preview.html @@ -1,6 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/banner.html" import banner_wrapper %} {% from "components/page-header.html" import page_header %} +{% from "components/radios.html" import radios %} {% block service_page_title %} {{ original_filename }} @@ -39,8 +40,9 @@ 'main.send_uploaded_letter', service_id=current_service.id, )}}" class='page-footer'> - - + {{ form.csrf_token }} + {{ radios(form.postage, hide_legend=true) }} + {{ form.file_id(value=file_id) }} From 9ba1dbfffa3638d6b6de6778e9e17d168b786115 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 9 Oct 2019 13:47:28 +0100 Subject: [PATCH 2/9] Do not show postage stamp for letter preview --- app/main/forms.py | 4 +++- app/main/views/uploads.py | 4 ++++ tests/app/main/views/test_uploads.py | 8 ++------ 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index ecd8af8a8..775de728c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -775,7 +775,9 @@ class LetterUploadPostageForm(StripWhitespaceForm): default='second', validators=[DataRequired()] ) - file_id = HiddenField() + file_id = HiddenField( + validators=[DataRequired()] + ) class ForgotPasswordForm(StripWhitespaceForm): diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 0fd7dbe84..f39ccf402 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -153,6 +153,10 @@ def uploaded_letter_preview(service_id, file_id): error = get_letter_validation_error(error_message, invalid_pages, page_count) template_dict = service_api_client.get_precompiled_template(service_id) + # Override pre compiled letter template postage to none as it has not yet been picked even though + # the pre compiled letter template has its postage set as second class as the DB currently requires + # a non null value of postage for letter templates + template_dict['postage'] = None form = LetterUploadPostageForm() diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index a45daaafc..a9f89bd3d 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -125,9 +125,7 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file( ) assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf' - assert len(page.select('.letter-postage')) == 1 - assert normalize_spaces(page.select_one('.letter-postage').text) == ('Postage: second class') - assert page.select_one('.letter-postage')['class'] == ['letter-postage', 'letter-postage-second'] + assert len(page.select('.letter-postage')) == 0 letter_images = page.select('main img') assert len(letter_images) == 3 @@ -283,9 +281,7 @@ def test_post_upload_letter_shows_letter_preview_for_invalid_file(mocker, client _follow_redirects=True, ) - assert len(page.select('.letter-postage')) == 1 - assert normalize_spaces(page.select_one('.letter-postage').text) == ('Postage: first class') - assert page.select_one('.letter-postage')['class'] == ['letter-postage', 'letter-postage-first'] + assert len(page.select('.letter-postage')) == 0 letter_images = page.select('main img') assert len(letter_images) == 1 From 2b8b4c25aab7a4caa7123aa93e9d66e396fc8ff1 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 9 Oct 2019 14:50:06 +0100 Subject: [PATCH 3/9] Send postage value to API when sending pre compiled letter --- app/main/views/uploads.py | 3 ++- app/notify_client/notification_api_client.py | 4 ++-- tests/app/main/views/test_uploads.py | 6 +++--- tests/app/notify_client/test_notification_client.py | 3 ++- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index f39ccf402..3fe4e30ab 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -202,13 +202,14 @@ def send_uploaded_letter(service_id): abort(403) file_id = request.form['file_id'] + postage = request.form['postage'] metadata = get_letter_metadata(service_id, file_id) filename = metadata.get('filename') if metadata.get('status') != 'valid': abort(403) - notification_api_client.send_precompiled_letter(service_id, filename, file_id) + notification_api_client.send_precompiled_letter(service_id, filename, file_id, postage) return redirect(url_for( '.view_notification', diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index 793af02ef..0ce251f6b 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -59,11 +59,11 @@ class NotificationApiClient(NotifyAdminAPIClient): data = _attach_current_user(data) return self.post(url='/service/{}/send-notification'.format(service_id), data=data) - def send_precompiled_letter(self, service_id, filename, file_id): + def send_precompiled_letter(self, service_id, filename, file_id, postage): data = { 'filename': filename, 'file_id': file_id, - 'postage': 'second', + 'postage': postage, } data = _attach_current_user(data) return self.post(url='/service/{}/send-pdf-letter'.format(service_id), data=data) diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index a9f89bd3d..d99efa3be 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -417,7 +417,7 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mo client_request.post( 'main.send_uploaded_letter', service_id=SERVICE_ONE_ID, - _data={'filename': 'my_file.pdf', 'file_id': file_id}, + _data={'filename': 'my_file.pdf', 'file_id': file_id, 'postage': 'first'}, _expected_redirect=url_for( 'main.view_notification', service_id=SERVICE_ONE_ID, @@ -425,7 +425,7 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mo _external=True ) ) - mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', file_id) + mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', file_id, 'first') @pytest.mark.parametrize('permissions', [ @@ -448,7 +448,7 @@ def test_send_uploaded_letter_when_service_does_not_have_correct_permissions( client_request.post( 'main.send_uploaded_letter', service_id=SERVICE_ONE_ID, - _data={'filename': 'my_file.pdf', 'file_id': file_id}, + _data={'filename': 'my_file.pdf', 'file_id': file_id, 'postage': 'first'}, _expected_status=403 ) assert not mock_send.called diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py index eee76a981..5df1a97b1 100644 --- a/tests/app/notify_client/test_notification_client.py +++ b/tests/app/notify_client/test_notification_client.py @@ -64,7 +64,8 @@ def test_send_precompiled_letter(mocker, logged_in_client, active_user_with_perm NotificationApiClient().send_precompiled_letter( 'abcd-1234', 'my_file.pdf', - 'file-ID' + 'file-ID', + 'second' ) mock_post.assert_called_once_with( url='/service/abcd-1234/send-pdf-letter', From 1c3095329b92e8b77125982c86bebe94437bc5e1 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 9 Oct 2019 16:16:22 +0100 Subject: [PATCH 4/9] Change radio buttons to inline Adds ability to have inline radio buttons using the fieldset.inline functionality from gov.uk elements. Then implements this for the radio buttons for choosing postage class. Also overrides the gov uk elements styling for the inline radio buttons to place them slightly closer together as this looks better. --- app/assets/stylesheets/components/radios.scss | 6 ++++++ app/templates/components/radios.html | 4 ++-- app/templates/components/select-input.html | 8 ++++---- app/templates/views/uploads/preview.html | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/assets/stylesheets/components/radios.scss b/app/assets/stylesheets/components/radios.scss index ca6b343dc..0b755e10f 100644 --- a/app/assets/stylesheets/components/radios.scss +++ b/app/assets/stylesheets/components/radios.scss @@ -111,3 +111,9 @@ } } + +.inline { + .multiple-choice { + margin-right: 15px; + } +} diff --git a/app/templates/components/radios.html b/app/templates/components/radios.html index dd56baba0..e41e57af4 100644 --- a/app/templates/components/radios.html +++ b/app/templates/components/radios.html @@ -1,7 +1,7 @@ {% from "components/select-input.html" import select, select_list, select_nested, select_wrapper, select_input %} -{% macro radios(field, hint=None, disable=[], option_hints={}, hide_legend=False) %} - {{ select(field, hint, disable, option_hints, hide_legend, input="radio") }} +{% macro radios(field, hint=None, disable=[], option_hints={}, hide_legend=False, inline=False) %} + {{ select(field, hint, disable, option_hints, hide_legend, input="radio", inline=inline) }} {% endmacro %} diff --git a/app/templates/components/select-input.html b/app/templates/components/select-input.html index f65647647..f8387c68e 100644 --- a/app/templates/components/select-input.html +++ b/app/templates/components/select-input.html @@ -1,6 +1,6 @@ -{% macro select(field, hint=None, disable=[], option_hints={}, hide_legend=False, collapsible_opts={}, legend_style="text", input="radio") %} +{% macro select(field, hint=None, disable=[], option_hints={}, hide_legend=False, collapsible_opts={}, legend_style="text", input="radio", inline=False) %} {% call select_wrapper( - field, hint, disable, option_hints, hide_legend, collapsible_opts, legend_style + field, hint, disable, option_hints, hide_legend, collapsible_opts, legend_style, inline=inline ) %} {% for option in field %} {{ select_input(option, disable, option_hints, input=input) }} @@ -35,13 +35,13 @@ {% endmacro %} -{% macro select_wrapper(field, hint=None, disable=[], option_hints={}, hide_legend=False, collapsible_opts={}, legend_style="text") %} +{% macro select_wrapper(field, hint=None, disable=[], option_hints={}, hide_legend=False, collapsible_opts={}, legend_style="text", inline=False) %} {% set is_collapsible = collapsible_opts|length %}
{% if is_collapsible %}
{% endif %} -
+
{% if hide_legend %}
{% endif %} From 183c45d5233f8d9e4d7aff5e24d496fcc6bbf13d Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 25 Oct 2019 18:08:58 +0100 Subject: [PATCH 8/9] Assert radio buttons exist in form --- tests/app/main/views/test_uploads.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index d99efa3be..23d481f85 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -126,6 +126,9 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file( assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf' assert len(page.select('.letter-postage')) == 0 + # Check postage radios exists and second class is checked by default + assert page.find('input', id="postage-0", value="first") + assert page.find('input', id="postage-1", value="second").has_attr('checked') letter_images = page.select('main img') assert len(letter_images) == 3 From 159d2ff6b23026313188cba8d575bd9222f3348e Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 1 Nov 2019 10:56:34 +0000 Subject: [PATCH 9/9] Add `form.validate_on_submit` for sending uploaded letter Decided it was better to call this then not. This does rely on the file_id not being corrupted so the file_id passed into `uploaded_letter_preview` is valid but am taking that risk given it should only change if a user is changing the form html. --- app/main/views/uploads.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 7fe967b9e..2ed2ae114 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -202,8 +202,11 @@ def send_uploaded_letter(service_id): abort(403) form = LetterUploadPostageForm() - file_id = form.file_id.data + + if not form.validate_on_submit(): + return uploaded_letter_preview(service_id, file_id) + postage = form.postage.data metadata = get_letter_metadata(service_id, file_id) filename = metadata.get('filename')