Merge pull request #2580 from alphagov/set-bad-email-addresses-to-perm-fail

Set bad email addresses to permanent-failure

app/__init__.py

@@ -114,7 +114,6 @@ def register_blueprint(application):
     from app.events.rest import events as events_blueprint
     from app.provider_details.rest import provider_details as provider_details_blueprint
     from app.email_branding.rest import email_branding_blueprint
-    from app.delivery.rest import delivery_blueprint
     from app.inbound_number.rest import inbound_number_blueprint
     from app.inbound_sms.rest import inbound_sms as inbound_sms_blueprint
     from app.notifications.receive_notifications import receive_notifications_blueprint
@@ -160,9 +159,6 @@ def register_blueprint(application):
     invite_blueprint.before_request(requires_admin_auth)
     application.register_blueprint(invite_blueprint)
 
-    delivery_blueprint.before_request(requires_admin_auth)
-    application.register_blueprint(delivery_blueprint)
-
     inbound_number_blueprint.before_request(requires_admin_auth)
     application.register_blueprint(inbound_number_blueprint)
 

app/celery/provider_tasks.py

@@ -9,7 +9,7 @@ from app.dao import notifications_dao
 from app.dao.notifications_dao import update_notification_status_by_id
 from app.delivery import send_to_providers
 from app.exceptions import NotificationTechnicalFailureException
-from app.models import NOTIFICATION_TECHNICAL_FAILURE
+from app.models import NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_TECHNICAL_FAILURE
 
 
 @notify_celery.task(bind=True, name="deliver_sms", max_retries=48, default_retry_delay=300)
@@ -47,8 +47,8 @@ def deliver_email(self, notification_id):
             raise NoResultFound()
         send_to_providers.send_email_to_provider(notification)
     except InvalidEmailError as e:
-        current_app.logger.exception(e)
+        current_app.logger.info(e)
-        update_notification_status_by_id(notification_id, 'technical-failure')
+        update_notification_status_by_id(notification_id, NOTIFICATION_PERMANENT_FAILURE)
     except Exception:
         try:
             current_app.logger.exception(

app/delivery/rest.py

@@ -1,48 +0,0 @@
-from flask import Blueprint, jsonify, current_app
-
-from app.config import QueueNames
-from app.delivery import send_to_providers
-from app.models import EMAIL_TYPE
-from app.celery import provider_tasks
-from app.dao import notifications_dao
-from app.errors import register_errors
-
-delivery_blueprint = Blueprint('delivery', __name__)
-
-
-register_errors(delivery_blueprint)
-
-
-@delivery_blueprint.route('/deliver/notification/<uuid:notification_id>', methods=['POST'])
-def send_notification_to_provider(notification_id):
-    notification = notifications_dao.get_notification_by_id(notification_id)
-    if not notification:
-        return jsonify({"result": "error", "message": "No result found"}), 404
-
-    if notification.notification_type == EMAIL_TYPE:
-        send_response(
-            send_to_providers.send_email_to_provider,
-            provider_tasks.deliver_email,
-            notification,
-            QueueNames.SEND_EMAIL
-        )
-    else:
-        send_response(
-            send_to_providers.send_sms_to_provider,
-            provider_tasks.deliver_sms,
-            notification,
-            QueueNames.SEND_SMS
-        )
-    return jsonify({}), 204
-
-
-def send_response(send_call, task_call, notification, queue):
-    try:
-        send_call(notification)
-    except Exception as e:
-        current_app.logger.exception(
-            "Failed to send notification, retrying in celery. ID {} type {}".format(
-                notification.id,
-                notification.notification_type),
-            e)
-        task_call.apply_async((str(notification.id)), queue=queue)

app/models.py

@@ -352,7 +352,7 @@ class Organisation(db.Model):
 
     services = db.relationship(
         'Service',
-        secondary='organisation_to_service',
+        secondary='services',
         uselist=True)
 
     agreement_signed = db.Column(db.Boolean, nullable=True)
@@ -479,11 +479,8 @@ class Service(db.Model, Versioned):
     go_live_user = db.relationship('User', foreign_keys=[go_live_user_id])
     go_live_at = db.Column(db.DateTime, nullable=True)
 
-    organisation = db.relationship(
+    organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True)
-        'Organisation',
+    organisation = db.relationship('Organisation', foreign_keys=[organisation_id])
-        secondary=organisation_to_service,
-        uselist=False,
-        single_parent=True)
 
     email_branding = db.relationship(
         'EmailBranding',

migrations/versions/0302_add_org_id_to_services.py

@@ -0,0 +1,29 @@
+"""
+
+Revision ID: 0302_add_org_id_to_services
+Revises: 0301_upload_letters_permission
+Create Date: 2019-08-06 09:43:57.993510
+
+"""
+from alembic import op
+import sqlalchemy as sa
+from sqlalchemy.dialects import postgresql
+
+revision = '0302_add_org_id_to_services'
+down_revision = '0301_upload_letters_permission'
+
+
+def upgrade():
+    op.add_column('services', sa.Column('organisation_id', postgresql.UUID(as_uuid=True), nullable=True))
+    op.create_index(op.f('ix_services_organisation_id'), 'services', ['organisation_id'], unique=False)
+    op.create_foreign_key("fk_service_organisation", 'services', 'organisation', ['organisation_id'], ['id'])
+    op.add_column('services_history', sa.Column('organisation_id', postgresql.UUID(as_uuid=True), nullable=True))
+    op.create_index(op.f('ix_services_history_organisation_id'), 'services_history', ['organisation_id'], unique=False)
+
+
+def downgrade():
+    op.drop_index(op.f('ix_services_history_organisation_id'), table_name='services_history')
+    op.drop_column('services_history', 'organisation_id')
+    op.drop_constraint("fk_service_organisation", 'services', type_='foreignkey')
+    op.drop_index(op.f('ix_services_organisation_id'), table_name='services')
+    op.drop_column('services', 'organisation_id')

tests/app/celery/test_provider_tasks.py

@@ -84,14 +84,14 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task
     assert str(sample_notification.id) in e.value.message
 
 
-def test_should_technical_error_and_not_retry_if_invalid_email(sample_notification, mocker):
+def test_should_go_into_permanent_failure_and_not_retry_if_invalid_email(sample_notification, mocker):
     mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=InvalidEmailError('bad email'))
     mocker.patch('app.celery.provider_tasks.deliver_email.retry')
 
     deliver_email(sample_notification.id)
 
     assert provider_tasks.deliver_email.retry.called is False
-    assert sample_notification.status == 'technical-failure'
+    assert sample_notification.status == 'permanent-failure'
 
 
 def test_should_retry_and_log_exception(sample_notification, mocker):

tests/app/delivery/test_rest.py

@@ -1,105 +0,0 @@
-from flask import json
-
-import app
-from tests import create_authorization_header
-
-
-def test_should_reject_if_not_authenticated(notify_api):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            response = client.post('/deliver/notification/{}'.format(app.create_uuid()))
-            assert response.status_code == 401
-
-
-def test_should_reject_if_invalid_uuid(notify_api):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            auth = create_authorization_header()
-            response = client.post(
-                '/deliver/notification/{}',
-                headers=[auth]
-            )
-        body = json.loads(response.get_data(as_text=True))
-        assert response.status_code == 404
-        assert body['message'] == 'The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again.'  # noqa
-        assert body['result'] == 'error'
-
-
-def test_should_reject_if_notification_id_cannot_be_found(notify_api):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            auth = create_authorization_header()
-            response = client.post(
-                '/deliver/notification/{}'.format(app.create_uuid()),
-                headers=[auth]
-            )
-        body = json.loads(response.get_data(as_text=True))
-        assert response.status_code == 404
-        assert body['message'] == 'No result found'
-        assert body['result'] == 'error'
-
-
-def test_should_call_send_sms_to_provider_as_primary(notify_api, sample_notification, mocker):
-    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            auth = create_authorization_header()
-            response = client.post(
-                '/deliver/notification/{}'.format(sample_notification.id),
-                headers=[auth]
-            )
-            app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification)
-            assert response.status_code == 204
-
-
-def test_should_call_send_email_to_provider_as_primary(notify_api, sample_email_notification, mocker):
-    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            auth = create_authorization_header()
-            response = client.post(
-                '/deliver/notification/{}'.format(sample_email_notification.id),
-                headers=[auth]
-            )
-        app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification)
-        assert response.status_code == 204
-
-
-def test_should_call_deliver_sms_task_if_send_sms_to_provider_fails(notify_api, sample_notification, mocker):
-    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception())
-    mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
-
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            auth = create_authorization_header()
-            response = client.post(
-                '/deliver/notification/{}'.format(sample_notification.id),
-                headers=[auth]
-            )
-        app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification)
-        app.celery.provider_tasks.deliver_sms.apply_async.assert_called_with(
-            (str(sample_notification.id)), queue='send-sms-tasks'
-        )
-        assert response.status_code == 204
-
-
-def test_should_call_deliver_email_task_if_send_email_to_provider_fails(
-        notify_api,
-        sample_email_notification,
-        mocker
-):
-    mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception())
-    mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
-
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            auth = create_authorization_header()
-            response = client.post(
-                '/deliver/notification/{}'.format(sample_email_notification.id),
-                headers=[auth]
-            )
-        app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification)
-        app.celery.provider_tasks.deliver_email.apply_async.assert_called_with(
-            (str(sample_email_notification.id)), queue='send-email-tasks'
-        )
-        assert response.status_code == 204