From a8d011cdd3bf8249b827f0143fc0755c879e4a18 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Wed, 29 Aug 2018 13:59:23 +0100 Subject: [PATCH 1/6] Update moto from 1.3.4 to 1.3.5 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index cbd525d84..ee6facf2b 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,6 +1,6 @@ -r requirements.txt flake8==3.5.0 -moto==1.3.4 +moto==1.3.5 pytest==3.7.3 pytest-env==0.6.2 pytest-mock==1.10.0 From 337496c5bc72aff710727c4252d5b9d9a1de9674 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 30 Aug 2018 15:51:39 +0100 Subject: [PATCH 2/6] 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 From 0c47d419772164a4aba769497acc62570be6ddfa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 30 Aug 2018 16:22:59 +0100 Subject: [PATCH 3/6] Remove govuk from possible brands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘GOV.UK’ doesn’t make sense as a type of brand. It only made sense as a type of branding that a service had. Since we’ve: - deprecated the service branding column - made sure it’s not used as a value in the email branding table we can remove this value from the table of possible brand types. --- app/delivery/send_to_providers.py | 6 +---- app/models.py | 6 ++--- .../0221_nullable_service_branding.py | 25 +++++++++++++++++++ tests/app/delivery/test_send_to_providers.py | 13 ++-------- tests/app/email_branding/test_rest.py | 8 +++--- 5 files changed, 35 insertions(+), 23 deletions(-) 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 d6dd3949a..39e7b2716 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): @@ -213,7 +213,7 @@ class EmailBranding(db.Model): db.ForeignKey('branding_type.name'), index=True, nullable=True, - default=BRANDING_GOVUK + default=BRANDING_ORG ) def serialize(self): diff --git a/migrations/versions/0221_nullable_service_branding.py b/migrations/versions/0221_nullable_service_branding.py index 7040ba484..f7280780d 100644 --- a/migrations/versions/0221_nullable_service_branding.py +++ b/migrations/versions/0221_nullable_service_branding.py @@ -4,6 +4,7 @@ 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' @@ -20,6 +21,22 @@ def upgrade(): 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(): @@ -30,3 +47,11 @@ def downgrade(): 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/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 1f7431472..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') @@ -446,15 +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): - 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) 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]' From 66431f82be7805a87c9c05055ba78a9d55b5ee85 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Fri, 31 Aug 2018 06:34:21 +0100 Subject: [PATCH 4/6] Update coveralls from 1.4.0 to 1.5.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index cbd525d84..8df43a63c 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -6,7 +6,7 @@ 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 From 834ad733046ea5a4bc010a893f63fb24aaa64e9b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 30 Aug 2018 16:51:53 +0100 Subject: [PATCH 5/6] Drop service branding column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re not using this column any more, so let’s clean up after ourselves. --- .../versions/0222_drop_service_branding.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 migrations/versions/0222_drop_service_branding.py 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))) From f095fa1ddf5fd2d9aa793b02f6e00d7f5a380507 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Sep 2018 16:48:08 +0100 Subject: [PATCH 6/6] Add unique constraint to email branding domain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brandings with a domain set should be considered canonical. It doesn’t make sense to have the same domain set on multiple different email brands – you can’t tell which one to use. --- app/models.py | 2 +- .../versions/0223_add_domain_constraint.py | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0223_add_domain_constraint.py diff --git a/app/models.py b/app/models.py index 39e7b2716..353b6e56c 100644 --- a/app/models.py +++ b/app/models.py @@ -207,7 +207,7 @@ 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'), 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')