From 5cbe4eb5b2a94a036e41caf00620b53842f14484 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 21 Dec 2016 16:15:18 +0000 Subject: [PATCH 1/2] use cloudfront instead of flask to serve static images branding in emails was previously hosted from admin app - this changes the url to be static.{domain}/images instead of {domain}/static/images, which redirects to cloudfront. some manipulation of the URL was required to make sure it still serves locally rather than returning "static.localhost:6012" for example. (if ADMIN_BASE_URL is localhost it just returns the old /static/ path) also was able to remove DB interaction from a test. woo! --- app/delivery/send_to_providers.py | 41 +++++++++++++++++++- config.py | 2 +- requirements.txt | 4 +- tests/app/delivery/test_send_to_providers.py | 32 +++++++++++---- 4 files changed, 67 insertions(+), 12 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 55da5d933..95f78f579 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -1,3 +1,4 @@ +from urllib import parse from datetime import datetime from flask import current_app @@ -115,17 +116,53 @@ def provider_to_use(notification_type, notification_id): return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) +def get_logo_url(base_url, branding_path, logo_file): + """ + Get the complete URL for a given logo. + + We have to convert the base_url into a static url. Our hosted environments all have their own cloudfront instances, + found at the static subdomain (eg https://static.notifications.service.gov.uk). + + If running locally (dev environment), don't try and use cloudfront - just stick to the actual underlying source + ({URL}/static/{PATH}) + """ + base_url = parse.urlparse(base_url) + netloc = base_url.netloc + + if base_url.netloc.startswith('localhost'): + netloc = 'localhost:6012' + path = '/static' + branding_path + logo_file + else: + if base_url.netloc.startswith('www'): + # strip "www." + netloc = base_url.netloc[4:] + + netloc = 'static.' + netloc + path = branding_path + logo_file + + logo_url = parse.ParseResult( + scheme=base_url.scheme, + netloc=netloc, + path=path, + params=base_url.params, + query=base_url.query, + fragment=base_url.fragment + ) + return parse.urlunparse(logo_url) + + def get_html_email_options(service): govuk_banner = service.branding != BRANDING_ORG if service.organisation: - logo = '{}{}{}'.format( + logo_url = get_logo_url( current_app.config['ADMIN_BASE_URL'], current_app.config['BRANDING_PATH'], service.organisation.logo ) + branding = { 'brand_colour': service.organisation.colour, - 'brand_logo': logo, + 'brand_logo': logo_url, 'brand_name': service.organisation.name, } else: diff --git a/config.py b/config.py index 5c4105ffc..884fed7eb 100644 --- a/config.py +++ b/config.py @@ -59,7 +59,7 @@ class Config(object): SQLALCHEMY_TRACK_MODIFICATIONS = True PAGE_SIZE = 50 SMS_CHAR_COUNT_LIMIT = 495 - BRANDING_PATH = '/static/images/email-template/crests/' + BRANDING_PATH = '/images/email-template/crests/' TEST_MESSAGE_FILENAME = 'Test message' NOTIFY_SERVICE_ID = 'd6aa2c68-a2d9-4437-ab19-3ae8eb202553' diff --git a/requirements.txt b/requirements.txt index cadf2f584..2ce5add5e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,8 +20,8 @@ statsd==3.2.1 jsonschema==2.5.1 Flask-Redis==0.1.0 -git+https://github.com/alphagov/notifications-python-client.git@3.0.0#egg=notifications-python-client==3.0.0 +notifications-python-client==3.0.0 -git+https://github.com/alphagov/notifications-utils.git@12.1.1#egg=notifications-utils==12.1.1 +git+https://github.com/alphagov/notifications-utils.git@12.2.0#egg=notifications-utils==12.2.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 2f37d1eed..9bf62286d 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -1,5 +1,6 @@ import uuid from datetime import datetime +from collections import namedtuple import pytest from unittest.mock import ANY @@ -379,18 +380,35 @@ def test_get_html_email_renderer_with_branding_details(branding_type, govuk_bann assert options['brand_name'] == 'Justice League' -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 - notify_db.session.add_all([sample_service, org]) - notify_db.session.commit() +def test_get_html_email_renderer_prepends_logo_path(notify_api): + Service = namedtuple('Service', ['branding', 'organisation']) + Organisation = namedtuple('Organisation', ['colour', 'name', 'logo']) - renderer = send_to_providers.get_html_email_options(sample_service) + org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') + service = Service(branding=BRANDING_ORG, organisation=org) + + renderer = send_to_providers.get_html_email_options(service) assert renderer['brand_logo'] == 'http://localhost:6012/static/images/email-template/crests/justice-league.png' +@pytest.mark.parametrize('base_url, expected_url', [ + # don't change localhost to prevent errors when testing locally + ('http://localhost:6012', 'http://localhost:6012/static/sub-path/filename.png'), + # on other environments, replace www with staging + ('https://www.notifications.service.gov.uk', 'https://static.notifications.service.gov.uk/sub-path/filename.png'), + ('https://www.notify.works', 'https://static.notify.works/sub-path/filename.png'), + ('https://notify.works', 'https://static.notify.works/sub-path/filename.png'), +]) +def test_get_logo_url_works_for_different_environments(base_url, expected_url): + branding_path = '/sub-path/' + logo_file = 'filename.png' + + logo_url = send_to_providers.get_logo_url(base_url, branding_path, logo_file) + + assert logo_url == expected_url + + def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker): mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") From bf0abce4a3532a15aaa26d8954b0d22a013e25ab Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 22 Dec 2016 13:53:27 +0000 Subject: [PATCH 2/2] update python-client version also unpin the patch version, since it shouldn't matter what patch we're on provided that the function signatures are the same --- requirements.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 2ce5add5e..a414d2549 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,7 +20,8 @@ statsd==3.2.1 jsonschema==2.5.1 Flask-Redis==0.1.0 -notifications-python-client==3.0.0 +# pin to minor version 3.1.x +notifications-python-client>=3.1,<3.2 git+https://github.com/alphagov/notifications-utils.git@12.2.0#egg=notifications-utils==12.2.0