mirror of
https://github.com/GSA/notifications-admin.git
synced 2026-08-24 08:16:51 -04:00
Remove ‘check’ page’s reliance on session
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.
This commit is contained in:
@@ -20,7 +20,6 @@ from notifications_utils.recipients import (
|
|||||||
optional_address_columns,
|
optional_address_columns,
|
||||||
)
|
)
|
||||||
from orderedset import OrderedSet
|
from orderedset import OrderedSet
|
||||||
from werkzeug.routing import RequestRedirect
|
|
||||||
from xlrd.biffh import XLRDError
|
from xlrd.biffh import XLRDError
|
||||||
from xlrd.xldate import XLDateError
|
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,
|
Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict,
|
||||||
current_app.config['AWS_REGION']
|
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(
|
return redirect(url_for(
|
||||||
'.check_messages',
|
'.check_messages',
|
||||||
service_id=service_id,
|
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):
|
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)
|
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']
|
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
|
email_reply_to = None
|
||||||
sms_sender = None
|
sms_sender = None
|
||||||
|
|
||||||
if db_template['template_type'] == 'email':
|
if db_template['template_type'] == 'email':
|
||||||
email_reply_to = get_email_reply_to_address_from_session(service_id)
|
email_reply_to = get_email_reply_to_address_from_session(service_id)
|
||||||
elif db_template['template_type'] == 'sms':
|
elif db_template['template_type'] == 'sms':
|
||||||
sms_sender = get_sms_sender_from_session(service_id)
|
sms_sender = get_sms_sender_from_session(service_id)
|
||||||
|
|
||||||
template = get_template(
|
template = get_template(
|
||||||
service_api_client.get_service_template(
|
service_api_client.get_service_template(
|
||||||
service_id,
|
service_id,
|
||||||
@@ -552,6 +532,10 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
|
|||||||
elif preview_row > 2:
|
elif preview_row > 2:
|
||||||
abort(404)
|
abort(404)
|
||||||
|
|
||||||
|
if 'file_uploads' not in session:
|
||||||
|
session['file_uploads'] = {}
|
||||||
|
session['file_uploads'][upload_id] = {}
|
||||||
|
|
||||||
if any(recipients) and not recipients.has_errors:
|
if any(recipients) and not recipients.has_errors:
|
||||||
session['file_uploads'][upload_id]['notification_count'] = len(recipients)
|
session['file_uploads'][upload_id]['notification_count'] = len(recipients)
|
||||||
session['file_uploads'][upload_id]['valid'] = True
|
session['file_uploads'][upload_id]['valid'] = True
|
||||||
@@ -719,12 +703,6 @@ def make_and_upload_csv_file(service_id, template):
|
|||||||
).as_dict,
|
).as_dict,
|
||||||
current_app.config['AWS_REGION'],
|
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(
|
return redirect(url_for(
|
||||||
'.check_messages',
|
'.check_messages',
|
||||||
upload_id=upload_id,
|
upload_id=upload_id,
|
||||||
|
|||||||
@@ -1443,9 +1443,10 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(
|
|||||||
follow_redirects=True
|
follow_redirects=True
|
||||||
)
|
)
|
||||||
with logged_in_client.session_transaction() as sess:
|
with logged_in_client.session_transaction() as sess:
|
||||||
assert sess['file_uploads'][fake_uuid]['template_id'] == fake_uuid
|
assert 'template_id' not in sess['file_uploads'][fake_uuid]
|
||||||
assert 'original_file_name' not in sess['file_uploads']
|
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]['notification_count'] == 53
|
||||||
|
assert sess['file_uploads'][fake_uuid]['valid'] is True
|
||||||
|
|
||||||
content = response.get_data(as_text=True)
|
content = response.get_data(as_text=True)
|
||||||
assert response.status_code == 200
|
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 я'
|
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', [
|
@pytest.mark.parametrize('existing_session_items', [
|
||||||
{},
|
{},
|
||||||
{'recipient': '07700900001'},
|
{'recipient': '07700900001'},
|
||||||
@@ -2782,23 +2763,6 @@ def test_sms_sender_is_previewed(
|
|||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize('endpoint, request_type, extra_args', [
|
@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',
|
'main.start_job',
|
||||||
'POST',
|
'POST',
|
||||||
|
|||||||
Reference in New Issue
Block a user