Merge pull request #3144 from alphagov/choose-postage

Choose postage

app/assets/stylesheets/components/radios.scss

@@ -111,3 +111,9 @@
   }
 
 }
+
+.inline {
+  .multiple-choice {
+    margin-right: 15px;
+  }
+}

app/main/forms.py

@@ -765,6 +765,21 @@ 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(
+        validators=[DataRequired()]
+    )
+
+
 class ForgotPasswordForm(StripWhitespaceForm):
     email_address = email_address(gov_user=False)
 

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,
@@ -153,6 +153,12 @@ 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()
 
     template = get_template(
         template_dict,
@@ -172,6 +178,7 @@ def uploaded_letter_preview(service_id, file_id):
         status=status,
         file_id=file_id,
         error=error,
+        form=form,
     )
 
 
@@ -194,14 +201,20 @@ def send_uploaded_letter(service_id):
     if not (current_service.has_permission('letter') and current_service.has_permission('upload_letters')):
         abort(403)
 
-    file_id = request.form['file_id']
+    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')
 
     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',

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)

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) %}
+{% macro radios(field, hint=None, disable=[], option_hints={}, hide_legend=False, inline=False) %}
-  {{ select(field, hint, disable, option_hints, hide_legend, input="radio") }}
+  {{ select(field, hint, disable, option_hints, hide_legend, input="radio", inline=inline) }}
 {% endmacro %}
 
 

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 %}
   <div class="form-group {% if field.errors %} form-group-error{% endif %}"{% if is_collapsible %} data-module="collapsible-checkboxes"{% if collapsible_opts.field %} data-field-label="{{ collapsible_opts.field }}"{% endif %}{% endif %}>
     {% if is_collapsible %}
     <div class="selection-summary" role="region" aria-live="polite"></div>
     {% endif %}
-    <fieldset id="{{ field.id }}">
+    <fieldset id="{{ field.id }}" {% if inline %}class="inline"{% endif %}>
       <legend class="{{ 'form-label' if not hide_legend else '' }}{% if legend_style != 'text' %} {{ legend_style }}{% endif %}">
         {% if hide_legend %}<span class="visually-hidden">{% endif %}
           {{ field.label.text|safe }}

app/templates/views/uploads/preview.html

@@ -1,6 +1,8 @@
 {% extends "withnav_template.html" %}
 {% from "components/banner.html" import banner_wrapper %}
 {% from "components/page-header.html" import page_header %}
+{% from "components/page-footer.html" import page_footer %}
+{% from "components/radios.html" import radios %}
 
 {% block service_page_title %}
   {{ original_filename }}
@@ -39,9 +41,9 @@
           'main.send_uploaded_letter',
           service_id=current_service.id,
         )}}" class='page-footer'>
-        <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
+          {{ radios(form.postage, hide_legend=true, inline=True) }}
-        <input type="hidden" name="file_id" value="{{ file_id }}" />
+          {{ form.file_id(value=file_id) }}
-        <button type="submit" class="button">Send 1 letter</button>
+          {{ page_footer("Send 1 letter") }}
       </form>
     </div>
   {% endif %}

requirements-app.txt

@@ -23,5 +23,5 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@35.0.0#egg=notifications-utils==35.0.0
+git+https://github.com/alphagov/notifications-utils.git@36.0.0#egg=notifications-utils==36.0.0
 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.3.0-alpha#egg=govuk-frontend-jinja==0.3.0-alpha

requirements.txt

@@ -25,14 +25,14 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@35.0.0#egg=notifications-utils==35.0.0
+git+https://github.com/alphagov/notifications-utils.git@36.0.0#egg=notifications-utils==36.0.0
 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.3.0-alpha#egg=govuk-frontend-jinja==0.3.0-alpha
 
 ## The following requirements were added by pip freeze:
-awscli==1.16.255
+awscli==1.16.266
 bleach==3.1.0
 boto3==1.9.221
-botocore==1.12.245
+botocore==1.13.2
 certifi==2019.9.11
 chardet==3.0.4
 Click==7.0
@@ -42,7 +42,7 @@ docopt==0.6.2
 docutils==0.15.2
 et-xmlfile==1.0.1
 flask-redis==0.4.0
-future==0.17.1
+future==0.18.1
 greenlet==0.4.15
 idna==2.8
 jdcal==1.4.1
@@ -63,7 +63,7 @@ PyPDF2==1.26.0
 python-dateutil==2.8.0
 python-json-logger==0.1.11
 PyYAML==5.1.2
-redis==3.3.8
+redis==3.3.11
 requests==2.22.0
 rsa==3.4.2
 s3transfer==0.2.1

tests/app/main/views/test_uploads.py

@@ -125,9 +125,10 @@ 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 len(page.select('.letter-postage')) == 0
-    assert normalize_spaces(page.select_one('.letter-postage').text) == ('Postage: second class')
+    # Check postage radios exists and second class is checked by default
-    assert page.select_one('.letter-postage')['class'] == ['letter-postage', 'letter-postage-second']
+    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
@@ -283,9 +284,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 len(page.select('.letter-postage')) == 0
-    assert normalize_spaces(page.select_one('.letter-postage').text) == ('Postage: first class')
-    assert page.select_one('.letter-postage')['class'] == ['letter-postage', 'letter-postage-first']
 
     letter_images = page.select('main img')
     assert len(letter_images) == 1
@@ -421,7 +420,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,
@@ -429,7 +428,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', [
@@ -452,7 +451,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

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',