From 0b3277b8a43cd3ef5a12044e00acf51169c437e0 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 29 Jun 2017 18:02:21 +0100 Subject: [PATCH] Refactored to make code clearer --- app/notifications/rest.py | 28 ++++++++++------------ app/notifications/validators.py | 13 ++-------- app/template/rest.py | 22 ++++++++--------- app/utils.py | 9 +++++++ app/v2/notifications/post_notifications.py | 9 +++++-- tests/app/template/test_rest.py | 12 +++++----- 6 files changed, 47 insertions(+), 46 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 00701b11f..907983508 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -27,7 +27,8 @@ from app.notifications.process_notifications import ( from app.notifications.validators import ( check_template_is_for_notification_type, check_template_is_active, - check_rate_limiting + check_rate_limiting, + service_has_permission, ) from app.schemas import ( email_notification_schema, @@ -121,10 +122,19 @@ def send_notification(notification_type): _service_allowed_to_send_to(notification_form, authenticated_service) if notification_type == SMS_TYPE: - _service_has_permission(authenticated_service, SMS_TYPE) + if service_has_permission(SMS_TYPE, authenticated_service.permissions) is False: + raise InvalidRequest( + {'to': ["Cannot send text messages"]}, + status_code=400 + ) + _service_can_send_internationally(authenticated_service, notification_form['to']) elif notification_type == EMAIL_TYPE: - _service_has_permission(authenticated_service, EMAIL_TYPE) + if service_has_permission(EMAIL_TYPE, authenticated_service.permissions) is False: + raise InvalidRequest( + {'to': ["Cannot send emails"]}, + status_code=400 + ) # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(notification_form['to'], notification_type) @@ -167,18 +177,6 @@ def get_notification_return_data(notification_id, notification, template): return output -def _service_has_permission(service, notify_type): - if notify_type not in [p.permission for p in service.permissions]: - notify_type_text = notify_type + 's' - if notify_type == SMS_TYPE: - notify_type_text = 'text messages' - - raise InvalidRequest( - {'to': ["Cannot send {}".format(notify_type_text)]}, - status_code=400 - ) - - def _service_can_send_internationally(service, number): international_phone_info = get_international_phone_info(number) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index f35982b68..17fddef41 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -73,17 +73,8 @@ def service_can_send_to_recipient(send_to, key_type, service): raise BadRequestError(message=message) -def service_has_permission(service, permission): - if permission not in [p.permission for p in service.permissions]: - action = 'send' - permission_text = permission + 's' - if permission == SMS_TYPE: - permission_text = 'text messages' - elif permission == SCHEDULE_NOTIFICATIONS: - action = 'schedule' - permission_text = "notifications (this feature is invite-only)" - - raise BadRequestError(message="Cannot {} {}".format(action, permission_text)) +def service_has_permission(notify_type, permissions): + return notify_type in [p.permission for p in permissions] def validate_and_format_recipient(send_to, key_type, service, notification_type): diff --git a/app/template/rest.py b/app/template/rest.py index c82e99eec..90d7e114a 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -17,6 +17,7 @@ from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.models import SMS_TYPE, EMAIL_TYPE +from app.notifications.validators import service_has_permission from app.schemas import (template_schema, template_history_schema) template_blueprint = Blueprint('template', __name__, url_prefix='/service//template') @@ -25,7 +26,7 @@ from app.errors import ( register_errors, InvalidRequest ) -from app.utils import get_template_instance +from app.utils import get_template_instance, get_public_notify_type_text register_errors(template_blueprint) @@ -37,15 +38,6 @@ def _content_count_greater_than_limit(content, template_type): return template.content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') -def _service_has_permission(template_type, action, permissions): - if template_type not in [p.permission for p in permissions]: - template_type_text = template_type - if template_type == SMS_TYPE: - template_type_text = 'text message' - raise InvalidRequest("{action} {type} template is not allowed".format( - action=action, type=template_type_text), 403) - - @template_blueprint.route('', methods=['POST']) def create_template(service_id): fetched_service = dao_fetch_service_by_id(service_id=service_id) @@ -53,7 +45,10 @@ def create_template(service_id): permissions = fetched_service.permissions new_template = template_schema.load(request.get_json()).data - _service_has_permission(new_template.template_type, 'Create', permissions) + if service_has_permission(new_template.template_type, permissions) is False: + raise InvalidRequest( + "Creating {} templates is not allowed".format( + get_public_notify_type_text(new_template.template_type)), 403) new_template.service = fetched_service over_limit = _content_count_greater_than_limit(new_template.content, new_template.template_type) @@ -71,7 +66,10 @@ def create_template(service_id): def update_template(service_id, template_id): fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id) - _service_has_permission(fetched_template.template_type, 'Update', fetched_template.service.permissions) + if service_has_permission(fetched_template.template_type, fetched_template.service.permissions) is False: + raise InvalidRequest( + "Updating {} templates is not allowed".format( + get_public_notify_type_text(fetched_template.template_type)), 403) data = request.get_json() diff --git a/app/utils.py b/app/utils.py index d93ea0613..ea30a6df2 100644 --- a/app/utils.py +++ b/app/utils.py @@ -77,3 +77,12 @@ def get_london_month_from_utc_column(column): def cache_key_for_service_template_counter(service_id, limit_days=7): return "{}-template-counter-limit-{}-days".format(service_id, limit_days) + + +def get_public_notify_type_text(notify_type, plural=False): + from app.models import SMS_TYPE + notify_type_text = notify_type + if notify_type == SMS_TYPE: + notify_type_text = 'text message' + + return '{}{}'.format(notify_type_text, 's' if plural else '') diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index e4b1ee52e..8f2706f7b 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -15,12 +15,14 @@ from app.notifications.validators import ( validate_template ) from app.schema_validation import validate +from app.utils import get_public_notify_type_text from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, create_post_sms_response_from_notification, post_email_request, create_post_email_response_from_notification) +from app.v2.errors import BadRequestError @v2_notification_blueprint.route('/', methods=['POST']) @@ -30,11 +32,14 @@ def post_notification(notification_type): else: form = validate(request.get_json(), post_sms_request) - service_has_permission(authenticated_service, notification_type) + if service_has_permission(notification_type, authenticated_service.permissions) is False: + raise BadRequestError(message="Cannot send {}".format( + get_public_notify_type_text(notification_type, plural=True))) scheduled_for = form.get("scheduled_for", None) if scheduled_for: - service_has_permission(authenticated_service, SCHEDULE_NOTIFICATIONS) + if service_has_permission(SCHEDULE_NOTIFICATIONS, authenticated_service.permissions) is False: + raise BadRequestError(message="Cannot schedule notifications (this feature is invite-only)") check_rate_limiting(authenticated_service, api_user) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index ad951f35a..00e9b4895 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -91,9 +91,9 @@ def test_should_raise_error_if_service_does_not_exist_on_create(client, sample_u @pytest.mark.parametrize('permissions, template_type, subject, expected_error', [ - ([EMAIL_TYPE], SMS_TYPE, None, 'Create text message template is not allowed'), - ([SMS_TYPE], EMAIL_TYPE, 'subject', 'Create email template is not allowed'), - ([SMS_TYPE], LETTER_TYPE, 'subject', 'Create letter template is not allowed'), + ([EMAIL_TYPE], SMS_TYPE, None, 'Creating text message templates is not allowed'), + ([SMS_TYPE], EMAIL_TYPE, 'subject', 'Creating email templates is not allowed'), + ([SMS_TYPE], LETTER_TYPE, 'subject', 'Creating letter templates is not allowed'), ]) def test_should_raise_error_on_create_if_no_permission( client, sample_user, permissions, template_type, subject, expected_error): @@ -123,9 +123,9 @@ def test_should_raise_error_on_create_if_no_permission( @pytest.mark.parametrize('template_factory, expected_error', [ - (sample_template_without_sms_permission, 'Update text message template is not allowed'), - (sample_template_without_email_permission, 'Update email template is not allowed'), - (sample_template_without_letter_permission, 'Update letter template is not allowed') + (sample_template_without_sms_permission, 'Updating text message templates is not allowed'), + (sample_template_without_email_permission, 'Updating email templates is not allowed'), + (sample_template_without_letter_permission, 'Updating letter templates is not allowed') ]) def test_should_be_error_on_update_if_no_permission( client, sample_user, template_factory, expected_error, notify_db, notify_db_session):