Refactored to make code clearer

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)
 

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):
+def service_has_permission(notify_type, permissions):
-    if permission not in [p.permission for p in service.permissions]:
+    return notify_type in [p.permission for p in 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 validate_and_format_recipient(send_to, key_type, service, notification_type):

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/<uuid:service_id>/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()
 

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 '')

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('/<notification_type>', 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)
 

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'),
+    ([EMAIL_TYPE], SMS_TYPE, None, 'Creating text message templates is not allowed'),
-    ([SMS_TYPE], EMAIL_TYPE, 'subject', 'Create email template is not allowed'),
+    ([SMS_TYPE], EMAIL_TYPE, 'subject', 'Creating email templates is not allowed'),
-    ([SMS_TYPE], LETTER_TYPE, 'subject', 'Create letter template 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_sms_permission, 'Updating text message templates is not allowed'),
-    (sample_template_without_email_permission, 'Update email template is not allowed'),
+    (sample_template_without_email_permission, 'Updating email templates is not allowed'),
-    (sample_template_without_letter_permission, 'Update letter template 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):