diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 5d292f7b7..6c9706ec8 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -10,7 +10,7 @@ from notifications_utils.recipients import ( from notifications_utils.template import Template from sqlalchemy.exc import SQLAlchemyError -from app import clients, statsd_client +from app import statsd_client from app import ( create_uuid, DATETIME_FORMAT, @@ -28,7 +28,6 @@ from app.dao.notifications_dao import ( dao_create_notification, dao_get_notification_statistics_for_service_and_day ) -from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id from app.models import ( @@ -202,7 +201,7 @@ def _save_notification(created_at, notification, notification_id, service_id, no status='created', created_at=datetime.strptime(created_at, DATETIME_FORMAT), personalisation=notification.get('personalisation'), - notification_type=EMAIL_TYPE, + notification_type=notification_type, api_key_id=api_key_id, key_type=key_type ) @@ -219,17 +218,3 @@ def service_allowed_to_send_to(recipient, service): [user.mobile_number, user.email_address] for user in service.users ) ) - - -def provider_to_use(notification_type, notification_id): - active_providers_in_order = [ - provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active - ] - - if not active_providers_in_order: - current_app.logger.error( - "{} {} failed as no active providers".format(notification_type, notification_id) - ) - raise Exception("No active {} providers".format(notification_type)) - - return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) diff --git a/migrations/versions/0039_fix_notifications.py b/migrations/versions/0039_fix_notifications.py new file mode 100644 index 000000000..14c509c49 --- /dev/null +++ b/migrations/versions/0039_fix_notifications.py @@ -0,0 +1,44 @@ +"""empty message + +Revision ID: 0039_fix_notifications +Revises: 0038_test_api_key_type +Create Date: 2016-07-06 13:28:48.381278 + +""" + +# revision identifiers, used by Alembic. +revision = '0039_fix_notifications' +down_revision = '0038_test_api_key_type' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.execute('update notifications set notification_type = (select cast(cast(template_type as text) as notification_type) from templates where templates.id= notifications.template_id)') + conn = op.get_bind() + del_sql = "select n.id, results.* from (select 'failed' as stat_type, count(status) as count, notification_type, date(created_at) as day, service_id " \ + "from notifications where status in ('temporary-failure', 'permanent-failure', 'technical-failure') group by service_id, notification_type, date(created_at) " \ + "union select 'delivered' as stat_type, count(status) , notification_type, date(created_at) as day, service_id " \ + "from notifications where status in ('delivered') group by service_id, notification_type, date(created_at)) as results, " \ + "notification_statistics n " \ + "where n.day = results.day and n.service_id = results.service_id order by results.day;" + + results = conn.execute(del_sql) + res = results.fetchall() + + for x in res: + if x.stat_type == 'delivered' and x.notification_type == 'email': + op.execute("update notification_statistics set emails_delivered = {} where id = '{}'".format(x.count, x.id)) + if x.stat_type == 'failed' and x.notification_type == 'email': + op.execute("update notification_statistics set emails_failed = {} where id = '{}'".format(x.count, x.id)) + if x.stat_type == 'delivered' and x.notification_type == 'sms': + op.execute("update notification_statistics set sms_delivered = {} where id = '{}'".format(x.count, x.id)) + if x.stat_type == 'failed' and x.notification_type == 'sms': + op.execute("update notification_statistics set sms_failed = {} where id = '{}'".format(x.count, x.id)) + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + pass + ### end Alembic commands ### diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index aad84cddb..1651000aa 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -7,11 +7,10 @@ from unittest.mock import ANY, call from notifications_utils.recipients import validate_phone_number, format_phone_number import app -from app import statsd_client, mmg_client, aws_ses_client +from app import statsd_client, mmg_client from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider from app.celery.research_mode_tasks import send_sms_response, send_email_response -from app.celery.tasks import provider_to_use from app.clients.email import EmailClientException from app.clients.sms import SmsClientException from app.dao import notifications_dao, provider_details_dao @@ -55,7 +54,7 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses first = providers[0] second = providers[1] - assert provider_to_use('sms', '1234').name == first.identifier + assert provider_tasks.provider_to_use('sms', '1234').name == first.identifier first.priority = 20 second.priority = 10 @@ -63,7 +62,7 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses provider_details_dao.dao_update_provider_details(first) provider_details_dao.dao_update_provider_details(second) - assert provider_to_use('sms', '1234').name == second.identifier + assert provider_tasks.provider_to_use('sms', '1234').name == second.identifier first.priority = 10 first.active = False @@ -72,12 +71,12 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses provider_details_dao.dao_update_provider_details(first) provider_details_dao.dao_update_provider_details(second) - assert provider_to_use('sms', '1234').name == second.identifier + assert provider_tasks.provider_to_use('sms', '1234').name == second.identifier first.active = True provider_details_dao.dao_update_provider_details(first) - assert provider_to_use('sms', '1234').name == first.identifier + assert provider_tasks.provider_to_use('sms', '1234').name == first.identifier def test_should_send_personalised_template_to_correct_sms_provider_and_persist( @@ -486,7 +485,7 @@ def test_send_email_should_use_service_reply_to_email( db_notification.id, ) - aws_ses_client.send_email.assert_called_once_with( + app.aws_ses_client.send_email.assert_called_once_with( ANY, ANY, ANY, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e5f2550cd..ab6a6cafb 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,5 +1,5 @@ import uuid -from datetime import datetime, timedelta +from datetime import datetime import pytest from freezegun import freeze_time @@ -14,11 +14,9 @@ from app.celery.tasks import s3 from app.celery.tasks import ( send_sms, process_job, - provider_to_use, send_email ) -from app.dao import jobs_dao, provider_details_dao -from app.dao.provider_statistics_dao import get_provider_statistics +from app.dao import jobs_dao from app.models import Notification, KEY_TYPE_TEAM from tests.app import load_example_csv from tests.app.conftest import ( @@ -55,37 +53,6 @@ def _notification_json(template, to, personalisation=None, job_id=None, row_numb return notification -# TODO moved to test_provider_tasks once send-email migrated -def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): - providers = provider_details_dao.get_provider_details_by_notification_type('sms') - first = providers[0] - second = providers[1] - - assert provider_to_use('sms', '1234').name == first.identifier - - first.priority = 20 - second.priority = 10 - - provider_details_dao.dao_update_provider_details(first) - provider_details_dao.dao_update_provider_details(second) - - assert provider_to_use('sms', '1234').name == second.identifier - - first.priority = 10 - first.active = False - second.priority = 20 - - provider_details_dao.dao_update_provider_details(first) - provider_details_dao.dao_update_provider_details(second) - - assert provider_to_use('sms', '1234').name == second.identifier - - first.active = True - provider_details_dao.dao_update_provider_details(first) - - assert provider_to_use('sms', '1234').name == first.identifier - - @freeze_time("2016-01-01 11:09:00.061258") def test_should_process_sms_job(sample_job, mocker, mock_celery_remove_job): mocker.patch('app.statsd_client.incr') @@ -346,6 +313,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi assert not persisted_notification.job_id assert persisted_notification.personalisation == {'name': 'Jo'} assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) + assert persisted_notification.notification_type == 'sms' def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): @@ -382,6 +350,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert not persisted_notification.sent_by assert not persisted_notification.job_id assert not persisted_notification.personalisation + assert persisted_notification.notification_type == 'sms' def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): @@ -458,6 +427,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert persisted_notification.job_row_number == 2 assert persisted_notification.api_key_id == sample_api_key.id assert persisted_notification.key_type == KEY_TYPE_TEAM + assert persisted_notification.notification_type == 'sms' def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, sample_api_key, mocker): @@ -505,6 +475,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) assert persisted_notification.api_key_id == sample_api_key.id assert persisted_notification.key_type == KEY_TYPE_TEAM + assert persisted_notification.notification_type == 'email' def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): @@ -538,6 +509,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by + assert persisted_notification.notification_type == 'email' def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): @@ -564,6 +536,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert not persisted_notification.sent_by assert persisted_notification.personalisation == {"name": "Jo"} assert not persisted_notification.reference + assert persisted_notification.notification_type == 'email' def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker):