diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a97364fa1..ba7111b19 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -287,23 +287,13 @@ def save_email(self, @notify_celery.task(bind=True, name="save-api-email", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") -def save_api_email(self, encrypted_notification): +def save_api_email(self, + encrypted_notification, + ): - save_api_email_or_sms(self, encrypted_notification) - - -@notify_celery.task(bind=True, name="save-api-sms", max_retries=5, default_retry_delay=300) -@statsd(namespace="tasks") -def save_api_sms(self, encrypted_notification): - save_api_email_or_sms(self, encrypted_notification) - - -def save_api_email_or_sms(self, encrypted_notification): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(notification['service_id']) - q = QueueNames.SEND_EMAIL if notification['notification_type'] == EMAIL_TYPE else QueueNames.SEND_SMS - provider_task = provider_tasks.deliver_email if notification['notification_type'] == EMAIL_TYPE \ - else provider_tasks.deliver_sms + try: persist_notification( @@ -313,7 +303,7 @@ def save_api_email_or_sms(self, encrypted_notification): recipient=notification['to'], service=service, personalisation=notification.get('personalisation'), - notification_type=notification['notification_type'], + notification_type=EMAIL_TYPE, client_reference=notification['client_reference'], api_key_id=notification.get('api_key_id'), key_type=KEY_TYPE_NORMAL, @@ -323,16 +313,14 @@ def save_api_email_or_sms(self, encrypted_notification): document_download_count=notification['document_download_count'] ) - q = q if not service.research_mode else QueueNames.RESEARCH_MODE - provider_task.apply_async( + q = QueueNames.SEND_EMAIL if not service.research_mode else QueueNames.RESEARCH_MODE + provider_tasks.deliver_email.apply_async( [notification['id']], queue=q ) - current_app.logger.debug( - f"{notification['notification_type']} {notification['id']} has been persisted and sent to delivery queue." - ) + current_app.logger.debug(f"Email {notification['id']} has been persisted and sent to delivery queue.") except IntegrityError: - current_app.logger.info(f"{notification['notification_type']} {notification['id']} already exists.") + current_app.logger.info(f"Email {notification['id']} already exists.") except SQLAlchemyError: diff --git a/app/config.py b/app/config.py index 03c83e61e..1ad9bcea7 100644 --- a/app/config.py +++ b/app/config.py @@ -32,7 +32,6 @@ class QueueNames(object): ANTIVIRUS = 'antivirus-tasks' SANITISE_LETTERS = 'sanitise-letter-tasks' SAVE_API_EMAIL = 'save-api-email-tasks' - SAVE_API_SMS = 'save-api-sms-tasks' @staticmethod def all_queues(): @@ -51,8 +50,7 @@ class QueueNames(object): QueueNames.CALLBACKS, QueueNames.LETTERS, QueueNames.SMS_CALLBACKS, - QueueNames.SAVE_API_EMAIL, - QueueNames.SAVE_API_SMS + QueueNames.SAVE_API_EMAIL ] diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 97f6dfeac..139702b45 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -18,7 +18,7 @@ from app import ( ) from app.celery.letters_pdf_tasks import get_pdf_for_templated_letter, sanitise_letter from app.celery.research_mode_tasks import create_fake_letter_response_file -from app.celery.tasks import save_api_email, save_api_sms +from app.celery.tasks import save_api_email from app.clients.document_download import DocumentDownloadError from app.config import QueueNames, TaskNames from app.dao.templates_dao import get_precompiled_letter_template @@ -69,6 +69,7 @@ from app.v2.notifications.notification_schemas import ( ) from app.v2.utils import get_valid_json + POST_NOTIFICATION_JSON_PARSE_DURATION_SECONDS = Histogram( 'post_notification_json_parse_duration_seconds', 'Time taken to parse and validate post request json', @@ -200,16 +201,14 @@ def process_sms_or_email_notification( template_with_content=template_with_content ) - if service.id in current_app.config.get('HIGH_VOLUME_SERVICE') \ - and api_user.key_type == KEY_TYPE_NORMAL \ - and notification_type in [EMAIL_TYPE, SMS_TYPE]: - # Put service with high volumes of notifications onto a queue + if service.id in current_app.config.get('HIGH_VOLUME_SERVICE') and api_user.key_type == KEY_TYPE_NORMAL \ + and notification_type == EMAIL_TYPE: + # Put GOV.UK Email notifications onto a queue # To take the pressure off the db for API requests put the notification for our high volume service onto a queue # the task will then save the notification, then call send_notification_to_queue. - # NOTE: The high volume service should be aware that the notification is not immediately - # available by a GET request, it is recommend they use callbacks to keep track of status updates. + # We know that this team does not use the GET request, but relies on callbacks to get the status updates. try: - save_email_or_sms_to_queue( + save_email_to_queue( form=form, notification_id=str(notification_id), notification_type=notification_type, @@ -259,7 +258,7 @@ def process_sms_or_email_notification( return resp -def save_email_or_sms_to_queue( +def save_email_to_queue( *, notification_id, form, @@ -275,7 +274,7 @@ def save_email_or_sms_to_queue( "id": notification_id, "template_id": str(template.id), "template_version": template.version, - "to": form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'], + "to": form['email_address'], "service_id": str(service_id), "personalisation": personalisation, "notification_type": notification_type, @@ -291,11 +290,7 @@ def save_email_or_sms_to_queue( data ) - if notification_type == EMAIL_TYPE: - save_api_email.apply_async([encrypted], queue=QueueNames.SAVE_API_EMAIL) - elif notification_type == SMS_TYPE: - save_api_sms.apply_async([encrypted], queue=QueueNames.SAVE_API_SMS) - + save_api_email.apply_async([encrypted], queue=QueueNames.SAVE_API_EMAIL) return Notification(**data) diff --git a/scripts/paas_app_wrapper.sh b/scripts/paas_app_wrapper.sh index f55f9cc3e..9e4c7f3e0 100755 --- a/scripts/paas_app_wrapper.sh +++ b/scripts/paas_app_wrapper.sh @@ -51,7 +51,7 @@ case $NOTIFY_APP_NAME in ;; delivery-worker-save-api-notifications) exec scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 \ - -Q save-api-email-tasks, save-api-sms-tasks 2> /dev/null + -Q save-api-email-tasks 2> /dev/null ;; delivery-celery-beat) exec scripts/run_app_paas.sh celery -A run_celery.notify_celery beat --loglevel=INFO diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index d3bc64f09..f903016ad 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -33,9 +33,8 @@ from app.celery.tasks import ( s3, send_inbound_sms_to_service, process_returned_letters_list, - get_recipient_csv_and_template_and_sender_id, save_api_email, - save_api_sms + get_recipient_csv_and_template_and_sender_id, ) from app.config import QueueNames from app.dao import jobs_dao, service_email_reply_to_dao, service_sms_sender_dao @@ -1792,66 +1791,17 @@ def test_process_returned_letters_populates_returned_letters_table( @freeze_time('2020-03-25 14:30') -@pytest.mark.parametrize('notification_type', ['sms', 'email']) -def test_save_api_email_or_sms(mocker, sample_service, notification_type): - template = create_template(sample_service) if notification_type == SMS_TYPE \ - else create_template(sample_service, template_type=EMAIL_TYPE) - mock_provider_task = mocker.patch(f'app.celery.provider_tasks.deliver_{notification_type}.apply_async') - api_key = create_api_key(service=template.service) +def test_save_api_email(sample_email_template, mocker): + mock_send_email_to_provider = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + api_key = create_api_key(service=sample_email_template.service) data = { "id": str(uuid.uuid4()), - "template_id": str(template.id), - "template_version": template.version, - "service_id": str(template.service_id), + "template_id": str(sample_email_template.id), + "template_version": sample_email_template.version, + "to": "jane.citizen@example.com", + "service_id": str(sample_email_template.service_id), "personalisation": None, - "notification_type": template.template_type, - "api_key_id": str(api_key.id), - "key_type": api_key.key_type, - "client_reference": 'our email', - "reply_to_text": None, - "document_download_count": 0, - "status": NOTIFICATION_CREATED, - "created_at": datetime.utcnow().strftime(DATETIME_FORMAT), - } - - if notification_type == EMAIL_TYPE: - data.update({"to": "jane.citizen@example.com"}) - expected_queue = QueueNames.SEND_EMAIL - else: - data.update({"to": "+447700900855"}) - expected_queue = QueueNames.SEND_SMS - - encrypted = encryption.encrypt( - data - ) - - assert len(Notification.query.all()) == 0 - if notification_type == EMAIL_TYPE: - save_api_email(encrypted_notification=encrypted) - else: - save_api_sms(encrypted_notification=encrypted) - notifications = Notification.query.all() - assert len(notifications) == 1 - assert str(notifications[0].id) == data['id'] - assert notifications[0].created_at == datetime(2020, 3, 25, 14, 30) - assert notifications[0].notification_type == notification_type - mock_provider_task.assert_called_once_with([data['id']], queue=expected_queue) - - -@freeze_time('2020-03-25 14:30') -@pytest.mark.parametrize('notification_type', ['sms', 'email']) -def test_save_api_email_dont_retry_if_notification_already_exists(sample_service, mocker, notification_type): - template = create_template(sample_service) if notification_type == SMS_TYPE \ - else create_template(sample_service, template_type=EMAIL_TYPE) - mock_provider_task = mocker.patch(f'app.celery.provider_tasks.deliver_{notification_type}.apply_async') - api_key = create_api_key(service=template.service) - data = { - "id": str(uuid.uuid4()), - "template_id": str(template.id), - "template_version": template.version, - "service_id": str(template.service_id), - "personalisation": None, - "notification_type": template.template_type, + "notification_type": sample_email_template.template_type, "api_key_id": str(api_key.id), "key_type": api_key.key_type, "client_reference": 'our email', @@ -1861,32 +1811,52 @@ def test_save_api_email_dont_retry_if_notification_already_exists(sample_service "created_at": datetime.utcnow().strftime(DATETIME_FORMAT), } - if notification_type == EMAIL_TYPE: - data.update({"to": "jane.citizen@example.com"}) - expected_queue = QueueNames.SEND_EMAIL - else: - data.update({"to": "+447700900855"}) - expected_queue = QueueNames.SEND_SMS + encrypted = encryption.encrypt( + data + ) + + assert len(Notification.query.all()) == 0 + save_api_email(encrypted) + notifications = Notification.query.all() + assert len(notifications) == 1 + assert str(notifications[0].id) == data['id'] + assert notifications[0].created_at == datetime(2020, 3, 25, 14, 30) + mock_send_email_to_provider.assert_called_once_with([data['id']], queue=QueueNames.SEND_EMAIL) + + +@freeze_time('2020-03-25 14:30') +def test_save_api_email_dont_retry_if_notification_already_exists(sample_email_template, mocker): + mock_send_email_to_provider = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + api_key = create_api_key(service=sample_email_template.service) + data = { + "id": str(uuid.uuid4()), + "template_id": str(sample_email_template.id), + "template_version": sample_email_template.version, + "to": "jane.citizen@example.com", + "service_id": str(sample_email_template.service_id), + "personalisation": None, + "notification_type": sample_email_template.template_type, + "api_key_id": str(api_key.id), + "key_type": api_key.key_type, + "client_reference": 'our email', + "reply_to_text": "our.email@gov.uk", + "document_download_count": 0, + "status": NOTIFICATION_CREATED, + "created_at": datetime.utcnow().strftime(DATETIME_FORMAT), + } encrypted = encryption.encrypt( data ) assert len(Notification.query.all()) == 0 - - if notification_type == EMAIL_TYPE: - save_api_email(encrypted_notification=encrypted) - else: - save_api_sms(encrypted_notification=encrypted) + save_api_email(encrypted) notifications = Notification.query.all() assert len(notifications) == 1 # call the task again with the same notification - if notification_type == EMAIL_TYPE: - save_api_email(encrypted_notification=encrypted) - else: - save_api_sms(encrypted_notification=encrypted) + save_api_email(encrypted) notifications = Notification.query.all() assert len(notifications) == 1 assert str(notifications[0].id) == data['id'] assert notifications[0].created_at == datetime(2020, 3, 25, 14, 30) # should only have sent the notification once. - mock_provider_task.assert_called_once_with([data['id']], queue=expected_queue) + mock_send_email_to_provider.assert_called_once_with([data['id']], queue=QueueNames.SEND_EMAIL) diff --git a/tests/app/test_config.py b/tests/app/test_config.py index febd6d936..fae5919fa 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -60,7 +60,7 @@ def test_load_config_if_cloudfoundry_not_available(reload_config): def test_queue_names_all_queues_correct(): # Need to ensure that all_queues() only returns queue names used in API queues = QueueNames.all_queues() - assert len(queues) == 16 + assert len(queues) == 15 assert set([ QueueNames.PRIORITY, QueueNames.PERIODIC, @@ -76,6 +76,5 @@ def test_queue_names_all_queues_correct(): QueueNames.CALLBACKS, QueueNames.LETTERS, QueueNames.SMS_CALLBACKS, - QueueNames.SAVE_API_EMAIL, - QueueNames.SAVE_API_SMS + QueueNames.SAVE_API_EMAIL ]) == set(queues) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 58da17c74..5e3738155 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -29,7 +29,6 @@ from tests.app.db import ( create_service_with_inbound_number, create_api_key ) -from tests.conftest import set_config_values @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -1030,147 +1029,129 @@ def test_post_email_notification_when_data_is_empty_returns_400(client, sample_s assert error_msg == 'email_address is a required property' -@pytest.mark.parametrize("notification_type", ("email", "sms")) -def test_post_notifications_saves_email_or_sms_to_queue(client, notify_db_session, mocker, notification_type): - save_task = mocker.patch(f"app.celery.tasks.save_api_{notification_type}.apply_async") - mock_send_task = mocker.patch(f'app.celery.provider_tasks.deliver_{notification_type}.apply_async') +def test_post_notifications_saves_email_to_queue(client, notify_db_session, mocker): + save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") + mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][0], service_name='high volume service', ) - with set_config_values(current_app, { - 'HIGH_VOLUME_SERVICE': [str(service.id)], + template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) + data = { + "email_address": "joe.citizen@example.com", + "template_id": template.id, + "personalisation": {"message": "Dear citizen, have a nice day"} + } + response = client.post( + path='/v2/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)] + ) - }): - template = create_template(service=service, content='((message))', template_type=notification_type) - data = { - "template_id": template.id, - "personalisation": {"message": "Dear citizen, have a nice day"} - } - data.update({"email_address": "joe.citizen@example.com"}) if notification_type == EMAIL_TYPE \ - else data.update({"phone_number": "+447700900855"}) + json_resp = response.get_json() - response = client.post( - path=f'/v2/notifications/{notification_type}', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)] - ) - - json_resp = response.get_json() - - assert response.status_code == 201 - assert json_resp['id'] - assert json_resp['content']['body'] == "Dear citizen, have a nice day" - assert json_resp['template']['id'] == str(template.id) - save_task.assert_called_once_with([mock.ANY], queue=f'save-api-{notification_type}-tasks') - assert not mock_send_task.called - assert len(Notification.query.all()) == 0 + assert response.status_code == 201 + assert json_resp['id'] + assert json_resp['content']['body'] == "Dear citizen, have a nice day" + assert json_resp['template']['id'] == str(template.id) + save_email_task.assert_called_once_with([mock.ANY], queue='save-api-email-tasks') + assert not mock_send_task.called + assert len(Notification.query.all()) == 0 -@pytest.mark.parametrize("notification_type", ("email", "sms")) -def test_post_notifications_saves_email_or_sms_normally_if_saving_to_queue_fails( - client, notify_db_session, mocker, notification_type -): - save_task = mocker.patch( - f"app.celery.tasks.save_api_{notification_type}.apply_async", +def test_post_notifications_saves_email_normally_if_save_email_to_queue_fails(client, notify_db_session, mocker): + save_email_task = mocker.patch( + "app.celery.tasks.save_api_email.apply_async", side_effect=SQSError({'some': 'json'}, 'some opname') ) - mock_send_task = mocker.patch(f'app.celery.provider_tasks.deliver_{notification_type}.apply_async') + mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][1], service_name='high volume service', ) - with set_config_values(current_app, { - 'HIGH_VOLUME_SERVICE': [str(service.id)], + template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) + data = { + "email_address": "joe.citizen@example.com", + "template_id": template.id, + "personalisation": {"message": "Dear citizen, have a nice day"} + } + response = client.post( + path='/v2/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)] + ) - }): - template = create_template(service=service, content='((message))', template_type=notification_type) - data = { - "template_id": template.id, - "personalisation": {"message": "Dear citizen, have a nice day"} - } - data.update({"email_address": "joe.citizen@example.com"}) if notification_type == EMAIL_TYPE \ - else data.update({"phone_number": "+447700900855"}) + json_resp = response.get_json() - response = client.post( - path=f'/v2/notifications/{notification_type}', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)] - ) - - json_resp = response.get_json() - - assert response.status_code == 201 - assert json_resp['id'] - assert json_resp['content']['body'] == "Dear citizen, have a nice day" - assert json_resp['template']['id'] == str(template.id) - save_task.assert_called_once_with([mock.ANY], queue=f'save-api-{notification_type}-tasks') - mock_send_task.assert_called_once_with([json_resp['id']], queue=f'send-{notification_type}-tasks') - assert Notification.query.count() == 1 + assert response.status_code == 201 + assert json_resp['id'] + assert json_resp['content']['body'] == "Dear citizen, have a nice day" + assert json_resp['template']['id'] == str(template.id) + # save email + save_email_task.assert_called_once_with([mock.ANY], queue='save-api-email-tasks') + mock_send_task.assert_called_once_with([json_resp['id']], queue='send-email-tasks') + assert Notification.query.count() == 1 -@pytest.mark.parametrize("notification_type", ("email", "sms")) -def test_post_notifications_doesnt_use_save_queue_for_test_notifications( - client, notify_db_session, mocker, notification_type -): - save_task = mocker.patch(f"app.celery.tasks.save_api_{notification_type}.apply_async") - mock_send_task = mocker.patch(f'app.celery.provider_tasks.deliver_{notification_type}.apply_async') +def test_post_notifications_doesnt_save_email_to_queue_for_test_emails(client, notify_db_session, mocker): + save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") + mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][2], service_name='high volume service', ) - with set_config_values(current_app, { - 'HIGH_VOLUME_SERVICE': [str(service.id)], + template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) + data = { + "email_address": "joe.citizen@example.com", + "template_id": template.id, + "personalisation": {"message": "Dear citizen, have a nice day"} + } + response = client.post( + path='/v2/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), + create_authorization_header(service_id=service.id, key_type='test')] + ) - }): - template = create_template(service=service, content='((message))', template_type=notification_type) - data = { - "template_id": template.id, - "personalisation": {"message": "Dear citizen, have a nice day"} - } - data.update({"email_address": "joe.citizen@example.com"}) if notification_type == EMAIL_TYPE \ - else data.update({"phone_number": "+447700900855"}) - response = client.post( - path=f'/v2/notifications/{notification_type}', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), - create_authorization_header(service_id=service.id, key_type='test')] - ) + json_resp = response.get_json() - json_resp = response.get_json() - - assert response.status_code == 201 - assert json_resp['id'] - assert json_resp['content']['body'] == "Dear citizen, have a nice day" - assert json_resp['template']['id'] == str(template.id) - assert mock_send_task.called - assert not save_task.called - assert len(Notification.query.all()) == 1 + assert response.status_code == 201 + assert json_resp['id'] + assert json_resp['content']['body'] == "Dear citizen, have a nice day" + assert json_resp['template']['id'] == str(template.id) + assert mock_send_task.called + assert not save_email_task.called + assert len(Notification.query.all()) == 1 -def test_post_notification_does_not_use_save_queue_for_letters(client, sample_letter_template, mocker): - mock_save = mocker.patch("app.v2.notifications.post_notifications.save_email_or_sms_to_queue") - mock_create_pdf_task = mocker.patch('app.celery.tasks.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async') +def test_post_notifications_doesnt_save_email_to_queue_for_sms(client, notify_db_session, mocker): + save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") + mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - with set_config_values(current_app, { - 'HIGH_VOLUME_SERVICE': [str(sample_letter_template.service_id)], + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][3], + service_name='high volume service', + ) + template = create_template(service=service, content='((message))', template_type=SMS_TYPE) + data = { + "phone_number": '+447700900855', + "template_id": template.id, + "personalisation": {"message": "Dear citizen, have a nice day"} + } + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)] + ) - }): - data = { - 'template_id': str(sample_letter_template.id), - 'personalisation': { - 'address_line_1': 'Her Royal Highness Queen Elizabeth II', - 'address_line_2': 'Buckingham Palace', - 'address_line_3': 'London', - 'postcode': 'SW1 1AA', - } - } - response = client.post( - path='/v2/notifications/letter', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), - create_authorization_header(service_id=sample_letter_template.service_id)] - ) - assert response.status_code == 201 - json_resp = response.get_json() - assert not mock_save.called - mock_create_pdf_task.assert_called_once_with([str(json_resp['id'])], queue='create-letters-pdf-tasks') + json_resp = response.get_json() + + assert response.status_code == 201 + assert json_resp['id'] + assert mock_send_task.called + assert not save_email_task.called + + assert len(Notification.query.all()) == 1