Merge pull request #1998 from alphagov/expanding-session

Don’t store info about bad uploads in session

app/main/views/send.py

@@ -541,8 +541,14 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
     elif preview_row > 2:
         abort(404)
 
-    session['file_uploads'][upload_id]['notification_count'] = len(recipients)
-    session['file_uploads'][upload_id]['valid'] = not recipients.has_errors
+    original_file_name = session['file_uploads'][upload_id].get('original_file_name')
+
+    if any(recipients) and not recipients.has_errors:
+        session['file_uploads'][upload_id]['notification_count'] = len(recipients)
+        session['file_uploads'][upload_id]['valid'] = True
+    else:
+        session['file_uploads'].pop(upload_id)
+
     return dict(
         recipients=recipients,
         template=template,
@@ -550,7 +556,7 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
         row_errors=get_errors_for_csv(recipients, template.template_type),
         count_of_recipients=len(recipients),
         count_of_displayed_recipients=len(list(recipients.displayed_rows)),
-        original_file_name=session['file_uploads'][upload_id].get('original_file_name'),
+        original_file_name=original_file_name,
         upload_id=upload_id,
         form=CsvUploadForm(),
         remaining_messages=remaining_messages,
@@ -610,17 +616,6 @@ def check_messages_preview(service_id, template_type, upload_id, filetype, row_i
     return TemplatePreview.from_utils_template(template, filetype)
 
 
-@main.route("/services/<service_id>/<template_type>/check/<upload_id>", methods=['POST'])
-@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", methods=['POST'])
-@login_required
-@user_has_permissions('send_messages', restrict_admin_usage=True)
-def recheck_messages(service_id, template_type, upload_id, row_index=0):
-    if not session.get('file_uploads', {}).get(upload_id):
-        return redirect(url_for('main.choose_template', service_id=service_id), code=301)
-
-    return send_messages(service_id, session['file_uploads'][upload_id].get('template_id'))
-
-
 @main.route("/services/<service_id>/start-job/<upload_id>", methods=['POST'])
 @login_required
 @user_has_permissions('send_messages', restrict_admin_usage=True)
@@ -628,6 +623,7 @@ def start_job(service_id, upload_id):
     try:
         upload_data = session['file_uploads'][upload_id]
     except KeyError:
+        current_app.logger.exception('upload_id not in session')
         return redirect(url_for('main.choose_template', service_id=service_id), code=301)
 
     if request.files or not upload_data.get('valid'):

app/templates/components/file-upload.html

@@ -1,5 +1,11 @@
-{% macro file_upload(field, button_text="Choose file", alternate_link=None, alternate_link_text=None) %}
-  <form method="post" enctype="multipart/form-data" class="{% if field.errors %}form-group-error{% endif %}" data-module="file-upload">
+{% macro file_upload(
+  field,
+  action=None,
+  button_text="Choose file",
+  alternate_link=None,
+  alternate_link_text=None
+) %}
+  <form method="post" enctype="multipart/form-data" {% if action %}action="{{ action }}"{% endif %} class="{% if field.errors %}form-group-error{% endif %}" data-module="file-upload">
     <label class="file-upload-label" for="{{ field.name }}">
       <span class="visually-hidden">{{ field.label.text }}</span>
       {% if hint %}

app/templates/views/check/column-errors.html

@@ -135,7 +135,11 @@
       {% if request.args.from_test %}
         <a href="{{ back_link }}" class="page-footer-back-link">Back</a>
       {% else %}
-        {{file_upload(form.file, button_text='Re-upload your file')}}
+        {{ file_upload(
+          form.file,
+          action=url_for('.send_messages', service_id=current_service.id, template_id=template.id),
+          button_text='Re-upload your file'
+        ) }}
       {% endif %}
     </div>
     <a href="#content" class="back-to-top-link">Back to top</a>

app/templates/views/check/row-errors.html

@@ -47,7 +47,11 @@
 
   <div class="js-stick-at-top-when-scrolling">
     <div class="form-group">
-      {{ file_upload(form.file, button_text='Re-upload your file') }}
+      {{ file_upload(
+        form.file,
+        action=url_for('.send_messages', service_id=current_service.id, template_id=template.id),
+        button_text='Re-upload your file'
+      ) }}
     </div>
     <a href="#content" class="back-to-top-link">Back to top</a>
   </div>

tests/app/main/views/test_send.py

@@ -372,27 +372,22 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
         """
     )
 
-    initial_upload = logged_in_client.post(
+    response = logged_in_client.post(
         url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid),
         data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
         content_type='multipart/form-data',
         follow_redirects=True
     )
 
-    reupload = logged_in_client.post(
-        url_for('main.check_messages', service_id=service_one['id'], template_type='sms', upload_id=fake_uuid),
-        data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
-        content_type='multipart/form-data',
-        follow_redirects=True
-    )
+    with logged_in_client.session_transaction() as session:
+        assert session['file_uploads'] == {}
 
-    for response in [initial_upload, reupload]:
-        assert response.status_code == 200
-        content = response.get_data(as_text=True)
-        assert 'There is a problem with invalid.csv' in content
-        assert '+447700900986' in content
-        assert 'Missing' in content
-        assert 'Re-upload your file' in content
+    assert response.status_code == 200
+    content = response.get_data(as_text=True)
+    assert 'There is a problem with invalid.csv' in content
+    assert '+447700900986' in content
+    assert 'Missing' in content
+    assert 'Re-upload your file' in content
 
 
 @pytest.mark.parametrize('file_contents, expected_error,', [
@@ -483,7 +478,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
     ),
 ])
 def test_upload_csvfile_with_missing_columns_shows_error(
-    logged_in_client,
+    client_request,
     mocker,
     mock_get_service_template_with_placeholders,
     mock_s3_upload,
@@ -497,15 +492,16 @@ def test_upload_csvfile_with_missing_columns_shows_error(
 
     mocker.patch('app.main.views.send.s3download', return_value=file_contents)
 
-    response = logged_in_client.post(
-        url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid),
-        data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
-        follow_redirects=True,
+    page = client_request.post(
+        'main.send_messages', service_id=service_one['id'], template_id=fake_uuid,
+        _data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
+        _follow_redirects=True,
     )
 
-    assert response.status_code == 200
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-    assert ' '.join(page.select('.banner-dangerous')[0].text.split()) == expected_error
+    with client_request.session_transaction() as session:
+        assert session['file_uploads'] == {}
+
+    assert normalize_spaces(page.select('.banner-dangerous')[0].text) == expected_error
 
 
 def test_upload_csv_invalid_extension(
@@ -1426,6 +1422,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(
     mock_get_service_template,
     mock_get_users_by_service,
     mock_get_detailed_service_for_today,
+    mock_get_live_service,
     service_one,
     fake_uuid,
     mock_s3_upload,
@@ -2787,14 +2784,6 @@ def test_sms_sender_is_previewed(
             'filetype': 'png'
         }
     ),
-    (
-        'main.recheck_messages',
-        'POST',
-        {
-            'template_type': 'email',
-            'upload_id': fake_uuid()
-        }
-    ),
     (
         'main.start_job',
         'POST',