From 2671ca3e5ec0a958820d4c87ea45d4fe1b800593 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 28 Aug 2018 14:25:16 +0100 Subject: [PATCH 1/2] Look in email_branding for brand_type, not service https://www.pivotaltracker.com/story/show/159986276 We are now setting the type of branding on the branding itself, not on the service. This commit switches over from looking in the old place (on the service) to looking in the new place (on the branding). --- app/delivery/send_to_providers.py | 36 +++++++----- tests/app/delivery/test_send_to_providers.py | 62 +++++++++++++++----- 2 files changed, 67 insertions(+), 31 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index c285c00e2..d2bedaf48 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -23,7 +23,7 @@ from app.exceptions import NotificationTechnicalFailureException from app.models import ( SMS_TYPE, KEY_TYPE_TEST, - BRANDING_ORG, + BRANDING_BOTH, BRANDING_ORG_BANNER, BRANDING_GOVUK, EMAIL_TYPE, @@ -189,24 +189,28 @@ def get_logo_url(base_url, logo_file): def get_html_email_options(service): - govuk_banner = service.branding not in (BRANDING_ORG, BRANDING_ORG_BANNER) - brand_banner = service.branding == BRANDING_ORG_BANNER - if service.branding != BRANDING_GOVUK and service.email_branding: - logo_url = get_logo_url( - current_app.config['ADMIN_BASE_URL'], - service.email_branding.logo - ) if service.email_branding.logo else None - - branding = { - 'brand_colour': service.email_branding.colour, - 'brand_logo': logo_url, - 'brand_name': service.email_branding.text, + if ( + service.email_branding is None or + service.email_branding.brand_type == BRANDING_GOVUK + ): + return { + 'govuk_banner': True, + 'brand_banner': False, } - else: - branding = {} - return dict(govuk_banner=govuk_banner, brand_banner=brand_banner, **branding) + logo_url = get_logo_url( + current_app.config['ADMIN_BASE_URL'], + service.email_branding.logo + ) if service.email_branding.logo else None + + return { + 'govuk_banner': service.email_branding.brand_type == BRANDING_BOTH, + 'brand_banner': service.email_branding.brand_type == BRANDING_ORG_BANNER, + 'brand_colour': service.email_branding.colour, + 'brand_logo': logo_url, + 'brand_name': service.email_branding.text, + } def technical_failure(notification): diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 48b5b6e04..4631d3dc4 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -409,7 +409,7 @@ def test_send_email_should_use_service_reply_to_email( def test_get_html_email_renderer_should_return_for_normal_service(sample_service): options = send_to_providers.get_html_email_options(sample_service) - assert options['govuk_banner'] + assert options['govuk_banner'] is True assert 'brand_colour' not in options.keys() assert 'brand_logo' not in options.keys() assert 'brand_name' not in options.keys() @@ -421,9 +421,16 @@ def test_get_html_email_renderer_should_return_for_normal_service(sample_service (BRANDING_ORG_BANNER, False) ]) def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service): - sample_service.branding = branding_type - email_branding = EmailBranding(colour='#000000', logo='justice-league.png', name='Justice League', - text='League of Justice') + + sample_service.branding = BRANDING_GOVUK # Expected to be ignored + + email_branding = EmailBranding( + brand_type=branding_type, + colour='#000000', + logo='justice-league.png', + name='Justice League', + text='League of Justice', + ) sample_service.email_branding = email_branding notify_db.session.add_all([sample_service, email_branding]) notify_db.session.commit() @@ -434,7 +441,7 @@ def test_get_html_email_renderer_with_branding_details(branding_type, govuk_bann assert options['brand_colour'] == '#000000' assert options['brand_name'] == 'League of Justice' - if sample_service.branding == BRANDING_ORG_BANNER: + if branding_type == BRANDING_ORG_BANNER: assert options['brand_banner'] is True else: assert options['brand_banner'] is False @@ -442,8 +449,13 @@ def test_get_html_email_renderer_with_branding_details(branding_type, govuk_bann def test_get_html_email_renderer_with_branding_details_and_render_govuk_banner_only(notify_db, sample_service): sample_service.branding = BRANDING_GOVUK - email_branding = EmailBranding(colour='#000000', logo='justice-league.png', name='Justice League', - text='League of Justice') + email_branding = EmailBranding( + brand_type=BRANDING_GOVUK, + colour='#000000', + logo='justice-league.png', + name='Justice League', + text='League of Justice', + ) sample_service.email_branding = email_branding notify_db.session.add_all([sample_service, email_branding]) notify_db.session.commit() @@ -455,12 +467,19 @@ def test_get_html_email_renderer_with_branding_details_and_render_govuk_banner_o def test_get_html_email_renderer_prepends_logo_path(notify_api): Service = namedtuple('Service', ['branding', 'email_branding']) - EmailBranding = namedtuple('EmailBranding', ['colour', 'name', 'logo', 'text']) + EmailBranding = namedtuple('EmailBranding', ['brand_type', 'colour', 'name', 'logo', 'text']) - email_branding = EmailBranding(colour='#000000', logo='justice-league.png', - name='Justice League', - text='League of Justice') - service = Service(branding=BRANDING_ORG, email_branding=email_branding) + email_branding = EmailBranding( + brand_type=BRANDING_ORG, + colour='#000000', + logo='justice-league.png', + name='Justice League', + text='League of Justice', + ) + service = Service( + branding=BRANDING_GOVUK, # expected to be ignored + email_branding=email_branding, + ) renderer = send_to_providers.get_html_email_options(service) @@ -469,14 +488,27 @@ def test_get_html_email_renderer_prepends_logo_path(notify_api): def test_get_html_email_renderer_handles_email_branding_without_logo(notify_api): Service = namedtuple('Service', ['branding', 'email_branding']) - EmailBranding = namedtuple('EmailBranding', ['colour', 'name', 'logo', 'text']) + EmailBranding = namedtuple('EmailBranding', ['brand_type', 'colour', 'name', 'logo', 'text']) - email_branding = EmailBranding(colour='#000000', logo=None, name='Justice League', text='League of Justice') - service = Service(branding=BRANDING_ORG_BANNER, email_branding=email_branding) + email_branding = EmailBranding( + brand_type=BRANDING_ORG_BANNER, + colour='#000000', + logo=None, + name='Justice League', + text='League of Justice', + ) + service = Service( + branding=BRANDING_GOVUK, # Expected to be ignored + email_branding=email_branding, + ) renderer = send_to_providers.get_html_email_options(service) + assert renderer['govuk_banner'] is False + assert renderer['brand_banner'] is True assert renderer['brand_logo'] is None + assert renderer['brand_name'] == 'League of Justice' + assert renderer['brand_colour'] == '#000000' @pytest.mark.parametrize('base_url, expected_url', [ From a0deef06e23e4c81e55d83afb63d4bbab1bdaaa5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 28 Aug 2018 14:38:51 +0100 Subject: [PATCH 2/2] =?UTF-8?q?Set=20branding=5Ftype=20to=20org=20if=20it?= =?UTF-8?q?=E2=80=99s=20none?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So later we can: - make it non-nullable later - remove `govuk` as an option This is mostly for people’s local databases, the manual work here has been done on production already. --- .../versions/0217_default_email_branding.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 migrations/versions/0217_default_email_branding.py diff --git a/migrations/versions/0217_default_email_branding.py b/migrations/versions/0217_default_email_branding.py new file mode 100644 index 000000000..caa185c0f --- /dev/null +++ b/migrations/versions/0217_default_email_branding.py @@ -0,0 +1,25 @@ +""" + Revision ID: 0217_default_email_branding +Revises: 0216_remove_colours +Create Date: 2018-08-24 13:36:49.346156 + """ +from alembic import op +from app.models import BRANDING_ORG + +revision = '0217_default_email_branding' +down_revision = '0216_remove_colours' + + +def upgrade(): + op.execute(""" + update + email_branding + set + brand_type = '{}' + where + brand_type = null + """.format(BRANDING_ORG)) + + +def downgrade(): + pass