From ba9935c49e9bb1d947d482f300b8b7e7e7998955 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 29 Mar 2018 17:08:21 +0100 Subject: [PATCH] =?UTF-8?q?Remove=20=E2=80=98check=E2=80=99=20page?= =?UTF-8?q?=E2=80=99s=20reliance=20on=20session?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A of this commit’s parent we are storing `template_id` and `original_file_name` in the URL. Getting them from the URL is better, so the check page no longer needs to look for them in the session. This commit removes the code that looks for these values in the session. --- app/main/views/send.py | 30 +++------------------- tests/app/main/views/test_send.py | 42 +++---------------------------- 2 files changed, 7 insertions(+), 65 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 573ee9102..41897aa22 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -20,7 +20,6 @@ from notifications_utils.recipients import ( optional_address_columns, ) from orderedset import OrderedSet -from werkzeug.routing import RequestRedirect from xlrd.biffh import XLRDError from xlrd.xldate import XLDateError @@ -126,14 +125,6 @@ def send_messages(service_id, template_id): Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict, current_app.config['AWS_REGION'] ) - if 'file_uploads' not in session: - session['file_uploads'] = {} - session['file_uploads'].update({ - upload_id: { - "template_id": template_id, - "original_file_name": form.file.data.filename - } - }) return redirect(url_for( '.check_messages', service_id=service_id, @@ -476,15 +467,6 @@ def send_test_preview(service_id, template_id, filetype): def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_pdf=False): - if not session.get('file_uploads', {}).get(upload_id): - # if we just return a `redirect` (302) object here, we'll get errors when we try and unpack in the - # check_messages route - so raise a werkzeug.routing redirect to ensure that doesn't happen. - - # NOTE: this is a 301 MOVED PERMANENTLY (httpstatus.es/301), so the browser will cache this redirect, and it'll - # *always* happen for that browser. _check_messages is only used by endpoints that contain `upload_id`, which - # is a one-time-use id (that ties to a given file in S3 that is already deleted if it's not in the session) - raise RequestRedirect(url_for('main.send_messages', service_id=service_id, template_id=template_id)) - users = user_api_client.get_users_for_service(service_id=service_id) statistics = service_api_client.get_detailed_service_for_today(service_id)['data']['statistics'] @@ -499,12 +481,10 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ email_reply_to = None sms_sender = None - if db_template['template_type'] == 'email': email_reply_to = get_email_reply_to_address_from_session(service_id) elif db_template['template_type'] == 'sms': sms_sender = get_sms_sender_from_session(service_id) - template = get_template( service_api_client.get_service_template( service_id, @@ -552,6 +532,10 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ elif preview_row > 2: abort(404) + if 'file_uploads' not in session: + session['file_uploads'] = {} + session['file_uploads'][upload_id] = {} + if any(recipients) and not recipients.has_errors: session['file_uploads'][upload_id]['notification_count'] = len(recipients) session['file_uploads'][upload_id]['valid'] = True @@ -719,12 +703,6 @@ def make_and_upload_csv_file(service_id, template): ).as_dict, current_app.config['AWS_REGION'], ) - if 'file_uploads' not in session: - session['file_uploads'] = {} - session['file_uploads'][upload_id] = { - "template_id": template.id, - "original_file_name": current_app.config['TEST_MESSAGE_FILENAME'] - } return redirect(url_for( '.check_messages', upload_id=upload_id, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index dd1d1a633..0c6795a88 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1443,9 +1443,10 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( follow_redirects=True ) with logged_in_client.session_transaction() as sess: - assert sess['file_uploads'][fake_uuid]['template_id'] == fake_uuid - assert 'original_file_name' not in sess['file_uploads'] + assert 'template_id' not in sess['file_uploads'][fake_uuid] + assert 'original_file_name' not in sess['file_uploads'][fake_uuid] assert sess['file_uploads'][fake_uuid]['notification_count'] == 53 + assert sess['file_uploads'][fake_uuid]['valid'] is True content = response.get_data(as_text=True) assert response.status_code == 200 @@ -2344,26 +2345,6 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error( assert page.find('span', class_='table-field-error-label').text == u'Can’t include П, е, т or я' -def test_check_messages_redirects_if_no_upload_data( - client_request, - fake_uuid, - mock_get_service_template, -): - client_request.get( - 'main.check_messages', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - upload_id=fake_uuid, - _expected_status=301, - _expected_redirect=url_for( - 'main.send_messages', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - _external=True, - ) - ) - - @pytest.mark.parametrize('existing_session_items', [ {}, {'recipient': '07700900001'}, @@ -2782,23 +2763,6 @@ def test_sms_sender_is_previewed( @pytest.mark.parametrize('endpoint, request_type, extra_args', [ - ( - 'main.check_messages', - 'GET', - { - 'template_id': fake_uuid(), - 'upload_id': fake_uuid(), - } - ), - ( - 'main.check_messages_preview', - 'GET', - { - 'template_id': fake_uuid(), - 'upload_id': fake_uuid(), - 'filetype': 'png' - } - ), ( 'main.start_job', 'POST',