From 4b23075488ac402f705ad970552c03326b93b971 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 1 Nov 2018 16:06:07 +0000 Subject: [PATCH 1/2] Delete unused parameters from the save_email and save_sms jobs These both had default arguments of `api_key_id` and `key_type` which were never being passed in, so these have been removed. --- app/celery/tasks.py | 20 +++----- tests/app/celery/test_tasks.py | 94 ++-------------------------------- 2 files changed, 11 insertions(+), 103 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 290a19a0c..a6371c6be 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -181,14 +181,12 @@ def __sending_limits_for_job_exceeded(service, job, job_id): def save_sms(self, service_id, notification_id, - encrypted_notification, - api_key_id=None, - key_type=KEY_TYPE_NORMAL): + encrypted_notification): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) template = dao_get_template_by_id(notification['template'], version=notification['template_version']) - if not service_allowed_to_send_to(notification['to'], service, key_type): + if not service_allowed_to_send_to(notification['to'], service, KEY_TYPE_NORMAL): current_app.logger.debug( "SMS {} failed as restricted service".format(notification_id) ) @@ -202,8 +200,8 @@ def save_sms(self, service=service, personalisation=notification.get('personalisation'), notification_type=SMS_TYPE, - api_key_id=api_key_id, - key_type=key_type, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, created_at=datetime.utcnow(), job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), @@ -232,15 +230,13 @@ def save_sms(self, def save_email(self, service_id, notification_id, - encrypted_notification, - api_key_id=None, - key_type=KEY_TYPE_NORMAL): + encrypted_notification): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) template = dao_get_template_by_id(notification['template'], version=notification['template_version']) - if not service_allowed_to_send_to(notification['to'], service, key_type): + if not service_allowed_to_send_to(notification['to'], service, KEY_TYPE_NORMAL): current_app.logger.info("Email {} failed as restricted service".format(notification_id)) return @@ -252,8 +248,8 @@ def save_email(self, service=service, personalisation=notification.get('personalisation'), notification_type=EMAIL_TYPE, - api_key_id=api_key_id, - key_type=key_type, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, created_at=datetime.utcnow(), job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 6f9082408..71880f1d2 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -37,8 +37,6 @@ from app.models import ( NotificationHistory, EMAIL_TYPE, KEY_TYPE_NORMAL, - KEY_TYPE_TEAM, - KEY_TYPE_TEST, JOB_STATUS_FINISHED, JOB_STATUS_ERROR, JOB_STATUS_IN_PROGRESS, @@ -489,58 +487,6 @@ def test_should_save_sms_if_restricted_service_and_valid_number(notify_db, notif ) -def test_should_save_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, - notify_db_session, - mocker): - user = create_user(mobile_number="07700 900205") - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = create_sample_template(notify_db, notify_db_session, service=service) - - notification = _notification_json(template, "07700 900849") - mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - - notification_id = uuid.uuid4() - save_sms( - service.id, - notification_id, - encryption.encrypt(notification), - key_type=KEY_TYPE_TEST - ) - - persisted_notification = Notification.query.one() - mocked_deliver_sms.assert_called_once_with( - [str(persisted_notification.id)], - queue="send-sms-tasks" - ) - - -def test_should_save_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db, - notify_db_session, - mocker): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello' - ) - - notification = _notification_json(template, to="test@example.com") - mocked_deliver_email = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - - notification_id = uuid.uuid4() - save_email( - service.id, - notification_id, - encryption.encrypt(notification), - key_type=KEY_TYPE_TEST - ) - - persisted_notification = Notification.query.one() - mocked_deliver_email.assert_called_once_with( - [str(persisted_notification.id)], - queue="send-email-tasks" - ) - - def test_save_email_should_save_default_email_reply_to_text_on_notification(notify_db_session, mocker): service = create_service() create_reply_to_email(service=service, email_address='reply_to@digital.gov.uk', is_default=True) @@ -554,7 +500,6 @@ def test_save_email_should_save_default_email_reply_to_text_on_notification(noti service.id, notification_id, encryption.encrypt(notification), - key_type=KEY_TYPE_TEST ) persisted_notification = Notification.query.one() @@ -573,7 +518,6 @@ def test_save_sms_should_save_default_smm_sender_notification_reply_to_text_on(n service.id, notification_id, encryption.encrypt(notification), - key_type=KEY_TYPE_TEST ) persisted_notification = Notification.query.one() @@ -644,7 +588,7 @@ def test_should_put_save_email_task_in_research_mode_queue_if_research_mode_serv ) -def test_should_save_sms_template_to_and_persist_with_job_id(sample_job, sample_api_key, mocker): +def test_should_save_sms_template_to_and_persist_with_job_id(sample_job, mocker): notification = _notification_json( sample_job.template, to="+447234123123", @@ -658,8 +602,6 @@ def test_should_save_sms_template_to_and_persist_with_job_id(sample_job, sample_ sample_job.service.id, notification_id, encryption.encrypt(notification), - api_key_id=str(sample_api_key.id), - key_type=KEY_TYPE_NORMAL ) persisted_notification = Notification.query.one() assert persisted_notification.to == '+447234123123' @@ -670,7 +612,7 @@ def test_should_save_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert persisted_notification.created_at >= now assert not persisted_notification.sent_by assert persisted_notification.job_row_number == 2 - assert persisted_notification.api_key_id == sample_api_key.id + assert persisted_notification.api_key_id is None assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'sms' @@ -680,33 +622,6 @@ def test_should_save_sms_template_to_and_persist_with_job_id(sample_job, sample_ ) -def test_should_not_save_email_if_team_key_and_recipient_not_in_team(sample_email_template_with_placeholders, - sample_team_api_key, - mocker): - notification = _notification_json( - sample_email_template_with_placeholders, - "my_email@my_email.com", - {"name": "Jo"}, - row_number=1) - apply_async = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - notification_id = uuid.uuid4() - - team_members = [user.email_address for user in sample_email_template_with_placeholders.service.users] - assert "my_email@my_email.com" not in team_members - - save_email( - sample_email_template_with_placeholders.service_id, - notification_id, - encryption.encrypt(notification), - api_key_id=str(sample_team_api_key.id), - key_type=KEY_TYPE_TEAM - ) - - assert Notification.query.count() == 0 - - apply_async.not_called() - - def test_should_not_save_sms_if_team_key_and_recipient_not_in_team(notify_db, notify_db_session, mocker): assert Notification.query.count() == 0 user = create_user(mobile_number="07700 900205") @@ -747,8 +662,6 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh sample_email_template_with_placeholders.service_id, notification_id, encryption.encrypt(notification), - api_key_id=str(sample_api_key.id), - key_type=sample_api_key.key_type ) persisted_notification = Notification.query.one() @@ -762,7 +675,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.job_row_number == 1 assert persisted_notification.personalisation == {'name': 'Jo'} assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) - assert persisted_notification.api_key_id == sample_api_key.id + assert persisted_notification.api_key_id is None assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'email' @@ -1112,7 +1025,6 @@ def test_save_sms_uses_sms_sender_reply_to_text(mocker, notify_db_session): service.id, notification_id, encryption.encrypt(notification), - key_type=KEY_TYPE_TEST ) persisted_notification = Notification.query.one() From 30fe41fd43589b8e850b2c74d62c4be84747acf1 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 1 Nov 2018 17:00:44 +0000 Subject: [PATCH 2/2] Pass sender_id argument to tasks Started passing `sender_id` to the `save_email`, `save_sms` and `process_job` tasks, with a default value of `None`. If `sender_id` is provided, the `save_email` and `save_sms` tasks will use it to determine the reply-to email address or the SMS sender for the notifications in the job. The `process_job` task will start using the value in another commit. --- app/celery/tasks.py | 24 ++++++++++--- tests/app/celery/test_tasks.py | 62 +++++++++++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a6371c6be..a7713e668 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -45,7 +45,9 @@ from app.dao.notifications_dao import ( dao_get_notification_by_reference, ) from app.dao.provider_details_dao import get_current_provider +from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service +from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id from app.exceptions import DVLAException, NotificationTechnicalFailureException @@ -75,7 +77,7 @@ from app.utils import convert_utc_to_bst @notify_celery.task(name="process-job") @statsd(namespace="tasks") -def process_job(job_id): +def process_job(job_id, sender_id=None): start = datetime.utcnow() job = dao_get_job_by_id(job_id) @@ -181,11 +183,17 @@ def __sending_limits_for_job_exceeded(service, job, job_id): def save_sms(self, service_id, notification_id, - encrypted_notification): + encrypted_notification, + sender_id=None): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) template = dao_get_template_by_id(notification['template'], version=notification['template_version']) + if sender_id: + reply_to_text = dao_get_service_sms_senders_by_id(service_id, sender_id).sms_sender + else: + reply_to_text = template.get_reply_to_text() + if not service_allowed_to_send_to(notification['to'], service, KEY_TYPE_NORMAL): current_app.logger.debug( "SMS {} failed as restricted service".format(notification_id) @@ -206,7 +214,7 @@ def save_sms(self, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), notification_id=notification_id, - reply_to_text=template.get_reply_to_text() + reply_to_text=reply_to_text ) provider_tasks.deliver_sms.apply_async( @@ -230,12 +238,18 @@ def save_sms(self, def save_email(self, service_id, notification_id, - encrypted_notification): + encrypted_notification, + sender_id=None): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) template = dao_get_template_by_id(notification['template'], version=notification['template_version']) + if sender_id: + reply_to_text = dao_get_reply_to_by_id(service_id, sender_id).email_address + else: + reply_to_text = template.get_reply_to_text() + if not service_allowed_to_send_to(notification['to'], service, KEY_TYPE_NORMAL): current_app.logger.info("Email {} failed as restricted service".format(notification_id)) return @@ -254,7 +268,7 @@ def save_email(self, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), notification_id=notification_id, - reply_to_text=template.get_reply_to_text() + reply_to_text=reply_to_text ) provider_tasks.deliver_email.apply_async( diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 71880f1d2..0348c6438 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -30,7 +30,7 @@ from app.celery.tasks import ( process_returned_letters_list, ) from app.config import QueueNames -from app.dao import jobs_dao, services_dao +from app.dao import jobs_dao, services_dao, service_email_reply_to_dao, service_sms_sender_dao from app.models import ( Job, Notification, @@ -739,6 +739,46 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi ) +def test_save_email_uses_the_reply_to_text_when_provided(sample_email_template, mocker): + notification = _notification_json(sample_email_template, "my_email@my_email.com") + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + + service = sample_email_template.service + notification_id = uuid.uuid4() + service_email_reply_to_dao.add_reply_to_email_address_for_service(service.id, 'default@example.com', True) + other_email_reply_to = service_email_reply_to_dao.add_reply_to_email_address_for_service( + service.id, 'other@example.com', False) + + save_email( + sample_email_template.service_id, + notification_id, + encryption.encrypt(notification), + sender_id=other_email_reply_to.id, + ) + persisted_notification = Notification.query.one() + assert persisted_notification.notification_type == 'email' + assert persisted_notification.reply_to_text == 'other@example.com' + + +def test_save_email_uses_the_default_reply_to_text_if_sender_id_is_none(sample_email_template, mocker): + notification = _notification_json(sample_email_template, "my_email@my_email.com") + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + + service = sample_email_template.service + notification_id = uuid.uuid4() + service_email_reply_to_dao.add_reply_to_email_address_for_service(service.id, 'default@example.com', True) + + save_email( + sample_email_template.service_id, + notification_id, + encryption.encrypt(notification), + sender_id=None, + ) + persisted_notification = Notification.query.one() + assert persisted_notification.notification_type == 'email' + assert persisted_notification.reply_to_text == 'default@example.com' + + def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker): notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') @@ -1031,6 +1071,26 @@ def test_save_sms_uses_sms_sender_reply_to_text(mocker, notify_db_session): assert persisted_notification.reply_to_text == '447123123123' +def test_save_sms_uses_non_default_sms_sender_reply_to_text_if_provided(mocker, notify_db_session): + service = create_service_with_defined_sms_sender(sms_sender_value='07123123123') + template = create_template(service=service) + new_sender = service_sms_sender_dao.dao_add_sms_sender_for_service(service.id, 'new-sender', False) + + notification = _notification_json(template, to="07700 900205") + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + notification_id = uuid.uuid4() + save_sms( + service.id, + notification_id, + encryption.encrypt(notification), + sender_id=new_sender.id, + ) + + persisted_notification = Notification.query.one() + assert persisted_notification.reply_to_text == 'new-sender' + + @pytest.mark.parametrize('env', ['staging', 'live']) def test_save_letter_sets_delivered_letters_as_pdf_permission_in_research_mode_in_staging_live( notify_api, mocker, notify_db_session, sample_letter_job, env):