Merge pull request #1075 from alphagov/letter-api-refactor

Letter api step 1 - refactor

app/notifications/process_notifications.py

@@ -36,6 +36,7 @@ def check_placeholders(template_object):
 
 
 def persist_notification(
+    *,
     template_id,
     template_version,
     recipient,

app/notifications/validators.py

@@ -9,7 +9,7 @@ from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_
 
 from app.dao import services_dao, templates_dao
 from app.models import (
-    INTERNATIONAL_SMS_TYPE, SMS_TYPE,
+    INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE,
     KEY_TYPE_TEST, KEY_TYPE_TEAM, SCHEDULE_NOTIFICATIONS
 )
 from app.service.utils import service_allowed_to_send_to
@@ -104,7 +104,7 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type)
             number=send_to,
             international=international_phone_info.international
         )
-    else:
+    elif notification_type == EMAIL_TYPE:
         return validate_and_format_email_address(email_address=send_to)
 
 

app/schema_validation/definitions.py

@@ -20,6 +20,9 @@ personalisation = {
 }
 
 
+letter_personalisation = dict(personalisation, required=["address_line_1", "postcode"])
+
+
 https_url = {
     "type": "string",
     "format": "uri",

app/v2/notifications/create_response.py

@@ -0,0 +1,45 @@
+
+def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
+        'from_number': from_number,
+        'body': content
+    }
+    return noti
+
+
+def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
+        "from_email": email_from,
+        "body": content,
+        "subject": subject
+    }
+    return noti
+
+
+def create_post_letter_response_from_notification(notification, content, subject, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
+        "body": content,
+        "subject": subject
+    }
+    return noti
+
+
+def __create_notification_response(notification, url_root, scheduled_for):
+    return {
+        "id": notification.id,
+        "reference": notification.client_reference,
+        "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)),
+        'template': {
+            "id": notification.template_id,
+            "version": notification.template_version,
+            "uri": "{}services/{}/templates/{}".format(
+                url_root,
+                str(notification.service_id),
+                str(notification.template_id)
+            )
+        },
+        "scheduled_for": scheduled_for if scheduled_for else None
+    }

app/v2/notifications/notification_schemas.py

@@ -1,5 +1,5 @@
 from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES
-from app.schema_validation.definitions import (uuid, personalisation)
+from app.schema_validation.definitions import (uuid, personalisation, letter_personalisation)
 
 
 template = {
@@ -192,40 +192,43 @@ post_email_response = {
 }
 
 
-def create_post_sms_response_from_notification(notification, body, from_number, url_root, service_id, scheduled_for):
+post_letter_request = {
-    return {"id": notification.id,
+    "$schema": "http://json-schema.org/draft-04/schema#",
-            "reference": notification.client_reference,
+    "description": "POST letter notification schema",
-            "content": {'body': body,
+    "type": "object",
-                        'from_number': from_number},
+    "title": "POST v2/notifications/letter",
-            "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)),
+    "properties": {
-            "template": __create_template_from_notification(notification=notification,
+        "reference": {"type": "string"},
-                                                            url_root=url_root,
+        "template_id": uuid,
-                                                            service_id=service_id),
+        "personalisation": letter_personalisation
-            "scheduled_for": scheduled_for if scheduled_for else None
+    },
-            }
+    "required": ["template_id", "personalisation"]
+}
 
+letter_content = {
+    "$schema": "http://json-schema.org/draft-04/schema#",
+    "description": "Letter content for POST letter notification",
+    "type": "object",
+    "title": "notification letter content",
+    "properties": {
+        "body": {"type": "string"},
+        "subject": {"type": "string"}
+    },
+    "required": ["body", "subject"]
+}
 
-def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, service_id,
+post_letter_response = {
-                                                 scheduled_for):
+    "$schema": "http://json-schema.org/draft-04/schema#",
-    return {
+    "description": "POST sms notification response schema",
-        "id": notification.id,
+    "type": "object",
-        "reference": notification.client_reference,
+    "title": "response v2/notifications/letter",
-        "content": {
+    "properties": {
-            "from_email": email_from,
+        "id": uuid,
-            "body": content,
+        "reference": {"type": ["string", "null"]},
-            "subject": subject
+        "content": letter_content,
-        },
+        "uri": {"type": "string", "format": "uri"},
-        "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)),
+        "template": template,
-        "template": __create_template_from_notification(notification=notification,
+        "scheduled_for": {"type": ["string", "null"]}
-                                                        url_root=url_root,
+    },
-                                                        service_id=service_id),
+    "required": ["id", "content", "uri", "template"]
-        "scheduled_for": scheduled_for if scheduled_for else None
+}
-    }
-
-
-def __create_template_from_notification(notification, url_root, service_id):
-    return {
-        "id": notification.template_id,
-        "version": notification.template_version,
-        "uri": "{}services/{}/templates/{}".format(url_root, str(service_id), str(notification.template_id))
-    }

app/v2/notifications/post_notifications.py

@@ -1,8 +1,10 @@
-from flask import request, jsonify, current_app
+import functools
+
+from flask import request, jsonify, current_app, abort
 
 from app import api_user, authenticated_service
 from app.config import QueueNames
-from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY, SCHEDULE_NOTIFICATIONS
+from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, PRIORITY
 from app.notifications.process_notifications import (
     persist_notification,
     send_notification_to_queue,
@@ -11,28 +13,34 @@ from app.notifications.process_notifications import (
 from app.notifications.validators import (
     validate_and_format_recipient,
     check_rate_limiting,
-    service_has_permission,
     check_service_can_schedule_notification,
     check_service_has_permission,
     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)
+    post_letter_request
-from app.v2.errors import BadRequestError
+)
+from app.v2.notifications.create_response import (
+    create_post_sms_response_from_notification,
+    create_post_email_response_from_notification,
+    create_post_letter_response_from_notification
+)
 
 
 @v2_notification_blueprint.route('/<notification_type>', methods=['POST'])
 def post_notification(notification_type):
     if notification_type == EMAIL_TYPE:
         form = validate(request.get_json(), post_email_request)
-    else:
+    elif notification_type == SMS_TYPE:
         form = validate(request.get_json(), post_sms_request)
+    elif notification_type == LETTER_TYPE:
+        form = validate(request.get_json(), post_letter_request)
+    else:
+        abort(404)
 
     check_service_has_permission(notification_type, authenticated_service.permissions)
 
@@ -41,12 +49,6 @@ def post_notification(notification_type):
 
     check_rate_limiting(authenticated_service, api_user)
 
-    form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address']
-    send_to = validate_and_format_recipient(send_to=form_send_to,
-                                            key_type=api_user.key_type,
-                                            service=authenticated_service,
-                                            notification_type=notification_type)
-
     template, template_with_content = validate_template(
         form['template_id'],
         form.get('personalisation', {}),
@@ -54,20 +56,74 @@ def post_notification(notification_type):
         notification_type,
     )
 
+    if notification_type == LETTER_TYPE:
+        notification = process_letter_notification(
+            form=form,
+            api_key=api_user,
+            template=template,
+            service=authenticated_service,
+        )
+    else:
+        notification = process_sms_or_email_notification(
+            form=form,
+            notification_type=notification_type,
+            api_key=api_user,
+            template=template,
+            service=authenticated_service
+        )
+
+    if notification_type == SMS_TYPE:
+        sms_sender = authenticated_service.sms_sender or current_app.config.get('FROM_NUMBER')
+        create_resp_partial = functools.partial(
+            create_post_sms_response_from_notification,
+            from_number=sms_sender
+        )
+    elif notification_type == EMAIL_TYPE:
+        create_resp_partial = functools.partial(
+            create_post_email_response_from_notification,
+            subject=template_with_content.subject,
+            email_from=authenticated_service.email_from
+        )
+    elif notification_type == LETTER_TYPE:
+        create_resp_partial = functools.partial(
+            create_post_letter_response_from_notification,
+            subject=template_with_content.subject,
+        )
+
+    resp = create_resp_partial(
+        notification=notification,
+        content=str(template_with_content),
+        url_root=request.url_root,
+        scheduled_for=scheduled_for
+    )
+    return jsonify(resp), 201
+
+
+def process_sms_or_email_notification(*, form, notification_type, api_key, template, service):
+    form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number']
+
+    send_to = validate_and_format_recipient(send_to=form_send_to,
+                                            key_type=api_key.key_type,
+                                            service=service,
+                                            notification_type=notification_type)
+
     # Do not persist or send notification to the queue if it is a simulated recipient
     simulated = simulated_recipient(send_to, notification_type)
 
-    notification = persist_notification(template_id=template.id,
+    notification = persist_notification(
-                                        template_version=template.version,
+        template_id=template.id,
-                                        recipient=form_send_to,
+        template_version=template.version,
-                                        service=authenticated_service,
+        recipient=form_send_to,
-                                        personalisation=form.get('personalisation', None),
+        service=service,
-                                        notification_type=notification_type,
+        personalisation=form.get('personalisation', None),
-                                        api_key_id=api_user.id,
+        notification_type=notification_type,
-                                        key_type=api_user.key_type,
+        api_key_id=api_key.id,
-                                        client_reference=form.get('reference', None),
+        key_type=api_key.key_type,
-                                        simulated=simulated)
+        client_reference=form.get('reference', None),
+        simulated=simulated
+    )
 
+    scheduled_for = form.get("scheduled_for", None)
     if scheduled_for:
         persist_scheduled_notification(notification.id, form["scheduled_for"])
     else:
@@ -75,26 +131,19 @@ def post_notification(notification_type):
             queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None
             send_notification_to_queue(
                 notification=notification,
-                research_mode=authenticated_service.research_mode,
+                research_mode=service.research_mode,
                 queue=queue_name
             )
         else:
             current_app.logger.info("POST simulated notification for id: {}".format(notification.id))
 
-    if notification_type == SMS_TYPE:
+    return notification
-        sms_sender = authenticated_service.sms_sender or current_app.config.get('FROM_NUMBER')
+
-        resp = create_post_sms_response_from_notification(notification=notification,
+
-                                                          body=str(template_with_content),
+def process_letter_notification(*, form, api_key, template, service):
-                                                          from_number=sms_sender,
+    # create job
-                                                          url_root=request.url_root,
+
-                                                          service_id=authenticated_service.id,
+    # create notification
-                                                          scheduled_for=scheduled_for)
+
-    else:
+    # trigger build_dvla_file task
-        resp = create_post_email_response_from_notification(notification=notification,
+    raise NotImplementedError
-                                                            content=str(template_with_content),
-                                                            subject=template_with_content.subject,
-                                                            email_from=authenticated_service.email_from,
-                                                            url_root=request.url_root,
-                                                            service_id=authenticated_service.id,
-                                                            scheduled_for=scheduled_for)
-    return jsonify(resp), 201

tests/app/notifications/test_process_notification.py

@@ -51,10 +51,18 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api
 
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
-    notification = persist_notification(sample_template.id, sample_template.version, '+447111111111',
+    notification = persist_notification(
-                                        sample_template.service, {}, 'sms', sample_api_key.id,
+        template_id=sample_template.id,
-                                        sample_api_key.key_type, job_id=sample_job.id,
+        template_version=sample_template.version,
-                                        job_row_number=100, reference="ref")
+        recipient='+447111111111',
+        service=sample_template.service,
+        personalisation={},
+        notification_type='sms',
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
+        job_id=sample_job.id,
+        job_row_number=100,
+        reference="ref")
 
     assert Notification.query.get(notification.id) is not None
     assert NotificationHistory.query.get(notification.id) is not None
@@ -127,14 +135,14 @@ def test_persist_notification_does_not_increment_cache_if_test_key(
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
     persist_notification(
-        sample_template.id,
+        template_id=sample_template.id,
-        sample_template.version,
+        template_version=sample_template.version,
-        '+447111111111',
+        recipient='+447111111111',
-        sample_template.service,
+        service=sample_template.service,
-        {},
+        personalisation={},
-        'sms',
+        notification_type='sms',
-        api_key.id,
+        api_key_id=api_key.id,
-        api_key.key_type,
+        key_type=api_key.key_type,
         job_id=sample_job.id,
         job_row_number=100,
         reference="ref",
@@ -193,18 +201,33 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa
     mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
     mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value')
 
-    persist_notification(sample_template.id, sample_template.version, '+447111111111',
+    persist_notification(
-                         sample_template.service, {}, 'sms', sample_api_key.id,
+        template_id=sample_template.id,
-                         sample_api_key.key_type, reference="ref")
+        template_version=sample_template.version,
+        recipient='+447111111111',
+        service=sample_template.service,
+        personalisation={},
+        notification_type='sms',
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
+        reference="ref"
+    )
     mock_incr.assert_not_called()
     mock_incr_hash_value.assert_not_called()
 
     mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
     mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
                  return_value={sample_template.id, 1})
-    persist_notification(sample_template.id, sample_template.version, '+447111111122',
+    persist_notification(
-                         sample_template.service, {}, 'sms', sample_api_key.id,
+        template_id=sample_template.id,
-                         sample_api_key.key_type, reference="ref2")
+        template_version=sample_template.version,
+        recipient='+447111111122',
+        service=sample_template.service,
+        personalisation={},
+        notification_type='sms',
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
+        reference="ref2")
     mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", )
     mock_incr_hash_value.assert_called_once_with(cache_key_for_service_template_counter(sample_template.service_id),
                                                  sample_template.id)

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -0,0 +1,166 @@
+
+import uuid
+
+from flask import url_for, json
+import pytest
+
+from app.models import Job, Notification, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE
+from app.v2.errors import RateLimitError
+
+from tests import create_authorization_header
+from tests.app.db import create_service, create_template
+
+
+pytestmark = pytest.mark.skip('Leters not currently implemented')
+
+
+def letter_request(client, data, service_id, _expected_status=201):
+    resp = client.post(
+        url_for('v2_notifications.post_notification', notification_type='letter'),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service_id)]
+    )
+    json_resp = json.loads(resp.get_data(as_text=True))
+    assert resp.status_code == _expected_status, json_resp
+    return json_resp
+
+
+@pytest.mark.parametrize('reference', [None, 'reference_from_client'])
+def test_post_letter_notification_returns_201(client, sample_letter_template, mocker, reference):
+    mocked = mocker.patch('app.celery.tasks.build_dvla_file.apply_async')
+    data = {
+        'template_id': str(sample_letter_template.id),
+        'personalisation': {
+            'address_line_1': 'Her Royal Highness Queen Elizabeth II',
+            'address_line_2': 'Buckingham Palace',
+            'address_line_3': 'London',
+            'postcode': 'SW1 1AA',
+            'name': 'Lizzie'
+        }
+    }
+
+    if reference:
+        data.update({'reference': reference})
+
+    resp_json = letter_request(client, data, service_id=sample_letter_template.service_id)
+
+    job = Job.query.one()
+    notification = Notification.query.all()
+    notification_id = notification.id
+    assert resp_json['id'] == str(notification_id)
+    assert resp_json['reference'] == reference
+    assert resp_json['content']['subject'] == sample_letter_template.subject
+    assert resp_json['content']['body'] == sample_letter_template.content
+    assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri']
+    assert resp_json['template']['id'] == str(sample_letter_template.id)
+    assert resp_json['template']['version'] == sample_letter_template.version
+    assert (
+        'services/{}/templates/{}'.format(
+            sample_letter_template.service_id,
+            sample_letter_template.id
+        ) in resp_json['template']['uri']
+    )
+    assert not resp_json['scheduled_for']
+
+    mocked.assert_called_once_with((str(job.id), ), queue='job-tasks')
+
+
+def test_post_letter_notification_returns_400_and_missing_template(
+    client,
+    sample_service
+):
+    data = {
+        'template_id': str(uuid.uuid4()),
+        'personalisation': {'address_line_1': '', 'postcode': ''}
+    }
+
+    error_json = letter_request(client, data, service_id=sample_service.id, _expected_status=400)
+
+    assert error_json['status_code'] == 400
+    assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Template not found'}]
+
+
+def test_post_notification_returns_403_and_well_formed_auth_error(
+    client,
+    sample_letter_template
+):
+    data = {
+        'template_id': str(sample_letter_template.id),
+        'personalisation': {'address_line_1': '', 'postcode': ''}
+    }
+
+    error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=401)
+
+    assert error_json['status_code'] == 401
+    assert error_json['errors'] == [{
+        'error': 'AuthError',
+        'message': 'Unauthorized, authentication token must be provided'
+    }]
+
+
+def test_notification_returns_400_for_schema_problems(
+    client,
+    sample_service
+):
+    data = {
+        'personalisation': {'address_line_1': '', 'postcode': ''}
+    }
+
+    error_json = letter_request(client, data, service_id=sample_service.id, _expected_status=400)
+
+    assert error_json['status_code'] == 400
+    assert error_json['errors'] == [{
+        'error': 'ValidationError',
+        'message': 'template_id is a required property'
+    }]
+
+
+def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(
+    client,
+    sample_letter_template,
+    mocker
+):
+    persist_mock = mocker.patch('app.v2.notifications.post_notifications.persist_notification')
+    mocker.patch(
+        'app.v2.notifications.post_notifications.check_rate_limiting',
+        side_effect=RateLimitError('LIMIT', 'INTERVAL', 'TYPE')
+    )
+
+    data = {
+        'template_id': str(sample_letter_template.id),
+        'personalisation': {'address_line_1': '', 'postcode': ''}
+    }
+
+    error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=429)
+
+    assert error_json['status_code'] == 429
+    assert error_json['errors'] == [{
+        'error': 'RateLimitError',
+        'message': 'Exceeded rate limit for key type TYPE of LIMIT requests per INTERVAL seconds'
+    }]
+
+    assert not persist_mock.called
+
+
+@pytest.mark.parametrize('service_args', [
+    {'service_permissions': [EMAIL_TYPE, SMS_TYPE]},
+    {'restricted': True}
+])
+def test_post_letter_notification_returns_403_if_not_allowed_to_send_notification(
+    client,
+    notify_db_session,
+    service_args
+):
+    service = create_service(**service_args)
+    template = create_template(service, template_type=LETTER_TYPE)
+
+    data = {
+        'template_id': str(template.id),
+        'personalisation': {'address_line_1': '', 'postcode': ''}
+    }
+
+    error_json = letter_request(client, data, service_id=service.id, _expected_status=400)
+    assert error_json['status_code'] == 403
+    assert error_json['errors'] == [
+        {'error': 'BadRequestError', 'message': 'Cannot send letters'}
+    ]

tests/app/v2/notifications/test_post_notifications.py

@@ -58,8 +58,8 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol
 @pytest.mark.parametrize("notification_type, key_send_to, send_to",
                          [("sms", "phone_number", "+447700900855"),
                           ("email", "email_address", "sample@email.com")])
-def test_post_sms_notification_returns_400_and_missing_template(client, sample_service,
+def test_post_notification_returns_400_and_missing_template(client, sample_service,
-                                                                notification_type, key_send_to, send_to):
+                                                            notification_type, key_send_to, send_to):
     data = {
         key_send_to: send_to,
         'template_id': str(uuid.uuid4())
@@ -434,3 +434,15 @@ def test_post_notification_raises_bad_request_if_service_not_invited_to_schedule
     error_json = json.loads(response.get_data(as_text=True))
     assert error_json['errors'] == [
         {"error": "BadRequestError", "message": 'Cannot schedule notifications (this feature is invite-only)'}]
+
+
+def test_post_notification_raises_bad_request_if_not_valid_notification_type(client, sample_service):
+    auth_header = create_authorization_header(service_id=sample_service.id)
+    response = client.post(
+        '/v2/notifications/foo',
+        data='{}',
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+    assert response.status_code == 404
+    error_json = json.loads(response.get_data(as_text=True))
+    assert 'The requested URL was not found on the server.' in error_json['message']