mirror of
https://github.com/GSA/notifications-admin.git
synced 2026-09-10 18:22:37 -04:00
Merge pull request #3193 from alphagov/validate-against-empty-messages
Validate CSVs against rows with empty messages
This commit is contained in:
Pea M. Tyczynska
@@ -543,6 +543,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
|
|||||||
recipients = RecipientCSV(
|
recipients = RecipientCSV(
|
||||||
contents,
|
contents,
|
||||||
template_type=template.template_type,
|
template_type=template.template_type,
|
||||||
|
template=template,
|
||||||
placeholders=template.placeholders,
|
placeholders=template.placeholders,
|
||||||
max_initial_rows_shown=50,
|
max_initial_rows_shown=50,
|
||||||
max_errors_shown=50,
|
max_errors_shown=50,
|
||||||
|
|||||||
@@ -0,0 +1,109 @@
|
|||||||
|
{% block top_of_page %}{% endblock %}
|
||||||
|
<!DOCTYPE html>
|
||||||
|
<!--[if lt IE 9]><html class="lte-ie8" lang="{{ html_lang|default('en') }}"><![endif]-->
|
||||||
|
<!--[if gt IE 8]><!--><html lang="{{ html_lang|default('en') }}"><!--<![endif]-->
|
||||||
|
<head>
|
||||||
|
<meta charset="utf-8" />
|
||||||
|
<title>{% block page_title %}GOV.UK - The best place to find government services and information{% endblock %}</title>
|
||||||
|
|
||||||
|
<!--[if gt IE 8]><!--><link rel="stylesheet" media="screen" href="{{ asset_path }}stylesheets/govuk-template.css?0.24.1"/><!--<![endif]-->
|
||||||
|
<!--[if IE 6]><link rel="stylesheet" media="screen" href="{{ asset_path }}stylesheets/govuk-template-ie6.css?0.24.1"/><![endif]-->
|
||||||
|
<!--[if IE 7]><link rel="stylesheet" media="screen" href="{{ asset_path }}stylesheets/govuk-template-ie7.css?0.24.1"/><![endif]-->
|
||||||
|
<!--[if IE 8]><link rel="stylesheet" media="screen" href="{{ asset_path }}stylesheets/govuk-template-ie8.css?0.24.1"/><![endif]-->
|
||||||
|
<link rel="stylesheet" media="print" href="{{ asset_path }}stylesheets/govuk-template-print.css?0.24.1"/>
|
||||||
|
|
||||||
|
<link rel="stylesheet" media="all" href="{{ asset_path }}stylesheets/fonts.css?0.24.1"/>
|
||||||
|
<!--[if lt IE 9]><script src="{{ asset_path }}javascripts/ie.js?0.24.1"></script><![endif]-->
|
||||||
|
|
||||||
|
<link rel="shortcut icon" href="{{ asset_path }}images/favicon.ico?0.24.1" type="image/x-icon" />
|
||||||
|
|
||||||
|
<link rel="mask-icon" href="{{ asset_path }}images/gov.uk_logotype_crown.svg?0.24.1" color="#0b0c0c">
|
||||||
|
<link rel="apple-touch-icon" sizes="180x180" href="{{ asset_path }}images/apple-touch-icon-180x180.png?0.24.1">
|
||||||
|
<link rel="apple-touch-icon" sizes="167x167" href="{{ asset_path }}images/apple-touch-icon-167x167.png?0.24.1">
|
||||||
|
<link rel="apple-touch-icon" sizes="152x152" href="{{ asset_path }}images/apple-touch-icon-152x152.png?0.24.1">
|
||||||
|
<link rel="apple-touch-icon" href="{{ asset_path }}images/apple-touch-icon.png?0.24.1">
|
||||||
|
|
||||||
|
|
||||||
|
<meta name="theme-color" content="#0b0c0c" />
|
||||||
|
|
||||||
|
<meta name="viewport" content="width=device-width, initial-scale=1">
|
||||||
|
|
||||||
|
{% block head %}{% endblock %}
|
||||||
|
|
||||||
|
|
||||||
|
<meta property="og:image" content="{{ asset_path }}images/opengraph-image.png?0.24.1">
|
||||||
|
</head>
|
||||||
|
|
||||||
|
<body class="{% block body_classes %}{% endblock %}">
|
||||||
|
<script>document.body.className = ((document.body.className) ? document.body.className + ' js-enabled' : 'js-enabled');</script>
|
||||||
|
|
||||||
|
{% block body_start %}{% endblock %}
|
||||||
|
|
||||||
|
<div id="skiplink-container">
|
||||||
|
<div>
|
||||||
|
<a href="#content" class="skiplink">{{ skip_link_message|default('Skip to main content') }}</a>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
|
||||||
|
<div id="global-cookie-message">
|
||||||
|
|
||||||
|
{% block cookie_message %}{% endblock %}
|
||||||
|
|
||||||
|
</div>
|
||||||
|
|
||||||
|
|
||||||
|
<header role="banner" id="global-header" class="{% block header_class %}{% endblock %}">
|
||||||
|
<div class="header-wrapper">
|
||||||
|
<div class="header-global">
|
||||||
|
<div class="header-logo">
|
||||||
|
<a href="{{ homepage_url|default('https://www.gov.uk') }}" title="{{ logo_link_title|default('Go to the GOV.UK homepage') }}" id="logo" class="content">
|
||||||
|
<img src="{{ asset_path }}images/gov.uk_logotype_crown_invert_trans.png?0.24.1" width="36" height="32" alt=""> {{ global_header_text|default('GOV.UK') }}
|
||||||
|
</a>
|
||||||
|
</div>
|
||||||
|
{% block inside_header %}{% endblock %}
|
||||||
|
</div>
|
||||||
|
{% block proposition_header %}{% endblock %}
|
||||||
|
</div>
|
||||||
|
</header>
|
||||||
|
|
||||||
|
|
||||||
|
{% block after_header %}{% endblock %}
|
||||||
|
|
||||||
|
<div id="global-header-bar"></div>
|
||||||
|
|
||||||
|
{% block content %}{% endblock %}
|
||||||
|
|
||||||
|
<footer class="group js-footer" id="footer" role="contentinfo">
|
||||||
|
|
||||||
|
<div class="footer-wrapper">
|
||||||
|
{% block footer_top %}{% endblock %}
|
||||||
|
|
||||||
|
<div class="footer-meta">
|
||||||
|
<div class="footer-meta-inner">
|
||||||
|
{% block footer_support_links %}{% endblock %}
|
||||||
|
|
||||||
|
<div class="open-government-licence">
|
||||||
|
<p class="logo"><a href="https://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/" rel="license">Open Government Licence</a></p>
|
||||||
|
|
||||||
|
{% block licence_message %}<p>All content is available under the <a href="https://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/" rel="license">Open Government Licence v3.0</a>, except where otherwise stated</p>{% endblock %}
|
||||||
|
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
|
||||||
|
<div class="copyright">
|
||||||
|
<a href="https://www.nationalarchives.gov.uk/information-management/re-using-public-sector-information/uk-government-licensing-framework/crown-copyright/">{{ crown_copyright_message|default('© Crown copyright')|safe }}</a>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
</footer>
|
||||||
|
|
||||||
|
<div id="global-app-error" class="app-error hidden"></div>
|
||||||
|
|
||||||
|
<script src="{{ asset_path }}javascripts/govuk-template.js?0.24.1"></script>
|
||||||
|
|
||||||
|
{% block body_end %}{% endblock %}
|
||||||
|
|
||||||
|
|
||||||
|
<script>if (typeof window.GOVUK === 'undefined') document.body.className = document.body.className.replace('js-enabled', '');</script>
|
||||||
|
</body>
|
||||||
|
</html>
|
||||||
@@ -25,10 +25,10 @@
|
|||||||
{% call banner_wrapper(type='dangerous') %}
|
{% call banner_wrapper(type='dangerous') %}
|
||||||
{% if row_errors|length == 1 %}
|
{% if row_errors|length == 1 %}
|
||||||
<h1 class='banner-title' data-module="track-error" data-error-type="Bad rows" data-error-label="{{ upload_id }}">
|
<h1 class='banner-title' data-module="track-error" data-error-type="Bad rows" data-error-label="{{ upload_id }}">
|
||||||
There is a problem with {{ original_file_name }}
|
There’s a problem with {{ original_file_name }}
|
||||||
</h1>
|
</h1>
|
||||||
<p>
|
<p>
|
||||||
You need to {{ row_errors[0] }}
|
You need to {{ row_errors[0] }}.
|
||||||
</p>
|
</p>
|
||||||
{% else %}
|
{% else %}
|
||||||
<h1 class='banner-title' data-module="track-error" data-error-type="Bad rows" data-error-label="{{ upload_id }}">
|
<h1 class='banner-title' data-module="track-error" data-error-type="Bad rows" data-error-label="{{ upload_id }}">
|
||||||
|
|||||||
@@ -131,6 +131,22 @@ def get_errors_for_csv(recipients, template_type):
|
|||||||
else:
|
else:
|
||||||
errors.append("enter missing data in {} rows".format(number_of_rows_with_missing_data))
|
errors.append("enter missing data in {} rows".format(number_of_rows_with_missing_data))
|
||||||
|
|
||||||
|
if any(recipients.rows_with_message_too_long):
|
||||||
|
number_of_rows_with_message_too_long = len(list(recipients.rows_with_message_too_long))
|
||||||
|
if 1 == number_of_rows_with_message_too_long:
|
||||||
|
errors.append("shorten the message in 1 row")
|
||||||
|
else:
|
||||||
|
errors.append("shorten the messages in {} rows".format(number_of_rows_with_message_too_long))
|
||||||
|
|
||||||
|
if any(recipients.rows_with_empty_message):
|
||||||
|
number_of_rows_with_empty_message = len(list(recipients.rows_with_empty_message))
|
||||||
|
if 1 == number_of_rows_with_empty_message:
|
||||||
|
errors.append("check you have content for the empty message in 1 row")
|
||||||
|
else:
|
||||||
|
errors.append("check you have content for the empty messages in {} rows".format(
|
||||||
|
number_of_rows_with_empty_message
|
||||||
|
))
|
||||||
|
|
||||||
return errors
|
return errors
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -8,41 +8,44 @@ MockRecipients = namedtuple(
|
|||||||
'RecipientCSV',
|
'RecipientCSV',
|
||||||
[
|
[
|
||||||
'rows_with_bad_recipients',
|
'rows_with_bad_recipients',
|
||||||
'rows_with_missing_data'
|
'rows_with_missing_data',
|
||||||
|
'rows_with_message_too_long',
|
||||||
|
'rows_with_empty_message'
|
||||||
]
|
]
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize(
|
@pytest.mark.parametrize(
|
||||||
"rows_with_bad_recipients,rows_with_missing_data,template_type,expected_errors",
|
"rows_with_bad_recipients, rows_with_missing_data, "
|
||||||
|
"rows_with_message_too_long, rows_with_empty_message, template_type, expected_errors",
|
||||||
[
|
[
|
||||||
(
|
(
|
||||||
[], [],
|
[], [], [], [],
|
||||||
'sms',
|
'sms',
|
||||||
[]
|
[]
|
||||||
),
|
),
|
||||||
(
|
(
|
||||||
{2}, [],
|
{2}, [], [], [],
|
||||||
'sms',
|
'sms',
|
||||||
['fix 1 phone number']
|
['fix 1 phone number']
|
||||||
),
|
),
|
||||||
(
|
(
|
||||||
{2, 4, 6}, [],
|
{2, 4, 6}, [], [], [],
|
||||||
'sms',
|
'sms',
|
||||||
['fix 3 phone numbers']
|
['fix 3 phone numbers']
|
||||||
),
|
),
|
||||||
(
|
(
|
||||||
{1}, [],
|
{1}, [], [], [],
|
||||||
'email',
|
'email',
|
||||||
['fix 1 email address']
|
['fix 1 email address']
|
||||||
),
|
),
|
||||||
(
|
(
|
||||||
{2, 4, 6}, [],
|
{2, 4, 6}, [], [], [],
|
||||||
'email',
|
'email',
|
||||||
['fix 3 email addresses']
|
['fix 3 email addresses']
|
||||||
),
|
),
|
||||||
(
|
(
|
||||||
{2}, {3},
|
{2}, {3}, [], [],
|
||||||
'sms',
|
'sms',
|
||||||
[
|
[
|
||||||
'fix 1 phone number',
|
'fix 1 phone number',
|
||||||
@@ -50,21 +53,51 @@ MockRecipients = namedtuple(
|
|||||||
]
|
]
|
||||||
),
|
),
|
||||||
(
|
(
|
||||||
{2, 4, 6, 8}, {3, 6, 9, 12},
|
{2, 4, 6, 8}, {3, 6, 9, 12}, [], [],
|
||||||
'sms',
|
'sms',
|
||||||
[
|
[
|
||||||
'fix 4 phone numbers',
|
'fix 4 phone numbers',
|
||||||
'enter missing data in 4 rows'
|
'enter missing data in 4 rows'
|
||||||
]
|
]
|
||||||
)
|
),
|
||||||
|
(
|
||||||
|
{}, {}, {3}, [],
|
||||||
|
'sms',
|
||||||
|
[
|
||||||
|
'shorten the message in 1 row'
|
||||||
|
]
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{}, {}, {3, 12}, [],
|
||||||
|
'sms',
|
||||||
|
[
|
||||||
|
'shorten the messages in 2 rows'
|
||||||
|
]
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{}, {}, {}, {2},
|
||||||
|
'sms',
|
||||||
|
[
|
||||||
|
'check you have content for the empty message in 1 row'
|
||||||
|
]
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{}, {}, {}, {2, 4, 8},
|
||||||
|
'sms',
|
||||||
|
[
|
||||||
|
'check you have content for the empty messages in 3 rows'
|
||||||
|
]
|
||||||
|
),
|
||||||
]
|
]
|
||||||
)
|
)
|
||||||
def test_get_errors_for_csv(
|
def test_get_errors_for_csv(
|
||||||
rows_with_bad_recipients, rows_with_missing_data,
|
rows_with_bad_recipients, rows_with_missing_data, rows_with_message_too_long, rows_with_empty_message,
|
||||||
template_type,
|
template_type,
|
||||||
expected_errors
|
expected_errors
|
||||||
):
|
):
|
||||||
assert get_errors_for_csv(
|
assert get_errors_for_csv(
|
||||||
MockRecipients(rows_with_bad_recipients, rows_with_missing_data),
|
MockRecipients(
|
||||||
|
rows_with_bad_recipients, rows_with_missing_data, rows_with_message_too_long, rows_with_empty_message
|
||||||
|
),
|
||||||
template_type
|
template_type
|
||||||
) == expected_errors
|
) == expected_errors
|
||||||
|
|||||||
@@ -414,12 +414,50 @@ def test_upload_csv_file_with_errors_shows_check_page_with_errors(
|
|||||||
|
|
||||||
assert response.status_code == 200
|
assert response.status_code == 200
|
||||||
content = response.get_data(as_text=True)
|
content = response.get_data(as_text=True)
|
||||||
assert 'There is a problem with invalid.csv' in content
|
assert 'There’s a problem with invalid.csv' in content
|
||||||
assert '+447700900986' in content
|
assert '+447700900986' in content
|
||||||
assert 'Missing' in content
|
assert 'Missing' in content
|
||||||
assert 'Upload your file again' in content
|
assert 'Upload your file again' in content
|
||||||
|
|
||||||
|
|
||||||
|
def test_upload_csv_file_with_empty_message_shows_check_page_with_errors(
|
||||||
|
logged_in_client,
|
||||||
|
service_one,
|
||||||
|
mocker,
|
||||||
|
mock_get_empty_service_template_with_optional_placeholder,
|
||||||
|
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.s3download',
|
||||||
|
return_value="""
|
||||||
|
phone number, show_placeholder
|
||||||
|
+447700900986, yes
|
||||||
|
+447700900986, no
|
||||||
|
"""
|
||||||
|
)
|
||||||
|
|
||||||
|
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
|
||||||
|
)
|
||||||
|
|
||||||
|
with logged_in_client.session_transaction() as session:
|
||||||
|
assert 'file_uploads' not in session
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
content = response.get_data(as_text=True)
|
||||||
|
assert 'There’s a problem with invalid.csv' in content
|
||||||
|
assert 'check you have content for the empty message in 1 row' in content
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize('file_contents, expected_error,', [
|
@pytest.mark.parametrize('file_contents, expected_error,', [
|
||||||
(
|
(
|
||||||
"""
|
"""
|
||||||
@@ -490,8 +528,8 @@ def test_upload_csv_file_with_errors_shows_check_page_with_errors(
|
|||||||
+447700900986, example
|
+447700900986, example
|
||||||
""",
|
""",
|
||||||
(
|
(
|
||||||
'There is a problem with invalid.csv '
|
'There’s a problem with invalid.csv '
|
||||||
'You need to enter missing data in 1 row '
|
'You need to enter missing data in 1 row. '
|
||||||
'Skip to file contents'
|
'Skip to file contents'
|
||||||
)
|
)
|
||||||
),
|
),
|
||||||
@@ -503,8 +541,8 @@ def test_upload_csv_file_with_errors_shows_check_page_with_errors(
|
|||||||
+447700900986, example
|
+447700900986, example
|
||||||
""",
|
""",
|
||||||
(
|
(
|
||||||
'There is a problem with invalid.csv '
|
'There’s a problem with invalid.csv '
|
||||||
'You need to enter missing data in 1 row '
|
'You need to enter missing data in 1 row. '
|
||||||
'Skip to file contents'
|
'Skip to file contents'
|
||||||
)
|
)
|
||||||
),
|
),
|
||||||
@@ -2855,8 +2893,8 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters(
|
|||||||
)
|
)
|
||||||
|
|
||||||
assert normalize_spaces(page.select_one('.banner-dangerous').text) == (
|
assert normalize_spaces(page.select_one('.banner-dangerous').text) == (
|
||||||
'There is a problem with example.xlsx '
|
'There’s a problem with example.xlsx '
|
||||||
'You need to enter missing data in 2 rows '
|
'You need to enter missing data in 2 rows. '
|
||||||
'Skip to file contents'
|
'Skip to file contents'
|
||||||
)
|
)
|
||||||
assert not page.select('.table-field-index a')
|
assert not page.select('.table-field-index a')
|
||||||
|
|||||||
@@ -850,6 +850,20 @@ def mock_get_service_template_with_placeholders(mocker):
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(scope='function')
|
||||||
|
def mock_get_empty_service_template_with_optional_placeholder(mocker):
|
||||||
|
def _get(service_id, template_id, version=None):
|
||||||
|
template = template_json(
|
||||||
|
service_id, template_id, name="Optional content", content="((show_placeholder??Some content))"
|
||||||
|
)
|
||||||
|
return {'data': template}
|
||||||
|
|
||||||
|
return mocker.patch(
|
||||||
|
'app.service_api_client.get_service_template',
|
||||||
|
side_effect=_get
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture(scope='function')
|
@pytest.fixture(scope='function')
|
||||||
def mock_get_service_template_with_multiple_placeholders(mocker):
|
def mock_get_service_template_with_multiple_placeholders(mocker):
|
||||||
def _get(service_id, template_id, version=None):
|
def _get(service_id, template_id, version=None):
|
||||||
|
|||||||
Reference in New Issue
Block a user