From 5d5047fb13673903849ed7fc281d08abab6cd131 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 4 Sep 2018 10:57:47 +0100 Subject: [PATCH 1/2] Add unique constraint to email branding domains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Does two things: 1. Revert "Revert "Add unique constraint to email branding domain"" This reverts commit af9cb30ef30fe3859507a5747a65255b7466af4b. 2. Don’t allow empty string in email branding domain Columns with multiple `null`s can have a uniqueness constraint. Columns with multiple empty string values are not considered unique. This commit: - removes any duplicate empty string values - casts empty strings to null string any time these columns are updated --- Squashed into this single commits because these two things are not atomic as individual commits. --- app/dao/email_branding_dao.py | 2 +- app/models.py | 2 +- .../versions/0223_add_domain_constraint.py | 28 +++++++++++++++++++ tests/app/dao/test_email_branding_dao.py | 12 ++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0223_add_domain_constraint.py diff --git a/app/dao/email_branding_dao.py b/app/dao/email_branding_dao.py index bcf4a93c8..87ccc61cf 100644 --- a/app/dao/email_branding_dao.py +++ b/app/dao/email_branding_dao.py @@ -23,5 +23,5 @@ def dao_create_email_branding(email_branding): @transactional def dao_update_email_branding(email_branding, **kwargs): for key, value in kwargs.items(): - setattr(email_branding, key, value) + setattr(email_branding, key, value or None) db.session.add(email_branding) 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..8d3f808dc --- /dev/null +++ b/migrations/versions/0223_add_domain_constraint.py @@ -0,0 +1,28 @@ +""" +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.execute(""" + update + email_branding + set + domain = null + where + domain = '' + """) + 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/tests/app/dao/test_email_branding_dao.py b/tests/app/dao/test_email_branding_dao.py index fc150beca..5f741da3e 100644 --- a/tests/app/dao/test_email_branding_dao.py +++ b/tests/app/dao/test_email_branding_dao.py @@ -51,3 +51,15 @@ def test_update_email_branding(notify_db, notify_db_session): assert len(email_branding) == 1 assert email_branding[0].name == updated_name + + +def test_domain_cant_be_empty_string(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 From d52ee4606a2e173dd8f6fc4341c91d238796a009 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 4 Sep 2018 12:59:53 +0100 Subject: [PATCH 2/2] Add error handler for duplicate domain --- app/email_branding/rest.py | 19 ++++++++++++++++++- tests/app/email_branding/test_rest.py | 27 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/app/email_branding/rest.py b/app/email_branding/rest.py index 0991c9d0b..990f1905e 100644 --- a/app/email_branding/rest.py +++ b/app/email_branding/rest.py @@ -1,4 +1,5 @@ -from flask import Blueprint, jsonify, request +from flask import Blueprint, current_app, jsonify, request +from sqlalchemy.exc import IntegrityError from app.dao.email_branding_dao import ( dao_create_email_branding, @@ -18,6 +19,22 @@ 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/tests/app/email_branding/test_rest.py b/tests/app/email_branding/test_rest.py index 03311514a..7c7954887 100644 --- a/tests/app/email_branding/test_rest.py +++ b/tests/app/email_branding/test_rest.py @@ -257,3 +257,30 @@ 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() + branding_2 = create_email_branding() + 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'}, + _expected_status=400, + ) + assert response['result'] == 'error' + assert response['message']['name'] == ["Duplicate domain 'example.com'"]