From 68191a93ef0005b5f77b60ac43e901017deabb83 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 May 2020 11:12:33 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20show=20postage=20choice=20for?= =?UTF-8?q?=20international=20letters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit International letters don’t have a choice of postage. Under the hood they are either `europe` or `rest-of-world`. So, for letters that we detect are international, this commit: - removes the radios buttons that give users the choice of postage - passes through either `europe` or `rest-of-world` to the API, depending on what address we find in the letter This will cause the API to 500 until it can accept `europe` or `rest-of-world` as postage types, but this is probably OK because it’s only our services that have international letters switched on at the moment. --- app/main/forms.py | 14 +++ app/main/views/uploads.py | 18 ++-- app/templates/views/uploads/preview.html | 9 +- tests/app/main/views/test_uploads.py | 105 ++++++++++++++++++++++- 4 files changed, 136 insertions(+), 10 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 04c51202f..ed27caa81 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -9,6 +9,7 @@ from flask_wtf import FlaskForm as Form from flask_wtf.file import FileAllowed from flask_wtf.file import FileField as FileField_wtf from notifications_utils.columns import Columns +from notifications_utils.countries.data import Postage from notifications_utils.formatters import strip_whitespace from notifications_utils.postal_address import PostalAddress from notifications_utils.recipients import ( @@ -850,6 +851,19 @@ class LetterTemplatePostageForm(StripWhitespaceForm): class LetterUploadPostageForm(StripWhitespaceForm): + + def __init__(self, *args, postage_zone, **kwargs): + + super().__init__(*args, **kwargs) + + if postage_zone != Postage.UK: + self.postage.choices = [(postage_zone, '')] + self.postage.data = postage_zone + + @property + def show_postage(self): + return len(self.postage.choices) > 1 + postage = RadioField( 'Choose the postage for this letter', choices=[ diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 8a4a25034..d72baf691 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -19,6 +19,7 @@ from flask import ( ) from notifications_utils.columns import Columns from notifications_utils.pdf import pdf_page_count +from notifications_utils.postal_address import PostalAddress from notifications_utils.recipients import RecipientCSV from notifications_utils.sanitise_text import SanitiseASCII from PyPDF2.utils import PdfReadError @@ -279,7 +280,7 @@ def uploaded_letter_preview(service_id, file_id): status = metadata.get('status') error_shortcode = metadata.get('message') invalid_pages = metadata.get('invalid_pages') - recipient = format_recipient(metadata.get('recipient', '')) + postal_address = PostalAddress(metadata.get('recipient', '')) if invalid_pages: invalid_pages = json.loads(invalid_pages) @@ -291,7 +292,9 @@ def uploaded_letter_preview(service_id, file_id): # a non null value of postage for letter templates template_dict['postage'] = None - form = LetterUploadPostageForm() + form = LetterUploadPostageForm( + postage_zone=postal_address.postage + ) template = get_template( template_dict, @@ -313,7 +316,8 @@ def uploaded_letter_preview(service_id, file_id): message=error_message, error_code=error_shortcode, form=form, - recipient=recipient, + recipient=format_recipient(postal_address.raw_address), + international=postal_address.international, re_upload_form=re_upload_form ) @@ -349,9 +353,11 @@ def send_uploaded_letter(service_id, file_id=None): if metadata.get('status') != 'valid': abort(403) - recipient_address = metadata.get('recipient') + postal_address = PostalAddress(metadata.get('recipient')) - form = LetterUploadPostageForm() + form = LetterUploadPostageForm( + postage_zone=postal_address.postage + ) if not form.validate_on_submit(): return uploaded_letter_preview(service_id, file_id) @@ -361,7 +367,7 @@ def send_uploaded_letter(service_id, file_id=None): metadata.get('filename'), file_id, form.postage.data, - recipient_address, + postal_address.raw_address, ) return redirect(url_for( diff --git a/app/templates/views/uploads/preview.html b/app/templates/views/uploads/preview.html index fb11980c9..602705559 100644 --- a/app/templates/views/uploads/preview.html +++ b/app/templates/views/uploads/preview.html @@ -52,12 +52,19 @@

{% if current_service.live %} + {% if international %} +

+ Postage: international +

+ {% endif %}
- {{ radios(form.postage, hide_legend=true, inline=True) }} + {% if form.show_postage %} + {{ radios(form.postage, hide_legend=true, inline=True) }} + {% endif %} {{ page_footer("Send 1 letter") }}
{% endif %} diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 24614b6f8..69fb54481 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -435,6 +435,64 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file( page=page_no) +def test_upload_international_letter_shows_preview_with_no_choice_of_postage( + mocker, + active_user_with_permissions, + service_one, + client_request, + fake_uuid, +): + letter_template = { + 'template_type': 'letter', + 'reply_to_text': '', + 'postage': 'second', + 'subject': 'hi', + 'content': 'my letter', + } + + mocker.patch('uuid.uuid4', return_value=fake_uuid) + mocker.patch('app.main.views.uploads.antivirus_client.scan', return_value=True) + mocker.patch('app.main.views.uploads.sanitise_letter', return_value=Mock( + content='The sanitised content', + json=lambda: {'file': 'VGhlIHNhbml0aXNlZCBjb250ZW50', 'recipient_address': 'The Queen'} + )) + mocker.patch('app.main.views.uploads.upload_letter_to_s3') + mocker.patch('app.main.views.uploads.pdf_page_count', return_value=3) + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ + 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', + 'page_count': '3', + 'status': 'valid', + 'recipient': ( + '123 Example Street\n' + 'Andorra la Vella\n' + 'Andorra' + ), + })) + mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template) + + service_one['restricted'] = False + client_request.login(active_user_with_permissions, service=service_one) + + with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: + page = client_request.post( + 'main.upload_letter', + service_id=SERVICE_ONE_ID, + _data={'file': file}, + _follow_redirects=True, + ) + + assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf' + assert not page.select('.letter-postage') + assert not page.select('input[type=radio]') + assert normalize_spaces( + page.select_one('.js-stick-at-bottom-when-scrolling').text + ) == ( + 'Recipient: 123 Example Street, Andorra la Vella, Andorra ' + 'Postage: international ' + 'Send 1 letter' + ) + + def test_post_upload_letter_shows_error_when_file_is_not_a_pdf(client_request): with open('tests/non_spreadsheet_files/actually_a_png.csv', 'rb') as file: page = client_request.post( @@ -766,13 +824,48 @@ def test_uploaded_letter_preview_image_400s_for_bad_page_type( ) +@pytest.mark.parametrize('address, post_data, expected_postage', ( + ( + 'address', + {'filename': 'my_file.pdf', 'postage': 'first'}, + 'first', + ), + ( + 'address', + {'filename': 'my_file.pdf'}, + 'second', + ), + ( + '123 Example Street\nLiechtenstein', + {'filename': 'my_file.pdf', 'postage': 'first'}, + 'europe', + ), + ( + '123 Example Street\nLiechtenstein', + {'filename': 'my_file.pdf'}, + 'europe', + ), + ( + '123 Example Street\nLesotho', + {'filename': 'my_file.pdf'}, + 'rest-of-world', + ), +)) def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page( mocker, service_one, client_request, fake_uuid, + address, + post_data, + expected_postage, ): - metadata = LetterMetadata({'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'address'}) + metadata = LetterMetadata({ + 'filename': 'my_file.pdf', + 'page_count': '1', + 'status': 'valid', + 'recipient': address, + }) mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', metadata)) mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') @@ -784,7 +877,7 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page( 'main.send_uploaded_letter', service_id=SERVICE_ONE_ID, file_id=fake_uuid, - _data={'filename': 'my_file.pdf', 'postage': 'first'}, + _data=post_data, _expected_redirect=url_for( 'main.view_notification', service_id=SERVICE_ONE_ID, @@ -792,7 +885,13 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page( _external=True ) ) - mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', fake_uuid, 'first', 'address') + mock_send.assert_called_once_with( + SERVICE_ONE_ID, + 'my_file.pdf', + fake_uuid, + expected_postage, + address, + ) @pytest.mark.parametrize('permissions', [