diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c3f533a6d..008b0fd5e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -182,7 +182,7 @@ def send_sms(self, provider_tasks.deliver_sms.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_SMS if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info( @@ -227,7 +227,7 @@ def send_email(self, provider_tasks.deliver_email.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_EMAIL if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 4dba91208..83236d521 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -24,16 +24,20 @@ def send_notification_to_provider(notification_id): send_response( send_to_providers.send_email_to_provider, provider_tasks.deliver_email, - notification) + notification, + QueueNames.SEND_EMAIL + ) else: send_response( send_to_providers.send_sms_to_provider, provider_tasks.deliver_sms, - notification) + notification, + QueueNames.SEND_SMS + ) return jsonify({}), 204 -def send_response(send_call, task_call, notification): +def send_response(send_call, task_call, notification, queue): try: send_call(notification) except Exception as e: @@ -42,4 +46,4 @@ def send_response(send_call, task_call, notification): notification.id, notification.notification_type), e) - task_call.apply_async((str(notification.id)), queue=QueueNames.SEND_COMBINED) + task_call.apply_async((str(notification.id)), queue=queue) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 78881d2b6..76da026c4 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -100,12 +100,14 @@ def persist_notification( def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE - elif not queue: - queue = QueueNames.SEND_COMBINED if notification.notification_type == SMS_TYPE: + if not queue: + queue = QueueNames.SEND_SMS deliver_task = provider_tasks.deliver_sms if notification.notification_type == EMAIL_TYPE: + if not queue: + queue = QueueNames.SEND_EMAIL deliver_task = provider_tasks.deliver_email try: diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index b4ad7e255..0282f8074 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -468,7 +468,7 @@ def test_should_send_all_scheduled_notifications_to_deliver_queue(sample_templat send_scheduled_notifications() - mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-tasks') + mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-sms-tasks') scheduled_notifications = dao_get_scheduled_notifications() assert not scheduled_notifications diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 55b154aa9..c6b09e333 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -422,7 +422,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi assert persisted_notification.notification_type == 'sms' mocked_deliver_sms.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -483,7 +483,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert persisted_notification.notification_type == 'sms' provider_tasks.deliver_sms.apply_async.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -509,7 +509,7 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key persisted_notification = Notification.query.one() mocked_deliver_sms.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -537,7 +537,7 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with persisted_notification = Notification.query.one() mocked_deliver_email.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-email-tasks" ) @@ -641,7 +641,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ provider_tasks.deliver_sms.apply_async.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -738,7 +738,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [str(persisted_notification.id)], queue='send-tasks') + [str(persisted_notification.id)], queue='send-email-tasks') def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): @@ -769,7 +769,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert not persisted_notification.sent_by assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], - queue='send-tasks') + queue='send-email-tasks') def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): @@ -795,7 +795,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [str(persisted_notification.id)], queue='send-tasks' + [str(persisted_notification.id)], queue='send-email-tasks' ) @@ -823,7 +823,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], - queue='send-tasks') + queue='send-email-tasks') def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): diff --git a/tests/app/delivery/test_rest.py b/tests/app/delivery/test_rest.py index 984bf90e1..08a5d0a05 100644 --- a/tests/app/delivery/test_rest.py +++ b/tests/app/delivery/test_rest.py @@ -78,7 +78,7 @@ def test_should_call_deliver_sms_task_if_send_sms_to_provider_fails(notify_api, ) 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-tasks' + (str(sample_notification.id)), queue='send-sms-tasks' ) assert response.status_code == 204 @@ -100,6 +100,6 @@ def test_should_call_deliver_email_task_if_send_email_to_provider_fails( ) 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-tasks' + (str(sample_email_notification.id)), queue='send-email-tasks' ) assert response.status_code == 204 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index aa7ab2d86..3822395cd 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -132,7 +132,7 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t mocked.assert_called_once_with( [notification_id], - queue="send-tasks" + queue="send-email-tasks" ) assert response.status_code == 201 assert response_data['body'] == u'Hello Jo\nThis is an email from GOV.\u200BUK' @@ -342,7 +342,7 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker response_data = json.loads(response.data)['data'] notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-tasks') + mocked.assert_called_once_with([notification_id], queue='send-sms-tasks') assert response.status_code == 201 assert notification_id assert 'subject' not in response_data @@ -395,7 +395,7 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template notification_id = response_data['notification']['id'] app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( [notification_id], - queue="send-tasks" + queue="send-email-tasks" ) assert response.status_code == 201 @@ -593,7 +593,10 @@ def test_should_send_email_if_team_api_key_and_a_service_user(notify_api, sample data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with([fake_uuid], queue='send-tasks') + app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( + [fake_uuid], + queue='send-email-tasks' + ) assert response.status_code == 201 @@ -689,57 +692,67 @@ def test_should_send_sms_if_team_api_key_and_a_service_user(notify_api, sample_t data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-tasks') + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-sms-tasks') assert response.status_code == 201 -@pytest.mark.parametrize('template_type', - [SMS_TYPE, EMAIL_TYPE]) -def test_should_persist_notification(notify_api, sample_template, - sample_email_template, - template_type, - fake_uuid, mocker): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) - template = sample_template if template_type == SMS_TYPE else sample_email_template - to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ - else sample_email_template.service.created_by.email_address - data = { - 'to': to, - 'template': template.id - } - api_key = ApiKey( - service=template.service, - name='team_key', - created_by=template.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) +@pytest.mark.parametrize('template_type,queue_name', [ + (SMS_TYPE, 'send-sms-tasks'), + (EMAIL_TYPE, 'send-email-tasks') +]) +def test_should_persist_notification( + client, + sample_template, + sample_email_template, + fake_uuid, + mocker, + template_type, + queue_name +): + mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + template = sample_template if template_type == SMS_TYPE else sample_email_template + to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ + else sample_email_template.service.created_by.email_address + data = { + 'to': to, + 'template': template.id + } + api_key = ApiKey( + service=template.service, + name='team_key', + created_by=template.created_by, + key_type=KEY_TYPE_TEAM) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/{}'.format(template_type), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + response = client.post( + path='/notifications/{}'.format(template_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - mocked.assert_called_once_with([fake_uuid], queue='send-tasks') - assert response.status_code == 201 + mocked.assert_called_once_with([fake_uuid], queue=queue_name) + assert response.status_code == 201 - notification = notifications_dao.get_notification_by_id(fake_uuid) - assert notification.to == to - assert notification.template_id == template.id - assert notification.notification_type == template_type + notification = notifications_dao.get_notification_by_id(fake_uuid) + assert notification.to == to + assert notification.template_id == template.id + assert notification.notification_type == template_type -@pytest.mark.parametrize('template_type', - [SMS_TYPE, EMAIL_TYPE]) +@pytest.mark.parametrize('template_type,queue_name', [ + (SMS_TYPE, 'send-sms-tasks'), + (EMAIL_TYPE, 'send-email-tasks') +]) def test_should_delete_notification_and_return_error_if_sqs_fails( - client, - sample_email_template, - sample_template, - fake_uuid, - mocker, - template_type): + client, + sample_email_template, + sample_template, + fake_uuid, + mocker, + template_type, + queue_name +): mocked = mocker.patch( 'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type), side_effect=Exception("failed to talk to SQS") @@ -768,7 +781,7 @@ def test_should_delete_notification_and_return_error_if_sqs_fails( ) assert str(e.value) == 'failed to talk to SQS' - mocked.assert_called_once_with([fake_uuid], queue='send-tasks') + mocked.assert_called_once_with([fake_uuid], queue=queue_name) assert not notifications_dao.get_notification_by_id(fake_uuid) assert not NotificationHistory.query.get(fake_uuid) @@ -1119,7 +1132,7 @@ def test_should_allow_store_original_number_on_sms_notification(client, sample_t response_data = json.loads(response.data)['data'] notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-tasks') + mocked.assert_called_once_with([notification_id], queue='send-sms-tasks') assert response.status_code == 201 assert notification_id notifications = Notification.query.all() diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ac73b36da..3e4d1d9c2 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -214,9 +214,9 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa [(True, None, 'research-mode-tasks', 'sms', 'normal'), (True, None, 'research-mode-tasks', 'email', 'normal'), (True, None, 'research-mode-tasks', 'email', 'team'), - (False, None, 'send-tasks', 'sms', 'normal'), - (False, None, 'send-tasks', 'email', 'normal'), - (False, None, 'send-tasks', 'sms', 'team'), + (False, None, 'send-sms-tasks', 'sms', 'normal'), + (False, None, 'send-email-tasks', 'email', 'normal'), + (False, None, 'send-sms-tasks', 'sms', 'team'), (False, None, 'research-mode-tasks', 'sms', 'test'), (True, 'notify-internal-tasks', 'research-mode-tasks', 'email', 'normal'), (False, 'notify-internal-tasks', 'notify-internal-tasks', 'sms', 'normal'),