From 0c160c3419ba28c34f73d42d90052f7ab97a3594 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 5 May 2017 15:19:57 +0100 Subject: [PATCH 01/68] Store the service we have used to authenticate the client API user against the request. We can then use this later - saving an extra DB query on every client facing API call - Note this doesn't affect admin calls which do not use the service from the api key, but use the one passed as part of the URL path. --- app/authentication/auth.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 1e9fbece8..77fba3d95 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -5,7 +5,7 @@ from sqlalchemy.orm.exc import NoResultFound from notifications_python_client.authentication import decode_jwt_token, get_token_issuer from notifications_python_client.errors import TokenDecodeError, TokenExpiredError, TokenIssuerError -from app.dao.services_dao import dao_fetch_service_by_id +from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys class AuthError(Exception): @@ -59,7 +59,7 @@ def requires_auth(): client = __get_token_issuer(auth_token) try: - service = dao_fetch_service_by_id(client) + service = dao_fetch_service_by_id_with_api_keys(client) except DataError: raise AuthError("Invalid token: service id is not the right data type", 403) except NoResultFound: @@ -81,7 +81,9 @@ def requires_auth(): raise AuthError("Invalid token: API key revoked", 403) g.service_id = api_key.service_id + _request_ctx_stack.top.authenticated_service = service _request_ctx_stack.top.api_user = api_key + return else: # service has API keys, but none matching the one the user provided From 34c9198d0cd324edb24c7be444a6e2e1b960e43d Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 5 May 2017 15:20:23 +0100 Subject: [PATCH 02/68] Make the service available to code on the request context --- app/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/__init__.py b/app/__init__.py index 8d8e18270..c5b509f30 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -40,6 +40,7 @@ performance_platform_client = PerformancePlatformClient() clients = Clients() api_user = LocalProxy(lambda: _request_ctx_stack.top.api_user) +authenticated_service = LocalProxy(lambda: _request_ctx_stack.top.authenticated_service) def create_app(app_name=None): From 07daa369a387ab0b0571c536a07e27a4fe9ab482 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 5 May 2017 15:20:34 +0100 Subject: [PATCH 03/68] Fixed some config issues --- app/config.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 4b0f170fb..3aead8a56 100644 --- a/app/config.py +++ b/app/config.py @@ -198,6 +198,7 @@ class Config(object): ###################### class Development(Config): + SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'notify.tools' CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' NOTIFY_ENVIRONMENT = 'development' @@ -236,7 +237,7 @@ class Test(Config): Queue('send-email', Exchange('default'), routing_key='send-email'), Queue('research-mode', Exchange('default'), routing_key='research-mode') ] - REDIS_ENABLED = True + API_RATE_LIMIT_ENABLED = True API_HOST_NAME = "http://localhost:6011" @@ -295,6 +296,7 @@ class Sandbox(CloudFoundryConfig): NOTIFY_ENVIRONMENT = 'sandbox' CSV_UPLOAD_BUCKET_NAME = 'cf-sandbox-notifications-csv-upload' FROM_NUMBER = 'sandbox' + REDIS_ENABLED = False configs = { From a340ed6f46baa951991143376cfee281f66b84f5 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 5 May 2017 15:21:13 +0100 Subject: [PATCH 04/68] Changes to how we log to avoid unneeded DB calls. --- app/notifications/process_notifications.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index e8510ba63..9247f813d 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -46,6 +46,8 @@ def persist_notification( notification_id=None, simulated=False ): + + notification_created_at = created_at or datetime.utcnow() notification = Notification( id=notification_id, template_id=template_id, @@ -57,7 +59,7 @@ def persist_notification( notification_type=notification_type, api_key_id=api_key_id, key_type=key_type, - created_at=created_at or datetime.utcnow(), + created_at=notification_created_at, job_id=job_id, job_row_number=job_row_number, client_reference=client_reference, @@ -80,7 +82,7 @@ def persist_notification( if redis_store.get_all_from_hash(cache_key_for_service_template_counter(service.id)): redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) current_app.logger.info( - "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) + "{} {} created at {}".format(notification_type, notification_id, notification_created_at) ) return notification From 064b7436db608c728cdebae7243c79dc583cfb2c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 5 May 2017 15:22:21 +0100 Subject: [PATCH 05/68] Extra call to explicitly load API keys on service load - avoids extra calls later. --- app/dao/services_dao.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index f55b6ec4c..1a8e75eee 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -60,6 +60,19 @@ def dao_fetch_service_by_id(service_id, only_active=False): return query.one() +def dao_fetch_service_by_id_with_api_keys(service_id, only_active=False): + query = Service.query.filter_by( + id=service_id + ).options( + joinedload('api_keys') + ) + + if only_active: + query = query.filter(Service.active) + + return query.one() + + def dao_fetch_all_services_by_user(user_id, only_active=False): query = Service.query.filter( Service.users.any(id=user_id) From 0e38259cb496e150fd6e7268699c1c926e39a547 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 5 May 2017 15:23:06 +0100 Subject: [PATCH 06/68] Remove all DAO get service calls, replaced with reference to new service object in context. --- app/notifications/rest.py | 42 +++++++++++----------- app/v2/notifications/get_notifications.py | 6 ++-- app/v2/notifications/post_notifications.py | 31 +++++++++------- app/v2/template/get_template.py | 5 ++- app/v2/template/post_template.py | 6 ++-- app/v2/templates/get_templates.py | 5 ++- 6 files changed, 50 insertions(+), 45 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index eb395736b..faa086e32 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -5,20 +5,23 @@ from flask import ( current_app ) -from app import api_user +from app import api_user, authenticated_service from app.dao import ( templates_dao, - services_dao, notifications_dao ) from app.models import KEY_TYPE_TEAM, PRIORITY from app.models import SMS_TYPE -from app.notifications.process_notifications import (persist_notification, - send_notification_to_queue, - simulated_recipient) -from app.notifications.validators import (check_service_over_daily_message_limit, - check_template_is_for_notification_type, - check_template_is_active, check_rate_limiting) +from app.notifications.process_notifications import ( + persist_notification, + send_notification_to_queue, + simulated_recipient +) +from app.notifications.validators import ( + check_template_is_for_notification_type, + check_template_is_active, + check_rate_limiting +) from app.schemas import ( email_notification_schema, sms_template_notification_schema, @@ -45,9 +48,10 @@ register_errors(notifications) @notifications.route('/notifications/', methods=['GET']) def get_notification_by_id(notification_id): - notification = notifications_dao.get_notification_with_personalisation(str(api_user.service_id), - notification_id, - key_type=None) + notification = notifications_dao.get_notification_with_personalisation( + str(authenticated_service.id), + notification_id, + key_type=None) return jsonify(data={"notification": notification_with_personalisation_schema.dump(notification).data}), 200 @@ -60,7 +64,7 @@ def get_all_notifications(): limit_days = data.get('limit_days') pagination = notifications_dao.get_notifications_for_service( - str(api_user.service_id), + str(authenticated_service.id), personalisation=True, filter_dict=data, page=page, @@ -96,8 +100,6 @@ def send_notification(notification_type): if notification_type not in ['sms', 'email']: assert False - service = services_dao.dao_fetch_service_by_id(api_user.service_id) - notification_form, errors = ( sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema ).load(request.get_json()) @@ -105,27 +107,27 @@ def send_notification(notification_type): if errors: raise InvalidRequest(errors, status_code=400) - check_rate_limiting(service, api_user) + check_rate_limiting(authenticated_service, api_user) template = templates_dao.dao_get_template_by_id_and_service_id( template_id=notification_form['template'], - service_id=service.id) + service_id=authenticated_service.id) check_template_is_for_notification_type(notification_type, template.template_type) check_template_is_active(template) template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {})) - _service_allowed_to_send_to(notification_form, service) + _service_allowed_to_send_to(notification_form, authenticated_service) if notification_type == SMS_TYPE: - _service_can_send_internationally(service, notification_form['to']) + _service_can_send_internationally(authenticated_service, notification_form['to']) # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(notification_form['to'], notification_type) notification_model = persist_notification(template_id=template.id, template_version=template.version, recipient=request.get_json()['to'], - service=service, + service=authenticated_service, personalisation=notification_form.get('personalisation', None), notification_type=notification_type, api_key_id=api_user.id, @@ -134,7 +136,7 @@ def send_notification(notification_type): if not simulated: queue_name = 'priority' if template.process_type == PRIORITY else None send_notification_to_queue(notification=notification_model, - research_mode=service.research_mode, + research_mode=authenticated_service.research_mode, queue=queue_name) else: current_app.logger.info("POST simulated notification for id: {}".format(notification_model.id)) diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 112d2e51e..08a0d3c5e 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -3,7 +3,7 @@ import uuid from flask import jsonify, request, url_for, current_app from werkzeug.exceptions import abort -from app import api_user +from app import api_user, authenticated_service from app.dao import notifications_dao from app.schema_validation import validate from app.v2.notifications import v2_notification_blueprint @@ -17,7 +17,7 @@ def get_notification_by_id(id): except ValueError or AttributeError: abort(404) notification = notifications_dao.get_notification_with_personalisation( - api_user.service_id, casted_id, key_type=None + authenticated_service.id, casted_id, key_type=None ) return jsonify(notification.serialize()), 200 @@ -38,7 +38,7 @@ def get_notifications(): data = validate(_data, get_notifications_request) paginated_notifications = notifications_dao.get_notifications_for_service( - str(api_user.service_id), + str(authenticated_service.id), filter_dict=data, key_type=api_user.key_type, personalisation=True, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 347884ed8..459cf0050 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,7 +1,7 @@ from flask import request, jsonify, current_app from sqlalchemy.orm.exc import NoResultFound -from app import api_user +from app import api_user, authenticated_service from app.dao import services_dao, templates_dao from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY from app.notifications.process_notifications import ( @@ -32,17 +32,15 @@ def post_notification(notification_type): else: form = validate(request.get_json(), post_sms_request) - service = services_dao.dao_fetch_service_by_id(api_user.service_id) - - check_rate_limiting(service, api_user) + check_rate_limiting(authenticated_service, api_user) form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address'] send_to = validate_and_format_recipient(send_to=form_send_to, key_type=api_user.key_type, - service=service, + service=authenticated_service, notification_type=notification_type) - template, template_with_content = __validate_template(form, service, notification_type) + template, template_with_content = __validate_template(form, authenticated_service, notification_type) # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(send_to, notification_type) @@ -50,7 +48,7 @@ def post_notification(notification_type): notification = persist_notification(template_id=template.id, template_version=template.version, recipient=form_send_to, - service=service, + service=authenticated_service, personalisation=form.get('personalisation', None), notification_type=notification_type, api_key_id=api_user.id, @@ -60,23 +58,32 @@ def post_notification(notification_type): if not simulated: queue_name = 'priority' if template.process_type == PRIORITY else None - send_notification_to_queue(notification=notification, research_mode=service.research_mode, queue=queue_name) + send_notification_to_queue( + notification=notification, + research_mode=authenticated_service.research_mode, + queue=queue_name + ) + else: current_app.logger.info("POST simulated notification for id: {}".format(notification.id)) if notification_type == SMS_TYPE: - sms_sender = service.sms_sender if service.sms_sender else current_app.config.get('FROM_NUMBER') + sms_sender = (authenticated_service.sms_sender + if authenticated_service.sms_sender + else current_app.config.get('FROM_NUMBER') + ) resp = create_post_sms_response_from_notification(notification=notification, body=str(template_with_content), from_number=sms_sender, url_root=request.url_root, - service_id=service.id) + service_id=authenticated_service.id) else: resp = create_post_email_response_from_notification(notification=notification, content=str(template_with_content), subject=template_with_content.subject, - email_from=service.email_from, + email_from=authenticated_service.email_from, url_root=request.url_root, - service_id=service.id) + service_id=authenticated_service.id) + return jsonify(resp), 201 diff --git a/app/v2/template/get_template.py b/app/v2/template/get_template.py index fef0e1b2e..4054e161a 100644 --- a/app/v2/template/get_template.py +++ b/app/v2/template/get_template.py @@ -1,7 +1,6 @@ from flask import jsonify -from jsonschema.exceptions import ValidationError -from app import api_user +from app import authenticated_service from app.dao import templates_dao from app.schema_validation import validate from app.v2.template import v2_template_blueprint @@ -18,6 +17,6 @@ def get_template_by_id(template_id, version=None): data = validate(_data, get_template_by_id_request) template = templates_dao.dao_get_template_by_id_and_service_id( - template_id, api_user.service_id, data.get('version')) + template_id, authenticated_service.id, data.get('version')) return jsonify(template.serialize()), 200 diff --git a/app/v2/template/post_template.py b/app/v2/template/post_template.py index 3da768ef1..4de01084a 100644 --- a/app/v2/template/post_template.py +++ b/app/v2/template/post_template.py @@ -1,9 +1,7 @@ from flask import jsonify, request -from jsonschema.exceptions import ValidationError -from app import api_user +from app import authenticated_service from app.dao import templates_dao -from app.models import SMS_TYPE from app.schema_validation import validate from app.utils import get_template_instance from app.v2.errors import BadRequestError @@ -22,7 +20,7 @@ def post_template_preview(template_id): data = validate(_data, post_template_preview_request) template = templates_dao.dao_get_template_by_id_and_service_id( - template_id, api_user.service_id) + template_id, authenticated_service.id) template_object = get_template_instance( template.__dict__, values=data.get('personalisation')) diff --git a/app/v2/templates/get_templates.py b/app/v2/templates/get_templates.py index 92c79fd63..cd34e46aa 100644 --- a/app/v2/templates/get_templates.py +++ b/app/v2/templates/get_templates.py @@ -1,7 +1,6 @@ from flask import jsonify, request -from jsonschema.exceptions import ValidationError -from app import api_user +from app import authenticated_service from app.dao import templates_dao from app.schema_validation import validate from app.v2.templates import v2_templates_blueprint @@ -12,7 +11,7 @@ from app.v2.templates.templates_schemas import get_all_template_request def get_templates(): data = validate(request.args.to_dict(), get_all_template_request) - templates = templates_dao.dao_get_all_templates_for_service(api_user.service_id, data.get('type')) + templates = templates_dao.dao_get_all_templates_for_service(authenticated_service.id, data.get('type')) return jsonify( templates=[template.serialize() for template in templates] From 6e888260fcc66d83ba81c19576646747f7e536cc Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 8 May 2017 16:46:44 +0100 Subject: [PATCH 07/68] Fix import --- app/notifications/validators.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 5d43e7341..e601ec6be 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -10,12 +10,11 @@ from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM, SMS_TYPE from app.service.utils import service_allowed_to_send_to from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError from app import redis_store -from notifications_utils.clients import redis def check_service_over_api_rate_limit(service, api_key): if current_app.config['API_RATE_LIMIT_ENABLED']: - cache_key = redis.rate_limit_cache_key(service.id, api_key.key_type) + cache_key = redis_store.rate_limit_cache_key(service.id, api_key.key_type) rate_limit = current_app.config['API_KEY_LIMITS'][api_key.key_type]['limit'] interval = current_app.config['API_KEY_LIMITS'][api_key.key_type]['interval'] if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval): @@ -25,7 +24,7 @@ def check_service_over_api_rate_limit(service, api_key): def check_service_over_daily_message_limit(key_type, service): if key_type != KEY_TYPE_TEST: - cache_key = redis.daily_limit_cache_key(service.id) + cache_key = redis_store.daily_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) if not service_stats: service_stats = services_dao.fetch_todays_total_message_count(service.id) From d647640b65191f7a5ab0304c8f06ca5d92734bce Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 11:22:05 +0100 Subject: [PATCH 08/68] Added job statistics table --- app/models.py | 17 ++++++++ migrations/versions/0081_add_job_stats.py | 53 +++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 migrations/versions/0081_add_job_stats.py diff --git a/app/models.py b/app/models.py index 0ef2de359..a5f52759c 100644 --- a/app/models.py +++ b/app/models.py @@ -973,3 +973,20 @@ class Rate(db.Model): valid_from = db.Column(db.DateTime, nullable=False) rate = db.Column(db.Float(asdecimal=False), nullable=False) notification_type = db.Column(notification_types, index=True, nullable=False) + + + +class JobStatistics(db.Model): + __tablename__ = 'job_statistics' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=True) + job = db.relationship('Job', backref=db.backref('job_statistics', lazy='dynamic')) + emails_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + emails_delivered = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + emails_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + sms_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + sms_delivered = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + sms_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + letters_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + letters_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) diff --git a/migrations/versions/0081_add_job_stats.py b/migrations/versions/0081_add_job_stats.py new file mode 100644 index 000000000..3df9a2d37 --- /dev/null +++ b/migrations/versions/0081_add_job_stats.py @@ -0,0 +1,53 @@ +"""empty message + +Revision ID: 0081_add_job_stats +Revises: 0080_fix_rate_start_date +Create Date: 2017-05-09 11:21:06.959075 + +""" + +# revision identifiers, used by Alembic. +revision = '0081_add_job_stats' +down_revision = '0080_fix_rate_start_date' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('job_statistics', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('job_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('emails_sent', sa.BigInteger(), nullable=False), + sa.Column('emails_delivered', sa.BigInteger(), nullable=False), + sa.Column('emails_failed', sa.BigInteger(), nullable=False), + sa.Column('sms_sent', sa.BigInteger(), nullable=False), + sa.Column('sms_delivered', sa.BigInteger(), nullable=False), + sa.Column('sms_failed', sa.BigInteger(), nullable=False), + sa.Column('letters_sent', sa.BigInteger(), nullable=False), + sa.Column('letters_failed', sa.BigInteger(), nullable=False), + sa.ForeignKeyConstraint(['job_id'], ['jobs.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_job_statistics_job_id'), 'job_statistics', ['job_id'], unique=True) + op.alter_column('notification_history', 'international', + existing_type=sa.BOOLEAN(), + nullable=False) + op.alter_column('notifications', 'international', + existing_type=sa.BOOLEAN(), + nullable=False) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('notifications', 'international', + existing_type=sa.BOOLEAN(), + nullable=True) + op.alter_column('notification_history', 'international', + existing_type=sa.BOOLEAN(), + nullable=True) + op.drop_index(op.f('ix_job_statistics_job_id'), table_name='job_statistics') + op.drop_table('job_statistics') + # ### end Alembic commands ### From 9bedcbc1b34c331a08aa899c2f3e2e1078c5e0b5 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 11:22:45 +0100 Subject: [PATCH 09/68] removed the reference to internation numbers from migration script --- migrations/versions/0081_add_job_stats.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/migrations/versions/0081_add_job_stats.py b/migrations/versions/0081_add_job_stats.py index 3df9a2d37..d6c20ea6b 100644 --- a/migrations/versions/0081_add_job_stats.py +++ b/migrations/versions/0081_add_job_stats.py @@ -15,7 +15,6 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.create_table('job_statistics', sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('job_id', postgresql.UUID(as_uuid=True), nullable=True), @@ -31,23 +30,8 @@ def upgrade(): sa.PrimaryKeyConstraint('id') ) op.create_index(op.f('ix_job_statistics_job_id'), 'job_statistics', ['job_id'], unique=True) - op.alter_column('notification_history', 'international', - existing_type=sa.BOOLEAN(), - nullable=False) - op.alter_column('notifications', 'international', - existing_type=sa.BOOLEAN(), - nullable=False) - # ### end Alembic commands ### def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.alter_column('notifications', 'international', - existing_type=sa.BOOLEAN(), - nullable=True) - op.alter_column('notification_history', 'international', - existing_type=sa.BOOLEAN(), - nullable=True) op.drop_index(op.f('ix_job_statistics_job_id'), table_name='job_statistics') op.drop_table('job_statistics') - # ### end Alembic commands ### From b669c4930ad2f2c55e849b23c93d605ea0aea39f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 12:30:31 +0100 Subject: [PATCH 10/68] Building the methods around async stats work --- app/celery/statistics_tasks.py | 9 +++++++++ app/dao/statistics_dao.py | 4 ++++ tests/app/dao/test_statistics_dao.py | 19 +++++++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 app/celery/statistics_tasks.py create mode 100644 app/dao/statistics_dao.py create mode 100644 tests/app/dao/test_statistics_dao.py diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py new file mode 100644 index 000000000..2712f1a49 --- /dev/null +++ b/app/celery/statistics_tasks.py @@ -0,0 +1,9 @@ +from app import notify_celery +from app.statsd_decorators import statsd + + +@notify_celery.task(bind=True, name='record_initial_job_statistics') +@statsd(namespace="tasks") +def record_initial_job_statistics(notification): + pass + diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py new file mode 100644 index 000000000..02ed25e78 --- /dev/null +++ b/app/dao/statistics_dao.py @@ -0,0 +1,4 @@ + + +def persist_initial_job_statistics(notification): + pass diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py new file mode 100644 index 000000000..a66dfbe42 --- /dev/null +++ b/tests/app/dao/test_statistics_dao.py @@ -0,0 +1,19 @@ +from app.dao.statistics_dao import persist_initial_job_statistics +from app.models import JobStatistics +from tests.app.conftest import sample_notification + + +def test_should_create_a_stats_entry_for_a_job( + notify_db, notify_db_session, sample_service, sample_template, sample_job +): + assert not len(JobStatistics.query.all()) + + notification = sample_notification( + notify_db, notify_db_session, service=sample_service, template=sample_template, job=sample_job + ) + + persist_initial_job_statistics(notification) + + stats = JobStatistics.query.all() + + assert len(stats) == 1 From 9c3ea95dcd5977d7df186d3a7bd594b0d82d0fc5 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 14:04:07 +0100 Subject: [PATCH 11/68] Changed job_stats table to have an update at time stamp --- migrations/versions/0081_add_job_stats.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/migrations/versions/0081_add_job_stats.py b/migrations/versions/0081_add_job_stats.py index d6c20ea6b..c1ef5e205 100644 --- a/migrations/versions/0081_add_job_stats.py +++ b/migrations/versions/0081_add_job_stats.py @@ -2,7 +2,7 @@ Revision ID: 0081_add_job_stats Revises: 0080_fix_rate_start_date -Create Date: 2017-05-09 11:21:06.959075 +Create Date: 2017-05-09 12:44:43.173269 """ @@ -26,6 +26,7 @@ def upgrade(): sa.Column('sms_failed', sa.BigInteger(), nullable=False), sa.Column('letters_sent', sa.BigInteger(), nullable=False), sa.Column('letters_failed', sa.BigInteger(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), sa.ForeignKeyConstraint(['job_id'], ['jobs.id'], ), sa.PrimaryKeyConstraint('id') ) From a5caed5ac83b48772f8e2cc3bc00f890057876e9 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 14:04:19 +0100 Subject: [PATCH 12/68] Changed job_stats table to have an update at time stamp --- app/models.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index a5f52759c..34e9e1376 100644 --- a/app/models.py +++ b/app/models.py @@ -379,7 +379,6 @@ class TemplateHistory(db.Model): default=NORMAL) def serialize(self): - serialized = { "id": self.id, "type": self.template_type, @@ -975,7 +974,6 @@ class Rate(db.Model): notification_type = db.Column(notification_types, index=True, nullable=False) - class JobStatistics(db.Model): __tablename__ = 'job_statistics' @@ -990,3 +988,20 @@ class JobStatistics(db.Model): sms_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + updated_at = db.Column( + db.DateTime, + index=False, + unique=False, + nullable=True, + onupdate=datetime.datetime.utcnow) + + def __str__(self): + the_string = "" + the_string += "email sent {} email delivered {} email failed {} ".format( + self.emails_sent, self.emails_delivered, self.emails_failed + ) + the_string += "sms sent {} sms delivered {} sms failed {} ".format( + self.sms_sent, self.sms_delivered, self.sms_failed + ) + the_string += "letter sent {} letter failed {} ".format(self.letters_sent, self.letters_failed) + return the_string From 15065c4bc72719a673e1241071898fc362126019 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 14:06:27 +0100 Subject: [PATCH 13/68] Added DAO methods to add and update the stats table for JOBS - create_or_update_job_sending_statistics This will try and update an existing row. if this fails as it hasn't been created, then it will insert a row. If this fails as another process has got there first then it should try and update again. This is a code version of upset - update_job_stats_outcome_count Will update the outcome states. Uses the NOTIFICATION_STATUS_TYPES_FAILED to determine if the notification failed. Any thing in DELIVERED will be marked as delivered. Statues not in the FAILED array or delivered provoke no update to the table. --- app/dao/statistics_dao.py | 79 ++++++- tests/app/dao/test_statistics_dao.py | 333 ++++++++++++++++++++++++++- 2 files changed, 404 insertions(+), 8 deletions(-) diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index 02ed25e78..a2fca3965 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -1,4 +1,79 @@ +from flask import current_app +from sqlalchemy.exc import SQLAlchemyError + +from app import db +from app.dao.dao_utils import transactional +from app.models import ( + JobStatistics, + EMAIL_TYPE, + SMS_TYPE, + LETTER_TYPE, + NOTIFICATION_STATUS_TYPES_FAILED, + NOTIFICATION_DELIVERED +) +from app.statsd_decorators import statsd -def persist_initial_job_statistics(notification): - pass +@statsd(namespace="dao") +@transactional +def create_or_update_job_sending_statistics(notification): + if __update_job_stats_sent_count(notification) == 0: + try: + __insert_job_stats(notification) + except SQLAlchemyError as e: + current_app.logger.exception(e) + __update_job_stats_sent_count(notification) + + +def __update_job_stats_sent_count(notification): + update = { + JobStatistics.emails_sent: + JobStatistics.emails_sent + 1 if notification.notification_type == EMAIL_TYPE else 0, + JobStatistics.sms_sent: + JobStatistics.sms_sent + 1 if notification.notification_type == SMS_TYPE else 0, + JobStatistics.letters_sent: + JobStatistics.letters_sent + 1 if notification.notification_type == LETTER_TYPE else 0 + } + return db.session.query(JobStatistics).filter_by( + job_id=notification.job_id, + ).update(update) + + +def __insert_job_stats(notification): + + stats = JobStatistics( + job_id=notification.job_id, + emails_sent=1 if notification.notification_type == EMAIL_TYPE else 0, + sms_sent=1 if notification.notification_type == SMS_TYPE else 0, + letters_sent=1 if notification.notification_type == LETTER_TYPE else 0 + ) + db.session.add(stats) + + +def update_job_stats_outcome_count(notification): + update = None + + if notification.status in NOTIFICATION_STATUS_TYPES_FAILED: + update = { + JobStatistics.emails_failed: + JobStatistics.emails_failed + 1 if notification.notification_type == EMAIL_TYPE else 0, + JobStatistics.sms_failed: + JobStatistics.sms_failed + 1 if notification.notification_type == SMS_TYPE else 0, + JobStatistics.letters_failed: + JobStatistics.letters_failed + 1 if notification.notification_type == LETTER_TYPE else 0 + } + + elif notification.status == NOTIFICATION_DELIVERED and notification.notification_type != LETTER_TYPE: + update = { + JobStatistics.emails_delivered: + JobStatistics.emails_delivered + 1 if notification.notification_type == EMAIL_TYPE else 0, + JobStatistics.sms_delivered: + JobStatistics.sms_delivered + 1 if notification.notification_type == SMS_TYPE else 0 + } + + if update: + return db.session.query(JobStatistics).filter_by( + job_id=notification.job_id, + ).update(update) + else: + return 0 diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index a66dfbe42..c1e80208f 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -1,19 +1,340 @@ -from app.dao.statistics_dao import persist_initial_job_statistics -from app.models import JobStatistics -from tests.app.conftest import sample_notification +from unittest.mock import call + +import pytest +from sqlalchemy.exc import SQLAlchemyError + +from app.dao.statistics_dao import ( + create_or_update_job_sending_statistics, + update_job_stats_outcome_count +) +from app.models import ( + JobStatistics, + SMS_TYPE, + EMAIL_TYPE, + LETTER_TYPE, + NOTIFICATION_DELIVERED, + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_TEMPORARY_FAILURE, + NOTIFICATION_PERMANENT_FAILURE, + NOTIFICATION_PENDING, NOTIFICATION_CREATED, NOTIFICATION_FAILED, NOTIFICATION_SENT, NOTIFICATION_SENDING) +from tests.app.conftest import sample_notification, sample_email_template, sample_template +@pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ + (SMS_TYPE, 1, 0, 0), + (EMAIL_TYPE, 0, 1, 0), + (LETTER_TYPE, 0, 0, 1) +]) def test_should_create_a_stats_entry_for_a_job( - notify_db, notify_db_session, sample_service, sample_template, sample_job + notify_db, + notify_db_session, + sample_job, + sample_letter_template, + notification_type, + sms_count, + email_count, + letter_count ): assert not len(JobStatistics.query.all()) + template = None + + if notification_type == SMS_TYPE: + template = sample_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == EMAIL_TYPE: + template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == LETTER_TYPE: + template = sample_letter_template + notification = sample_notification( - notify_db, notify_db_session, service=sample_service, template=sample_template, job=sample_job + notify_db, notify_db_session, service=sample_job.service, template=template, job=sample_job ) - persist_initial_job_statistics(notification) + create_or_update_job_sending_statistics(notification) stats = JobStatistics.query.all() assert len(stats) == 1 + + stat = stats[0] + assert stat.job_id == sample_job.id + + assert stat.emails_sent == email_count + assert stat.sms_sent == sms_count + assert stat.letters_sent == letter_count + + assert stat.emails_delivered == 0 + assert stat.emails_failed == 0 + assert stat.sms_delivered == 0 + assert stat.sms_failed == 0 + assert stat.letters_failed == 0 + + +@pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ + (SMS_TYPE, 2, 0, 0), + (EMAIL_TYPE, 0, 2, 0), + (LETTER_TYPE, 0, 0, 2) +]) +def test_should_update_a_stats_entry_for_a_job( + notify_db, + notify_db_session, + sample_job, + sample_letter_template, + notification_type, + sms_count, + email_count, + letter_count +): + assert not len(JobStatistics.query.all()) + + template = None + + if notification_type == SMS_TYPE: + template = sample_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == EMAIL_TYPE: + template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == LETTER_TYPE: + template = sample_letter_template + + notification = sample_notification( + notify_db, notify_db_session, service=sample_job.service, template=template, job=sample_job + ) + + create_or_update_job_sending_statistics(notification) + + stats = JobStatistics.query.all() + + assert len(stats) == 1 + + create_or_update_job_sending_statistics(notification) + + stat = stats[0] + assert stat.job_id == sample_job.id + + assert stat.emails_sent == email_count + assert stat.sms_sent == sms_count + assert stat.letters_sent == letter_count + + assert stat.emails_delivered == 0 + assert stat.emails_failed == 0 + assert stat.sms_delivered == 0 + assert stat.sms_failed == 0 + assert stat.letters_failed == 0 + + +def test_should_handle_case_where_stats_row_created_by_another_thread( + notify_db, + notify_db_session, + sample_notification, + mocker): + create_mock = mocker.patch("app.dao.statistics_dao.__insert_job_stats", side_effect=SQLAlchemyError("beaten to it")) + update_mock = mocker.patch("app.dao.statistics_dao.__update_job_stats_sent_count", return_value=0) + + create_or_update_job_sending_statistics(sample_notification) + + update_mock.assert_has_calls([call(sample_notification), call(sample_notification)]) + create_mock.assert_called_once_with(sample_notification) + + +@pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ + (SMS_TYPE, 1, 0, 0), + (EMAIL_TYPE, 0, 1, 0), + (LETTER_TYPE, 0, 0, 1) +]) +def test_should_update_a_stats_entry_with_its_success_outcome_for_a_job( + notify_db, + notify_db_session, + sample_job, + sample_letter_template, + notification_type, + sms_count, + email_count, + letter_count +): + assert not len(JobStatistics.query.all()) + + template = None + + if notification_type == SMS_TYPE: + template = sample_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == EMAIL_TYPE: + template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == LETTER_TYPE: + template = sample_letter_template + + notification = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=template, + job=sample_job, + status=NOTIFICATION_DELIVERED + ) + + create_or_update_job_sending_statistics(notification) + + stats = JobStatistics.query.all() + + assert len(stats) == 1 + + update_job_stats_outcome_count(notification) + + stat = stats[0] + assert stat.job_id == sample_job.id + + assert stat.emails_sent == email_count + assert stat.sms_sent == sms_count + assert stat.letters_sent == letter_count + + assert stat.emails_delivered == email_count + assert stat.sms_delivered == sms_count + + assert stat.emails_failed == 0 + assert stat.sms_failed == 0 + assert stat.letters_failed == 0 + + +@pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ + (SMS_TYPE, 1, 0, 0, NOTIFICATION_TECHNICAL_FAILURE), + (SMS_TYPE, 1, 0, 0, NOTIFICATION_TEMPORARY_FAILURE), + (SMS_TYPE, 1, 0, 0, NOTIFICATION_PERMANENT_FAILURE), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_TECHNICAL_FAILURE), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_PERMANENT_FAILURE), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_TEMPORARY_FAILURE), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_PERMANENT_FAILURE), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_TEMPORARY_FAILURE), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_TECHNICAL_FAILURE) +]) +def test_should_update_a_stats_entry_with_its_error_outcomes_for_a_job( + notify_db, + notify_db_session, + sample_job, + sample_letter_template, + notification_type, + sms_count, + email_count, + letter_count, + status +): + assert not len(JobStatistics.query.all()) + + template = None + + if notification_type == SMS_TYPE: + template = sample_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == EMAIL_TYPE: + template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == LETTER_TYPE: + template = sample_letter_template + + notification = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=template, + job=sample_job, + status=status + ) + + create_or_update_job_sending_statistics(notification) + + stats = JobStatistics.query.all() + + assert len(stats) == 1 + + update_job_stats_outcome_count(notification) + + stat = stats[0] + assert stat.job_id == sample_job.id + + assert stat.emails_sent == email_count + assert stat.sms_sent == sms_count + assert stat.letters_sent == letter_count + + assert stat.emails_failed == email_count + assert stat.letters_failed == letter_count + assert stat.sms_failed == sms_count + + assert stat.emails_delivered == 0 + assert stat.sms_delivered == 0 + + +@pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ + (SMS_TYPE, 1, 0, 0, NOTIFICATION_PENDING), + (SMS_TYPE, 1, 0, 0, NOTIFICATION_CREATED), + (SMS_TYPE, 1, 0, 0, NOTIFICATION_FAILED), + (SMS_TYPE, 1, 0, 0, NOTIFICATION_SENT), + (SMS_TYPE, 1, 0, 0, NOTIFICATION_SENDING), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_PENDING), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_CREATED), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_FAILED), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_SENT), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_SENDING), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_PENDING), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_CREATED), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_FAILED), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_SENT), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_SENDING) +]) +def test_should_not_update_job_stats_if_irrelevant_status( + notify_db, + notify_db_session, + sample_job, + sample_letter_template, + notification_type, + sms_count, + email_count, + letter_count, + status +): + assert not len(JobStatistics.query.all()) + + template = None + + if notification_type == SMS_TYPE: + template = sample_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == EMAIL_TYPE: + template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == LETTER_TYPE: + template = sample_letter_template + + notification = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=template, + job=sample_job, + status=status + ) + + create_or_update_job_sending_statistics(notification) + + stats = JobStatistics.query.all() + + assert len(stats) == 1 + + update_job_stats_outcome_count(notification) + + stat = stats[0] + assert stat.job_id == sample_job.id + + assert stat.emails_sent == email_count + assert stat.sms_sent == sms_count + assert stat.letters_sent == letter_count + + assert stat.emails_failed == 0 + assert stat.letters_failed == 0 + assert stat.sms_failed == 0 + + assert stat.emails_delivered == 0 + assert stat.sms_delivered == 0 From 4faafe01d9329acfdfc1d1c38b6c3e56f689ca62 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 14:41:29 +0100 Subject: [PATCH 14/68] Tightened the error checking. - Looks now for a specific Integrity error, not a generic SQLAlchemy error when checking for an insert failure - If the update fails post an insert error, then there is an issue so raise an exception so tasks can be retried. --- app/dao/statistics_dao.py | 9 +++++---- tests/app/dao/test_statistics_dao.py | 18 +++++++++++------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index a2fca3965..11c0c2e79 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -1,5 +1,5 @@ from flask import current_app -from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app import db from app.dao.dao_utils import transactional @@ -15,14 +15,14 @@ from app.statsd_decorators import statsd @statsd(namespace="dao") -@transactional def create_or_update_job_sending_statistics(notification): if __update_job_stats_sent_count(notification) == 0: try: __insert_job_stats(notification) - except SQLAlchemyError as e: + except IntegrityError as e: current_app.logger.exception(e) - __update_job_stats_sent_count(notification) + if __update_job_stats_sent_count(notification) == 0: + raise SQLAlchemyError("Failed to create job statistics for {}".format(notification.job_id)) def __update_job_stats_sent_count(notification): @@ -39,6 +39,7 @@ def __update_job_stats_sent_count(notification): ).update(update) +@transactional def __insert_job_stats(notification): stats = JobStatistics( diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index c1e80208f..21979ba1a 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -1,7 +1,7 @@ from unittest.mock import call import pytest -from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app.dao.statistics_dao import ( create_or_update_job_sending_statistics, @@ -126,18 +126,22 @@ def test_should_update_a_stats_entry_for_a_job( assert stat.letters_failed == 0 -def test_should_handle_case_where_stats_row_created_by_another_thread( +def test_should_handle_error_conditions( notify_db, notify_db_session, - sample_notification, + sample_job, mocker): - create_mock = mocker.patch("app.dao.statistics_dao.__insert_job_stats", side_effect=SQLAlchemyError("beaten to it")) + create_mock = mocker.patch("app.dao.statistics_dao.__insert_job_stats", side_effect=IntegrityError("1", "2", "3")) update_mock = mocker.patch("app.dao.statistics_dao.__update_job_stats_sent_count", return_value=0) - create_or_update_job_sending_statistics(sample_notification) + notification = sample_notification(notify_db, notify_db_session, job=sample_job) - update_mock.assert_has_calls([call(sample_notification), call(sample_notification)]) - create_mock.assert_called_once_with(sample_notification) + with pytest.raises(SQLAlchemyError) as e: + create_or_update_job_sending_statistics(notification) + assert 'Failed to create job statistics for {}'.format(sample_job.id) in str(e.value) + + update_mock.assert_has_calls([call(notification), call(notification)]) + create_mock.assert_called_once_with(notification) @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ From 2cbf8d29fdf527f32ca02d14c41b42d074639e74 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 14:46:15 +0100 Subject: [PATCH 15/68] Removed white space for pep8 --- app/celery/statistics_tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index 2712f1a49..bd92e6e5d 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -6,4 +6,3 @@ from app.statsd_decorators import statsd @statsd(namespace="tasks") def record_initial_job_statistics(notification): pass - From b4bf332ea055256273803fc68d678ed1e0494c8b Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 15:24:05 +0100 Subject: [PATCH 16/68] Added in tasks for created initial job stats and for updating following an outcome. --- app/celery/statistics_tasks.py | 34 ++++++++++-- tests/app/celery/test_statistics_tasks.py | 63 +++++++++++++++++++++++ 2 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 tests/app/celery/test_statistics_tasks.py diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index bd92e6e5d..f8b893a6e 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -1,8 +1,36 @@ +from sqlalchemy.exc import SQLAlchemyError + from app import notify_celery +from flask import current_app from app.statsd_decorators import statsd +from app.dao.statistics_dao import create_or_update_job_sending_statistics, update_job_stats_outcome_count -@notify_celery.task(bind=True, name='record_initial_job_statistics') +@notify_celery.task(bind=True, name='record_initial_job_statistics', max_retries=20, default_retry_delay=300) @statsd(namespace="tasks") -def record_initial_job_statistics(notification): - pass +def record_initial_job_statistics(self, notification): + try: + create_or_update_job_sending_statistics(notification) + except SQLAlchemyError as e: + current_app.logger.exception(e) + self.retry(queue="retry") + except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task record_initial_job_statistics failed for notification {}".format(notification.id) + ) + + +@notify_celery.task(bind=True, name='record_outcome_job_statistics', max_retries=20, default_retry_delay=300) +@statsd(namespace="tasks") +def record_outcome_job_statistics(self, notification): + try: + updated_count = update_job_stats_outcome_count(notification) + if updated_count == 0: + self.retry(queue="retry") + except SQLAlchemyError as e: + current_app.logger.exception(e) + self.retry(queue="retry") + except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task update_job_stats_outcome_count failed for notification {}".format(notification.id) + ) diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py new file mode 100644 index 000000000..edc451c17 --- /dev/null +++ b/tests/app/celery/test_statistics_tasks.py @@ -0,0 +1,63 @@ +from app.celery.statistics_tasks import record_initial_job_statistics, record_outcome_job_statistics +from sqlalchemy.exc import SQLAlchemyError + +import app + + +def test_should_call_create_job_stats_dao_methods(notify_db, notify_db_session, sample_notification, mocker): + dao_mock = mocker.patch("app.celery.statistics_tasks.create_or_update_job_sending_statistics") + record_initial_job_statistics(sample_notification) + + dao_mock.assert_called_once_with(sample_notification) + + +def test_should_retry_if_persisting_the_job_stats_has_a_sql_alchemy_exception( + notify_db, + notify_db_session, + sample_notification, + mocker): + dao_mock = mocker.patch( + "app.celery.statistics_tasks.create_or_update_job_sending_statistics", + side_effect=SQLAlchemyError() + ) + retry_mock = mocker.patch('app.celery.statistics_tasks.record_initial_job_statistics.retry') + + record_initial_job_statistics(sample_notification) + dao_mock.assert_called_once_with(sample_notification) + retry_mock.assert_called_with(queue="retry") + + +def test_should_call_update_job_stats_dao_outcome_methods(notify_db, notify_db_session, sample_notification, mocker): + dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count") + record_outcome_job_statistics(sample_notification) + + dao_mock.assert_called_once_with(sample_notification) + + +def test_should_retry_if_persisting_the_job_outcome_stats_has_a_sql_alchemy_exception( + notify_db, + notify_db_session, + sample_notification, + mocker): + dao_mock = mocker.patch( + "app.celery.statistics_tasks.update_job_stats_outcome_count", + side_effect=SQLAlchemyError() + ) + retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') + + record_outcome_job_statistics(sample_notification) + dao_mock.assert_called_once_with(sample_notification) + retry_mock.assert_called_with(queue="retry") + + +def test_should_retry_if_persisting_the_job_outcome_stats_updates_zero_rows( + notify_db, + notify_db_session, + sample_notification, + mocker): + dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count", return_value=0) + retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') + + record_outcome_job_statistics(sample_notification) + dao_mock.assert_called_once_with(sample_notification) + retry_mock.assert_called_with(queue="retry") From 70bc468da00b90045fcf6f93258a533712a6cd86 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 18:16:44 +0100 Subject: [PATCH 17/68] Ensure date set on creation --- app/dao/statistics_dao.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index 11c0c2e79..caa37aaf0 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -1,3 +1,5 @@ +from datetime import datetime + from flask import current_app from sqlalchemy.exc import IntegrityError, SQLAlchemyError @@ -25,6 +27,7 @@ def create_or_update_job_sending_statistics(notification): raise SQLAlchemyError("Failed to create job statistics for {}".format(notification.job_id)) +@transactional def __update_job_stats_sent_count(notification): update = { JobStatistics.emails_sent: @@ -46,11 +49,13 @@ def __insert_job_stats(notification): job_id=notification.job_id, emails_sent=1 if notification.notification_type == EMAIL_TYPE else 0, sms_sent=1 if notification.notification_type == SMS_TYPE else 0, - letters_sent=1 if notification.notification_type == LETTER_TYPE else 0 + letters_sent=1 if notification.notification_type == LETTER_TYPE else 0, + updated_at=datetime.utcnow() ) db.session.add(stats) +@transactional def update_job_stats_outcome_count(notification): update = None From f2a47044a4e4b196b11a80763430e52819887983 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 18:17:26 +0100 Subject: [PATCH 18/68] Add a wrapper method around calling the tasks to manage when to call stats tasks --- app/celery/statistics_tasks.py | 40 +++++++++--- tests/app/celery/test_statistics_tasks.py | 78 +++++++++++++++++++++-- 2 files changed, 102 insertions(+), 16 deletions(-) diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index f8b893a6e..3efcd389d 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -3,14 +3,32 @@ from sqlalchemy.exc import SQLAlchemyError from app import notify_celery from flask import current_app from app.statsd_decorators import statsd -from app.dao.statistics_dao import create_or_update_job_sending_statistics, update_job_stats_outcome_count +from app.dao.statistics_dao import ( + create_or_update_job_sending_statistics, + update_job_stats_outcome_count +) +from app.dao.notifications_dao import get_notification_by_id -@notify_celery.task(bind=True, name='record_initial_job_statistics', max_retries=20, default_retry_delay=300) +def create_initial_notification_statistic_tasks(notification): + if notification.job_id: + record_initial_job_statistics.apply_async((str(notification.id),), queue="notify") + + +def create_outcome_notification_statistic_tasks(notification): + if notification.job_id: + record_outcome_job_statistics.apply_async((str(notification.id),), queue="notify") + + +@notify_celery.task(bind=True, name='record_initial_job_statistics', max_retries=20, default_retry_delay=10) @statsd(namespace="tasks") -def record_initial_job_statistics(self, notification): +def record_initial_job_statistics(self, notification_id): try: - create_or_update_job_sending_statistics(notification) + notification = get_notification_by_id(notification_id) + if notification: + create_or_update_job_sending_statistics(notification) + else: + raise SQLAlchemyError("Failed to find notification with id {}".format(notification_id)) except SQLAlchemyError as e: current_app.logger.exception(e) self.retry(queue="retry") @@ -20,13 +38,17 @@ def record_initial_job_statistics(self, notification): ) -@notify_celery.task(bind=True, name='record_outcome_job_statistics', max_retries=20, default_retry_delay=300) +@notify_celery.task(bind=True, name='record_outcome_job_statistics', max_retries=20, default_retry_delay=10) @statsd(namespace="tasks") -def record_outcome_job_statistics(self, notification): +def record_outcome_job_statistics(self, notification_id): try: - updated_count = update_job_stats_outcome_count(notification) - if updated_count == 0: - self.retry(queue="retry") + notification = get_notification_by_id(notification_id) + if notification: + updated_count = update_job_stats_outcome_count(notification) + if updated_count == 0: + self.retry(queue="retry") + else: + raise SQLAlchemyError("Failed to find notification with id {}".format(notification_id)) except SQLAlchemyError as e: current_app.logger.exception(e) self.retry(queue="retry") diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py index edc451c17..2dbc4451a 100644 --- a/tests/app/celery/test_statistics_tasks.py +++ b/tests/app/celery/test_statistics_tasks.py @@ -1,12 +1,51 @@ -from app.celery.statistics_tasks import record_initial_job_statistics, record_outcome_job_statistics +from app.celery.statistics_tasks import ( + record_initial_job_statistics, + record_outcome_job_statistics, + create_initial_notification_statistic_tasks, + create_outcome_notification_statistic_tasks) from sqlalchemy.exc import SQLAlchemyError +from app import create_uuid +from tests.app.conftest import sample_notification + + +def test_should_create_initial_job_task_if_notification_is_related_to_a_job( + notify_db, notify_db_session, sample_job, mocker +): + mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") + notification = sample_notification(notify_db, notify_db_session, job=sample_job) + create_initial_notification_statistic_tasks(notification) + mock.assert_called_once_with((str(notification.id), ), queue="notify") + + +def test_should_not_create_initial_job_task_if_notification_is_related_to_a_job( + notify_db, notify_db_session, sample_notification, mocker +): + mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") + create_initial_notification_statistic_tasks(sample_notification) + mock.assert_not_called() + + +def test_should_create_outcome_job_task_if_notification_is_related_to_a_job( + notify_db, notify_db_session, sample_job, mocker +): + mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") + notification = sample_notification(notify_db, notify_db_session, job=sample_job) + create_outcome_notification_statistic_tasks(notification) + mock.assert_called_once_with((str(notification.id), ), queue="notify") + + +def test_should_not_create_outcome_job_task_if_notification_is_related_to_a_job( + notify_db, notify_db_session, sample_notification, mocker +): + mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") + create_outcome_notification_statistic_tasks(sample_notification) + mock.assert_not_called() -import app def test_should_call_create_job_stats_dao_methods(notify_db, notify_db_session, sample_notification, mocker): dao_mock = mocker.patch("app.celery.statistics_tasks.create_or_update_job_sending_statistics") - record_initial_job_statistics(sample_notification) + record_initial_job_statistics(str(sample_notification.id)) dao_mock.assert_called_once_with(sample_notification) @@ -22,14 +61,14 @@ def test_should_retry_if_persisting_the_job_stats_has_a_sql_alchemy_exception( ) retry_mock = mocker.patch('app.celery.statistics_tasks.record_initial_job_statistics.retry') - record_initial_job_statistics(sample_notification) + record_initial_job_statistics(str(sample_notification.id)) dao_mock.assert_called_once_with(sample_notification) retry_mock.assert_called_with(queue="retry") def test_should_call_update_job_stats_dao_outcome_methods(notify_db, notify_db_session, sample_notification, mocker): dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count") - record_outcome_job_statistics(sample_notification) + record_outcome_job_statistics(str(sample_notification.id)) dao_mock.assert_called_once_with(sample_notification) @@ -45,7 +84,7 @@ def test_should_retry_if_persisting_the_job_outcome_stats_has_a_sql_alchemy_exce ) retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') - record_outcome_job_statistics(sample_notification) + record_outcome_job_statistics(str(sample_notification.id)) dao_mock.assert_called_once_with(sample_notification) retry_mock.assert_called_with(queue="retry") @@ -58,6 +97,31 @@ def test_should_retry_if_persisting_the_job_outcome_stats_updates_zero_rows( dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count", return_value=0) retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') - record_outcome_job_statistics(sample_notification) + record_outcome_job_statistics(str(sample_notification.id)) dao_mock.assert_called_once_with(sample_notification) retry_mock.assert_called_with(queue="retry") + + +def test_should_retry_if_persisting_the_job_stats_creation_cant_find_notification_by_id( + notify_db, + notify_db_session, + mocker): + dao_mock = mocker.patch("app.celery.statistics_tasks.create_or_update_job_sending_statistics") + retry_mock = mocker.patch('app.celery.statistics_tasks.record_initial_job_statistics.retry') + + record_initial_job_statistics(str(create_uuid())) + dao_mock.assert_not_called() + retry_mock.assert_called_with(queue="retry") + + +def test_should_retry_if_persisting_the_job_stats_outcome_cant_find_notification_by_id( + notify_db, + notify_db_session, + mocker): + + dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count") + retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') + + record_outcome_job_statistics(str(create_uuid())) + dao_mock.assert_not_called() + retry_mock.assert_called_with(queue="retry") From caed19364745f40047d31b766886f6ea0af2065a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 18:17:55 +0100 Subject: [PATCH 19/68] Use the new task wrapper methods rather than creating a task directly --- app/delivery/send_to_providers.py | 6 ++ .../notifications_ses_callback.py | 5 +- tests/app/delivery/test_send_to_providers.py | 99 ++++++++++++++----- .../test_notifications_ses_callback.py | 71 +++++++++++-- 4 files changed, 145 insertions(+), 36 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index f9ca7861f..0a9aa22f0 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -18,6 +18,8 @@ from app.dao.templates_dao import dao_get_template_by_id from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE, \ NOTIFICATION_SENT, NOTIFICATION_SENDING +from app.celery.statistics_tasks import record_initial_job_statistics, create_initial_notification_statistic_tasks + def send_sms_to_provider(notification): service = notification.service @@ -57,6 +59,8 @@ def send_sms_to_provider(notification): notification.billable_units = template.fragment_count update_notification(notification, provider, notification.international) + create_initial_notification_statistic_tasks(notification) + current_app.logger.info( "SMS {} sent to provider {} at {}".format(notification.id, provider.get_name(), notification.sent_at) ) @@ -107,6 +111,8 @@ def send_email_to_provider(notification): notification.reference = reference update_notification(notification, provider) + create_initial_notification_statistic_tasks(notification) + current_app.logger.info( "Email {} sent to provider at {}".format(notification.id, notification.sent_at) ) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index ce8dec9ba..c5ba3306c 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -13,7 +13,7 @@ from app.clients.email.aws_ses import get_aws_responses from app.dao import ( notifications_dao ) - +from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.notifications.process_client_response import validate_callback_data from app.notifications.utils import confirm_subscription @@ -95,6 +95,9 @@ def process_ses_response(): datetime.utcnow(), notification.sent_at ) + + create_outcome_notification_statistic_tasks(notification) + return jsonify( result="success", message="SES callback succeeded" ), 200 diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 0e35ad413..1d243b3e5 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -1,7 +1,7 @@ import uuid from datetime import datetime from collections import namedtuple -from unittest.mock import ANY +from unittest.mock import ANY, call import pytest from notifications_utils.recipients import validate_and_format_phone_number @@ -18,8 +18,7 @@ from app.models import ( KEY_TYPE_TEST, KEY_TYPE_TEAM, BRANDING_ORG, - BRANDING_BOTH, - ProviderDetails) + BRANDING_BOTH) from tests.app.db import create_service, create_template, create_notification @@ -64,6 +63,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( status='created') mocker.patch('app.mmg_client.send_sms') + stats_mock = mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') send_to_providers.send_sms_to_provider( db_notification @@ -75,6 +75,9 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( reference=str(db_notification.id), sender=None ) + + stats_mock.assert_called_once_with(db_notification) + notification = Notification.query.filter_by(id=db_notification.id).one() assert notification.status == 'sending' @@ -95,6 +98,7 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist ) mocker.patch('app.aws_ses_client.send_email', return_value='reference') + stats_mock = mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') send_to_providers.send_email_to_provider( db_notification @@ -108,6 +112,8 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist html_body=ANY, reply_to_address=None ) + stats_mock.assert_called_once_with(db_notification) + assert ' Date: Tue, 9 May 2017 22:03:13 +0100 Subject: [PATCH 20/68] Put the statistics tasks into a new queue. This currently has no worker in PaaS so blocks the deployment until new PaaS worker is configured. --- app/celery/statistics_tasks.py | 14 ++++++++++---- app/config.py | 6 ++++-- tests/app/celery/test_statistics_tasks.py | 5 ++--- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index 3efcd389d..2c746e57b 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -12,17 +12,18 @@ from app.dao.notifications_dao import get_notification_by_id def create_initial_notification_statistic_tasks(notification): if notification.job_id: - record_initial_job_statistics.apply_async((str(notification.id),), queue="notify") + record_initial_job_statistics.apply_async((str(notification.id),), queue="statistics") def create_outcome_notification_statistic_tasks(notification): if notification.job_id: - record_outcome_job_statistics.apply_async((str(notification.id),), queue="notify") + record_outcome_job_statistics.apply_async((str(notification.id),), queue="statistics") @notify_celery.task(bind=True, name='record_initial_job_statistics', max_retries=20, default_retry_delay=10) @statsd(namespace="tasks") def record_initial_job_statistics(self, notification_id): + notification = None try: notification = get_notification_by_id(notification_id) if notification: @@ -34,13 +35,16 @@ def record_initial_job_statistics(self, notification_id): self.retry(queue="retry") except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task record_initial_job_statistics failed for notification {}".format(notification.id) + "RETRY FAILED: task record_initial_job_statistics failed for notification {}".format( + notification.id if notification else "missing ID" + ) ) @notify_celery.task(bind=True, name='record_outcome_job_statistics', max_retries=20, default_retry_delay=10) @statsd(namespace="tasks") def record_outcome_job_statistics(self, notification_id): + notification = None try: notification = get_notification_by_id(notification_id) if notification: @@ -54,5 +58,7 @@ def record_outcome_job_statistics(self, notification_id): self.retry(queue="retry") except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task update_job_stats_outcome_count failed for notification {}".format(notification.id) + "RETRY FAILED: task update_job_stats_outcome_count failed for notification {}".format( + notification.id if notification else "missing ID" + ) ) diff --git a/app/config.py b/app/config.py index 4b0f170fb..c30d30c32 100644 --- a/app/config.py +++ b/app/config.py @@ -211,7 +211,8 @@ class Development(Config): Queue('db-letter', Exchange('default'), routing_key='db-letter'), Queue('send-sms', Exchange('default'), routing_key='send-sms'), Queue('send-email', Exchange('default'), routing_key='send-email'), - Queue('research-mode', Exchange('default'), routing_key='research-mode') + Queue('research-mode', Exchange('default'), routing_key='research-mode'), + Queue('statistics', Exchange('default'), routing_key='statistics') ] API_HOST_NAME = "http://localhost:6011" API_RATE_LIMIT_ENABLED = True @@ -234,7 +235,8 @@ class Test(Config): Queue('db-letter', Exchange('default'), routing_key='db-letter'), Queue('send-sms', Exchange('default'), routing_key='send-sms'), Queue('send-email', Exchange('default'), routing_key='send-email'), - Queue('research-mode', Exchange('default'), routing_key='research-mode') + Queue('research-mode', Exchange('default'), routing_key='research-mode'), + Queue('statistics', Exchange('default'), routing_key='statistics') ] REDIS_ENABLED = True API_RATE_LIMIT_ENABLED = True diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py index 2dbc4451a..ebbb7716e 100644 --- a/tests/app/celery/test_statistics_tasks.py +++ b/tests/app/celery/test_statistics_tasks.py @@ -14,7 +14,7 @@ def test_should_create_initial_job_task_if_notification_is_related_to_a_job( mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") notification = sample_notification(notify_db, notify_db_session, job=sample_job) create_initial_notification_statistic_tasks(notification) - mock.assert_called_once_with((str(notification.id), ), queue="notify") + mock.assert_called_once_with((str(notification.id), ), queue="statistics") def test_should_not_create_initial_job_task_if_notification_is_related_to_a_job( @@ -31,7 +31,7 @@ def test_should_create_outcome_job_task_if_notification_is_related_to_a_job( mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") notification = sample_notification(notify_db, notify_db_session, job=sample_job) create_outcome_notification_statistic_tasks(notification) - mock.assert_called_once_with((str(notification.id), ), queue="notify") + mock.assert_called_once_with((str(notification.id), ), queue="statistics") def test_should_not_create_outcome_job_task_if_notification_is_related_to_a_job( @@ -42,7 +42,6 @@ def test_should_not_create_outcome_job_task_if_notification_is_related_to_a_job( mock.assert_not_called() - def test_should_call_create_job_stats_dao_methods(notify_db, notify_db_session, sample_notification, mocker): dao_mock = mocker.patch("app.celery.statistics_tasks.create_or_update_job_sending_statistics") record_initial_job_statistics(str(sample_notification.id)) From 19c14982a20c3e43b1c139b24a43cdf710d0b173 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 May 2017 22:03:57 +0100 Subject: [PATCH 21/68] Call the task wrapper outcome function in the statistics_tasks file. This wraps the logic around which tasks to creates and simplifies the logic in the Callback classes. --- app/notifications/process_client_response.py | 10 ++++-- .../test_notifications_ses_callback.py | 24 +++++++++---- .../test_process_client_response.py | 35 ++++++++++++++++--- 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index f07366422..534408ce6 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -7,6 +7,8 @@ from app import statsd_client from app.dao import notifications_dao from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.mmg import get_mmg_responses +from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks + sms_response_mapper = { 'MMG': get_mmg_responses, @@ -45,8 +47,9 @@ def process_sms_client_response(status, reference, client_name): # validate status try: response_dict = response_parser(status) - current_app.logger.info('{} callback return status of {} for reference: {}'.format(client_name, - status, reference)) + current_app.logger.info('{} callback return status of {} for reference: {}'.format( + client_name, status, reference) + ) except KeyError: msg = "{} callback failed: status {} not found.".format(client_name, status) return success, msg @@ -77,5 +80,8 @@ def process_sms_client_response(status, reference, client_name): datetime.utcnow(), notification.sent_at ) + + create_outcome_notification_statistic_tasks(notification) + success = "{} callback succeeded. reference {} updated".format(client_name, reference) return success, errors diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index c1bf8c78e..0a02b5d58 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -21,7 +21,9 @@ def test_ses_callback_should_not_need_auth(client): def test_ses_callback_should_fail_if_invalid_json(client, mocker): - stats_mock = mocker.patch('app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks') + stats_mock = mocker.patch( + 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' + ) response = client.post( path='/notifications/email/ses', @@ -36,7 +38,9 @@ def test_ses_callback_should_fail_if_invalid_json(client, mocker): def test_ses_callback_should_autoconfirm_subscriptions(client, rmock, mocker): - stats_mock = mocker.patch('app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks') + stats_mock = mocker.patch( + 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' + ) endpoint = json.loads(ses_confirmation_callback())['SubscribeURL'] rmock.request( @@ -61,7 +65,9 @@ def test_ses_callback_should_autoconfirm_subscriptions(client, rmock, mocker): def test_ses_callback_autoconfirm_raises_exception_if_not_200(client, rmock, mocker): - stats_mock = mocker.patch('app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks') + stats_mock = mocker.patch( + 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' + ) endpoint = json.loads(ses_confirmation_callback())['SubscribeURL'] rmock.request( @@ -84,7 +90,9 @@ def test_ses_callback_autoconfirm_raises_exception_if_not_200(client, rmock, moc def test_ses_callback_should_fail_if_invalid_notification_type(client, mocker): - stats_mock = mocker.patch('app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks') + stats_mock = mocker.patch( + 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' + ) response = client.post( path='/notifications/email/ses', @@ -99,7 +107,9 @@ def test_ses_callback_should_fail_if_invalid_notification_type(client, mocker): def test_ses_callback_should_fail_if_missing_message_id(client, mocker): - stats_mock = mocker.patch('app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks') + stats_mock = mocker.patch( + 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' + ) response = client.post( path='/notifications/email/ses', @@ -114,7 +124,9 @@ def test_ses_callback_should_fail_if_missing_message_id(client, mocker): def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, notify_db_session, client, mocker): - stats_mock = mocker.patch('app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks') + stats_mock = mocker.patch( + 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' + ) response = client.post( path='/notifications/email/ses', diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 57bb659f9..a4e3d2413 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -45,25 +45,52 @@ def test_validate_callback_data_returns_error_for_empty_string(): assert "{} callback failed: {} missing".format(client_name, 'status') in result -def test_process_sms_response_return_success_for_send_sms_code_reference(): +def test_outcome_statistics_called_for_successful_callback(sample_notification, mocker): + stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') + mocker.patch( + 'app.notifications.process_client_response.notifications_dao.update_notification_status_by_id', + return_value=sample_notification + ) + + reference = str(uuid.uuid4()) + + success, error = process_sms_client_response(status='3', reference=reference, client_name='MMG') + assert success == "MMG callback succeeded. reference {} updated".format(str(reference)) + assert error is None + stats_mock.assert_called_once_with(sample_notification) + + +def test_process_sms_response_return_success_for_send_sms_code_reference(mocker): + stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') + success, error = process_sms_client_response(status='000', reference='send-sms-code', client_name='sms-client') assert success == "{} callback succeeded: send-sms-code".format('sms-client') assert error is None + stats_mock.assert_not_called() -def test_process_sms_response_returns_error_bad_reference(): +def test_process_sms_response_returns_error_bad_reference(mocker): + stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') + success, error = process_sms_client_response(status='000', reference='something-bad', client_name='sms-client') assert success is None assert error == "{} callback with invalid reference {}".format('sms-client', 'something-bad') + stats_mock.assert_not_called() -def test_process_sms_response_returns_error_for_unknown_sms_client(): +def test_process_sms_response_returns_error_for_unknown_sms_client(mocker): + stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') success, error = process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='sms-client') + assert success is None assert error == 'unknown sms client: {}'.format('sms-client') + stats_mock.assert_not_called() -def test_process_sms_response_returns_error_for_unknown_status(): +def test_process_sms_response_returns_error_for_unknown_status(mocker): + stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') + success, error = process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='Firetext') assert success is None assert error == "{} callback failed: status {} not found.".format('Firetext', '000') + stats_mock.assert_not_called() From 22a47106af1c4c8e73a22748104ce2219ab5ad1a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 12:09:57 +0100 Subject: [PATCH 22/68] Refactored the DAO to be clearer, and wrote tests for the bug whereby different types of inserts/updates caused others to reset. --- app/dao/statistics_dao.py | 69 ++++--- tests/app/dao/test_statistics_dao.py | 277 ++++++++++++++++++++++++++- 2 files changed, 312 insertions(+), 34 deletions(-) diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index caa37aaf0..9d9044b9b 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -11,8 +11,8 @@ from app.models import ( SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_FAILED, - NOTIFICATION_DELIVERED -) + NOTIFICATION_DELIVERED, + NOTIFICATION_SENT) from app.statsd_decorators import statsd @@ -29,22 +29,17 @@ def create_or_update_job_sending_statistics(notification): @transactional def __update_job_stats_sent_count(notification): - update = { - JobStatistics.emails_sent: - JobStatistics.emails_sent + 1 if notification.notification_type == EMAIL_TYPE else 0, - JobStatistics.sms_sent: - JobStatistics.sms_sent + 1 if notification.notification_type == SMS_TYPE else 0, - JobStatistics.letters_sent: - JobStatistics.letters_sent + 1 if notification.notification_type == LETTER_TYPE else 0 - } + column = columns(notification.notification_type, 'sent') + return db.session.query(JobStatistics).filter_by( job_id=notification.job_id, - ).update(update) + ).update({ + column: column + 1 + }) @transactional def __insert_job_stats(notification): - stats = JobStatistics( job_id=notification.job_id, emails_sent=1 if notification.notification_type == EMAIL_TYPE else 0, @@ -55,31 +50,43 @@ def __insert_job_stats(notification): db.session.add(stats) +def columns(notification_type, status): + keys = { + EMAIL_TYPE: { + 'failed': JobStatistics.emails_failed, + 'delivered': JobStatistics.emails_delivered, + 'sent': JobStatistics.emails_sent + }, + SMS_TYPE: { + 'failed': JobStatistics.sms_failed, + 'delivered': JobStatistics.sms_delivered, + 'sent': JobStatistics.sms_sent + }, + LETTER_TYPE: { + 'failed': JobStatistics.letters_failed, + 'sent': JobStatistics.letters_sent + } + } + return keys.get(notification_type).get(status) + + @transactional def update_job_stats_outcome_count(notification): - update = None - if notification.status in NOTIFICATION_STATUS_TYPES_FAILED: - update = { - JobStatistics.emails_failed: - JobStatistics.emails_failed + 1 if notification.notification_type == EMAIL_TYPE else 0, - JobStatistics.sms_failed: - JobStatistics.sms_failed + 1 if notification.notification_type == SMS_TYPE else 0, - JobStatistics.letters_failed: - JobStatistics.letters_failed + 1 if notification.notification_type == LETTER_TYPE else 0 - } + column = columns(notification.notification_type, 'failed') - elif notification.status == NOTIFICATION_DELIVERED and notification.notification_type != LETTER_TYPE: - update = { - JobStatistics.emails_delivered: - JobStatistics.emails_delivered + 1 if notification.notification_type == EMAIL_TYPE else 0, - JobStatistics.sms_delivered: - JobStatistics.sms_delivered + 1 if notification.notification_type == SMS_TYPE else 0 - } + elif notification.status in [NOTIFICATION_DELIVERED, + NOTIFICATION_SENT] and notification.notification_type != LETTER_TYPE: + column = columns(notification.notification_type, 'delivered') - if update: + else: + column = None + + if column: return db.session.query(JobStatistics).filter_by( job_id=notification.job_id, - ).update(update) + ).update({ + column: column + 1 + }) else: return 0 diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index 21979ba1a..a1284575a 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -271,21 +271,82 @@ def test_should_update_a_stats_entry_with_its_error_outcomes_for_a_job( assert stat.sms_delivered == 0 +@pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ + (SMS_TYPE, 1, 0, 0, NOTIFICATION_DELIVERED), + (SMS_TYPE, 1, 0, 0, NOTIFICATION_SENT), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_DELIVERED), + (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_SENT), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_SENT), + (LETTER_TYPE, 0, 0, 1, NOTIFICATION_DELIVERED), +]) +def test_should_update_a_stats_entry_with_its_success_outcomes_for_a_job( + notify_db, + notify_db_session, + sample_job, + sample_letter_template, + notification_type, + sms_count, + email_count, + letter_count, + status +): + assert not len(JobStatistics.query.all()) + + template = None + + if notification_type == SMS_TYPE: + template = sample_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == EMAIL_TYPE: + template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + if notification_type == LETTER_TYPE: + template = sample_letter_template + + notification = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=template, + job=sample_job, + status=status + ) + + create_or_update_job_sending_statistics(notification) + + stats = JobStatistics.query.all() + + assert len(stats) == 1 + + update_job_stats_outcome_count(notification) + + stat = stats[0] + assert stat.job_id == sample_job.id + + assert stat.emails_sent == email_count + assert stat.sms_sent == sms_count + assert stat.letters_sent == letter_count + + assert stat.emails_failed == 0 + assert stat.letters_failed == 0 + assert stat.sms_failed == 0 + + assert stat.emails_delivered == email_count + assert stat.sms_delivered == sms_count + + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ (SMS_TYPE, 1, 0, 0, NOTIFICATION_PENDING), (SMS_TYPE, 1, 0, 0, NOTIFICATION_CREATED), (SMS_TYPE, 1, 0, 0, NOTIFICATION_FAILED), - (SMS_TYPE, 1, 0, 0, NOTIFICATION_SENT), (SMS_TYPE, 1, 0, 0, NOTIFICATION_SENDING), (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_PENDING), (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_CREATED), (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_FAILED), - (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_SENT), (EMAIL_TYPE, 0, 1, 0, NOTIFICATION_SENDING), (LETTER_TYPE, 0, 0, 1, NOTIFICATION_PENDING), (LETTER_TYPE, 0, 0, 1, NOTIFICATION_CREATED), (LETTER_TYPE, 0, 0, 1, NOTIFICATION_FAILED), - (LETTER_TYPE, 0, 0, 1, NOTIFICATION_SENT), (LETTER_TYPE, 0, 0, 1, NOTIFICATION_SENDING) ]) def test_should_not_update_job_stats_if_irrelevant_status( @@ -342,3 +403,213 @@ def test_should_not_update_job_stats_if_irrelevant_status( assert stat.emails_delivered == 0 assert stat.sms_delivered == 0 + + +@pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ + (SMS_TYPE, 2, 1, 1), + (EMAIL_TYPE, 1, 2, 1), + (LETTER_TYPE, 1, 1, 2) +]) +def test_inserting_one_type_of_notification_maintains_other_counts( + notify_db, + notify_db_session, + sample_job, + sample_letter_template, + notification_type, + sms_count, + email_count, + letter_count +): + assert not len(JobStatistics.query.all()) + + sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + letter_template = sample_letter_template + + template = None + + if notification_type == SMS_TYPE: + template = sms_template + + if notification_type == EMAIL_TYPE: + template = email_template + + if notification_type == LETTER_TYPE: + template = letter_template + + notification = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + letter = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=letter_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + email = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=email_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + sms = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=sms_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + create_or_update_job_sending_statistics(email) + create_or_update_job_sending_statistics(sms) + create_or_update_job_sending_statistics(letter) + + intitial_stats = JobStatistics.query.all() + assert len(intitial_stats) == 1 + assert intitial_stats[0].emails_sent == 1 + assert intitial_stats[0].sms_sent == 1 + assert intitial_stats[0].letters_sent == 1 + + create_or_update_job_sending_statistics(notification) + + updated_stats = JobStatistics.query.all() + assert updated_stats[0].job_id == sample_job.id + + assert updated_stats[0].emails_sent == email_count + assert updated_stats[0].sms_sent == sms_count + assert updated_stats[0].letters_sent == letter_count + + +def test_updating_one_type_of_notification_to_success_maintains_other_counts( + notify_db, + notify_db_session, + sample_job, + sample_letter_template +): + assert not len(JobStatistics.query.all()) + + sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + letter_template = sample_letter_template + + letter = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=letter_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + email = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=email_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + sms = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=sms_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + create_or_update_job_sending_statistics(email) + create_or_update_job_sending_statistics(sms) + create_or_update_job_sending_statistics(letter) + + sms.status = NOTIFICATION_DELIVERED + email.status = NOTIFICATION_DELIVERED + letter.status = NOTIFICATION_DELIVERED + + update_job_stats_outcome_count(letter) + update_job_stats_outcome_count(email) + update_job_stats_outcome_count(sms) + + stats = JobStatistics.query.all() + assert len(stats) == 1 + assert stats[0].emails_sent == 1 + assert stats[0].sms_sent == 1 + assert stats[0].letters_sent == 1 + assert stats[0].emails_delivered == 1 + assert stats[0].sms_delivered == 1 + + +def test_updating_one_type_of_notification_to_error_maintains_other_counts( + notify_db, + notify_db_session, + sample_job, + sample_letter_template +): + assert not len(JobStatistics.query.all()) + + sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + letter_template = sample_letter_template + + letter = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=letter_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + email = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=email_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + sms = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=sms_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + create_or_update_job_sending_statistics(email) + create_or_update_job_sending_statistics(sms) + create_or_update_job_sending_statistics(letter) + + sms.status = NOTIFICATION_TECHNICAL_FAILURE + email.status = NOTIFICATION_TECHNICAL_FAILURE + letter.status = NOTIFICATION_TECHNICAL_FAILURE + + update_job_stats_outcome_count(letter) + update_job_stats_outcome_count(email) + update_job_stats_outcome_count(sms) + + stats = JobStatistics.query.all() + assert len(stats) == 1 + assert stats[0].emails_sent == 1 + assert stats[0].sms_sent == 1 + assert stats[0].letters_sent == 1 + assert stats[0].emails_delivered == 0 + assert stats[0].sms_delivered == 0 + assert stats[0].sms_failed == 1 + assert stats[0].emails_failed == 1 From 76494a285e66294763244e3bd39e264e0c531d5e Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 12:10:46 +0100 Subject: [PATCH 23/68] New logic to: - increment sent only if the notifications is in a NON-finished state - Increment outcome only if notification is in a finished state --- app/celery/statistics_tasks.py | 7 ++- tests/app/celery/test_statistics_tasks.py | 58 +++++++++++++++++++++-- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index 2c746e57b..d7f0fc8c3 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -2,21 +2,24 @@ from sqlalchemy.exc import SQLAlchemyError from app import notify_celery from flask import current_app + +from app.models import JobStatistics from app.statsd_decorators import statsd from app.dao.statistics_dao import ( create_or_update_job_sending_statistics, update_job_stats_outcome_count ) from app.dao.notifications_dao import get_notification_by_id +from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED def create_initial_notification_statistic_tasks(notification): - if notification.job_id: + if notification.job_id and notification.status not in NOTIFICATION_STATUS_TYPES_COMPLETED: record_initial_job_statistics.apply_async((str(notification.id),), queue="statistics") def create_outcome_notification_statistic_tasks(notification): - if notification.job_id: + if notification.job_id and notification.status in NOTIFICATION_STATUS_TYPES_COMPLETED: record_outcome_job_statistics.apply_async((str(notification.id),), queue="statistics") diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py index ebbb7716e..cdab47b5f 100644 --- a/tests/app/celery/test_statistics_tasks.py +++ b/tests/app/celery/test_statistics_tasks.py @@ -1,3 +1,4 @@ +import pytest from app.celery.statistics_tasks import ( record_initial_job_statistics, record_outcome_job_statistics, @@ -6,6 +7,8 @@ from app.celery.statistics_tasks import ( from sqlalchemy.exc import SQLAlchemyError from app import create_uuid from tests.app.conftest import sample_notification +from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED, NOTIFICATION_SENT, NOTIFICATION_SENDING, \ + NOTIFICATION_PENDING, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED def test_should_create_initial_job_task_if_notification_is_related_to_a_job( @@ -17,11 +20,34 @@ def test_should_create_initial_job_task_if_notification_is_related_to_a_job( mock.assert_called_once_with((str(notification.id), ), queue="statistics") -def test_should_not_create_initial_job_task_if_notification_is_related_to_a_job( - notify_db, notify_db_session, sample_notification, mocker +@pytest.mark.parametrize('status', [ + NOTIFICATION_SENDING, NOTIFICATION_CREATED, NOTIFICATION_PENDING +]) +def test_should_create_intial_job_task_if_notification_is_not_in_completed_state( + notify_db, notify_db_session, sample_job, mocker, status ): mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") - create_initial_notification_statistic_tasks(sample_notification) + notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) + create_initial_notification_statistic_tasks(notification) + mock.assert_called_once_with((str(notification.id), ), queue="statistics") + + +@pytest.mark.parametrize('status', NOTIFICATION_STATUS_TYPES_COMPLETED) +def test_should_not_create_initial_job_task_if_notification_in_completed_state_already( + notify_db, notify_db_session, sample_job, mocker, status +): + mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") + notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) + create_initial_notification_statistic_tasks(notification) + mock.assert_not_called() + + +def test_should_not_create_initial_job_task_if_notification_is_not_related_to_a_job( + notify_db, notify_db_session, mocker +): + notification = sample_notification(notify_db, notify_db_session, status=NOTIFICATION_CREATED) + mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") + create_initial_notification_statistic_tasks(notification) mock.assert_not_called() @@ -29,12 +55,34 @@ def test_should_create_outcome_job_task_if_notification_is_related_to_a_job( notify_db, notify_db_session, sample_job, mocker ): mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") - notification = sample_notification(notify_db, notify_db_session, job=sample_job) + notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=NOTIFICATION_DELIVERED) create_outcome_notification_statistic_tasks(notification) mock.assert_called_once_with((str(notification.id), ), queue="statistics") -def test_should_not_create_outcome_job_task_if_notification_is_related_to_a_job( +@pytest.mark.parametrize('status', NOTIFICATION_STATUS_TYPES_COMPLETED) +def test_should_create_outcome_job_task_if_notification_is_in_completed_state( + notify_db, notify_db_session, sample_job, mocker, status +): + mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") + notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) + create_outcome_notification_statistic_tasks(notification) + mock.assert_called_once_with((str(notification.id), ), queue='statistics') + + +@pytest.mark.parametrize('status', [ + NOTIFICATION_SENDING, NOTIFICATION_CREATED, NOTIFICATION_PENDING +]) +def test_should_not_create_outcome_job_task_if_notification_is_not_in_completed_state_already( + notify_db, notify_db_session, sample_job, mocker, status +): + mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") + notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) + create_outcome_notification_statistic_tasks(notification) + mock.assert_not_called() + + +def test_should_not_create_outcome_job_task_if_notification_is_not_related_to_a_job( notify_db, notify_db_session, sample_notification, mocker ): mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") From e993a91a11042863c547927fa5beac0e52a98b3e Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 15:21:27 +0100 Subject: [PATCH 24/68] Added a created at column to the job stats table. --- .../versions/0082_add_created_to_job_stats.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 migrations/versions/0082_add_created_to_job_stats.py diff --git a/migrations/versions/0082_add_created_to_job_stats.py b/migrations/versions/0082_add_created_to_job_stats.py new file mode 100644 index 000000000..9d8d72271 --- /dev/null +++ b/migrations/versions/0082_add_created_to_job_stats.py @@ -0,0 +1,22 @@ +"""empty message + +Revision ID: 0082_add_created_to_job_stats +Revises: 0081_add_job_stats +Create Date: 2017-05-11 15:05:53.420946 + +""" + +# revision identifiers, used by Alembic. +revision = '0082_add_created_to_job_stats' +down_revision = '0081_add_job_stats' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('job_statistics', sa.Column('created_at', sa.DateTime(), nullable=True)) + + +def downgrade(): + op.drop_column('job_statistics', 'created_at') From b51932179854844f291ad369e23c331087794e3c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 15:22:43 +0100 Subject: [PATCH 25/68] Adds a query to timeout the job counts. After three days we timeout the notifications that we have not received a receipt for. In the same way we bump the failed count to match the job count if there is a descepenecy. We do this after the same period that we do for notifications. --- app/dao/statistics_dao.py | 25 ++++++- tests/app/dao/test_statistics_dao.py | 101 ++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 3 deletions(-) diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index 9d9044b9b..ca5e134a9 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, timedelta from flask import current_app from sqlalchemy.exc import IntegrityError, SQLAlchemyError @@ -16,6 +16,29 @@ from app.models import ( from app.statsd_decorators import statsd +@transactional +def timeout_job_counts(notifications_type, timeout_start): + sent = columns(notifications_type, 'sent') + delivered = columns(notifications_type, 'delivered') + failed = columns(notifications_type, 'failed') + + query = JobStatistics.query.filter( + JobStatistics.created_at < timeout_start, + sent != failed + delivered + ) + return query.update( + {failed: sent - delivered}, synchronize_session=False + ) + + +@statsd(namespace="dao") +def timeout_job_statistics(timeout_period): + timeout_start = datetime.utcnow() - timedelta(seconds=timeout_period) + sms_count = timeout_job_counts(SMS_TYPE, timeout_start) + email_count = timeout_job_counts(EMAIL_TYPE, timeout_start) + return sms_count + email_count + + @statsd(namespace="dao") def create_or_update_job_sending_statistics(notification): if __update_job_stats_sent_count(notification) == 0: diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index a1284575a..7aa0b0fd8 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -1,3 +1,4 @@ +from datetime import datetime, timedelta from unittest.mock import call import pytest @@ -5,8 +6,8 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app.dao.statistics_dao import ( create_or_update_job_sending_statistics, - update_job_stats_outcome_count -) + update_job_stats_outcome_count, + timeout_job_statistics) from app.models import ( JobStatistics, SMS_TYPE, @@ -613,3 +614,99 @@ def test_updating_one_type_of_notification_to_error_maintains_other_counts( assert stats[0].sms_delivered == 0 assert stats[0].sms_failed == 1 assert stats[0].emails_failed == 1 + + +def test_will_timeout_job_counts_after_notification_timeouts(notify_db, notify_db_session, sample_job): + sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + one_minute_ago = datetime.utcnow() - timedelta(minutes=1) + + sms = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=sms_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + email = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=email_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + create_or_update_job_sending_statistics(email) + create_or_update_job_sending_statistics(sms) + + JobStatistics.query.update({JobStatistics.created_at: one_minute_ago}) + + intial_stats = JobStatistics.query.all() + + assert intial_stats[0].emails_sent == 1 + assert intial_stats[0].sms_sent == 1 + assert intial_stats[0].emails_delivered == 0 + assert intial_stats[0].sms_delivered == 0 + assert intial_stats[0].sms_failed == 0 + assert intial_stats[0].emails_failed == 0 + + timeout_job_statistics(59) + updated_stats = JobStatistics.query.all() + assert updated_stats[0].emails_sent == 1 + assert updated_stats[0].sms_sent == 1 + assert updated_stats[0].emails_delivered == 0 + assert updated_stats[0].sms_delivered == 0 + assert updated_stats[0].sms_failed == 1 + assert updated_stats[0].emails_failed == 1 + + +def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, notify_db_session, sample_job): + sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + + one_minute_ago = datetime.utcnow() - timedelta(minutes=1) + + sms = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=sms_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + email = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=email_template, + job=sample_job, + status=NOTIFICATION_CREATED + ) + + create_or_update_job_sending_statistics(email) + create_or_update_job_sending_statistics(sms) + + JobStatistics.query.update({JobStatistics.created_at: one_minute_ago}) + + intial_stats = JobStatistics.query.all() + + assert intial_stats[0].emails_sent == 1 + assert intial_stats[0].sms_sent == 1 + assert intial_stats[0].emails_delivered == 0 + assert intial_stats[0].sms_delivered == 0 + assert intial_stats[0].sms_failed == 0 + assert intial_stats[0].emails_failed == 0 + + timeout_job_statistics(61) + updated_stats = JobStatistics.query.all() + assert updated_stats[0].emails_sent == 1 + assert updated_stats[0].sms_sent == 1 + assert updated_stats[0].emails_delivered == 0 + assert updated_stats[0].sms_delivered == 0 + assert updated_stats[0].sms_failed == 0 + assert updated_stats[0].emails_failed == 0 From 8818bd5dbaf2856f46e71bab559c26eb84954256 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 15:22:57 +0100 Subject: [PATCH 26/68] Scheduled task to call the timeout function --- app/celery/scheduled_tasks.py | 10 ++++++++++ tests/app/celery/test_scheduled_tasks.py | 9 ++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index b76122819..3acdd5341 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -16,6 +16,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, is_delivery_slow_for_provider ) +from app.dao.statistics_dao import timeout_job_statistics as dao_timeout_job_statistics from app.dao.provider_details_dao import ( get_current_provider, dao_toggle_sms_provider @@ -180,3 +181,12 @@ def switch_current_sms_provider_on_slow_delivery(): ) dao_toggle_sms_provider(current_provider.identifier) + + +@notify_celery.task(name='timeout-job-statistics') +@statsd(namespace="tasks") +def timeout_job_statistics(): + updated = dao_timeout_job_statistics(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) + if updated: + current_app.logger.info( + "Timeout period reached for {} job statistics, failure count has been updated.".format(updated)) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 038d2ca1c..50bf5f982 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -5,7 +5,7 @@ from functools import partial from flask import current_app from freezegun import freeze_time -from app.celery.scheduled_tasks import s3 +from app.celery.scheduled_tasks import s3, timeout_job_statistics from app.celery import scheduled_tasks from app.celery.scheduled_tasks import ( delete_verify_codes, @@ -409,3 +409,10 @@ def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifi switch_current_sms_provider_on_slow_delivery() current_provider = get_current_provider('sms') assert starting_provider.identifier == current_provider.identifier + + +def test_timeout_job_statistics_called_with_notification_timeout(notify_api, mocker): + notify_api.config['SENDING_NOTIFICATIONS_TIMEOUT_PERIOD'] = 999 + dao_mock = mocker.patch('app.celery.scheduled_tasks.dao_timeout_job_statistics') + timeout_job_statistics() + dao_mock.assert_called_once_with(999) From f0e47029f28d82ab1e527910ed9e64e450d77a62 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 15:23:18 +0100 Subject: [PATCH 27/68] "Created at" on the model --- app/models.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/models.py b/app/models.py index 34e9e1376..a92c2c727 100644 --- a/app/models.py +++ b/app/models.py @@ -988,6 +988,12 @@ class JobStatistics(db.Model): sms_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + created_at = db.Column( + db.DateTime, + index=False, + unique=False, + nullable=True, + default=datetime.datetime.utcnow) updated_at = db.Column( db.DateTime, index=False, From 02f7abcc48352caa99c1a4a4aec522756b41fbdd Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 15:23:45 +0100 Subject: [PATCH 28/68] Wire in the new task. Runs at 5am after the other tasks. --- app/config.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/config.py b/app/config.py index c30d30c32..6129b7023 100644 --- a/app/config.py +++ b/app/config.py @@ -147,6 +147,11 @@ class Config(object): 'task': 'remove_csv_files', 'schedule': crontab(minute=0, hour=4), 'options': {'queue': 'periodic'} + }, + 'timeout-job-statistics': { + 'task': 'timeout-job-statistics', + 'schedule': crontab(minute=0, hour=5), + 'options': {'queue': 'periodic'} } } CELERY_QUEUES = [ @@ -198,6 +203,7 @@ class Config(object): ###################### class Development(Config): + SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'notify.tools' CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' NOTIFY_ENVIRONMENT = 'development' From e382d699a66f80691efb280ca62b6e4002c6efab Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 15:24:10 +0100 Subject: [PATCH 29/68] using the periodic worker to process the stats tasks. May need it's own worker in due course, but right now piggybacking. --- manifest-delivery-base.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 2b85b9d15..e35324240 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -38,7 +38,7 @@ applications: NOTIFY_APP_NAME: delivery-worker-sender - name: notify-delivery-worker-periodic - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=2 -Q periodic + command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=2 -Q periodic, statistics instances: 1 memory: 2G env: From 76a267b8969adecc80fe270226b43195b42d046c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 11 May 2017 15:40:56 +0100 Subject: [PATCH 30/68] Script to update international flag 10,000 rows at a time. --- migrations/versions/0083_set_international.py | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 migrations/versions/0083_set_international.py diff --git a/migrations/versions/0083_set_international.py b/migrations/versions/0083_set_international.py new file mode 100644 index 000000000..8251901eb --- /dev/null +++ b/migrations/versions/0083_set_international.py @@ -0,0 +1,45 @@ +"""empty message + +Revision ID: 0083_set_international +Revises: 0082_add_go_live_template +Create Date: 2017-05-05 15:26:34.621670 + +""" +from datetime import datetime +from alembic import op + +# revision identifiers, used by Alembic. +revision = '0083_set_international' +down_revision = '0082_add_go_live_template' + + +def upgrade(): + conn = op.get_bind() + start = datetime.utcnow() + notification_history = "select id from notification_history where international is null limit 10000" + + results = conn.execute(notification_history) + res = results.fetchall() + + while len(res) > 0: + conn.execute("update notification_history set international = False where id in ({})".format( + notification_history)) + results = conn.execute(notification_history) + res = results.fetchall() + + notifications = "select id from notifications where international is null limit 10000" + results2 = conn.execute(notifications) + res2 = results2.fetchall() + while len(res2) > 0: + conn.execute("update notifications set international = False where id in ({})".format(notifications)) + + results2 = conn.execute(notifications) + res2 = results2.fetchall() + + end = datetime.utcnow() + print("Started at: {} ended at: {}".format(start, end)) + + +def downgrade(): + # There is no way to downgrade this update. + pass From 2643a891fcaa010879222433cd79d57b9f8e592f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 11 May 2017 15:43:52 +0100 Subject: [PATCH 31/68] Renumber the sql migrations. --- .../{0081_add_job_stats.py => 0083_add_job_stats.py} | 6 +++--- ...ted_to_job_stats.py => 0084_add_created_to_job_stats.py} | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) rename migrations/versions/{0081_add_job_stats.py => 0083_add_job_stats.py} (92%) rename migrations/versions/{0082_add_created_to_job_stats.py => 0084_add_created_to_job_stats.py} (74%) diff --git a/migrations/versions/0081_add_job_stats.py b/migrations/versions/0083_add_job_stats.py similarity index 92% rename from migrations/versions/0081_add_job_stats.py rename to migrations/versions/0083_add_job_stats.py index c1ef5e205..cf028ded6 100644 --- a/migrations/versions/0081_add_job_stats.py +++ b/migrations/versions/0083_add_job_stats.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0081_add_job_stats +Revision ID: 0083_add_job_stats Revises: 0080_fix_rate_start_date Create Date: 2017-05-09 12:44:43.173269 """ # revision identifiers, used by Alembic. -revision = '0081_add_job_stats' -down_revision = '0080_fix_rate_start_date' +revision = '0083_add_job_stats' +down_revision = '0082_add_go_live_template' from alembic import op import sqlalchemy as sa diff --git a/migrations/versions/0082_add_created_to_job_stats.py b/migrations/versions/0084_add_created_to_job_stats.py similarity index 74% rename from migrations/versions/0082_add_created_to_job_stats.py rename to migrations/versions/0084_add_created_to_job_stats.py index 9d8d72271..c7470bef1 100644 --- a/migrations/versions/0082_add_created_to_job_stats.py +++ b/migrations/versions/0084_add_created_to_job_stats.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0082_add_created_to_job_stats +Revision ID: 0084_add_created_to_job_stats Revises: 0081_add_job_stats Create Date: 2017-05-11 15:05:53.420946 """ # revision identifiers, used by Alembic. -revision = '0082_add_created_to_job_stats' -down_revision = '0081_add_job_stats' +revision = '0084_add_created_to_job_stats' +down_revision = '0083_add_job_stats' from alembic import op import sqlalchemy as sa From e443dc7fed5088a051e9326364e91be9e4a6fb3c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 May 2017 16:50:36 +0100 Subject: [PATCH 32/68] Output contact blocks with placeholders replaced Brings in: - [ ] https://github.com/alphagov/notifications-utils/pull/161 Also brings in: https://github.com/alphagov/notifications-utils/compare/16.1.3...17.0.2 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 3f688a907..fe25533c1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,6 +28,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@16.1.3#egg=notifications-utils==16.1.3 +git+https://github.com/alphagov/notifications-utils.git@17.1.0#egg=notifications-utils==17.1.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 8d4ccc300336ac1d12a7ec7da1b9462f1980b318 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 12:17:34 +0100 Subject: [PATCH 33/68] Updated jobs_Dao to make a job stats row when making a job - saves the ambiguity later as to whether the row exists. --- app/dao/jobs_dao.py | 7 ++++++- tests/app/dao/test_jobs_dao.py | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 6d4a8d6cb..b712b0e70 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -11,7 +11,7 @@ from app.models import (Job, Template, JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, - LETTER_TYPE) + LETTER_TYPE, JobStatistics) from app.statsd_decorators import statsd @@ -109,6 +109,11 @@ def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): def dao_create_job(job): + job_stats = JobStatistics( + job_id=job.id, + updated_at=datetime.utcnow() + ) + db.session.add(job_stats) db.session.add(job) db.session.commit() diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index f01b631ba..034048a32 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -16,7 +16,7 @@ from app.dao.jobs_dao import ( dao_update_job_status, dao_get_all_notifications_for_job, dao_get_jobs_older_than_limited_by) -from app.models import Job +from app.models import Job, JobStatistics from tests.app.conftest import sample_notification as create_notification from tests.app.conftest import sample_job as create_job @@ -130,10 +130,23 @@ def test_create_job(sample_template): dao_create_job(job) assert Job.query.count() == 1 + assert JobStatistics.query.count() == 1 job_from_db = Job.query.get(job_id) assert job == job_from_db assert job_from_db.notifications_delivered == 0 assert job_from_db.notifications_failed == 0 + job_stats_from_db = JobStatistics.query.filter_by(job_id=job_id).all() + assert len(job_stats_from_db) == 1 + assert job_stats_from_db[0].sms_sent == 0 + assert job_stats_from_db[0].emails_sent == 0 + assert job_stats_from_db[0].letters_sent == 0 + + assert job_stats_from_db[0].sms_failed == 0 + assert job_stats_from_db[0].emails_failed == 0 + assert job_stats_from_db[0].letters_failed == 0 + + assert job_stats_from_db[0].sms_delivered == 0 + assert job_stats_from_db[0].emails_delivered == 0 def test_get_job_by_id(sample_job): From f84694fb29d7a2901a4616c0996a6afef9e8b0cb Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 12:19:27 +0100 Subject: [PATCH 34/68] updated the timeout query to base outcome on notifications Previous: assumed discrepancy in stats counts to be related to timeouts Now: If discrepancy exists do the math on the notifications for that job to work out counts based on statuses to redo stats. --- app/dao/statistics_dao.py | 44 +++++- tests/app/dao/test_statistics_dao.py | 215 ++++++++++++++++++--------- 2 files changed, 183 insertions(+), 76 deletions(-) diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index ca5e134a9..20c75112e 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -1,12 +1,15 @@ from datetime import datetime, timedelta +from itertools import groupby from flask import current_app +from sqlalchemy import func from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app import db from app.dao.dao_utils import transactional from app.models import ( JobStatistics, + Notification, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, @@ -18,21 +21,50 @@ from app.statsd_decorators import statsd @transactional def timeout_job_counts(notifications_type, timeout_start): + total_updated = 0 + sent = columns(notifications_type, 'sent') delivered = columns(notifications_type, 'delivered') failed = columns(notifications_type, 'failed') - query = JobStatistics.query.filter( + results = db.session.query( + JobStatistics.job_id.label('job_id'), + func.count(Notification.status).label('count'), + Notification.status.label('status') + ).filter( + JobStatistics.job_id == Notification.job_id, JobStatistics.created_at < timeout_start, sent != failed + delivered - ) - return query.update( - {failed: sent - delivered}, synchronize_session=False - ) + ).group_by(Notification.status, JobStatistics.job_id).order_by(JobStatistics.job_id).all() + + sort = sorted(results, key=lambda result: result.job_id) + groups = [] + for k, g in groupby(sort, key=lambda result: result.job_id): + groups.append(list(g)) + + for job in groups: + sent_count = 0 + delivered_count = 0 + failed_count = 0 + for notification_status in job: + if notification_status.status in [NOTIFICATION_DELIVERED, NOTIFICATION_SENT]: + delivered_count += notification_status.count + else: + failed_count += notification_status.count + sent_count += notification_status.count + + total_updated += JobStatistics.query.filter_by( + job_id=notification_status.job_id + ).update({ + sent: sent_count, + failed: failed_count, + delivered: delivered_count + }, synchronize_session=False) + return total_updated @statsd(namespace="dao") -def timeout_job_statistics(timeout_period): +def dao_timeout_job_statistics(timeout_period): timeout_start = datetime.utcnow() - timedelta(seconds=timeout_period) sms_count = timeout_job_counts(SMS_TYPE, timeout_start) email_count = timeout_job_counts(EMAIL_TYPE, timeout_start) diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index 7aa0b0fd8..128c595d3 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -7,7 +7,7 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app.dao.statistics_dao import ( create_or_update_job_sending_statistics, update_job_stats_outcome_count, - timeout_job_statistics) + dao_timeout_job_statistics) from app.models import ( JobStatistics, SMS_TYPE, @@ -17,8 +17,9 @@ from app.models import ( NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, - NOTIFICATION_PENDING, NOTIFICATION_CREATED, NOTIFICATION_FAILED, NOTIFICATION_SENT, NOTIFICATION_SENDING) -from tests.app.conftest import sample_notification, sample_email_template, sample_template + NOTIFICATION_PENDING, NOTIFICATION_CREATED, NOTIFICATION_FAILED, NOTIFICATION_SENT, NOTIFICATION_SENDING, + NOTIFICATION_STATUS_TYPES_COMPLETED, Notification, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_SUCCESS) +from tests.app.conftest import sample_notification, sample_email_template, sample_template, sample_job @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ @@ -36,8 +37,6 @@ def test_should_create_a_stats_entry_for_a_job( email_count, letter_count ): - assert not len(JobStatistics.query.all()) - template = None if notification_type == SMS_TYPE: @@ -88,8 +87,6 @@ def test_should_update_a_stats_entry_for_a_job( email_count, letter_count ): - assert not len(JobStatistics.query.all()) - template = None if notification_type == SMS_TYPE: @@ -160,8 +157,6 @@ def test_should_update_a_stats_entry_with_its_success_outcome_for_a_job( email_count, letter_count ): - assert not len(JobStatistics.query.all()) - template = None if notification_type == SMS_TYPE: @@ -227,8 +222,6 @@ def test_should_update_a_stats_entry_with_its_error_outcomes_for_a_job( letter_count, status ): - assert not len(JobStatistics.query.all()) - template = None if notification_type == SMS_TYPE: @@ -291,8 +284,6 @@ def test_should_update_a_stats_entry_with_its_success_outcomes_for_a_job( letter_count, status ): - assert not len(JobStatistics.query.all()) - template = None if notification_type == SMS_TYPE: @@ -361,8 +352,6 @@ def test_should_not_update_job_stats_if_irrelevant_status( letter_count, status ): - assert not len(JobStatistics.query.all()) - template = None if notification_type == SMS_TYPE: @@ -421,8 +410,6 @@ def test_inserting_one_type_of_notification_maintains_other_counts( email_count, letter_count ): - assert not len(JobStatistics.query.all()) - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) letter_template = sample_letter_template @@ -500,8 +487,6 @@ def test_updating_one_type_of_notification_to_success_maintains_other_counts( sample_job, sample_letter_template ): - assert not len(JobStatistics.query.all()) - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) letter_template = sample_letter_template @@ -560,8 +545,6 @@ def test_updating_one_type_of_notification_to_error_maintains_other_counts( sample_job, sample_letter_template ): - assert not len(JobStatistics.query.all()) - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) letter_template = sample_letter_template @@ -616,54 +599,6 @@ def test_updating_one_type_of_notification_to_error_maintains_other_counts( assert stats[0].emails_failed == 1 -def test_will_timeout_job_counts_after_notification_timeouts(notify_db, notify_db_session, sample_job): - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) - - one_minute_ago = datetime.utcnow() - timedelta(minutes=1) - - sms = sample_notification( - notify_db, - notify_db_session, - service=sample_job.service, - template=sms_template, - job=sample_job, - status=NOTIFICATION_CREATED - ) - - email = sample_notification( - notify_db, - notify_db_session, - service=sample_job.service, - template=email_template, - job=sample_job, - status=NOTIFICATION_CREATED - ) - - create_or_update_job_sending_statistics(email) - create_or_update_job_sending_statistics(sms) - - JobStatistics.query.update({JobStatistics.created_at: one_minute_ago}) - - intial_stats = JobStatistics.query.all() - - assert intial_stats[0].emails_sent == 1 - assert intial_stats[0].sms_sent == 1 - assert intial_stats[0].emails_delivered == 0 - assert intial_stats[0].sms_delivered == 0 - assert intial_stats[0].sms_failed == 0 - assert intial_stats[0].emails_failed == 0 - - timeout_job_statistics(59) - updated_stats = JobStatistics.query.all() - assert updated_stats[0].emails_sent == 1 - assert updated_stats[0].sms_sent == 1 - assert updated_stats[0].emails_delivered == 0 - assert updated_stats[0].sms_delivered == 0 - assert updated_stats[0].sms_failed == 1 - assert updated_stats[0].emails_failed == 1 - - def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, notify_db_session, sample_job): sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) @@ -702,7 +637,7 @@ def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, not assert intial_stats[0].sms_failed == 0 assert intial_stats[0].emails_failed == 0 - timeout_job_statistics(61) + dao_timeout_job_statistics(61) updated_stats = JobStatistics.query.all() assert updated_stats[0].emails_sent == 1 assert updated_stats[0].sms_sent == 1 @@ -710,3 +645,143 @@ def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, not assert updated_stats[0].sms_delivered == 0 assert updated_stats[0].sms_failed == 0 assert updated_stats[0].emails_failed == 0 + + +@pytest.mark.parametrize('notification_type, sms_count, email_count', [ + (SMS_TYPE, 3, 0), + (EMAIL_TYPE, 0, 3), +]) +def test_timeout_job_counts_timesout_multiple_jobs( + notify_db, notify_db_session, notification_type, sms_count, email_count +): + one_minute_ago = datetime.utcnow() - timedelta(minutes=1) + + job_1 = sample_job(notify_db, notify_db_session) + job_2 = sample_job(notify_db, notify_db_session) + job_3 = sample_job(notify_db, notify_db_session) + + jobs = [job_1, job_2, job_3] + + for job in jobs: + if notification_type == EMAIL_TYPE: + template = sample_email_template(notify_db, notify_db_session, service=job.service) + else: + template = sample_template(notify_db, notify_db_session, service=job.service) + + for i in range(3): + n = sample_notification( + notify_db, + notify_db_session, + service=job.service, + template=template, + job=job, + status=NOTIFICATION_CREATED + ) + create_or_update_job_sending_statistics(n) + + JobStatistics.query.update({JobStatistics.created_at: one_minute_ago}) + initial_stats = JobStatistics.query.all() + for stats in initial_stats: + assert stats.emails_sent == email_count + assert stats.sms_sent == sms_count + assert stats.emails_delivered == 0 + assert stats.sms_delivered == 0 + assert stats.sms_failed == 0 + assert stats.emails_failed == 0 + + dao_timeout_job_statistics(1) + updated_stats = JobStatistics.query.all() + for stats in updated_stats: + assert stats.emails_sent == email_count + assert stats.sms_sent == sms_count + assert stats.emails_delivered == 0 + assert stats.sms_delivered == 0 + assert stats.sms_failed == sms_count + assert stats.emails_failed == email_count + +count_notifications = len(NOTIFICATION_STATUS_TYPES) +count_success_notifications = len(NOTIFICATION_STATUS_SUCCESS) +count_error_notifications = len(NOTIFICATION_STATUS_TYPES) - len(NOTIFICATION_STATUS_SUCCESS) + + +def test_timeout_job_sets_all_non_delivered_emails_to_error( + notify_db, + notify_db_session, + sample_job +): + # Make a notification in every state + for i in range(len(NOTIFICATION_STATUS_TYPES)): + n = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=sample_email_template(notify_db, notify_db_session, service=sample_job.service), + job=sample_job, + status=NOTIFICATION_STATUS_TYPES[i] + ) + create_or_update_job_sending_statistics(n) + + # fudge the created at time on the job stats table to make the eligible for timeout query + JobStatistics.query.update({JobStatistics.created_at: datetime.utcnow() - timedelta(minutes=1)}) + + # should have sent an email for every state (len(NOTIFICATION_STATUS_TYPES)) + initial_stats = JobStatistics.query.all() + for stats in initial_stats: + assert stats.emails_sent == count_notifications + assert stats.sms_sent == 0 + assert stats.emails_delivered == 0 + assert stats.sms_delivered == 0 + assert stats.sms_failed == 0 + assert stats.emails_failed == 0 + + # timeout the notifications + dao_timeout_job_statistics(1) + + # after timeout all delivered states are success and ALL other states are failed + updated_stats = JobStatistics.query.all() + for stats in updated_stats: + assert stats.emails_sent == count_notifications + assert stats.sms_sent == 0 + assert stats.emails_delivered == count_success_notifications + assert stats.sms_delivered == 0 + assert stats.sms_failed == 0 + assert stats.emails_failed == count_error_notifications + + +# this test is as above, but for SMS not email +def test_timeout_job_sets_all_non_delivered_states_to_error( + notify_db, + notify_db_session, + sample_job +): + for i in range(len(NOTIFICATION_STATUS_TYPES)): + n = sample_notification( + notify_db, + notify_db_session, + service=sample_job.service, + template=sample_template(notify_db, notify_db_session, service=sample_job.service), + job=sample_job, + status=NOTIFICATION_STATUS_TYPES[i] + ) + create_or_update_job_sending_statistics(n) + + JobStatistics.query.update({JobStatistics.created_at: datetime.utcnow() - timedelta(minutes=1)}) + initial_stats = JobStatistics.query.all() + for stats in initial_stats: + assert stats.emails_sent == 0 + assert stats.sms_sent == count_notifications + assert stats.emails_delivered == 0 + assert stats.sms_delivered == 0 + assert stats.sms_failed == 0 + assert stats.emails_failed == 0 + + dao_timeout_job_statistics(1) + updated_stats = JobStatistics.query.all() + + for stats in updated_stats: + assert stats.emails_sent == 0 + assert stats.sms_sent == count_notifications + assert stats.emails_delivered == 0 + assert stats.sms_delivered == count_success_notifications + assert stats.sms_failed == count_error_notifications + assert stats.emails_failed == 0 From 63b7a4e9fea1d1f51bff47efe53892261963e865 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 12:19:44 +0100 Subject: [PATCH 35/68] New status grouping - success --- app/models.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 7eeba1e6f..ecc2670e0 100644 --- a/app/models.py +++ b/app/models.py @@ -602,6 +602,11 @@ NOTIFICATION_STATUS_TYPES_COMPLETED = [ NOTIFICATION_PERMANENT_FAILURE, ] +NOTIFICATION_STATUS_SUCCESS = [ + NOTIFICATION_SENT, + NOTIFICATION_DELIVERED +] + NOTIFICATION_STATUS_TYPES_BILLABLE = [ NOTIFICATION_SENDING, NOTIFICATION_SENT, @@ -623,6 +628,7 @@ NOTIFICATION_STATUS_TYPES = [ NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, ] + NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type') @@ -1060,5 +1066,11 @@ class JobStatistics(db.Model): the_string += "sms sent {} sms delivered {} sms failed {} ".format( self.sms_sent, self.sms_delivered, self.sms_failed ) - the_string += "letter sent {} letter failed {} ".format(self.letters_sent, self.letters_failed) + the_string += "letter sent {} letter failed {} ".format( + self.letters_sent, self.letters_failed + ) + the_string += "job_id {} ".format( + self.job_id + ) + the_string += "created at {}".format(self.created_at) return the_string From eb07fce3ee81ba5e77f8689de07924f7c37b0cca Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 12:19:56 +0100 Subject: [PATCH 36/68] Renamed method. --- app/celery/scheduled_tasks.py | 2 +- app/celery/statistics_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 3acdd5341..fbea3bdb0 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -16,7 +16,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, is_delivery_slow_for_provider ) -from app.dao.statistics_dao import timeout_job_statistics as dao_timeout_job_statistics +from app.dao.statistics_dao import dao_timeout_job_statistics from app.dao.provider_details_dao import ( get_current_provider, dao_toggle_sms_provider diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index d7f0fc8c3..a82a3791f 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -14,7 +14,7 @@ from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED def create_initial_notification_statistic_tasks(notification): - if notification.job_id and notification.status not in NOTIFICATION_STATUS_TYPES_COMPLETED: + if notification.job_id and notification.status: record_initial_job_statistics.apply_async((str(notification.id),), queue="statistics") From 1a61f06f5331821d49b10b62424bc6df479b7d1d Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 12:21:36 +0100 Subject: [PATCH 37/68] Removed test as no longer make distinction on status. --- tests/app/celery/test_statistics_tasks.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py index cdab47b5f..24aaba97d 100644 --- a/tests/app/celery/test_statistics_tasks.py +++ b/tests/app/celery/test_statistics_tasks.py @@ -32,16 +32,6 @@ def test_should_create_intial_job_task_if_notification_is_not_in_completed_state mock.assert_called_once_with((str(notification.id), ), queue="statistics") -@pytest.mark.parametrize('status', NOTIFICATION_STATUS_TYPES_COMPLETED) -def test_should_not_create_initial_job_task_if_notification_in_completed_state_already( - notify_db, notify_db_session, sample_job, mocker, status -): - mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") - notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) - create_initial_notification_statistic_tasks(notification) - mock.assert_not_called() - - def test_should_not_create_initial_job_task_if_notification_is_not_related_to_a_job( notify_db, notify_db_session, mocker ): From 4751673f4357fc3e9bf612499d7fd3a4ef8da2e7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 13:08:09 +0100 Subject: [PATCH 38/68] Fixed service test that deletes all the things to delete the job stats too. --- app/dao/services_dao.py | 10 ++++++++-- tests/app/dao/test_statistics_dao.py | 1 + 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c5679289c..736051174 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -28,7 +28,7 @@ from app.models import ( KEY_TYPE_TEST, NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES, -) + JobStatistics) from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc @@ -160,6 +160,10 @@ def delete_service_and_all_associated_db_objects(service): query.delete() db.session.commit() + job_stats = JobStatistics.query.join(Job).filter(Job.service_id == service.id) + list(map(db.session.delete, job_stats)) + db.session.commit() + _delete_commit(NotificationStatistics.query.filter_by(service=service)) _delete_commit(TemplateStatistics.query.filter_by(service=service)) _delete_commit(ProviderStatistics.query.filter_by(service=service)) @@ -167,12 +171,14 @@ def delete_service_and_all_associated_db_objects(service): _delete_commit(Permission.query.filter_by(service=service)) _delete_commit(ApiKey.query.filter_by(service=service)) _delete_commit(ApiKey.get_history_model().query.filter_by(service_id=service.id)) + _delete_commit(Job.query.filter_by(service=service)) _delete_commit(NotificationHistory.query.filter_by(service=service)) _delete_commit(Notification.query.filter_by(service=service)) - _delete_commit(Job.query.filter_by(service=service)) _delete_commit(Template.query.filter_by(service=service)) _delete_commit(TemplateHistory.query.filter_by(service_id=service.id)) + + verify_codes = VerifyCode.query.join(User).filter(User.id.in_([x.id for x in service.users])) list(map(db.session.delete, verify_codes)) db.session.commit() diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index 128c595d3..388a64b6f 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -699,6 +699,7 @@ def test_timeout_job_counts_timesout_multiple_jobs( assert stats.sms_failed == sms_count assert stats.emails_failed == email_count + count_notifications = len(NOTIFICATION_STATUS_TYPES) count_success_notifications = len(NOTIFICATION_STATUS_SUCCESS) count_error_notifications = len(NOTIFICATION_STATUS_TYPES) - len(NOTIFICATION_STATUS_SUCCESS) From 889c650724afb23086af085c53453c7c8b9ec919 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 13:09:59 +0100 Subject: [PATCH 39/68] Whitespace --- app/dao/services_dao.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 736051174..f0e49d598 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -177,8 +177,6 @@ def delete_service_and_all_associated_db_objects(service): _delete_commit(Template.query.filter_by(service=service)) _delete_commit(TemplateHistory.query.filter_by(service_id=service.id)) - - verify_codes = VerifyCode.query.join(User).filter(User.id.in_([x.id for x in service.users])) list(map(db.session.delete, verify_codes)) db.session.commit() From 835dcc62735bb06409838733b061ddad25e78f19 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 13:18:12 +0100 Subject: [PATCH 40/68] Not null job_id and rebuilt the migration script to include this --- app/models.py | 2 +- migrations/versions/0083_add_job_stats.py | 7 +++--- .../versions/0084_add_created_to_job_stats.py | 22 ------------------- 3 files changed, 5 insertions(+), 26 deletions(-) delete mode 100644 migrations/versions/0084_add_created_to_job_stats.py diff --git a/app/models.py b/app/models.py index ecc2670e0..d8b364489 100644 --- a/app/models.py +++ b/app/models.py @@ -1035,7 +1035,7 @@ class JobStatistics(db.Model): __tablename__ = 'job_statistics' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=True) + job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=True, nullable=False) job = db.relationship('Job', backref=db.backref('job_statistics', lazy='dynamic')) emails_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) emails_delivered = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) diff --git a/migrations/versions/0083_add_job_stats.py b/migrations/versions/0083_add_job_stats.py index cf028ded6..808b75c29 100644 --- a/migrations/versions/0083_add_job_stats.py +++ b/migrations/versions/0083_add_job_stats.py @@ -1,8 +1,8 @@ """empty message Revision ID: 0083_add_job_stats -Revises: 0080_fix_rate_start_date -Create Date: 2017-05-09 12:44:43.173269 +Revises: 0082_add_go_live_template +Create Date: 2017-05-12 13:16:14.147368 """ @@ -17,7 +17,7 @@ from sqlalchemy.dialects import postgresql def upgrade(): op.create_table('job_statistics', sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('job_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('job_id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('emails_sent', sa.BigInteger(), nullable=False), sa.Column('emails_delivered', sa.BigInteger(), nullable=False), sa.Column('emails_failed', sa.BigInteger(), nullable=False), @@ -26,6 +26,7 @@ def upgrade(): sa.Column('sms_failed', sa.BigInteger(), nullable=False), sa.Column('letters_sent', sa.BigInteger(), nullable=False), sa.Column('letters_failed', sa.BigInteger(), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=True), sa.Column('updated_at', sa.DateTime(), nullable=True), sa.ForeignKeyConstraint(['job_id'], ['jobs.id'], ), sa.PrimaryKeyConstraint('id') diff --git a/migrations/versions/0084_add_created_to_job_stats.py b/migrations/versions/0084_add_created_to_job_stats.py deleted file mode 100644 index c7470bef1..000000000 --- a/migrations/versions/0084_add_created_to_job_stats.py +++ /dev/null @@ -1,22 +0,0 @@ -"""empty message - -Revision ID: 0084_add_created_to_job_stats -Revises: 0081_add_job_stats -Create Date: 2017-05-11 15:05:53.420946 - -""" - -# revision identifiers, used by Alembic. -revision = '0084_add_created_to_job_stats' -down_revision = '0083_add_job_stats' - -from alembic import op -import sqlalchemy as sa - - -def upgrade(): - op.add_column('job_statistics', sa.Column('created_at', sa.DateTime(), nullable=True)) - - -def downgrade(): - op.drop_column('job_statistics', 'created_at') From 8ac821fcc46020a51fd45949f026de4a8d6f466e Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 16:10:00 +0100 Subject: [PATCH 41/68] Fixed import paths --- app/notifications/validators.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index e601ec6be..a680f446e 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -10,11 +10,12 @@ from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM, SMS_TYPE from app.service.utils import service_allowed_to_send_to from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError from app import redis_store +from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_cache_key def check_service_over_api_rate_limit(service, api_key): if current_app.config['API_RATE_LIMIT_ENABLED']: - cache_key = redis_store.rate_limit_cache_key(service.id, api_key.key_type) + cache_key = rate_limit_cache_key(service.id, api_key.key_type) rate_limit = current_app.config['API_KEY_LIMITS'][api_key.key_type]['limit'] interval = current_app.config['API_KEY_LIMITS'][api_key.key_type]['interval'] if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval): @@ -24,7 +25,7 @@ def check_service_over_api_rate_limit(service, api_key): def check_service_over_daily_message_limit(key_type, service): if key_type != KEY_TYPE_TEST: - cache_key = redis_store.daily_limit_cache_key(service.id) + cache_key = daily_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) if not service_stats: service_stats = services_dao.fetch_todays_total_message_count(service.id) From f4020aec05f65f9004c2f8ccabedc60ac07c190b Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 16:10:21 +0100 Subject: [PATCH 42/68] these three tests replicate some testing that is done in the client tests themselves. --- .../test_process_notification.py | 19 ---------- tests/app/notifications/test_validators.py | 36 ------------------- 2 files changed, 55 deletions(-) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index f697047e7..be2356c69 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -97,25 +97,6 @@ def test_persist_notification_throws_exception_when_missing_template(sample_api_ assert NotificationHistory.query.count() == 0 -def test_exception_thown_by_redis_store_get_should_not_be_fatal(sample_template, sample_api_key, mocker): - mocker.patch( - 'app.notifications.process_notifications.redis_store.redis_store.incr', - side_effect=Exception("broken redis")) - - notification = persist_notification( - sample_template.id, - sample_template.version, - '+447111111111', - sample_template.service, - {}, - 'sms', - sample_api_key.id, - sample_api_key.key_type) - assert Notification.query.count() == 1 - assert Notification.query.get(notification.id) is not None - assert NotificationHistory.query.count() == 1 - - def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_key, mocker): mocked_redis = mocker.patch('app.redis_store.get') mock_service_template_cache = mocker.patch('app.redis_store.get_all_from_hash') diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index ea964fc43..9758b4c3d 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,42 +23,6 @@ from tests.app.conftest import ( sample_api_key) -@pytest.mark.parametrize('key_type', ['team', 'normal']) -def test_exception_thrown_by_redis_store_get_should_not_be_fatal( - notify_db, - notify_db_session, - notify_api, - key_type, - mocker): - with freeze_time("2016-01-01 12:00:00.000000"): - - mocker.patch('app.notifications.validators.redis_store.redis_store.get', side_effect=Exception("broken redis")) - mocker.patch('app.notifications.validators.redis_store.redis_store.set') - - service = create_service(notify_db, notify_db_session, restricted=True, limit=4) - for x in range(5): - create_notification(notify_db, notify_db_session, service=service) - - with pytest.raises(TooManyRequestsError) as e: - check_service_over_daily_message_limit(key_type, service) - assert e.value.status_code == 429 - assert e.value.message == 'Exceeded send limits (4) for today' - assert e.value.fields == [] - app.notifications.validators.redis_store.redis_store.set.assert_called_with( - "{}-2016-01-01-count".format(str(service.id)), 5, 3600, None, False, False - ) - - -@pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) -def test_exception_thown_by_redis_store_set_should_not_be_fatal( - key_type, - sample_service, - mocker): - mocker.patch('app.notifications.validators.redis_store.redis_store.set', side_effect=Exception("broken redis")) - mocker.patch('app.notifications.validators.redis_store.get', return_value=None) - assert not check_service_over_daily_message_limit(key_type, sample_service) - - @pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allowed( key_type, From 8936b63dccc6f650f7bf193e1eca25520a4a4fb1 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 12 May 2017 16:11:01 +0100 Subject: [PATCH 43/68] Removed redis from API requirements - it's pulled in by the utils. --- requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 3bc64fbc2..3f688a907 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,7 +17,6 @@ celery==3.1.23 monotonic==1.2 statsd==3.2.1 jsonschema==2.5.1 -Flask-Redis==0.1.0 gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 From 114d4d84d497c1b057b875f019efc831ab7aba6c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 11 May 2017 15:22:58 +0100 Subject: [PATCH 44/68] Add service permissions DAO and refactor user service permission mock --- app/dao/service_permissions_dao.py | 41 ++++++++++ migrations/README | 8 +- .../0083_add_perm_types_and_svc_perm.py | 8 +- tests/app/conftest.py | 10 +-- tests/app/dao/test_service_permissions_dao.py | 82 +++++++++++++++++++ tests/app/db.py | 14 +++- tests/app/service/test_rest.py | 16 ++-- tests/app/user/test_rest.py | 6 +- tests/conftest.py | 3 +- 9 files changed, 165 insertions(+), 23 deletions(-) create mode 100644 app/dao/service_permissions_dao.py create mode 100644 tests/app/dao/test_service_permissions_dao.py diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py new file mode 100644 index 000000000..7ea9eb0d3 --- /dev/null +++ b/app/dao/service_permissions_dao.py @@ -0,0 +1,41 @@ +from sqlalchemy import exc + +from app import db +from app.dao.dao_utils import transactional +from app.models import Service, ServicePermission, SERVICE_PERMISSION_TYPES + + +def dao_fetch_service_permissions(service_id): + return ServicePermission.query.filter( + ServicePermission.service_id == service_id).all() + + +def make_service_permissions_list(service_id, permissions): + arr = [] + for permission in permissions: + if permission not in SERVICE_PERMISSION_TYPES: + raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) + + service_permission = ServicePermission(service_id=service_id, permission=permission) + arr.append(service_permission) + + return arr + + +@transactional +def dao_add_and_commit_service_permissions(service_id, permissions): + service_permissions = make_service_permissions_list(service_id, permissions) + + try: + db.session.add_all(service_permissions) + db.session.commit() + except exc.IntegrityError as e: + if "duplicate key value violates unique constraint" in str(e.orig): + raise ValueError(e.orig) + raise + + +def dao_remove_service_permission(service_id, permission=None): + return ServicePermission.query.filter( + ServicePermission.service_id == service_id, + ServicePermission.permission == permission if permission else None).delete() diff --git a/migrations/README b/migrations/README index 98e4f9c44..6c36a3e0e 100644 --- a/migrations/README +++ b/migrations/README @@ -1 +1,7 @@ -Generic single-database configuration. \ No newline at end of file +Generic single-database configuration. + +python application.py db migration to generate migration script. + +python application.py db upgrade to upgrade db with script. + +python application.py db downgrade to rollback db changes. diff --git a/migrations/versions/0083_add_perm_types_and_svc_perm.py b/migrations/versions/0083_add_perm_types_and_svc_perm.py index 485ddef5d..2bebb273e 100644 --- a/migrations/versions/0083_add_perm_types_and_svc_perm.py +++ b/migrations/versions/0083_add_perm_types_and_svc_perm.py @@ -15,10 +15,10 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - service_permission_types = op.create_table('service_permission_types', - sa.Column('name', sa.String(length=255), nullable=False), - sa.PrimaryKeyConstraint('name')) + ### commands auto generated by Alembic - please adjust! ### + service_permission_types=op.create_table('service_permission_types', + sa.Column('name', sa.String(length=255), nullable=False), + sa.PrimaryKeyConstraint('name')) op.bulk_insert(service_permission_types, [ diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fc9748870..00b4604b1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -666,11 +666,11 @@ def sample_permission(notify_db, @pytest.fixture(scope='function') -def sample_service_permission(notify_db, - notify_db_session, - service=None, - user=None, - permission="manage_settings"): +def sample_user_service_permission(notify_db, + notify_db_session, + service=None, + user=None, + permission="manage_settings"): if user is None: user = create_user() if service is None: diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py new file mode 100644 index 000000000..b6130f0da --- /dev/null +++ b/tests/app/dao/test_service_permissions_dao.py @@ -0,0 +1,82 @@ +import pytest + +from app.dao.service_permissions_dao import ( + dao_fetch_service_permissions, dao_remove_service_permission) +from app.models import ( + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) + +from tests.app.db import create_service_permissions, create_service + + +def test_create_service_permissions(sample_service): + service_permission_types = [SMS_TYPE, INTERNATIONAL_SMS_TYPE] + + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + assert len(service_permissions) == len(service_permission_types) + assert all(sp.service_id == sample_service.id for sp in service_permissions) + assert all(sp.permission in service_permission_types for sp in service_permissions) + + +def test_fetch_service_permissions_gets_service_permissions(sample_service): + service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] + create_service_permissions(service_id=sample_service.id, permissions=service_permission_types) + service_permissions = dao_fetch_service_permissions(sample_service.id) + + assert len(service_permission_types) == len(service_permission_types) + assert all(sp.service_id == sample_service.id for sp in service_permissions) + assert all(sp.permission in service_permission_types for sp in service_permissions) + + +def test_add_service_permissions_to_existing_permissions(sample_service): + service_permission_types_1 = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_2 = [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] + + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_1) + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_2) + + permissions = dao_fetch_service_permissions(sample_service.id) + + assert len(permissions) == len(service_permission_types_1 + service_permission_types_2) + + +def test_create_invalid_service_permissions_raises_error(sample_service): + service_permission_types = ['invalid'] + + with pytest.raises(ValueError) as e: + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + assert "'invalid' not of service permission type: " + str(SERVICE_PERMISSION_TYPES) in str(e.value) + + +def test_remove_service_permission(sample_service): + service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_type_to_remove = EMAIL_TYPE + service_permission_type_remaining = INCOMING_SMS_TYPE + + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) + + permissions = dao_fetch_service_permissions(sample_service.id) + assert len(permissions) == 1 + assert permissions[0].permission == service_permission_type_remaining + assert permissions[0].service_id == sample_service.id + + +def test_adding_duplicate_service_id_permission_raises_value_error(sample_service): + service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_with_duplicate_email_type = [LETTER_TYPE, EMAIL_TYPE] + + with pytest.raises(ValueError) as e: + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_with_duplicate_email_type) + + assert "duplicate key value violates unique constraint \"service_permissions_pkey\"" in str(e.value) diff --git a/tests/app/db.py b/tests/app/db.py index b5838d693..c193fd4a9 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -2,11 +2,14 @@ from datetime import datetime import uuid from app.dao.jobs_dao import dao_create_job -from app.models import Service, User, Template, Notification, SMS_TYPE, KEY_TYPE_NORMAL, Job +from app.models import (Service, User, Template, Notification, EMAIL_TYPE, LETTER_TYPE, + SMS_TYPE, KEY_TYPE_NORMAL, Job, ServicePermission) from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service +from app.dao.service_permissions_dao import ( + dao_add_and_commit_service_permissions, make_service_permissions_list) def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -141,3 +144,12 @@ def create_job(template, job = Job(**data) dao_create_job(job) return job + + +def create_service_permissions(service_id, permissions=[EMAIL_TYPE, LETTER_TYPE]): + dao_add_and_commit_service_permissions( + service_id if service_id else create_service().id, permissions) + + service_permissions = ServicePermission.query.all() + + return service_permissions diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 91c928751..2f4732473 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -15,7 +15,7 @@ from tests import create_authorization_header from tests.app.db import create_template from tests.app.conftest import ( sample_service as create_service, - sample_service_permission as create_service_permission, + sample_user_service_permission as create_user_service_permission, sample_notification as create_sample_notification, sample_notification_history as create_notification_history, sample_notification_with_job @@ -941,12 +941,12 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db assert result['message'] == expected_message -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: second_user = create_user(email="new@digital.cabinet-office.gov.uk") # Simulates successfully adding a user to the service - second_permission = create_service_permission( + second_permission = create_user_service_permission( notify_db, notify_db_session, user=second_user) @@ -961,13 +961,13 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp assert resp.status_code == 204 -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: second_user = create_user(email="new@digital.cabinet-office.gov.uk") endpoint = url_for( 'service.remove_user_from_service', - service_id=str(sample_service_permission.service.id), + service_id=str(sample_user_service_permission.service.id), user_id=str(second_user.id)) auth_header = create_authorization_header() resp = client.delete( @@ -979,13 +979,13 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp def test_cannot_remove_only_user_from_service(notify_api, notify_db, notify_db_session, - sample_service_permission): + sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: endpoint = url_for( 'service.remove_user_from_service', - service_id=str(sample_service_permission.service.id), - user_id=str(sample_service_permission.user.id)) + service_id=str(sample_user_service_permission.service.id), + user_id=str(sample_user_service_permission.user.id)) auth_header = create_authorization_header() resp = client.delete( endpoint, diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index e476b2ebb..ddbb3eaae 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -290,13 +290,13 @@ def test_get_user_by_email_bad_url_returns_404(client, sample_user): assert json_resp['message'] == 'Invalid request. Email query string param required' -def test_get_user_with_permissions(client, sample_service_permission): +def test_get_user_with_permissions(client, sample_user_service_permission): header = create_authorization_header() - response = client.get(url_for('user.get_user', user_id=str(sample_service_permission.user.id)), + response = client.get(url_for('user.get_user', user_id=str(sample_user_service_permission.user.id)), headers=[header]) assert response.status_code == 200 permissions = json.loads(response.get_data(as_text=True))['data']['permissions'] - assert sample_service_permission.permission in permissions[str(sample_service_permission.service.id)] + assert sample_user_service_permission.permission in permissions[str(sample_user_service_permission.service.id)] def test_set_user_permissions(client, sample_user, sample_service): diff --git a/tests/conftest.py b/tests/conftest.py index 61cd17ce9..bf9823331 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,7 +77,8 @@ def notify_db_session(notify_db): "provider_details_history", "template_process_type", "dvla_organisation", - "notification_status_types"]: + "notification_status_types", + "service_permission_types"]: notify_db.engine.execute(tbl.delete()) notify_db.session.commit() From e6db9ffc1b2057a4c21f034dce2284ed8cb7cb04 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 16 May 2017 10:29:27 +0100 Subject: [PATCH 45/68] Force parse JSON received from SNS: * An SNS callback containing JSON has a plaintext header set. Using * request.get_json() will return None if the header is not * application/json unless the force parameter is set to True --- app/notifications/notifications_letter_callback.py | 5 +++-- tests/app/notifications/rest/test_callbacks.py | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 2f758b7ac..1de6c0009 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -35,7 +35,7 @@ def validate_schema(schema): def decorator(f): @wraps(f) def wrapper(*args, **kw): - validate(request.json, schema) + validate(request.get_json(force=True), schema) return f(*args, **kw) return wrapper return decorator @@ -44,7 +44,8 @@ def validate_schema(schema): @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST']) @validate_schema(dvla_sns_callback_schema) def process_letter_response(): - req_json = request.json + req_json = request.get_json(force=True) + current_app.logger.info('Received SNS callback: {}'.format(req_json)) if not autoconfirm_subscription(req_json): # The callback should have one record for an S3 Put Event. filename = req_json['Message']['Records'][0]['s3']['object']['key'] diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index a15a607df..5c8f96baa 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -71,6 +71,18 @@ def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): update_task.assert_called_with(['bar.txt'], queue='notify') +def test_dvla_callback_does_not_raise_error_parsing_json_for_plaintext_header(client, mocker): + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + data = _sample_sns_s3_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'text/plain')] + ) + + assert response.status_code == 200 + + def test_firetext_callback_should_not_need_auth(client, mocker): mocker.patch('app.statsd_client.incr') response = client.post( From 54d801979caf0bb0c25711b2b29e4df3e586b416 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 10:57:57 +0100 Subject: [PATCH 46/68] Refactored to handle single service permission --- app/dao/service_permissions_dao.py | 27 ++----- tests/app/dao/test_service_permissions_dao.py | 71 ++++++------------- tests/app/db.py | 9 ++- 3 files changed, 32 insertions(+), 75 deletions(-) diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 7ea9eb0d3..26b1f6f9e 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -10,29 +10,14 @@ def dao_fetch_service_permissions(service_id): ServicePermission.service_id == service_id).all() -def make_service_permissions_list(service_id, permissions): - arr = [] - for permission in permissions: - if permission not in SERVICE_PERMISSION_TYPES: - raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - - service_permission = ServicePermission(service_id=service_id, permission=permission) - arr.append(service_permission) - - return arr - - @transactional -def dao_add_and_commit_service_permissions(service_id, permissions): - service_permissions = make_service_permissions_list(service_id, permissions) +def dao_add_and_commit_service_permission(service_id, permission): + if permission not in SERVICE_PERMISSION_TYPES: + raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - try: - db.session.add_all(service_permissions) - db.session.commit() - except exc.IntegrityError as e: - if "duplicate key value violates unique constraint" in str(e.orig): - raise ValueError(e.orig) - raise + service_permission = ServicePermission(service_id=service_id, permission=permission) + + db.session.add(service_permission) def dao_remove_service_permission(service_id, permission=None): diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index b6130f0da..cf3768306 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -5,61 +5,47 @@ from app.dao.service_permissions_dao import ( from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) -from tests.app.db import create_service_permissions, create_service +from tests.app.db import create_service_permission, create_service -def test_create_service_permissions(sample_service): - service_permission_types = [SMS_TYPE, INTERNATIONAL_SMS_TYPE] +def test_create_service_permission(sample_service): + service_permission_type = SMS_TYPE - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + service_permission = create_service_permission( + service_id=sample_service.id, permission=service_permission_type) + + assert len(service_permission) == 1 + assert all(sp.service_id == sample_service.id for sp in service_permission) + assert all(sp.permission in service_permission_type for sp in service_permission) + + +def test_fetch_service_permissions_gets_service_permissions(sample_service): + service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] + for spt in service_permission_types: + create_service_permission(service_id=sample_service.id, permission=spt) + service_permissions = dao_fetch_service_permissions(sample_service.id) assert len(service_permissions) == len(service_permission_types) assert all(sp.service_id == sample_service.id for sp in service_permissions) assert all(sp.permission in service_permission_types for sp in service_permissions) -def test_fetch_service_permissions_gets_service_permissions(sample_service): - service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] - create_service_permissions(service_id=sample_service.id, permissions=service_permission_types) - service_permissions = dao_fetch_service_permissions(sample_service.id) - - assert len(service_permission_types) == len(service_permission_types) - assert all(sp.service_id == sample_service.id for sp in service_permissions) - assert all(sp.permission in service_permission_types for sp in service_permissions) - - -def test_add_service_permissions_to_existing_permissions(sample_service): - service_permission_types_1 = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_types_2 = [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] - - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_1) - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_2) - - permissions = dao_fetch_service_permissions(sample_service.id) - - assert len(permissions) == len(service_permission_types_1 + service_permission_types_2) - - def test_create_invalid_service_permissions_raises_error(sample_service): - service_permission_types = ['invalid'] + service_permission_type = 'invalid' with pytest.raises(ValueError) as e: - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + create_service_permission(service_id=sample_service.id, permission=service_permission_type) - assert "'invalid' not of service permission type: " + str(SERVICE_PERMISSION_TYPES) in str(e.value) + assert "'invalid' not of service permission type: {}".format(str(SERVICE_PERMISSION_TYPES)) in str(e.value) def test_remove_service_permission(sample_service): - service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_to_create = [EMAIL_TYPE, INCOMING_SMS_TYPE] service_permission_type_to_remove = EMAIL_TYPE service_permission_type_remaining = INCOMING_SMS_TYPE - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + for spt in service_permission_types_to_create: + create_service_permission(service_id=sample_service.id, permission=spt) dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) @@ -67,16 +53,3 @@ def test_remove_service_permission(sample_service): assert len(permissions) == 1 assert permissions[0].permission == service_permission_type_remaining assert permissions[0].service_id == sample_service.id - - -def test_adding_duplicate_service_id_permission_raises_value_error(sample_service): - service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_types_with_duplicate_email_type = [LETTER_TYPE, EMAIL_TYPE] - - with pytest.raises(ValueError) as e: - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_with_duplicate_email_type) - - assert "duplicate key value violates unique constraint \"service_permissions_pkey\"" in str(e.value) diff --git a/tests/app/db.py b/tests/app/db.py index c193fd4a9..e4cded3cc 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -8,8 +8,7 @@ from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -from app.dao.service_permissions_dao import ( - dao_add_and_commit_service_permissions, make_service_permissions_list) +from app.dao.service_permissions_dao import dao_add_and_commit_service_permission def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -146,9 +145,9 @@ def create_job(template, return job -def create_service_permissions(service_id, permissions=[EMAIL_TYPE, LETTER_TYPE]): - dao_add_and_commit_service_permissions( - service_id if service_id else create_service().id, permissions) +def create_service_permission(service_id, permission=EMAIL_TYPE): + dao_add_and_commit_service_permission( + service_id if service_id else create_service().id, permission) service_permissions = ServicePermission.query.all() From d1aff5bb6e053355a4eec0617771eff2e18beaac Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 16 May 2017 11:36:52 +0100 Subject: [PATCH 47/68] Complex test to check updating one type of notification job, doesn't update the other - so if we timeout both SMS and Email the counts are correct --- tests/app/dao/test_statistics_dao.py | 84 +++++++++++++++++++--------- 1 file changed, 59 insertions(+), 25 deletions(-) diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index 388a64b6f..23e15f65f 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -19,7 +19,7 @@ from app.models import ( NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_PENDING, NOTIFICATION_CREATED, NOTIFICATION_FAILED, NOTIFICATION_SENT, NOTIFICATION_SENDING, NOTIFICATION_STATUS_TYPES_COMPLETED, Notification, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_SUCCESS) -from tests.app.conftest import sample_notification, sample_email_template, sample_template, sample_job +from tests.app.conftest import sample_notification, sample_email_template, sample_template, sample_job, sample_service @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ @@ -705,48 +705,82 @@ count_success_notifications = len(NOTIFICATION_STATUS_SUCCESS) count_error_notifications = len(NOTIFICATION_STATUS_TYPES) - len(NOTIFICATION_STATUS_SUCCESS) -def test_timeout_job_sets_all_non_delivered_emails_to_error( +def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sms( notify_db, - notify_db_session, - sample_job + notify_db_session ): - # Make a notification in every state + service = sample_service(notify_db, notify_db_session) + + sms_template = sample_template(notify_db, notify_db_session, service=service) + email_template = sample_email_template(notify_db, notify_db_session, service=service) + + email_job = sample_job( + notify_db, notify_db_session, template=email_template, service=service + ) + sms_job = sample_job( + notify_db, notify_db_session, template=sms_template, service=service + ) + + # Make an email notification in every state for i in range(len(NOTIFICATION_STATUS_TYPES)): n = sample_notification( notify_db, notify_db_session, - service=sample_job.service, - template=sample_email_template(notify_db, notify_db_session, service=sample_job.service), - job=sample_job, + service=email_job.service, + template=email_template, + job=email_job, status=NOTIFICATION_STATUS_TYPES[i] ) create_or_update_job_sending_statistics(n) + # single sms notification + sms_notification = sample_notification( + notify_db, notify_db_session, service=service, template=sms_template, job=sms_job + ) + create_or_update_job_sending_statistics(sms_notification) + # fudge the created at time on the job stats table to make the eligible for timeout query - JobStatistics.query.update({JobStatistics.created_at: datetime.utcnow() - timedelta(minutes=1)}) + JobStatistics.query.update({ + JobStatistics.created_at: datetime.utcnow() - timedelta(minutes=1) + }) # should have sent an email for every state (len(NOTIFICATION_STATUS_TYPES)) - initial_stats = JobStatistics.query.all() - for stats in initial_stats: - assert stats.emails_sent == count_notifications - assert stats.sms_sent == 0 - assert stats.emails_delivered == 0 - assert stats.sms_delivered == 0 - assert stats.sms_failed == 0 - assert stats.emails_failed == 0 + initial_stats = JobStatistics.query.filter_by(job_id=email_job.id).all() + assert len(initial_stats) == 1 + assert initial_stats[0].emails_sent == count_notifications + assert initial_stats[0].sms_sent == 0 + assert initial_stats[0].emails_delivered == 0 + assert initial_stats[0].sms_delivered == 0 + assert initial_stats[0].sms_failed == 0 + assert initial_stats[0].emails_failed == 0 + + all = JobStatistics.query.all() + for a in all: + print(a) # timeout the notifications dao_timeout_job_statistics(1) + all = JobStatistics.query.all() + for a in all: + print(a) + # after timeout all delivered states are success and ALL other states are failed - updated_stats = JobStatistics.query.all() - for stats in updated_stats: - assert stats.emails_sent == count_notifications - assert stats.sms_sent == 0 - assert stats.emails_delivered == count_success_notifications - assert stats.sms_delivered == 0 - assert stats.sms_failed == 0 - assert stats.emails_failed == count_error_notifications + updated_stats = JobStatistics.query.filter_by(job_id=email_job.id).all() + assert updated_stats[0].emails_sent == count_notifications + assert updated_stats[0].sms_sent == 0 + assert updated_stats[0].emails_delivered == count_success_notifications + assert updated_stats[0].sms_delivered == 0 + assert updated_stats[0].sms_failed == 0 + assert updated_stats[0].emails_failed == count_error_notifications + + sms_stats = JobStatistics.query.filter_by(job_id=sms_job.id).all() + assert sms_stats[0].emails_sent == 0 + assert sms_stats[0].sms_sent == 1 + assert sms_stats[0].emails_delivered == 0 + assert sms_stats[0].sms_delivered == 0 + assert sms_stats[0].sms_failed == 1 + assert sms_stats[0].emails_failed == 100 # this test is as above, but for SMS not email From 2d4f10bd221cb4899bdd4fb8a518ef5ae4869c39 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 16 May 2017 12:33:01 +0100 Subject: [PATCH 48/68] Comma-format number when emailing live services 250,000 is easier to read than 250000. --- app/service/rest.py | 2 +- tests/app/service/test_rest.py | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index db8c1aefa..180740063 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -132,7 +132,7 @@ def update_service(service_id): template_id=current_app.config['SERVICE_NOW_LIVE_TEMPLATE_ID'], personalisation={ 'service_name': current_data['name'], - 'message_limit': current_data['message_limit'] + 'message_limit': '{:,}'.format(current_data['message_limit']) }, include_user_fields=['name'] ) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 1681444b7..ccaafb456 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1682,7 +1682,15 @@ def test_update_service_calls_send_notification_as_service_becomes_live(notify_d ) assert resp.status_code == 200 - assert send_notification_mock.called + send_notification_mock.assert_called_once_with( + service_id=restricted_service.id, + template_id='618185c6-3636-49cd-b7d2-6f6f5eb3bdde', + personalisation={ + 'service_name': restricted_service.name, + 'message_limit': '1,000' + }, + include_user_fields=['name'] + ) def test_update_service_does_not_call_send_notification_for_live_service(sample_service, client, mocker): From 733c16b2bbf20247947565c11594e685326da1f5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:33:27 +0100 Subject: [PATCH 49/68] Update to strip down DAO and clarify tests --- app/dao/service_permissions_dao.py | 13 ++---- tests/app/conftest.py | 8 ++-- tests/app/dao/test_service_permissions_dao.py | 42 +++++++------------ tests/app/db.py | 4 +- 4 files changed, 23 insertions(+), 44 deletions(-) diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 26b1f6f9e..5c38ad6a2 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -1,8 +1,6 @@ -from sqlalchemy import exc - from app import db from app.dao.dao_utils import transactional -from app.models import Service, ServicePermission, SERVICE_PERMISSION_TYPES +from app.models import ServicePermission, SERVICE_PERMISSION_TYPES def dao_fetch_service_permissions(service_id): @@ -11,16 +9,13 @@ def dao_fetch_service_permissions(service_id): @transactional -def dao_add_and_commit_service_permission(service_id, permission): - if permission not in SERVICE_PERMISSION_TYPES: - raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - +def dao_create_service_permission(service_id, permission): service_permission = ServicePermission(service_id=service_id, permission=permission) db.session.add(service_permission) -def dao_remove_service_permission(service_id, permission=None): +def dao_remove_service_permission(service_id, permission): return ServicePermission.query.filter( ServicePermission.service_id == service_id, - ServicePermission.permission == permission if permission else None).delete() + ServicePermission.permission == permission).delete() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 00b4604b1..c10c053cb 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -666,11 +666,9 @@ def sample_permission(notify_db, @pytest.fixture(scope='function') -def sample_user_service_permission(notify_db, - notify_db_session, - service=None, - user=None, - permission="manage_settings"): +def sample_user_service_permission( + notify_db, notify_db_session, service=None, user=None, permission="manage_settings" +): if user is None: user = create_user() if service is None: diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index cf3768306..248dd6ae6 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -5,51 +5,37 @@ from app.dao.service_permissions_dao import ( from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) -from tests.app.db import create_service_permission, create_service +from tests.app.db import create_service_permission def test_create_service_permission(sample_service): - service_permission_type = SMS_TYPE - service_permission = create_service_permission( - service_id=sample_service.id, permission=service_permission_type) + service_id=sample_service.id, permission=SMS_TYPE) assert len(service_permission) == 1 - assert all(sp.service_id == sample_service.id for sp in service_permission) - assert all(sp.permission in service_permission_type for sp in service_permission) + assert service_permission[0].service_id == sample_service.id + assert service_permission[0].permission == SMS_TYPE def test_fetch_service_permissions_gets_service_permissions(sample_service): - service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] - for spt in service_permission_types: - create_service_permission(service_id=sample_service.id, permission=spt) + create_service_permission(service_id=sample_service.id, permission=LETTER_TYPE) + create_service_permission(service_id=sample_service.id, permission=INTERNATIONAL_SMS_TYPE) + create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) + service_permissions = dao_fetch_service_permissions(sample_service.id) - assert len(service_permissions) == len(service_permission_types) + assert len(service_permissions) == 3 assert all(sp.service_id == sample_service.id for sp in service_permissions) - assert all(sp.permission in service_permission_types for sp in service_permissions) - - -def test_create_invalid_service_permissions_raises_error(sample_service): - service_permission_type = 'invalid' - - with pytest.raises(ValueError) as e: - create_service_permission(service_id=sample_service.id, permission=service_permission_type) - - assert "'invalid' not of service permission type: {}".format(str(SERVICE_PERMISSION_TYPES)) in str(e.value) + assert all(sp.permission in [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] for sp in service_permissions) def test_remove_service_permission(sample_service): - service_permission_types_to_create = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_type_to_remove = EMAIL_TYPE - service_permission_type_remaining = INCOMING_SMS_TYPE + create_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE) + create_service_permission(service_id=sample_service.id, permission=INCOMING_SMS_TYPE) - for spt in service_permission_types_to_create: - create_service_permission(service_id=sample_service.id, permission=spt) - - dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) + dao_remove_service_permission(sample_service.id, EMAIL_TYPE) permissions = dao_fetch_service_permissions(sample_service.id) assert len(permissions) == 1 - assert permissions[0].permission == service_permission_type_remaining + assert permissions[0].permission == INCOMING_SMS_TYPE assert permissions[0].service_id == sample_service.id diff --git a/tests/app/db.py b/tests/app/db.py index e4cded3cc..228d49ec4 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -8,7 +8,7 @@ from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -from app.dao.service_permissions_dao import dao_add_and_commit_service_permission +from app.dao.service_permissions_dao import dao_create_service_permission def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -146,7 +146,7 @@ def create_job(template, def create_service_permission(service_id, permission=EMAIL_TYPE): - dao_add_and_commit_service_permission( + dao_create_service_permission( service_id if service_id else create_service().id, permission) service_permissions = ServicePermission.query.all() From a5dae0bebd1e901b19fcf20e0d35556e21b37862 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 16 May 2017 12:49:20 +0100 Subject: [PATCH 50/68] Fixed test - I had deliberately failed the test as part of debugging and not fixed the assert. Doh. --- app/dao/statistics_dao.py | 4 +++- tests/app/dao/test_statistics_dao.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index 20c75112e..baff82ba4 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -14,6 +14,7 @@ from app.models import ( SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_FAILED, + NOTIFICATION_STATUS_SUCCESS, NOTIFICATION_DELIVERED, NOTIFICATION_SENT) from app.statsd_decorators import statsd @@ -32,6 +33,7 @@ def timeout_job_counts(notifications_type, timeout_start): func.count(Notification.status).label('count'), Notification.status.label('status') ).filter( + Notification.notification_type == notifications_type, JobStatistics.job_id == Notification.job_id, JobStatistics.created_at < timeout_start, sent != failed + delivered @@ -47,7 +49,7 @@ def timeout_job_counts(notifications_type, timeout_start): delivered_count = 0 failed_count = 0 for notification_status in job: - if notification_status.status in [NOTIFICATION_DELIVERED, NOTIFICATION_SENT]: + if notification_status.status in NOTIFICATION_STATUS_SUCCESS: delivered_count += notification_status.count else: failed_count += notification_status.count diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index 23e15f65f..4425a3409 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -780,7 +780,7 @@ def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sm assert sms_stats[0].emails_delivered == 0 assert sms_stats[0].sms_delivered == 0 assert sms_stats[0].sms_failed == 1 - assert sms_stats[0].emails_failed == 100 + assert sms_stats[0].emails_failed == 0 # this test is as above, but for SMS not email From b233ae46f3899680fbfb0b641353e27076acde64 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:53:46 +0100 Subject: [PATCH 51/68] Tidy up test code for service permissions --- tests/app/dao/test_service_permissions_dao.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index 248dd6ae6..8005e2af2 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,20 +1,17 @@ import pytest -from app.dao.service_permissions_dao import ( - dao_fetch_service_permissions, dao_remove_service_permission) -from app.models import ( - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) +from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission +from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE from tests.app.db import create_service_permission def test_create_service_permission(sample_service): - service_permission = create_service_permission( - service_id=sample_service.id, permission=SMS_TYPE) + service_permissions = create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) - assert len(service_permission) == 1 - assert service_permission[0].service_id == sample_service.id - assert service_permission[0].permission == SMS_TYPE + assert len(service_permissions) == 1 + assert service_permissions[0].service_id == sample_service.id + assert service_permissions[0].permission == SMS_TYPE def test_fetch_service_permissions_gets_service_permissions(sample_service): From 2a488910255848eb0559fad7986b1faac29dd807 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:54:32 +0100 Subject: [PATCH 52/68] Removed unused pytest from test --- tests/app/dao/test_service_permissions_dao.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index 8005e2af2..a098d1b0f 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,5 +1,3 @@ -import pytest - from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE From 1dc3970595e6587eaf52c5680ef1a60364b8f817 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 16 May 2017 12:55:12 +0100 Subject: [PATCH 53/68] Code tidy up as be @imdad comments. --- app/notifications/process_notifications.py | 5 ++--- app/v2/notifications/post_notifications.py | 5 +---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 9247f813d..41b6614cb 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -38,7 +38,7 @@ def persist_notification( notification_type, api_key_id, key_type, - created_at=None, + created_at=datetime.utcnow(), job_id=None, job_row_number=None, reference=None, @@ -47,7 +47,6 @@ def persist_notification( simulated=False ): - notification_created_at = created_at or datetime.utcnow() notification = Notification( id=notification_id, template_id=template_id, @@ -59,7 +58,7 @@ def persist_notification( notification_type=notification_type, api_key_id=api_key_id, key_type=key_type, - created_at=notification_created_at, + created_at=created_at, job_id=job_id, job_row_number=job_row_number, client_reference=client_reference, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 459cf0050..a5a3a1ec4 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -67,10 +67,7 @@ def post_notification(notification_type): else: current_app.logger.info("POST simulated notification for id: {}".format(notification.id)) if notification_type == SMS_TYPE: - sms_sender = (authenticated_service.sms_sender - if authenticated_service.sms_sender - else current_app.config.get('FROM_NUMBER') - ) + sms_sender = authenticated_service.sms_sender or current_app.config.get('FROM_NUMBER') resp = create_post_sms_response_from_notification(notification=notification, body=str(template_with_content), from_number=sms_sender, From 3602431c2a7212c8c1b89cfe5d2c5c598fa743fe Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 13:41:54 +0100 Subject: [PATCH 54/68] Renamed test and refactored fixtures --- tests/app/service/test_rest.py | 62 +++++++++++++++++----------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2f4732473..93621668f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -941,39 +941,39 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db assert result['message'] == expected_message -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - second_user = create_user(email="new@digital.cabinet-office.gov.uk") - # Simulates successfully adding a user to the service - second_permission = create_user_service_permission( - notify_db, - notify_db_session, - user=second_user) - endpoint = url_for( - 'service.remove_user_from_service', - service_id=str(second_permission.service.id), - user_id=str(second_permission.user.id)) - auth_header = create_authorization_header() - resp = client.delete( - endpoint, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 204 +def test_remove_user_from_service( + notify_db, notify_db_session, client, sample_user_service_permission +): + second_user = create_user(email="new@digital.cabinet-office.gov.uk") + # Simulates successfully adding a user to the service + second_permission = create_user_service_permission( + notify_db, + notify_db_session, + user=second_user) + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(second_permission.service.id), + user_id=str(second_permission.user.id)) + auth_header = create_authorization_header() + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 204 -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - second_user = create_user(email="new@digital.cabinet-office.gov.uk") - endpoint = url_for( - 'service.remove_user_from_service', - service_id=str(sample_user_service_permission.service.id), - user_id=str(second_user.id)) - auth_header = create_authorization_header() - resp = client.delete( - endpoint, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 404 +def test_remove_non_existant_user_from_service( + client, sample_user_service_permission +): + second_user = create_user(email="new@digital.cabinet-office.gov.uk") + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(sample_user_service_permission.service.id), + user_id=str(second_user.id)) + auth_header = create_authorization_header() + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 404 def test_cannot_remove_only_user_from_service(notify_api, From d2a7a7b3c9ef84bdba60c9ef927834d326ae707d Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 16 May 2017 13:55:32 +0100 Subject: [PATCH 55/68] Fixed error in code --- app/notifications/process_notifications.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 41b6614cb..7a8cf9065 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -81,7 +81,7 @@ def persist_notification( if redis_store.get_all_from_hash(cache_key_for_service_template_counter(service.id)): redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) current_app.logger.info( - "{} {} created at {}".format(notification_type, notification_id, notification_created_at) + "{} {} created at {}".format(notification_type, notification_id, created_at) ) return notification From 09f45332eb34339065d63fe2afff5a8ca9f4a879 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 17 May 2017 13:25:40 +0100 Subject: [PATCH 56/68] Fixed issues caused by single evaluation of method signature defaults --- app/notifications/process_notifications.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 7a8cf9065..f791ab754 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -38,7 +38,7 @@ def persist_notification( notification_type, api_key_id, key_type, - created_at=datetime.utcnow(), + created_at, job_id=None, job_row_number=None, reference=None, @@ -47,6 +47,8 @@ def persist_notification( simulated=False ): + notification_created_at = created_at or datetime.utcnow() + notification = Notification( id=notification_id, template_id=template_id, @@ -58,7 +60,7 @@ def persist_notification( notification_type=notification_type, api_key_id=api_key_id, key_type=key_type, - created_at=created_at, + created_at=notification_created_at, job_id=job_id, job_row_number=job_row_number, client_reference=client_reference, @@ -81,7 +83,7 @@ def persist_notification( if redis_store.get_all_from_hash(cache_key_for_service_template_counter(service.id)): redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) current_app.logger.info( - "{} {} created at {}".format(notification_type, notification_id, created_at) + "{} {} created at {}".format(notification_type, notification_id, notification_created_at) ) return notification From ee5bb5f01a1258308ac3c7394ab519e6c93e04e9 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 17 May 2017 13:27:05 +0100 Subject: [PATCH 57/68] Fixed issues caused by single evaluation of method signature defaults --- app/notifications/process_notifications.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index f791ab754..1d03efe8b 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -38,7 +38,7 @@ def persist_notification( notification_type, api_key_id, key_type, - created_at, + created_at=None, job_id=None, job_row_number=None, reference=None, From 54446d5f4dfe9099510af930b128397446578298 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 17 May 2017 14:09:18 +0100 Subject: [PATCH 58/68] Add default permissions when creating a service --- app/dao/service_permissions_dao.py | 7 ++- app/dao/services_dao.py | 11 +++- app/models.py | 53 +++++++++------- tests/app/dao/test_service_permissions_dao.py | 40 +++++++----- tests/app/dao/test_services_dao.py | 63 ++++++++++++++++++- tests/app/db.py | 9 +-- 6 files changed, 135 insertions(+), 48 deletions(-) diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 5c38ad6a2..3bba3cc0d 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -9,13 +9,14 @@ def dao_fetch_service_permissions(service_id): @transactional -def dao_create_service_permission(service_id, permission): +def dao_add_service_permission(service_id, permission): service_permission = ServicePermission(service_id=service_id, permission=permission) - db.session.add(service_permission) def dao_remove_service_permission(service_id, permission): - return ServicePermission.query.filter( + deleted = ServicePermission.query.filter( ServicePermission.service_id == service_id, ServicePermission.permission == permission).delete() + db.session.commit() + return deleted diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 4bf073215..9cc90a9d6 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -25,9 +25,12 @@ from app.models import ( User, InvitedUser, Service, + ServicePermission, KEY_TYPE_TEST, NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES, + SMS_TYPE, + EMAIL_TYPE ) from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd @@ -124,13 +127,18 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, service_id=None): +def dao_create_service(service, user, service_id=None, service_permissions=[SMS_TYPE, EMAIL_TYPE]): from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) service.id = service_id or uuid.uuid4() # must be set now so version history model can use same id service.active = True service.research_mode = False + + for permission in service_permissions: + service_permission = ServicePermission(service_id=service.id, permission=permission) + db.session.add(service_permission) + db.session.add(service) @@ -185,6 +193,7 @@ def delete_service_and_all_associated_db_objects(service): _delete_commit(Job.query.filter_by(service=service)) _delete_commit(Template.query.filter_by(service=service)) _delete_commit(TemplateHistory.query.filter_by(service_id=service.id)) + _delete_commit(ServicePermission.query.filter_by(service_id=service.id)) verify_codes = VerifyCode.query.join(User).filter(User.id.in_([x.id for x in service.users])) list(map(db.session.delete, verify_codes)) diff --git a/app/models.py b/app/models.py index ecbc37f28..c8a9b442f 100644 --- a/app/models.py +++ b/app/models.py @@ -143,6 +143,30 @@ class DVLAOrganisation(db.Model): name = db.Column(db.String(255), nullable=True) +INTERNATIONAL_SMS_TYPE = 'international_sms' +INCOMING_SMS_TYPE = 'incoming_sms' + +SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE] + + +class ServicePermissionTypes(db.Model): + __tablename__ = 'service_permission_types' + + name = db.Column(db.String(255), primary_key=True) + + +class ServicePermission(db.Model): + __tablename__ = "service_permissions" + + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), + primary_key=True, index=True, nullable=False) + service = db.relationship('Service') + permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), + index=True, primary_key=True, nullable=False) + created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + class Service(db.Model, Versioned): __tablename__ = 'services' @@ -193,30 +217,13 @@ class Service(db.Model, Versioned): nullable=False, default=BRANDING_GOVUK ) + permissions = db.relationship('ServicePermission') - -INTERNATIONAL_SMS_TYPE = 'international_sms' -INCOMING_SMS_TYPE = 'incoming_sms' - -SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE] - - -class ServicePermissionTypes(db.Model): - __tablename__ = 'service_permission_types' - - name = db.Column(db.String(255), primary_key=True) - - -class ServicePermission(db.Model): - __tablename__ = "service_permissions" - - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), - primary_key=True, index=True, nullable=False) - service = db.relationship('Service') - permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), - index=True, primary_key=True, nullable=False) - created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) - updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + # This is only for backward compatibility and will be dropped when the columns are removed from the data model + def set_permissions(self): + if self.permissions: + self.can_send_letters = LETTER_TYPE in [p.permission for p in self.permissions] + self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions] MOBILE_TYPE = 'mobile' diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index a098d1b0f..c41c891f4 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,36 +1,44 @@ +import pytest + from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE -from tests.app.db import create_service_permission +from tests.app.db import create_service_permission, create_service -def test_create_service_permission(sample_service): - service_permissions = create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) +@pytest.fixture(scope='function') +def service_without_permissions(notify_db, notify_db_session): + return create_service(service_permissions=[]) + + +def test_create_service_permission(service_without_permissions): + service_permissions = create_service_permission( + service_id=service_without_permissions.id, permission=SMS_TYPE) assert len(service_permissions) == 1 - assert service_permissions[0].service_id == sample_service.id + assert service_permissions[0].service_id == service_without_permissions.id assert service_permissions[0].permission == SMS_TYPE -def test_fetch_service_permissions_gets_service_permissions(sample_service): - create_service_permission(service_id=sample_service.id, permission=LETTER_TYPE) - create_service_permission(service_id=sample_service.id, permission=INTERNATIONAL_SMS_TYPE) - create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) +def test_fetch_service_permissions_gets_service_permissions(service_without_permissions): + create_service_permission(service_id=service_without_permissions.id, permission=LETTER_TYPE) + create_service_permission(service_id=service_without_permissions.id, permission=INTERNATIONAL_SMS_TYPE) + create_service_permission(service_id=service_without_permissions.id, permission=SMS_TYPE) - service_permissions = dao_fetch_service_permissions(sample_service.id) + service_permissions = dao_fetch_service_permissions(service_without_permissions.id) assert len(service_permissions) == 3 - assert all(sp.service_id == sample_service.id for sp in service_permissions) + assert all(sp.service_id == service_without_permissions.id for sp in service_permissions) assert all(sp.permission in [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] for sp in service_permissions) -def test_remove_service_permission(sample_service): - create_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE) - create_service_permission(service_id=sample_service.id, permission=INCOMING_SMS_TYPE) +def test_remove_service_permission(service_without_permissions): + create_service_permission(service_id=service_without_permissions.id, permission=EMAIL_TYPE) + create_service_permission(service_id=service_without_permissions.id, permission=INCOMING_SMS_TYPE) - dao_remove_service_permission(sample_service.id, EMAIL_TYPE) + dao_remove_service_permission(service_without_permissions.id, EMAIL_TYPE) - permissions = dao_fetch_service_permissions(sample_service.id) + permissions = dao_fetch_service_permissions(service_without_permissions.id) assert len(permissions) == 1 assert permissions[0].permission == INCOMING_SMS_TYPE - assert permissions[0].service_id == sample_service.id + assert permissions[0].service_id == service_without_permissions.id diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 89b909431..3faf911e4 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -27,6 +27,7 @@ from app.dao.services_dao import ( dao_resume_service, dao_fetch_active_users_for_service ) +from app.dao.service_permissions_dao import dao_add_service_permission, dao_remove_service_permission from app.dao.users_dao import save_model_user from app.models import ( NotificationStatistics, @@ -47,7 +48,11 @@ from app.models import ( DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, - KEY_TYPE_TEST + KEY_TYPE_TEST, + EMAIL_TYPE, + SMS_TYPE, + LETTER_TYPE, + INTERNATIONAL_SMS_TYPE ) from tests.app.db import create_user, create_service @@ -245,6 +250,62 @@ def test_get_service_by_id_returns_service(service_factory): assert dao_fetch_service_by_id(service.id).name == 'testing' +def test_create_service_returns_service_with_default_permissions(service_factory): + service = service_factory.get('testing', email_from='testing') + + service = dao_fetch_service_by_id(service.id) + assert len(service.permissions) == 2 + assert all(p.permission in [SMS_TYPE, EMAIL_TYPE] for p in service.permissions) + + +# This test is only for backward compatibility and will be removed +# when the 'can_use' columns are dropped from the Service data model +@pytest.mark.parametrize("permission_to_add, can_send_letters, can_send_international_sms", + [(LETTER_TYPE, True, False), + (INTERNATIONAL_SMS_TYPE, False, True)]) +def test_create_service_by_id_adding_service_permission_returns_service_with_permissions_set( + service_factory, permission_to_add, can_send_letters, can_send_international_sms): + service = service_factory.get('testing', email_from='testing') + + dao_add_service_permission(service_id=service.id, permission=permission_to_add) + service.set_permissions() + + service = dao_fetch_service_by_id(service.id) + assert len(service.permissions) == 3 + assert all(p.permission in [SMS_TYPE, EMAIL_TYPE, permission_to_add] for p in service.permissions) + assert service.can_send_letters == can_send_letters + assert service.can_send_international_sms == can_send_international_sms + + +def test_remove_permission_from_service_by_id_returns_service_with_correct_permissions(service_factory): + service = service_factory.get('testing', email_from='testing') + dao_remove_service_permission(service_id=service.id, permission=SMS_TYPE) + + service = dao_fetch_service_by_id(service.id) + assert len(service.permissions) == 1 + assert service.permissions[0].permission == EMAIL_TYPE + + +def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory): + service = service_factory.get('testing', email_from='testing') + + dao_add_service_permission(service_id=service.id, permission=LETTER_TYPE) + service.set_permissions() + + service = dao_fetch_service_by_id(service.id) + assert len(service.permissions) == 3 + assert all(p.permission in [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE] for p in service.permissions) + assert service.can_send_letters + + dao_remove_service_permission(service_id=service.id, permission=LETTER_TYPE) + service.set_permissions() + service = dao_fetch_service_by_id(service.id) + + assert len(service.permissions) == 2 + assert all(p.permission in [SMS_TYPE, EMAIL_TYPE] for p in service.permissions) + assert not service.can_send_letters + + def test_create_service_creates_a_history_record_with_current_data(sample_user): assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 diff --git a/tests/app/db.py b/tests/app/db.py index 49524b131..3b391f3da 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -8,7 +8,7 @@ from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -from app.dao.service_permissions_dao import dao_create_service_permission +from app.dao.service_permissions_dao import dao_add_service_permission def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk", state='active'): @@ -27,7 +27,8 @@ def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-off return user -def create_service(user=None, service_name="Sample service", service_id=None, restricted=False): +def create_service( + user=None, service_name="Sample service", service_id=None, service_permissions=[EMAIL_TYPE, SMS_TYPE]): service = Service( name=service_name, message_limit=1000, @@ -35,7 +36,7 @@ def create_service(user=None, service_name="Sample service", service_id=None, re email_from=service_name.lower().replace(' ', '.'), created_by=user or create_user() ) - dao_create_service(service, service.created_by, service_id) + dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) return service @@ -147,7 +148,7 @@ def create_job(template, def create_service_permission(service_id, permission=EMAIL_TYPE): - dao_create_service_permission( + dao_add_service_permission( service_id if service_id else create_service().id, permission) service_permissions = ServicePermission.query.all() From edff192efccbaa9cfa4f2b2e8836d8a7dbf9a26e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 17 May 2017 14:10:13 +0100 Subject: [PATCH 59/68] Remove whitespace --- tests/app/dao/test_services_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 3faf911e4..2dc6f199f 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -258,7 +258,7 @@ def test_create_service_returns_service_with_default_permissions(service_factory assert all(p.permission in [SMS_TYPE, EMAIL_TYPE] for p in service.permissions) -# This test is only for backward compatibility and will be removed +# This test is only for backward compatibility and will be removed # when the 'can_use' columns are dropped from the Service data model @pytest.mark.parametrize("permission_to_add, can_send_letters, can_send_international_sms", [(LETTER_TYPE, True, False), From e68c3900e832efb70fee6be3c44814b58878b78a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 17 May 2017 16:06:35 +0100 Subject: [PATCH 60/68] Add restricted argument for service mock --- tests/app/db.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/app/db.py b/tests/app/db.py index 3b391f3da..d418b1c3b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -28,7 +28,8 @@ def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-off def create_service( - user=None, service_name="Sample service", service_id=None, service_permissions=[EMAIL_TYPE, SMS_TYPE]): + user=None, service_name="Sample service", service_id=None, restricted=False, + service_permissions=[EMAIL_TYPE, SMS_TYPE]): service = Service( name=service_name, message_limit=1000, From ea794705ab8ffb12893a5b5d559620dd579f16c7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 May 2017 10:24:03 +0100 Subject: [PATCH 61/68] Removed whitespace from queue list --- manifest-delivery-base.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index e35324240..73457b3c9 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -38,7 +38,7 @@ applications: NOTIFY_APP_NAME: delivery-worker-sender - name: notify-delivery-worker-periodic - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=2 -Q periodic, statistics + command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=2 -Q periodic,statistics instances: 1 memory: 2G env: From a75db813346ce64f9974b5fb3259b156a845299e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 18 May 2017 13:12:05 +0100 Subject: [PATCH 62/68] Fix name condflicts with migration script --- ..._set_international.py => 0085_set_international.py} | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) rename migrations/versions/{0083_set_international.py => 0085_set_international.py} (83%) diff --git a/migrations/versions/0083_set_international.py b/migrations/versions/0085_set_international.py similarity index 83% rename from migrations/versions/0083_set_international.py rename to migrations/versions/0085_set_international.py index 8251901eb..1fe1818ad 100644 --- a/migrations/versions/0083_set_international.py +++ b/migrations/versions/0085_set_international.py @@ -1,7 +1,7 @@ """empty message -Revision ID: 0083_set_international -Revises: 0082_add_go_live_template +Revision ID: 0085_set_international +Revises: 0084_add_job_stats Create Date: 2017-05-05 15:26:34.621670 """ @@ -9,8 +9,8 @@ from datetime import datetime from alembic import op # revision identifiers, used by Alembic. -revision = '0083_set_international' -down_revision = '0082_add_go_live_template' +revision = '0085_set_international' +down_revision = '0084_add_job_stats' def upgrade(): @@ -37,7 +37,7 @@ def upgrade(): res2 = results2.fetchall() end = datetime.utcnow() - print("Started at: {} ended at: {}".format(start, end)) + print("Started at: {} ended at: {}, taking {}".format(start, end, end-start)) def downgrade(): From 097510b029dc616077b067e753a1494427d682ba Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 18 May 2017 14:28:49 +0100 Subject: [PATCH 63/68] Testing the update script --- migrations/versions/0085_set_international.py | 1 + 1 file changed, 1 insertion(+) diff --git a/migrations/versions/0085_set_international.py b/migrations/versions/0085_set_international.py index 1fe1818ad..2c2bbfe9a 100644 --- a/migrations/versions/0085_set_international.py +++ b/migrations/versions/0085_set_international.py @@ -13,6 +13,7 @@ revision = '0085_set_international' down_revision = '0084_add_job_stats' + def upgrade(): conn = op.get_bind() start = datetime.utcnow() From 9b79ac21892777c58c2e4ccd1c007889e445a336 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 18 May 2017 15:45:50 +0100 Subject: [PATCH 64/68] Revert "Script to update international flag 10,000 rows at a time." --- migrations/versions/0085_set_international.py | 46 ------------------- 1 file changed, 46 deletions(-) delete mode 100644 migrations/versions/0085_set_international.py diff --git a/migrations/versions/0085_set_international.py b/migrations/versions/0085_set_international.py deleted file mode 100644 index 2c2bbfe9a..000000000 --- a/migrations/versions/0085_set_international.py +++ /dev/null @@ -1,46 +0,0 @@ -"""empty message - -Revision ID: 0085_set_international -Revises: 0084_add_job_stats -Create Date: 2017-05-05 15:26:34.621670 - -""" -from datetime import datetime -from alembic import op - -# revision identifiers, used by Alembic. -revision = '0085_set_international' -down_revision = '0084_add_job_stats' - - - -def upgrade(): - conn = op.get_bind() - start = datetime.utcnow() - notification_history = "select id from notification_history where international is null limit 10000" - - results = conn.execute(notification_history) - res = results.fetchall() - - while len(res) > 0: - conn.execute("update notification_history set international = False where id in ({})".format( - notification_history)) - results = conn.execute(notification_history) - res = results.fetchall() - - notifications = "select id from notifications where international is null limit 10000" - results2 = conn.execute(notifications) - res2 = results2.fetchall() - while len(res2) > 0: - conn.execute("update notifications set international = False where id in ({})".format(notifications)) - - results2 = conn.execute(notifications) - res2 = results2.fetchall() - - end = datetime.utcnow() - print("Started at: {} ended at: {}, taking {}".format(start, end, end-start)) - - -def downgrade(): - # There is no way to downgrade this update. - pass From 0e0c18583f0ab0a9ac681564ad6b66f91675e4af Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 19 May 2017 10:16:34 +0100 Subject: [PATCH 65/68] Fix test data and how we parse the JSON --- .../notifications_letter_callback.py | 5 +++- .../app/notifications/rest/test_callbacks.py | 30 +------------------ 2 files changed, 5 insertions(+), 30 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 1de6c0009..cfcaf28df 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -1,3 +1,5 @@ +import json + from functools import wraps from flask import ( @@ -48,7 +50,8 @@ def process_letter_response(): current_app.logger.info('Received SNS callback: {}'.format(req_json)) if not autoconfirm_subscription(req_json): # The callback should have one record for an S3 Put Event. - filename = req_json['Message']['Records'][0]['s3']['object']['key'] + message = json.loads(req_json['Message']) + filename = message['Records'][0]['s3']['object']['key'] current_app.logger.info('Received file from DVLA: {}'.format(filename)) current_app.logger.info('DVLA callback: Calling task to update letter notifications') update_letter_notifications_statuses.apply_async([filename], queue='notify') diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 5c8f96baa..2c2ffd9cf 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -531,33 +531,5 @@ def _sample_sns_s3_callback(): "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d", "Subject": "Amazon S3 Notification", "TopicArn": "sample-topic-arn", - "Message": { - "Records": [{ - "eventVersion": "2.0", - "eventSource": "aws:s3", - "awsRegion": "eu-west-1", - "eventTime": "2017-05-03T08:35:12.826Z", - "eventName": "ObjectCreated:Put", - "userIdentity": {"principalId": "some-p-id"}, - "requestParameters": {"sourceIPAddress": "8.8.8.8"}, - "responseElements": {"x-amz-request-id": "some-req-id", "x-amz-id-2": "some-amz-id"}, - "s3": { - "s3SchemaVersion": "1.0", - "configurationId": "some-config-id", - "bucket": { - "name": "some-bucket", - "ownerIdentity": {"principalId": "some-p-id"}, - "arn": "some-bucket-arn" - }, - "object": { - "key": "bar.txt", - "size": 200, - "eTag": "some-etag", - "versionId": "some-v-id", - "sequencer": "some-seq" - } - } - } - ] - } + "Message": '{"Records":[{"eventVersion":"2.0","eventSource":"aws:s3","awsRegion":"eu-west-1","eventTime":"2017-05-16T11:38:41.073Z","eventName":"ObjectCreated:Put","userIdentity":{"principalId":"some-p-id"},"requestParameters":{"sourceIPAddress":"8.8.8.8"},"responseElements":{"x-amz-request-id":"some-r-id","x-amz-id-2":"some-x-am-id"},"s3":{"s3SchemaVersion":"1.0","configurationId":"some-c-id","bucket":{"name":"some-bucket","ownerIdentity":{"principalId":"some-p-id"},"arn":"some-bucket-arn"},"object":{"key":"bar.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}' # noqa }) From 069cf35878e7c25ff951109e75723e234a53c553 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 19 May 2017 10:31:36 +0100 Subject: [PATCH 66/68] Script to update the international flag from null to false --- migrations/versions/0085_set_international.py | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 migrations/versions/0085_set_international.py diff --git a/migrations/versions/0085_set_international.py b/migrations/versions/0085_set_international.py new file mode 100644 index 000000000..1fe1818ad --- /dev/null +++ b/migrations/versions/0085_set_international.py @@ -0,0 +1,45 @@ +"""empty message + +Revision ID: 0085_set_international +Revises: 0084_add_job_stats +Create Date: 2017-05-05 15:26:34.621670 + +""" +from datetime import datetime +from alembic import op + +# revision identifiers, used by Alembic. +revision = '0085_set_international' +down_revision = '0084_add_job_stats' + + +def upgrade(): + conn = op.get_bind() + start = datetime.utcnow() + notification_history = "select id from notification_history where international is null limit 10000" + + results = conn.execute(notification_history) + res = results.fetchall() + + while len(res) > 0: + conn.execute("update notification_history set international = False where id in ({})".format( + notification_history)) + results = conn.execute(notification_history) + res = results.fetchall() + + notifications = "select id from notifications where international is null limit 10000" + results2 = conn.execute(notifications) + res2 = results2.fetchall() + while len(res2) > 0: + conn.execute("update notifications set international = False where id in ({})".format(notifications)) + + results2 = conn.execute(notifications) + res2 = results2.fetchall() + + end = datetime.utcnow() + print("Started at: {} ended at: {}, taking {}".format(start, end, end-start)) + + +def downgrade(): + # There is no way to downgrade this update. + pass From 315e776ebb3cf13cec5129c5d552e19edb4634fd Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 19 May 2017 12:22:05 +0100 Subject: [PATCH 67/68] Revert "Script to update the international flag from null to false" --- migrations/versions/0085_set_international.py | 45 ------------------- 1 file changed, 45 deletions(-) delete mode 100644 migrations/versions/0085_set_international.py diff --git a/migrations/versions/0085_set_international.py b/migrations/versions/0085_set_international.py deleted file mode 100644 index 1fe1818ad..000000000 --- a/migrations/versions/0085_set_international.py +++ /dev/null @@ -1,45 +0,0 @@ -"""empty message - -Revision ID: 0085_set_international -Revises: 0084_add_job_stats -Create Date: 2017-05-05 15:26:34.621670 - -""" -from datetime import datetime -from alembic import op - -# revision identifiers, used by Alembic. -revision = '0085_set_international' -down_revision = '0084_add_job_stats' - - -def upgrade(): - conn = op.get_bind() - start = datetime.utcnow() - notification_history = "select id from notification_history where international is null limit 10000" - - results = conn.execute(notification_history) - res = results.fetchall() - - while len(res) > 0: - conn.execute("update notification_history set international = False where id in ({})".format( - notification_history)) - results = conn.execute(notification_history) - res = results.fetchall() - - notifications = "select id from notifications where international is null limit 10000" - results2 = conn.execute(notifications) - res2 = results2.fetchall() - while len(res2) > 0: - conn.execute("update notifications set international = False where id in ({})".format(notifications)) - - results2 = conn.execute(notifications) - res2 = results2.fetchall() - - end = datetime.utcnow() - print("Started at: {} ended at: {}, taking {}".format(start, end, end-start)) - - -def downgrade(): - # There is no way to downgrade this update. - pass From 02db3be37c8ffd986baef3b946efc5068676d50e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 22 May 2017 10:12:18 +0100 Subject: [PATCH 68/68] General refactor --- app/celery/tasks.py | 25 +++++++++++-------------- tests/app/celery/test_tasks.py | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 48923d228..cce6b05f0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,5 +1,3 @@ -import random - from datetime import (datetime) from collections import namedtuple @@ -361,21 +359,20 @@ def get_template_class(template_type): @statsd(namespace="tasks") def update_letter_notifications_statuses(self, filename): bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']) - response_file = s3.get_s3_file(bucket_location, filename) + response_file_content = s3.get_s3_file(bucket_location, filename) try: - NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) - notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()] - + notification_updates = process_updates_from_file(response_file_content) except TypeError: current_app.logger.exception('DVLA response file: {} has an invalid format'.format(filename)) raise - else: - if notification_updates: - for update in notification_updates: - current_app.logger.info('DVLA update: {}'.format(str(update))) - # TODO: Update notifications with desired status - return notification_updates - else: - current_app.logger.exception('DVLA response file contained no updates') + for update in notification_updates: + current_app.logger.info('DVLA update: {}'.format(str(update))) + # TODO: Update notifications with desired status + + +def process_updates_from_file(response_file): + NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) + notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()] + return notification_updates diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 560f68c27..27c9833a4 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -24,7 +24,8 @@ from app.celery.tasks import ( persist_letter, get_template_class, update_job_to_sent_to_dvla, - update_letter_notifications_statuses + update_letter_notifications_statuses, + process_updates_from_file ) from app.dao import jobs_dao, services_dao from app.models import ( @@ -1094,10 +1095,19 @@ def test_update_letter_notifications_statuses_calls_with_correct_bucket_location s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt') -def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): +def test_update_letter_notifications_statuses_builds_updates_from_content(notify_api, mocker): valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) - updates = update_letter_notifications_statuses(filename='foo.txt') + update_mock = mocker.patch('app.celery.tasks.process_updates_from_file') + + update_letter_notifications_statuses(filename='foo.txt') + + update_mock.assert_called_with('ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted') + + +def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): + valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' + updates = process_updates_from_file(valid_file) assert len(updates) == 2