darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-06-19 12:46:20 -04:00
Code Issues Packages Projects Releases Wiki Activity

Store template ID and original file name in URL

Browse Source 
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.
This commit is contained in:
Chris Hill-Scott
2018-03-29 17:01:35 +01:00
parent 2e795be2a7
commit 3dab34cd38
4 changed files with 110 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

62
app/main/views/send.py
View File

@@ -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/<service_id>/<template_type>/check/<upload_id>", methods=['GET'])
@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", methods=['GET'])
@main.route("/services/<service_id>/<uuid:template_id>/check/<upload_id>", methods=['GET'])
@main.route("/services/<service_id>/<uuid:template_id>/check/<upload_id>/row-<int:row_index>", 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/<service_id>/<template_type>/check/<upload_id>.<filetype>", methods=['GET'])
@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>.<filetype>", methods=['GET'])
@main.route(
"/services/<service_id>/<uuid:template_id>/check/<upload_id>.<filetype>",
methods=['GET'],
)
@main.route(
"/services/<service_id>/<uuid:template_id>/check/<upload_id>/row-<int:row_index>.<filetype>",
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', '')
)

6
app/templates/views/check/ok.html
View File

@@ -27,7 +27,7 @@
{{ template|string }}
<div class="bottom-gutter-3-2">
<form method="post" enctype="multipart/form-data" action="{{url_for('main.start_job', service_id=current_service.id, upload_id=upload_id)}}" class='page-footer'>
<form method="post" enctype="multipart/form-data" action="{{url_for('main.start_job', service_id=current_service.id, upload_id=upload_id, original_file_name=original_file_name)}}" class='page-footer'>
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
<input type="hidden" name="help" value="{{ '3' if help else 0 }}" />
{% if choose_time_form and template.template_type != 'letter' %}
@@ -39,7 +39,7 @@
{% if template.template_type != 'letter' or not request.args.from_test %}
<button type="submit" class="button">Send {{ count_of_recipients }} {{ message_count_label(count_of_recipients, template.template_type, suffix='') }}</button>
{% else %}
<a href="{{ url_for('main.check_messages_preview', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, filetype='pdf') }}" download class="button">Download as a printable PDF</a>
<a href="{{ url_for('main.check_messages_preview', service_id=current_service.id, template_id=template.id, upload_id=upload_id, filetype='pdf') }}" download class="button">Download as a printable PDF</a>
{% endif %}
<a href="{{ back_link }}" class="page-footer-back-link">Back</a>
</form>
@@ -63,7 +63,7 @@
{% if (item.index + 2) == preview_row %}
{{ item.index + 2 }}
{% else %}
<a href="{{ url_for('.check_messages', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, row_index=(item.index + 2)) }}">{{ item.index + 2 }}</a>
<a href="{{ url_for('.check_messages', service_id=current_service.id, template_id=template.id, upload_id=upload_id, row_index=(item.index + 2)) }}">{{ item.index + 2 }}</a>
{% endif %}
</span>
{% endcall %}

2
app/templates/views/notifications/check.html
View File

@@ -53,7 +53,7 @@
{% if template.template_type != 'letter' or not request.args.from_test %}
<button type="submit" class="button">Send 1 {{ message_count_label(1, template.template_type, suffix='') }}</button>
{% else %}
<a href="{{ url_for('main.check_messages_preview', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, filetype='pdf') }}" download class="button">Download as a printable PDF</a>
<a href="{{ url_for('main.check_messages_preview', service_id=current_service.id, template_id=template.id, upload_id=upload_id, filetype='pdf') }}" download class="button">Download as a printable PDF</a>
{% endif %}
{% endif %}
<a href="{{ back_link }}" class="page-footer-back-link">Back</a>

114
tests/app/main/views/test_send.py
View File

@@ -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
)
)