diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c354273a9..5aef56d93 100644 --- a/app/celery/tasks.py +++ b/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: diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index fa38da961..7f46a8a24 100644 --- a/app/clients/email/aws_ses.py +++ b/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': { - 'Text': { - 'Data': body}} + 'Body': body }, ReplyToAddresses=reply_to_addresses) elapsed_time = monotonic() - start_time diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f19d05dd0..93162729d 100644 --- a/app/dao/notifications_dao.py +++ b/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): - query = Notification.query.filter_by(service_id=service_id, job_id=job_id) \ - .order_by(desc(Notification.created_at)) \ - .paginate( +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 = filter_query(query, filter_dict) + 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): - query = Notification.query.filter_by(service_id=service_id).order_by(desc(Notification.created_at)).paginate( +def get_notifications_for_service(service_id, filter_dict=None, page=1): + 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 diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 100209cc7..0b8b18f01 100644 --- a/app/notifications/rest.py +++ b/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: - return jsonify(result="error", message="Invalid page"), 400 + page = data['page'] if 'page' in data else 1 - 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//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: - return jsonify(result="error", message="Invalid page"), 400 + page = data['page'] if 'page' in data else 1 - 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//job//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: - return jsonify(result="error", message="Invalid page"), 400 + page = data['page'] if 'page' in data else 1 - 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) diff --git a/app/schemas.py b/app/schemas.py index 575b23ea9..c812ca689 100644 --- a/app/schemas.py +++ b/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() diff --git a/requirements.txt b/requirements.txt index 405250156..118a5876a 100644 --- a/requirements.txt +++ b/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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 737f08864..767a4cb5c 100644 --- a/tests/app/celery/test_tasks.py +++ b/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 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 06cbc5223..dfb584820 100644 --- a/tests/app/notifications/test_rest.py +++ b/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_2 = 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_1 = create_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 = 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_2 = sample_service(notify_db, notify_db_session, service_name="2") + service_1 = create_sample_service(notify_db, notify_db_session, service_name="1") + 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_2 = sample_notification(notify_db, notify_db_session, service=service_1) - notification_3 = 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 = create_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) - another_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 = 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) - notification_2 = 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) + notification_2 = create_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) - email_template = sample_email_template(notify_db, notify_db_session, service=service) - sample_notification( + service = create_sample_service(notify_db, notify_db_session, limit=1) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + 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) - email_template = sample_email_template(notify_db, notify_db_session, service=service) - sms_template = sample_template(notify_db, notify_db_session, service=service) - sample_notification( + service = create_sample_service(notify_db, notify_db_session, limit=1) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + sms_template = create_sample_template(notify_db, notify_db_session, service=service) + 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) - email_template = sample_email_template(notify_db, notify_db_session, service=service) - sms_template = sample_template(notify_db, notify_db_session, service=service) - sample_notification(notify_db, notify_db_session, template=email_template, service=service) + service = create_sample_service(notify_db, notify_db_session, limit=2) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + sms_template = create_sample_template(notify_db, notify_db_session, 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') - notification2 = sample_notification(notify_db, notify_db_session, status='delivered') - notification3 = sample_notification(notify_db, notify_db_session, status='delivered') + notification1 = create_sample_notification(notify_db, notify_db_session, status='delivered') + notification2 = create_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 = sample_notification( + notification = create_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,