From b1ed722252ec06888aca9dbf9a3779575f1fcf74 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 8 Apr 2022 17:39:55 +0100 Subject: [PATCH 1/3] When creating a new NHS org, set default email branding to NHS This is more appropriate default for that org than gov.uk branding and will help us with our work to make setting the branding more self-service. --- app/config.py | 1 + app/organisation/rest.py | 5 +++- tests/app/db.py | 6 ++++- tests/app/organisation/test_rest.py | 38 +++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/app/config.py b/app/config.py index 0778e4c93..691b4b56b 100644 --- a/app/config.py +++ b/app/config.py @@ -185,6 +185,7 @@ class Config(object): MOU_SIGNED_ON_BEHALF_ON_BEHALF_RECEIPT_TEMPLATE_ID = '522b6657-5ca5-4368-a294-6b527703bd0b' NOTIFY_INTERNATIONAL_SMS_SENDER = '07984404008' LETTERS_VOLUME_EMAIL_TEMPLATE_ID = '11fad854-fd38-4a7c-bd17-805fb13dfc12' + NHS_EMAIL_BRANDING_ID = 'a7dc4e56-660b-4db7-8cff-12c37b12b5ea' # we only need real email in Live environment (production) DVLA_EMAIL_ADDRESSES = json.loads(os.environ.get('DVLA_EMAIL_ADDRESSES', '[]')) diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 18bbdc3e9..0f5bed9c1 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -22,7 +22,7 @@ from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id from app.dao.users_dao import get_user_by_id from app.errors import InvalidRequest, register_errors -from app.models import KEY_TYPE_NORMAL, Organisation +from app.models import KEY_TYPE_NORMAL, NHS_ORGANISATION_TYPES, Organisation from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, @@ -93,6 +93,9 @@ def create_organisation(): validate(data, post_create_organisation_schema) + if data["organisation_type"] in NHS_ORGANISATION_TYPES: + data["email_branding_id"] = current_app.config['NHS_EMAIL_BRANDING_ID'] + organisation = Organisation(**data) dao_create_organisation(organisation) return jsonify(organisation.serialize()), 201 diff --git a/tests/app/db.py b/tests/app/db.py index f5b91f7d7..9e691bd04 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -502,13 +502,17 @@ def create_service_callback_api( return service_callback_api -def create_email_branding(colour='blue', logo='test_x2.png', name='test_org_1', text='DisplayName'): +def create_email_branding( + id=None, colour='blue', logo='test_x2.png', name='test_org_1', text='DisplayName' +): data = { 'colour': colour, 'logo': logo, 'name': name, 'text': text, } + if id: + data['id'] = id email_branding = EmailBranding(**data) dao_create_email_branding(email_branding) diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 27c9bd215..08d83f050 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -2,6 +2,7 @@ import uuid from datetime import datetime import pytest +from flask import current_app from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError @@ -187,6 +188,43 @@ def test_post_create_organisation(admin_request, notify_db_session, crown): assert len(organisation) == 1 +@pytest.mark.parametrize('org_type', ["nhs_central", "nhs_local", "nhs_gp"]) +def test_post_create_organisation_sets_default_nhs_branding_for_nhs_orgs( + admin_request, notify_db_session, org_type +): + # we wipe email_branding table in test db between the tests, so we have to recreate this branding + # that is normally present on all environments and applied through migration + nhs_email_branding_id = current_app.config['NHS_EMAIL_BRANDING_ID'] + create_email_branding( + id=nhs_email_branding_id, + logo='1ac6f483-3105-4c9e-9017-dd7fb2752c44-nhs-blue_x2.png', + name='NHS' + ) + + data = { + 'name': 'test organisation', + 'active': True, + 'crown': False, + 'organisation_type': org_type, + } + + response = admin_request.post( + 'organisation.create_organisation', + _data=data, + _expected_status=201 + ) + + organisations = Organisation.query.all() + + assert data['name'] == response['name'] + assert data['active'] == response['active'] + assert data['crown'] == response['crown'] + assert data['organisation_type'] == response['organisation_type'] + + assert len(organisations) == 1 + assert organisations[0].email_branding_id == uuid.UUID(nhs_email_branding_id) + + def test_post_create_organisation_existing_name_raises_400(admin_request, sample_organisation): data = { 'name': sample_organisation.name, From 769b71cdc0a94e9d79323e72ff1ffdc6c87d88bb Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 12 Apr 2022 17:18:57 +0100 Subject: [PATCH 2/3] When updating org type to NHS type also update email branding if none set --- app/organisation/rest.py | 6 ++ tests/app/db.py | 2 + tests/app/organisation/test_rest.py | 87 ++++++++++++++++++++++++++--- 3 files changed, 87 insertions(+), 8 deletions(-) diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 0f5bed9c1..f7184fbe3 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -105,6 +105,12 @@ def create_organisation(): def update_organisation(organisation_id): data = request.get_json() validate(data, post_update_organisation_schema) + + organisation = dao_get_organisation_by_id(organisation_id) + + if data.get('organisation_type') in NHS_ORGANISATION_TYPES and not organisation.email_branding_id: + data["email_branding_id"] = current_app.config['NHS_EMAIL_BRANDING_ID'] + result = dao_update_organisation(organisation_id, **data) if data.get('agreement_signed') is True: diff --git a/tests/app/db.py b/tests/app/db.py index 9e691bd04..ac7b976db 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -675,6 +675,7 @@ def create_organisation( billing_contact_names=None, billing_contact_email_addresses=None, billing_reference=None, + email_branding_id=None, ): data = { 'id': organisation_id, @@ -685,6 +686,7 @@ def create_organisation( 'billing_contact_names': billing_contact_names, 'billing_contact_email_addresses': billing_contact_email_addresses, 'billing_reference': billing_reference, + 'email_branding_id': email_branding_id } organisation = Organisation(**data) dao_create_organisation(organisation) diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 08d83f050..1eef23ae8 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -178,14 +178,16 @@ def test_post_create_organisation(admin_request, notify_db_session, crown): _expected_status=201 ) - organisation = Organisation.query.all() + organisations = Organisation.query.all() assert data['name'] == response['name'] assert data['active'] == response['active'] assert data['crown'] == response['crown'] assert data['organisation_type'] == response['organisation_type'] - assert len(organisation) == 1 + assert len(organisations) == 1 + # check that for non-nhs orgs, default branding is not set + assert organisations[0].email_branding_id is None @pytest.mark.parametrize('org_type', ["nhs_central", "nhs_local", "nhs_gp"]) @@ -208,7 +210,7 @@ def test_post_create_organisation_sets_default_nhs_branding_for_nhs_orgs( 'organisation_type': org_type, } - response = admin_request.post( + admin_request.post( 'organisation.create_organisation', _data=data, _expected_status=201 @@ -216,11 +218,6 @@ def test_post_create_organisation_sets_default_nhs_branding_for_nhs_orgs( organisations = Organisation.query.all() - assert data['name'] == response['name'] - assert data['active'] == response['active'] - assert data['crown'] == response['crown'] - assert data['organisation_type'] == response['organisation_type'] - assert len(organisations) == 1 assert organisations[0].email_branding_id == uuid.UUID(nhs_email_branding_id) @@ -382,6 +379,80 @@ def test_update_other_organisation_attributes_doesnt_clear_domains( ] +@pytest.mark.parametrize('new_org_type', ["nhs_central", "nhs_local", "nhs_gp"]) +def test_post_update_organisation_to_nhs_type_updates_branding_if_none_present( + admin_request, + notify_db_session, + new_org_type +): + # we wipe email_branding table in test db between the tests, so we have to recreate this branding + # that is normally present on all environments and applied through migration + nhs_email_branding_id = current_app.config['NHS_EMAIL_BRANDING_ID'] + create_email_branding( + id=nhs_email_branding_id, + logo='1ac6f483-3105-4c9e-9017-dd7fb2752c44-nhs-blue_x2.png', + name='NHS' + ) + + org = create_organisation(organisation_type='central') + data = { + 'organisation_type': new_org_type, + } + + admin_request.post( + 'organisation.update_organisation', + _data=data, + organisation_id=org.id, + _expected_status=204 + ) + + organisation = Organisation.query.all() + + assert len(organisation) == 1 + assert organisation[0].id == org.id + assert organisation[0].organisation_type == new_org_type + assert organisation[0].email_branding_id == uuid.UUID(nhs_email_branding_id) + + +@pytest.mark.parametrize('new_org_type', ["nhs_central", "nhs_local", "nhs_gp"]) +def test_post_update_organisation_to_nhs_type_does_not_update_branding_if_default_branding_set( + admin_request, + notify_db_session, + new_org_type +): + # we wipe email_branding table in test db between the tests, so we have to recreate this branding + # that is normally present on all environment and applied through migration + nhs_email_branding_id = current_app.config['NHS_EMAIL_BRANDING_ID'] + create_email_branding( + id=nhs_email_branding_id, + logo='1ac6f483-3105-4c9e-9017-dd7fb2752c44-nhs-blue_x2.png', + name='NHS' + ) + + current_branding = create_email_branding( + logo='example.png', + name='custom branding' + ) + org = create_organisation(organisation_type='central', email_branding_id=current_branding.id) + data = { + 'organisation_type': new_org_type, + } + + admin_request.post( + 'organisation.update_organisation', + _data=data, + organisation_id=org.id, + _expected_status=204 + ) + + organisation = Organisation.query.all() + + assert len(organisation) == 1 + assert organisation[0].id == org.id + assert organisation[0].organisation_type == new_org_type + assert organisation[0].email_branding_id == current_branding.id + + def test_update_organisation_default_branding( admin_request, notify_db_session, From 124562b50af90c0ec9a8436c592a414fb075af48 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 19 Apr 2022 12:25:11 +0100 Subject: [PATCH 3/3] Refactor creating nhs branding in tests into a fixture --- tests/app/conftest.py | 14 ++++++++++++ tests/app/organisation/test_rest.py | 35 +++++------------------------ 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c8653e75f..71ab86795 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -53,6 +53,7 @@ from app.models import ( from tests import create_admin_authorization_header from tests.app.db import ( create_api_key, + create_email_branding, create_inbound_number, create_invited_org_user, create_job, @@ -919,6 +920,19 @@ def broadcast_organisation(notify_db_session): return org +@pytest.fixture +def nhs_email_branding(notify_db_session): + # we wipe email_branding table in test db between the tests, so we have to recreate this branding + # that is normally present on all environments and applied through migration + nhs_email_branding_id = current_app.config['NHS_EMAIL_BRANDING_ID'] + + return create_email_branding( + id=nhs_email_branding_id, + logo='1ac6f483-3105-4c9e-9017-dd7fb2752c44-nhs-blue_x2.png', + name='NHS' + ) + + @pytest.fixture def restore_provider_details(notify_db, notify_db_session): """ diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 1eef23ae8..478571edc 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -192,17 +192,8 @@ def test_post_create_organisation(admin_request, notify_db_session, crown): @pytest.mark.parametrize('org_type', ["nhs_central", "nhs_local", "nhs_gp"]) def test_post_create_organisation_sets_default_nhs_branding_for_nhs_orgs( - admin_request, notify_db_session, org_type + admin_request, notify_db_session, nhs_email_branding, org_type ): - # we wipe email_branding table in test db between the tests, so we have to recreate this branding - # that is normally present on all environments and applied through migration - nhs_email_branding_id = current_app.config['NHS_EMAIL_BRANDING_ID'] - create_email_branding( - id=nhs_email_branding_id, - logo='1ac6f483-3105-4c9e-9017-dd7fb2752c44-nhs-blue_x2.png', - name='NHS' - ) - data = { 'name': 'test organisation', 'active': True, @@ -219,7 +210,7 @@ def test_post_create_organisation_sets_default_nhs_branding_for_nhs_orgs( organisations = Organisation.query.all() assert len(organisations) == 1 - assert organisations[0].email_branding_id == uuid.UUID(nhs_email_branding_id) + assert organisations[0].email_branding_id == uuid.UUID(current_app.config['NHS_EMAIL_BRANDING_ID']) def test_post_create_organisation_existing_name_raises_400(admin_request, sample_organisation): @@ -382,18 +373,10 @@ def test_update_other_organisation_attributes_doesnt_clear_domains( @pytest.mark.parametrize('new_org_type', ["nhs_central", "nhs_local", "nhs_gp"]) def test_post_update_organisation_to_nhs_type_updates_branding_if_none_present( admin_request, + nhs_email_branding, notify_db_session, new_org_type ): - # we wipe email_branding table in test db between the tests, so we have to recreate this branding - # that is normally present on all environments and applied through migration - nhs_email_branding_id = current_app.config['NHS_EMAIL_BRANDING_ID'] - create_email_branding( - id=nhs_email_branding_id, - logo='1ac6f483-3105-4c9e-9017-dd7fb2752c44-nhs-blue_x2.png', - name='NHS' - ) - org = create_organisation(organisation_type='central') data = { 'organisation_type': new_org_type, @@ -411,24 +394,16 @@ def test_post_update_organisation_to_nhs_type_updates_branding_if_none_present( assert len(organisation) == 1 assert organisation[0].id == org.id assert organisation[0].organisation_type == new_org_type - assert organisation[0].email_branding_id == uuid.UUID(nhs_email_branding_id) + assert organisation[0].email_branding_id == uuid.UUID(current_app.config['NHS_EMAIL_BRANDING_ID']) @pytest.mark.parametrize('new_org_type', ["nhs_central", "nhs_local", "nhs_gp"]) def test_post_update_organisation_to_nhs_type_does_not_update_branding_if_default_branding_set( admin_request, + nhs_email_branding, notify_db_session, new_org_type ): - # we wipe email_branding table in test db between the tests, so we have to recreate this branding - # that is normally present on all environment and applied through migration - nhs_email_branding_id = current_app.config['NHS_EMAIL_BRANDING_ID'] - create_email_branding( - id=nhs_email_branding_id, - logo='1ac6f483-3105-4c9e-9017-dd7fb2752c44-nhs-blue_x2.png', - name='NHS' - ) - current_branding = create_email_branding( logo='example.png', name='custom branding'