From d38fdb2d113d3c356677b3fe0cfe544553b158e3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 4 Aug 2016 12:35:47 +0100 Subject: [PATCH 01/23] add organisation and branding models a service now has branding and organisation_id columns, and two new tables have been aded to reflect these: * branding is a static types table referring to how a service wants their emails to be branded: * 'govuk' for GOV UK branding (default) * 'org' for organisational branding only * 'both' for co-branded output with both * organisation is a table defining an organisation's branding. this contains three entries, all of which are nullable * colour - a hex code for a coloured bar on the logo's left * logo - relative path for that org's logo image * name - the name to display on the right of the logo --- app/models.py | 2 +- migrations/versions/0046_organisations_and_branding.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index e3aeb3d13..2cb9b709e 100644 --- a/app/models.py +++ b/app/models.py @@ -116,7 +116,7 @@ class Service(db.Model, Versioned): secondary=user_to_service, backref=db.backref('user_to_service', lazy='dynamic')) restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False) - research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=True, default=False) + research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=False) email_from = db.Column(db.Text, index=False, unique=True, nullable=False) created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) diff --git a/migrations/versions/0046_organisations_and_branding.py b/migrations/versions/0046_organisations_and_branding.py index c03faf2ce..fc31bdcd7 100644 --- a/migrations/versions/0046_organisations_and_branding.py +++ b/migrations/versions/0046_organisations_and_branding.py @@ -33,6 +33,7 @@ def upgrade(): op.add_column('services_history', sa.Column('organisation_id', postgresql.UUID(as_uuid=True))) op.execute("INSERT INTO branding_type VALUES ('govuk'), ('org'), ('both')") + # insert UKVI data as initial test data. hex and crest pulled from alphagov/whitehall op.execute("""INSERT INTO organisation VALUES ( '9d25d02d-2915-4e98-874b-974e123e8536', From ebb894729043c5172e6a80e83d08adb1c205c927 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 5 Aug 2016 17:06:06 +0100 Subject: [PATCH 02/23] look at service's organisation for branding to pass through to email renderer --- app/celery/provider_tasks.py | 18 ++++++++- tests/app/celery/test_provider_tasks.py | 50 ++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 0f23ce321..cb164b3ea 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -23,7 +23,7 @@ from app.dao.templates_dao import dao_get_template_by_id from notifications_utils.template import Template, get_sms_fragment_count from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage -from app.models import SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST +from app.models import SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, BRANDING_ORG from app.statsd_decorators import statsd @@ -133,7 +133,7 @@ def send_email_to_provider(self, service_id, notification_id): html_email = Template( template_dict, values=notification.personalisation, - renderer=HTMLEmail() + renderer=get_html_email_renderer(service) ) plain_text_email = Template( @@ -185,3 +185,17 @@ def send_email_to_provider(self, service_id, notification_id): ) delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000 statsd_client.timing("email.total-time", delta_milliseconds) + + +def get_html_email_renderer(service): + govuk_banner = service.branding != BRANDING_ORG + if service.organisation: + branding = { + 'brand_colour': service.organisation.colour, + 'brand_logo': service.organisation.logo, + 'brand_name': service.organisation.name, + } + else: + branding = {} + + return HTMLEmail(govuk_banner=govuk_banner, **branding) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 648c4a33d..ae5146129 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -16,7 +16,16 @@ from app.clients.sms import SmsClientException from app.dao import notifications_dao, provider_details_dao from app.dao import provider_statistics_dao from app.dao.provider_statistics_dao import get_provider_statistics -from app.models import Notification, NotificationStatistics, Job, KEY_TYPE_NORMAL, KEY_TYPE_TEST +from app.models import ( + Notification, + NotificationStatistics, + Job, + Organisation, + KEY_TYPE_NORMAL, + KEY_TYPE_TEST, + BRANDING_ORG, + BRANDING_BOTH +) from tests.app.conftest import sample_notification @@ -503,6 +512,45 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic assert persisted_notification.billable_units == 0 +def test_get_html_email_renderer_should_return_for_normal_service(sample_service): + renderer = provider_tasks.get_html_email_renderer(sample_service) + assert renderer.govuk_banner == True + assert renderer.brand_colour == None + assert renderer.brand_logo == None + assert renderer.brand_name == None + + +@pytest.mark.parametrize('branding_type, govuk_banner', [ + (BRANDING_ORG, False), + (BRANDING_BOTH, True) +]) +def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service): + sample_service.branding = branding_type + org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') + sample_service.organisation = org + notify_db.session.add_all([sample_service, org]) + notify_db.session.commit() + + renderer = provider_tasks.get_html_email_renderer(sample_service) + + assert renderer.govuk_banner == govuk_banner + assert renderer.brand_colour == '#000000' + assert renderer.brand_name == 'Justice League' + + +@pytest.mark.xfail(strict=True) +def test_get_html_email_renderer_prepends_logo_path(branding_type, govuk_banner, notify_db, sample_service): + sample_service.branding = BRANDING_ORG + org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') + sample_service.organisation = org + notify_db.session.add_all([sample_service, org]) + notify_db.session.commit() + + renderer = provider_tasks.get_html_email_renderer(sample_service) + + assert renderer.brand_logo == 'https://localhost:6062/test/assets/images/justice-league.png' + + def _get_provider_statistics(service, **kwargs): query = ProviderStatistics.query.filter_by(service=service) if 'providers' in kwargs: From e88624ed4a91a52b5b30725939ee10fb43b4ad36 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 5 Aug 2016 17:26:40 +0100 Subject: [PATCH 03/23] attempt to pull logos from the admin app's static images directory (this is configured by a config value) --- app/celery/provider_tasks.py | 23 ++++++++++++++--------- config.py | 1 + tests/app/celery/test_provider_tasks.py | 15 +++++++-------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index cb164b3ea..5fd215d8a 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,27 +1,27 @@ from datetime import datetime from monotonic import monotonic +from urllib.parse import urljoin + from flask import current_app +from notifications_utils.recipients import ( + validate_and_format_phone_number +) +from notifications_utils.template import Template, get_sms_fragment_count +from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage + from app import notify_celery, statsd_client, clients, create_uuid from app.clients.email import EmailClientException from app.clients.sms import SmsClientException - from app.dao.notifications_dao import ( update_provider_stats, get_notification_by_id, dao_update_notification, update_notification_status_by_id ) - from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.services_dao import dao_fetch_service_by_id from app.celery.research_mode_tasks import send_sms_response, send_email_response -from notifications_utils.recipients import ( - validate_and_format_phone_number -) - from app.dao.templates_dao import dao_get_template_by_id -from notifications_utils.template import Template, get_sms_fragment_count -from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage from app.models import SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, BRANDING_ORG from app.statsd_decorators import statsd @@ -190,9 +190,14 @@ def send_email_to_provider(self, service_id, notification_id): def get_html_email_renderer(service): govuk_banner = service.branding != BRANDING_ORG if service.organisation: + logo = '{}{}{}'.format( + current_app.config['ADMIN_BASE_URL'], + current_app.config['BRANDING_PATH'], + service.organisation.logo + ) branding = { 'brand_colour': service.organisation.colour, - 'brand_logo': service.organisation.logo, + 'brand_logo': logo, 'brand_name': service.organisation.name, } else: diff --git a/config.py b/config.py index 3ae8441c7..14610f079 100644 --- a/config.py +++ b/config.py @@ -28,6 +28,7 @@ class Config(object): PAGE_SIZE = 50 SMS_CHAR_COUNT_LIMIT = 495 MMG_URL = os.environ['MMG_URL'] + BRANDING_PATH = '/static/images/email-template/crests/' NOTIFY_SERVICE_ID = 'd6aa2c68-a2d9-4437-ab19-3ae8eb202553' INVITATION_EMAIL_TEMPLATE_ID = '4f46df42-f795-4cc4-83bb-65ca312f49cc' diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index ae5146129..11d0839cd 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -514,10 +514,10 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic def test_get_html_email_renderer_should_return_for_normal_service(sample_service): renderer = provider_tasks.get_html_email_renderer(sample_service) - assert renderer.govuk_banner == True - assert renderer.brand_colour == None - assert renderer.brand_logo == None - assert renderer.brand_name == None + assert renderer.govuk_banner + assert renderer.brand_colour is None + assert renderer.brand_logo is None + assert renderer.brand_name is None @pytest.mark.parametrize('branding_type, govuk_banner', [ @@ -534,12 +534,11 @@ def test_get_html_email_renderer_with_branding_details(branding_type, govuk_bann renderer = provider_tasks.get_html_email_renderer(sample_service) assert renderer.govuk_banner == govuk_banner - assert renderer.brand_colour == '#000000' + assert renderer.brand_colour == '000000' assert renderer.brand_name == 'Justice League' -@pytest.mark.xfail(strict=True) -def test_get_html_email_renderer_prepends_logo_path(branding_type, govuk_banner, notify_db, sample_service): +def test_get_html_email_renderer_prepends_logo_path(notify_db, sample_service): sample_service.branding = BRANDING_ORG org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') sample_service.organisation = org @@ -548,7 +547,7 @@ def test_get_html_email_renderer_prepends_logo_path(branding_type, govuk_banner, renderer = provider_tasks.get_html_email_renderer(sample_service) - assert renderer.brand_logo == 'https://localhost:6062/test/assets/images/justice-league.png' + assert renderer.brand_logo == 'http://localhost:6012/static/images/email-template/crests/justice-league.png' def _get_provider_statistics(service, **kwargs): From 8a3a4f7759dfc2aad26586762ccc02f65c060d3c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 8 Aug 2016 14:05:35 +0100 Subject: [PATCH 04/23] ensure s3 bucket parity betwixt admin + api it's no longer just martyn's bucket :innocent: --- config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.py b/config.py index 14610f079..d8067dc53 100644 --- a/config.py +++ b/config.py @@ -112,7 +112,7 @@ class Config(object): class Development(Config): NOTIFY_ENVIRONMENT = 'development' - CSV_UPLOAD_BUCKET_NAME = 'developement-martyn-notifications-csv-upload' + CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' DEBUG = True From 049514d4b2f02f8a5f191bd1c62229721ff20f9d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 2 Aug 2016 16:23:14 +0100 Subject: [PATCH 05/23] remove history-meta for templates, replace with hand-made history table history-meta's dynamic magic is insufficient for templates, where we need to be able to refer to the specific history table to take advantage of sqlalchemy's relationship management (2/3rds of an ORM). So replace it with a custom made version table. Had to change the version decorator slightly for this --- app/dao/dao_utils.py | 13 ++++++++----- app/dao/services_dao.py | 3 ++- app/dao/templates_dao.py | 21 ++++++++++++--------- app/history_meta.py | 14 +++++--------- app/models.py | 22 +++++++++++++++++++++- app/service/rest.py | 2 +- tests/app/dao/test_services_dao.py | 2 +- tests/app/dao/test_templates_dao.py | 14 +++++++------- 8 files changed, 57 insertions(+), 34 deletions(-) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index f5179e8b5..d5ec27fb5 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -1,6 +1,7 @@ import itertools -from functools import wraps -from app.history_meta import versioned_objects, create_history +from functools import wraps, partial + +from app.history_meta import create_history def transactional(func): @@ -19,14 +20,16 @@ def transactional(func): return commit_or_rollback -def version_class(model_class): +def version_class(model_class, history_cls=None): + create_hist = partial(create_history, history_cls=history_cls) + def versioned(func): @wraps(func) def record_version(*args, **kwargs): from app import db func(*args, **kwargs) - history_objects = [create_history(obj) for obj in - versioned_objects(itertools.chain(db.session.new, db.session.dirty)) + history_objects = [create_hist(obj) for obj in + itertools.chain(db.session.new, db.session.dirty) if isinstance(obj, model_class)] for h_obj in history_objects: db.session.add(h_obj) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 5bd2e975a..eff4679dd 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -16,6 +16,7 @@ from app.models import ( VerifyCode, ApiKey, Template, + TemplateHistory, Job, NotificationHistory, Notification, @@ -122,7 +123,7 @@ def delete_service_and_all_associated_db_objects(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(Template.get_history_model().query.filter_by(service_id=service.id)) + _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)) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index d69b530e0..f7f4c5e4d 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -1,8 +1,9 @@ import uuid -from app import db -from app.models import (Template, Service) + from sqlalchemy import (asc, desc) +from app import db +from app.models import (Template, TemplateHistory) from app.dao.dao_utils import ( transactional, version_class @@ -10,7 +11,7 @@ from app.dao.dao_utils import ( @transactional -@version_class(Template) +@version_class(Template, TemplateHistory) def dao_create_template(template): template.id = uuid.uuid4() # must be set now so version history model can use same id template.archived = False @@ -18,14 +19,14 @@ def dao_create_template(template): @transactional -@version_class(Template) +@version_class(Template, TemplateHistory) def dao_update_template(template): db.session.add(template) def dao_get_template_by_id_and_service_id(template_id, service_id, version=None): if version is not None: - return Template.get_history_model().query.filter_by( + return TemplateHistory.query.filter_by( id=template_id, service_id=service_id, version=version).one() @@ -34,7 +35,7 @@ def dao_get_template_by_id_and_service_id(template_id, service_id, version=None) def dao_get_template_by_id(template_id, version=None): if version is not None: - return Template.get_history_model().query.filter_by( + return TemplateHistory.query.filter_by( id=template_id, version=version).one() return Template.query.filter_by(id=template_id).one() @@ -50,6 +51,8 @@ def dao_get_all_templates_for_service(service_id): def dao_get_template_versions(service_id, template_id): - history_model = Template.get_history_model() - return history_model.query.filter_by(service_id=service_id, id=template_id).order_by( - desc(history_model.version)).all() + return TemplateHistory.query.filter_by( + service_id=service_id, id=template_id + ).order_by( + desc(TemplateHistory.version) + ).all() diff --git a/app/history_meta.py b/app/history_meta.py index 9cbbe07f1..49a420572 100644 --- a/app/history_meta.py +++ b/app/history_meta.py @@ -171,17 +171,13 @@ class Versioned(object): return history_mapper.class_ -def versioned_objects(iter): - for obj in iter: - if hasattr(obj, '__history_mapper__'): - yield obj +def create_history(obj, history_cls=None): + if not history_cls: + history_mapper = obj.__history_mapper__ + history_cls = history_mapper.class_ - -def create_history(obj): - obj_mapper = object_mapper(obj) - history_mapper = obj.__history_mapper__ - history_cls = history_mapper.class_ history = history_cls() + obj_mapper = object_mapper(obj) obj_state = attributes.instance_state(obj) data = {} diff --git a/app/models.py b/app/models.py index e3aeb3d13..09fcaf05a 100644 --- a/app/models.py +++ b/app/models.py @@ -206,7 +206,7 @@ TEMPLATE_TYPES = [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE] template_types = db.Enum(*TEMPLATE_TYPES, name='template_type') -class Template(db.Model, Versioned): +class Template(db.Model): __tablename__ = 'templates' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) @@ -231,6 +231,26 @@ class Template(db.Model, Versioned): subject = db.Column(db.Text, index=False, unique=False, nullable=True) created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) created_by = db.relationship('User') + version = db.Column(db.Integer, default=1) + + +class TemplateHistory(db.Model): + __tablename__ = 'templates_history' + + id = db.Column(UUID(as_uuid=True), primary_key=True) + name = db.Column(db.String(255), nullable=False) + template_type = db.Column(template_types, nullable=False) + created_at = db.Column(db.DateTime, nullable=False) + updated_at = db.Column(db.DateTime) + content = db.Column(db.Text, nullable=False) + archived = db.Column(db.Boolean, nullable=False, default=False) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) + service = db.relationship('Service') + subject = db.Column(db.Text) + created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) + created_by = db.relationship('User') + version = db.Column(db.Integer) + MMG_PROVIDER = "mmg" FIRETEXT_PROVIDER = "firetext" diff --git a/app/service/rest.py b/app/service/rest.py index bf84fb859..776ab6400 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -189,7 +189,7 @@ def get_service_history(service_id): api_key_history = ApiKey.get_history_model().query.filter_by(service_id=service_id).all() api_keys_data = api_key_history_schema.dump(api_key_history, many=True).data - template_history = Template.get_history_model().query.filter_by(service_id=service_id).all() + template_history = TemplateHistory.query.filter_by(service_id=service_id).all() template_data, errors = template_history_schema.dump(template_history, many=True) events = Event.query.all() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 1af238b4d..26494dda9 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -331,7 +331,7 @@ def test_delete_service_and_associated_objects(notify_db, assert ApiKey.query.count() == 0 assert ApiKey.get_history_model().query.count() == 0 assert Template.query.count() == 0 - assert Template.get_history_model().query.count() == 0 + assert TemplateHistory.query.count() == 0 assert Job.query.count() == 0 assert Notification.query.count() == 0 assert Permission.query.count() == 0 diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 82b943963..b2a29e1b5 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -185,7 +185,7 @@ def test_get_template_by_id_and_service_returns_none_if_no_template(sample_servi def test_create_template_creates_a_history_record_with_current_data(sample_service, sample_user): assert Template.query.count() == 0 - assert Template.get_history_model().query.count() == 0 + assert TemplateHistory.query.count() == 0 data = { 'name': 'Sample Template', 'template_type': "email", @@ -200,7 +200,7 @@ def test_create_template_creates_a_history_record_with_current_data(sample_servi assert Template.query.count() == 1 template_from_db = Template.query.first() - template_history = Template.get_history_model().query.first() + template_history = TemplateHistory.query.first() assert template_from_db.id == template_history.id assert template_from_db.name == template_history.name @@ -212,7 +212,7 @@ def test_create_template_creates_a_history_record_with_current_data(sample_servi def test_update_template_creates_a_history_record_with_current_data(sample_service, sample_user): assert Template.query.count() == 0 - assert Template.get_history_model().query.count() == 0 + assert TemplateHistory.query.count() == 0 data = { 'name': 'Sample Template', 'template_type': "email", @@ -228,20 +228,20 @@ def test_update_template_creates_a_history_record_with_current_data(sample_servi assert created.name == 'Sample Template' assert Template.query.count() == 1 assert Template.query.first().version == 1 - assert Template.get_history_model().query.count() == 1 + assert TemplateHistory.query.count() == 1 created.name = 'new name' dao_update_template(created) assert Template.query.count() == 1 - assert Template.get_history_model().query.count() == 2 + assert TemplateHistory.query.count() == 2 template_from_db = Template.query.first() assert template_from_db.version == 2 - assert Template.get_history_model().query.filter_by(name='Sample Template').one().version == 1 - assert Template.get_history_model().query.filter_by(name='new name').one().version == 2 + assert TemplateHistory.query.filter_by(name='Sample Template').one().version == 1 + assert TemplateHistory.query.filter_by(name='new name').one().version == 2 def test_get_template_history_version(sample_user, sample_service, sample_template): From c820938ceda49d389254b10773b6dada441fa354 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 8 Aug 2016 16:57:39 +0100 Subject: [PATCH 06/23] fix schema and primary key * version is an additional primary key so we need to indicate that * schema no longer relies on Template model, and uses nested user --- app/models.py | 2 +- app/schemas.py | 12 +++--------- app/service/rest.py | 2 +- tests/app/dao/test_services_dao.py | 1 + tests/app/dao/test_templates_dao.py | 20 ++++++++++++-------- 5 files changed, 18 insertions(+), 19 deletions(-) diff --git a/app/models.py b/app/models.py index 09fcaf05a..cc68e1843 100644 --- a/app/models.py +++ b/app/models.py @@ -249,7 +249,7 @@ class TemplateHistory(db.Model): subject = db.Column(db.Text) created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) created_by = db.relationship('User') - version = db.Column(db.Integer) + version = db.Column(db.Integer, primary_key=True) MMG_PROVIDER = "mmg" diff --git a/app/schemas.py b/app/schemas.py index 168faa02c..9c2454e34 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -172,17 +172,11 @@ class TemplateSchema(BaseTemplateSchema): class TemplateHistorySchema(BaseSchema): - class Meta: - # Use the base model class that the history class is created from - model = models.Template - # We have to use a method here because the relationship field on the - # history object is not created. - created_by = fields.Method("populate_created_by", dump_only=True) + created_by = fields.Nested(UserSchema, only=['id', 'name', 'email_address'], dump_only=True) created_at = field_for(models.Template, 'created_at', format='%Y-%m-%d %H:%M:%S.%f') - def populate_created_by(self, data): - usr = models.User.query.filter_by(id=data.created_by_id).one() - return {'id': str(usr.id), 'name': usr.name, 'email_address': usr.email_address} + class Meta: + model = models.TemplateHistory class NotificationsStatisticsSchema(BaseSchema): diff --git a/app/service/rest.py b/app/service/rest.py index 776ab6400..6f45b6da8 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -176,7 +176,7 @@ def get_service_provider_aggregate_statistics(service_id): # tables. This is so product owner can pass stories as done @service.route('//history', methods=['GET']) def get_service_history(service_id): - from app.models import (Service, ApiKey, Template, Event) + from app.models import (Service, ApiKey, Template, TemplateHistory, Event) from app.schemas import ( service_history_schema, api_key_history_schema, diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 26494dda9..654c3b686 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -29,6 +29,7 @@ from app.models import ( VerifyCode, ApiKey, Template, + TemplateHistory, Job, Notification, NotificationHistory, diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index b2a29e1b5..a01126445 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -6,7 +6,7 @@ from app.dao.templates_dao import ( dao_update_template, dao_get_template_versions) from tests.app.conftest import sample_template as create_sample_template -from app.models import Template +from app.models import Template, TemplateHistory import pytest @@ -261,12 +261,16 @@ def test_get_template_versions(sample_template): sample_template.content = 'new version' dao_update_template(sample_template) versions = dao_get_template_versions(service_id=sample_template.service_id, template_id=sample_template.id) - assert versions.__len__() == 2 - for x in versions: - if x.version == 2: - assert x.content == 'new version' - else: - assert x.content == original_content + assert len(versions) == 2 + versions = sorted(versions, key=lambda x: x.version) + assert versions[0].content == original_content + assert versions[1].content == 'new version' + + assert versions[0].created_at == versions[1].created_at + + assert versions[0].updated_at is None + assert versions[1].updated_at is not None + from app.schemas import template_history_schema v = template_history_schema.load(versions, many=True) - assert v.__len__() == 2 + assert len(v) == 2 From 46c0728b12dd65a0617712d317f09c11e723d809 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 13:07:48 +0100 Subject: [PATCH 07/23] add actual_template relationship to notification also renamed the function to make it apparent that it'll join and grab personalisation --- app/dao/notifications_dao.py | 12 +++++++++--- app/models.py | 11 ++++++++++- app/notifications/rest.py | 11 +++++++---- tests/app/celery/test_provider_tasks.py | 6 +++--- tests/app/dao/test_notification_dao.py | 10 ++++++---- tests/app/notifications/test_rest.py | 15 ++++++++------- 6 files changed, 43 insertions(+), 22 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d25adac3a..b70d1fad7 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -8,6 +8,7 @@ from datetime import ( from flask import current_app from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, Integer, or_, and_, asc) +from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import cast from notifications_utils.template import get_sms_fragment_count @@ -351,12 +352,12 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page @statsd(namespace="dao") -def get_notification(service_id, notification_id, key_type=None): +def get_notification_with_personalisation(service_id, notification_id, key_type): filter_dict = {'service_id': service_id, 'id': notification_id} if key_type: filter_dict['key_type'] = key_type - return Notification.query.filter_by(**filter_dict).one() + return Notification.query.filter_by(**filter_dict).options(joinedload('actual_template')).one() @statsd(namespace="dao") @@ -374,7 +375,8 @@ def get_notifications_for_service(service_id, page=1, page_size=None, limit_days=None, - key_type=None): + key_type=None, + personalisation=False): if page_size is None: page_size = current_app.config['PAGE_SIZE'] filters = [Notification.service_id == service_id] @@ -388,6 +390,10 @@ def get_notifications_for_service(service_id, query = Notification.query.filter(*filters) query = _filter_query(query, filter_dict) + if personalisation: + query.options( + joinedload('actual_template') + ) return query.order_by(desc(Notification.created_at)).paginate( page=page, per_page=page_size diff --git a/app/models.py b/app/models.py index cc68e1843..3d9e12cfa 100644 --- a/app/models.py +++ b/app/models.py @@ -5,7 +5,8 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, text +from sqlalchemy import UniqueConstraint, text, ForeignKeyConstraint, and_ +from sqlalchemy.orm import foreign, remote from app.encryption import ( hashpw, @@ -447,6 +448,14 @@ class Notification(db.Model): reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) + # __table_args__ = ( + # ForeignKeyConstraint(['template_id', 'template_version'], ['template_history.id', 'template_history.version']), + # ) + actual_template = db.relationship('TemplateHistory', primaryjoin=and_( + foreign(template_id) == remote(TemplateHistory.id), + foreign(template_version) == remote(TemplateHistory.version) + )) + @property def personalisation(self): if self._personalisation: diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 5461c198e..b54831105 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,5 +1,6 @@ from datetime import datetime import itertools + from flask import ( Blueprint, jsonify, @@ -7,6 +8,7 @@ from flask import ( current_app, json ) + from notifications_utils.recipients import allowed_to_send_to, first_column_heading from notifications_utils.template import Template from notifications_utils.renderers import PassThrough @@ -170,10 +172,10 @@ def process_firetext_response(): @notifications.route('/notifications/', methods=['GET']) -def get_notifications(notification_id): - notification = notifications_dao.get_notification(str(api_user.service_id), - notification_id, - key_type=api_user.key_type) +def get_notification_by_id(notification_id): + notification = notifications_dao.get_notification_with_personalisation(str(api_user.service_id), + notification_id, + key_type=api_user.key_type) return jsonify(data={"notification": notification_with_personalisation_schema.dump(notification).data}), 200 @@ -186,6 +188,7 @@ def get_all_notifications(): pagination = notifications_dao.get_notifications_for_service( str(api_user.service_id), + personalisation=True, filter_dict=data, page=page, page_size=page_size, diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 648c4a33d..9ef5681b4 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -188,7 +188,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( sender=None ) - persisted_notification = notifications_dao.get_notification(sample_template.service_id, db_notification.id) + persisted_notification = notifications_dao.get_notification_by_id(db_notification.id) assert persisted_notification.to == db_notification.to assert persisted_notification.template_id == sample_template.id assert persisted_notification.template_version == version_on_notification @@ -223,7 +223,7 @@ def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_s ('mmg', str(sample_notification.id), sample_notification.to), queue='research-mode' ) - persisted_notification = notifications_dao.get_notification(sample_service.id, sample_notification.id) + persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) assert persisted_notification.to == sample_notification.to assert persisted_notification.template_id == sample_notification.template_id assert persisted_notification.status == 'sending' @@ -499,7 +499,7 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic sample_notification.id ) - persisted_notification = notifications_dao.get_notification(sample_service.id, sample_notification.id) + persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) assert persisted_notification.billable_units == 0 diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 40c5228a2..cbed327cb 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -23,7 +23,7 @@ from app.models import ( from app.dao.notifications_dao import ( dao_create_notification, dao_update_notification, - get_notification, + get_notification_with_personalisation, get_notification_for_job, get_notifications_for_job, dao_get_notification_statistics_for_service, @@ -57,7 +57,7 @@ def test_should_have_decorated_notifications_dao_functions(): assert update_notification_status_by_reference.__wrapped__.__name__ == 'update_notification_status_by_reference' # noqa assert get_notification_for_job.__wrapped__.__name__ == 'get_notification_for_job' # noqa assert get_notifications_for_job.__wrapped__.__name__ == 'get_notifications_for_job' # noqa - assert get_notification.__wrapped__.__name__ == 'get_notification' # noqa + assert get_notification_with_personalisation.__wrapped__.__name__ == 'get_notification_with_personalisation' # noqa assert get_notifications_for_service.__wrapped__.__name__ == 'get_notifications_for_service' # noqa assert get_notification_by_id.__wrapped__.__name__ == 'get_notification_by_id' # noqa assert delete_notifications_created_more_than_a_week_ago.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago' # noqa @@ -637,9 +637,11 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): def test_get_notification(sample_notification): - notification_from_db = get_notification( + notification_from_db = get_notification_with_personalisation( sample_notification.service.id, - sample_notification.id) + sample_notification.id, + key_type=None + ) assert sample_notification == notification_from_db diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 49813da1b..c5774c6a9 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -640,14 +640,15 @@ def test_get_notification_selects_correct_template_for_personalisation(notify_ap auth_header = create_authorization_header(service_id=sample_template.service_id) response = client.get(path='/notifications', headers=[auth_header]) - assert response.status_code == 200 - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 2 - assert resp['notifications'][0]['template_version'] == 1 - assert resp['notifications'][0]['body'] == 'This is a template' - assert resp['notifications'][1]['template_version'] == 2 - assert resp['notifications'][1]['body'] == 'foo' + assert response.status_code == 200 + + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 2 + assert resp['notifications'][0]['template_version'] == 1 + assert resp['notifications'][0]['body'] == 'This is a template' + assert resp['notifications'][1]['template_version'] == 2 + assert resp['notifications'][1]['body'] == 'foo' def _create_auth_header_from_key(api_key): From 4ba3745159e85e615caca40781b0c2c1e64807e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 16:53:09 +0100 Subject: [PATCH 08/23] update schema to use template_history for accurate template details only in the public notification endpoint so far for fear of breaking things - in an ideal world i'd remove the template relationship from models entirely and replace that with actual_template --- app/dao/notifications_dao.py | 2 +- app/models.py | 3 --- app/schemas.py | 12 +++++++++- tests/app/notifications/test_rest.py | 34 +++++++++++++++++++--------- 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index b70d1fad7..1e85d6cab 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -391,7 +391,7 @@ def get_notifications_for_service(service_id, query = Notification.query.filter(*filters) query = _filter_query(query, filter_dict) if personalisation: - query.options( + query = query.options( joinedload('actual_template') ) return query.order_by(desc(Notification.created_at)).paginate( diff --git a/app/models.py b/app/models.py index 3d9e12cfa..ae52827c4 100644 --- a/app/models.py +++ b/app/models.py @@ -448,9 +448,6 @@ class Notification(db.Model): reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) - # __table_args__ = ( - # ForeignKeyConstraint(['template_id', 'template_version'], ['template_history.id', 'template_history.version']), - # ) actual_template = db.relationship('TemplateHistory', primaryjoin=and_( foreign(template_id) == remote(TemplateHistory.id), foreign(template_version) == remote(TemplateHistory.version) diff --git a/app/schemas.py b/app/schemas.py index 9c2454e34..6de8c5ed4 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -282,12 +282,21 @@ class NotificationWithTemplateSchema(BaseSchema): strict = True exclude = ('_personalisation',) - template = fields.Nested(TemplateSchema, only=["id", "name", "template_type", "content", "subject"], dump_only=True) + template = fields.Nested( + TemplateSchema, + only=['id', 'version', 'name', 'template_type', 'content', 'subject'], + dump_only=True + ) job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) personalisation = fields.Dict(required=False) class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): + template = None + actual_template = fields.Nested(TemplateHistorySchema, + only=['id', 'name', 'template_type', 'content', 'subject', 'version'], + dump_only=True) + @pre_dump def handle_personalisation_property(self, in_data): self.personalisation = in_data.personalisation @@ -295,6 +304,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): @post_dump def handle_template_merge(self, in_data): + in_data['template'] = in_data.pop('actual_template') from notifications_utils.template import Template template = Template( in_data['template'], diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index c5774c6a9..df9277724 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -8,6 +8,7 @@ from freezegun import freeze_time from app.dao.notifications_dao import dao_update_notification from app.dao.api_key_dao import save_model_api_key +from app.dao.templates_dao import dao_update_template from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification @@ -29,7 +30,9 @@ def test_get_sms_notification_by_id(notify_api, sample_notification): assert notification['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, - 'template_type': sample_notification.template.template_type} + 'template_type': sample_notification.template.template_type, + 'version': 1 + } assert notification['job'] == { 'id': str(sample_notification.job.id), 'original_file_name': sample_notification.job.original_file_name @@ -62,7 +65,9 @@ def test_get_email_notification_by_id(notify_api, notify_db, notify_db_session, assert notification['template'] == { 'id': str(email_notification.template.id), 'name': email_notification.template.name, - 'template_type': email_notification.template.template_type} + 'template_type': email_notification.template.template_type, + 'version': 1 + } assert notification['job'] == { 'id': str(email_notification.job.id), 'original_file_name': email_notification.job.original_file_name @@ -166,7 +171,9 @@ def test_get_all_notifications(notify_api, sample_notification): assert notifications['notifications'][0]['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, - 'template_type': sample_notification.template.template_type} + 'template_type': sample_notification.template.template_type, + 'version': 1 + } assert notifications['notifications'][0]['job'] == { 'id': str(sample_notification.job.id), 'original_file_name': sample_notification.job.original_file_name @@ -626,8 +633,9 @@ def test_get_notification_selects_correct_template_for_personalisation(notify_ap notify_db_session, service=sample_template.service, template=sample_template) - + original_content = sample_template.content sample_template.content = '((name))' + dao_update_template(sample_template) notify_db.session.commit() create_sample_notification(notify_db, @@ -641,14 +649,18 @@ def test_get_notification_selects_correct_template_for_personalisation(notify_ap response = client.get(path='/notifications', headers=[auth_header]) - assert response.status_code == 200 + assert response.status_code == 200 - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 2 - assert resp['notifications'][0]['template_version'] == 1 - assert resp['notifications'][0]['body'] == 'This is a template' - assert resp['notifications'][1]['template_version'] == 2 - assert resp['notifications'][1]['body'] == 'foo' + resp = json.loads(response.get_data(as_text=True)) + notis = sorted(resp['notifications'], key=lambda x: x['template_version']) + assert len(notis) == 2 + assert notis[0]['template_version'] == 1 + assert notis[0]['body'] == original_content + assert notis[1]['template_version'] == 2 + assert notis[1]['body'] == 'foo' + + assert notis[0]['template_version'] == notis[0]['template']['version'] + assert notis[1]['template_version'] == notis[1]['template']['version'] def _create_auth_header_from_key(api_key): From 648b4a17c8761a083ff73f5eff684fda950c9cca Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 16:54:43 +0100 Subject: [PATCH 09/23] remove xfail flag - the test passes properly now! :tada: --- tests/app/notifications/test_rest.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index df9277724..3386f1d84 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -623,7 +623,6 @@ def test_get_notifications_for_service_returns_merged_template_content(notify_ap } -@pytest.mark.xfail(strict=True, raises=NeededByTemplateError) def test_get_notification_selects_correct_template_for_personalisation(notify_api, notify_db, notify_db_session, From 5b8fbada11dcf350757ea91eecf920831eda2217 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 10 Aug 2016 08:46:37 +0100 Subject: [PATCH 10/23] Extend the retry time on the create notification DB tasks - Was previoulsy 5 attempts 5 seconds apart - No 5 times 5 minutes apart. - Gives 25 mins of retries before an error --- app/celery/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index bdcb37327..df8c52333 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -126,7 +126,7 @@ def remove_job(job_id): current_app.logger.info("Job {} has been removed from s3.".format(job_id)) -@notify_celery.task(bind=True, name="send-sms", max_retries=5, default_retry_delay=5) +@notify_celery.task(bind=True, name="send-sms", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def send_sms(self, service_id, @@ -158,7 +158,7 @@ def send_sms(self, raise self.retry(queue="retry", exc=e) -@notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=5) +@notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def send_email(self, service_id, notification_id, From 67a4ee7d5165f31429ba4c8c5ec69c11cee6e41b Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 10 Aug 2016 08:53:15 +0100 Subject: [PATCH 11/23] In event of a task retry ensure the log message is identifier as such - "RETRY" prefixes the messages In event of the retry attempts completing without successfully completing the task identify message as such - "RETRY FAILED" prefixes the messages Applies to the send_sms|send_email and send_sms_to_provider|send_email_to_provider tasks These are there to try and ensure we can alert on these events so that we know if we have started retrying messages Retry messages also contain notification ids to aid debugging. --- app/celery/provider_tasks.py | 12 ++++++++++-- app/celery/tasks.py | 20 ++++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 0f23ce321..86b5f5fb7 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -92,11 +92,15 @@ def send_sms_to_provider(self, service_id, notification_id): except SmsClientException as e: try: current_app.logger.error( - "SMS notification {} failed".format(notification_id) + "RETRY: SMS notification {} failed".format(notification_id) ) current_app.logger.exception(e) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification.id), + e + ) update_notification_status_by_id(notification.id, 'technical-failure', 'failure') current_app.logger.info( @@ -173,11 +177,15 @@ def send_email_to_provider(self, service_id, notification_id): except EmailClientException as e: try: current_app.logger.error( - "Email notification {} failed".format(notification_id) + "RETRY: Email notification {} failed".format(notification_id) ) current_app.logger.exception(e) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification.id), + e + ) update_notification_status_by_id(notification.id, 'technical-failure', 'failure') current_app.logger.info( diff --git a/app/celery/tasks.py b/app/celery/tasks.py index df8c52333..9dc332469 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -154,8 +154,14 @@ def send_sms(self, ) except SQLAlchemyError as e: - current_app.logger.exception(e) - raise self.retry(queue="retry", exc=e) + current_app.logger.exception("RETRY: send_sms notification {}".format(notification_id), e) + try: + raise self.retry(queue="retry", exc=e) + except self.MaxRetriesExceededError: + current_app.logger.exception( + "RETRY FAILED: task send_sms failed for notification {}".format(notification.id), + e + ) @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) @@ -180,8 +186,14 @@ def send_email(self, service_id, current_app.logger.info("Email {} created at {}".format(notification_id, created_at)) except SQLAlchemyError as e: - current_app.logger.exception(e) - raise self.retry(queue="retry", exc=e) + current_app.logger.exception("RETRY: send_email notification {}".format(notification_id), e) + try: + raise self.retry(queue="retry", exc=e) + except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task send_email failed for notification {}".format(notification.id), + e + ) def _save_notification(created_at, notification, notification_id, service_id, notification_type, api_key_id, key_type): From 5b3a0f03d342dc3d6811789b8b6829ab996032c4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 10 Aug 2016 16:57:27 +0100 Subject: [PATCH 12/23] rename actual_template to template_history it's slightly less emotionally charged --- app/dao/notifications_dao.py | 4 ++-- app/models.py | 2 +- app/schemas.py | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1e85d6cab..d944a3202 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -357,7 +357,7 @@ def get_notification_with_personalisation(service_id, notification_id, key_type) if key_type: filter_dict['key_type'] = key_type - return Notification.query.filter_by(**filter_dict).options(joinedload('actual_template')).one() + return Notification.query.filter_by(**filter_dict).options(joinedload('template_history')).one() @statsd(namespace="dao") @@ -392,7 +392,7 @@ def get_notifications_for_service(service_id, query = _filter_query(query, filter_dict) if personalisation: query = query.options( - joinedload('actual_template') + joinedload('template_history') ) return query.order_by(desc(Notification.created_at)).paginate( page=page, diff --git a/app/models.py b/app/models.py index ae52827c4..d391022c7 100644 --- a/app/models.py +++ b/app/models.py @@ -448,7 +448,7 @@ class Notification(db.Model): reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) - actual_template = db.relationship('TemplateHistory', primaryjoin=and_( + template_history = db.relationship('TemplateHistory', primaryjoin=and_( foreign(template_id) == remote(TemplateHistory.id), foreign(template_version) == remote(TemplateHistory.version) )) diff --git a/app/schemas.py b/app/schemas.py index 7cad5de42..977c378fc 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -292,9 +292,9 @@ class NotificationWithTemplateSchema(BaseSchema): class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): - actual_template = fields.Nested(TemplateHistorySchema, - only=['id', 'name', 'template_type', 'content', 'subject', 'version'], - dump_only=True) + template_history = fields.Nested(TemplateHistorySchema, + only=['id', 'name', 'template_type', 'content', 'subject', 'version'], + dump_only=True) class Meta(NotificationWithTemplateSchema.Meta): # mark as many fields as possible as required since this is a public api. @@ -307,7 +307,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): # computed fields 'personalisation', # relationships - 'service', 'job', 'api_key', 'actual_template' + 'service', 'job', 'api_key', 'template_history' ) @pre_dump @@ -317,7 +317,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): @post_dump def handle_template_merge(self, in_data): - in_data['template'] = in_data.pop('actual_template') + in_data['template'] = in_data.pop('template_history') from notifications_utils.template import Template template = Template( in_data['template'], From cc7ea8043c5fc05ea00dea49e34ed64ff6096e57 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 12 Aug 2016 11:40:57 +0100 Subject: [PATCH 13/23] add organisation and branding to GET /service response dict --- app/schemas.py | 5 +++-- tests/app/service/test_rest.py | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 168faa02c..7d7ca6388 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -103,6 +103,8 @@ class ProviderDetailsSchema(BaseSchema): class ServiceSchema(BaseSchema): created_by = field_for(models.Service, 'created_by', required=True) + organisation = field_for(models.Service, 'organisation_id', dump_only=True) + branding = field_for(models.Service, 'branding') class Meta: model = models.Service @@ -114,8 +116,7 @@ class ServiceSchema(BaseSchema): 'old_id', 'template_statistics', 'service_provider_stats', - 'service_notification_stats', - 'organisation') + 'service_notification_stats') strict = True @validates('sms_sender') diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 74ae55f5e..661270374 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -108,6 +108,8 @@ def test_get_service_by_id(notify_api, sample_service): assert json_resp['data']['name'] == sample_service.name assert json_resp['data']['id'] == str(sample_service.id) assert not json_resp['data']['research_mode'] + assert json_resp['data']['organisation'] is None + assert json_resp['data']['branding'] == 'govuk' def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db): From 5491668579feb332898e48b977fd52cef960d715 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 15 Aug 2016 10:54:26 +0100 Subject: [PATCH 14/23] let users set organisation on POST /service/{id} --- app/schemas.py | 2 +- tests/app/service/test_rest.py | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 7d7ca6388..1a028645c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -103,7 +103,7 @@ class ProviderDetailsSchema(BaseSchema): class ServiceSchema(BaseSchema): created_by = field_for(models.Service, 'created_by', required=True) - organisation = field_for(models.Service, 'organisation_id', dump_only=True) + organisation = field_for(models.Service, 'organisation') branding = field_for(models.Service, 'branding') class Meta: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 661270374..1d3612b01 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -7,7 +7,7 @@ from freezegun import freeze_time from app.dao.users_dao import save_model_user from app.dao.services_dao import dao_remove_user_from_service -from app.models import User +from app.models import User, Organisation from tests import create_authorization_header from tests.app.conftest import ( sample_service as create_sample_service, @@ -341,7 +341,11 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email assert "Duplicate service name '{}'".format(service_name) in json_resp['message']['name'] -def test_update_service(notify_api, sample_service): +def test_update_service(notify_api, notify_db, sample_service): + org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') + notify_db.session.add(org) + notify_db.session.commit() + with notify_api.test_request_context(): with notify_api.test_client() as client: auth_header = create_authorization_header() @@ -356,7 +360,8 @@ def test_update_service(notify_api, sample_service): data = { 'name': 'updated service name', 'email_from': 'updated.service.name', - 'created_by': str(sample_service.created_by.id) + 'created_by': str(sample_service.created_by.id), + 'organisation': str(org.id) } auth_header = create_authorization_header() @@ -370,6 +375,7 @@ def test_update_service(notify_api, sample_service): assert resp.status_code == 200 assert result['data']['name'] == 'updated service name' assert result['data']['email_from'] == 'updated.service.name' + assert result['data']['organisation'] == str(org.id) def test_update_service_research_mode(notify_api, sample_service): From 989fbb094d1751e50b221e483a6512f747eb618a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 17 Aug 2016 13:18:33 +0100 Subject: [PATCH 15/23] Missing the name on the aws start celery command - means that logs are branded as application:api not delivery. --- aws_run_celery.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_run_celery.py b/aws_run_celery.py index 1165af098..e1db882a2 100644 --- a/aws_run_celery.py +++ b/aws_run_celery.py @@ -6,5 +6,5 @@ import os # on aws get secrets and export to env os.environ.update(getAllSecrets(region="eu-west-1")) -application = create_app() +application = create_app("delivery") application.app_context().push() From f74000f548cb40cde40fc3246ddd474209e5ba35 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 12:06:00 +0100 Subject: [PATCH 16/23] Add query to get template usage from the Notifications History table - groups by template Id and Day. Returns count per day, template name, template id, template type, and day. Ordered by day (desc) and template name (acc) --- app/dao/notifications_dao.py | 23 +++- tests/app/dao/test_notification_dao.py | 144 ++++++++++++++++++++++++- 2 files changed, 164 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d25adac3a..d90427ca6 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -9,7 +9,6 @@ from flask import current_app from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, Integer, or_, and_, asc) from sqlalchemy.sql.expression import cast -from notifications_utils.template import get_sms_fragment_count from app import db from app.dao import days_ago @@ -135,6 +134,28 @@ def dao_get_7_day_agg_notification_statistics_for_service(service_id, ) +@statsd(namespace="dao") +def dao_get_template_usage(service_id, limit_days=None): + + query = db.session.query( + func.count(NotificationHistory.template_id).label('count'), + NotificationHistory.template_id, + func.DATE(NotificationHistory.created_at).label('day'), + Template.name, + Template.template_type + ) + + query_filter = [NotificationHistory.service_id == service_id] + if limit_days is not None: + query_filter.append(NotificationHistory.created_at >= days_ago(limit_days)) + + return query.filter(*query_filter) \ + .join(Template)\ + .group_by(NotificationHistory.template_id, func.DATE(NotificationHistory.created_at), Template.name, Template.template_type)\ + .order_by(desc(func.DATE(NotificationHistory.created_at)), asc(Template.name))\ + .all() # noqa + + @statsd(namespace="dao") def dao_get_template_statistics_for_service(service_id, limit_days=None): query_filter = [TemplateStatistics.service_id == service_id] diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 40c5228a2..8affde42b 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -35,15 +35,16 @@ from app.dao.notifications_dao import ( dao_get_template_statistics_for_service, get_notifications_for_service, dao_get_7_day_agg_notification_statistics_for_service, dao_get_potential_notification_statistics_for_day, dao_get_notification_statistics_for_day, - dao_get_template_statistics_for_template, get_notification_by_id) + dao_get_template_statistics_for_template, get_notification_by_id, dao_get_template_usage) from notifications_utils.template import get_sms_fragment_count -from tests.app.conftest import (sample_notification) +from tests.app.conftest import (sample_notification, sample_template, sample_email_template, sample_service) def test_should_have_decorated_notifications_dao_functions(): assert dao_get_notification_statistics_for_service.__wrapped__.__name__ == 'dao_get_notification_statistics_for_service' # noqa + assert dao_get_template_usage.__wrapped__.__name__ == 'dao_get_template_usage' # noqa assert dao_get_notification_statistics_for_service_and_day.__wrapped__.__name__ == 'dao_get_notification_statistics_for_service_and_day' # noqa assert dao_get_notification_statistics_for_day.__wrapped__.__name__ == 'dao_get_notification_statistics_for_day' # noqa assert dao_get_potential_notification_statistics_for_day.__wrapped__.__name__ == 'dao_get_potential_notification_statistics_for_day' # noqa @@ -63,6 +64,145 @@ def test_should_have_decorated_notifications_dao_functions(): assert delete_notifications_created_more_than_a_week_ago.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago' # noqa +def test_should_by_able_to_get_template_count_from_notifications_history(notify_db, notify_db_session, sample_service): + sms = sample_template(notify_db, notify_db_session) + email = sample_email_template(notify_db, notify_db_session) + sample_notification(notify_db, notify_db_session, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, service=sample_service, template=email) + + results = dao_get_template_usage(sample_service.id) + assert results[0].name == 'Email Template Name' + assert results[0].template_type == 'email' + assert results[0].count == 2 + + assert results[1].name == 'Template Name' + assert results[1].template_type == 'sms' + assert results[1].count == 3 + + +def test_should_by_able_to_get_template_count_from_notifications_history_for_service( + notify_db, + notify_db_session): + service_1 = sample_service(notify_db, notify_db_session, service_name="test1", email_from="test1") + service_2 = sample_service(notify_db, notify_db_session, service_name="test2", email_from="test2") + service_3 = sample_service(notify_db, notify_db_session, service_name="test3", email_from="test3") + + sms = sample_template(notify_db, notify_db_session) + + sample_notification(notify_db, notify_db_session, service=service_1, template=sms) + sample_notification(notify_db, notify_db_session, service=service_1, template=sms) + sample_notification(notify_db, notify_db_session, service=service_2, template=sms) + + assert dao_get_template_usage(service_1.id)[0].count == 2 + assert dao_get_template_usage(service_2.id)[0].count == 1 + assert len(dao_get_template_usage(service_3.id)) == 0 + + +def test_should_by_able_to_get_zero_count_from_notifications_history_if_no_rows(sample_service): + results = dao_get_template_usage(sample_service.id) + assert len(results) == 0 + + +def test_should_by_able_to_get_zero_count_from_notifications_history_if_no_service(): + results = dao_get_template_usage(str(uuid.uuid4())) + assert len(results) == 0 + + +def test_should_by_able_to_get_template_count_from_notifications_history_across_days( + notify_db, + notify_db_session, + sample_service): + sms = sample_template(notify_db, notify_db_session) + email = sample_email_template(notify_db, notify_db_session) + + today = datetime.now() + yesterday = datetime.now() - timedelta(days=1) + one_month_ago = datetime.now() - timedelta(days=30) + + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + + results = dao_get_template_usage(sample_service.id) + + assert len(results) == 5 + + assert [(row.name, row.template_type, row.count, row.day) for row in results] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), + ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + ] + + +def test_should_by_able_to_get_template_count_from_notifications_history_with_day_limit( + notify_db, + notify_db_session, + sample_service): + sms = sample_template(notify_db, notify_db_session) + + email = sample_email_template(notify_db, notify_db_session) + + today = datetime.now() + yesterday = datetime.now() - timedelta(days=1) + one_month_ago = datetime.now() - timedelta(days=30) + + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + + results_day_one = dao_get_template_usage(sample_service.id, limit_days=0) + assert len(results_day_one) == 2 + + results_day_two = dao_get_template_usage(sample_service.id, limit_days=1) + assert len(results_day_two) == 4 + + results_day_30 = dao_get_template_usage(sample_service.id, limit_days=31) + assert len(results_day_30) == 5 + + assert [(row.name, row.template_type, row.count, row.day) for row in results_day_one] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()) + ] + + assert [(row.name, row.template_type, row.count, row.day) for row in results_day_two] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), + ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()) + ] + + assert [(row.name, row.template_type, row.count, row.day) for row in results_day_30] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), + ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + ] + + def test_should_by_able_to_update_status_by_reference(sample_email_template, ses_provider): data = _notification_json(sample_email_template, status='sending') From 7a5acea71bba7872dfd9be047372a627e2968558 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 14:01:31 +0100 Subject: [PATCH 17/23] New endpoint for template stats to use new query. Breaking change to the returned JSON, so adding on a different url. --- app/template_statistics/rest.py | 26 ++++ tests/app/template_statistics/test_rest.py | 134 ++++++++++++++++++++- 2 files changed, 158 insertions(+), 2 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index d4990ba04..0bed6329e 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -5,6 +5,7 @@ from flask import ( ) from app.dao.notifications_dao import ( + dao_get_template_usage, dao_get_template_statistics_for_service, dao_get_template_statistics_for_template ) @@ -36,6 +37,31 @@ def get_template_statistics_for_service(service_id): return jsonify(data=data) +@template_statistics.route('/replacement') +def get_template_statistics_for_service_by_day(service_id): + if request.args.get('limit_days'): + try: + limit_days = int(request.args['limit_days']) + except ValueError as e: + error = '{} is not an integer'.format(request.args['limit_days']) + message = {'limit_days': [error]} + raise InvalidRequest(message, status_code=400) + else: + limit_days = None + stats = dao_get_template_usage(service_id, limit_days=limit_days) + + def serialize(row): + return { + 'count': row.count, + 'day': str(row.day), + 'template_id': str(row.template_id), + 'template_name': row.name, + 'template_type': row.template_type + } + + return jsonify(data=[serialize(row) for row in stats]) + + @template_statistics.route('/') def get_template_statistics_for_template_id(service_id, template_id): stats = dao_get_template_statistics_for_template(template_id) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 4d7b3384a..70191d086 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, timedelta import json from freezegun import freeze_time @@ -6,7 +6,137 @@ from freezegun import freeze_time from app import db from app.models import TemplateStatistics from tests import create_authorization_header -from tests.app.conftest import sample_template as create_sample_template +from tests.app.conftest import sample_template as create_sample_template, sample_template, sample_notification, \ + sample_email_template + + +def test_get_all_template_statistics_with_bad_arg_returns_400(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 'blurk'} + ) + + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} + + +@freeze_time('2016-08-18') +def test_get_template_statistics_for_service(notify_db, notify_db_session, notify_api, sample_service): + sms = sample_template(notify_db, notify_db_session, service=sample_service) + email = sample_email_template(notify_db, notify_db_session, service=sample_service) + today = datetime.now() + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 2 + assert json_resp['data'][0]['template_id'] == str(email.id) + assert json_resp['data'][0]['template_name'] == email.name + assert json_resp['data'][0]['template_type'] == email.template_type + assert json_resp['data'][0]['day'] == '2016-08-18' + assert json_resp['data'][1]['count'] == 2 + assert json_resp['data'][1]['template_id'] == str(sms.id) + assert json_resp['data'][1]['template_name'] == sms.name + assert json_resp['data'][1]['template_type'] == sms.template_type + assert json_resp['data'][1]['day'] == '2016-08-18' + + +@freeze_time('2016-08-18') +def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session, notify_api, sample_service): + sms = sample_template(notify_db, notify_db_session, service=sample_service) + email = sample_email_template(notify_db, notify_db_session, service=sample_service) + today = datetime.now() + a_week_ago = datetime.now() - timedelta(days=7) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 1} + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 1 + assert json_resp['data'][0]['template_id'] == str(email.id) + assert json_resp['data'][0]['template_name'] == email.name + assert json_resp['data'][0]['template_type'] == email.template_type + assert json_resp['data'][0]['day'] == '2016-08-18' + assert json_resp['data'][1]['count'] == 1 + assert json_resp['data'][1]['template_id'] == str(sms.id) + assert json_resp['data'][1]['template_name'] == sms.name + assert json_resp['data'][1]['template_type'] == sms.template_type + assert json_resp['data'][1]['day'] == '2016-08-18' + + response_for_a_week = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 7} + ) + + assert response.status_code == 200 + json_resp = json.loads(response_for_a_week.get_data(as_text=True)) + assert len(json_resp['data']) == 4 + assert json_resp['data'][0]['count'] == 1 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][0]['day'] == '2016-08-18' + assert json_resp['data'][1]['count'] == 1 + assert json_resp['data'][1]['template_name'] == 'Template Name' + assert json_resp['data'][1]['day'] == '2016-08-18' + assert json_resp['data'][2]['count'] == 1 + assert json_resp['data'][2]['template_name'] == 'Email Template Name' + assert json_resp['data'][2]['day'] == '2016-08-11' + assert json_resp['data'][3]['count'] == 1 + assert json_resp['data'][3]['template_name'] == 'Template Name' + assert json_resp['data'][3]['day'] == '2016-08-11' + + +@freeze_time('2016-08-18') +def test_returns_empty_list_if_no_templates_used(notify_api, sample_service): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 0 @freeze_time('2016-04-09') From ede7d0cbeaec36016451b599e332d313315b5c63 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 14:06:12 +0100 Subject: [PATCH 18/23] Removed old endpoint. Going to handle the migration in the clients. --- app/template_statistics/rest.py | 16 --- tests/app/template_statistics/test_rest.py | 137 +-------------------- 2 files changed, 6 insertions(+), 147 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 0bed6329e..3b25b4340 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -22,22 +22,6 @@ register_errors(template_statistics) @template_statistics.route('') -def get_template_statistics_for_service(service_id): - if request.args.get('limit_days'): - try: - limit_days = int(request.args['limit_days']) - except ValueError as e: - error = '{} is not an integer'.format(request.args['limit_days']) - message = {'limit_days': [error]} - raise InvalidRequest(message, status_code=400) - else: - limit_days = None - stats = dao_get_template_statistics_for_service(service_id, limit_days=limit_days) - data = template_statistics_schema.dump(stats, many=True).data - return jsonify(data=data) - - -@template_statistics.route('/replacement') def get_template_statistics_for_service_by_day(service_id): if request.args.get('limit_days'): try: diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 70191d086..65af24166 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -10,14 +10,14 @@ from tests.app.conftest import sample_template as create_sample_template, sample sample_email_template -def test_get_all_template_statistics_with_bad_arg_returns_400(notify_api): +def test_get_all_template_statistics_with_bad_arg_returns_400(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_template.service_id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header], query_string={'limit_days': 'blurk'} ) @@ -44,7 +44,7 @@ def test_get_template_statistics_for_service(notify_db, notify_db_session, notif auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header] ) @@ -80,7 +80,7 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header], query_string={'limit_days': 1} ) @@ -100,7 +100,7 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session assert json_resp['data'][1]['day'] == '2016-08-18' response_for_a_week = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header], query_string={'limit_days': 7} ) @@ -130,7 +130,7 @@ def test_returns_empty_list_if_no_templates_used(notify_api, sample_service): auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header] ) @@ -139,131 +139,6 @@ def test_returns_empty_list_if_no_templates_used(notify_api, sample_service): assert len(json_resp['data']) == 0 -@freeze_time('2016-04-09') -def test_get_template_statistics_for_service_for_last_week(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 7} - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 8 - assert json_resp['data'][0]['day'] == '2016-04-09' - assert json_resp['data'][6]['day'] == '2016-04-03' - - -@freeze_time('2016-04-30') -def test_get_template_statistics_for_service_for_last_week_with_no_data(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - # Date is frozen at 2016-04-30 and no data written since - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 7} - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 0 - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 30} - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 9 - - -def test_get_all_template_statistics_for_service(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header] - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 9 - assert json_resp['data'][0]['day'] == '2016-04-09' - assert json_resp['data'][8]['day'] == '2016-04-01' - - -def test_get_all_template_statistics_with_bad_limit_arg_returns_400(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 'blurk'} - ) - - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['result'] == 'error' - assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} - - def test_get_template_statistics_for_template_only_returns_for_provided_template( notify_db, notify_db_session, From 9eb559d4b236be5c57927dee4bebed9d47e39a51 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 15:29:56 +0100 Subject: [PATCH 19/23] Renamed the param to avoid shadowing --- app/template_statistics/rest.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 3b25b4340..c044eb70e 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -34,13 +34,13 @@ def get_template_statistics_for_service_by_day(service_id): limit_days = None stats = dao_get_template_usage(service_id, limit_days=limit_days) - def serialize(row): + def serialize(data): return { - 'count': row.count, - 'day': str(row.day), - 'template_id': str(row.template_id), - 'template_name': row.name, - 'template_type': row.template_type + 'count': data.count, + 'day': str(data.day), + 'template_id': str(data.template_id), + 'template_name': data.name, + 'template_type': data.template_type } return jsonify(data=[serialize(row) for row in stats]) From b7476a197512af3ea1510f17f6c43d5def273d6c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Aug 2016 10:38:44 +0100 Subject: [PATCH 20/23] Removed the group by day aspects of template stats. Not needed. Grouped by template only. --- app/dao/notifications_dao.py | 20 +++++---- app/template_statistics/rest.py | 1 - tests/app/dao/test_notification_dao.py | 38 +++++++--------- tests/app/template_statistics/test_rest.py | 52 +++++++++++++++------- 4 files changed, 62 insertions(+), 49 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d90427ca6..16301e64f 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -137,23 +137,27 @@ def dao_get_7_day_agg_notification_statistics_for_service(service_id, @statsd(namespace="dao") def dao_get_template_usage(service_id, limit_days=None): + table = NotificationHistory + + if limit_days and limit_days <= 7: # can get this data from notifications table + table = Notification + query = db.session.query( - func.count(NotificationHistory.template_id).label('count'), - NotificationHistory.template_id, - func.DATE(NotificationHistory.created_at).label('day'), + func.count(table.template_id).label('count'), + table.template_id, Template.name, Template.template_type ) - query_filter = [NotificationHistory.service_id == service_id] + query_filter = [table.service_id == service_id] if limit_days is not None: - query_filter.append(NotificationHistory.created_at >= days_ago(limit_days)) + query_filter.append(table.created_at >= days_ago(limit_days)) return query.filter(*query_filter) \ .join(Template)\ - .group_by(NotificationHistory.template_id, func.DATE(NotificationHistory.created_at), Template.name, Template.template_type)\ - .order_by(desc(func.DATE(NotificationHistory.created_at)), asc(Template.name))\ - .all() # noqa + .group_by(table.template_id, Template.name, Template.template_type)\ + .order_by(asc(Template.name))\ + .all() @statsd(namespace="dao") diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index c044eb70e..191645b96 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -37,7 +37,6 @@ def get_template_statistics_for_service_by_day(service_id): def serialize(data): return { 'count': data.count, - 'day': str(data.day), 'template_id': str(data.template_id), 'template_name': data.name, 'template_type': data.template_type diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 8affde42b..b75ec5ccf 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -137,14 +137,11 @@ def test_should_by_able_to_get_template_count_from_notifications_history_across_ results = dao_get_template_usage(sample_service.id) - assert len(results) == 5 + assert len(results) == 2 - assert [(row.name, row.template_type, row.count, row.day) for row in results] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), - ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + assert [(row.name, row.template_type, row.count) for row in results] == [ + ('Email Template Name', 'email', 5), + ('Template Name', 'sms', 5) ] @@ -177,29 +174,24 @@ def test_should_by_able_to_get_template_count_from_notifications_history_with_da assert len(results_day_one) == 2 results_day_two = dao_get_template_usage(sample_service.id, limit_days=1) - assert len(results_day_two) == 4 + assert len(results_day_two) == 2 results_day_30 = dao_get_template_usage(sample_service.id, limit_days=31) - assert len(results_day_30) == 5 + assert len(results_day_30) == 2 - assert [(row.name, row.template_type, row.count, row.day) for row in results_day_one] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()) + assert [(row.name, row.template_type, row.count) for row in results_day_one] == [ + ('Email Template Name', 'email', 2), + ('Template Name', 'sms', 1) ] - assert [(row.name, row.template_type, row.count, row.day) for row in results_day_two] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), - ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()) + assert [(row.name, row.template_type, row.count) for row in results_day_two] == [ + ('Email Template Name', 'email', 5), + ('Template Name', 'sms', 2), ] - assert [(row.name, row.template_type, row.count, row.day) for row in results_day_30] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), - ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + assert [(row.name, row.template_type, row.count) for row in results_day_30] == [ + ('Email Template Name', 'email', 5), + ('Template Name', 'sms', 5), ] diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 65af24166..1544874f7 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -55,24 +55,25 @@ def test_get_template_statistics_for_service(notify_db, notify_db_session, notif assert json_resp['data'][0]['template_id'] == str(email.id) assert json_resp['data'][0]['template_name'] == email.name assert json_resp['data'][0]['template_type'] == email.template_type - assert json_resp['data'][0]['day'] == '2016-08-18' assert json_resp['data'][1]['count'] == 2 assert json_resp['data'][1]['template_id'] == str(sms.id) assert json_resp['data'][1]['template_name'] == sms.name assert json_resp['data'][1]['template_type'] == sms.template_type - assert json_resp['data'][1]['day'] == '2016-08-18' @freeze_time('2016-08-18') -def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session, notify_api, sample_service): +def test_get_template_statistics_for_service_limited_by_day(notify_db, notify_db_session, notify_api, sample_service): sms = sample_template(notify_db, notify_db_session, service=sample_service) email = sample_email_template(notify_db, notify_db_session, service=sample_service) today = datetime.now() a_week_ago = datetime.now() - timedelta(days=7) + a_month_ago = datetime.now() - timedelta(days=30) sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=sms) sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=email) - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=a_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_month_ago, service=sample_service, template=email) with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -92,12 +93,10 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session assert json_resp['data'][0]['template_id'] == str(email.id) assert json_resp['data'][0]['template_name'] == email.name assert json_resp['data'][0]['template_type'] == email.template_type - assert json_resp['data'][0]['day'] == '2016-08-18' assert json_resp['data'][1]['count'] == 1 assert json_resp['data'][1]['template_id'] == str(sms.id) assert json_resp['data'][1]['template_name'] == sms.name assert json_resp['data'][1]['template_type'] == sms.template_type - assert json_resp['data'][1]['day'] == '2016-08-18' response_for_a_week = client.get( '/service/{}/template-statistics'.format(sample_service.id), @@ -107,19 +106,38 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session assert response.status_code == 200 json_resp = json.loads(response_for_a_week.get_data(as_text=True)) - assert len(json_resp['data']) == 4 - assert json_resp['data'][0]['count'] == 1 + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 2 assert json_resp['data'][0]['template_name'] == 'Email Template Name' - assert json_resp['data'][0]['day'] == '2016-08-18' - assert json_resp['data'][1]['count'] == 1 + assert json_resp['data'][1]['count'] == 2 + assert json_resp['data'][1]['template_name'] == 'Template Name' + + response_for_a_month = client.get( + '/service/{}/template-statistics'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 30} + ) + + assert response_for_a_month.status_code == 200 + json_resp = json.loads(response_for_a_month.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 3 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][1]['count'] == 3 + assert json_resp['data'][1]['template_name'] == 'Template Name' + + response_for_all = client.get( + '/service/{}/template-statistics'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response_for_all.status_code == 200 + json_resp = json.loads(response_for_all.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 3 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][1]['count'] == 3 assert json_resp['data'][1]['template_name'] == 'Template Name' - assert json_resp['data'][1]['day'] == '2016-08-18' - assert json_resp['data'][2]['count'] == 1 - assert json_resp['data'][2]['template_name'] == 'Email Template Name' - assert json_resp['data'][2]['day'] == '2016-08-11' - assert json_resp['data'][3]['count'] == 1 - assert json_resp['data'][3]['template_name'] == 'Template Name' - assert json_resp['data'][3]['day'] == '2016-08-11' @freeze_time('2016-08-18') From d5ec73bc8a4489508c03bee373dc884557a0fdf4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 22 Aug 2016 13:38:41 +0100 Subject: [PATCH 21/23] update utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 7e4ac3885..d2c5d8ce4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,4 +23,4 @@ statsd==3.2.1 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@8.7.5#egg=notifications-utils==8.7.5 +git+https://github.com/alphagov/notifications-utils.git@8.7.5#egg=notifications-utils==8.7.6 From 0a052d45a8928ae7df3678475c827157512ccfda Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 22 Aug 2016 14:40:02 +0100 Subject: [PATCH 22/23] actually bump version missed a number last time :anguished: --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index d2c5d8ce4..904bd6e3e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,4 +23,4 @@ statsd==3.2.1 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@8.7.5#egg=notifications-utils==8.7.6 +git+https://github.com/alphagov/notifications-utils.git@8.7.6#egg=notifications-utils==8.7.6 From 68c113537956aebd025ce6edc62a6c4243c88125 Mon Sep 17 00:00:00 2001 From: bandesz Date: Tue, 16 Aug 2016 17:51:52 +0100 Subject: [PATCH 23/23] Create Docker build image, build project with Docker --- .gitignore | 6 +- .travis.yml | 2 +- Makefile | 144 ++++++++++++++++++++++++++++ app/{version.py => version.py.dist} | 3 +- deploy-exclude.lst | 9 ++ docker/Dockerfile-build | 25 +++++ docker/Makefile | 10 ++ scripts/common_functions.sh | 14 +-- scripts/register_with_elb.sh | 2 +- scripts/run_tests.sh | 3 + scripts/update_version_file.sh | 7 -- 11 files changed, 205 insertions(+), 20 deletions(-) create mode 100644 Makefile rename app/{version.py => version.py.dist} (56%) create mode 100644 deploy-exclude.lst create mode 100644 docker/Dockerfile-build create mode 100644 docker/Makefile delete mode 100755 scripts/update_version_file.sh diff --git a/.gitignore b/.gitignore index 1c6c16612..8be1355da 100644 --- a/.gitignore +++ b/.gitignore @@ -24,6 +24,7 @@ var/ *.egg-info/ .installed.cfg *.egg +/cache # PyInstaller # Usually these files are written by a python script from a template @@ -62,5 +63,8 @@ target/ # Mac *.DS_Store environment.sh +.envrc -celerybeat-schedule \ No newline at end of file +celerybeat-schedule + +app/version.py diff --git a/.travis.yml b/.travis.yml index 0d29a03cf..f7d9a4b66 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,6 +14,7 @@ install: before_script: - psql -c 'create database test_notification_api;' -U postgres script: +- make generate-version-file - ./scripts/run_tests.sh env: secure: tgeumbN2oayp1twu3iVuuo5eMz/XVO303A2wdnR6bFMCfLCA7SVeKvDZ21ZpSh+J7tu8/9MQD2ATo95qyO9oraSg09BQ7UoEtpyrrcP21UBcNMbIsAdmOUAostlvg4hy1ZuSjytpzLDMZfS0QCjWPtZiXKW3XzmHHJyIdatcHsO3Jpi1vPRP11cZHd1SKwd1POYXDuX3Y9e68yt0P7Wr1/3mZ8u0XHtSg++SnZ0qDDwnWIsHqkcxr7R/n1MYvyUD8XPB+guqEq/7G6ipR+QrHN0fCVGXFksXGLSMSBg9sGQ1Mr+2yiOXL+4EmCfpx9VofmEOFDTdK70lFFn1sOG/GmceW4JT2Y2vLG+6vSJTmaHxeZmpYoKRa1EJJqyEpvjRM3A8lV993qIdAEBIE8s0w+DhkmXXCI3chSDT+2B/SlFbGw7G7E4hto/3FUrk7N7w+c5WaOQSqz4ZxTX4iIg9T7Bxo3s8l+UYYw4NfzEreRieEiFo58FgYrghEOvMp9PZ3tN3u2H+2yISE0C/+MSFUB2CWgFiTTD2XtWuQJgGNxyTYD1sbHaYT1EeDoz8JbhsACvIxpQdycVibHjP4hvP32nFFaznNpCm1ArS+vDtzR6Psx2vYb/u0rf1QoipVE/GPzqB9bwGHZ/0Cpsqy4KxYM74MOu3Gi3KCYzKGq7hRGI= @@ -160,7 +161,6 @@ deploy: wait-until-deployed: true on: *2 before_deploy: - - ./scripts/update_version_file.sh - zip -r --exclude=*__pycache__* notifications-api * - mkdir -p dpl_cd_upload - mv notifications-api.zip dpl_cd_upload/notifications-api-$TRAVIS_BRANCH-$TRAVIS_BUILD_NUMBER-$TRAVIS_COMMIT.zip diff --git a/Makefile b/Makefile new file mode 100644 index 000000000..ff8aea5e4 --- /dev/null +++ b/Makefile @@ -0,0 +1,144 @@ +.DEFAULT_GOAL := help +SHELL := /bin/bash +DATE = $(shell date +%Y-%m-%d:%H:%M:%S) + +PIP_ACCEL_CACHE ?= ${CURDIR}/cache/pip-accel +APP_VERSION_FILE = app/version.py + +GIT_BRANCH = $(shell git symbolic-ref --short HEAD 2> /dev/null || echo "detached") +GIT_COMMIT ?= $(shell git rev-parse HEAD) + +DOCKER_BUILDER_IMAGE_NAME = govuk/notify-api-builder + +BUILD_TAG ?= notifications-api-manual +BUILD_NUMBER ?= 0 +DEPLOY_BUILD_NUMBER ?= ${BUILD_NUMBER} +BUILD_URL ?= + +DOCKER_CONTAINER_PREFIX = ${USER}-${BUILD_TAG} + +.PHONY: help +help: + @cat $(MAKEFILE_LIST) | grep -E '^[a-zA-Z_-]+:.*?## .*$$' | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' + +.PHONY: venv +venv: venv/bin/activate ## Create virtualenv if it does not exist + +venv/bin/activate: + test -d venv || virtualenv venv + ./venv/bin/pip install pip-accel + +.PHONY: check-env-vars +check-env-vars: ## Check mandatory environment variables + $(if ${DEPLOY_ENV},,$(error Must specify DEPLOY_ENV)) + $(if ${DNS_NAME},,$(error Must specify DNS_NAME)) + $(if ${AWS_ACCESS_KEY_ID},,$(error Must specify AWS_ACCESS_KEY_ID)) + $(if ${AWS_SECRET_ACCESS_KEY},,$(error Must specify AWS_SECRET_ACCESS_KEY)) + +.PHONY: development +development: ## Set environment to development + $(eval export DEPLOY_ENV=development) + $(eval export DNS_NAME="notify.works") + @true + +.PHONY: staging +staging: ## Set environment to staging + $(eval export DEPLOY_ENV=staging) + $(eval export DNS_NAME="staging-notify.works") + @true + +.PHONY: production +production: ## Set environment to production + $(eval export DEPLOY_ENV=production) + $(eval export DNS_NAME="notifications.service.gov.uk") + @true + +.PHONY: dependencies +dependencies: venv ## Install build dependencies + mkdir -p ${PIP_ACCEL_CACHE} + PIP_ACCEL_CACHE=${PIP_ACCEL_CACHE} ./venv/bin/pip-accel install -r requirements_for_test.txt + +.PHONY: generate-version-file +generate-version-file: ## Generates the app version file + @echo -e "__travis_commit__ = \"${GIT_COMMIT}\"\n__time__ = \"${DATE}\"\n__travis_job_number__ = \"${BUILD_NUMBER}\"\n__travis_job_url__ = \"${BUILD_URL}\"" > ${APP_VERSION_FILE} + +.PHONY: build +build: dependencies generate-version-file ## Build project + +.PHONY: build-codedeploy-artifact +build-codedeploy-artifact: ## Build the deploy artifact for CodeDeploy + mkdir -p target + zip -r -x@deploy-exclude.lst target/notifications-api.zip * + +.PHONY: upload-codedeploy-artifact ## Upload the deploy artifact for CodeDeploy +upload-codedeploy-artifact: check-env-vars + aws s3 cp --region eu-west-1 target/notifications-api.zip s3://${DNS_NAME}-codedeploy/notifications-api-${DEPLOY_BUILD_NUMBER}.zip + +.PHONY: test +test: venv ## Run tests + ./scripts/run_tests.sh + +.PHONY: deploy-api +deploy-api: check-env-vars ## Trigger CodeDeploy for the api + aws deploy create-deployment --application-name notify-api --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-api --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 + +.PHONY: deploy-admin-api +deploy-admin-api: check-env-vars ## Trigger CodeDeploy for the admin api + aws deploy create-deployment --application-name notify-admin-api --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-admin-api --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 + +.PHONY: deploy-delivery +deploy-delivery: check-env-vars ## Trigger CodeDeploy for the delivery app + aws deploy create-deployment --application-name notify-delivery --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-delivery --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 + +.PHONY: coverage +coverage: venv ## Create coverage report + ./venv/bin/coveralls + +.PHONY: prepare-docker-build-image +prepare-docker-build-image: ## Prepare the Docker builder image + mkdir -p ${PIP_ACCEL_CACHE} + make -C docker build-build-image + +.PHONY: build-with-docker +build-with-docker: prepare-docker-build-image ## Build inside a Docker container + docker run -i --rm \ + --name "${DOCKER_CONTAINER_PREFIX}-build" \ + -v `pwd`:/var/project \ + -v ${PIP_ACCEL_CACHE}:/var/project/cache/pip-accel \ + ${DOCKER_BUILDER_IMAGE_NAME} \ + make build + +.PHONY: test-with-docker +test-with-docker: prepare-docker-build-image create-docker-test-db ## Run tests inside a Docker container + docker run -i --rm \ + --name "${DOCKER_CONTAINER_PREFIX}-test" \ + --link "${DOCKER_CONTAINER_PREFIX}-db:postgres" \ + -e TEST_DATABASE=postgresql://postgres:postgres@postgres/test_notification_api \ + -v `pwd`:/var/project \ + ${DOCKER_BUILDER_IMAGE_NAME} \ + make test + +.PHONY: test-with-docker +create-docker-test-db: ## Start the test database in a Docker container + docker rm -f ${DOCKER_CONTAINER_PREFIX}-db 2> /dev/null || true + docker run -d \ + --name "${DOCKER_CONTAINER_PREFIX}-db" \ + -e POSTGRES_PASSWORD="postgres" \ + -e POSTGRES_DB=test_notification_api \ + postgres:9.5 + sleep 3 + +.PHONY: clean-docker-containers +clean-docker-containers: ## Clean up any remaining docker containers + docker rm -f $(shell docker ps -q -f "name=${DOCKER_CONTAINER_PREFIX}") 2> /dev/null || true + +.PHONY: coverage-with-docker +coverage-with-docker: prepare-docker-build-image ## Generates coverage report inside a Docker container + docker run -i --rm \ + --name "${DOCKER_CONTAINER_PREFIX}-coverage" \ + -v `pwd`:/var/project \ + ${DOCKER_BUILDER_IMAGE_NAME} \ + make coverage + +clean: + rm -rf node_modules cache target venv .coverage diff --git a/app/version.py b/app/version.py.dist similarity index 56% rename from app/version.py rename to app/version.py.dist index d6d5d6781..295df522a 100644 --- a/app/version.py +++ b/app/version.py.dist @@ -1,3 +1,4 @@ __travis_commit__ = "" -__time__ = "2016-07-05:15:38:37" +__time__ = "" __travis_job_number__ = "" +__travis_job_url__ = "" diff --git a/deploy-exclude.lst b/deploy-exclude.lst new file mode 100644 index 000000000..5eaf61748 --- /dev/null +++ b/deploy-exclude.lst @@ -0,0 +1,9 @@ +*__pycache__* +*.git* +*app/assets* +*bower_components* +*cache* +*docker* +*node_modules* +*target* +*venv* diff --git a/docker/Dockerfile-build b/docker/Dockerfile-build new file mode 100644 index 000000000..4fa6afb2d --- /dev/null +++ b/docker/Dockerfile-build @@ -0,0 +1,25 @@ +FROM python:3.4-slim + +ENV PYTHONUNBUFFERED=1 \ + DEBIAN_FRONTEND=noninteractive + +RUN \ + echo "Install base packages" \ + && apt-get update \ + && apt-get install -y --no-install-recommends \ + make \ + git \ + build-essential \ + zip \ + libpq-dev \ + + && echo "Clean up" \ + && rm -rf /var/lib/apt/lists/* /tmp/* + +RUN \ + echo "Install global pip packages" \ + && pip install \ + virtualenv \ + awscli + +WORKDIR /var/project diff --git a/docker/Makefile b/docker/Makefile new file mode 100644 index 000000000..2db0aade2 --- /dev/null +++ b/docker/Makefile @@ -0,0 +1,10 @@ +.DEFAULT_GOAL := help +SHELL := /bin/bash + +.PHONY: help +help: + @cat $(MAKEFILE_LIST) | grep -E '^[a-zA-Z_-]+:.*?## .*$$' | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' + +.PHONY: build-build-image +build-build-image: + docker build -f Dockerfile-build -t govuk/notify-api-builder . diff --git a/scripts/common_functions.sh b/scripts/common_functions.sh index a2e136b25..a489377ef 100644 --- a/scripts/common_functions.sh +++ b/scripts/common_functions.sh @@ -361,7 +361,6 @@ get_instance_name_from_tags() { error_exit "Couldn't get instance name for '$instance_id'" fi echo $instance_name - echo $instance_id return $? } @@ -371,15 +370,12 @@ get_elb_name_for_instance_name() { local instance_name=$1 declare -A elb_to_instance_mapping - + + elb_to_instance_mapping['notify-api']='notify-api' + elb_to_instance_mapping['notify-admin-api']='notify-admin-api' + elb_to_instance_mapping['notify_api']='notify-api-elb' elb_to_instance_mapping['notify_admin_api']='notify-admin-api-elb' - elb_to_instance_mapping['live_notify_api']='live-notify-api-elb' - elb_to_instance_mapping['staging_notify_api']='staging-notify-api-elb' - elb_to_instance_mapping['NotifyApi']='notify-api-elb' - elb_to_instance_mapping['live_notify_admin_api']='live-notify-admin-api-elb' - elb_to_instance_mapping['staging_notify_admin_api']='staging-notify-admin-api-elb' - elb_to_instance_mapping['NotifyAdminApi']='notify-admin-api-elb' local elb_name=${elb_to_instance_mapping[${instance_name}]} if [ -z $elb_name ]; then @@ -387,4 +383,4 @@ get_elb_name_for_instance_name() { else ELB_NAME=$elb_name fi -} \ No newline at end of file +} diff --git a/scripts/register_with_elb.sh b/scripts/register_with_elb.sh index 8e37f7757..7e6fb80ac 100755 --- a/scripts/register_with_elb.sh +++ b/scripts/register_with_elb.sh @@ -28,7 +28,6 @@ msg "Started $(basename $0) at $(/bin/date "+%F %T")" start_sec=$(/bin/date +%s.%N) msg "Getting relevant load balancer" - INSTANCE_NAME=$(get_instance_name_from_tags $INSTANCE_ID) if [[ "$(tr [:upper:] [:lower:] <<< "${INSTANCE_NAME}")" =~ 'delivery' ]]; then @@ -37,6 +36,7 @@ if [[ "$(tr [:upper:] [:lower:] <<< "${INSTANCE_NAME}")" =~ 'delivery' ]]; then fi get_elb_name_for_instance_name $INSTANCE_NAME +ELB_LIST=$ELB_NAME get_elb_list $INSTANCE_ID $ELB_NAME msg "Checking that user set at least one load balancer" diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index dd229b408..652094dd2 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -24,6 +24,9 @@ function display_result { fi } +if [ -d venv ]; then + source ./venv/bin/activate +fi pep8 . display_result $? 1 "Code style check" diff --git a/scripts/update_version_file.sh b/scripts/update_version_file.sh deleted file mode 100755 index 231c210b7..000000000 --- a/scripts/update_version_file.sh +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/bash -# -# Update the version file of the project from the Travis build details -# -sed -i -e "s/__travis_commit__ =.*/__travis_commit__ = \"$TRAVIS_COMMIT\"/g" ./app/version.py -sed -i -e "s/__travis_job_number__ =.*/__travis_job_number__ = \"$TRAVIS_BUILD_NUMBER\"/g" ./app/version.py -sed -i -e "s/__time__ =.*/__time__ = \"$(date +%Y-%m-%d:%H:%M:%S)\"/g" ./app/version.py \ No newline at end of file