darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-02-05 10:53:28 -05:00
Code Issues Packages Projects Releases Wiki Activity

Give the user better error messages for CSV files

Browse Source 
Makes uses of the additions to utils in https://github.com/alphagov/notifications-utils/pull/9

This commit strips out a lot of the complex stuff that the views and templates
in this app were doing. There is now a cleaner separation of concerns:

- utils returns the number and type of errors in the csv
- `get_errors_for_csv` helper in this app maps the number and type of errors
  onto human-friendly error messages
- the view and template just doing the glueing-together of all the pieces

This is (hopefully) easier to understand, definitely makes the component
parts easier to test in isolation, and makes it easier to give more specific
error messages.
This commit is contained in:
Chris Hill-Scott
2016-03-07 18:47:05 +00:00
parent 0a5bf0bc44
commit eb3734f1d1
14 changed files with 340 additions and 255 deletions
Download Patch File Download Diff File Expand all files Collapse all files

79
tests/app/main/test_errors_for_csv.py Normal file
View File

@@ -0,0 +1,79 @@
from collections import namedtuple
import pytest
from app.utils import get_errors_for_csv
MockRecipients = namedtuple(
'RecipientCSV',
[
'missing_column_headers',
'rows_with_bad_recipients',
'rows_with_missing_data'
]
)
@pytest.mark.parametrize(
"missing_column_headers,rows_with_bad_recipients,rows_with_missing_data,template_type,expected_errors",
[
(
[], [], [],
'sms',
[]
),
(
[], {2}, [],
'sms',
['fix 1 phone number']
),
(
[], {2, 4, 6}, [],
'sms',
['fix 3 phone numbers']
),
(
[], {1}, [],
'email',
['fix 1 email address']
),
(
[], {2, 4, 6}, [],
'email',
['fix 3 email addresses']
),
(
['name'], {2}, {3},
'sms',
[
'add a column called ‘name’',
'fix 1 phone number',
'fill in 1 empty cell'
]
),
(
['name', 'date'], [], [],
'sms',
['add 2 columns, ‘name’ and ‘date’']
),
(
['name', 'date', 'time'], {2, 4, 6, 8}, {3, 6, 9, 12},
'sms',
[
'add columns called ‘name’, ‘date’, and ‘time’',
'fix 4 phone numbers',
'fill in 4 empty cells'
]
)
]
)
def test_get_errors_for_csv(
missing_column_headers, rows_with_bad_recipients, rows_with_missing_data,
template_type,
expected_errors
):
assert get_errors_for_csv(
MockRecipients(missing_column_headers, rows_with_bad_recipients, rows_with_missing_data),
template_type
) == expected_errors

10
tests/app/main/test_utils.py → tests/app/main/test_permissions.py
View File

@@ -1,7 +1,7 @@
import pytest
from flask import url_for
from app.utils import user_has_permissions, validate_header_row, validate_recipient, InvalidHeaderError
from app.utils import user_has_permissions
from app.main.views.index import index
from werkzeug.exceptions import Forbidden
@@ -68,11 +68,3 @@ def test_exact_permissions(app_,
decorator = user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
decorated_index = decorator(index)
response = decorated_index()
def test_validate_header_row():
row = {'bad': '+44 7700 900981'}
try:
validate_header_row(row, 'sms')
except InvalidHeaderError as e:
assert e.message == 'Invalid header name, should be phone number'

2
tests/app/main/views/test_register.py
View File

@@ -55,7 +55,7 @@ def test_process_register_returns_400_when_mobile_number_is_invalid(app_,
'password': 'validPassword!'})
assert response.status_code == 200
assert 'Must be a UK mobile number (eg 07700 900460)' in response.get_data(as_text=True)
assert 'Must not contain letters or symbols' in response.get_data(as_text=True)
def test_should_return_400_when_email_is_not_gov_uk(app_,

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

@@ -33,33 +33,7 @@ def test_choose_template(
assert '{} template two content'.format(template_type) in content
def test_upload_empty_csvfile_returns_to_upload_page(
app_,
api_user_active,
mock_login,
mock_get_user,
mock_get_service,
mock_get_service_templates,
mock_check_verify_code,
mock_get_service_template,
mock_has_permissions
):
with app_.test_request_context():
with app_.test_client() as client:
client.login(api_user_active)
upload_data = {'file': (BytesIO(''.encode('utf-8')), 'emtpy.csv')}
response = client.post(
url_for('main.send_messages', service_id=12345, template_id=54321),
data=upload_data,
follow_redirects=True
)
assert response.status_code == 200
content = response.get_data(as_text=True)
assert 'The file emtpy.csv contained no data' in content
def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(
def test_upload_csvfile_with_errors_shows_check_page_with_errors(
app_,
api_user_active,
mocker,
@@ -85,43 +59,11 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(
)
assert response.status_code == 200
content = response.get_data(as_text=True)
assert 'Your CSV file contained missing or invalid data' in content
assert 'There was a problem with invalid.csv' in content
assert '+44 123' in content
assert '+44 456' in content
assert 'Upload a CSV file' in content
def test_upload_csvfile_removes_empty_lines_and_trailing_commas(
app_,
api_user_active,
mocker,
mock_login,
mock_get_service,
mock_get_service_template,
mock_s3_upload,
mock_has_permissions
):
contents = 'phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n'
file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv')
expected_data = {'data': ['phone number,name', '++44 7700 900981,test1', '+44 7700 900981,test2'],
'file_name': 'invalid.csv'}
mocker.patch('app.main.views.send.s3download',
return_value='phone number,name\n++44 7700 900981,test1\n+44 7700 900981,test2')
with app_.test_request_context():
with app_.test_client() as client:
client.login(api_user_active)
upload_data = {'file': file_data}
response = client.post(
url_for('main.send_messages', service_id=12345, template_id=54321),
data=upload_data,
follow_redirects=True
)
assert response.status_code == 200
mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1')
assert 'Not a UK mobile number' in content
assert 'Re-upload your file' in content
def test_send_test_message_to_self(
@@ -160,7 +102,7 @@ def test_send_test_message_to_self(
mock_has_permissions
):
expected_data = {'data': ['email address', 'test@user.gov.uk'], 'file_name': 'Test run'}
expected_data = {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Test run'}
mocker.patch('app.main.views.send.s3download', return_value='email address\r\ntest@user.gov.uk')
with app_.test_request_context():
@@ -207,31 +149,32 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(
mock_has_permissions
):
contents = 'phone number\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986' # noqa
file_data = (BytesIO(contents.encode('utf-8')), 'valid.csv')
mocker.patch('app.main.views.send.s3download', return_value=contents)
mocker.patch(
'app.main.views.send.s3download',
return_value='phone number\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986' # noqa
)
with app_.test_request_context():
with app_.test_client() as client:
client.login(api_user_active)
upload_data = {'file': file_data}
response = client.post(url_for('main.send_messages', service_id=12345, template_id=54321),
data=upload_data,
follow_redirects=True)
response = client.post(
url_for('main.send_messages', service_id=12345, template_id=54321),
data={'file': (BytesIO(), 'valid.csv')},
follow_redirects=True
)
with client.session_transaction() as sess:
assert int(sess['upload_data']['template_id']) == 54321
assert sess['upload_data']['original_file_name'] == 'valid.csv'
assert sess['upload_data']['notification_count'] == 6
content = response.get_data(as_text=True)
assert response.status_code == 200
assert '+44 7700 900981' in content
assert '+44 7700 900982' in content
assert '+44 7700 900983' in content
assert '+44 7700 900984' in content
assert '+44 7700 900985' in content
assert '+44 7700 900986' in content
assert '1 more row not shown' in content
def test_create_job_should_call_api(
@@ -260,8 +203,9 @@ def test_create_job_should_call_api(
with client.session_transaction() as session:
session['upload_data'] = {'original_file_name': original_file_name,
'template_id': template_id,
'notification_count': notification_count}
url = url_for('main.check_messages', service_id=service_one['id'], upload_id=job_id)
'notification_count': notification_count,
'valid': True}
url = url_for('main.start_job', service_id=service_one['id'], upload_id=job_id)
response = client.post(url, data=job_data, follow_redirects=True)
assert response.status_code == 200
@@ -284,11 +228,11 @@ def test_check_messages_should_revalidate_file_when_uploading_file(
):
service_id = service_one['id']
contents = 'phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n'
file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv')
upload_data = {'file': file_data}
mocker.patch('app.main.views.send.s3download', return_value=contents)
mocker.patch(
'app.main.views.send.s3download',
return_value='phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n'
)
with app_.test_request_context():
with app_.test_client() as client:
client.login(api_user_active)
@@ -296,7 +240,10 @@ def test_check_messages_should_revalidate_file_when_uploading_file(
session['upload_data'] = {'original_file_name': 'invalid.csv',
'template_id': job_data['template'],
'notification_count': job_data['notification_count']}
url = url_for('main.check_messages', service_id=service_id, upload_id=job_data['id'])
response = client.post(url, data=upload_data, follow_redirects=True)
response = client.post(
url_for('main.check_messages', service_id=service_id, upload_id=job_data['id']),
data={'file': (BytesIO(), 'invalid.csv')},
follow_redirects=True
)
assert response.status_code == 200
assert 'Your CSV file contained missing or invalid data' in response.get_data(as_text=True)
assert 'There was a problem with invalid.csv' in response.get_data(as_text=True)