diff --git a/app/main/forms.py b/app/main/forms.py index 0312537ef..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=[ @@ -859,9 +873,6 @@ class LetterUploadPostageForm(StripWhitespaceForm): default='second', validators=[DataRequired()] ) - file_id = HiddenField( - validators=[DataRequired()] - ) class ForgotPasswordForm(StripWhitespaceForm): diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 672f5b83c..e1093f708 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 @@ -251,23 +252,6 @@ def _get_error_from_upload_form(form_errors): return error -def format_recipient(address): - ''' - To format the recipient we need to: - - remove new line characters - - remove whitespace around the lines - - join the address lines, separated by a comma - ''' - if not address: - return address - stripped_address_lines_no_trailing_commas = [ - line.lstrip().rstrip(' ,') - for line in address.splitlines() if line - ] - one_line_address = ', '.join(stripped_address_lines_no_trailing_commas) - return one_line_address - - @main.route("/services//preview-letter/") @user_has_permissions('send_messages') def uploaded_letter_preview(service_id, file_id): @@ -279,7 +263,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 +275,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 +299,7 @@ def uploaded_letter_preview(service_id, file_id): message=error_message, error_code=error_shortcode, form=form, - recipient=recipient, + postal_address=postal_address, re_upload_form=re_upload_form ) @@ -338,28 +324,33 @@ def view_letter_upload_as_preview(service_id, file_id): return TemplatePreview.from_valid_pdf_file(pdf_file, page) -@main.route("/services//upload-letter/send", methods=['POST']) @main.route("/services//upload-letter/send/", methods=['POST']) @user_has_permissions('send_messages', restrict_admin_usage=True) -def send_uploaded_letter(service_id, file_id=None): +def send_uploaded_letter(service_id, file_id): if not (current_service.has_permission('letter') and current_service.has_permission('upload_letters')): abort(403) - form = LetterUploadPostageForm(file_id=file_id) - file_id = file_id or 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') - recipient_address = metadata.get('recipient') if metadata.get('status') != 'valid': abort(403) - notification_api_client.send_precompiled_letter(service_id, filename, file_id, postage, recipient_address) + postal_address = PostalAddress(metadata.get('recipient')) + + form = LetterUploadPostageForm( + postage_zone=postal_address.postage + ) + + if not form.validate_on_submit(): + return uploaded_letter_preview(service_id, file_id) + + notification_api_client.send_precompiled_letter( + service_id, + metadata.get('filename'), + file_id, + form.postage.data, + postal_address.raw_address, + ) return redirect(url_for( '.view_notification', diff --git a/app/templates/views/uploads/preview.html b/app/templates/views/uploads/preview.html index fb11980c9..5e9538a98 100644 --- a/app/templates/views/uploads/preview.html +++ b/app/templates/views/uploads/preview.html @@ -48,16 +48,23 @@ {% if status == 'valid' %}

- Recipient: {{ recipient }} + Recipient: {{ postal_address.as_single_line }}

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

+ Postage: international +

+ {% endif %} {% endif %} diff --git a/requirements-app.txt b/requirements-app.txt index 0aad711d9..80a3f30f7 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -23,5 +23,5 @@ notifications-python-client==5.5.1 awscli-cwlogs>=1.4,<1.5 itsdangerous==1.1.0 -git+https://github.com/alphagov/notifications-utils.git@39.2.0#egg=notifications-utils==39.2.0 +git+https://github.com/alphagov/notifications-utils.git@39.3.0#egg=notifications-utils==39.3.0 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha diff --git a/requirements.txt b/requirements.txt index 810a6e2b0..f5d52fee8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,14 +25,14 @@ notifications-python-client==5.5.1 awscli-cwlogs>=1.4,<1.5 itsdangerous==1.1.0 -git+https://github.com/alphagov/notifications-utils.git@39.2.0#egg=notifications-utils==39.2.0 +git+https://github.com/alphagov/notifications-utils.git@39.3.0#egg=notifications-utils==39.3.0 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha ## The following requirements were added by pip freeze: -awscli==1.18.61 +awscli==1.18.63 bleach==3.1.4 boto3==1.10.38 -botocore==1.16.11 +botocore==1.16.13 certifi==2020.4.5.1 chardet==3.0.4 click==7.1.2 @@ -49,7 +49,7 @@ jdcal==1.4.1 Jinja2==2.11.2 jmespath==0.10.0 lml==0.0.9 -lxml==4.5.0 +lxml==4.5.1 MarkupSafe==1.1.1 mistune==0.8.4 monotonic==1.5 diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 163a6bee2..941321500 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -8,7 +8,6 @@ from flask import make_response, url_for from freezegun import freeze_time from requests import RequestException -from app.main.views.uploads import format_recipient from app.s3_client.s3_letter_upload_client import LetterMetadata from app.utils import normalize_spaces from tests.conftest import ( @@ -435,6 +434,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,42 +823,48 @@ def test_uploaded_letter_preview_image_400s_for_bad_page_type( ) -def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mocker, service_one, client_request): - 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') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=metadata) - - service_one['permissions'] = ['letter', 'upload_letters'] - file_id = 'abcd-1234' - - client_request.post( - 'main.send_uploaded_letter', - service_id=SERVICE_ONE_ID, - _data={'filename': 'my_file.pdf', 'file_id': file_id, 'postage': 'first'}, - _expected_redirect=url_for( - 'main.view_notification', - service_id=SERVICE_ONE_ID, - notification_id=file_id, - _external=True - ) - ) - mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', file_id, 'first', 'address') - - -@pytest.mark.parametrize('form_data', ( - {'filename': 'my_file.pdf', 'postage': 'first'}, - {'filename': 'my_file.pdf', 'postage': 'first', 'file_id': 'Ignored in favour of URL'}, +@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_accepts_file_id_in_url( +def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page( mocker, service_one, client_request, fake_uuid, - form_data, + 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') @@ -813,7 +876,7 @@ def test_send_uploaded_letter_accepts_file_id_in_url( 'main.send_uploaded_letter', service_id=SERVICE_ONE_ID, file_id=fake_uuid, - _data=form_data, + _data=post_data, _expected_redirect=url_for( 'main.view_notification', service_id=SERVICE_ONE_ID, @@ -821,32 +884,13 @@ def test_send_uploaded_letter_accepts_file_id_in_url( _external=True ) ) - mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', fake_uuid, 'first', 'address') - - -def test_send_uploaded_letter_needs_file_id_in_form_if_not_in_url( - mocker, - service_one, - client_request, - mock_template_preview, - fake_uuid, -): - metadata = LetterMetadata({'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'address'}) - - mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=metadata) - mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template') - - service_one['permissions'] = ['letter', 'upload_letters'] - - client_request.post( - 'main.send_uploaded_letter', - service_id=SERVICE_ONE_ID, - _data={'filename': 'my_file.pdf', 'postage': 'first'}, - _expected_status=200, - _expected_redirect=None, + mock_send.assert_called_once_with( + SERVICE_ONE_ID, + 'my_file.pdf', + fake_uuid, + expected_postage, + address, ) - assert mock_send.called is False @pytest.mark.parametrize('permissions', [ @@ -859,23 +903,26 @@ def test_send_uploaded_letter_when_service_does_not_have_correct_permissions( service_one, client_request, permissions, + fake_uuid, ): mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', {'status': 'valid'})) mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') service_one['permissions'] = permissions - file_id = 'abcd-1234' client_request.post( 'main.send_uploaded_letter', service_id=SERVICE_ONE_ID, - _data={'filename': 'my_file.pdf', 'file_id': file_id, 'postage': 'first'}, + file_id=fake_uuid, + _data={'filename': 'my_file.pdf', 'postage': 'first'}, _expected_status=403 ) assert not mock_send.called -def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, service_one, client_request): +def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid( + mocker, service_one, client_request, fake_uuid, +): mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') mocker.patch( 'app.main.views.uploads.get_letter_metadata', @@ -888,29 +935,17 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic ) service_one['permissions'] = ['letter', 'upload_letters'] - file_id = 'abcd-1234' client_request.post( 'main.send_uploaded_letter', service_id=SERVICE_ONE_ID, - _data={'filename': 'my_file.pdf', 'file_id': file_id}, + file_id=fake_uuid, + _data={'filename': 'my_file.pdf'}, _expected_status=403 ) assert not mock_send.called -@pytest.mark.parametrize('original_address,expected_address', [ - ('The Queen, Buckingham Palace, SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ('The Queen Buckingham Palace SW1 1AA', 'The Queen Buckingham Palace SW1 1AA'), - ('The Queen,\nBuckingham Palace,\r\nSW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ('The Queen ,,\nBuckingham Palace,\rSW1 1AA,', 'The Queen, Buckingham Palace, SW1 1AA'), - (' The Queen\n Buckingham Palace\n SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ('', ''), -]) -def test_format_recipient(original_address, expected_address): - assert format_recipient(original_address) == expected_address - - @pytest.mark.parametrize('user', ( create_active_caseworking_user(), create_active_user_with_permissions(),