Catch itegrity errors and return 400.

When creating or updating an organisation an itegrity error is raise if the name is already used.
This change adds a new error handler for the organisation to catch the named unique index and return a 400 with a sensible message.
We have an other error handler for unique service names which was caught in the error handler for all blueprints. A new error handler for the service_blueprint has been created for catch those specific unique constraints.
This is a nice way to encapulate the specific errors for a specific blueprint.

.gitignore

@@ -72,3 +72,5 @@ celerybeat-schedule
 
 # CloudFoundry
 .cf
+
+/scripts/run_my_tests.sh

app/errors.py

@@ -3,7 +3,7 @@ from flask import (
     current_app,
     json)
 from notifications_utils.recipients import InvalidEmailError
-from sqlalchemy.exc import SQLAlchemyError, DataError
+from sqlalchemy.exc import DataError
 from sqlalchemy.orm.exc import NoResultFound
 from marshmallow import ValidationError
 from jsonschema import ValidationError as JsonSchemaValidationError
@@ -95,20 +95,6 @@ def register_errors(blueprint):
         current_app.logger.info(e)
         return jsonify(result='error', message="No result found"), 404
 
-    @blueprint.errorhandler(SQLAlchemyError)
-    def db_error(e):
-        if hasattr(e, 'orig') and hasattr(e.orig, 'pgerror') and e.orig.pgerror and \
-            ('duplicate key value violates unique constraint "services_name_key"' in e.orig.pgerror or
-             'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror):
-            return jsonify(
-                result='error',
-                message={'name': ["Duplicate service name '{}'".format(
-                    e.params.get('name', e.params.get('email_from', ''))
-                )]}
-            ), 400
-        current_app.logger.exception(e)
-        return jsonify(result='error', message="Internal server error"), 500
-
     # this must be defined after all other error handlers since it catches the generic Exception object
     @blueprint.app_errorhandler(500)
     @blueprint.errorhandler(Exception)

app/organisation/rest.py

@@ -1,4 +1,4 @@
-from flask import Blueprint, jsonify, request
+from flask import Blueprint, jsonify, request, current_app
 from sqlalchemy.exc import IntegrityError
 
 from app.dao.organisation_dao import (
@@ -26,12 +26,14 @@ register_errors(organisation_blueprint)
 @organisation_blueprint.errorhandler(IntegrityError)
 def handle_integrity_error(exc):
     """
-    Handle integrity errors caused by the unique contraint on ix_organisation_name
+    Handle integrity errors caused by the unique constraint on ix_organisation_name
     """
     if 'ix_organisation_name' in str(exc):
-        raise InvalidRequest('Organisation name already exists', 400)
+        return jsonify(result="error",
+                       message="Organisation name already exists"), 400
 
-    raise
+    current_app.logger.exception(exc)
+    return jsonify(result='error', message="Internal server error"), 500
 
 
 @organisation_blueprint.route('', methods=['GET'])
@@ -57,7 +59,6 @@ def create_organisation():
 
     organisation = Organisation(**data)
     dao_create_organisation(organisation)
-
     return jsonify(organisation.serialize()), 201
 
 
@@ -66,7 +67,6 @@ def update_organisation(organisation_id):
     data = request.get_json()
     validate(data, post_update_organisation_schema)
     result = dao_update_organisation(organisation_id, **data)
-
     if result:
         return '', 204
     else:

app/service/rest.py

@@ -7,6 +7,7 @@ from flask import (
     current_app,
     Blueprint
 )
+from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm.exc import NoResultFound
 
 from app.dao import notifications_dao
@@ -94,6 +95,22 @@ service_blueprint = Blueprint('service', __name__)
 register_errors(service_blueprint)
 
 
+@service_blueprint.errorhandler(IntegrityError)
+def handle_integrity_error(exc):
+    """
+    Handle integrity errors caused by the unique constraint on ix_organisation_name
+    """
+    if 'services_name_key' or 'services_email_from_key' in str(exc):
+        return jsonify(
+            result='error',
+            message={'name': ["Duplicate service name '{}'".format(
+                exc.params.get('name', exc.params.get('email_from', ''))
+            )]}
+        ), 400
+    current_app.logger.exception(exc)
+    return jsonify(result='error', message="Internal server error"), 500
+
+
 @service_blueprint.route('/platform-stats', methods=['GET'])
 def get_platform_stats():
     include_from_test_key = request.args.get('include_from_test_key', 'True') != 'False'

tests/app/organisation/test_rest.py

@@ -1,6 +1,3 @@
-import pytest
-
-from app.errors import InvalidRequest
 from app.models import Organisation
 from app.dao.organisation_dao import dao_add_service_to_organisation
 from tests.app.db import create_organisation, create_service
@@ -62,17 +59,16 @@ def test_post_create_organisation_existing_name_raises_400(admin_request, sample
         'active': True
     }
 
-    with pytest.raises(InvalidRequest) as e:
-        admin_request.post(
-            'organisation.create_organisation',
-            _data=data,
-            _expected_status=400
-        )
+    response = admin_request.post(
+        'organisation.create_organisation',
+        _data=data,
+        _expected_status=400
+    )
 
     organisation = Organisation.query.all()
 
     assert len(organisation) == 1
-    assert 'Organisation name already exists' in str(e.value)
+    assert response['message'] == 'Organisation name already exists'
 
 
 def test_post_create_organisation_with_missing_name_gives_validation_error(admin_request, notify_db_session):
@@ -114,22 +110,21 @@ def test_post_update_organisation_updates_fields(admin_request, notify_db_sessio
 
 
 def test_post_update_organisation_raises_400_on_existing_org_name(
-        admin_request, notify_db_session, sample_organisation):
+        admin_request, sample_organisation):
     org = create_organisation()
     data = {
         'name': sample_organisation.name,
         'active': False
     }
 
-    with pytest.raises(expected_exception=InvalidRequest) as e:
-        admin_request.post(
-            'organisation.update_organisation',
-            _data=data,
-            organisation_id=org.id,
-            _expected_status=400
-        )
+    response = admin_request.post(
+        'organisation.update_organisation',
+        _data=data,
+        organisation_id=org.id,
+        _expected_status=400
+    )
 
-    assert 'Organisation name already exists' in str(e.value)
+    assert response['message'] == 'Organisation name already exists'
 
 
 def test_post_update_organisation_gives_404_status_if_org_does_not_exist(admin_request, notify_db_session):