Merge pull request #765 from alphagov/use-notify-queue-to-deliver

Send Notify's 2FA codes via only one queue

app/notifications/process_notifications.py

@@ -70,19 +70,22 @@ def persist_notification(template_id,
     return notification
 
 
-def send_notification_to_queue(notification, research_mode):
-    try:
-        research_mode = research_mode or notification.key_type == KEY_TYPE_TEST
+def send_notification_to_queue(notification, research_mode, queue=None):
+    if research_mode or notification.key_type == KEY_TYPE_TEST:
+        queue = 'research-mode'
+    elif not queue:
         if notification.notification_type == SMS_TYPE:
-            provider_tasks.deliver_sms.apply_async(
-                [str(notification.id)],
-                queue='send-sms' if not research_mode else 'research-mode'
-            )
+            queue = 'send-sms'
         if notification.notification_type == EMAIL_TYPE:
-            provider_tasks.deliver_email.apply_async(
-                [str(notification.id)],
-                queue='send-email' if not research_mode else 'research-mode'
-            )
+            queue = 'send-email'
+
+    if notification.notification_type == SMS_TYPE:
+        deliver_task = provider_tasks.deliver_sms
+    if notification.notification_type == EMAIL_TYPE:
+        deliver_task = provider_tasks.deliver_email
+
+    try:
+        deliver_task.apply_async([str(notification.id)], queue=queue)
     except Exception as e:
         current_app.logger.exception(e)
         dao_delete_notifications_and_history_by_id(notification.id)

app/user/rest.py

@@ -18,7 +18,11 @@ from app.dao.users_dao import (
 from app.dao.permissions_dao import permission_dao
 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 SMS_TYPE
+from app.models import SMS_TYPE, KEY_TYPE_NORMAL
+from app.notifications.process_notifications import (
+    persist_notification,
+    send_notification_to_queue
+)
 from app.schemas import (
     email_data_request_schema,
     user_schema,
@@ -143,20 +147,22 @@ def send_user_sms_code(user_id):
     mobile = user_to_send_to.mobile_number if verify_code.get('to', None) is None else verify_code.get('to')
     sms_code_template_id = current_app.config['SMS_CODE_TEMPLATE_ID']
     sms_code_template = dao_get_template_by_id(sms_code_template_id)
-    verification_message = encryption.encrypt({
-        'template': sms_code_template_id,
-        'template_version': sms_code_template.version,
-        'to': mobile,
-        'personalisation': {
-            'verify_code': secret_code
-        }
+    notify_service_id = current_app.config['NOTIFY_SERVICE_ID']
 
-    })
-    send_sms.apply_async([current_app.config['NOTIFY_SERVICE_ID'],
-                          str(uuid.uuid4()),
-                          verification_message,
-                          datetime.utcnow().strftime(DATETIME_FORMAT)
-                          ], queue='notify')
+    saved_notification = persist_notification(
+        template_id=sms_code_template_id,
+        template_version=sms_code_template.version,
+        recipient=mobile,
+        service_id=notify_service_id,
+        personalisation={'verify_code': secret_code},
+        notification_type=SMS_TYPE,
+        api_key_id=None,
+        key_type=KEY_TYPE_NORMAL
+    )
+    # Assume that we never want to observe the Notify service's research mode
+    # setting for this notification - we still need to be able to log into the
+    # admin even if we're doing user research using this service:
+    send_notification_to_queue(saved_notification, False, queue='notify')
 
     return jsonify({}), 204
 

tests/app/notifications/test_process_notification.py

@@ -5,6 +5,7 @@ import pytest
 from boto3.exceptions import Boto3Error
 from sqlalchemy.exc import SQLAlchemyError
 from freezegun import freeze_time
+from collections import namedtuple
 
 from app.models import Template, Notification, NotificationHistory
 from app.notifications import SendNotificationToQueueError
@@ -135,24 +136,33 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker)
     assert persisted_notification.reference is None
 
 
-@pytest.mark.parametrize('research_mode, queue, notification_type, key_type',
-                         [(True, 'research-mode', 'sms', 'normal'),
-                          (True, 'research-mode', 'email', 'normal'),
-                          (True, 'research-mode', 'email', 'team'),
-                          (False, 'send-sms', 'sms', 'normal'),
-                          (False, 'send-email', 'email', 'normal'),
-                          (False, 'send-sms', 'sms', 'team'),
-                          (False, 'research-mode', 'sms', 'test')])
+@pytest.mark.parametrize('research_mode, requested_queue, expected_queue, notification_type, key_type',
+                         [(True, None, 'research-mode', 'sms', 'normal'),
+                          (True, None, 'research-mode', 'email', 'normal'),
+                          (True, None, 'research-mode', 'email', 'team'),
+                          (False, None, 'send-sms', 'sms', 'normal'),
+                          (False, None, 'send-email', 'email', 'normal'),
+                          (False, None, 'send-sms', 'sms', 'team'),
+                          (False, None, 'research-mode', 'sms', 'test'),
+                          (True, 'notify', 'research-mode', 'email', 'normal'),
+                          (False, 'notify', 'notify', 'sms', 'normal'),
+                          (False, 'notify', 'notify', 'email', 'normal'),
+                          (False, 'notify', 'research-mode', 'sms', 'test')])
 def test_send_notification_to_queue(notify_db, notify_db_session,
-                                    research_mode, notification_type,
-                                    queue, key_type, mocker):
+                                    research_mode, requested_queue, expected_queue,
+                                    notification_type, key_type, mocker):
     mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type))
-    template = sample_template(notify_db, notify_db_session) if notification_type == 'sms' \
-        else sample_email_template(notify_db, notify_db_session)
-    notification = sample_notification(notify_db, notify_db_session, template=template, key_type=key_type)
-    send_notification_to_queue(notification=notification, research_mode=research_mode)
+    Notification = namedtuple('Notification', ['id', 'key_type', 'notification_type', 'created_at'])
+    notification = Notification(
+        id=uuid.uuid4(),
+        key_type=key_type,
+        notification_type=notification_type,
+        created_at=datetime.datetime(2016, 11, 11, 16, 8, 18),
+    )
 
-    mocked.assert_called_once_with([str(notification.id)], queue=queue)
+    send_notification_to_queue(notification=notification, research_mode=research_mode, queue=requested_queue)
+
+    mocked.assert_called_once_with([str(notification.id)], queue=expected_queue)
 
 
 def test_send_notification_to_queue_throws_exception_deletes_notification(sample_notification, mocker):

tests/app/user/test_rest_verify.py

@@ -1,5 +1,6 @@
 import json
 import moto
+import pytest
 
 from datetime import (
     datetime,
@@ -7,9 +8,11 @@ from datetime import (
 )
 
 from flask import url_for, current_app
+from app.dao.services_dao import dao_update_service, dao_fetch_service_by_id
 from app.models import (
     VerifyCode,
-    User
+    User,
+    Notification
 )
 
 from app import db, encryption
@@ -214,41 +217,47 @@ def test_user_verify_password_missing_password(notify_api,
             assert 'Required field missing data' in json_resp['message']['password']
 
 
+@pytest.mark.parametrize('research_mode', [True, False])
 @freeze_time("2016-01-01 11:09:00.061258")
 def test_send_user_sms_code(notify_api,
                             sample_user,
                             sms_code_template,
-                            mocker):
+                            mocker,
+                            research_mode):
     """
     Tests POST endpoint /user/<user_id>/sms-code
     """
 
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
+            if research_mode:
+                notify_service = dao_fetch_service_by_id(current_app.config['NOTIFY_SERVICE_ID'])
+                notify_service.research_mode = True
+                dao_update_service(notify_service)
+
             data = json.dumps({})
             auth_header = create_authorization_header()
             mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111')
-            mocker.patch('app.celery.tasks.send_sms.apply_async')
-            mocker.patch('uuid.uuid4', return_value='some_uuid')  # for the notification id
+            mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
+
             resp = client.post(
                 url_for('user.send_user_sms_code', user_id=sample_user.id),
                 data=data,
                 headers=[('Content-Type', 'application/json'), auth_header])
             assert resp.status_code == 204
+
             assert mocked.call_count == 1
-            encrypted = encryption.encrypt({
-                'template': current_app.config['SMS_CODE_TEMPLATE_ID'],
-                'template_version': 1,
-                'to': sample_user.mobile_number,
-                'personalisation': {
-                    'verify_code': '11111'
-                }
-            })
-            app.celery.tasks.send_sms.apply_async.assert_called_once_with(
-                ([current_app.config['NOTIFY_SERVICE_ID'],
-                  "some_uuid",
-                  encrypted,
-                  "2016-01-01T11:09:00.061258Z"]),
+            assert VerifyCode.query.count() == 1
+            assert VerifyCode.query.first().check_code('11111')
+
+            assert Notification.query.count() == 1
+            notification = Notification.query.first()
+            assert notification.personalisation == {'verify_code': '11111'}
+            assert notification.to == sample_user.mobile_number
+            assert str(notification.service_id) == current_app.config['NOTIFY_SERVICE_ID']
+
+            app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+                ([str(notification.id)]),
                 queue="notify"
             )
 
@@ -257,38 +266,29 @@ def test_send_user_sms_code(notify_api,
 def test_send_user_code_for_sms_with_optional_to_field(notify_api,
                                                        sample_user,
                                                        sms_code_template,
-                                                       mock_encryption,
                                                        mocker):
     """
-    Tests POST endpoint '/<user_id>/code' successful sms with optional to field
+    Tests POST endpoint /user/<user_id>/sms-code with optional to field
     """
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
+            to_number = '+441119876757'
             mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111')
-            mocker.patch('uuid.uuid4', return_value='some_uuid')  # for the notification id
-            mocker.patch('app.celery.tasks.send_sms.apply_async')
-            data = json.dumps({'to': '+441119876757'})
+            mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
+            data = json.dumps({'to': to_number})
             auth_header = create_authorization_header()
+
             resp = client.post(
                 url_for('user.send_user_sms_code', user_id=sample_user.id),
                 data=data,
                 headers=[('Content-Type', 'application/json'), auth_header])
 
             assert resp.status_code == 204
-            encrypted = encryption.encrypt({
-                'template': current_app.config['SMS_CODE_TEMPLATE_ID'],
-                'template_version': 1,
-                'to': '+441119876757',
-                'personalisation': {
-                    'verify_code': '11111'
-                }
-            })
             assert mocked.call_count == 1
-            app.celery.tasks.send_sms.apply_async.assert_called_once_with(
-                ([current_app.config['NOTIFY_SERVICE_ID'],
-                  "some_uuid",
-                  encrypted,
-                  "2016-01-01T11:09:00.061258Z"]),
+            notification = Notification.query.first()
+            assert notification.to == to_number
+            app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+                ([str(notification.id)]),
                 queue="notify"
             )