From e030c2be8838931e4367ba0e117f84582286b047 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 25 Jan 2019 15:03:01 +0000 Subject: [PATCH] Removing platform_default as a concept. No service actually wants to send letters with the default hm-government logo so we are going to remove it as a constraint. However, until we can create a letter without a logo, we will still default to hm-government, because the dvla_organisation is set on the service. This does simplify the code. Also removed the inserts to letter_branding in the data migration file, because we can deploy this before the rest of the work is finished. But we will need to do it later. --- app/dao/letter_branding_dao.py | 15 ++----- app/letter_branding/letter_branding_schema.py | 5 +-- app/models.py | 4 +- app/service/rest.py | 4 +- .../versions/0252_letter_branding_table.py | 9 ---- tests/app/celery/test_letters_pdf_tasks.py | 2 +- tests/app/dao/test_letter_branding_dao.py | 30 +++++-------- tests/app/dao/test_services_dao.py | 2 +- tests/app/db.py | 3 +- .../test_letter_branding_rest.py | 34 ++++----------- tests/app/service/test_rest.py | 42 ++++++++++++++++--- 11 files changed, 65 insertions(+), 85 deletions(-) diff --git a/app/dao/letter_branding_dao.py b/app/dao/letter_branding_dao.py index da18eb03c..3b735df41 100644 --- a/app/dao/letter_branding_dao.py +++ b/app/dao/letter_branding_dao.py @@ -3,17 +3,10 @@ from app.dao.dao_utils import transactional from app.models import LetterBranding -def dao_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 dao_get_letter_branding_by_domain(domain): + return LetterBranding.query.filter( + LetterBranding.domain == domain + ).first() def dao_get_all_letter_branding(): diff --git a/app/letter_branding/letter_branding_schema.py b/app/letter_branding/letter_branding_schema.py index 7c95546c2..5f91bf829 100644 --- a/app/letter_branding/letter_branding_schema.py +++ b/app/letter_branding/letter_branding_schema.py @@ -7,8 +7,5 @@ post_letter_branding_schema = { "filename": {"type": ["string", "null"]}, "domain": {"type": ["string", "null"]}, }, - "required": ("name", "filename", "domain"), - # Typically we allow additional properties but we don't want to update letter_branding platform_admin, - # this can handle this without adding extra code in the rest endpoint. - "additionalProperties": False + "required": ("name", "filename", "domain") } diff --git a/app/models.py b/app/models.py index c4dc96053..a009db003 100644 --- a/app/models.py +++ b/app/models.py @@ -255,15 +255,13 @@ class LetterBranding(db.Model): 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) def serialize(self): return { - "id": self.id, + "id": str(self.id), "name": self.name, "filename": self.filename, "domain": self.domain, - "platform_default": self.platform_default } diff --git a/app/service/rest.py b/app/service/rest.py index e22eb467b..c27876b14 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -27,7 +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 dao_get_letter_branding_or_platform_default +from app.dao.letter_branding_dao import dao_get_letter_branding_by_domain from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.service_data_retention_dao import ( fetch_service_data_retention, @@ -192,7 +192,7 @@ def create_service(): # unpack valid json into service object valid_service = Service.from_json(data) - letter_branding = dao_get_letter_branding_or_platform_default(domain) + letter_branding = dao_get_letter_branding_by_domain(domain) dao_create_service(valid_service, user, letter_branding=letter_branding) return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/migrations/versions/0252_letter_branding_table.py b/migrations/versions/0252_letter_branding_table.py index aea6168e5..19662825d 100644 --- a/migrations/versions/0252_letter_branding_table.py +++ b/migrations/versions/0252_letter_branding_table.py @@ -20,7 +20,6 @@ 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'), @@ -36,14 +35,6 @@ def upgrade(): op.get_bind() - 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("""UPDATE letter_branding set platform_default = True - WHERE filename='hm-government' - """) - def downgrade(): op.drop_table('service_letter_branding') diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 4c12028d7..3a5089902 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -203,7 +203,7 @@ def test_create_letters_gets_the_right_logo_when_service_has_dvla_logo( def test_create_letters_gets_the_right_logo_when_service_has_letter_branding_logo( notify_api, mocker, sample_letter_notification ): - letter_branding = create_letter_branding(name='test brand', filename='test-brand', platform_default=False) + letter_branding = create_letter_branding(name='test brand', filename='test-brand') sample_letter_notification.service.letter_branding = letter_branding mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) mocker.patch('app.letters.utils.s3upload') diff --git a/tests/app/dao/test_letter_branding_dao.py b/tests/app/dao/test_letter_branding_dao.py index 4438f14ea..fee02e665 100644 --- a/tests/app/dao/test_letter_branding_dao.py +++ b/tests/app/dao/test_letter_branding_dao.py @@ -1,5 +1,5 @@ from app.dao.letter_branding_dao import ( - dao_get_letter_branding_or_platform_default, + dao_get_letter_branding_by_domain, dao_get_all_letter_branding, dao_create_letter_branding, dao_update_letter_branding @@ -8,36 +8,29 @@ from app.models import LetterBranding from tests.app.db import create_letter_branding -def test_dao_get_letter_branding_or_platform_default_returns_platform_default_if_domain_is_none(notify_db_session): - create_letter_branding() - result = dao_get_letter_branding_or_platform_default(domain=None) - assert result.filename == 'hm-government' +def test_dao_get_letter_branding_by_domain_returns_none_if_no_matching_domains(notify_db_session): + result = dao_get_letter_branding_by_domain(domain="test.domain") + assert not result -def test_dao_get_letter_branding_or_platform_default_if_domain_is_not_associated_with_a_brand(notify_db_session): - create_letter_branding() - result = dao_get_letter_branding_or_platform_default(domain="foo.bar") - assert result.filename == 'hm-government' - - -def test_dao_get_letter_branding_or_platform_default_returns_correct_brand_for_domain(notify_db_session): - create_letter_branding() +def test_dao_get_letter_branding_by_domain_returns_correct_brand_for_domain(notify_db_session): + create_letter_branding(domain='gov.uk') test_domain_branding = create_letter_branding( - name='test domain', filename='test-domain', domain='test.domain', platform_default=False + name='test domain', filename='test-domain', domain='test.domain' ) - result = dao_get_letter_branding_or_platform_default(domain='test.domain') + result = dao_get_letter_branding_by_domain(domain='test.domain') result == test_domain_branding def test_dao_get_all_letter_branding(notify_db_session): - platform_default = create_letter_branding() + hm_gov = create_letter_branding() test_domain = create_letter_branding( - name='test domain', filename='test-domain', domain='test.domain', platform_default=False + name='test domain', filename='test-domain', domain='test.domain' ) results = dao_get_all_letter_branding() - assert platform_default in results + assert hm_gov in results assert test_domain in results assert len(results) == 2 @@ -61,7 +54,6 @@ def test_dao_create_letter_branding(notify_db_session): assert new_letter_branding.name == data['name'] assert new_letter_branding.domain == data['domain'] assert new_letter_branding.filename == data['name'] - assert not new_letter_branding.platform_default def test_dao_update_letter_branding(notify_db_session): diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 23e05c62e..50efaf659 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -106,7 +106,7 @@ 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 + name='test domain', filename='test-domain', domain='test.domain' ) assert Service.query.count() == 0 service = Service(name="service_name", diff --git a/tests/app/db.py b/tests/app/db.py index 5426446f7..a58d1fbcb 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -727,11 +727,10 @@ def create_template_folder(service, name='foo', parent=None): return tf -def create_letter_branding(name='HM Government', filename='hm-government', domain=None, platform_default=True): +def create_letter_branding(name='HM Government', filename='hm-government', domain=None): test_domain_branding = LetterBranding(name=name, filename=filename, domain=domain, - platform_default=platform_default ) db.session.add(test_domain_branding) db.session.commit() diff --git a/tests/app/letter_branding/test_letter_branding_rest.py b/tests/app/letter_branding/test_letter_branding_rest.py index feb0f56a2..4ff066a35 100644 --- a/tests/app/letter_branding/test_letter_branding_rest.py +++ b/tests/app/letter_branding/test_letter_branding_rest.py @@ -1,6 +1,5 @@ import json -import pytest from app.models import LetterBranding from tests import create_authorization_header @@ -8,19 +7,21 @@ from tests.app.db import create_letter_branding def test_get_all_letter_brands(client, notify_db_session): - platform_default = create_letter_branding() + hm_gov = create_letter_branding() test_domain_branding = create_letter_branding( - name='test domain', filename='test-domain', domain='test.domain', platform_default=False + name='test domain', filename='test-domain', domain='test.domain' ) response = client.get('/letter-branding', headers=[create_authorization_header()]) assert response.status_code == 200 json_response = json.loads(response.get_data(as_text=True)) assert len(json_response) == 2 for brand in json_response: - if brand['id'] == platform_default: - platform_default.serialize() == brand + if brand['id'] == str(hm_gov.id): + assert hm_gov.serialize() == brand + elif brand['id'] == str(test_domain_branding.id): + assert test_domain_branding.serialize() == brand else: - test_domain_branding.serialize() == brand + assert False def test_create_letter_branding(client, notify_db_session): @@ -42,26 +43,6 @@ def test_create_letter_branding(client, notify_db_session): assert letter_brand.name == form['name'] assert letter_brand.domain == form['domain'] assert letter_brand.filename == form['filename'] - assert not letter_brand.platform_default - - -def test_create_letter_branding_returns_400_if_platform_default_is_passed_in_the_form(client, notify_db_session): - form = { - 'name': 'super brand', - 'domain': 'super.brand', - 'filename': 'super-brand', - 'platform_default': True - } - - response = client.post( - '/letter-branding', - data=json.dumps(form), - headers=[('Content-Type', 'application/json'), create_authorization_header()], - ) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['errors'][0]['message'] == \ - "Additional properties are not allowed (platform_default was unexpected)" def test_create_letter_branding_returns_400_if_name_already_exists(client, notify_db_session): @@ -102,5 +83,4 @@ def test_update_letter_branding_returns_400_when_integrity_error_is_thrown( assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) - # Why is this name and not domain? copied this pattern from email_branding assert json_resp['message'] == {"name": ["Duplicate domain 'duplicate'"]} diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 65fc2ac73..4d4233aad 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -219,7 +219,6 @@ 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), @@ -243,6 +242,7 @@ def test_create_service(client, sample_user): assert not json_resp['data']['research_mode'] assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['rate_limit'] == 3000 + assert json_resp['data']['letter_branding'] is None service_db = Service.query.get(json_resp['data']['id']) assert service_db.name == 'created service' @@ -261,12 +261,11 @@ 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 + name='test domain', filename='test-domain', domain='test.domain' ) data = { 'name': 'created service', @@ -286,6 +285,7 @@ def test_create_service_with_domain_sets_letter_branding(client, sample_user): headers=headers) json_resp = resp.json assert resp.status_code == 201 + assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['letter_branding'] == str(letter_branding.id) assert json_resp['data']['letter_logo_filename'] == str(letter_branding.filename) @@ -310,7 +310,7 @@ def test_create_service_when_letter_branding_is_empty(client, sample_user): headers=headers) json_resp = resp.json assert resp.status_code == 201 - assert not json_resp['data']['letter_branding'] + assert json_resp['data']['letter_branding'] is None assert json_resp['data']['letter_logo_filename'] == 'hm-government' @@ -318,7 +318,7 @@ 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 + name='test domain', filename='test-domain', domain='test.domain' ) data = { 'letter_branding': str(letter_branding.id) @@ -334,6 +334,7 @@ def test_get_service_by_id_returns_letter_branding_not_dvla_organisation( 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_branding'] == str(letter_branding.id) assert json_resp['data']['letter_logo_filename'] == 'test-domain' @@ -515,7 +516,7 @@ def test_update_service(client, notify_db, sample_service): 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) + letter_branding = create_letter_branding(name='test brand', filename='test-brand') data = { 'letter_branding': str(letter_branding.id) } @@ -532,6 +533,35 @@ def test_update_service_letter_branding(client, notify_db, sample_service): assert result['data']['letter_branding'] == str(letter_branding.id) +def test_update_service_remove_letter_branding(client, notify_db, sample_service): + letter_branding = create_letter_branding(name='test brand', filename='test-brand') + sample_service + data = { + 'letter_branding': str(letter_branding.id) + } + + auth_header = create_authorization_header() + + client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + data = { + 'letter_branding': None + } + 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'] is None + + 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