Merge pull request #3455 from alphagov/remove-file_id-from-form

Don’t show postage choice for international letters

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):

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/<uuid:service_id>/preview-letter/<uuid:file_id>")
 @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/<uuid:service_id>/upload-letter/send", methods=['POST'])
 @main.route("/services/<uuid:service_id>/upload-letter/send/<uuid:file_id>", 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',

app/templates/views/uploads/preview.html

@@ -48,16 +48,23 @@
     {% if status == 'valid' %}
     <div class="js-stick-at-bottom-when-scrolling">
       <p class="top-gutter-0 bottom-gutter-1-2 send-recipient" title="{{ recipient }}">
-        Recipient: {{ recipient }}
+        Recipient: {{ postal_address.as_single_line }}
       </p>
 
       {% if current_service.live %}
+        {% if postal_address.international %}
+          <p class="govuk-body">
+            Postage: international
+          </p>
+        {% endif %}
         <form method="post" enctype="multipart/form-data" action="{{url_for(
             'main.send_uploaded_letter',
             service_id=current_service.id,
             file_id=file_id,
           )}}" class='page-footer'>
-            {{ 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") }}
         </form>
       {% endif %}

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

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

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(),