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

Merge pull request #3416 from alphagov/validate-3-lines-csv

Browse Source 
Allow all the new address goodness in spreadsheets
This commit is contained in:
Chris Hill-Scott
2020-05-01 15:37:08 +01:00
committed by GitHub
parent 4892b2db77 06108de0f7
commit 0ef076f59e
11 changed files with 281 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
app/main/forms.py
View File

@@ -781,6 +781,10 @@ class SMSTemplateForm(BaseTemplateForm):
class LetterAddressForm(StripWhitespaceForm):
def __init__(self, *args, allow_international_letters=False, **kwargs):
self.allow_international_letters = allow_international_letters
super().__init__(*args, **kwargs)
address = PostalAddressField(
'Address',
validators=[DataRequired(message="Cannot be empty")]
@@ -788,7 +792,10 @@ class LetterAddressForm(StripWhitespaceForm):
def validate_address(self, field):
address = PostalAddress(field.data)
address = PostalAddress(
field.data,
allow_international_letters=self.allow_international_letters,
)
if not address.has_enough_lines:
raise ValidationError(
@@ -800,7 +807,11 @@ class LetterAddressForm(StripWhitespaceForm):
f'Address must be no more than {PostalAddress.MAX_LINES} lines long'
)
if not address.postcode:
if not address.has_valid_last_line:
if self.allow_international_letters:
raise ValidationError(
f'Last line of the address must be a UK postcode or another country'
)
raise ValidationError(
f'Last line of the address must be a real UK postcode'
)

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

@@ -18,14 +18,12 @@ from notifications_python_client.errors import HTTPError
from notifications_utils import LETTER_MAX_PAGE_COUNT, SMS_CHAR_COUNT_LIMIT
from notifications_utils.columns import Columns
from notifications_utils.pdf import is_letter_too_long
from notifications_utils.postal_address import PostalAddress
from notifications_utils.recipients import (
RecipientCSV,
first_column_headings,
optional_address_columns,
from notifications_utils.postal_address import (
PostalAddress,
address_lines_1_to_6_and_postcode_keys,
)
from notifications_utils.recipients import RecipientCSV, first_column_headings
from notifications_utils.sanitise_text import SanitiseASCII
from orderedset import OrderedSet
from xlrd.biffh import XLRDError
from xlrd.xldate import XLDateError
@@ -64,6 +62,11 @@ from app.utils import (
user_has_permissions,
)
letter_address_columns = [
column.replace('_', ' ')
for column in address_lines_1_to_6_and_postcode_keys
]
def get_example_csv_fields(column_headers, use_example_as_example, submitted_fields):
if use_example_as_example:
@@ -82,7 +85,7 @@ def get_example_csv_rows(template, use_example_as_example=True, submitted_fields
(submitted_fields or {}).get(
key, get_example_letter_address(key) if use_example_as_example else key
)
for key in first_column_headings['letter']
for key in letter_address_columns
]
}[template.template_type] + get_example_csv_fields(
(
@@ -369,7 +372,10 @@ def send_one_off_letter_address(service_id, template_id):
get_normalised_placeholders_from_session()
)
form = LetterAddressForm(address=current_session_address.normalised)
form = LetterAddressForm(
address=current_session_address.normalised,
allow_international_letters=current_service.has_permission('international_letters'),
)
if form.validate_on_submit():
session['placeholders'].update(PostalAddress(form.address.data).as_personalisation)
@@ -381,7 +387,8 @@ def send_one_off_letter_address(service_id, template_id):
if all_placeholders_in_session(placeholders):
return get_notification_check_endpoint(service_id, template)
first_non_address_placeholder_index = len(first_column_headings['letter'])
first_non_address_placeholder_index = len(address_lines_1_to_6_and_postcode_keys)
return redirect(url_for(
'main.send_one_off_step',
service_id=service_id,
@@ -473,7 +480,7 @@ def send_test_step(service_id, template_id, step_index):
# if we're in a letter, we should show address block rather than "address line #" or "postcode"
if template.template_type == 'letter':
if step_index < len(first_column_headings['letter']):
if step_index < len(address_lines_1_to_6_and_postcode_keys):
return redirect(url_for(
'.send_one_off_letter_address',
service_id=service_id,
@@ -663,16 +670,15 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
)
recipients = RecipientCSV(
contents,
template_type=template.template_type,
template=template,
placeholders=template.placeholders,
max_initial_rows_shown=50,
max_errors_shown=50,
whitelist=itertools.chain.from_iterable(
[user.name, user.mobile_number, user.email_address] for user in Users(service_id)
) if current_service.trial_mode else None,
remaining_messages=remaining_messages,
international_sms=current_service.has_permission('international_sms'),
allow_international_sms=current_service.has_permission('international_sms'),
allow_international_letters=current_service.has_permission('international_letters'),
)
if request.args.get('from_test'):
@@ -711,13 +717,15 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
current_service.trial_mode,
template.template_type == 'letter',
)),
required_recipient_columns=OrderedSet(recipients.recipient_column_headers) - optional_address_columns,
first_recipient_column=recipients.recipient_column_headers[0],
preview_row=preview_row,
sent_previously=job_api_client.has_sent_previously(
service_id, template.id, db_template['version'], request.args.get('original_file_name', '')
),
letter_too_long=is_letter_too_long(page_count),
letter_max_pages=LETTER_MAX_PAGE_COUNT,
letter_min_address_lines=PostalAddress.MIN_LINES,
letter_max_address_lines=PostalAddress.MAX_LINES,
page_count=page_count
)
@@ -850,10 +858,12 @@ def go_to_dashboard_after_tour(service_id, example_template_id):
def fields_to_fill_in(template, prefill_current_user=False):
recipient_columns = first_column_headings[template.template_type]
if 'letter' == template.template_type:
return letter_address_columns + list(template.placeholders)
if not prefill_current_user:
return first_column_headings[template.template_type] + list(template.placeholders)
if 'letter' == template.template_type or not prefill_current_user:
return recipient_columns + list(template.placeholders)
if template.template_type == 'sms':
session['recipient'] = current_user.mobile_number
session['placeholders']['phone number'] = current_user.mobile_number
@@ -1131,8 +1141,14 @@ def get_sms_sender_from_session():
def get_spreadsheet_column_headings_from_template(template):
column_headings = []
if template.template_type == 'letter':
# We want to avoid showing `address line 7` for now
recipient_columns = letter_address_columns
else:
recipient_columns = first_column_headings[template.template_type]
for column_heading in (
first_column_headings[template.template_type] + list(template.placeholders)
recipient_columns + list(template.placeholders)
):
if column_heading not in Columns.from_keys(column_headings):
column_headings.append(column_heading)

5
app/main/views/uploads.py
View File

@@ -43,6 +43,7 @@ from app.utils import (
generate_previous_dict,
get_errors_for_csv,
get_letter_validation_error,
get_sample_template,
get_template,
unicode_truncate,
user_has_permissions,
@@ -357,12 +358,12 @@ def check_contact_list(service_id, upload_id):
recipients = RecipientCSV(
contents,
template_type=template_type or 'sms',
template=get_sample_template(template_type or 'sms'),
whitelist=itertools.chain.from_iterable(
[user.name, user.mobile_number, user.email_address]
for user in current_service.active_users
) if current_service.trial_mode else None,
international_sms=current_service.has_permission('international_sms'),
allow_international_sms=current_service.has_permission('international_sms'),
max_initial_rows_shown=50,
max_errors_shown=50,
)

5
app/models/contact_list.py
View File

@@ -14,6 +14,7 @@ from app.s3_client.s3_csv_client import (
s3upload,
set_metadata_on_csv_upload,
)
from app.utils import get_sample_template
class ContactList(JSONModel):
@@ -119,8 +120,8 @@ class ContactList(JSONModel):
def recipients(self):
return RecipientCSV(
self.contents,
template_type=self.template_type,
international_sms=True,
template=get_sample_template(self.template_type),
allow_international_sms=True,
max_initial_rows_shown=50,
)

15
app/templates/views/check/column-errors.html
View File

@@ -58,15 +58,16 @@
<h1 class='banner-title' data-module="track-error" data-error-type="Missing recipient columns" data-error-label="{{ upload_id }}">
There’s a problem with your column names
</h1>
{% if template.template_type == 'letter' %}
<p>
Your file needs {{ (
recipients.missing_column_headers
if template.template_type == 'letter' else required_recipient_columns
) | formatted_list(
prefix='a column called',
prefix_plural='columns called'
) }}.
Your file needs at least 3 address columns, for example ‘address line 1’,
‘address line 2’ and ‘address line 3’.
</p>
{% else %}
<p>
Your file needs a column called ‘{{ first_recipient_column }}’.
</p>
{% endif %}
<p>
Right now it has {{ recipients.column_headers | formatted_list(
prefix='one column, called ',

10
app/templates/views/check/row-errors.html
View File

@@ -80,6 +80,16 @@
No content for this message
{% elif item.message_too_long %}
Message is too long
{% elif not item.as_postal_address.has_enough_lines %}
Address must be at least {{ letter_min_address_lines }} lines long
{% elif item.as_postal_address.has_too_many_lines %}
Address must be no more than {{ letter_max_address_lines }} lines long
{% elif not item.as_postal_address.has_valid_last_line %}
{% if item.as_postal_address.allow_international_letters %}
Last line of the address must be a UK postcode or another country
{% else %}
Last line of the address must be a real UK postcode
{% endif %}
{% endif %}
</span>
{% endcall %}

11
app/utils.py
View File

@@ -151,6 +151,15 @@ def get_errors_for_csv(recipients, template_type):
return errors
def get_sample_template(template_type):
if template_type == 'email':
return EmailPreviewTemplate({'content': 'any', 'subject': '', 'template_type': 'email'})
if template_type == 'sms':
return SMSPreviewTemplate({'content': 'any', 'template_type': 'sms'})
if template_type == 'letter':
return LetterImageTemplate({'content': 'any', 'subject': '', 'template_type': 'letter'})
def generate_notifications_csv(**kwargs):
from app import notification_api_client
from app.s3_client.s3_csv_client import s3download
@@ -161,7 +170,7 @@ def generate_notifications_csv(**kwargs):
original_file_contents = s3download(kwargs['service_id'], kwargs['job_id'])
original_upload = RecipientCSV(
original_file_contents,
template_type=kwargs['template_type'],
template=get_sample_template(kwargs['template_type']),
)
original_column_headers = original_upload.column_headers
fieldnames = ['Row number'] + original_column_headers + ['Template', 'Type', 'Job', 'Status', 'Time']

2
requirements-app.txt
View File

@@ -23,5 +23,5 @@ notifications-python-client==5.5.1
awscli-cwlogs>=1.4,<1.5
itsdangerous==1.1.0
git+https://github.com/alphagov/notifications-utils.git@37.3.0#egg=notifications-utils==37.3.0
git+https://github.com/alphagov/notifications-utils.git@39.0.0#egg=notifications-utils==39.0.0
git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha

2
requirements.txt
View File

@@ -25,7 +25,7 @@ notifications-python-client==5.5.1
awscli-cwlogs>=1.4,<1.5
itsdangerous==1.1.0
git+https://github.com/alphagov/notifications-utils.git@37.3.0#egg=notifications-utils==37.3.0
git+https://github.com/alphagov/notifications-utils.git@39.0.0#egg=notifications-utils==39.0.0
git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha
## The following requirements were added by pip freeze:

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

@@ -19,6 +19,7 @@ from notifications_utils.recipients import RecipientCSV
from notifications_utils.template import (
LetterImageTemplate,
LetterPreviewTemplate,
SMSPreviewTemplate,
)
from xlrd.biffh import XLRDError
from xlrd.xldate import (
@@ -309,6 +310,37 @@ def test_example_spreadsheet(
)
def test_example_spreadsheet_for_letters(
client_request,
mocker,
mock_get_service_letter_template_with_placeholders,
fake_uuid,
):
mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1)
page = client_request.get(
'.send_messages',
service_id=SERVICE_ONE_ID,
template_id=fake_uuid
)
assert list(zip(*[
[normalize_spaces(cell.text) for cell in page.select('tbody tr')[row].select('td')]
for row in (0, 1)
])) == [
('1', '2'),
('address line 1', 'A. Name'),
('address line 2', '123 Example Street'),
('address line 3', ''),
('address line 4', ''),
('address line 5', ''),
('address line 6', ''),
('postcode', 'XM4 5HQ'),
('name', 'example'),
('date', 'example'),
]
@pytest.mark.parametrize(
"filename, acceptable_file",
list(zip(test_spreadsheet_files, repeat(True))) +
@@ -556,6 +588,116 @@ def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors
assert page.select('tbody tr td')[1]['colspan'] == '2'
def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_errors(
logged_in_client,
service_one,
mocker,
mock_get_service_letter_template,
mock_s3_upload,
mock_get_users_by_service,
mock_get_service_statistics,
mock_get_job_doesnt_exist,
mock_get_jobs,
fake_uuid,
):
mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9)
mocker.patch(
'app.main.views.send.s3download',
return_value='''
address line 1, address line 3, address line 6,
Firstname Lastname, 123 Example St., SW1A 1AA
Firstname Lastname, 123 Example St., SW!A !AA
Firstname Lastname, 123 Example St., France
, 123 Example St., SW!A !AA
"1\n2\n3\n4\n5\n6\n7\n8"
'''
)
response = logged_in_client.post(
url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid),
data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
content_type='multipart/form-data',
follow_redirects=True
)
assert response.status_code == 200
page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
assert normalize_spaces(
page.select_one('.banner-dangerous').text
) == (
'There’s a problem with invalid.csv '
'You need to fix 4 addresses. '
'Skip to file contents'
)
assert [
normalize_spaces(row.text) for row in page.select('tbody tr')
] == [
'3 Last line of the address must be a real UK postcode',
'Firstname Lastname 123 Example St. SW!A !AA',
'4 Last line of the address must be a real UK postcode',
'Firstname Lastname 123 Example St. France',
'5 Address must be at least 3 lines long',
'123 Example St. SW!A !AA',
'6 Address must be no more than 7 lines long',
'1 2 3 4 5 6 7 8',
]
def test_upload_csv_file_with_international_letters_permission_shows_appropriate_errors(
logged_in_client,
service_one,
mocker,
mock_get_service_letter_template,
mock_s3_upload,
mock_get_users_by_service,
mock_get_service_statistics,
mock_get_job_doesnt_exist,
mock_get_jobs,
fake_uuid,
):
service_one['permissions'] += ['international_letters']
mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9)
mocker.patch(
'app.main.views.send.s3download',
return_value='''
address line 1, address line 3, address line 6,
Firstname Lastname, 123 Example St., SW1A 1AA
Firstname Lastname, 123 Example St., France
Firstname Lastname, 123 Example St., SW!A !AA
Firstname Lastname, 123 Example St., Not France
'''
)
response = logged_in_client.post(
url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid),
data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
content_type='multipart/form-data',
follow_redirects=True
)
assert response.status_code == 200
page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
assert normalize_spaces(
page.select_one('.banner-dangerous').text
) == (
'There’s a problem with invalid.csv '
'You need to fix 2 addresses. '
'Skip to file contents'
)
assert [
normalize_spaces(row.text) for row in page.select('tbody tr')
] == [
'4 Last line of the address must be a UK postcode or another country',
'Firstname Lastname 123 Example St. SW!A !AA',
'5 Last line of the address must be a UK postcode or another country',
'Firstname Lastname 123 Example St. Not France',
]
@pytest.mark.parametrize('file_contents, expected_error,', [
(
"""
@@ -2420,20 +2562,45 @@ def test_send_one_off_letter_address_populates_address_fields_in_session(
assert session['placeholders'] == expected_placeholders
@pytest.mark.parametrize(['form_data', 'expected_error_message'], [
('', 'Cannot be empty'),
('a\n\n\n\nb', 'Address must be at least 3 lines long'),
('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']), 'Address must be no more than 7 lines long'),
('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']), 'Last line of the address must be a real UK postcode'),
@pytest.mark.parametrize('form_data, extra_permissions, expected_error_message', [
(
'',
[],
'Cannot be empty'
),
(
'a\n\n\n\nb',
[],
'Address must be at least 3 lines long',
),
(
'\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']),
[],
'Address must be no more than 7 lines long'
),
(
'\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']),
[],
'Last line of the address must be a real UK postcode',
),
(
'\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']),
['international_letters'],
'Last line of the address must be a UK postcode or another country',
),
])
def test_send_one_off_letter_address_rejects_bad_addresses(
client_request,
service_one,
fake_uuid,
mock_get_service_letter_template,
mock_template_preview,
form_data,
extra_permissions,
expected_error_message
):
service_one['permissions'] += extra_permissions
with client_request.session_transaction() as session:
session['recipient'] = None
session['placeholders'] = {}
@@ -2575,7 +2742,9 @@ def test_upload_csvfile_with_international_validates(
mocker.patch('app.main.views.send.s3download', return_value='')
mock_recipients = mocker.patch(
'app.main.views.send.RecipientCSV',
return_value=RecipientCSV("", template_type="sms"),
return_value=RecipientCSV("", template=SMSPreviewTemplate(
{'content': 'foo', 'template_type': 'sms'}
)),
)
response = logged_in_client.post(
@@ -2586,7 +2755,7 @@ def test_upload_csvfile_with_international_validates(
)
assert response.status_code == 200
assert mock_recipients.call_args[1]['international_sms'] == should_allow_international
assert mock_recipients.call_args[1]['allow_international_sms'] == should_allow_international
def test_job_from_contact_list_knows_where_its_come_from(
@@ -3381,7 +3550,7 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters(
assert normalize_spaces(page.select_one('.banner-dangerous').text) == (
'There’s a problem with example.xlsx '
'You need to enter missing data in 2 rows. '
'You need to fix 2 addresses. '
'Skip to file contents'
)
assert not page.select('.table-field-index a')
@@ -3470,7 +3639,8 @@ def test_check_messages_column_error_doesnt_show_optional_columns(
assert normalize_spaces(page.select_one('.banner-dangerous').text) == (
'There’s a problem with your column names '
'Your file needs a column called ‘postcode’. '
'Your file needs at least 3 address columns, for example ‘address line 1’, '
'‘address line 2’ and ‘address line 3’. '
'Right now it has columns called ‘address_line_1’, ‘address_line_2’ and ‘foo’. '
'Skip to file contents'
)

19
tests/conftest.py
View File

@@ -878,6 +878,25 @@ def mock_get_service_letter_template(mocker):
)
@pytest.fixture(scope='function')
def mock_get_service_letter_template_with_placeholders(mocker):
def _get(service_id, template_id, version=None, postage='second'):
template = template_json(
service_id,
template_id,
name="Two week reminder",
type_="letter",
content="Hello ((name)) your thing is due on ((date))",
subject="Subject",
postage=postage,
)
return {'data': template}
return mocker.patch(
'app.service_api_client.get_service_template', side_effect=_get
)
@pytest.fixture(scope='function')
def mock_create_service_template(mocker, fake_uuid):
def _create(name, type_, content, service, subject=None, process_type=None, parent_folder_id=None):