From d38fdb2d113d3c356677b3fe0cfe544553b158e3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 4 Aug 2016 12:35:47 +0100 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 cc7ea8043c5fc05ea00dea49e34ed64ff6096e57 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 12 Aug 2016 11:40:57 +0100 Subject: [PATCH 5/6] 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 6/6] 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):