diff --git a/app/main/views/send.py b/app/main/views/send.py index 21fb7bf32..84100af2d 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -21,7 +21,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 @@ -135,18 +134,13 @@ 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, - 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 @@ -480,16 +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): - - 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.choose_template', service_id=service_id)) +def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_pdf=False): users = user_api_client.get_users_for_service(service_id=service_id) @@ -497,29 +482,35 @@ 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, @@ -550,7 +541,9 @@ 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 '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) @@ -565,7 +558,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, @@ -582,13 +575,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 @@ -611,16 +605,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) @@ -645,7 +645,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', '') ) @@ -712,12 +712,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/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 }}
- @@ -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..0c6795a88 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, @@ -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 sess['file_uploads'][fake_uuid]['original_file_name'] == 'valid.csv' + 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 @@ -1517,7 +1518,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 +1553,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 +1584,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 +1697,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 +1724,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 +1939,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 +2022,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 +2031,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 +2070,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 +2123,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 +2166,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 +2205,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 +2243,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 +2282,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 +2322,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,18 +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(logged_in_client, service_one): - response = logged_in_client.get(url_for( - '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) - - @pytest.mark.parametrize('existing_session_items', [ {}, {'recipient': '07700900001'}, @@ -2660,8 +2657,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 +2690,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 +2709,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', [ @@ -2767,23 +2763,6 @@ def test_sms_sender_is_previewed( @pytest.mark.parametrize('endpoint, request_type, extra_args', [ - ( - 'main.check_messages', - 'GET', - { - 'template_type': 'email', - 'upload_id': fake_uuid(), - } - ), - ( - 'main.check_messages_preview', - 'GET', - { - 'template_type': 'email', - 'upload_id': fake_uuid(), - 'filetype': 'png' - } - ), ( 'main.start_job', 'POST', @@ -2802,6 +2781,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 +2797,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 +2807,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 + ) )