diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index d2bedaf48..82c7879ba 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -25,7 +25,6 @@ from app.models import ( KEY_TYPE_TEST, BRANDING_BOTH, BRANDING_ORG_BANNER, - BRANDING_GOVUK, EMAIL_TYPE, NOTIFICATION_CREATED, NOTIFICATION_TECHNICAL_FAILURE, @@ -190,10 +189,7 @@ def get_logo_url(base_url, logo_file): def get_html_email_options(service): - if ( - service.email_branding is None or - service.email_branding.brand_type == BRANDING_GOVUK - ): + if service.email_branding is None: return { 'govuk_banner': True, 'brand_banner': False, diff --git a/app/models.py b/app/models.py index 9a691a4d4..353b6e56c 100644 --- a/app/models.py +++ b/app/models.py @@ -188,11 +188,11 @@ user_to_organisation = db.Table( ) -BRANDING_GOVUK = 'govuk' +BRANDING_GOVUK = 'govuk' # Deprecated outside migrations BRANDING_ORG = 'org' BRANDING_BOTH = 'both' BRANDING_ORG_BANNER = 'org_banner' -BRANDING_TYPES = [BRANDING_GOVUK, BRANDING_ORG, BRANDING_BOTH, BRANDING_ORG_BANNER] +BRANDING_TYPES = [BRANDING_ORG, BRANDING_BOTH, BRANDING_ORG_BANNER] class BrandingTypes(db.Model): @@ -207,13 +207,13 @@ class EmailBranding(db.Model): logo = db.Column(db.String(255), nullable=True) name = db.Column(db.String(255), nullable=True) text = db.Column(db.String(255), nullable=True) - domain = db.Column(db.Text, nullable=True) + domain = db.Column(db.Text, unique=True, nullable=True) brand_type = db.Column( db.String(255), db.ForeignKey('branding_type.name'), index=True, nullable=True, - default=BRANDING_GOVUK + default=BRANDING_ORG ) def serialize(self): @@ -346,13 +346,6 @@ class Service(db.Model, Versioned): default=DVLA_ORG_HM_GOVERNMENT ) dvla_organisation = db.relationship('DVLAOrganisation') - branding = db.Column( - db.String(255), - db.ForeignKey('branding_type.name'), - index=True, - nullable=False, - default=BRANDING_GOVUK - ) organisation_type = db.Column( db.String(255), nullable=True, diff --git a/app/schemas.py b/app/schemas.py index d9bb60e9e..ceb17bedb 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -204,7 +204,6 @@ class ServiceSchema(BaseSchema): created_by = field_for(models.Service, 'created_by', required=True) organisation_type = field_for(models.Service, 'organisation_type') - branding = field_for(models.Service, 'branding') dvla_organisation = field_for(models.Service, 'dvla_organisation') permissions = fields.Method("service_permissions") email_branding = field_for(models.Service, 'email_branding') diff --git a/migrations/versions/0221_nullable_service_branding.py b/migrations/versions/0221_nullable_service_branding.py new file mode 100644 index 000000000..f7280780d --- /dev/null +++ b/migrations/versions/0221_nullable_service_branding.py @@ -0,0 +1,57 @@ +""" + Revision ID: 0221_nullable_service_branding +Revises: 0220_email_brand_type_non_null +Create Date: 2018-08-24 13:36:49.346156 + """ +from alembic import op +from app.models import BRANDING_ORG, BRANDING_GOVUK + + +revision = '0221_nullable_service_branding' +down_revision = '0220_email_brand_type_non_null' + + +def upgrade(): + + op.drop_constraint('services_branding_fkey', 'services', type_='foreignkey') + + op.drop_index('ix_services_history_branding', table_name='services_history') + op.drop_index('ix_services_branding', table_name='services') + + op.alter_column('services_history', 'branding', nullable=True) + op.alter_column('services', 'branding', nullable=True) + + op.execute(""" + update + email_branding + set + brand_type = '{}' + where + brand_type = '{}' + """.format(BRANDING_ORG, BRANDING_GOVUK)) + + op.execute(""" + delete from + branding_type + where + name = '{}' + """.format(BRANDING_GOVUK)) + + +def downgrade(): + + op.create_index(op.f('ix_services_branding'), 'services', ['branding'], unique=False) + op.create_index(op.f('ix_services_history_branding'), 'services_history', ['branding'], unique=False) + + op.create_foreign_key(None, 'services', 'branding_type', ['branding'], ['name']) + + op.alter_column('services', 'branding', nullable=False) + op.alter_column('services_history', 'branding', nullable=False) + + op.execute(""" + insert into + branding_type + (name) + values + ('{}') + """.format(BRANDING_GOVUK)) diff --git a/migrations/versions/0222_drop_service_branding.py b/migrations/versions/0222_drop_service_branding.py new file mode 100644 index 000000000..12463ba0b --- /dev/null +++ b/migrations/versions/0222_drop_service_branding.py @@ -0,0 +1,23 @@ +""" + Revision ID: 0222_drop_service_branding +Revises: 0221_nullable_service_branding +Create Date: 2018-08-24 13:36:49.346156 + """ +from alembic import op +import sqlalchemy as sa + + +revision = '0222_drop_service_branding' +down_revision = '0221_nullable_service_branding' + + +def upgrade(): + + op.drop_column('services_history', 'branding') + op.drop_column('services', 'branding') + + +def downgrade(): + + op.add_column('services', sa.Column('branding', sa.String(length=255))) + op.add_column('services_history', sa.Column('branding', sa.String(length=255))) diff --git a/migrations/versions/0223_add_domain_constraint.py b/migrations/versions/0223_add_domain_constraint.py new file mode 100644 index 000000000..4b8f8d915 --- /dev/null +++ b/migrations/versions/0223_add_domain_constraint.py @@ -0,0 +1,20 @@ +""" +Revision ID: 0223_add_domain_constraint +Revises: 0222_drop_service_branding +Create Date: 2018-08-24 13:36:49.346156 + """ +from alembic import op + + +revision = '0223_add_domain_constraint' +down_revision = '0222_drop_service_branding' + + +def upgrade(): + + op.create_unique_constraint('uq_email_branding_domain', 'email_branding', ['domain']) + + +def downgrade(): + + op.drop_constraint('uq_email_branding_domain', 'email_branding') diff --git a/requirements_for_test.txt b/requirements_for_test.txt index c26aad5ca..fc9055849 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,12 +1,12 @@ -r requirements.txt flake8==3.5.0 -moto==1.3.4 pytest==3.7.4 +moto==1.3.5 pytest-env==0.6.2 pytest-mock==1.10.0 pytest-cov==2.5.1 pytest-xdist==1.23.0 -coveralls==1.4.0 +coveralls==1.5.0 freezegun==0.3.10 requests-mock==1.5.2 # optional requirements for jsonschema diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 516ffba15..a4045e96d 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -49,7 +49,6 @@ from app.models import ( Service, ServicePermission, ServicePermissionTypes, - BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, @@ -97,7 +96,6 @@ def test_create_service(sample_user): service_db = Service.query.one() assert service_db.name == "service_name" assert service_db.id == service.id - assert service_db.branding == BRANDING_GOVUK assert service_db.dvla_organisation_id == DVLA_ORG_HM_GOVERNMENT assert service_db.email_from == 'email_from' assert service_db.research_mode is False @@ -349,9 +347,7 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user): assert service_from_db.version == service_history.version assert sample_user.id == service_history.created_by_id assert service_from_db.created_by.id == service_history.created_by_id - assert service_from_db.branding == BRANDING_GOVUK assert service_from_db.dvla_organisation_id == DVLA_ORG_HM_GOVERNMENT - assert service_history.branding == BRANDING_GOVUK assert service_history.dvla_organisation_id == DVLA_ORG_HM_GOVERNMENT diff --git a/tests/app/db.py b/tests/app/db.py index c2ecf8488..669a8666c 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -78,7 +78,6 @@ def create_service( prefix_sms=True, message_limit=1000, organisation_type='central', - branding=None ): service = Service( name=service_name, @@ -88,7 +87,6 @@ def create_service( created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), prefix_sms=prefix_sms, organisation_type=organisation_type, - branding=branding ) dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 4631d3dc4..f0bb62dc2 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -21,7 +21,6 @@ from app.models import ( KEY_TYPE_TEST, KEY_TYPE_TEAM, BRANDING_ORG, - BRANDING_GOVUK, BRANDING_BOTH, BRANDING_ORG_BANNER ) @@ -202,7 +201,6 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( def test_should_call_send_sms_response_task_if_research_mode( notify_db, sample_service, sample_notification, mocker, research_mode, key_type ): - sample_service.branding = BRANDING_GOVUK mocker.patch('app.mmg_client.send_sms') mocker.patch('app.delivery.send_to_providers.send_sms_response') @@ -422,8 +420,6 @@ def test_get_html_email_renderer_should_return_for_normal_service(sample_service ]) def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service): - sample_service.branding = BRANDING_GOVUK # Expected to be ignored - email_branding = EmailBranding( brand_type=branding_type, colour='#000000', @@ -448,16 +444,8 @@ 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( - 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]) + sample_service.email_branding = None + notify_db.session.add_all([sample_service]) notify_db.session.commit() options = send_to_providers.get_html_email_options(sample_service) @@ -466,7 +454,7 @@ 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']) + Service = namedtuple('Service', ['email_branding']) EmailBranding = namedtuple('EmailBranding', ['brand_type', 'colour', 'name', 'logo', 'text']) email_branding = EmailBranding( @@ -477,7 +465,6 @@ def test_get_html_email_renderer_prepends_logo_path(notify_api): text='League of Justice', ) service = Service( - branding=BRANDING_GOVUK, # expected to be ignored email_branding=email_branding, ) @@ -487,7 +474,7 @@ 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']) + Service = namedtuple('Service', ['email_branding']) EmailBranding = namedtuple('EmailBranding', ['brand_type', 'colour', 'name', 'logo', 'text']) email_branding = EmailBranding( @@ -498,7 +485,6 @@ def test_get_html_email_renderer_handles_email_branding_without_logo(notify_api) text='League of Justice', ) service = Service( - branding=BRANDING_GOVUK, # Expected to be ignored email_branding=email_branding, ) diff --git a/tests/app/email_branding/test_rest.py b/tests/app/email_branding/test_rest.py index 6ef4edfd9..03311514a 100644 --- a/tests/app/email_branding/test_rest.py +++ b/tests/app/email_branding/test_rest.py @@ -1,6 +1,6 @@ import pytest -from app.models import EmailBranding, BRANDING_GOVUK, BRANDING_ORG +from app.models import EmailBranding, BRANDING_ORG from tests.app.db import create_email_branding @@ -76,7 +76,7 @@ def test_post_create_email_branding_without_brand_type_defaults(admin_request, n _data=data, _expected_status=201 ) - assert BRANDING_GOVUK == response['data']['brand_type'] + assert BRANDING_ORG == response['data']['brand_type'] def test_post_create_email_branding_without_logo_is_ok(admin_request, notify_db_session): @@ -240,7 +240,7 @@ def test_create_email_branding_reject_invalid_brand_type(admin_request): _expected_status=400 ) - assert response['errors'][0]['message'] == 'brand_type NOT A TYPE is not one of [govuk, org, both, org_banner]' + assert response['errors'][0]['message'] == 'brand_type NOT A TYPE is not one of [org, both, org_banner]' def test_update_email_branding_reject_invalid_brand_type(admin_request, notify_db_session): @@ -256,4 +256,4 @@ def test_update_email_branding_reject_invalid_brand_type(admin_request, notify_d email_branding_id=email_branding.id ) - assert response['errors'][0]['message'] == 'brand_type NOT A TYPE is not one of [govuk, org, both, org_banner]' + assert response['errors'][0]['message'] == 'brand_type NOT A TYPE is not one of [org, both, org_banner]' diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ac4fb027a..e6d4942d6 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -135,7 +135,7 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['id'] == str(sample_service.id) assert not json_resp['data']['research_mode'] assert json_resp['data']['email_branding'] is None - assert json_resp['data']['branding'] == 'govuk' + assert 'branding' not in json_resp['data'] assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['prefix_sms'] is True