diff --git a/app/dao/letter_branding_dao.py b/app/dao/letter_branding_dao.py new file mode 100644 index 000000000..e80eeea35 --- /dev/null +++ b/app/dao/letter_branding_dao.py @@ -0,0 +1,18 @@ +from app.models import LetterBranding + + +def get_letter_branding_or_platform_default(domain=None): + letter_branding = None + if domain: + letter_branding = LetterBranding.query.filter( + LetterBranding.domain == domain + ).first() + if not letter_branding: + letter_branding = LetterBranding.query.filter( + LetterBranding.platform_default == True # noqa + ).first() + return letter_branding + + +def get_all_letter_branding(): + return LetterBranding.query.order_by(LetterBranding.name).all() diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 52536c116..da8b80b42 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -150,7 +150,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, service_id=None, service_permissions=None): +def dao_create_service(service, user, service_id=None, service_permissions=None, letter_branding=None): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. @@ -172,6 +172,8 @@ def dao_create_service(service, user, service_id=None, service_permissions=None) # do we just add the default - or will we get a value from FE? insert_service_sms_sender(service, current_app.config['FROM_NUMBER']) + if letter_branding: + service.letter_branding = letter_branding db.session.add(service) diff --git a/app/models.py b/app/models.py index 71bc1c4db..7b6346d8f 100644 --- a/app/models.py +++ b/app/models.py @@ -251,10 +251,11 @@ class DVLAOrganisation(db.Model): class LetterBranding(db.Model): __tablename__ = 'letter_branding' - id = db.Column(UUID(as_uuid=True), primary_key=True) + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) name = db.Column(db.String(255), unique=True, nullable=False) filename = db.Column(db.String(255), unique=True, nullable=False) domain = db.Column(db.Text, unique=True, nullable=True) + platform_default = db.Column(db.Boolean, nullable=False, default=False) service_letter_branding = db.Table( @@ -389,6 +390,11 @@ class Service(db.Model, Versioned): secondary=service_email_branding, uselist=False, backref=db.backref('services', lazy='dynamic')) + letter_branding = db.relationship( + 'LetterBranding', + secondary=service_letter_branding, + uselist=False, + backref=db.backref('services', lazy='dynamic')) @classmethod def from_json(cls, data): diff --git a/app/schemas.py b/app/schemas.py index 0ebc42dc4..4353d98d0 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -213,7 +213,8 @@ class ServiceSchema(BaseSchema): letter_contact_block = fields.Method(serialize="get_letter_contact") def get_letter_logo_filename(self, service): - return service.dvla_organisation.filename + return service.letter_branding.filename if service.letter_branding\ + else service.dvla_organisation.filename def service_permissions(self, service): return [p.permission for p in service.permissions] diff --git a/app/service/rest.py b/app/service/rest.py index 279d00ed5..2088d96ff 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -27,6 +27,7 @@ from app.dao.fact_notification_status_dao import ( fetch_stats_for_all_services_by_date_range, fetch_monthly_template_usage_for_service ) from app.dao.inbound_numbers_dao import dao_allocate_number_for_service +from app.dao.letter_branding_dao import get_letter_branding_or_platform_default from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.service_data_retention_dao import ( fetch_service_data_retention, @@ -82,7 +83,7 @@ from app.errors import ( register_errors ) from app.letters.utils import letter_print_day -from app.models import LETTER_TYPE, NOTIFICATION_CANCELLED, Service, EmailBranding +from app.models import LETTER_TYPE, NOTIFICATION_CANCELLED, Service, EmailBranding, LetterBranding from app.schema_validation import validate from app.service import statistics from app.service.service_data_retention_schema import ( @@ -182,7 +183,7 @@ def create_service(): if not data.get('user_id'): errors = {'user_id': ['Missing data for required field.']} raise InvalidRequest(errors, status_code=400) - + domain = data.pop('service_domain', None) # validate json with marshmallow service_schema.load(data) @@ -191,7 +192,9 @@ def create_service(): # unpack valid json into service object valid_service = Service.from_json(data) - dao_create_service(valid_service, user) + letter_branding = get_letter_branding_or_platform_default(domain) + dao_create_service(valid_service, user, letter_branding=letter_branding) + return jsonify(data=service_schema.dump(valid_service).data), 201 @@ -212,7 +215,9 @@ def update_service(service_id): if 'email_branding' in req_json: email_branding_id = req_json['email_branding'] service.email_branding = None if not email_branding_id else EmailBranding.query.get(email_branding_id) - + if 'letter_branding' in req_json: + letter_branding_id = req_json['letter_branding'] + service.letter_branding = None if not letter_branding_id else LetterBranding.query.get(letter_branding_id) dao_update_service(service) if service_going_live: diff --git a/migrations/versions/0251_letter_branding_table.py b/migrations/versions/0252_letter_branding_table.py similarity index 74% rename from migrations/versions/0251_letter_branding_table.py rename to migrations/versions/0252_letter_branding_table.py index 33718f627..aea6168e5 100644 --- a/migrations/versions/0251_letter_branding_table.py +++ b/migrations/versions/0252_letter_branding_table.py @@ -1,7 +1,7 @@ """ -Revision ID: 0251_letter_branding_table -Revises: 0250_drop_stats_template_table +Revision ID: 0252_letter_branding_table +Revises: 0251_another_letter_org Create Date: 2019-01-17 15:45:33.242955 """ @@ -10,8 +10,8 @@ from alembic import op import sqlalchemy as sa from sqlalchemy.dialects import postgresql -revision = '0251_letter_branding_table' -down_revision = '0250_drop_stats_template_table' +revision = '0252_letter_branding_table' +down_revision = '0251_another_letter_org' def upgrade(): @@ -20,6 +20,7 @@ def upgrade(): sa.Column('name', sa.String(length=255), nullable=False), sa.Column('filename', sa.String(length=255), nullable=False), sa.Column('domain', sa.Text(), nullable=True), + sa.Column('platform_default', sa.Boolean(), nullable=False), sa.PrimaryKeyConstraint('id'), sa.UniqueConstraint('domain'), sa.UniqueConstraint('filename'), @@ -35,16 +36,13 @@ def upgrade(): op.get_bind() - op.execute("""INSERT INTO letter_branding (id, name, filename, domain) - SELECT uuid_in(md5(random()::text)::cstring), name, filename, null + op.execute("""INSERT INTO letter_branding (id, name, filename, domain, platform_default) + SELECT uuid_in(md5(random()::text)::cstring), name, filename, null, false from dvla_organisation""") - op.execute("""INSERT INTO service_letter_branding (service_id, letter_branding_id) - SELECT S.id, LB.id - FROM services s - JOIN dvla_organisation d on (s.dvla_organisation_id = d.id) - JOIN letter_branding lb on (lb.filename = d.filename) - """) + op.execute("""UPDATE letter_branding set platform_default = True + WHERE filename='hm-government' + """) def downgrade(): diff --git a/tests/app/dao/test_letter_branding_dao.py b/tests/app/dao/test_letter_branding_dao.py new file mode 100644 index 000000000..d7cb0f772 --- /dev/null +++ b/tests/app/dao/test_letter_branding_dao.py @@ -0,0 +1,23 @@ +from app.dao.letter_branding_dao import get_letter_branding_or_platform_default +from tests.app.db import create_letter_branding + + +def test_get_letter_branding_or_platform_default_returns_platform_default_if_domain_is_none(notify_db_session): + create_letter_branding() + result = get_letter_branding_or_platform_default(domain=None) + assert result.filename == 'hm-government' + + +def test_get_letter_branding_or_platform_default_if_domain_is_not_associated_with_a_brand(notify_db_session): + create_letter_branding() + result = get_letter_branding_or_platform_default(domain="foo.bar") + assert result.filename == 'hm-government' + + +def test_get_letter_branding_or_platform_default_returns_correct_brand_for_domain(notify_db_session): + create_letter_branding() + test_domain_branding = create_letter_branding( + name='test domain', filename='test-domain', domain='test.domain', platform_default=False + ) + result = get_letter_branding_or_platform_default(domain='test.domain') + result == test_domain_branding diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 4a1ec0b4b..23e05c62e 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -65,7 +65,8 @@ from tests.app.db import ( create_template, create_notification, create_api_key, - create_invited_user + create_invited_user, + create_letter_branding, ) @@ -76,6 +77,7 @@ def test_should_have_decorated_services_dao_functions(): def test_create_service(notify_db_session): user = create_user() + create_letter_branding() assert Service.query.count() == 0 service = Service(name="service_name", email_from="email_from", @@ -97,6 +99,27 @@ def test_create_service(notify_db_session): assert user in service_db.users assert service_db.organisation_type == 'central' assert service_db.crown is True + assert not service.letter_branding + + +def test_create_service_with_letter_branding(notify_db_session): + user = create_user() + create_letter_branding() + letter_branding = create_letter_branding( + name='test domain', filename='test-domain', domain='test.domain', platform_default=False + ) + assert Service.query.count() == 0 + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + restricted=False, + organisation_type='central', + created_by=user) + dao_create_service(service, user, letter_branding=letter_branding) + assert Service.query.count() == 1 + service_db = Service.query.one() + assert service_db.id == service.id + assert service.letter_branding == letter_branding def test_cannot_create_two_services_with_same_name(notify_db_session): @@ -332,6 +355,7 @@ def test_create_service_by_id_adding_and_removing_letter_returns_service_without def test_create_service_creates_a_history_record_with_current_data(notify_db_session): user = create_user() + create_letter_branding() assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 service = Service(name="service_name", diff --git a/tests/app/db.py b/tests/app/db.py index 5c4889057..5426446f7 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -50,6 +50,7 @@ from app.models import ( Complaint, InvitedUser, TemplateFolder, + LetterBranding ) @@ -724,3 +725,14 @@ def create_template_folder(service, name='foo', parent=None): db.session.add(tf) db.session.commit() return tf + + +def create_letter_branding(name='HM Government', filename='hm-government', domain=None, platform_default=True): + test_domain_branding = LetterBranding(name=name, + filename=filename, + domain=domain, + platform_default=platform_default + ) + db.session.add(test_domain_branding) + db.session.commit() + return test_domain_branding diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 931149480..65fc2ac73 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -43,7 +43,8 @@ from tests.app.db import ( create_letter_contact, create_inbound_number, create_service_sms_sender, - create_service_with_defined_sms_sender + create_service_with_defined_sms_sender, + create_letter_branding ) from tests.app.db import create_user @@ -218,6 +219,7 @@ def test_get_service_by_id_should_404_if_no_service_for_user(notify_api, sample_ def test_create_service(client, sample_user): + platform_default = create_letter_branding() data = { 'name': 'created service', 'user_id': str(sample_user.id), @@ -259,6 +261,80 @@ def test_create_service(client, sample_user): service_sms_senders = ServiceSmsSender.query.filter_by(service_id=service_db.id).all() assert len(service_sms_senders) == 1 assert service_sms_senders[0].sms_sender == current_app.config['FROM_NUMBER'] + assert json_resp['data']['letter_branding'] == str(platform_default.id) + + +def test_create_service_with_domain_sets_letter_branding(client, sample_user): + letter_branding = create_letter_branding( + name='test domain', filename='test-domain', domain='test.domain', platform_default=False + ) + data = { + 'name': 'created service', + 'user_id': str(sample_user.id), + 'message_limit': 1000, + 'restricted': False, + 'active': False, + 'email_from': 'created.service', + 'created_by': str(sample_user.id), + 'service_domain': letter_branding.domain + } + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + resp = client.post( + '/service', + data=json.dumps(data), + headers=headers) + json_resp = resp.json + assert resp.status_code == 201 + assert json_resp['data']['letter_branding'] == str(letter_branding.id) + assert json_resp['data']['letter_logo_filename'] == str(letter_branding.filename) + + +def test_create_service_when_letter_branding_is_empty(client, sample_user): + # test create service before the data migration + data = { + 'name': 'created service', + 'user_id': str(sample_user.id), + 'message_limit': 1000, + 'restricted': False, + 'active': False, + 'email_from': 'created.service', + 'created_by': str(sample_user.id), + 'service_domain': 'test.domain' + } + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + resp = client.post( + '/service', + data=json.dumps(data), + headers=headers) + json_resp = resp.json + assert resp.status_code == 201 + assert not json_resp['data']['letter_branding'] + assert json_resp['data']['letter_logo_filename'] == 'hm-government' + + +def test_get_service_by_id_returns_letter_branding_not_dvla_organisation( + client, sample_service +): + letter_branding = create_letter_branding( + name='test domain', filename='test-domain', domain='test.domain', platform_default=False + ) + data = { + 'letter_branding': str(letter_branding.id) + } + client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + resp = client.get('/service/{}'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + json_resp = resp.json + assert json_resp['data']['name'] == sample_service.name + assert json_resp['data']['id'] == str(sample_service.id) + assert json_resp['data']['dvla_organisation'] == '001' + assert json_resp['data']['letter_logo_filename'] == 'test-domain' def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_uuid): @@ -438,6 +514,24 @@ def test_update_service(client, notify_db, sample_service): assert result['data']['organisation_type'] == 'foo' +def test_update_service_letter_branding(client, notify_db, sample_service): + letter_branding = create_letter_branding(name='test brand', filename='test-brand', platform_default=False) + data = { + 'letter_branding': str(letter_branding.id) + } + + auth_header = create_authorization_header() + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['letter_branding'] == str(letter_branding.id) + + def test_update_service_remove_email_branding(admin_request, notify_db, sample_service): brand = EmailBranding(colour='#000000', logo='justice-league.png', name='Justice League') sample_service.email_branding = brand