From e74d261ec33514d2e6ef91e3bf0e9d57d8d28e4c Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 29 Mar 2018 11:56:53 +0100
Subject: [PATCH 1/2] =?UTF-8?q?Don=E2=80=99t=20store=20info=20about=20bad?=
 =?UTF-8?q?=20uploads=20in=20session?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Because we now[1] store info about each file upload separately in the
session the session isn’t overridden every time you upload a file. This
is good because you can do multiple file uploads idempotently.

Generally we are cleaning up after ourselves because we pop anything to
do with that upload from the session. However there is an edge case: if
you never send the file then the info about the file stays in the
session in perpetuity[2]. This is generally happening when people are
uploading files that are impossible to send, ie ones that have errors.

So this commit makes two changes:

1. remove info about a file upload from the session as soon as we know
   that it contains errors
2. `POST` reuploads to the same endpoint as initial uploads because
   otherwise we need to keep info about bad uploads in the session,
   which would prevent us from doing 1.

1. https://github.com/alphagov/notifications-admin/pull/1968
2. or at least until the session is cleared by the user logging out
---
 app/main/views/send.py                       | 23 ++++-----
 app/templates/components/file-upload.html    | 10 +++-
 app/templates/views/check/column-errors.html |  6 ++-
 app/templates/views/check/row-errors.html    |  6 ++-
 tests/app/main/views/test_send.py            | 49 ++++++++------------
 5 files changed, 46 insertions(+), 48 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index e9066770c..0d419c513 100644
--- a/app/main/views/send.py
+++ b/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)
diff --git a/app/templates/components/file-upload.html b/app/templates/components/file-upload.html
index a8bfe8cba..8843da7c3 100644
--- a/app/templates/components/file-upload.html
+++ b/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 %}
diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html
index cfe37a8a7..c30145912 100644
--- a/app/templates/views/check/column-errors.html
+++ b/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>
diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html
index 0bf3f6f15..c2db83b61 100644
--- a/app/templates/views/check/row-errors.html
+++ b/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>
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 4db0e94b6..dd4541839 100644
--- a/tests/app/main/views/test_send.py
+++ b/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',

From becee15e1e1eac031887d7a867067c78963aff1f Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 29 Mar 2018 12:07:37 +0100
Subject: [PATCH 2/2] Add logging for session not being set bug

This is so we can be alerted to it before our users alert us to it.
---
 app/main/views/send.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 0d419c513..a9c215e0f 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -623,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'):