From ea4214c7d50450aee210cd7216b5c82b6f0cd984 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 14 Mar 2017 10:50:09 +0000 Subject: [PATCH 01/11] Added a jobs_dao method to answer if the all the notifications have been created in the database for the given job. --- app/dao/jobs_dao.py | 20 +++++++++++++++++++- tests/app/dao/test_jobs_dao.py | 21 +++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index a9f1bd9b9..2a0ef4731 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -5,7 +5,11 @@ from sqlalchemy import func, desc, asc, cast, Date as sql_date from app import db from app.dao import days_ago -from app.models import Job, NotificationHistory, JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING +from app.models import (Job, + Notification, + NotificationHistory, + JOB_STATUS_SCHEDULED, + JOB_STATUS_PENDING) from app.statsd_decorators import statsd @@ -24,6 +28,20 @@ def dao_get_notification_outcomes_for_job(service_id, job_id): .all() +@statsd(namespace="dao") +def are_all_notifications_created_for_job(job_id): + query = db.session.query(func.count(Notification.id))\ + .join(Job)\ + .filter(Job.id == job_id)\ + .group_by(Job.id)\ + .having(func.count(Notification.id) == Job.notification_count).first() + + if query: + return True + else: + return False + + def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).one() diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index dca1eb7d8..45af77bd6 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -12,8 +12,8 @@ from app.dao.jobs_dao import ( dao_set_scheduled_jobs_to_pending, dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job, - dao_get_jobs_older_than -) + dao_get_jobs_older_than, + are_all_notifications_created_for_job) from app.models import Job from tests.app.conftest import sample_notification as create_notification @@ -314,3 +314,20 @@ def test_get_jobs_for_service_doesnt_return_test_messages(notify_db, notify_db_s jobs = dao_get_jobs_by_service_id(sample_job.service_id).items assert jobs == [sample_job] + + +def test_are_all_notifications_created_for_job_returns_true(notify_db, notify_db_session, sample_job): + create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job) + job_is_complete = are_all_notifications_created_for_job(sample_job.id) + assert job_is_complete + + +def test_are_all_notifications_created_for_job_returns_false(notify_db, notify_db_session): + job = create_job(notify_db=notify_db, notify_db_session=notify_db_session, notification_count=2) + job_is_complete = are_all_notifications_created_for_job(job.id) + assert not job_is_complete + + +def test_are_all_notifications_created_for_job_returns_false_when_job_does_not_exist(notify_db, notify_db_session): + job_is_complete = are_all_notifications_created_for_job(uuid.uuid4()) + assert not job_is_complete From 9e4b1b2bfc940d65a706b59d2f76cc2cc4535d13 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 14 Mar 2017 15:25:36 +0000 Subject: [PATCH 02/11] Add schemas, endpoints and supporting tests --- app/__init__.py | 2 + app/models.py | 30 ++++++ app/v2/template/__init__.py | 7 ++ app/v2/template/get_template.py | 31 +++++++ app/v2/template/template_schemas.py | 41 ++++++++ tests/app/v2/template/__init__.py | 0 tests/app/v2/template/test_get_template.py | 93 +++++++++++++++++++ .../app/v2/template/test_template_schemas.py | 67 +++++++++++++ 8 files changed, 271 insertions(+) create mode 100644 app/v2/template/__init__.py create mode 100644 app/v2/template/get_template.py create mode 100644 app/v2/template/template_schemas.py create mode 100644 tests/app/v2/template/__init__.py create mode 100644 tests/app/v2/template/test_get_template.py create mode 100644 tests/app/v2/template/test_template_schemas.py diff --git a/app/__init__.py b/app/__init__.py index d40b58895..39271546c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -110,9 +110,11 @@ def register_blueprint(application): def register_v2_blueprints(application): from app.v2.notifications.post_notifications import notification_blueprint as post_notifications from app.v2.notifications.get_notifications import notification_blueprint as get_notifications + from app.v2.template.get_template import template_blueprint application.register_blueprint(post_notifications) application.register_blueprint(get_notifications) + application.register_blueprint(template_blueprint) def init_app(app): diff --git a/app/models.py b/app/models.py index 32417b47d..894f42df6 100644 --- a/app/models.py +++ b/app/models.py @@ -320,6 +320,21 @@ class Template(db.Model): _external=True ) + def serialize(self): + + serialized = { + "id": self.id, + "type": self.template_type, + "created_at": self.created_at.strftime(DATETIME_FORMAT), + "updated_at": self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, + "created_by": self.created_by.email_address, + "version": self.version, + "body": self.content, + "subject": self.subject if self.template_type == EMAIL_TYPE else None + } + + return serialized + class TemplateHistory(db.Model): __tablename__ = 'templates_history' @@ -343,6 +358,21 @@ class TemplateHistory(db.Model): nullable=False, default=NORMAL) + def serialize(self): + + serialized = { + "id": self.id, + "type": self.template_type, + "created_at": self.created_at.strftime(DATETIME_FORMAT), + "updated_at": self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, + "created_by": self.created_by.email_address, + "version": self.version, + "body": self.content, + "subject": self.subject if self.template_type == EMAIL_TYPE else None + } + + return serialized + MMG_PROVIDER = "mmg" FIRETEXT_PROVIDER = "firetext" diff --git a/app/v2/template/__init__.py b/app/v2/template/__init__.py new file mode 100644 index 000000000..ad051cfca --- /dev/null +++ b/app/v2/template/__init__.py @@ -0,0 +1,7 @@ +from flask import Blueprint + +from app.v2.errors import register_errors + +template_blueprint = Blueprint("v2_template", __name__, url_prefix='/v2/template') + +register_errors(template_blueprint) diff --git a/app/v2/template/get_template.py b/app/v2/template/get_template.py new file mode 100644 index 000000000..25d6511c3 --- /dev/null +++ b/app/v2/template/get_template.py @@ -0,0 +1,31 @@ +import uuid + +from flask import jsonify, request +from werkzeug.exceptions import abort + +from app import api_user +from app.dao import templates_dao +from app.schema_validation import validate +from app.v2.template import template_blueprint +from app.v2.template.template_schemas import get_template_by_id_request + + +@template_blueprint.route("/", methods=['GET']) +@template_blueprint.route("//version/", methods=['GET']) +def get_template_by_id(template_id, version=None): + try: + casted_id = uuid.UUID(template_id) + + _data = {} + _data['id'] = template_id + if version: + _data['version'] = int(version) + + data = validate(_data, get_template_by_id_request) + except ValueError or AttributeError: + abort(404) + + template = templates_dao.dao_get_template_by_id_and_service_id( + casted_id, api_user.service_id, data.get('version')) + + return jsonify(template.serialize()), 200 diff --git a/app/v2/template/template_schemas.py b/app/v2/template/template_schemas.py new file mode 100644 index 000000000..2a740e6cf --- /dev/null +++ b/app/v2/template/template_schemas.py @@ -0,0 +1,41 @@ +from app.models import TEMPLATE_TYPES +from app.schema_validation.definitions import uuid + + +get_template_by_id_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "schema for query parameters allowed when getting list of notifications", + "type": "object", + "properties": { + "id": uuid, + "version": {"type": ["integer", "null"], "minimum": 1} + }, + "required": ["id"], + "additionalProperties": False, +} + +get_template_by_id_response = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "GET template by id schema response", + "type": "object", + "title": "reponse v2/template", + "properties": { + "id": uuid, + "type": {"enum": TEMPLATE_TYPES}, + "created_at": { + "format": "date-time", + "type": "string", + "description": "Date+time created" + }, + "updated_at": { + "format": "date-time", + "type": "string", + "description": "Date+time updated" + }, + "created_by": {"type": "string"}, + "version": {"type": "integer"}, + "body": {"type": "string"}, + "subject": {"type": ["string", "null"]} + }, + "required": ["id", "type", "created_at", "updated_at", "version", "created_by", "body"] +} diff --git a/tests/app/v2/template/__init__.py b/tests/app/v2/template/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/v2/template/test_get_template.py b/tests/app/v2/template/test_get_template.py new file mode 100644 index 000000000..c809532c9 --- /dev/null +++ b/tests/app/v2/template/test_get_template.py @@ -0,0 +1,93 @@ +import pytest + +from flask import json + +from app import DATETIME_FORMAT +from tests import create_authorization_header +from tests.app.conftest import sample_template as create_sample_template + +EMAIL_TYPE = 'email' +SMS_TYPE = 'sms' +LETTER_TYPE = 'letter' + +template_types = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE] +valid_version_params = [None, 1] + + +@pytest.mark.parametrize("tmp_type", template_types) +@pytest.mark.parametrize("version", valid_version_params) +def test_get_email_template_by_id_returns_200(client, notify_db, notify_db_session, sample_service, tmp_type, version): + template = create_sample_template(notify_db, notify_db_session, template_type=tmp_type) + auth_header = create_authorization_header(service_id=sample_service.id) + + version_path = '/version/{}'.format(version) if version else '' + + response = client.get(path='/v2/template/{}{}'.format(template.id, version_path), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 200 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + + expected_response = { + 'id': '{}'.format(template.id), + 'type': '{}'.format(template.template_type), + 'created_at': template.created_at.strftime(DATETIME_FORMAT), + 'updated_at': None, + 'version': template.version, + 'created_by': template.created_by.email_address, + 'body': template.content, + "subject": template.subject if tmp_type == EMAIL_TYPE else None + } + + assert json_response == expected_response + + +def test_get_template_with_invalid_template_id_returns_404(client, sample_service): + auth_header = create_authorization_header(service_id=sample_service.id) + + invalid_template_id = 'some_other_id' + + response = client.get(path='/v2/template/{}'.format(invalid_template_id), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 404 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + + assert json_response == { + "message": "The requested URL was not found on the server. " + "If you entered the URL manually please check your spelling and try again.", + "result": "error" + } + + +@pytest.mark.parametrize("tmp_type", template_types) +def test_get_template_with_invalid_version_returns_404(client, notify_db, notify_db_session, sample_service, tmp_type): + template = create_sample_template( + notify_db, notify_db_session, template_type=tmp_type) + + auth_header = create_authorization_header(service_id=sample_service.id) + + # test with version number beyond latest version + invalid_version = template.version + 1 + + response = client.get(path='/v2/template/{}/version/{}'.format(template.id, invalid_version), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 404 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + + assert json_response == { + "errors": [ + { + "error": "NoResultFound", + "message": "No result found" + } + ], + "status_code": 404 + } diff --git a/tests/app/v2/template/test_template_schemas.py b/tests/app/v2/template/test_template_schemas.py new file mode 100644 index 000000000..fa747b05b --- /dev/null +++ b/tests/app/v2/template/test_template_schemas.py @@ -0,0 +1,67 @@ +import uuid + +import pytest +from flask import json + +from app.v2.template.template_schemas import ( + get_template_by_id_response, + get_template_by_id_request +) +from app.schema_validation import validate +from jsonschema.exceptions import ValidationError + + +valid_json = { + 'id': str(uuid.uuid4()), + 'type': 'email', + 'created_at': '2017-01-10T18:25:43.511Z', + 'updated_at': '2017-04-23T18:25:43.511Z', + 'version': 1, + 'created_by': 'someone@test.com', + 'body': "some body" +} + +valid_json_with_optionals = { + 'id': str(uuid.uuid4()), + 'type': 'email', + 'created_at': '2017-01-10T18:25:43.511Z', + 'updated_at': '2017-04-23T18:25:43.511Z', + 'version': 1, + 'created_by': 'someone', + 'body': "some body", + 'subject': "some subject" +} + +valid_request_args = [ + {"id": str(uuid.uuid4()), "version": 1}, {"id": str(uuid.uuid4())}] + +invalid_request_args = [ + ({"id": str(uuid.uuid4()), "version": "test"}, ["version test is not of type integer, null"]), + ({"id": str(uuid.uuid4()), "version": 0}, ["version 0 is less than the minimum of 1"]), + ({"version": 1}, ["id is a required property"]), + ({"id": "invalid_uuid"}, ["id is not a valid UUID"]), + ({"id": "invalid_uuid", "version": 0}, ["version 0 is less than the minimum of 1", + "id is not a valid UUID"]) +] + + +@pytest.mark.parametrize("args", valid_request_args) +def test_get_template_request_schema__against_valid_args_is_valid(args): + assert validate(args, get_template_by_id_request) == args + + +@pytest.mark.parametrize("args,error_message", invalid_request_args) +def test_get_template_request_schema_against_invalid_args_is_invalid(args, error_message): + with pytest.raises(ValidationError) as e: + validate(args, get_template_by_id_request) + errors = json.loads(str(e.value)) + + assert errors['status_code'] == 400 + + for error in errors['errors']: + assert error['message'] in error_message + + +@pytest.mark.parametrize("response", [valid_json, valid_json_with_optionals]) +def test_get_template_response_schema_is_valid(response): + assert validate(response, get_template_by_id_response) == response From a596a1bb87b19b24bb5456b5d96b508d85e66d0b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 14 Mar 2017 17:51:30 +0000 Subject: [PATCH 03/11] Refactored tests and get template endpoint --- app/v2/template/get_template.py | 9 +++--- tests/app/v2/template/test_get_template.py | 34 +++++++++++----------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/app/v2/template/get_template.py b/app/v2/template/get_template.py index 25d6511c3..794cba614 100644 --- a/app/v2/template/get_template.py +++ b/app/v2/template/get_template.py @@ -1,6 +1,7 @@ import uuid from flask import jsonify, request +from jsonschema.exceptions import ValidationError from werkzeug.exceptions import abort from app import api_user @@ -11,21 +12,19 @@ from app.v2.template.template_schemas import get_template_by_id_request @template_blueprint.route("/", methods=['GET']) -@template_blueprint.route("//version/", methods=['GET']) +@template_blueprint.route("//version/", methods=['GET']) def get_template_by_id(template_id, version=None): try: - casted_id = uuid.UUID(template_id) - _data = {} _data['id'] = template_id if version: - _data['version'] = int(version) + _data['version'] = version data = validate(_data, get_template_by_id_request) except ValueError or AttributeError: abort(404) template = templates_dao.dao_get_template_by_id_and_service_id( - casted_id, api_user.service_id, data.get('version')) + template_id, api_user.service_id, data.get('version')) return jsonify(template.serialize()), 200 diff --git a/tests/app/v2/template/test_get_template.py b/tests/app/v2/template/test_get_template.py index c809532c9..f883dec56 100644 --- a/tests/app/v2/template/test_get_template.py +++ b/tests/app/v2/template/test_get_template.py @@ -1,14 +1,12 @@ import pytest +import uuid from flask import json from app import DATETIME_FORMAT +from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE from tests import create_authorization_header -from tests.app.conftest import sample_template as create_sample_template - -EMAIL_TYPE = 'email' -SMS_TYPE = 'sms' -LETTER_TYPE = 'letter' +from tests.app.db import create_template template_types = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE] valid_version_params = [None, 1] @@ -16,8 +14,8 @@ valid_version_params = [None, 1] @pytest.mark.parametrize("tmp_type", template_types) @pytest.mark.parametrize("version", valid_version_params) -def test_get_email_template_by_id_returns_200(client, notify_db, notify_db_session, sample_service, tmp_type, version): - template = create_sample_template(notify_db, notify_db_session, template_type=tmp_type) +def test_get_email_template_by_id_returns_200(client, sample_service, tmp_type, version): + template = create_template(sample_service, template_type=tmp_type) auth_header = create_authorization_header(service_id=sample_service.id) version_path = '/version/{}'.format(version) if version else '' @@ -44,12 +42,12 @@ def test_get_email_template_by_id_returns_200(client, notify_db, notify_db_sessi assert json_response == expected_response -def test_get_template_with_invalid_template_id_returns_404(client, sample_service): +def test_get_template_with_non_existent_template_id_returns_404(client, sample_service): auth_header = create_authorization_header(service_id=sample_service.id) - invalid_template_id = 'some_other_id' + random_template_id = str(uuid.uuid4()) - response = client.get(path='/v2/template/{}'.format(invalid_template_id), + response = client.get(path='/v2/template/{}'.format(random_template_id), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 404 @@ -58,20 +56,22 @@ def test_get_template_with_invalid_template_id_returns_404(client, sample_servic json_response = json.loads(response.get_data(as_text=True)) assert json_response == { - "message": "The requested URL was not found on the server. " - "If you entered the URL manually please check your spelling and try again.", - "result": "error" + "errors": [ + { + "error": "NoResultFound", + "message": "No result found" + } + ], + "status_code": 404 } @pytest.mark.parametrize("tmp_type", template_types) -def test_get_template_with_invalid_version_returns_404(client, notify_db, notify_db_session, sample_service, tmp_type): - template = create_sample_template( - notify_db, notify_db_session, template_type=tmp_type) +def test_get_template_with_non_existent_version_returns_404(client, sample_service, tmp_type): + template = create_template(sample_service, template_type=tmp_type) auth_header = create_authorization_header(service_id=sample_service.id) - # test with version number beyond latest version invalid_version = template.version + 1 response = client.get(path='/v2/template/{}/version/{}'.format(template.id, invalid_version), From 7558a7661a9d6a158ec7dabf4287f4b8beb16132 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 14 Mar 2017 17:53:49 +0000 Subject: [PATCH 04/11] Updated schema description --- app/v2/template/template_schemas.py | 2 +- tests/app/v2/template/test_template_schemas.py | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/v2/template/template_schemas.py b/app/v2/template/template_schemas.py index 2a740e6cf..8356dbd0e 100644 --- a/app/v2/template/template_schemas.py +++ b/app/v2/template/template_schemas.py @@ -4,7 +4,7 @@ from app.schema_validation.definitions import uuid get_template_by_id_request = { "$schema": "http://json-schema.org/draft-04/schema#", - "description": "schema for query parameters allowed when getting list of notifications", + "description": "schema for parameters allowed when getting template by id", "type": "object", "properties": { "id": uuid, diff --git a/tests/app/v2/template/test_template_schemas.py b/tests/app/v2/template/test_template_schemas.py index fa747b05b..9e946c2e2 100644 --- a/tests/app/v2/template/test_template_schemas.py +++ b/tests/app/v2/template/test_template_schemas.py @@ -40,13 +40,12 @@ invalid_request_args = [ ({"id": str(uuid.uuid4()), "version": 0}, ["version 0 is less than the minimum of 1"]), ({"version": 1}, ["id is a required property"]), ({"id": "invalid_uuid"}, ["id is not a valid UUID"]), - ({"id": "invalid_uuid", "version": 0}, ["version 0 is less than the minimum of 1", - "id is not a valid UUID"]) + ({"id": "invalid_uuid", "version": 0}, ["version 0 is less than the minimum of 1", "id is not a valid UUID"]) ] @pytest.mark.parametrize("args", valid_request_args) -def test_get_template_request_schema__against_valid_args_is_valid(args): +def test_get_template_request_schema_against_valid_args_is_valid(args): assert validate(args, get_template_by_id_request) == args From 140179b4b6f6f59f669acb46235da949b0ff0800 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 15 Mar 2017 15:26:58 +0000 Subject: [PATCH 05/11] Create new task to build dvla file. This will transform each notification in a job to a row in a file. The file is then uploaded to S3. The files will later be aggregated by the notifications-ftp app to send to dvla. The method to upload the file to S3 should be pulled into notifications-utils package. It is the same method used in notifications-admin. --- app/celery/tasks.py | 63 +++++++++++++++++++++++++++++++--- app/config.py | 2 ++ app/dao/jobs_dao.py | 11 ++++-- tests/app/celery/test_tasks.py | 56 ++++++++++++++++++++++++++++-- tests/app/dao/test_jobs_dao.py | 27 ++++++++++----- tests/app/db.py | 30 +++++++++++++++- 6 files changed, 169 insertions(+), 20 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index cb9a40c88..fe2d37484 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,10 +1,12 @@ +import botocore +from boto3 import resource from datetime import (datetime) from flask import current_app from notifications_utils.recipients import ( RecipientCSV ) -from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate +from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from sqlalchemy.exc import SQLAlchemyError from app import ( create_uuid, @@ -16,8 +18,9 @@ from app.aws import s3 from app.celery import provider_tasks from app.dao.jobs_dao import ( dao_update_job, - dao_get_job_by_id -) + dao_get_job_by_id, + all_notifications_are_created_for_job, + dao_get_all_notifications_for_job) from app.dao.notifications_dao import get_notification_by_id from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id @@ -76,6 +79,8 @@ def process_job(job_id): current_app.logger.info( "Job {} created at {} started at {} finished at {}".format(job_id, job.created_at, start, finished) ) + if template.template_type == LETTER_TYPE: + build_dvla_file.apply_async([str(job.id)], queue='process-job') def process_row(row_number, recipient, personalisation, template, job, service): @@ -248,13 +253,61 @@ def persist_letter( notification_id=notification_id ) - # TODO: deliver letters - current_app.logger.info("Letter {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: handle_exception(self, notification, notification_id, e) +@notify_celery.task(bind=True, name="build-dvla-file", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def build_dvla_file(self, job_id): + if all_notifications_are_created_for_job(job_id): + notifications = dao_get_all_notifications_for_job(job_id) + file = "" + for n in notifications: + t = {"content": n.template.content, "subject": n.template.subject} + template = LetterDVLATemplate(t, n.personalisation, 1) + # print(str(template)) + file = file + str(template) + "\n" + s3upload(filedata=file, + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], + file_location="{}-dvla-job.text".format(job_id)) + else: + self.retry(queue="retry", exc="All notifications for job {} are not persisted".format(job_id)) + + +def s3upload(filedata, region, bucket_name, file_location): + # TODO: move this method to utils. Will need to change the filedata from here to send contents in filedata['data'] + _s3 = resource('s3') + # contents = filedata['data'] + contents = filedata + + exists = True + try: + _s3.meta.client.head_bucket( + Bucket=bucket_name) + except botocore.exceptions.ClientError as e: + error_code = int(e.response['Error']['Code']) + if error_code == 404: + exists = False + else: + current_app.logger.error( + "Unable to create s3 bucket {}".format(bucket_name)) + raise e + + if not exists: + _s3.create_bucket(Bucket=bucket_name, + CreateBucketConfiguration={'LocationConstraint': region}) + + upload_id = create_uuid() + upload_file_name = file_location + key = _s3.Object(bucket_name, upload_file_name) + key.put(Body=contents, ServerSideEncryption='AES256') + + return upload_id + + def handle_exception(task, notification, notification_id, exc): if not get_notification_by_id(notification_id): retry_msg = '{task} notification for job {job} row number {row} and notification id {noti}'.format( diff --git a/app/config.py b/app/config.py index 5cb6cf2a4..7019b7a00 100644 --- a/app/config.py +++ b/app/config.py @@ -175,6 +175,8 @@ class Config(object): FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = None FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = None + DVLA_UPLOAD_BUCKET_NAME = "{}-dvla-file-per-job".format(os.getenv('NOTIFY_ENVIRONMENT')) + ###################### # Config overrides ### diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 2a0ef4731..aa4b210a7 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -29,12 +29,12 @@ def dao_get_notification_outcomes_for_job(service_id, job_id): @statsd(namespace="dao") -def are_all_notifications_created_for_job(job_id): - query = db.session.query(func.count(Notification.id))\ +def all_notifications_are_created_for_job(job_id): + query = db.session.query(func.count(Notification.id), Job.id)\ .join(Job)\ .filter(Job.id == job_id)\ .group_by(Job.id)\ - .having(func.count(Notification.id) == Job.notification_count).first() + .having(func.count(Notification.id) == Job.notification_count).all() if query: return True @@ -42,6 +42,11 @@ def are_all_notifications_created_for_job(job_id): return False +@statsd(namespace="dao") +def dao_get_all_notifications_for_job(job_id): + return db.session.query(Notification).filter(Notification.job_id == job_id).all() + + def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).one() diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 34242d180..e792400ed 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,8 +1,9 @@ import uuid from datetime import datetime -from unittest.mock import Mock, ANY, call +from unittest.mock import Mock import pytest +from flask import current_app from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate @@ -11,7 +12,7 @@ from celery.exceptions import Retry from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks -from app.celery.tasks import s3 +from app.celery.tasks import s3, build_dvla_file from app.celery.tasks import ( process_job, process_row, @@ -31,7 +32,7 @@ from tests.app.conftest import ( sample_email_template, sample_notification ) -from tests.app.db import create_user +from tests.app.db import create_user, create_notification, create_job class AnyStringWith(str): @@ -73,7 +74,9 @@ def test_should_process_sms_job(sample_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) mocker.patch('app.celery.tasks.send_sms.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + mocker.patch('app.celery.tasks.build_dvla_file') mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + mocker.patch('app.celery.tasks.build_dvla_file') process_job(sample_job.id) s3.get_job_from_s3.assert_called_once_with( @@ -94,6 +97,7 @@ def test_should_process_sms_job(sample_job, mocker): ) job = jobs_dao.dao_get_job_by_id(sample_job.id) assert job.job_status == 'finished' + tasks.build_dvla_file.assert_not_called() @freeze_time("2016-01-01 11:09:00.061258") @@ -105,6 +109,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) mocker.patch('app.celery.tasks.process_row') + mocker.patch('app.celery.tasks.build_dvla_file') process_job(job.id) @@ -112,6 +117,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False assert tasks.process_row.called is False + tasks.build_dvla_file.assert_not_called() def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify_db, @@ -124,6 +130,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) mocker.patch('app.celery.tasks.process_row') + mocker.patch('app.celery.tasks.build_dvla_file') process_job(job.id) @@ -131,6 +138,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False assert tasks.process_row.called is False + tasks.build_dvla_file.assert_not_called() def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(notify_db, notify_db_session, mocker): @@ -142,6 +150,7 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.process_row') + mocker.patch('app.celery.tasks.build_dvla_file') process_job(job.id) @@ -149,6 +158,7 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False assert tasks.process_row.called is False + tasks.build_dvla_file.assert_not_called() @freeze_time("2016-01-01 11:09:00.061258") @@ -159,6 +169,7 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.process_row') + mocker.patch('app.celery.tasks.build_dvla_file') process_job(job.id) @@ -166,6 +177,7 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False assert tasks.process_row.called is False + tasks.build_dvla_file.assert_not_called() def test_should_not_process_job_if_already_pending(notify_db, notify_db_session, mocker): @@ -173,11 +185,13 @@ def test_should_not_process_job_if_already_pending(notify_db, notify_db_session, mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.process_row') + mocker.patch('app.celery.tasks.build_dvla_file') process_job(job.id) assert s3.get_job_from_s3.called is False assert tasks.process_row.called is False + tasks.build_dvla_file.assert_not_called() @freeze_time("2016-01-01 11:09:00.061258") @@ -269,6 +283,7 @@ def test_should_process_letter_job(sample_letter_job, mocker): mocker.patch('app.celery.tasks.send_email.apply_async') process_row_mock = mocker.patch('app.celery.tasks.process_row') mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + mocker.patch('app.celery.tasks.build_dvla_file') process_job(sample_letter_job.id) @@ -294,6 +309,7 @@ def test_should_process_letter_job(sample_letter_job, mocker): assert process_row_mock.call_count == 1 assert sample_letter_job.job_status == 'finished' + tasks.build_dvla_file.apply_async.assert_called_once_with([str(sample_letter_job.id)], queue="process-job") def test_should_process_all_sms_job(sample_job, @@ -930,6 +946,7 @@ def test_should_cancel_job_if_service_is_inactive(sample_service, mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.process_row') + mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file') process_job(sample_job.id) @@ -937,6 +954,7 @@ def test_should_cancel_job_if_service_is_inactive(sample_service, assert job.job_status == 'cancelled' s3.get_job_from_s3.assert_not_called() tasks.process_row.assert_not_called() + mock_dvla_file_task.assert_not_called() @pytest.mark.parametrize('template_type, expected_class', [ @@ -946,3 +964,35 @@ def test_should_cancel_job_if_service_is_inactive(sample_service, ]) def test_get_template_class(template_type, expected_class): assert get_template_class(template_type) == expected_class + + +def test_build_dvla_file(sample_letter_template, mocker): + job = create_job(template=sample_letter_template, notification_count=2) + create_notification(template=job.template, job=job) + create_notification(template=job.template, job=job) + + mocked = mocker.patch("app.celery.tasks.s3upload") + mocker.patch("app.celery.tasks.LetterDVLATemplate.__str__", return_value="dvla|string") + build_dvla_file(job.id) + + file = "dvla|string\ndvla|string\n" + + assert mocked.called + mocked.assert_called_once_with(filedata=file, + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], + file_location="{}-dvla-job.text".format(job.id)) + + +def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_letter_template, mocker): + job = create_job(template=sample_letter_template, notification_count=2) + create_notification(template=job.template, job=job) + + mocked = mocker.patch("app.celery.tasks.s3upload") + mocker.patch('app.celery.tasks.build_dvla_file.retry', side_effect=Retry) + with pytest.raises(Retry): + build_dvla_file(job.id) + mocked.assert_not_called() + + tasks.build_dvla_file.retry.assert_called_with(queue='retry', + exc="All notifications for job {} are not persisted".format(job.id)) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 45af77bd6..b51480237 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -13,7 +13,8 @@ from app.dao.jobs_dao import ( dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job, dao_get_jobs_older_than, - are_all_notifications_created_for_job) + all_notifications_are_created_for_job, + dao_get_all_notifications_for_job) from app.models import Job from tests.app.conftest import sample_notification as create_notification @@ -316,18 +317,28 @@ def test_get_jobs_for_service_doesnt_return_test_messages(notify_db, notify_db_s assert jobs == [sample_job] -def test_are_all_notifications_created_for_job_returns_true(notify_db, notify_db_session, sample_job): - create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job) - job_is_complete = are_all_notifications_created_for_job(sample_job.id) +def test_all_notifications_are_created_for_job_returns_true(notify_db, notify_db_session): + job = create_job(notify_db=notify_db, notify_db_session=notify_db_session, notification_count=2) + create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=job) + create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=job) + job_is_complete = all_notifications_are_created_for_job(job.id) assert job_is_complete -def test_are_all_notifications_created_for_job_returns_false(notify_db, notify_db_session): +def test_all_notifications_are_created_for_job_returns_false(notify_db, notify_db_session): job = create_job(notify_db=notify_db, notify_db_session=notify_db_session, notification_count=2) - job_is_complete = are_all_notifications_created_for_job(job.id) + job_is_complete = all_notifications_are_created_for_job(job.id) assert not job_is_complete -def test_are_all_notifications_created_for_job_returns_false_when_job_does_not_exist(notify_db, notify_db_session): - job_is_complete = are_all_notifications_created_for_job(uuid.uuid4()) +def test_are_all_notifications_created_for_job_returns_false_when_job_does_not_exist(): + job_is_complete = all_notifications_are_created_for_job(uuid.uuid4()) assert not job_is_complete + + +def test_dao_get_all_notifications_for_job(notify_db, notify_db_session, sample_job): + create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job) + create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job) + create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job) + + assert len(dao_get_all_notifications_for_job(sample_job.id)) == 3 diff --git a/tests/app/db.py b/tests/app/db.py index b0eee5a31..bcd94c9c3 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,7 +1,8 @@ from datetime import datetime import uuid -from app.models import Service, User, Template, Notification, SMS_TYPE, KEY_TYPE_NORMAL +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.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 @@ -105,3 +106,30 @@ def create_notification( notification = Notification(**data) dao_create_notification(notification) return notification + + +def create_job(template, + notification_count=1, + created_at=None, + job_status='pending', + scheduled_for=None, + processing_started=None, + original_file_name='some.csv'): + + data = { + 'id': uuid.uuid4(), + 'service_id': template.service_id, + 'service': template.service, + 'template_id': template.id, + 'template_version': template.version, + 'original_file_name': original_file_name, + 'notification_count': notification_count, + 'created_at': created_at or datetime.utcnow(), + 'created_by': template.created_by, + 'job_status': job_status, + 'scheduled_for': scheduled_for, + 'processing_started': processing_started + } + job = Job(**data) + dao_create_job(job) + return job From 6e7482aaac5c3891bb28a7fc6778a81239b64295 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 15 Mar 2017 15:42:52 +0000 Subject: [PATCH 06/11] Increase max_retries for the task. --- app/celery/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index fe2d37484..ae16f6c5e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -258,7 +258,7 @@ def persist_letter( handle_exception(self, notification, notification_id, e) -@notify_celery.task(bind=True, name="build-dvla-file", max_retries=5, default_retry_delay=300) +@notify_celery.task(bind=True, name="build-dvla-file", max_retries=15, default_retry_delay=300) @statsd(namespace="tasks") def build_dvla_file(self, job_id): if all_notifications_are_created_for_job(job_id): From ea8a3754a04995eb7841562224cd9b737c062a97 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 15 Mar 2017 15:48:28 +0000 Subject: [PATCH 07/11] Updated schema to handle null updated_at --- app/v2/template/template_schemas.py | 2 +- tests/app/v2/template/test_template_schemas.py | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/v2/template/template_schemas.py b/app/v2/template/template_schemas.py index 8356dbd0e..fc727302b 100644 --- a/app/v2/template/template_schemas.py +++ b/app/v2/template/template_schemas.py @@ -29,7 +29,7 @@ get_template_by_id_response = { }, "updated_at": { "format": "date-time", - "type": "string", + "type": ["string", "null"], "description": "Date+time updated" }, "created_by": {"type": "string"}, diff --git a/tests/app/v2/template/test_template_schemas.py b/tests/app/v2/template/test_template_schemas.py index 9e946c2e2..3a89cf5e2 100644 --- a/tests/app/v2/template/test_template_schemas.py +++ b/tests/app/v2/template/test_template_schemas.py @@ -15,7 +15,7 @@ valid_json = { 'id': str(uuid.uuid4()), 'type': 'email', 'created_at': '2017-01-10T18:25:43.511Z', - 'updated_at': '2017-04-23T18:25:43.511Z', + 'updated_at': None, 'version': 1, 'created_by': 'someone@test.com', 'body': "some body" @@ -25,7 +25,7 @@ valid_json_with_optionals = { 'id': str(uuid.uuid4()), 'type': 'email', 'created_at': '2017-01-10T18:25:43.511Z', - 'updated_at': '2017-04-23T18:25:43.511Z', + 'updated_at': None, 'version': 1, 'created_by': 'someone', 'body': "some body", @@ -62,5 +62,9 @@ def test_get_template_request_schema_against_invalid_args_is_invalid(args, error @pytest.mark.parametrize("response", [valid_json, valid_json_with_optionals]) -def test_get_template_response_schema_is_valid(response): +@pytest.mark.parametrize("updated_datetime", [None, '2017-01-11T18:25:43.511Z']) +def test_get_template_response_schema_is_valid(response, updated_datetime): + if updated_datetime: + response['updated_at'] = updated_datetime + assert validate(response, get_template_by_id_response) == response From 13d1016982b9e00890847384a79e4f17fff62931 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 15 Mar 2017 17:07:52 +0000 Subject: [PATCH 08/11] Forgot to make the id unique --- app/celery/tasks.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ae16f6c5e..4488f551d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,3 +1,5 @@ +import random + import botocore from boto3 import resource from datetime import (datetime) @@ -266,7 +268,9 @@ def build_dvla_file(self, job_id): file = "" for n in notifications: t = {"content": n.template.content, "subject": n.template.subject} - template = LetterDVLATemplate(t, n.personalisation, 1) + # This unique id is a 7 digits requested by DVLA, not known if this number needs to be sequential. + unique_id = int(''.join(map(str, random.sample(range(9), 7)))) + template = LetterDVLATemplate(t, n.personalisation, unique_id) # print(str(template)) file = file + str(template) + "\n" s3upload(filedata=file, From 4e486766078a8419cce1d119f505a146e864c0bf Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 15 Mar 2017 17:08:27 +0000 Subject: [PATCH 09/11] Remove comment --- app/celery/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 4488f551d..37c17e0f3 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -271,7 +271,6 @@ def build_dvla_file(self, job_id): # This unique id is a 7 digits requested by DVLA, not known if this number needs to be sequential. unique_id = int(''.join(map(str, random.sample(range(9), 7)))) template = LetterDVLATemplate(t, n.personalisation, unique_id) - # print(str(template)) file = file + str(template) + "\n" s3upload(filedata=file, region=current_app.config['AWS_REGION'], From c032ea5eee36d137cfd36575e82a643e51db292c Mon Sep 17 00:00:00 2001 From: bandesz Date: Thu, 16 Mar 2017 12:33:06 +0000 Subject: [PATCH 10/11] PaaS rollback: check if the rollback app is in a started state --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index b6e72d81c..1d07ff562 100644 --- a/Makefile +++ b/Makefile @@ -295,6 +295,7 @@ cf-check-api-db-migration-task: ## Get the status for the last notify-api-db-mig cf-rollback: ## Rollbacks the app to the previous release $(if ${CF_APP},,$(error Must specify CF_APP)) @cf app --guid ${CF_APP}-rollback || exit 1 + @[ $$(cf curl /v2/apps/`cf app --guid ${CF_APP}-rollback` | jq -r ".entity.state") = "STARTED" ] || (echo "Error: rollback is not possible because ${CF_APP}-rollback is not in a started state" && exit 1) cf delete -f ${CF_APP} || true cf rename ${CF_APP}-rollback ${CF_APP} From d090451dd8a070a3d467ca8aa6809ecb7091011e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 17 Mar 2017 10:01:28 +0000 Subject: [PATCH 11/11] Update code as per review comment. --- app/dao/jobs_dao.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index aa4b210a7..1387bb65f 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -36,10 +36,7 @@ def all_notifications_are_created_for_job(job_id): .group_by(Job.id)\ .having(func.count(Notification.id) == Job.notification_count).all() - if query: - return True - else: - return False + return query @statsd(namespace="dao")