Merge pull request #512 from alphagov/fix-count

Fix bug where all notifications were getting a type = email.

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)

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 ###

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,

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):