From ed1bc8d8068a9697774b35b4c7776e2ec93f371e Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 25 Feb 2020 16:11:53 +0000 Subject: [PATCH 1/5] All services can send files by email if they have set contact_link --- app/notifications/validators.py | 8 +++++++- app/v2/notifications/post_notifications.py | 12 ++++++------ tests/app/notifications/test_validators.py | 13 +++++++++++++ .../app/v2/notifications/test_post_notifications.py | 7 ++++--- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index a591d3c0d..1ef0a49a4 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -85,7 +85,13 @@ def service_has_permission(notify_type, permissions): def check_service_has_permission(notify_type, permissions): if not service_has_permission(notify_type, permissions): raise BadRequestError(message="Service is not allowed to send {}".format( - get_public_notify_type_text(notify_type, plural=True))) + get_public_notify_type_text(notify_type, plural=True) + )) + + +def check_if_service_can_send_files_by_email(service_contact_link): + if not service_contact_link: + raise BadRequestError(message="Go to Service Settings to turn on sending files by email") def check_service_can_schedule_notification(permissions, scheduled_for): diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index c37d67d19..cc60b9579 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -16,7 +16,6 @@ from app.models import ( SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, - UPLOAD_DOCUMENT, PRIORITY, KEY_TYPE_TEST, KEY_TYPE_TEAM, @@ -35,13 +34,14 @@ from app.notifications.process_notifications import ( simulated_recipient ) from app.notifications.validators import ( - validate_and_format_recipient, + check_if_service_can_send_files_by_email, check_rate_limiting, check_service_can_schedule_notification, - check_service_has_permission, - validate_template, check_service_email_reply_to_id, - check_service_sms_sender_id + check_service_has_permission, + check_service_sms_sender_id, + validate_and_format_recipient, + validate_template, ) from app.schema_validation import validate from app.v2.errors import BadRequestError @@ -235,7 +235,7 @@ def process_document_uploads(personalisation_data, service, simulated=False): personalisation_data = personalisation_data.copy() - check_service_has_permission(UPLOAD_DOCUMENT, authenticated_service.permissions) + check_if_service_can_send_files_by_email(authenticated_service.contact_link) for key in file_keys: if simulated: diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 49ea02ca3..5e6342c38 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -8,6 +8,7 @@ from app.dao import templates_dao from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE from app.notifications.process_notifications import create_content_for_notification from app.notifications.validators import ( + check_if_service_can_send_files_by_email, check_notification_content_is_not_empty, check_service_over_daily_message_limit, check_template_is_for_notification_type, @@ -528,3 +529,15 @@ def test_check_reply_to_sms_type(sample_service): def test_check_reply_to_letter_type(sample_service): letter_contact = create_letter_contact(service=sample_service, contact_block='123456') assert check_reply_to(sample_service.id, letter_contact.id, LETTER_TYPE) == '123456' + + +def test_check_if_service_can_send_files_by_email_raises_if_no_contact_link_set(sample_service): + with pytest.raises(BadRequestError) as e: + check_if_service_can_send_files_by_email(sample_service.contact_link) + assert e.value.status_code == 400 + assert e.value.message == "Go to Service Settings to turn on sending files by email" + + +def test_check_if_service_can_send_files_by_email_passes_if_contact_link_set(sample_service): + sample_service.contact_link = 'contact.me@gov.uk' + check_if_service_can_send_files_by_email(sample_service.contact_link) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 02cab3c15..ee4d46104 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -11,7 +11,6 @@ from app.models import ( NOTIFICATION_CREATED, SCHEDULE_NOTIFICATIONS, SMS_TYPE, - UPLOAD_DOCUMENT, INTERNATIONAL_SMS_TYPE ) from flask import json, current_app @@ -775,7 +774,8 @@ def test_post_email_notification_with_archived_reply_to_id_returns_400(client, s def test_post_notification_with_document_upload(client, notify_db_session, mocker): - service = create_service(service_permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) + service = create_service(service_permissions=[EMAIL_TYPE]) + service.contact_link = 'contact.me@gov.uk' template = create_template( service=service, template_type='email', @@ -822,7 +822,8 @@ def test_post_notification_with_document_upload(client, notify_db_session, mocke def test_post_notification_with_document_upload_simulated(client, notify_db_session, mocker): - service = create_service(service_permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) + service = create_service(service_permissions=[EMAIL_TYPE]) + service.contact_link = 'contact.me@gov.uk' template = create_template( service=service, template_type='email', From 9a12d0e80ea05e2d6ac7288c4c3830ea42be1af0 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 25 Feb 2020 17:10:22 +0000 Subject: [PATCH 2/5] Update error message after discussion with Karl --- app/notifications/validators.py | 4 +++- tests/app/notifications/test_validators.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 1ef0a49a4..5e5bf8ce0 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -91,7 +91,9 @@ def check_service_has_permission(notify_type, permissions): def check_if_service_can_send_files_by_email(service_contact_link): if not service_contact_link: - raise BadRequestError(message="Go to Service Settings to turn on sending files by email") + raise BadRequestError( + message="Send files by email is not set up yet. Go to your settings page to manage send files by email" + ) def check_service_can_schedule_notification(permissions, scheduled_for): diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 5e6342c38..b6405f0e9 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -534,8 +534,10 @@ def test_check_reply_to_letter_type(sample_service): def test_check_if_service_can_send_files_by_email_raises_if_no_contact_link_set(sample_service): with pytest.raises(BadRequestError) as e: check_if_service_can_send_files_by_email(sample_service.contact_link) + + expected_message = "Send files by email is not set up yet. Go to your settings page to manage send files by email" assert e.value.status_code == 400 - assert e.value.message == "Go to Service Settings to turn on sending files by email" + assert e.value.message == expected_message def test_check_if_service_can_send_files_by_email_passes_if_contact_link_set(sample_service): From 6d21515adff16239a19a1d92f78fad1e5ac4c63d Mon Sep 17 00:00:00 2001 From: "Pea M. Tyczynska" Date: Wed, 26 Feb 2020 10:40:12 +0000 Subject: [PATCH 3/5] Apply suggestions from code review Better grammar for our new error message. Co-Authored-By: karlchillmaid --- app/notifications/validators.py | 2 +- tests/app/notifications/test_validators.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 5e5bf8ce0..532d6e857 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -92,7 +92,7 @@ def check_service_has_permission(notify_type, permissions): def check_if_service_can_send_files_by_email(service_contact_link): if not service_contact_link: raise BadRequestError( - message="Send files by email is not set up yet. Go to your settings page to manage send files by email" + message="Send files by email has not been set up. Go to your Settings page to manage Send files by email." ) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index b6405f0e9..70f4f3b01 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -535,9 +535,9 @@ def test_check_if_service_can_send_files_by_email_raises_if_no_contact_link_set( with pytest.raises(BadRequestError) as e: check_if_service_can_send_files_by_email(sample_service.contact_link) - expected_message = "Send files by email is not set up yet. Go to your settings page to manage send files by email" + message = "Send files by email has not been set up. Go to your Settings page to manage Send files by email." assert e.value.status_code == 400 - assert e.value.message == expected_message + assert e.value.message == message def test_check_if_service_can_send_files_by_email_passes_if_contact_link_set(sample_service): From 7ca35b44a71835c930e1de304a2526ee339df440 Mon Sep 17 00:00:00 2001 From: "Pea M. Tyczynska" Date: Wed, 26 Feb 2020 10:40:12 +0000 Subject: [PATCH 4/5] Our new error message now has better grammar and form matching other errors more Co-Authored-By: karlchillmaid --- app/notifications/validators.py | 2 +- tests/app/notifications/test_validators.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 5e5bf8ce0..25d6583e1 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -92,7 +92,7 @@ def check_service_has_permission(notify_type, permissions): def check_if_service_can_send_files_by_email(service_contact_link): if not service_contact_link: raise BadRequestError( - message="Send files by email is not set up yet. Go to your settings page to manage send files by email" + message="Send files by email has not been set up - go to your Settings page to manage Send files by email." ) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index b6405f0e9..173c704af 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -535,9 +535,9 @@ def test_check_if_service_can_send_files_by_email_raises_if_no_contact_link_set( with pytest.raises(BadRequestError) as e: check_if_service_can_send_files_by_email(sample_service.contact_link) - expected_message = "Send files by email is not set up yet. Go to your settings page to manage send files by email" + message = "Send files by email has not been set up - go to your Settings page to manage Send files by email." assert e.value.status_code == 400 - assert e.value.message == expected_message + assert e.value.message == message def test_check_if_service_can_send_files_by_email_passes_if_contact_link_set(sample_service): From 95d48d40a93039f2f82fd0a9ae85fd58cbcb0b49 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 26 Feb 2020 16:04:15 +0000 Subject: [PATCH 5/5] Update error message, now includes the url where the service can add contact details. --- app/notifications/validators.py | 6 ++++-- app/v2/notifications/post_notifications.py | 5 ++++- tests/app/notifications/test_validators.py | 13 ++++++++++--- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 532d6e857..39e4b5386 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -89,10 +89,12 @@ def check_service_has_permission(notify_type, permissions): )) -def check_if_service_can_send_files_by_email(service_contact_link): +def check_if_service_can_send_files_by_email(service_contact_link, service_id): if not service_contact_link: + raise BadRequestError( - message="Send files by email has not been set up. Go to your Settings page to manage Send files by email." + message=f"Send files by email has not been set up - add contact details for your service at " + f"{current_app.config['ADMIN_BASE_URL']}/services/{service_id}/service-settings/send-files-by-email" ) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index cc60b9579..04aae8776 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -235,7 +235,10 @@ def process_document_uploads(personalisation_data, service, simulated=False): personalisation_data = personalisation_data.copy() - check_if_service_can_send_files_by_email(authenticated_service.contact_link) + check_if_service_can_send_files_by_email( + service_contact_link=authenticated_service.contact_link, + service_id=authenticated_service.id + ) for key in file_keys: if simulated: diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 70f4f3b01..f4ee4c6ee 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -533,13 +533,20 @@ def test_check_reply_to_letter_type(sample_service): def test_check_if_service_can_send_files_by_email_raises_if_no_contact_link_set(sample_service): with pytest.raises(BadRequestError) as e: - check_if_service_can_send_files_by_email(sample_service.contact_link) + check_if_service_can_send_files_by_email( + service_contact_link=sample_service.contact_link, + service_id=sample_service.id + ) - message = "Send files by email has not been set up. Go to your Settings page to manage Send files by email." + message = f"Send files by email has not been set up - add contact details for your service at " \ + f"http://localhost:6012/services/{sample_service.id}/service-settings/send-files-by-email" assert e.value.status_code == 400 assert e.value.message == message def test_check_if_service_can_send_files_by_email_passes_if_contact_link_set(sample_service): sample_service.contact_link = 'contact.me@gov.uk' - check_if_service_can_send_files_by_email(sample_service.contact_link) + check_if_service_can_send_files_by_email( + service_contact_link=sample_service.contact_link, + service_id=sample_service.id + )