From 337496c5bc72aff710727c4252d5b9d9a1de9674 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 30 Aug 2018 15:51:39 +0100 Subject: [PATCH] Deprecate service branding column We want to drop this column. First we have to stop using it anywhere. Needs to be made nullable so we can stop writing to it. --- app/models.py | 7 ---- app/schemas.py | 1 - .../0221_nullable_service_branding.py | 32 +++++++++++++++++++ tests/app/dao/test_services_dao.py | 4 --- tests/app/db.py | 2 -- tests/app/delivery/test_send_to_providers.py | 9 ++---- tests/app/service/test_rest.py | 2 +- 7 files changed, 35 insertions(+), 22 deletions(-) create mode 100644 migrations/versions/0221_nullable_service_branding.py diff --git a/app/models.py b/app/models.py index 9a691a4d4..d6dd3949a 100644 --- a/app/models.py +++ b/app/models.py @@ -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..7040ba484 --- /dev/null +++ b/migrations/versions/0221_nullable_service_branding.py @@ -0,0 +1,32 @@ +""" + 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 + + +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) + + +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) 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..1f7431472 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -422,8 +422,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,7 +446,6 @@ 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', @@ -466,7 +463,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 +474,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 +483,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 +494,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/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