diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 41a8a12ab..247eaf3a5 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -148,7 +148,7 @@ def send_email_to_provider(self, service_id, notification_id, reply_to_addresses template.replaced_subject, body=template.replaced_govuk_escaped, html_body=template.as_HTML_email, - reply_to_addresses=reply_to_addresses, + reply_to_address=service.reply_to_email_address, ) update_provider_stats( diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 6d710f38d..3bbb3ef37 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -106,7 +106,6 @@ def process_job(job_id): create_uuid(), encrypted, datetime.utcnow().strftime(DATETIME_FORMAT)), - {'reply_to_addresses': service.reply_to_email_address}, queue='bulk-email') finished = datetime.utcnow() @@ -182,8 +181,7 @@ def send_email(self, service_id, try: _save_notification(created_at, notification, notification_id, service_id, EMAIL_TYPE, api_key_id, key_type) - send_email_to_provider.apply_async((service_id, notification_id, reply_to_addresses), - queue='email') + send_email_to_provider.apply_async((service_id, notification_id), queue='email') current_app.logger.info("Email {} created at {}".format(notification_id, created_at)) statsd_client.incr("notifications.tasks.send-email") diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index b33eeed13..6fe62e82a 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -60,14 +60,12 @@ class AwsSesClient(EmailClient): subject, body, html_body='', - reply_to_addresses=None): + reply_to_address=None): try: if isinstance(to_addresses, str): to_addresses = [to_addresses] - if reply_to_addresses and isinstance(reply_to_addresses, str): - reply_to_addresses = [reply_to_addresses] - elif reply_to_addresses is None: - reply_to_addresses = [] + + reply_to_addresses = [reply_to_address] if reply_to_address else [] body = { 'Text': {'Data': body} diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 8683fe64e..57c8ddbec 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -2,11 +2,11 @@ import uuid from datetime import datetime from celery.exceptions import MaxRetriesExceededError -from mock import ANY, call +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 +from app import statsd_client, mmg_client, aws_ses_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 @@ -414,7 +414,6 @@ def test_send_email_to_provider_statsd_updates(notify_db, notify_db_session, sam mocker.patch('app.statsd_client.timing') mocker.patch('app.aws_ses_client.send_email', return_value='reference') mocker.patch('app.aws_ses_client.get_name', return_value="ses") - mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_email_template) send_email_to_provider( @@ -448,3 +447,33 @@ def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_c app.aws_ses_client.send_email.assert_not_called() app.celery.research_mode_tasks.send_email_response.apply_async.assert_not_called() + + +def test_send_email_should_use_service_reply_to_email( + notify_db, notify_db_session, + sample_service, + sample_email_template, + mocker): + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing') + mocker.patch('app.aws_ses_client.send_email', return_value='reference') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') + + db_notification = sample_notification(notify_db, notify_db_session, template=sample_email_template) + sample_service.reply_to_email_address = 'foo@bar.com' + + send_email_to_provider( + db_notification.service_id, + db_notification.id, + reply_to_addresses="waz@baz.com" + ) + + aws_ses_client.send_email.assert_called_once_with( + ANY, + ANY, + ANY, + body=ANY, + html_body=ANY, + reply_to_address=sample_service.reply_to_email_address + ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 5b585f3ab..6cbe2ead8 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -231,7 +231,6 @@ def test_should_process_email_job_if_exactly_on_send_limits(notify_db, "something_encrypted", "2016-01-01T11:09:00.061258" ), - {'reply_to_addresses': None}, queue="bulk-email" ) mock_celery_remove_job.assert_called_once_with((str(job.id),), queue="remove-job") @@ -276,7 +275,6 @@ def test_should_process_email_job(sample_email_job, mocker, mock_celery_remove_j "something_encrypted", "2016-01-01T11:09:00.061258" ), - {'reply_to_addresses': None}, queue="bulk-email" ) job = jobs_dao.dao_get_job_by_id(sample_email_job.id) @@ -492,8 +490,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh statsd_client.timing.assert_called_once_with("notifications.tasks.send-email.task-time", ANY) persisted_notification = Notification.query.filter_by(id=notification_id).one() provider_tasks.send_email_to_provider.apply_async.assert_called_once_with( - (sample_email_template_with_placeholders.service_id, - notification_id, None), queue='email') + (sample_email_template_with_placeholders.service_id, notification_id), queue='email') assert persisted_notification.id == notification_id assert persisted_notification.to == 'my_email@my_email.com' @@ -529,9 +526,8 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email now.strftime(DATETIME_FORMAT) ) - provider_tasks.send_email_to_provider.apply_async.assert_called_with((sample_email_template.service_id, - notification_id, - None), queue='email') + provider_tasks.send_email_to_provider.apply_async.assert_called_once_with((sample_email_template.service_id, + notification_id), queue='email') persisted_notification = Notification.query.filter_by(id=notification_id).one() assert persisted_notification.id == notification_id @@ -558,8 +554,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi now.strftime(DATETIME_FORMAT) ) provider_tasks.send_email_to_provider.apply_async.assert_called_once_with( - (sample_email_template_with_placeholders.service_id, - notification_id, None), queue='email' + (sample_email_template_with_placeholders.service_id, notification_id, ), queue='email' ) persisted_notification = Notification.query.filter_by(id=notification_id).one() assert persisted_notification.id == notification_id @@ -585,7 +580,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em now.strftime(DATETIME_FORMAT) ) provider_tasks.send_email_to_provider.apply_async.assert_called_once_with((sample_email_template.service_id, - notification_id, None), queue='email') + notification_id), queue='email') persisted_notification = Notification.query.filter_by(id=notification_id).one() assert persisted_notification.id == notification_id @@ -654,7 +649,7 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem assert 'No row was found for one' in str(e.value) -def test_process_email_job_should_use_reply_to_email_if_present(sample_email_job, mocker, mock_celery_remove_job): +def test_process_email_should_not_use_reply_to_email(sample_email_job, mocker, mock_celery_remove_job): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email')) mocker.patch('app.celery.tasks.send_email.apply_async') mocker.patch('app.encryption.encrypt', return_value='something_encrypted') @@ -671,6 +666,5 @@ def test_process_email_job_should_use_reply_to_email_if_present(sample_email_job "something_encrypted", ANY ), - {'reply_to_addresses': 'somereply@testservice.gov.uk'}, queue="bulk-email" )