Merge branch 'master' into dont-send-message-twice

Conflicts:
	tests/app/notifications/test_validators.py

app/dao/notifications_dao.py

@@ -8,7 +8,7 @@ from itertools import groupby
 
 from flask import current_app
 from werkzeug.datastructures import MultiDict
-from sqlalchemy import (desc, func, or_, and_, asc, cast, Text)
+from sqlalchemy import (desc, func, or_, and_, asc)
 from sqlalchemy.orm import joinedload
 
 from app import db, create_uuid

app/schema_validation/__init__.py

@@ -42,6 +42,4 @@ def build_error_message(errors):
 
 
 def __format_message(e):
-    s = e.message.split("'")
+    return e.message.replace("'", "") if not e.cause else "{} {}".format(e.path[0], e.cause.message)
-    msg = "{}{}".format(s[1], s[2])
-    return msg if not e.cause else "{} {}".format(e.path[0], e.cause.message)

app/v2/notifications/get_notifications.py

@@ -1,9 +1,9 @@
 from flask import jsonify, request, url_for
-
 from app import api_user
 from app.dao import notifications_dao
-from app.schemas import notifications_filter_schema
+from app.schema_validation import validate
 from app.v2.notifications import notification_blueprint
+from app.v2.notifications.notification_schemas import get_notifications_request
 
 
 @notification_blueprint.route("/<uuid:id>", methods=['GET'])
@@ -17,7 +17,12 @@ def get_notification_by_id(id):
 
 @notification_blueprint.route("", methods=['GET'])
 def get_notifications():
-    data = notifications_filter_schema.load(request.args).data
+    _data = request.args.to_dict(flat=False)
+    if 'older_than' in _data:
+        # flat=False makes everything a list, but we only ever allow one value for "older_than"
+        _data['older_than'] = _data['older_than'][0]
+
+    data = validate(_data, get_notifications_request)
 
     paginated_notifications = notifications_dao.get_notifications_for_service(
         str(api_user.service_id),
@@ -29,11 +34,11 @@ def get_notifications():
 
     def _build_links(notifications):
         _links = {
-            'current': url_for(".get_notifications", _external=True, **request.args.to_dict(flat=False)),
+            'current': url_for(".get_notifications", _external=True, **data),
         }
 
         if len(notifications):
-            next_query_params = dict(request.args.to_dict(flat=False), older_than=notifications[-1].id)
+            next_query_params = dict(data, older_than=notifications[-1].id)
             _links['next'] = url_for(".get_notifications", _external=True, **next_query_params)
 
         return _links

app/v2/notifications/notification_schemas.py

@@ -1,3 +1,4 @@
+from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES
 from app.schema_validation.definitions import (uuid, personalisation)
 
 # this may belong in a templates module
@@ -72,6 +73,28 @@ get_notification_response = {
     ]
 }
 
+get_notifications_request = {
+    "$schema": "http://json-schema.org/draft-04/schema#",
+    "description": "schema for query parameters allowed when getting list of notifications",
+    "type": "object",
+    "properties": {
+        "status": {
+            "type": "array",
+            "items": {
+                "enum": NOTIFICATION_STATUS_TYPES
+            }
+        },
+        "template_type": {
+            "type": "array",
+            "items": {
+                "enum": TEMPLATE_TYPES
+            }
+        },
+        "older_than": uuid
+    },
+    "additionalProperties": False,
+}
+
 get_notifications_response = {
     "$schema": "http://json-schema.org/draft-04/schema#",
     "description": "GET list of notifications response schema",

app/v2/notifications/post_notifications.py

@@ -42,7 +42,7 @@ def post_sms_notification():
     send_notification_to_queue(notification, service.research_mode)
 
     resp = create_post_sms_response_from_notification(notification,
-                                                      template_with_content.content,
+                                                      template_with_content.replaced,
                                                       service.sms_sender,
                                                       request.url_root)
     return jsonify(resp), 201
@@ -70,7 +70,7 @@ def post_email_notification():
     send_notification_to_queue(notification, service.research_mode)
 
     resp = create_post_email_response_from_notification(notification=notification,
-                                                        content=template_with_content.content,
+                                                        content=template_with_content.replaced,
                                                         subject=template_with_content.subject,
                                                         email_from=service.email_from,
                                                         url_root=request.url_root)

tests/app/notifications/test_validators.py

@@ -1,4 +1,3 @@
-from datetime import datetime
 import pytest
 from freezegun import freeze_time
 
@@ -22,9 +21,10 @@ from tests.app.conftest import (
 
 
 @pytest.mark.parametrize('key_type', ['team', 'normal'])
-def test_exception_thrown_by_redis_store_get_should_not_be_fatal(
+def test_exception_thown_by_redis_store_get_should_not_be_fatal(
         notify_db,
         notify_db_session,
+        notify_api,
         key_type,
         mocker):
     mocker.patch('app.notifications.validators.redis_store.redis_store.get', side_effect=Exception("broken redis"))
@@ -39,8 +39,7 @@ def test_exception_thrown_by_redis_store_get_should_not_be_fatal(
     assert e.value.status_code == 429
     assert e.value.message == 'Exceeded send limits (4) for today'
     assert e.value.fields == []
-    app.notifications.validators.redis_store.set\
+    assert app.notifications.validators.redis_store.set.called
-        .assert_called_with("{}-{}-count".format(service.id, datetime.utcnow().strftime("%Y-%m-%d")), 5, ex=3600)
 
 
 @pytest.mark.parametrize('key_type', ['test', 'team', 'normal'])

tests/app/v2/notifications/test_get_notifications.py

@@ -60,6 +60,44 @@ def test_get_notification_by_id_returns_200(
     assert json_response == expected_response
 
 
+def test_get_notification_by_id_nonexistent_id(client, sample_notification):
+    auth_header = create_authorization_header(service_id=sample_notification.service_id)
+    response = client.get(
+        path='/v2/notifications/dd4b8b9d-d414-4a83-9256-580046bf18f9',
+        headers=[('Content-Type', 'application/json'), auth_header])
+
+    assert response.status_code == 404
+    assert response.headers['Content-type'] == 'application/json'
+
+    json_response = json.loads(response.get_data(as_text=True))
+    assert json_response == {
+        "errors": [
+            {
+                "error": "NoResultFound",
+                "message": "No result found"
+            }
+        ],
+        "status_code": 404
+    }
+
+
+def test_get_notification_by_id_invalid_id(client, sample_notification):
+    auth_header = create_authorization_header(service_id=sample_notification.service_id)
+    response = client.get(
+        path='/v2/notifications/1234-badly-formatted-id-7890',
+        headers=[('Content-Type', 'application/json'), auth_header])
+
+    assert response.status_code == 404
+    assert response.headers['Content-type'] == 'application/json'
+
+    json_response = json.loads(response.get_data(as_text=True))
+    assert json_response == {
+        "message": "The requested URL was not found on the server.  "
+                   "If you entered the URL manually please check your spelling and try again.",
+        "result": "error"
+    }
+
+
 def test_get_all_notifications_returns_200(client, notify_db, notify_db_session):
     notifications = [create_sample_notification(notify_db, notify_db_session) for _ in range(2)]
     notification = notifications[-1]
@@ -88,7 +126,7 @@ def test_get_all_notifications_returns_200(client, notify_db, notify_db_session)
     assert json_response['notifications'][0]['type'] == "sms"
 
 
-def test_get_all_notifications_no_notifications_if_no_notificatons(client, sample_service):
+def test_get_all_notifications_no_notifications_if_no_notifications(client, sample_service):
     auth_header = create_authorization_header(service_id=sample_service.id)
     response = client.get(
         path='/v2/notifications',
@@ -135,6 +173,22 @@ def test_get_all_notifications_filter_by_template_type(client, notify_db, notify
     assert json_response['notifications'][0]['type'] == "email"
 
 
+def test_get_all_notifications_filter_by_template_type_invalid_template_type(client, sample_notification):
+    auth_header = create_authorization_header(service_id=sample_notification.service_id)
+    response = client.get(
+        path='/v2/notifications?template_type=orange',
+        headers=[('Content-Type', 'application/json'), auth_header])
+
+    json_response = json.loads(response.get_data(as_text=True))
+
+    assert response.status_code == 400
+    assert response.headers['Content-type'] == "application/json"
+
+    assert json_response['status_code'] == 400
+    assert len(json_response['errors']) == 1
+    assert json_response['errors'][0]['message'] == "orange is not one of [sms, email, letter]"
+
+
 def test_get_all_notifications_filter_by_single_status(client, notify_db, notify_db_session):
     notification = create_sample_notification(notify_db, notify_db_session, status="pending")
     create_sample_notification(notify_db, notify_db_session)
@@ -156,6 +210,23 @@ def test_get_all_notifications_filter_by_single_status(client, notify_db, notify
     assert json_response['notifications'][0]['status'] == "pending"
 
 
+def test_get_all_notifications_filter_by_status_invalid_status(client, sample_notification):
+    auth_header = create_authorization_header(service_id=sample_notification.service_id)
+    response = client.get(
+        path='/v2/notifications?status=elephant',
+        headers=[('Content-Type', 'application/json'), auth_header])
+
+    json_response = json.loads(response.get_data(as_text=True))
+
+    assert response.status_code == 400
+    assert response.headers['Content-type'] == "application/json"
+
+    assert json_response['status_code'] == 400
+    assert len(json_response['errors']) == 1
+    assert json_response['errors'][0]['message'] == "elephant is not one of [created, sending, delivered, " \
+        "pending, failed, technical-failure, temporary-failure, permanent-failure]"
+
+
 def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, notify_db_session):
     notifications = [
         create_sample_notification(notify_db, notify_db_session, status=_status)
@@ -230,6 +301,19 @@ def test_get_all_notifications_filter_by_id(client, notify_db, notify_db_session
     assert json_response['notifications'][0]['id'] == str(older_notification.id)
 
 
+def test_get_all_notifications_filter_by_id_invalid_id(client, sample_notification):
+    auth_header = create_authorization_header(service_id=sample_notification.service_id)
+    response = client.get(
+        path='/v2/notifications?older_than=1234-badly-formatted-id-7890',
+        headers=[('Content-Type', 'application/json'), auth_header])
+
+    json_response = json.loads(response.get_data(as_text=True))
+
+    assert json_response['status_code'] == 400
+    assert len(json_response['errors']) == 1
+    assert json_response['errors'][0]['message'] == "older_than is not a valid UUID"
+
+
 def test_get_all_notifications_filter_by_id_no_notifications_if_nonexistent_id(client, notify_db, notify_db_session):
     notification = create_sample_notification(notify_db, notify_db_session)
 

tests/app/v2/notifications/test_notification_schemas.py

@@ -5,10 +5,80 @@ from flask import json
 from jsonschema import ValidationError
 
 from app.v2.notifications.notification_schemas import (
+    get_notifications_request,
     post_sms_request, post_sms_response, post_email_request, post_email_response
 )
 from app.schema_validation import validate
 
+
+@pytest.mark.parametrize('invalid_statuses, valid_statuses', [
+    # one invalid status
+    (["elephant"], []),
+    # multiple invalid statuses
+    (["elephant", "giraffe", "cheetah"], []),
+    # one bad status and one good status
+    (["elephant"], ["created"]),
+])
+def test_get_notifications_request_invalid_statuses(
+        invalid_statuses, valid_statuses
+):
+    partial_error_status = "is not one of " \
+        "[created, sending, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]"
+
+    with pytest.raises(ValidationError) as e:
+        validate({'status': invalid_statuses + valid_statuses}, get_notifications_request)
+
+    errors = json.loads(e.value.message).get('errors')
+    assert len(errors) == len(invalid_statuses)
+    for index, value in enumerate(invalid_statuses):
+        assert errors[index]['message'] == "{} {}".format(value, partial_error_status)
+
+
+@pytest.mark.parametrize('invalid_template_types, valid_template_types', [
+    # one invalid template_type
+    (["orange"], []),
+    # multiple invalid template_types
+    (["orange", "avocado", "banana"], []),
+    # one bad template_type and one good template_type
+    (["orange"], ["sms"]),
+])
+def test_get_notifications_request_invalid_template_types(
+        invalid_template_types, valid_template_types
+):
+    partial_error_template_type = "is not one of [sms, email, letter]"
+
+    with pytest.raises(ValidationError) as e:
+        validate({'template_type': invalid_template_types + valid_template_types}, get_notifications_request)
+
+    errors = json.loads(e.value.message).get('errors')
+    assert len(errors) == len(invalid_template_types)
+    for index, value in enumerate(invalid_template_types):
+        assert errors[index]['message'] == "{} {}".format(value, partial_error_template_type)
+
+
+def test_get_notifications_request_invalid_statuses_and_template_types():
+    with pytest.raises(ValidationError) as e:
+        validate({
+            'status': ["created", "elephant", "giraffe"],
+            'template_type': ["sms", "orange", "avocado"]
+        }, get_notifications_request)
+
+    errors = json.loads(e.value.message).get('errors')
+
+    assert len(errors) == 4
+
+    error_messages = [error['message'] for error in errors]
+
+    for invalid_status in ["elephant", "giraffe"]:
+        assert "{} is not one of [created, sending, delivered, " \
+            "pending, failed, technical-failure, temporary-failure, permanent-failure]".format(
+                invalid_status
+            ) in error_messages
+
+    for invalid_template_type in ["orange", "avocado"]:
+        assert "{} is not one of [sms, email, letter]".format(invalid_template_type) in error_messages
+
+
 valid_json = {"phone_number": "07515111111",
               "template_id": str(uuid.uuid4())
               }

tests/app/v2/notifications/test_post_notifications.py

@@ -7,23 +7,23 @@ from tests import create_authorization_header
 
 
 @pytest.mark.parametrize("reference", [None, "reference_from_client"])
-def test_post_sms_notification_returns_201(notify_api, sample_template, mocker, reference):
+def test_post_sms_notification_returns_201(notify_api, sample_template_with_placeholders, mocker, reference):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
             mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
             data = {
                 'phone_number': '+447700900855',
-                'template_id': str(sample_template.id)
+                'template_id': str(sample_template_with_placeholders.id),
+                'personalisation': {' Name': 'Jo'}
             }
             if reference:
                 data.update({"reference": reference})
-            auth_header = create_authorization_header(service_id=sample_template.service_id)
+            auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id)
 
             response = client.post(
                 path='/v2/notifications/sms',
                 data=json.dumps(data),
                 headers=[('Content-Type', 'application/json'), auth_header])
-
             assert response.status_code == 201
             resp_json = json.loads(response.get_data(as_text=True))
             notifications = Notification.query.all()
@@ -31,12 +31,12 @@ def test_post_sms_notification_returns_201(notify_api, sample_template, mocker,
             notification_id = notifications[0].id
             assert resp_json['id'] == str(notification_id)
             assert resp_json['reference'] == reference
-            assert resp_json['content']['body'] == sample_template.content
+            assert resp_json['content']['body'] == sample_template_with_placeholders.content.replace("(( Name))", "Jo")
-            assert resp_json['content']['from_number'] == sample_template.service.sms_sender
+            assert resp_json['content']['from_number'] == sample_template_with_placeholders.service.sms_sender
             assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri']
-            assert resp_json['template']['id'] == str(sample_template.id)
+            assert resp_json['template']['id'] == str(sample_template_with_placeholders.id)
-            assert resp_json['template']['version'] == sample_template.version
+            assert resp_json['template']['version'] == sample_template_with_placeholders.version
-            assert 'v2/templates/{}'.format(sample_template.id) in resp_json['template']['uri']
+            assert 'v2/templates/{}'.format(sample_template_with_placeholders.id) in resp_json['template']['uri']
             assert mocked.called
 
 
@@ -108,15 +108,16 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s
 
 
 @pytest.mark.parametrize("reference", [None, "reference_from_client"])
-def test_post_email_notification_returns_201(client, sample_email_template, mocker, reference):
+def test_post_email_notification_returns_201(client, sample_email_template_with_placeholders, mocker, reference):
     mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
     data = {
-        "email_address": sample_email_template.service.users[0].email_address,
+        "email_address": sample_email_template_with_placeholders.service.users[0].email_address,
-        "template_id": sample_email_template.id,
+        "template_id": sample_email_template_with_placeholders.id,
+        "personalisation": {"name": "Bob"}
     }
     if reference:
         data.update({"reference": reference})
-    auth_header = create_authorization_header(service_id=sample_email_template.service_id)
+    auth_header = create_authorization_header(service_id=sample_email_template_with_placeholders.service_id)
     response = client.post(
         path="v2/notifications/email",
         data=json.dumps(data),
@@ -127,13 +128,13 @@ def test_post_email_notification_returns_201(client, sample_email_template, mock
     assert resp_json['id'] == str(notification.id)
     assert resp_json['reference'] == reference
     assert notification.reference is None
-    assert resp_json['content']['body'] == sample_email_template.content
+    assert resp_json['content']['body'] == sample_email_template_with_placeholders.content.replace('((name))', 'Bob')
-    assert resp_json['content']['subject'] == sample_email_template.subject
+    assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject
-    assert resp_json['content']['from_email'] == sample_email_template.service.email_from
+    assert resp_json['content']['from_email'] == sample_email_template_with_placeholders.service.email_from
     assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri']
-    assert resp_json['template']['id'] == str(sample_email_template.id)
+    assert resp_json['template']['id'] == str(sample_email_template_with_placeholders.id)
-    assert resp_json['template']['version'] == sample_email_template.version
+    assert resp_json['template']['version'] == sample_email_template_with_placeholders.version
-    assert 'v2/templates/{}'.format(sample_email_template.id) in resp_json['template']['uri']
+    assert 'v2/templates/{}'.format(sample_email_template_with_placeholders.id) in resp_json['template']['uri']
     assert mocked.called