From fac34aff101b310212deca4cac2f28874d5a2dcc Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 4 Apr 2016 13:13:29 +0100 Subject: [PATCH 01/10] Added functionality to allow filtering by multiple arguments. Removed commented out code. --- app/dao/notifications_dao.py | 15 +++- app/dao/permissions_dao.py | 6 +- app/schemas.py | 31 ++++++- tests/app/notifications/test_rest.py | 120 +++++++++++++++++++++++---- 4 files changed, 145 insertions(+), 27 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index e5787720f..d7b049290 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -6,6 +6,7 @@ from datetime import ( ) from flask import current_app +from werkzeug.datastructures import MultiDict from app import db from app.models import ( @@ -191,10 +192,16 @@ def get_notifications_for_service(service_id, filter_dict=None, page=1): 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']) + if filter_dict is None: + filter_dict = MultiDict() + else: + filter_dict = MultiDict(filter_dict) + statuses = filter_dict.getlist('status') if 'status' in filter_dict else None + if statuses: + query = query.filter(Notification.status.in_(statuses)) + template_types = filter_dict.getlist('template_type') if 'template_type' in filter_dict else None + if template_types: + query = query.join(Template).filter(Template.template_type.in_(template_types)) return query diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index 2213c0040..eca2446e9 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -32,8 +32,10 @@ class PermissionDAO(DAOClass): class Meta: model = Permission - def get_query(self, filter_by_dict={}): - if isinstance(filter_by_dict, dict): + def get_query(self, filter_by_dict=None): + if filter_by_dict is None: + filter_by_dict = MultiDict() + else: filter_by_dict = MultiDict(filter_by_dict) query = self.Meta.model.query if 'id' in filter_by_dict: diff --git a/app/schemas.py b/app/schemas.py index e943e8419..432296f4a 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -2,7 +2,7 @@ from flask_marshmallow.fields import fields from . import ma from . import models from app.dao.permissions_dao import permission_dao -from marshmallow import (post_load, ValidationError, validates, validates_schema) +from marshmallow import (post_load, ValidationError, validates, validates_schema, pre_load) from marshmallow_sqlalchemy import field_for from utils.recipients import ( validate_email_address, InvalidEmailError, @@ -89,6 +89,11 @@ class ServiceSchema(BaseSchema): raise ValidationError(error_msg, 'name') +class NotificationModelSchema(BaseSchema): + class Meta: + model = models.Notification + + class TemplateSchema(BaseSchema): class Meta: model = models.Template @@ -218,10 +223,29 @@ class EmailDataSchema(ma.Schema): 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) + template_type = fields.Nested(TemplateSchema, only='template_type', many=True) + status = fields.Nested(NotificationModelSchema, only='status', many=True) page = fields.Int(required=False) + @pre_load + def handle_multidict(self, in_data): + if isinstance(in_data, dict) and hasattr(in_data, 'getlist'): + out_data = dict([(k, in_data.get(k)) for k in in_data.keys()]) + if 'template_type' in in_data: + out_data['template_type'] = [{'template_type': x} for x in in_data.getlist('template_type')] + if 'status' in in_data: + out_data['status'] = [{"status": x} for x in in_data.getlist('status')] + + return out_data + + @post_load + def convert_schema_object_to_field(self, in_data): + if 'template_type' in in_data: + in_data['template_type'] = [x.template_type for x in in_data['template_type']] + if 'status' in in_data: + in_data['status'] = [x.status for x in in_data['status']] + return in_data + @validates('page') def validate_page(self, value): try: @@ -231,6 +255,7 @@ class NotificationsFilterSchema(ma.Schema): except: raise ValidationError("Not a positive integer") + user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) service_schema = ServiceSchema() diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index d255180a5..a7729974a 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -230,28 +230,34 @@ def test_should_reject_invalid_page_param(notify_api, sample_email_template): def test_should_return_pagination_links(notify_api, notify_db, notify_db_session, sample_email_template): with notify_api.test_request_context(): with notify_api.test_client() as client: - notify_api.config['PAGE_SIZE'] = 1 + # Effectively mocking page size + original_page_size = notify_api.config['PAGE_SIZE'] + try: + notify_api.config['PAGE_SIZE'] = 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) - create_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, - path='/notifications', - method='GET') + auth_header = create_authorization_header( + service_id=sample_email_template.service_id, + path='/notifications', + method='GET') - response = client.get( - '/notifications?page=2', - headers=[auth_header]) + response = client.get( + '/notifications?page=2', + headers=[auth_header]) - notifications = json.loads(response.get_data(as_text=True)) - assert len(notifications['notifications']) == 1 - assert notifications['links']['last'] == '/notifications?page=3' - assert notifications['links']['prev'] == '/notifications?page=1' - assert notifications['links']['next'] == '/notifications?page=3' - assert notifications['notifications'][0]['to'] == notification_2.to - assert response.status_code == 200 + notifications = json.loads(response.get_data(as_text=True)) + assert len(notifications['notifications']) == 1 + assert notifications['links']['last'] == '/notifications?page=3' + assert notifications['links']['prev'] == '/notifications?page=1' + assert notifications['links']['next'] == '/notifications?page=3' + assert notifications['notifications'][0]['to'] == notification_2.to + assert response.status_code == 200 + + finally: + notify_api.config['PAGE_SIZE'] = original_page_size def test_get_all_notifications_returns_empty_list(notify_api, sample_api_key): @@ -301,6 +307,41 @@ def test_filter_by_template_type(notify_api, notify_db, notify_db_session, sampl assert response.status_code == 200 +def test_filter_by_multiple_template_types(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&template_type=email', + headers=[auth_header]) + + assert response.status_code == 200 + notifications = json.loads(response.get_data(as_text=True)) + assert len(notifications['notifications']) == 2 + set(['sms', 'email']) == set( + [x['template']['template_type'] for x in notifications['notifications']]) + + 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: @@ -309,8 +350,15 @@ def test_filter_by_status(notify_api, notify_db, notify_db_session, sample_email notify_db, notify_db_session, service=sample_email_template.service, + template=sample_email_template, status="delivered") + 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', @@ -326,6 +374,42 @@ def test_filter_by_status(notify_api, notify_db, notify_db_session, sample_email assert response.status_code == 200 +def test_filter_by_multiple_statuss(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, + template=sample_email_template, + status="delivered") + + 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?status=delivered&status=sent', + headers=[auth_header]) + + assert response.status_code == 200 + notifications = json.loads(response.get_data(as_text=True)) + assert len(notifications['notifications']) == 2 + set(['delivered', 'sent']) == set( + [x['status'] for x in notifications['notifications']]) + + def test_filter_by_status_and_template_type(notify_api, notify_db, notify_db_session, From 0d06be05e1572c25ebefff5b7348b1a0050081b7 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 4 Apr 2016 12:21:38 +0100 Subject: [PATCH 02/10] [WIP] Added dao method and rest endpoint for getting template statistics by service. Some cosmetic changes to imports. Added fix for job rest not correctly returning errors. --- app/__init__.py | 2 + app/dao/notifications_dao.py | 94 ++++++++------ app/job/rest.py | 2 + app/models.py | 2 +- app/schemas.py | 35 ++++-- app/template_statistics/__init__.py | 0 app/template_statistics/rest.py | 36 ++++++ tests/app/dao/test_notification_dao.py | 127 ++++++++++++++++++- tests/app/template_statistics/__init__.py | 0 tests/app/template_statistics/test_rest.py | 136 +++++++++++++++++++++ 10 files changed, 390 insertions(+), 44 deletions(-) create mode 100644 app/template_statistics/__init__.py create mode 100644 app/template_statistics/rest.py create mode 100644 tests/app/template_statistics/__init__.py create mode 100644 tests/app/template_statistics/test_rest.py diff --git a/app/__init__.py b/app/__init__.py index 6802f5353..8dd8f120d 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -56,6 +56,7 @@ def create_app(app_name=None): from app.permission.rest import permission as permission_blueprint from app.accept_invite.rest import accept_invite from app.notifications_statistics.rest import notifications_statistics as notifications_statistics_blueprint + from app.template_statistics.rest import template_statistics as template_statistics_blueprint application.register_blueprint(service_blueprint, url_prefix='/service') application.register_blueprint(user_blueprint, url_prefix='/user') @@ -67,6 +68,7 @@ def create_app(app_name=None): application.register_blueprint(permission_blueprint, url_prefix='/permission') application.register_blueprint(accept_invite, url_prefix='/invite') application.register_blueprint(notifications_statistics_blueprint) + application.register_blueprint(template_statistics_blueprint) return application diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d7b049290..1c94fad53 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -25,6 +25,23 @@ from app.clients import ( STATISTICS_REQUESTED ) +from functools import wraps + + +def transactional(func): + @wraps(func) + def commit_or_rollback(*args, **kwargs): + from flask import current_app + from app import db + try: + func(*args, **kwargs) + db.session.commit() + except Exception as e: + current_app.logger.error(e) + db.session.rollback() + raise + return commit_or_rollback + def dao_get_notification_statistics_for_service(service_id): return NotificationStatistics.query.filter_by( @@ -39,46 +56,55 @@ def dao_get_notification_statistics_for_service_and_day(service_id, day): ).order_by(desc(NotificationStatistics.day)).first() +def dao_get_template_statistics_for_service(service_id, limit_days=None): + filter = [TemplateStatistics.service_id == service_id] + if limit_days: + latest_stat = TemplateStatistics.query.filter_by(service_id=service_id).order_by( + desc(TemplateStatistics.day)).limit(1).first() + if latest_stat: + last_date_to_fetch = latest_stat.day - timedelta(days=limit_days) + else: + last_date_to_fetch = date.today() - timedelta(days=limit_days) + filter.append(TemplateStatistics.day > last_date_to_fetch) + return TemplateStatistics.query.filter(*filter).order_by(desc(TemplateStatistics.day)).all() + + +@transactional def dao_create_notification(notification, notification_type): - try: - if notification.job_id: - db.session.query(Job).filter_by( - id=notification.job_id - ).update({ - Job.notifications_sent: Job.notifications_sent + 1, - Job.updated_at: datetime.utcnow() - }) + if notification.job_id: + db.session.query(Job).filter_by( + id=notification.job_id + ).update({ + Job.notifications_sent: Job.notifications_sent + 1, + Job.updated_at: datetime.utcnow() + }) - update_count = db.session.query(NotificationStatistics).filter_by( + update_count = db.session.query(NotificationStatistics).filter_by( + day=notification.created_at.strftime('%Y-%m-%d'), + service_id=notification.service_id + ).update(update_query(notification_type, 'requested')) + + if update_count == 0: + stats = NotificationStatistics( day=notification.created_at.strftime('%Y-%m-%d'), - service_id=notification.service_id - ).update(update_query(notification_type, 'requested')) - - if update_count == 0: - stats = NotificationStatistics( - day=notification.created_at.strftime('%Y-%m-%d'), - service_id=notification.service_id, - sms_requested=1 if notification_type == TEMPLATE_TYPE_SMS else 0, - emails_requested=1 if notification_type == TEMPLATE_TYPE_EMAIL else 0 - ) - db.session.add(stats) - - update_count = db.session.query(TemplateStatistics).filter_by( - day=date.today(), service_id=notification.service_id, - template_id=notification.template_id - ).update({'usage_count': TemplateStatistics.usage_count + 1}) + sms_requested=1 if notification_type == TEMPLATE_TYPE_SMS else 0, + emails_requested=1 if notification_type == TEMPLATE_TYPE_EMAIL else 0 + ) + db.session.add(stats) - if update_count == 0: - template_stats = TemplateStatistics(template_id=notification.template_id, - service_id=notification.service_id) - db.session.add(template_stats) + update_count = db.session.query(TemplateStatistics).filter_by( + day=date.today(), + service_id=notification.service_id, + template_id=notification.template_id + ).update({'usage_count': TemplateStatistics.usage_count + 1}) - db.session.add(notification) - db.session.commit() - except: - db.session.rollback() - raise + if update_count == 0: + template_stats = TemplateStatistics(template_id=notification.template_id, + service_id=notification.service_id) + db.session.add(template_stats) + + db.session.add(notification) def update_query(notification_type, status): diff --git a/app/job/rest.py b/app/job/rest.py index 78db23716..59eda8666 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -37,6 +37,8 @@ def get_job_by_service_and_job_id(service_id, job_id): def get_jobs_by_service(service_id): jobs = dao_get_jobs_by_service_id(service_id) data, errors = job_schema.dump(jobs, many=True) + if errors: + return jsonify(result="error", message=errors), 400 return jsonify(data=data) diff --git a/app/models.py b/app/models.py index 333d62a20..165903a90 100644 --- a/app/models.py +++ b/app/models.py @@ -357,7 +357,7 @@ class TemplateStatistics(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) - service = db.relationship('Service', backref=db.backref('template_statics', lazy='dynamic')) + service = db.relationship('Service', backref=db.backref('template_statistics', lazy='dynamic')) template_id = db.Column(db.BigInteger, db.ForeignKey('templates.id'), index=True, nullable=False, unique=False) template = db.relationship('Template') usage_count = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=1) diff --git a/app/schemas.py b/app/schemas.py index 1181a7bef..0f440a771 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -1,15 +1,27 @@ from flask_marshmallow.fields import fields -from . import ma -from . import models -from app.dao.permissions_dao import permission_dao -from marshmallow import (post_load, ValidationError, validates, validates_schema, pre_load) + +from marshmallow import ( + post_load, + ValidationError, + validates, + validates_schema, + pre_load +) + from marshmallow_sqlalchemy import field_for + from utils.recipients import ( - validate_email_address, InvalidEmailError, - validate_phone_number, InvalidPhoneError, + validate_email_address, + InvalidEmailError, + validate_phone_number, + InvalidPhoneError, validate_and_format_phone_number ) +from app import ma +from app import models +from app.dao.permissions_dao import permission_dao + # TODO I think marshmallow provides a better integration and error handling. # Would be better to replace functionality in dao with the marshmallow supported @@ -19,7 +31,7 @@ from utils.recipients import ( class BaseSchema(ma.ModelSchema): - def __init__(self, *args, load_json=False, **kwargs): + def __init__(self, load_json=False, *args, **kwargs): self.load_json = load_json super(BaseSchema, self).__init__(*args, **kwargs) @@ -241,6 +253,14 @@ class NotificationsFilterSchema(ma.Schema): raise ValidationError("Not a positive integer") +class TemplateStatisticsSchema(BaseSchema): + + template = fields.Nested(TemplateSchema, only=["id", "name", "template_type"], dump_only=True) + + class Meta: + model = models.TemplateStatistics + + user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) service_schema = ServiceSchema() @@ -264,3 +284,4 @@ permission_schema = PermissionSchema() email_data_request_schema = EmailDataSchema() notifications_statistics_schema = NotificationsStatisticsSchema() notifications_filter_schema = NotificationsFilterSchema() +template_statistics_schema = TemplateStatisticsSchema() diff --git a/app/template_statistics/__init__.py b/app/template_statistics/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py new file mode 100644 index 000000000..b935cdc06 --- /dev/null +++ b/app/template_statistics/rest.py @@ -0,0 +1,36 @@ +from flask import ( + Blueprint, + jsonify, + request, + current_app +) + +from app.dao.notifications_dao import dao_get_template_statistics_for_service + +from app.schemas import template_statistics_schema + +template_statistics = Blueprint('template-statistics', + __name__, + url_prefix='/service//template-statistics') + +from app.errors import register_errors + +register_errors(template_statistics) + + +@template_statistics.route('') +def get_template_statistics_for_service(service_id): + if request.args.get('limit_days'): + try: + limit_days = int(request.args['limit_days']) + except ValueError as e: + error = 'Limit days {} is not an integer'.format(request.args['limit_days']) + current_app.logger.error(error) + return jsonify(result="error", message=[error]), 400 + else: + limit_days = None + stats = dao_get_template_statistics_for_service(service_id, limit_days=limit_days) + data, errors = template_statistics_schema.dump(stats, many=True) + if errors: + return jsonify(result="error", message=errors), 400 + return jsonify(data=data) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 83d79bd33..958b56a06 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -28,7 +28,8 @@ from app.dao.notifications_dao import ( dao_get_notification_statistics_for_service_and_day, update_notification_status_by_id, update_notification_reference_by_id, - update_notification_status_by_reference + update_notification_status_by_reference, + dao_get_template_statistics_for_service ) from tests.app.conftest import sample_job @@ -886,10 +887,132 @@ def test_successful_notification_inserts_followed_by_failure_does_not_increment_ db.session.execute('DROP TABLE TEMPLATE_STATISTICS') dao_create_notification(failing_notification, sample_template.template_type) except Exception as e: - # There should be no additional notification stats or counts assert NotificationStatistics.query.count() == 1 notication_stats = NotificationStatistics.query.filter( NotificationStatistics.service_id == sample_template.service.id ).first() assert notication_stats.sms_requested == 3 + + +@freeze_time("2016-03-30") +def test_get_template_stats_for_service_returns_stats_in_reverse_date_order(sample_template, sample_job): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': sample_template.service, + 'service_id': sample_template.service.id, + 'template': sample_template, + 'template_id': sample_template.id, + 'created_at': datetime.utcnow() + } + + notification = Notification(**data) + dao_create_notification(notification, sample_template.template_type) + + # move on one day + with freeze_time('2016-03-31'): + new_notification = Notification(**data) + dao_create_notification(new_notification, sample_template.template_type) + + # move on one more day + with freeze_time('2016-04-01'): + new_notification = Notification(**data) + dao_create_notification(new_notification, sample_template.template_type) + + template_stats = dao_get_template_statistics_for_service(sample_template.service_id) + assert len(template_stats) == 3 + assert template_stats[0].day == date(2016, 4, 1) + assert template_stats[1].day == date(2016, 3, 31) + assert template_stats[2].day == date(2016, 3, 30) + + +@freeze_time('2016-04-09') +def test_get_template_stats_for_service_returns_stats_can_limit_number_of_days_returned(sample_template): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + # Retrieve last week of stats + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=7) + assert len(template_stats) == 7 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[6].day == date(2016, 4, 3) + + +@freeze_time('2016-04-09') +def test_get_template_stats_for_service_returns_stats_returns_all_stats_if_no_limit(sample_template): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + template_stats = dao_get_template_statistics_for_service(sample_template.service_id) + assert len(template_stats) == 9 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[8].day == date(2016, 4, 1) + + +@freeze_time('2016-04-30') +def test_get_template_stats_for_service_returns_results_from_first_day_with_data(sample_template): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + + # make 9 stats records from 1st to 9th April - no data after 10th + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + # Retrieve one day of stats - read date is 2016-04-30 + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=1) + assert len(template_stats) == 1 + assert template_stats[0].day == date(2016, 4, 9) + + # Retrieve three days of stats + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=3) + assert len(template_stats) == 3 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[1].day == date(2016, 4, 8) + assert template_stats[2].day == date(2016, 4, 7) + + # Retrieve nine days of stats + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=9) + assert len(template_stats) == 9 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[8].day == date(2016, 4, 1) + + # Retrieve with no limit + template_stats = dao_get_template_statistics_for_service(sample_template.service_id) + assert len(template_stats) == 9 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[8].day == date(2016, 4, 1) + + +def test_get_template_stats_for_service_with_limit_if_no_records_returns_empty_list(sample_template): + template_stats = dao_get_template_statistics_for_service(sample_template.service.id, limit_days=7) + assert len(template_stats) == 0 diff --git a/tests/app/template_statistics/__init__.py b/tests/app/template_statistics/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py new file mode 100644 index 000000000..51826d90a --- /dev/null +++ b/tests/app/template_statistics/test_rest.py @@ -0,0 +1,136 @@ +import json +from freezegun import freeze_time + +from app import db +from app.models import TemplateStatistics + +from tests import create_authorization_header + + +@freeze_time('2016-04-09') +def test_get_template_statistics_for_service_for_last_week(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 7} + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 7 + assert json_resp['data'][0]['day'] == '2016-04-09' + assert json_resp['data'][6]['day'] == '2016-04-03' + + +@freeze_time('2016-04-30') +def test_get_template_statistics_for_service_for_last_actual_week_with_data(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + # Date is frozen at 2016-04-30 and no data written since + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 7} + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 7 + assert json_resp['data'][0]['day'] == '2016-04-09' + assert json_resp['data'][6]['day'] == '2016-04-03' + + +def test_get_all_template_statistics_for_service(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 9 + assert json_resp['data'][0]['day'] == '2016-04-09' + assert json_resp['data'][8]['day'] == '2016-04-01' + + +def test_get_all_template_statistics_with_bad_limit_arg_returns_400(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 'blurk'} + ) + + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == ['Limit days blurk is not an integer'] From e4a5e3890a4e0a3e260742b5988d0cb20aa35fdf Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 4 Apr 2016 14:51:56 +0100 Subject: [PATCH 03/10] Corrected error message format --- app/template_statistics/rest.py | 4 ++-- tests/app/template_statistics/test_rest.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index b935cdc06..3446d2623 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -24,9 +24,9 @@ def get_template_statistics_for_service(service_id): try: limit_days = int(request.args['limit_days']) except ValueError as e: - error = 'Limit days {} is not an integer'.format(request.args['limit_days']) + error = '{} is not an integer'.format(request.args['limit_days']) current_app.logger.error(error) - return jsonify(result="error", message=[error]), 400 + return jsonify(result="error", message={'limit_days': [error]}), 400 else: limit_days = None stats = dao_get_template_statistics_for_service(service_id, limit_days=limit_days) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 51826d90a..20c99ba67 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -133,4 +133,4 @@ def test_get_all_template_statistics_with_bad_limit_arg_returns_400(notify_api, assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert json_resp['message'] == ['Limit days blurk is not an integer'] + assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} From f6620792b5b5e483bed12c0642487927413ffd37 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 4 Apr 2016 17:51:24 +0100 Subject: [PATCH 04/10] Additional sort order by template name for template statistics. --- app/dao/notifications_dao.py | 9 +++++-- tests/app/dao/test_notification_dao.py | 36 ++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1c94fad53..a4e657b41 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,4 +1,8 @@ -from sqlalchemy import desc +from sqlalchemy import ( + desc, + func +) + from datetime import ( datetime, timedelta, @@ -66,7 +70,8 @@ def dao_get_template_statistics_for_service(service_id, limit_days=None): else: last_date_to_fetch = date.today() - timedelta(days=limit_days) filter.append(TemplateStatistics.day > last_date_to_fetch) - return TemplateStatistics.query.filter(*filter).order_by(desc(TemplateStatistics.day)).all() + return TemplateStatistics.query.filter(*filter).order_by( + desc(TemplateStatistics.day)).join(Template).order_by(func.lower(Template.name)).all() @transactional diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 958b56a06..f042e52e3 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1016,3 +1016,39 @@ def test_get_template_stats_for_service_returns_results_from_first_day_with_data def test_get_template_stats_for_service_with_limit_if_no_records_returns_empty_list(sample_template): template_stats = dao_get_template_statistics_for_service(sample_template.service.id, limit_days=7) assert len(template_stats) == 0 + + +def test_get_template_stats_for_service_for_day_returns_stats_in_template_name_order(sample_service, sample_job): + + from app.dao.templates_dao import dao_create_template + from app.models import Template + + template_stats = dao_get_template_statistics_for_service(sample_service.id) + assert len(template_stats) == 0 + + template_names = ['Aardvark', 'ant', 'zebra', 'walrus', 'Donkey', 'Komodo dragon'] + for name in template_names: + data = { + 'name': name, + 'template_type': 'sms', + 'content': 'blah', + 'service': sample_service + } + template = Template(**data) + dao_create_template(template) + + notification_data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': template.service, + 'service_id': template.service.id, + 'template': template, + 'template_id': template.id, + 'created_at': datetime.utcnow() + } + notification = Notification(**notification_data) + dao_create_notification(notification, template.template_type) + template_stats = dao_get_template_statistics_for_service(template.service.id) + + for i, name in enumerate(sorted(template_names, key=str.lower)): + assert template_stats[i].template.name == name From e56aee5d1d77142b78434f3faeddfd2cb1a6e9bf Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 4 Apr 2016 15:02:25 +0100 Subject: [PATCH 05/10] Validate recipient for restricted service w/ utils MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements https://github.com/alphagov/notifications-utils/pull/16 Once https://github.com/alphagov/notifications-admin/pull/376 is merged it will no longer be possible for a user to upload a CSV file containing recipients that they’re not allowed to send to. So this commit also removes any restricted service checks in the task, because any public phone numbers/email addresses no longer have any way of reach this point if the service is restricted. --- app/celery/tasks.py | 119 ++++++++++----------------- app/notifications/rest.py | 21 +++-- app/validation.py | 15 ---- requirements.txt | 2 +- tests/app/celery/test_tasks.py | 25 ------ tests/app/notifications/test_rest.py | 4 +- tests/app/test_validation.py | 65 --------------- 7 files changed, 62 insertions(+), 189 deletions(-) delete mode 100644 app/validation.py delete mode 100644 tests/app/test_validation.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index fd8ccb3a9..d41271e06 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -49,11 +49,6 @@ from app.models import ( TEMPLATE_TYPE_SMS ) -from app.validation import ( - allowed_send_to_email, - allowed_send_to_number -) - @notify_celery.task(name="delete-verify-codes") def delete_verify_codes(): @@ -197,16 +192,6 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): client = firetext_client try: - status = 'sent' - can_send = True - - if not allowed_send_to_number(service, notification['to']): - current_app.logger.info( - "SMS {} failed as restricted service".format(notification_id) - ) - status = 'failed' - can_send = False - sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, @@ -214,7 +199,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status=status, + status='sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() @@ -222,30 +207,29 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) - if can_send: - try: - template = Template( - dao_get_template_by_id(notification['template']).__dict__, - values=notification.get('personalisation', {}), - prefix=service.name - ) - - client.send_sms( - to=validate_and_format_phone_number(notification['to']), - content=template.replaced, - reference=str(notification_id) - ) - except FiretextClientException as e: - current_app.logger.error( - "SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - notification_db_object.status = 'failed' - dao_update_notification(notification_db_object) - - current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) + try: + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}), + prefix=service.name ) + + client.send_sms( + to=validate_and_format_phone_number(notification['to']), + content=template.replaced, + reference=str(notification_id) + ) + except FiretextClientException as e: + current_app.logger.error( + "SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) + + current_app.logger.info( + "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) @@ -253,21 +237,9 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): @notify_celery.task(name="send-email") def send_email(service_id, notification_id, subject, from_address, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) - service = dao_fetch_service_by_id(service_id) - client = aws_ses_client try: - status = 'sent' - can_send = True - - if not allowed_send_to_email(service, notification['to']): - current_app.logger.info( - "Email {} failed as restricted service".format(notification_id) - ) - status = 'failed' - can_send = False - sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, @@ -275,36 +247,35 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status=status, + status='sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_EMAIL) - if can_send: - try: - template = Template( - dao_get_template_by_id(notification['template']).__dict__, - values=notification.get('personalisation', {}) - ) - - reference = client.send_email( - from_address, - notification['to'], - subject, - body=template.replaced, - html_body=template.as_HTML_email, - ) - update_notification_reference_by_id(notification_id, reference) - except AwsSesClientException as e: - current_app.logger.exception(e) - notification_db_object.status = 'failed' - dao_update_notification(notification_db_object) - - current_app.logger.info( - "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) + try: + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}) ) + + reference = client.send_email( + from_address, + notification['to'], + subject, + body=template.replaced, + html_body=template.as_HTML_email, + ) + update_notification_reference_by_id(notification_id, reference) + except AwsSesClientException as e: + current_app.logger.exception(e) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) + + current_app.logger.info( + "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 3e536b4ed..edd4a4e51 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,5 +1,6 @@ from datetime import datetime import uuid +import itertools from flask import ( Blueprint, @@ -11,6 +12,7 @@ from flask import ( ) from utils.template import Template +from utils.recipients import allowed_to_send_to, first_column_heading from app.clients.sms.firetext import FiretextResponses from app.clients.email.aws_ses import AwsSesResponses from app import api_user, encryption, create_uuid, DATETIME_FORMAT, DATE_FORMAT @@ -28,7 +30,6 @@ from app.schemas import ( notifications_filter_schema ) from app.celery.tasks import send_sms, send_email -from app.validation import allowed_send_to_number, allowed_send_to_email notifications = Blueprint('notifications', __name__) @@ -356,12 +357,21 @@ def send_notification(notification_type): } ), 400 + if service.restricted and not allowed_to_send_to( + notification['to'], + itertools.chain.from_iterable( + [user.mobile_number, user.email_address] for user in service.users + ) + ): + return jsonify( + result="error", message={ + 'to': ['Invalid {} for restricted service'.format(first_column_heading[notification_type])] + } + ), 400 + notification_id = create_uuid() if notification_type == 'sms': - if not allowed_send_to_number(service, notification['to']): - return jsonify( - result="error", message={'to': ['Invalid phone number for restricted service']}), 400 send_sms.apply_async(( service_id, notification_id, @@ -369,9 +379,6 @@ def send_notification(notification_type): datetime.utcnow().strftime(DATETIME_FORMAT) ), queue='sms') else: - if not allowed_send_to_email(service, notification['to']): - return jsonify( - result="error", message={'to': ['Email address not permitted for restricted service']}), 400 send_email.apply_async(( service_id, notification_id, diff --git a/app/validation.py b/app/validation.py deleted file mode 100644 index 49a6b0664..000000000 --- a/app/validation.py +++ /dev/null @@ -1,15 +0,0 @@ -from utils.recipients import format_phone_number, validate_phone_number - - -def allowed_send_to_number(service, to): - if service.restricted and format_phone_number(validate_phone_number(to)) not in [ - format_phone_number(validate_phone_number(user.mobile_number)) for user in service.users - ]: - return False - return True - - -def allowed_send_to_email(service, to): - if service.restricted and to not in [user.email_address for user in service.users]: - return False - return True diff --git a/requirements.txt b/requirements.txt index 3f4ed5ceb..7edbd4718 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@3.1.3#egg=notifications-utils==3.1.3 +git+https://github.com/alphagov/notifications-utils.git@3.2.0#egg=notifications-utils==3.2.0 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 767a4cb5c..39a755a71 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -343,31 +343,6 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif ) -def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): - user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template(notify_db, notify_db_session, service=service) - - notification = { - "template": template.id, - "to": "07700 900849" - } - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.firetext_client.send_sms') - mocker.patch('app.firetext_client.get_name', return_value="firetext") - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - service.id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - - firetext_client.send_sms.assert_not_called() - - def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, email="test@restricted.com") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 4882ba420..5f55059a1 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -879,7 +879,7 @@ def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Email address not permitted for restricted service' in json_resp['message']['to'] + assert 'Invalid email address for restricted service' in json_resp['message']['to'] def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user( @@ -915,7 +915,7 @@ def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user( app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Email address not permitted for restricted service' in json_resp['message']['to'] + assert 'Invalid email address for restricted service' in json_resp['message']['to'] @freeze_time("2016-01-01 11:09:00.061258") diff --git a/tests/app/test_validation.py b/tests/app/test_validation.py deleted file mode 100644 index 9c822533d..000000000 --- a/tests/app/test_validation.py +++ /dev/null @@ -1,65 +0,0 @@ -from app.models import User, Service -from app.validation import allowed_send_to_number, allowed_send_to_email - - -def test_allowed_send_to_number_returns_true_for_restricted_service_with_same_number(): - mobile_number = '07524609792' - service = _create_service_data(mobile_number) - assert allowed_send_to_number(service, mobile_number) - - -def test_allowed_send_to_number_returns_false_for_restricted_service_with_different_number(): - mobile_number = '00447524609792' - service = _create_service_data(mobile_number) - assert not allowed_send_to_number(service, '+447344609793') - - -def test_allowed_send_to_number_returns_true_for_unrestricted_service_with_different_number(): - mobile_number = '+447524609792' - service = _create_service_data(mobile_number, False) - assert allowed_send_to_number(service, '+447344609793') - - -def test_allowed_send_to_email__returns_true_for_restricted_service_with_same_email(): - email = 'testing@it.gov.uk' - service = _create_service_data(email_address=email) - assert allowed_send_to_email(service, email) - - -def test_allowed_send_to_email__returns_false_for_restricted_service_with_different_email(): - email = 'testing@it.gov.uk' - service = _create_service_data(email_address=email) - assert not allowed_send_to_email(service, 'another@it.gov.uk') - - -def test_allowed_send_to_email__returns_false_for_restricted_service_with_different_email(): - email = 'testing@it.gov.uk' - service = _create_service_data(email_address=email) - assert not allowed_send_to_email(service, 'another@it.gov.uk') - - -def test_allowed_send_to_email__returns_true_for_unrestricted_service_with_different_email(): - email = 'testing@it.gov.uk' - service = _create_service_data(email_address=email, restricted=False) - assert allowed_send_to_number(service, 'another@it.gov.uk') - - -def _create_service_data(mobile_number='+447524609792', restricted=True, email_address='test_user@it.gov.uk'): - usr = { - 'name': 'Test User', - 'email_address': email_address, - 'password': 'password', - 'mobile_number': mobile_number, - 'state': 'active' - } - user = User(**usr) - data = { - 'name': 'Test service', - 'limit': 10, - 'active': False, - 'restricted': restricted, - 'email_from': 'test_service@it.gov.uk' - } - service = Service(**data) - service.users = [user] - return service From 4d15409781192e48fd8ce0c46c7361275b1f72f8 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 5 Apr 2016 11:07:21 +0100 Subject: [PATCH 06/10] Successful notifications are deleted after a week now. All tests passing. --- app/celery/tasks.py | 7 +++---- app/dao/notifications_dao.py | 8 ++++---- tests/app/celery/test_tasks.py | 12 ++++++------ tests/app/dao/test_notification_dao.py | 16 ++++++++-------- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d41271e06..f07e3fe09 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -32,8 +32,7 @@ from app.dao.invited_user_dao import delete_invitations_created_more_than_two_da from app.dao.notifications_dao import ( dao_create_notification, dao_update_notification, - delete_failed_notifications_created_more_than_a_week_ago, - delete_successful_notifications_created_more_than_a_day_ago, + delete_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, update_notification_reference_by_id ) @@ -67,7 +66,7 @@ def delete_verify_codes(): def delete_successful_notifications(): try: start = datetime.utcnow() - deleted = delete_successful_notifications_created_more_than_a_day_ago() + deleted = delete_notifications_created_more_than_a_week_ago('sent') current_app.logger.info( "Delete job started {} finished {} deleted {} successful notifications".format( start, @@ -84,7 +83,7 @@ def delete_successful_notifications(): def delete_failed_notifications(): try: start = datetime.utcnow() - deleted = delete_failed_notifications_created_more_than_a_week_ago() + deleted = delete_notifications_created_more_than_a_week_ago('failed') current_app.logger.info( "Delete job started {} finished {} deleted {} failed notifications".format( start, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index a4e657b41..6196a7842 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -236,19 +236,19 @@ def filter_query(query, filter_dict=None): return query -def delete_successful_notifications_created_more_than_a_day_ago(): +def delete_notifications_created_more_than_a_day_ago(status): deleted = db.session.query(Notification).filter( Notification.created_at < datetime.utcnow() - timedelta(days=1), - Notification.status == 'sent' + Notification.status == status ).delete() db.session.commit() return deleted -def delete_failed_notifications_created_more_than_a_week_ago(): +def delete_notifications_created_more_than_a_week_ago(status): deleted = db.session.query(Notification).filter( Notification.created_at < datetime.utcnow() - timedelta(days=7), - Notification.status == 'failed' + Notification.status == status ).delete() db.session.commit() return deleted diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 39a755a71..7554c6bc0 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -46,16 +46,16 @@ def firetext_error(): return {'code': 0, 'description': 'error'} -def test_should_call_delete_successful_notifications_in_task(notify_api, mocker): - mocker.patch('app.celery.tasks.delete_successful_notifications_created_more_than_a_day_ago') +def test_should_call_delete_notifications_more_than_week_in_task(notify_api, mocker): + mocker.patch('app.celery.tasks.delete_notifications_created_more_than_a_week_ago') delete_successful_notifications() - assert tasks.delete_successful_notifications_created_more_than_a_day_ago.call_count == 1 + assert tasks.delete_notifications_created_more_than_a_week_ago.call_count == 1 -def test_should_call_delete_failed_notifications_in_task(notify_api, mocker): - mocker.patch('app.celery.tasks.delete_failed_notifications_created_more_than_a_week_ago') +def test_should_call_delete_notifications_more_than_week_in_task(notify_api, mocker): + mocker.patch('app.celery.tasks.delete_notifications_created_more_than_a_week_ago') delete_failed_notifications() - assert tasks.delete_failed_notifications_created_more_than_a_week_ago.call_count == 1 + assert tasks.delete_notifications_created_more_than_a_week_ago.call_count == 1 def test_should_call_delete_codes_on_delete_verify_codes_task(notify_api, mocker): diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index f042e52e3..5f7d80cd0 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -23,8 +23,8 @@ from app.dao.notifications_dao import ( get_notification_for_job, get_notifications_for_job, dao_get_notification_statistics_for_service, - delete_successful_notifications_created_more_than_a_day_ago, - delete_failed_notifications_created_more_than_a_week_ago, + delete_notifications_created_more_than_a_day_ago, + delete_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, update_notification_status_by_id, update_notification_reference_by_id, @@ -701,21 +701,21 @@ def test_update_notification(sample_notification, sample_template): assert notification_from_db.status == 'failed' -def test_should_delete_sent_notifications_after_one_day(notify_db, notify_db_session): +def test_should_delete_notifications_after_one_day(notify_db, notify_db_session): created_at = datetime.utcnow() - timedelta(hours=24) sample_notification(notify_db, notify_db_session, created_at=created_at) sample_notification(notify_db, notify_db_session, created_at=created_at) assert len(Notification.query.all()) == 2 - delete_successful_notifications_created_more_than_a_day_ago() + delete_notifications_created_more_than_a_day_ago('sent') assert len(Notification.query.all()) == 0 -def test_should_delete_failed_notifications_after_seven_days(notify_db, notify_db_session): +def test_should_delete_notifications_after_seven_days(notify_db, notify_db_session): created_at = datetime.utcnow() - timedelta(hours=24 * 7) sample_notification(notify_db, notify_db_session, created_at=created_at, status="failed") sample_notification(notify_db, notify_db_session, created_at=created_at, status="failed") assert len(Notification.query.all()) == 2 - delete_failed_notifications_created_more_than_a_week_ago() + delete_notifications_created_more_than_a_week_ago('failed') assert len(Notification.query.all()) == 0 @@ -726,7 +726,7 @@ def test_should_not_delete_sent_notifications_before_one_day(notify_db, notify_d sample_notification(notify_db, notify_db_session, created_at=valid, to_field="valid") assert len(Notification.query.all()) == 2 - delete_successful_notifications_created_more_than_a_day_ago() + delete_notifications_created_more_than_a_day_ago('sent') assert len(Notification.query.all()) == 1 assert Notification.query.first().to == 'valid' @@ -737,7 +737,7 @@ def test_should_not_delete_failed_notifications_before_seven_days(notify_db, not sample_notification(notify_db, notify_db_session, created_at=expired, status="failed", to_field="expired") sample_notification(notify_db, notify_db_session, created_at=valid, status="failed", to_field="valid") assert len(Notification.query.all()) == 2 - delete_failed_notifications_created_more_than_a_week_ago() + delete_notifications_created_more_than_a_week_ago('failed') assert len(Notification.query.all()) == 1 assert Notification.query.first().to == 'valid' From 7f7c5bc9d3b0f98347f11c78e050c4e21c52c23c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 5 Apr 2016 12:00:31 +0100 Subject: [PATCH 07/10] Fix bug with whitelist Implements https://github.com/alphagov/notifications-utils/pull/17 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 7edbd4718..6483a8296 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@3.2.0#egg=notifications-utils==3.2.0 +git+https://github.com/alphagov/notifications-utils.git@3.2.1#egg=notifications-utils==3.2.1 From 4cc0028b011eb0ea7c48d84dd6d919ae465e33d3 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 5 Apr 2016 14:28:19 +0100 Subject: [PATCH 08/10] Remove csv after process job is finished. Fixed new tests. --- app/aws/s3.py | 15 ++++++++++++--- app/celery/tasks.py | 8 ++++++++ config.py | 1 + tests/app/celery/test_tasks.py | 30 +++++++++++++++++++++++------- tests/app/conftest.py | 5 +++++ 5 files changed, 49 insertions(+), 10 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index ea7df1f31..fd316cf1e 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,7 +1,16 @@ from boto3 import resource -def get_job_from_s3(bucket_name, job_id): +def get_s3_job_object(bucket_name, job_id): s3 = resource('s3') - key = s3.Object(bucket_name, '{}.csv'.format(job_id)) - return key.get()['Body'].read().decode('utf-8') + return s3.Object(bucket_name, '{}.csv'.format(job_id)) + + +def get_job_from_s3(bucket_name, job_id): + obj = get_s3_job_object(bucket_name, job_id) + return obj.get()['Body'].read().decode('utf-8') + + +def remove_job_from_s3(bucket_name, job_id): + obj = get_s3_job_object(bucket_name, job_id) + return obj.delete() diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d41271e06..29c4ba8ef 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -179,11 +179,19 @@ def process_job(job_id): job.processing_started = start job.processing_finished = finished dao_update_job(job) + remove_job.apply_async((str(job_id),), queue='remove-job') current_app.logger.info( "Job {} created at {} started at {} finished at {}".format(job_id, job.created_at, start, finished) ) +@notify_celery.task(name="remove-job") +def remove_job(job_id): + job = dao_get_job_by_id(job_id) + s3.remove_job_from_s3(job.bucket_name, job_id) + current_app.logger.info("Job {} has been removed from s3.".format(job_id)) + + @notify_celery.task(name="send-sms") def send_sms(service_id, notification_id, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) diff --git a/config.py b/config.py index 0bf45618c..e38251b28 100644 --- a/config.py +++ b/config.py @@ -67,6 +67,7 @@ class Config(object): Queue('email-code', Exchange('default'), routing_key='email-code'), Queue('email-reset-password', Exchange('default'), routing_key='email-reset-password'), Queue('process-job', Exchange('default'), routing_key='process-job'), + Queue('remove-job', Exchange('default'), routing_key='remove-job'), Queue('bulk-sms', Exchange('default'), routing_key='bulk-sms'), Queue('bulk-email', Exchange('default'), routing_key='bulk-email'), Queue('email-invited-user', Exchange('default'), routing_key='email-invited-user'), diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 39a755a71..4483f50cc 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -71,7 +71,7 @@ def test_should_call_delete_invotations_on_delete_invitations_task(notify_api, m @freeze_time("2016-01-01 11:09:00.061258") -def test_should_process_sms_job(sample_job, mocker): +def test_should_process_sms_job(sample_job, mocker, mock_celery_remove_job): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) mocker.patch('app.celery.tasks.send_sms.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") @@ -94,7 +94,10 @@ def test_should_process_sms_job(sample_job, mocker): @freeze_time("2016-01-01 11:09:00.061258") -def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, notify_db_session, mocker): +def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, + notify_db_session, + mocker, + mock_celery_remove_job): service = sample_service(notify_db, notify_db_session, limit=9) job = sample_job(notify_db, notify_db_session, service=service, notification_count=10) @@ -109,9 +112,13 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, notif job = jobs_dao.dao_get_job_by_id(job.id) assert job.status == 'sending limits exceeded' tasks.send_sms.apply_async.assert_not_called() + mock_celery_remove_job.assert_not_called() -def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify_db, notify_db_session, mocker): +def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify_db, + notify_db_session, + mocker, + mock_celery_remove_job): service = sample_service(notify_db, notify_db_session, limit=1) job = sample_job(notify_db, notify_db_session, service=service) @@ -128,6 +135,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify assert job.status == 'sending limits exceeded' s3.get_job_from_s3.assert_not_called() tasks.send_sms.apply_async.assert_not_called() + mock_celery_remove_job.assert_not_called() def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(notify_db, notify_db_session, mocker): @@ -170,7 +178,10 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not @freeze_time("2016-01-01 11:09:00.061258") -def test_should_process_sms_job_if_exactly_on_send_limits(notify_db, notify_db_session, mocker): +def test_should_process_sms_job_if_exactly_on_send_limits(notify_db, + notify_db_session, + mocker, + mock_celery_remove_job): service = sample_service(notify_db, notify_db_session, limit=10) template = sample_email_template(notify_db, notify_db_session, service=service) job = sample_job(notify_db, notify_db_session, service=service, template=template, notification_count=10) @@ -194,9 +205,10 @@ def test_should_process_sms_job_if_exactly_on_send_limits(notify_db, notify_db_s "2016-01-01T11:09:00.061258"), queue="bulk-email" ) + mock_celery_remove_job.assert_called_once_with((str(job.id),), queue="remove-job") -def test_should_not_create_send_task_for_empty_file(sample_job, mocker): +def test_should_not_create_send_task_for_empty_file(sample_job, mocker, mock_celery_remove_job): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('empty')) mocker.patch('app.celery.tasks.send_sms.apply_async') @@ -209,7 +221,7 @@ def test_should_not_create_send_task_for_empty_file(sample_job, mocker): @freeze_time("2016-01-01 11:09:00.061258") -def test_should_process_email_job(sample_email_job, mocker): +def test_should_process_email_job(sample_email_job, mocker, mock_celery_remove_job): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email')) mocker.patch('app.celery.tasks.send_email.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") @@ -231,9 +243,13 @@ def test_should_process_email_job(sample_email_job, mocker): ) job = jobs_dao.dao_get_job_by_id(sample_email_job.id) assert job.status == 'finished' + mock_celery_remove_job.assert_called_once_with((str(job.id),), queue="remove-job") -def test_should_process_all_sms_job(sample_job, sample_job_with_placeholdered_template, mocker): +def test_should_process_all_sms_job(sample_job, + sample_job_with_placeholdered_template, + mocker, + mock_celery_remove_job): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) mocker.patch('app.celery.tasks.send_sms.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 60e57a5dc..73d645139 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -345,6 +345,11 @@ def mock_encryption(mocker): return mocker.patch('app.encryption.encrypt', return_value="something_encrypted") +@pytest.fixture(scope='function') +def mock_celery_remove_job(mocker): + return mocker.patch('app.celery.tasks.remove_job.apply_async') + + @pytest.fixture(scope='function') def sample_invited_user(notify_db, notify_db_session, From eef6d80ae2fe216524d83a494766be7b6b6b6804 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 5 Apr 2016 14:51:55 +0100 Subject: [PATCH 09/10] Catch sending to restricted recipients in Celery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Celery `send_sms` and `send_email` tasks _could_ assume that all the recipients it gets are safe, because they have been checked either: - when the admin app processes the CSV - in the `/notifications/sms|email` endpoint *However*, it’s probably safer to make the check again when the Celery task run and passes the message off to the third party. This also means that changing a service _back_ to restricted would have an effect on messages that were queued, as well as all subsequent messages. This commit: - restores the test that was removed here: https://github.com/alphagov/notifications-api/commit/e56aee5d1d77142b78434f3faeddfd2cb1a6e9bf#diff-e5627619e387fccc04783c32a23e6859L346 - adds checks back into the Celery tasks for sending email and SMS, using the `allowed_to_send_to` function that was introduced into utils in https://github.com/alphagov/notifications-utils/pull/16 --- app/celery/tasks.py | 45 +++++++++++++++++++++++++++++++--- tests/app/celery/test_tasks.py | 25 +++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d41271e06..260b93b08 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,3 +1,4 @@ +import itertools from datetime import datetime from flask import current_app @@ -12,7 +13,8 @@ from utils.template import Template from utils.recipients import ( RecipientCSV, - validate_and_format_phone_number + validate_and_format_phone_number, + allowed_to_send_to ) from app import ( @@ -188,9 +190,16 @@ def process_job(job_id): def send_sms(service_id, notification_id, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) - client = firetext_client + restricted = False + + if not service_allowed_to_send_to(notification['to'], service): + current_app.logger.info( + "SMS {} failed as restricted service".format(notification_id) + ) + restricted = True + try: sent_at = datetime.utcnow() notification_db_object = Notification( @@ -199,7 +208,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status='sent', + status='failed' if restricted else 'sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() @@ -207,6 +216,9 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) + if restricted: + return + try: template = Template( dao_get_template_by_id(notification['template']).__dict__, @@ -238,6 +250,15 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): def send_email(service_id, notification_id, subject, from_address, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) client = aws_ses_client + service = dao_fetch_service_by_id(service_id) + + restricted = False + + if not service_allowed_to_send_to(notification['to'], service): + current_app.logger.info( + "Email {} failed as restricted service".format(notification_id) + ) + restricted = True try: sent_at = datetime.utcnow() @@ -247,13 +268,16 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status='sent', + status='failed' if restricted else 'sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_EMAIL) + if restricted: + return + try: template = Template( dao_get_template_by_id(notification['template']).__dict__, @@ -388,3 +412,16 @@ def email_registration_verification(encrypted_verification_message): url=verification_message['url'])) except AwsSesClientException as e: current_app.logger.exception(e) + + +def service_allowed_to_send_to(recipient, service): + + if not service.restricted: + return True + + return allowed_to_send_to( + recipient, + itertools.chain.from_iterable( + [user.mobile_number, user.email_address] for user in service.users + ) + ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 39a755a71..767a4cb5c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -343,6 +343,31 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif ) +def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): + user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") + service = sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = sample_template(notify_db, notify_db_session, service=service) + + notification = { + "template": template.id, + "to": "07700 900849" + } + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.firetext_client.send_sms') + mocker.patch('app.firetext_client.get_name', return_value="firetext") + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_sms( + service.id, + notification_id, + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + + firetext_client.send_sms.assert_not_called() + + def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, email="test@restricted.com") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) From a63d6aa168734b13083f25f1fc6b5e9d1562b7cc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 5 Apr 2016 15:27:16 +0100 Subject: [PATCH 10/10] Add test for sending email while service restricted --- tests/app/celery/test_tasks.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 767a4cb5c..0935ae6e8 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -400,6 +400,32 @@ def test_should_send_email_if_restricted_service_and_valid_email(notify_db, noti ) +def test_should_not_send_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker): + user = sample_user(notify_db, notify_db_session) + service = sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = sample_template( + notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello' + ) + + notification = { + "template": template.id, + "to": "test@example.com" + } + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.aws_ses_client.send_email') + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_sms( + service.id, + notification_id, + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + + aws_ses_client.send_email.assert_not_called() + + def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sample_job, mocker): notification = { "template": sample_job.template.id,