darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-08-11 17:38:50 -04:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2001 from alphagov/store-less-in-session

Browse Source 
Stop storing `template_id` and `original_file_name` in session
This commit is contained in:
Rebecca Law
2018-04-04 14:42:38 +01:00
committed by GitHub
parent c4fde48c53 ba9935c49e
commit fc846cf4a2
4 changed files with 93 additions and 115 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

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

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>

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