From 584fac9683f82e93838e60c65819bf16310c0b94 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 3 Mar 2016 11:14:43 +0000 Subject: [PATCH 1/3] If the file was invalid and Upload a CSV file was clicked, the job was created, then the send would fail when sending the file, trying to replace a placeholder that didn't exist. This commit calls send_messages again if the files exist on the request. --- app/main/views/send.py | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index f656c20e4..71349ae8c 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -197,21 +197,25 @@ def check_messages(service_id, upload_id): form=CsvUploadForm() ) elif request.method == 'POST': - original_file_name = upload_data.get('original_file_name') - notification_count = upload_data.get('notification_count') - session.pop('upload_data') - try: - job_api_client.create_job(upload_id, service_id, template_id, original_file_name, notification_count) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + if request.files: + # The csv was invalid, validate the csv again + return send_messages(service_id, template_id) + else: + original_file_name = upload_data.get('original_file_name') + notification_count = upload_data.get('notification_count') + session.pop('upload_data') + try: + job_api_client.create_job(upload_id, service_id, template_id, original_file_name, notification_count) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e - flash('We’ve started sending your messages', 'default_with_tick') - return redirect( - url_for('main.view_job', service_id=service_id, job_id=upload_id) - ) + flash('We’ve started sending your messages', 'default_with_tick') + return redirect( + url_for('main.view_job', service_id=service_id, job_id=upload_id) + ) def _get_filedata(file): From 35c3be514646f8b75bd514f5dd0c24a299313990 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 3 Mar 2016 15:26:52 +0000 Subject: [PATCH 2/3] Add test. Remove else since it is not needed. --- app/main/views/send.py | 30 ++++++++++++++--------------- tests/app/main/views/test_send.py | 32 +++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 71349ae8c..3b3e8d19e 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -200,22 +200,22 @@ def check_messages(service_id, upload_id): if request.files: # The csv was invalid, validate the csv again return send_messages(service_id, template_id) - else: - original_file_name = upload_data.get('original_file_name') - notification_count = upload_data.get('notification_count') - session.pop('upload_data') - try: - job_api_client.create_job(upload_id, service_id, template_id, original_file_name, notification_count) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e - flash('We’ve started sending your messages', 'default_with_tick') - return redirect( - url_for('main.view_job', service_id=service_id, job_id=upload_id) - ) + original_file_name = upload_data.get('original_file_name') + notification_count = upload_data.get('notification_count') + session.pop('upload_data') + try: + job_api_client.create_job(upload_id, service_id, template_id, original_file_name, notification_count) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e + + flash('We’ve started sending your messages', 'default_with_tick') + return redirect( + url_for('main.view_job', service_id=service_id, job_id=upload_id) + ) def _get_filedata(file): diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 600b8c6b0..c9f892c69 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -257,3 +257,35 @@ def test_create_job_should_call_api( assert response.status_code == 200 assert 'We’ve started sending your messages' in response.get_data(as_text=True) mock_create_job.assert_called_with(job_id, service_id, template_id, original_file_name, notification_count) + + +def test_check_messages_should_revalidate_file_when_uploading_file( + app_, + service_one, + api_user_active, + mock_login, + mock_get_service, + job_data, + mock_create_job, + mock_get_service_template, + mock_s3_upload, + mocker +): + + service_id = service_one['id'] + contents = 'phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n' + file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') + upload_data = {'file': file_data} + + mocker.patch('app.main.views.send.s3download', return_value=contents) + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + with client.session_transaction() as session: + session['upload_data'] = {'original_file_name': 'invalid.csv', + 'template_id': job_data['template'], + 'notification_count': job_data['notification_count']} + url = url_for('main.check_messages', service_id=service_id, upload_id=job_data['id']) + response = client.post(url, data=upload_data, follow_redirects=True) + assert response.status_code == 200 + assert 'Your CSV file contained missing or invalid data' in response.get_data(as_text=True) From 72ec653846d96882045f9241a028382648585bd6 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 3 Mar 2016 15:39:15 +0000 Subject: [PATCH 3/3] Add mock_has_permissions on new test --- tests/app/main/views/test_send.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index f3e194009..4a84ddcdd 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -279,7 +279,8 @@ def test_check_messages_should_revalidate_file_when_uploading_file( mock_create_job, mock_get_service_template, mock_s3_upload, - mocker + mocker, + mock_has_permissions ): service_id = service_one['id']