From ee966668bd186c991aa048010c68f817100363e8 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 5 Apr 2019 16:26:52 +0100
Subject: [PATCH] Remove domain columns from branding table
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This relationship is via the `Organisation` now; we don’t use this
column to fudge a relationship based on the user’s email address and the
matching something in these columns.
---
 app/dao/letter_branding_dao.py                |  8 ---
 app/email_branding/email_branding_schema.py   |  2 -
 app/email_branding/rest.py                    | 19 +-----
 app/letter_branding/letter_branding_rest.py   |  2 +-
 app/letter_branding/letter_branding_schema.py |  3 +-
 app/models.py                                 |  4 --
 app/service/rest.py                           |  7 +--
 tests/app/dao/test_email_branding_dao.py      | 10 +--
 tests/app/dao/test_letter_branding_dao.py     | 23 +------
 tests/app/dao/test_services_dao.py            |  2 +-
 tests/app/db.py                               |  3 +-
 tests/app/email_branding/test_rest.py         | 36 +----------
 .../test_letter_branding_rest.py              | 36 +++--------
 tests/app/service/test_rest.py                | 63 -------------------
 14 files changed, 23 insertions(+), 195 deletions(-)

diff --git a/app/dao/letter_branding_dao.py b/app/dao/letter_branding_dao.py
index 1011e392e..4a140412c 100644
--- a/app/dao/letter_branding_dao.py
+++ b/app/dao/letter_branding_dao.py
@@ -11,14 +11,6 @@ def dao_get_letter_branding_by_name(letter_branding_name):
     return LetterBranding.query.filter_by(name=letter_branding_name).first()
 
 
-def dao_get_letter_branding_by_domain(domain):
-    if not domain:
-        return None
-    return LetterBranding.query.filter(
-        LetterBranding.domain == domain
-    ).first()
-
-
 def dao_get_all_letter_branding():
     return LetterBranding.query.order_by(LetterBranding.name).all()
 
diff --git a/app/email_branding/email_branding_schema.py b/app/email_branding/email_branding_schema.py
index 2f562d2ae..c8fec0c81 100644
--- a/app/email_branding/email_branding_schema.py
+++ b/app/email_branding/email_branding_schema.py
@@ -9,7 +9,6 @@ post_create_email_branding_schema = {
         "name": {"type": "string"},
         "text": {"type": ["string", "null"]},
         "logo": {"type": ["string", "null"]},
-        "domain": {"type": ["string", "null"]},
         "brand_type": {"enum": BRANDING_TYPES},
     },
     "required": ["name"]
@@ -24,7 +23,6 @@ post_update_email_branding_schema = {
         "name": {"type": ["string", "null"]},
         "text": {"type": ["string", "null"]},
         "logo": {"type": ["string", "null"]},
-        "domain": {"type": ["string", "null"]},
         "brand_type": {"enum": BRANDING_TYPES},
     },
     "required": []
diff --git a/app/email_branding/rest.py b/app/email_branding/rest.py
index 990f1905e..0991c9d0b 100644
--- a/app/email_branding/rest.py
+++ b/app/email_branding/rest.py
@@ -1,5 +1,4 @@
-from flask import Blueprint, current_app, jsonify, request
-from sqlalchemy.exc import IntegrityError
+from flask import Blueprint, jsonify, request
 
 from app.dao.email_branding_dao import (
     dao_create_email_branding,
@@ -19,22 +18,6 @@ email_branding_blueprint = Blueprint('email_branding', __name__)
 register_errors(email_branding_blueprint)
 
 
-@email_branding_blueprint.errorhandler(IntegrityError)
-def handle_integrity_error(exc):
-    """
-    Handle integrity errors caused by the unique constraint on domain
-    """
-    if 'domain' in str(exc):
-        return jsonify(
-            result='error',
-            message={'name': ["Duplicate domain '{}'".format(
-                exc.params.get('domain')
-            )]}
-        ), 400
-    current_app.logger.exception(exc)
-    return jsonify(result='error', message="Internal server error"), 500
-
-
 @email_branding_blueprint.route('', methods=['GET'])
 def get_email_branding_options():
     email_branding_options = [o.serialize() for o in dao_get_email_branding_options()]
diff --git a/app/letter_branding/letter_branding_rest.py b/app/letter_branding/letter_branding_rest.py
index 2c105da67..c07987b3b 100644
--- a/app/letter_branding/letter_branding_rest.py
+++ b/app/letter_branding/letter_branding_rest.py
@@ -22,7 +22,7 @@ def handle_integrity_error(exc):
     """
     Handle integrity errors caused by the unique constraint
     """
-    for col in {'domain', 'name', 'filename'}:
+    for col in {'name', 'filename'}:
         if 'letter_branding_{}_key'.format(col) in str(exc):
             return jsonify(
                 result='error',
diff --git a/app/letter_branding/letter_branding_schema.py b/app/letter_branding/letter_branding_schema.py
index 5f91bf829..e13ef64b6 100644
--- a/app/letter_branding/letter_branding_schema.py
+++ b/app/letter_branding/letter_branding_schema.py
@@ -5,7 +5,6 @@ post_letter_branding_schema = {
     "properties": {
         "name": {"type": ["string", "null"]},
         "filename": {"type": ["string", "null"]},
-        "domain": {"type": ["string", "null"]},
     },
-    "required": ("name", "filename", "domain")
+    "required": ("name", "filename")
 }
diff --git a/app/models.py b/app/models.py
index 41ba0398f..50c25330f 100644
--- a/app/models.py
+++ b/app/models.py
@@ -221,7 +221,6 @@ class EmailBranding(db.Model):
     logo = db.Column(db.String(255), nullable=True)
     name = db.Column(db.String(255), unique=True, nullable=False)
     text = db.Column(db.String(255), nullable=True)
-    domain = db.Column(db.Text, unique=True, nullable=True)
     brand_type = db.Column(
         db.String(255),
         db.ForeignKey('branding_type.name'),
@@ -237,7 +236,6 @@ class EmailBranding(db.Model):
             "logo": self.logo,
             "name": self.name,
             "text": self.text,
-            "domain": self.domain,
             "brand_type": self.brand_type
         }
 
@@ -258,14 +256,12 @@ class LetterBranding(db.Model):
     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)
 
     def serialize(self):
         return {
             "id": str(self.id),
             "name": self.name,
             "filename": self.filename,
-            "domain": self.domain,
         }
 
 
diff --git a/app/service/rest.py b/app/service/rest.py
index ecaa3a58b..828dcfdaa 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -27,7 +27,6 @@ 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_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,
@@ -186,7 +185,8 @@ 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)
+    data.pop('service_domain', None)
+
     # validate json with marshmallow
     service_schema.load(data)
 
@@ -195,8 +195,7 @@ def create_service():
     # unpack valid json into service object
     valid_service = Service.from_json(data)
 
-    letter_branding = dao_get_letter_branding_by_domain(domain)
-    dao_create_service(valid_service, user, letter_branding=letter_branding)
+    dao_create_service(valid_service, user)
 
     return jsonify(data=service_schema.dump(valid_service).data), 201
 
diff --git a/tests/app/dao/test_email_branding_dao.py b/tests/app/dao/test_email_branding_dao.py
index 5f741da3e..9e47093d9 100644
--- a/tests/app/dao/test_email_branding_dao.py
+++ b/tests/app/dao/test_email_branding_dao.py
@@ -53,13 +53,7 @@ def test_update_email_branding(notify_db, notify_db_session):
     assert email_branding[0].name == updated_name
 
 
-def test_domain_cant_be_empty_string(notify_db, notify_db_session):
+def test_email_branding_has_no_domain(notify_db, notify_db_session):
     create_email_branding()
-
     email_branding = EmailBranding.query.all()
-    assert email_branding[0].domain is None
-
-    dao_update_email_branding(email_branding[0], domain='')
-
-    email_branding = EmailBranding.query.all()
-    assert email_branding[0].domain is None
+    assert not hasattr(email_branding, 'domain')
diff --git a/tests/app/dao/test_letter_branding_dao.py b/tests/app/dao/test_letter_branding_dao.py
index d8e056ef7..effc9efc8 100644
--- a/tests/app/dao/test_letter_branding_dao.py
+++ b/tests/app/dao/test_letter_branding_dao.py
@@ -4,7 +4,6 @@ import pytest
 from sqlalchemy.exc import SQLAlchemyError
 
 from app.dao.letter_branding_dao import (
-    dao_get_letter_branding_by_domain,
     dao_get_all_letter_branding,
     dao_create_letter_branding,
     dao_update_letter_branding,
@@ -26,30 +25,16 @@ def test_dao_get_letter_brand_by_id_raises_exception_if_does_not_exist(notify_db
         dao_get_letter_branding_by_id(uuid.uuid4())
 
 
-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_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'
-    )
-    result = dao_get_letter_branding_by_domain(domain='test.domain')
-    result == test_domain_branding
-
-
 def test_dao_get_all_letter_branding(notify_db_session):
     hm_gov = create_letter_branding()
-    test_domain = create_letter_branding(
-        name='test domain', filename='test-domain', domain='test.domain'
+    test_branding = create_letter_branding(
+        name='test branding', filename='test-branding',
     )
 
     results = dao_get_all_letter_branding()
 
     assert hm_gov in results
-    assert test_domain in results
+    assert test_branding in results
     assert len(results) == 2
 
 
@@ -60,7 +45,6 @@ def test_dao_get_all_letter_branding_returns_empty_list_if_no_brands_exist(notif
 def test_dao_create_letter_branding(notify_db_session):
     data = {
         'name': 'test-logo',
-        'domain': 'test.co.uk',
         'filename': 'test-logo'
     }
     assert LetterBranding.query.count() == 0
@@ -70,7 +54,6 @@ def test_dao_create_letter_branding(notify_db_session):
 
     new_letter_branding = LetterBranding.query.first()
     assert new_letter_branding.name == data['name']
-    assert new_letter_branding.domain == data['domain']
     assert new_letter_branding.filename == data['name']
 
 
diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py
index 29da6d7eb..4a79d1b20 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'
+        name='test domain', filename='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 8b3702794..bac88fe59 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -774,10 +774,9 @@ def create_template_folder(service, name='foo', parent=None):
     return tf
 
 
-def create_letter_branding(name='HM Government', filename='hm-government', domain=None):
+def create_letter_branding(name='HM Government', filename='hm-government'):
     test_domain_branding = LetterBranding(name=name,
                                           filename=filename,
-                                          domain=domain,
                                           )
     db.session.add(test_domain_branding)
     db.session.commit()
diff --git a/tests/app/email_branding/test_rest.py b/tests/app/email_branding/test_rest.py
index 0a2655eee..df2208fde 100644
--- a/tests/app/email_branding/test_rest.py
+++ b/tests/app/email_branding/test_rest.py
@@ -34,7 +34,7 @@ def test_get_email_branding_by_id(admin_request, notify_db, notify_db_session):
     )
 
     assert set(response['email_branding'].keys()) == {'colour', 'logo', 'name', 'id', 'text',
-                                                      'domain', 'brand_type'}
+                                                      'brand_type'}
     assert response['email_branding']['colour'] == '#FFFFFF'
     assert response['email_branding']['logo'] == '/path/image.png'
     assert response['email_branding']['name'] == 'Some Org'
@@ -48,7 +48,6 @@ def test_post_create_email_branding(admin_request, notify_db_session):
         'name': 'test email_branding',
         'colour': '#0000ff',
         'logo': '/images/test_x2.png',
-        'domain': 'gov.uk',
         'brand_type': BRANDING_ORG
     }
     response = admin_request.post(
@@ -60,7 +59,6 @@ def test_post_create_email_branding(admin_request, notify_db_session):
     assert data['colour'] == response['data']['colour']
     assert data['logo'] == response['data']['logo']
     assert data['name'] == response['data']['text']
-    assert data['domain'] == response['data']['domain']
     assert data['brand_type'] == response['data']['brand_type']
 
 
@@ -69,7 +67,6 @@ def test_post_create_email_branding_without_brand_type_defaults(admin_request, n
         'name': 'test email_branding',
         'colour': '#0000ff',
         'logo': '/images/test_x2.png',
-        'domain': 'gov.uk',
     }
     response = admin_request.post(
         'email_branding.create_email_branding',
@@ -181,8 +178,8 @@ def test_post_create_email_branding_returns_400_when_name_is_missing(admin_reque
     ({'name': 'test email_branding 1'}),
     ({'logo': 'images/text_x3.png', 'colour': '#ffffff'}),
     ({'logo': 'images/text_x3.png'}),
-    ({'logo': 'images/text_x3.png', 'domain': 'gov.uk'}),
-    ({'logo': 'images/text_x3.png', 'brand_type': 'org'}),
+    ({'logo': 'images/text_x3.png'}),
+    ({'logo': 'images/text_x3.png'}),
 ])
 def test_post_update_email_branding_updates_field(admin_request, notify_db_session, data_update):
     data = {
@@ -273,30 +270,3 @@ def test_update_email_branding_reject_invalid_brand_type(admin_request, notify_d
     )
 
     assert response['errors'][0]['message'] == 'brand_type NOT A TYPE is not one of [org, both, org_banner]'
-
-
-def test_400_for_duplicate_domain(admin_request, notify_db_session):
-    branding_1 = create_email_branding(name='first brand')
-    branding_2 = create_email_branding(name='second brand')
-    admin_request.post(
-        'email_branding.update_email_branding',
-        _data={'domain': 'example.com', },
-        email_branding_id=branding_1.id,
-    )
-
-    response = admin_request.post(
-        'email_branding.update_email_branding',
-        _data={'domain': 'example.com'},
-        email_branding_id=branding_2.id,
-        _expected_status=400,
-    )
-    assert response['result'] == 'error'
-    assert response['message']['name'] == ["Duplicate domain 'example.com'"]
-
-    response = admin_request.post(
-        'email_branding.create_email_branding',
-        _data={'domain': 'example.com', 'name': 'another brand'},
-        _expected_status=400,
-    )
-    assert response['result'] == 'error'
-    assert response['message']['name'] == ["Duplicate domain 'example.com'"]
diff --git a/tests/app/letter_branding/test_letter_branding_rest.py b/tests/app/letter_branding/test_letter_branding_rest.py
index f83f569b5..ef4211919 100644
--- a/tests/app/letter_branding/test_letter_branding_rest.py
+++ b/tests/app/letter_branding/test_letter_branding_rest.py
@@ -8,8 +8,8 @@ from tests.app.db import create_letter_branding
 
 def test_get_all_letter_brands(client, notify_db_session):
     hm_gov = create_letter_branding()
-    test_domain_branding = create_letter_branding(
-        name='test domain', filename='test-domain', domain='test.domain'
+    test_branding = create_letter_branding(
+        name='test branding', filename='test-branding',
     )
     response = client.get('/letter-branding', headers=[create_authorization_header()])
     assert response.status_code == 200
@@ -18,8 +18,8 @@ def test_get_all_letter_brands(client, notify_db_session):
     for brand in json_response:
         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
+        elif brand['id'] == str(test_branding.id):
+            assert test_branding.serialize() == brand
         else:
             assert False
 
@@ -27,7 +27,7 @@ def test_get_all_letter_brands(client, notify_db_session):
 def test_get_letter_branding_by_id(client, notify_db_session):
     hm_gov = create_letter_branding()
     create_letter_branding(
-        name='test domain', filename='test-domain', domain='test.domain'
+        name='test domain', filename='test-domain'
     )
     response = client.get('/letter-branding/{}'.format(hm_gov.id), headers=[create_authorization_header()])
 
@@ -43,7 +43,6 @@ def test_get_letter_branding_by_id_returns_404_if_does_not_exist(client, notify_
 def test_create_letter_branding(client, notify_db_session):
     form = {
         'name': 'super brand',
-        'domain': 'super.brand',
         'filename': 'super-brand'
     }
 
@@ -57,37 +56,16 @@ def test_create_letter_branding(client, notify_db_session):
     json_response = json.loads(response.get_data(as_text=True))
     letter_brand = LetterBranding.query.get(json_response['id'])
     assert letter_brand.name == form['name']
-    assert letter_brand.domain == form['domain']
     assert letter_brand.filename == form['filename']
 
 
-def test_create_letter_branding_returns_400_if_domain_already_exists(client, notify_db_session):
-    create_letter_branding(name='duplicate', domain='duplicate', filename='duplicate')
-    form = {
-        'name': 'super brand',
-        'domain': 'duplicate',
-        'filename': 'super-brand',
-    }
-
-    response = client.post(
-        '/letter-branding',
-        headers=[('Content-Type', 'application/json'), create_authorization_header()],
-        data=json.dumps(form)
-    )
-
-    assert response.status_code == 400
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert json_resp['message'] == {'domain': ["Domain already in use"]}
-
-
 def test_update_letter_branding_returns_400_when_integrity_error_is_thrown(
         client, notify_db_session
 ):
-    create_letter_branding(name='duplicate', domain='duplicate', filename='duplicate')
-    brand_to_update = create_letter_branding(name='super brand', domain='super brand', filename='super brand')
+    create_letter_branding(name='duplicate', filename='duplicate')
+    brand_to_update = create_letter_branding(name='super brand', filename='super brand')
     form = {
         'name': 'duplicate',
-        'domain': 'super brand',
         'filename': 'super-brand',
     }
 
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 0be65e94d..75b03f730 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -349,69 +349,6 @@ def test_create_service_inherits_branding_from_organisation(
     assert json_resp['data']['letter_branding'] == str(letter_branding.id)
 
 
-def test_create_service_with_domain_sets_letter_branding(admin_request, sample_user):
-    letter_branding = create_letter_branding(
-        name='test domain', filename='test-domain', domain='test.domain'
-    )
-    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
-    }
-    json_resp = admin_request.post('service.create_service', _data=data, _expected_status=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_with_no_domain_doesnt_set_letter_branding(admin_request, sample_user):
-    create_letter_branding(name='no domain', filename='no-domain', domain=None)
-    create_letter_branding(name='test domain', filename='test-domain', domain='test.domain')
-
-    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': None
-    }
-
-    json_resp = admin_request.post('service.create_service', _data=data, _expected_status=201)
-
-    assert json_resp['data']['letter_branding'] is None
-    assert json_resp['data']['letter_logo_filename'] is None
-
-
-def test_get_service_by_id_returns_letter_branding(
-        client, sample_service
-):
-    letter_branding = create_letter_branding(
-        name='test domain', filename='test-domain', domain='test.domain'
-    )
-    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']['letter_branding'] == str(letter_branding.id)
-    assert json_resp['data']['letter_logo_filename'] == 'test-domain'
-
-
 def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_uuid):
     with notify_api.test_request_context():
         with notify_api.test_client() as client: