Merge branch 'master' into capture-delivery-outcomes

Conflicts:
	tests/app/notifications/test_rest.py

app/celery/tasks.py

@@ -267,7 +267,8 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not
                     from_address,
                     notification['to'],
                     subject,
-                    template.replaced
+                    body=template.replaced,
+                    html_body=template.as_HTML_email,
                 )
                 update_notification_reference_by_id(notification_id, reference)
             except AwsSesClientException as e:

app/clients/email/aws_ses.py

@@ -52,6 +52,7 @@ class AwsSesClient(EmailClient):
                    to_addresses,
                    subject,
                    body,
+                   html_body='',
                    reply_to_addresses=None):
         try:
             if isinstance(to_addresses, str):
@@ -61,6 +62,15 @@ class AwsSesClient(EmailClient):
             elif reply_to_addresses is None:
                 reply_to_addresses = []
 
+            body = {
+                'Text': {'Data': body}
+            }
+
+            if html_body:
+                body.update({
+                    'Html': {'Data': html_body}
+                })
+
             start_time = monotonic()
             response = self._client.send_email(
                 Source=source,
@@ -73,9 +83,7 @@ class AwsSesClient(EmailClient):
                     'Subject': {
                         'Data': subject,
                     },
-                    'Body': {
+                    'Body': body
-                        'Text': {
-                            'Data': body}}
                 },
                 ReplyToAddresses=reply_to_addresses)
             elapsed_time = monotonic() - start_time

app/dao/notifications_dao.py

@@ -1,6 +1,12 @@
 from flask import current_app
 from app import db
-from app.models import Notification, Job, NotificationStatistics, TEMPLATE_TYPE_SMS, TEMPLATE_TYPE_EMAIL
+from app.models import (
+    Notification,
+    Job,
+    NotificationStatistics,
+    TEMPLATE_TYPE_SMS,
+    TEMPLATE_TYPE_EMAIL,
+    Template)
 from sqlalchemy import desc
 from datetime import datetime, timedelta
 from app.clients import (STATISTICS_FAILURE, STATISTICS_DELIVERED, STATISTICS_REQUESTED)
@@ -131,14 +137,14 @@ def get_notification_for_job(service_id, job_id, notification_id):
     return Notification.query.filter_by(service_id=service_id, job_id=job_id, id=notification_id).one()
 
 
-def get_notifications_for_job(service_id, job_id, page=1):
+def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1):
-    query = Notification.query.filter_by(service_id=service_id, job_id=job_id) \
+    query = Notification.query.filter_by(service_id=service_id, job_id=job_id)
-        .order_by(desc(Notification.created_at)) \
+    query = filter_query(query, filter_dict)
-        .paginate(
+    pagination = query.order_by(desc(Notification.created_at)).paginate(
         page=page,
         per_page=current_app.config['PAGE_SIZE']
     )
-    return query
+    return pagination
 
 
 def get_notification(service_id, notification_id):
@@ -149,11 +155,21 @@ def get_notification_by_id(notification_id):
     return Notification.query.filter_by(id=notification_id).first()
 
 
-def get_notifications_for_service(service_id, page=1):
+def get_notifications_for_service(service_id, filter_dict=None, page=1):
-    query = Notification.query.filter_by(service_id=service_id).order_by(desc(Notification.created_at)).paginate(
+    query = Notification.query.filter_by(service_id=service_id)
+    query = filter_query(query, filter_dict)
+    pagination = query.order_by(desc(Notification.created_at)).paginate(
         page=page,
         per_page=current_app.config['PAGE_SIZE']
     )
+    return pagination
+
+
+def filter_query(query, filter_dict=None):
+    if filter_dict and 'status' in filter_dict:
+        query = query.filter_by(status=filter_dict['status'])
+    if filter_dict and 'template_type' in filter_dict:
+        query = query.join(Template).filter(Template.template_type == filter_dict['template_type'])
     return query
 
 

app/notifications/rest.py

@@ -23,7 +23,9 @@ from app.dao import (
 from app.schemas import (
     email_notification_schema,
     sms_template_notification_schema,
-    notification_status_schema
+    notification_status_schema,
+    template_schema,
+    notifications_filter_schema
 )
 from app.celery.tasks import send_sms, send_email
 from app.validation import allowed_send_to_number, allowed_send_to_email
@@ -218,16 +220,20 @@ def get_notifications(notification_id):
 
 @notifications.route('/notifications', methods=['GET'])
 def get_all_notifications():
-    page = get_page_from_request()
+    data, errors = notifications_filter_schema.load(request.args)
+    if errors:
+        return jsonify(result="error", message=errors), 400
 
-    if not page:
+    page = data['page'] if 'page' in data else 1
-        return jsonify(result="error", message="Invalid page"), 400
 
-    all_notifications = notifications_dao.get_notifications_for_service(api_user['client'], page)
+    pagination = notifications_dao.get_notifications_for_service(
+        api_user['client'],
+        filter_dict=data,
+        page=page)
     return jsonify(
-        notifications=notification_status_schema.dump(all_notifications.items, many=True).data,
+        notifications=notification_status_schema.dump(pagination.items, many=True).data,
         links=pagination_links(
-            all_notifications,
+            pagination,
             '.get_all_notifications',
             **request.args.to_dict()
         )
@@ -237,18 +243,22 @@ def get_all_notifications():
 @notifications.route('/service/<service_id>/notifications', methods=['GET'])
 @require_admin()
 def get_all_notifications_for_service(service_id):
-    page = get_page_from_request()
+    data, errors = notifications_filter_schema.load(request.args)
+    if errors:
+        return jsonify(result="error", message=errors), 400
 
-    if not page:
+    page = data['page'] if 'page' in data else 1
-        return jsonify(result="error", message="Invalid page"), 400
 
-    all_notifications = notifications_dao.get_notifications_for_service(service_id, page)
+    pagination = notifications_dao.get_notifications_for_service(
+        service_id,
+        filter_dict=data,
+        page=page)
     kwargs = request.args.to_dict()
     kwargs['service_id'] = service_id
     return jsonify(
-        notifications=notification_status_schema.dump(all_notifications.items, many=True).data,
+        notifications=notification_status_schema.dump(pagination.items, many=True).data,
         links=pagination_links(
-            all_notifications,
+            pagination,
             '.get_all_notifications_for_service',
             **kwargs
         )
@@ -258,36 +268,30 @@ def get_all_notifications_for_service(service_id):
 @notifications.route('/service/<service_id>/job/<job_id>/notifications', methods=['GET'])
 @require_admin()
 def get_all_notifications_for_service_job(service_id, job_id):
-    page = get_page_from_request()
+    data, errors = notifications_filter_schema.load(request.args)
+    if errors:
+        return jsonify(result="error", message=errors), 400
 
-    if not page:
+    page = data['page'] if 'page' in data else 1
-        return jsonify(result="error", message="Invalid page"), 400
 
-    all_notifications = notifications_dao.get_notifications_for_job(service_id, job_id, page)
+    pagination = notifications_dao.get_notifications_for_job(
+        service_id,
+        job_id,
+        filter_dict=data,
+        page=page)
     kwargs = request.args.to_dict()
     kwargs['service_id'] = service_id
     kwargs['job_id'] = job_id
     return jsonify(
-        notifications=notification_status_schema.dump(all_notifications.items, many=True).data,
+        notifications=notification_status_schema.dump(pagination.items, many=True).data,
         links=pagination_links(
-            all_notifications,
+            pagination,
             '.get_all_notifications_for_service_job',
             **kwargs
         )
     ), 200
 
 
-def get_page_from_request():
-    if 'page' in request.args:
-        try:
-            return int(request.args['page'])
-
-        except ValueError:
-            return None
-    else:
-        return 1
-
-
 def pagination_links(pagination, endpoint, **kwargs):
     if 'page' in kwargs:
         kwargs.pop('page', None)

app/schemas.py

@@ -218,6 +218,21 @@ class EmailDataSchema(ma.Schema):
         except InvalidEmailError as e:
             raise ValidationError(e.message)
 
+
+class NotificationsFilterSchema(ma.Schema):
+    template_type = field_for(models.Template, 'template_type', load_only=True, required=False)
+    status = field_for(models.Notification, 'status', load_only=True, required=False)
+    page = fields.Int(required=False)
+
+    @validates('page')
+    def validate_page(self, value):
+        try:
+            page_int = int(value)
+            if page_int < 1:
+                raise ValidationError("Not a positive integer")
+        except:
+            raise ValidationError("Not a positive integer")
+
 user_schema = UserSchema()
 user_schema_load_json = UserSchema(load_json=True)
 service_schema = ServiceSchema()
@@ -240,3 +255,4 @@ invited_user_schema = InvitedUserSchema()
 permission_schema = PermissionSchema()
 email_data_request_schema = EmailDataSchema()
 notifications_statistics_schema = NotificationsStatisticsSchema()
+notifications_filter_schema = NotificationsFilterSchema()

requirements.txt

@@ -21,4 +21,4 @@ monotonic==0.3
 
 git+https://github.com/alphagov/notifications-python-client.git@0.2.6#egg=notifications-python-client==0.2.6
 
-git+https://github.com/alphagov/notifications-utils.git@2.0.1#egg=notifications-utils==2.0.1
+git+https://github.com/alphagov/notifications-utils.git@3.1.0#egg=notifications-utils==3.1.0

tests/app/celery/test_tasks.py

@@ -37,6 +37,11 @@ from tests.app.conftest import (
 )
 
 
+class AnyStringWith(str):
+    def __eq__(self, other):
+        return self in other
+
+
 def firetext_error():
     return {'code': 0, 'description': 'error'}
 
@@ -390,7 +395,8 @@ def test_should_send_email_if_restricted_service_and_valid_email(notify_db, noti
         "email_from",
         "test@restricted.com",
         "subject",
-        template.content
+        body=template.content,
+        html_body=AnyStringWith(template.content)
     )
 
 
@@ -452,7 +458,8 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh
         "email_from",
         "my_email@my_email.com",
         "subject",
-        "Hello Jo"
+        body="Hello Jo",
+        html_body=AnyStringWith("Hello Jo")
     )
     persisted_notification = notifications_dao.get_notification(
         sample_email_template_with_placeholders.service_id, notification_id
@@ -515,7 +522,8 @@ def test_should_use_email_template_and_persist_without_personalisation(
         "email_from",
         "my_email@my_email.com",
         "subject",
-        "This is a template"
+        body="This is a template",
+        html_body=AnyStringWith("This is a template")
     )
 
 
@@ -577,7 +585,8 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai
         "email_from",
         "my_email@my_email.com",
         "subject",
-        sample_email_template.content
+        body=sample_email_template.content,
+        html_body=AnyStringWith(sample_email_template.content)
     )
     persisted_notification = notifications_dao.get_notification(sample_email_template.service_id, notification_id)
     assert persisted_notification.id == notification_id

tests/app/notifications/test_rest.py

@@ -2,7 +2,11 @@ from datetime import datetime
 import uuid
 import app.celery.tasks
 from tests import create_authorization_header
-from tests.app.conftest import sample_notification, sample_job, sample_service, sample_email_template, sample_template
+from tests.app.conftest import sample_notification as create_sample_notification
+from tests.app.conftest import sample_job as create_sample_job
+from tests.app.conftest import sample_service as create_sample_service
+from tests.app.conftest import sample_email_template as create_sample_email_template
+from tests.app.conftest import sample_template as create_sample_template
 from flask import json
 from app.models import Service
 from app.dao.templates_dao import dao_get_all_templates_for_service
@@ -87,9 +91,9 @@ def test_get_all_notifications(notify_api, sample_notification):
 def test_get_all_notifications_newest_first(notify_api, notify_db, notify_db_session, sample_email_template):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            notification_1 = sample_notification(notify_db, notify_db_session, sample_email_template.service)
+            notification_1 = create_sample_notification(notify_db, notify_db_session, sample_email_template.service)
-            notification_2 = sample_notification(notify_db, notify_db_session, sample_email_template.service)
+            notification_2 = create_sample_notification(notify_db, notify_db_session, sample_email_template.service)
-            notification_3 = sample_notification(notify_db, notify_db_session, sample_email_template.service)
+            notification_3 = create_sample_notification(notify_db, notify_db_session, sample_email_template.service)
 
             auth_header = create_authorization_header(
                 service_id=sample_email_template.service_id,
@@ -111,14 +115,14 @@ def test_get_all_notifications_newest_first(notify_api, notify_db, notify_db_ses
 def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notify_db_session):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            service_1 = sample_service(notify_db, notify_db_session, service_name="1")
+            service_1 = create_sample_service(notify_db, notify_db_session, service_name="1")
-            service_2 = sample_service(notify_db, notify_db_session, service_name="2")
+            service_2 = create_sample_service(notify_db, notify_db_session, service_name="2")
 
-            sample_notification(notify_db, notify_db_session, service=service_2)
+            create_sample_notification(notify_db, notify_db_session, service=service_2)
 
-            notification_1 = sample_notification(notify_db, notify_db_session, service=service_1)
+            notification_1 = create_sample_notification(notify_db, notify_db_session, service=service_1)
-            notification_2 = sample_notification(notify_db, notify_db_session, service=service_1)
+            notification_2 = create_sample_notification(notify_db, notify_db_session, service=service_1)
-            notification_3 = sample_notification(notify_db, notify_db_session, service=service_1)
+            notification_3 = create_sample_notification(notify_db, notify_db_session, service=service_1)
 
             auth_header = create_authorization_header(
                 path='/service/{}/notifications'.format(service_1.id),
@@ -139,19 +143,19 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif
 def test_get_all_notifications_for_job_in_order(notify_api, notify_db, notify_db_session, sample_service):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            main_job = sample_job(notify_db, notify_db_session, service=sample_service)
+            main_job = create_sample_job(notify_db, notify_db_session, service=sample_service)
-            another_job = sample_job(notify_db, notify_db_session, service=sample_service)
+            another_job = create_sample_job(notify_db, notify_db_session, service=sample_service)
 
-            notification_1 = sample_notification(
+            notification_1 = create_sample_notification(
                 notify_db, notify_db_session, job=main_job, to_field="1", created_at=datetime.utcnow()
             )
-            notification_2 = sample_notification(
+            notification_2 = create_sample_notification(
                 notify_db, notify_db_session, job=main_job, to_field="2", created_at=datetime.utcnow()
             )
-            notification_3 = sample_notification(
+            notification_3 = create_sample_notification(
                 notify_db, notify_db_session, job=main_job, to_field="3", created_at=datetime.utcnow()
             )
-            sample_notification(notify_db, notify_db_session, job=another_job)
+            create_sample_notification(notify_db, notify_db_session, job=another_job)
 
             auth_header = create_authorization_header(
                 path='/service/{}/job/{}/notifications'.format(sample_service.id, main_job.id),
@@ -220,7 +224,7 @@ def test_should_reject_invalid_page_param(notify_api, sample_email_template):
             notifications = json.loads(response.get_data(as_text=True))
             assert response.status_code == 400
             assert notifications['result'] == 'error'
-            assert notifications['message'] == 'Invalid page'
+            assert 'Not a valid integer.' in notifications['message']['page']
 
 
 def test_should_return_pagination_links(notify_api, notify_db, notify_db_session, sample_email_template):
@@ -228,9 +232,9 @@ def test_should_return_pagination_links(notify_api, notify_db, notify_db_session
         with notify_api.test_client() as client:
             notify_api.config['PAGE_SIZE'] = 1
 
-            sample_notification(notify_db, notify_db_session, sample_email_template.service)
+            create_sample_notification(notify_db, notify_db_session, sample_email_template.service)
-            notification_2 = sample_notification(notify_db, notify_db_session, sample_email_template.service)
+            notification_2 = create_sample_notification(notify_db, notify_db_session, sample_email_template.service)
-            sample_notification(notify_db, notify_db_session, sample_email_template.service)
+            create_sample_notification(notify_db, notify_db_session, sample_email_template.service)
 
             auth_header = create_authorization_header(
                 service_id=sample_email_template.service_id,
@@ -267,6 +271,102 @@ def test_get_all_notifications_returns_empty_list(notify_api, sample_api_key):
             assert len(notifications['notifications']) == 0
 
 
+def test_filter_by_template_type(notify_api, notify_db, notify_db_session, sample_template, sample_email_template):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+
+            notification_1 = create_sample_notification(
+                notify_db,
+                notify_db_session,
+                service=sample_email_template.service,
+                template=sample_template)
+            notification_2 = create_sample_notification(
+                notify_db,
+                notify_db_session,
+                service=sample_email_template.service,
+                template=sample_email_template)
+
+            auth_header = create_authorization_header(
+                service_id=sample_email_template.service_id,
+                path='/notifications',
+                method='GET')
+
+            response = client.get(
+                '/notifications?template_type=sms',
+                headers=[auth_header])
+
+            notifications = json.loads(response.get_data(as_text=True))
+            assert len(notifications['notifications']) == 1
+            assert notifications['notifications'][0]['template']['template_type'] == 'sms'
+            assert response.status_code == 200
+
+
+def test_filter_by_status(notify_api, notify_db, notify_db_session, sample_email_template):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+
+            notification_1 = create_sample_notification(
+                notify_db,
+                notify_db_session,
+                service=sample_email_template.service,
+                status="delivered")
+
+            auth_header = create_authorization_header(
+                service_id=sample_email_template.service_id,
+                path='/notifications',
+                method='GET')
+
+            response = client.get(
+                '/notifications?status=delivered',
+                headers=[auth_header])
+
+            notifications = json.loads(response.get_data(as_text=True))
+            assert len(notifications['notifications']) == 1
+            assert notifications['notifications'][0]['status'] == 'delivered'
+            assert response.status_code == 200
+
+
+def test_filter_by_status_and_template_type(notify_api,
+                                            notify_db,
+                                            notify_db_session,
+                                            sample_template,
+                                            sample_email_template):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+
+            notification_1 = create_sample_notification(
+                notify_db,
+                notify_db_session,
+                service=sample_email_template.service,
+                template=sample_template)
+            notification_2 = create_sample_notification(
+                notify_db,
+                notify_db_session,
+                service=sample_email_template.service,
+                template=sample_email_template)
+            notification_3 = create_sample_notification(
+                notify_db,
+                notify_db_session,
+                service=sample_email_template.service,
+                template=sample_email_template,
+                status="delivered")
+
+            auth_header = create_authorization_header(
+                service_id=sample_email_template.service_id,
+                path='/notifications',
+                method='GET')
+
+            response = client.get(
+                '/notifications?template_type=email&status=delivered',
+                headers=[auth_header])
+
+            notifications = json.loads(response.get_data(as_text=True))
+            assert len(notifications['notifications']) == 1
+            assert notifications['notifications'][0]['template']['template_type'] == 'email'
+            assert notifications['notifications'][0]['status'] == 'delivered'
+            assert response.status_code == 200
+
+
 def test_create_sms_should_reject_if_missing_required_fields(notify_api, sample_api_key, mocker):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
@@ -776,9 +876,9 @@ def test_should_block_api_call_if_over_day_limit(notify_db, notify_db_session, n
             mocker.patch('app.celery.tasks.send_email.apply_async')
             mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
 
-            service = sample_service(notify_db, notify_db_session, limit=1)
+            service = create_sample_service(notify_db, notify_db_session, limit=1)
-            email_template = sample_email_template(notify_db, notify_db_session, service=service)
+            email_template = create_sample_email_template(notify_db, notify_db_session, service=service)
-            sample_notification(
+            create_sample_notification(
                 notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow()
             )
 
@@ -811,10 +911,10 @@ def test_should_block_api_call_if_over_day_limit_regardless_of_type(notify_db, n
             mocker.patch('app.celery.tasks.send_sms.apply_async')
             mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
 
-            service = sample_service(notify_db, notify_db_session, limit=1)
+            service = create_sample_service(notify_db, notify_db_session, limit=1)
-            email_template = sample_email_template(notify_db, notify_db_session, service=service)
+            email_template = create_sample_email_template(notify_db, notify_db_session, service=service)
-            sms_template = sample_template(notify_db, notify_db_session, service=service)
+            sms_template = create_sample_template(notify_db, notify_db_session, service=service)
-            sample_notification(
+            create_sample_notification(
                 notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow()
             )
 
@@ -846,10 +946,10 @@ def test_should_allow_api_call_if_under_day_limit_regardless_of_type(notify_db,
             mocker.patch('app.celery.tasks.send_sms.apply_async')
             mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
 
-            service = sample_service(notify_db, notify_db_session, limit=2)
+            service = create_sample_service(notify_db, notify_db_session, limit=2)
-            email_template = sample_email_template(notify_db, notify_db_session, service=service)
+            email_template = create_sample_email_template(notify_db, notify_db_session, service=service)
-            sms_template = sample_template(notify_db, notify_db_session, service=service)
+            sms_template = create_sample_template(notify_db, notify_db_session, service=service)
-            sample_notification(notify_db, notify_db_session, template=email_template, service=service)
+            create_sample_notification(notify_db, notify_db_session, template=email_template, service=service)
 
             data = {
                 'to': '+447634123123',
@@ -1042,7 +1142,7 @@ def test_firetext_callback_should_update_notification_status_failed(notify_api,
 def test_firetext_callback_should_update_notification_status_sent(notify_api, notify_db, notify_db_session):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            notification = sample_notification(notify_db, notify_db_session, status='delivered')
+            notification = create_sample_notification(notify_db, notify_db_session, status='delivered')
             original = get_notification_by_id(notification.id)
             assert original.status == 'delivered'
 
@@ -1069,9 +1169,9 @@ def test_firetext_callback_should_update_notification_status_sent(notify_api, no
 def test_firetext_callback_should_update_multiple_notification_status_sent(notify_api, notify_db, notify_db_session):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            notification1 = sample_notification(notify_db, notify_db_session, status='delivered')
+            notification1 = create_sample_notification(notify_db, notify_db_session, status='delivered')
-            notification2 = sample_notification(notify_db, notify_db_session, status='delivered')
+            notification2 = create_sample_notification(notify_db, notify_db_session, status='delivered')
-            notification3 = sample_notification(notify_db, notify_db_session, status='delivered')
+            notification3 = create_sample_notification(notify_db, notify_db_session, status='delivered')
 
             client.post(
                 path='/notifications/sms/firetext',
@@ -1176,8 +1276,7 @@ def test_ses_callback_should_update_notification_status(
         sample_email_template):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-
+            notification = create_sample_notification(
-            notification = sample_notification(
                 notify_db,
                 notify_db_session,
                 template=sample_email_template,
@@ -1209,21 +1308,21 @@ def test_ses_callback_should_update_multiple_notification_status_sent(
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
 
-            notification1 = sample_notification(
+            notification1 = create_sample_notification(
                 notify_db,
                 notify_db_session,
                 template=sample_email_template,
                 reference='ref1',
                 status='delivered')
 
-            notification2 = sample_notification(
+            notification2 = create_sample_notification(
                 notify_db,
                 notify_db_session,
                 template=sample_email_template,
                 reference='ref2',
                 status='delivered')
 
-            notification3 = sample_notification(
+            notification3 = create_sample_notification(
                 notify_db,
                 notify_db_session,
                 template=sample_email_template,