From 3dab34cd38ee628883e0fb7514f9a967e13e96ca Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 29 Mar 2018 17:01:35 +0100 Subject: [PATCH] Store template ID and original file name in URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment you can’t press refresh on the check page if there’s errors. This is because the session gets cleared when there’s errors. This is a bad user experience. The data that this page is relying on (from the session) is: - template ID - original file name Neither of these things need to be in the session because: - they are not secret - the user can modify them already (by choosing a different template or renaming their file locally) So this commit additionally stores them in the URL. --- app/main/views/send.py | 62 ++++++---- app/templates/views/check/ok.html | 6 +- app/templates/views/notifications/check.html | 2 +- tests/app/main/views/test_send.py | 114 +++++++++++-------- 4 files changed, 110 insertions(+), 74 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index a9c215e0f..573ee9102 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -134,10 +134,13 @@ def send_messages(service_id, template_id): "original_file_name": form.file.data.filename } }) - return redirect(url_for('.check_messages', - service_id=service_id, - upload_id=upload_id, - template_type=template.template_type)) + return redirect(url_for( + '.check_messages', + service_id=service_id, + upload_id=upload_id, + template_id=template.id, + original_file_name=form.file.data.filename, + )) except (UnicodeDecodeError, BadZipFile, XLRDError): flash('Couldn’t read {}. Try using a different file format.'.format( form.file.data.filename @@ -471,7 +474,7 @@ def send_test_preview(service_id, template_id, filetype): return TemplatePreview.from_utils_template(template, filetype, page=request.args.get('page')) -def _check_messages(service_id, template_type, upload_id, preview_row, letters_as_pdf=False): +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 @@ -480,7 +483,7 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a # 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.choose_template', service_id=service_id)) + 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) @@ -488,29 +491,37 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values())) contents = s3download(service_id, upload_id) + + db_template = service_api_client.get_service_template( + service_id, + str(template_id), + )['data'] + email_reply_to = None sms_sender = None - if template_type == 'email': + + if db_template['template_type'] == 'email': email_reply_to = get_email_reply_to_address_from_session(service_id) - elif template_type == 'sms': + 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, - session['file_uploads'][upload_id].get('template_id') + str(template_id), )['data'], current_service, show_recipient=True, letter_preview_url=url_for( '.check_messages_preview', service_id=service_id, - template_type=template_type, + template_id=template_id, upload_id=upload_id, filetype='png', row_index=preview_row, ) if not letters_as_pdf else None, email_reply_to=email_reply_to, - sms_sender=sms_sender + sms_sender=sms_sender, ) recipients = RecipientCSV( contents, @@ -541,8 +552,6 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a elif preview_row > 2: abort(404) - 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 @@ -556,7 +565,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=original_file_name, + original_file_name=request.args.get('original_file_name'), upload_id=upload_id, form=CsvUploadForm(), remaining_messages=remaining_messages, @@ -573,13 +582,14 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a ) -@main.route("/services///check/", methods=['GET']) -@main.route("/services///check//row-", methods=['GET']) +@main.route("/services///check/", methods=['GET']) +@main.route("/services///check//row-", methods=['GET']) @login_required @user_has_permissions('send_messages', restrict_admin_usage=True) -def check_messages(service_id, template_type, upload_id, row_index=2): +def check_messages(service_id, template_id, upload_id, row_index=2): + db_template = service_api_client.get_service_template(service_id, template_id)['data'] - data = _check_messages(service_id, template_type, upload_id, row_index) + data = _check_messages(service_id, db_template['id'], upload_id, row_index) if ( data['recipients'].too_many_rows or @@ -602,16 +612,22 @@ def check_messages(service_id, template_type, upload_id, row_index=2): return render_template('views/check/ok.html', **data) -@main.route("/services///check/.", methods=['GET']) -@main.route("/services///check//row-.", methods=['GET']) +@main.route( + "/services///check/.", + methods=['GET'], +) +@main.route( + "/services///check//row-.", + methods=['GET'], +) @login_required @user_has_permissions('send_messages') -def check_messages_preview(service_id, template_type, upload_id, filetype, row_index=2): +def check_messages_preview(service_id, template_id, upload_id, filetype, row_index=2): if filetype not in ('pdf', 'png'): abort(404) template = _check_messages( - service_id, template_type, upload_id, row_index, letters_as_pdf=True + service_id, template_id, upload_id, row_index, letters_as_pdf=True )['template'] return TemplatePreview.from_utils_template(template, filetype) @@ -636,7 +652,7 @@ def start_job(service_id, upload_id): upload_id, service_id, upload_data.get('template_id'), - upload_data.get('original_file_name'), + request.args.get('original_file_name'), upload_data.get('notification_count'), scheduled_for=request.form.get('scheduled_for', '') ) diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index 3e895f404..551a32405 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -27,7 +27,7 @@ {{ template|string }}
-
+ {% if choose_time_form and template.template_type != 'letter' %} @@ -39,7 +39,7 @@ {% if template.template_type != 'letter' or not request.args.from_test %} {% else %} - Download as a printable PDF + Download as a printable PDF {% endif %} Back
@@ -63,7 +63,7 @@ {% if (item.index + 2) == preview_row %} {{ item.index + 2 }} {% else %} - {{ item.index + 2 }} + {{ item.index + 2 }} {% endif %} {% endcall %} diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index 8619daf79..fc2aee128 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -53,7 +53,7 @@ {% if template.template_type != 'letter' or not request.args.from_test %} {% else %} - Download as a printable PDF + Download as a printable PDF {% endif %} {% endif %} Back diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index dd4541839..dd1d1a633 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -586,7 +586,7 @@ def test_upload_valid_csv_shows_preview_and_table( page = client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, - template_type='sms', + template_id=fake_uuid, upload_id=fake_uuid, **extra_args ) @@ -599,7 +599,7 @@ def test_upload_valid_csv_shows_preview_and_table( if expected_link_in_first_row: assert page.select_one('.table-field-index a')['href'] == url_for( - 'main.check_messages', service_id=SERVICE_ONE_ID, template_type='sms', upload_id=fake_uuid, row_index=2 + 'main.check_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid, upload_id=fake_uuid, row_index=2 ) else: assert not page.select_one('.table-field-index').select_one('a') @@ -673,7 +673,7 @@ def test_show_all_columns_if_there_are_duplicate_recipient_columns( page = client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, - template_type='sms', + template_id=fake_uuid, upload_id=fake_uuid, _test_page_title=False, ) @@ -721,7 +721,7 @@ def test_404_for_previewing_a_row_out_of_range( client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, - template_type='sms', + template_id=fake_uuid, upload_id=fake_uuid, row_index=row_index, _expected_status=expected_status, @@ -1444,7 +1444,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( ) with logged_in_client.session_transaction() as sess: assert sess['file_uploads'][fake_uuid]['template_id'] == fake_uuid - assert sess['file_uploads'][fake_uuid]['original_file_name'] == 'valid.csv' + assert 'original_file_name' not in sess['file_uploads'] assert sess['file_uploads'][fake_uuid]['notification_count'] == 53 content = response.get_data(as_text=True) @@ -1517,7 +1517,7 @@ def test_test_message_can_only_be_sent_now( 'main.check_messages', service_id=service_one['id'], upload_id=fake_uuid, - template_type='sms', + template_id=fake_uuid, from_test=True )) @@ -1552,7 +1552,7 @@ def test_letter_can_only_be_sent_now( 'main.check_messages', service_id=service_one['id'], upload_id=fake_uuid, - template_type='letter', + template_id=fake_uuid, from_test=True )) @@ -1583,15 +1583,22 @@ def test_create_job_should_call_api( with logged_in_client.session_transaction() as session: session['file_uploads'] = { fake_uuid: { - 'original_file_name': original_file_name, 'template_id': template_id, 'notification_count': notification_count, 'valid': True } } - url = url_for('main.start_job', service_id=service_one['id'], upload_id=job_id) - response = logged_in_client.post(url, data={'scheduled_for': when}, follow_redirects=True) + response = logged_in_client.post( + url_for( + 'main.start_job', + service_id=service_one['id'], + upload_id=job_id, + original_file_name=original_file_name + ), + data={'scheduled_for': when}, + follow_redirects=True, + ) assert response.status_code == 200 assert original_file_name in response.get_data(as_text=True) @@ -1689,7 +1696,7 @@ def test_should_show_preview_letter_message( url_for( 'main.check_messages_preview', service_id=service_id, - template_type='letter', + template_id=fake_uuid, upload_id=fake_uuid, filetype=filetype, **extra_args @@ -1716,7 +1723,7 @@ def test_dont_show_preview_letter_templates_for_bad_filetype( url_for( 'main.check_messages_preview', service_id=service_one['id'], - template_type='letter', + template_id=fake_uuid, upload_id=fake_uuid, filetype='blah' ) @@ -1931,7 +1938,7 @@ def test_check_messages_back_link( 'main.check_messages', service_id=fake_uuid, upload_id=fake_uuid, - template_type='sms', + template_id=fake_uuid, **extra_args )) assert response.status_code == 200 @@ -2014,7 +2021,6 @@ def test_check_messages_shows_too_many_messages_errors( with logged_in_client.session_transaction() as session: session['file_uploads'] = { fake_uuid: { - 'original_file_name': 'valid.csv', 'template_id': fake_uuid, 'notification_count': 1, 'valid': True @@ -2024,8 +2030,9 @@ def test_check_messages_shows_too_many_messages_errors( response = logged_in_client.get(url_for( 'main.check_messages', service_id=fake_uuid, - template_type='sms', - upload_id=fake_uuid + template_id=fake_uuid, + upload_id=fake_uuid, + original_file_name='valid.csv', )) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -2062,7 +2069,7 @@ def test_check_messages_shows_trial_mode_error( response = logged_in_client.get(url_for( 'main.check_messages', service_id=uuid.uuid4(), - template_type='sms', + template_id=fake_uuid, upload_id=fake_uuid )) assert response.status_code == 200 @@ -2115,7 +2122,7 @@ def test_check_messages_shows_trial_mode_error_for_letters( page = client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, - template_type='letter', + template_id=fake_uuid, upload_id=fake_uuid, _test_page_title=False, ) @@ -2158,9 +2165,10 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters( page = client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, - template_type='letter', + template_id=fake_uuid, upload_id=fake_uuid, _test_page_title=False, + original_file_name='example.xlsx', ) assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( @@ -2196,7 +2204,7 @@ def test_check_messages_column_error_doesnt_show_optional_columns( page = client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, - template_type='letter', + template_id=fake_uuid, upload_id=fake_uuid, _test_page_title=False, ) @@ -2234,7 +2242,7 @@ def test_generate_test_letter_doesnt_block_in_trial_mode( page = client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, - template_type='letter', + template_id=fake_uuid, upload_id=fake_uuid, from_test=True, _test_page_title=False, @@ -2273,7 +2281,7 @@ def test_check_messages_shows_over_max_row_error( response = logged_in_client.get(url_for( 'main.check_messages', service_id=fake_uuid, - template_type='sms', + template_id=fake_uuid, upload_id=fake_uuid )) assert response.status_code == 200 @@ -2313,15 +2321,15 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error( session['file_uploads'] = { fake_uuid: { 'template_id': fake_uuid, - 'original_file_name': 'unicode.csv', } } response = logged_in_client.get(url_for( 'main.check_messages', service_id=fake_uuid, - template_type='letter', - upload_id=fake_uuid + template_id=fake_uuid, + upload_id=fake_uuid, + original_file_name='unicode.csv' )) assert response.status_code == 200 @@ -2336,16 +2344,24 @@ 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(logged_in_client, service_one): - response = logged_in_client.get(url_for( +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_type='bar', - upload_id='baz' - )) - - assert response.status_code == 301 - assert response.location == url_for('main.choose_template', service_id=service_one['id'], _external=True) + 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', [ @@ -2660,8 +2676,12 @@ def test_send_notification_shows_email_error_in_trial_mode( @pytest.mark.parametrize('endpoint, extra_args', [ - ('main.check_messages', {'template_type': 'email', 'upload_id': fake_uuid()}), - ('main.send_one_off_step', {'template_id': fake_uuid(), 'step_index': 0}), + ('main.check_messages', { + 'template_id': fake_uuid(), 'upload_id': fake_uuid(), 'original_file_name': 'example.csv' + }), + ('main.send_one_off_step', { + 'template_id': fake_uuid(), 'step_index': 0 + }), ]) @pytest.mark.parametrize('reply_to_address', [ None, @@ -2689,12 +2709,7 @@ def test_reply_to_is_previewed_if_chosen( session['recipient'] = 'notify@digital.cabinet-office.gov.uk' session['placeholders'] = {} session['file_uploads'] = { - fake_uuid: { - 'original_file_name': 'example.csv', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True - } + fake_uuid: {'template_id': fake_uuid} } session['sender_id'] = reply_to_address @@ -2713,7 +2728,7 @@ def test_reply_to_is_previewed_if_chosen( @pytest.mark.parametrize('endpoint, extra_args', [ - ('main.check_messages', {'template_type': 'sms', 'upload_id': fake_uuid()}), + ('main.check_messages', {'template_id': fake_uuid(), 'upload_id': fake_uuid()}), ('main.send_one_off_step', {'template_id': fake_uuid(), 'step_index': 0}), ]) @pytest.mark.parametrize('sms_sender', [ @@ -2771,7 +2786,7 @@ def test_sms_sender_is_previewed( 'main.check_messages', 'GET', { - 'template_type': 'email', + 'template_id': fake_uuid(), 'upload_id': fake_uuid(), } ), @@ -2779,7 +2794,7 @@ def test_sms_sender_is_previewed( 'main.check_messages_preview', 'GET', { - 'template_type': 'email', + 'template_id': fake_uuid(), 'upload_id': fake_uuid(), 'filetype': 'png' } @@ -2802,6 +2817,7 @@ def test_sms_sender_is_previewed( ]) def test_redirects_to_choose_template_if_no_session_exists_for_upload_id( client_request, + mock_get_service_email_template, endpoint, request_type, session_data, @@ -2817,7 +2833,9 @@ def test_redirects_to_choose_template_if_no_session_exists_for_upload_id( service_id=SERVICE_ONE_ID, **extra_args, _expected_status=301, - _expected_redirect=url_for('main.choose_template', service_id=SERVICE_ONE_ID, _external=True) + _expected_redirect=url_for( + 'main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid, _external=True + ) ) else: client_request.post( @@ -2825,5 +2843,7 @@ def test_redirects_to_choose_template_if_no_session_exists_for_upload_id( service_id=SERVICE_ONE_ID, **extra_args, _expected_status=301, - _expected_redirect=url_for('main.choose_template', service_id=SERVICE_ONE_ID, _external=True) + _expected_redirect=url_for( + 'main.choose_template', service_id=SERVICE_ONE_ID, _external=True + ) )