From 92efa9c82c70cfc95d6faca88aab24ab27930f1c Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Wed, 22 Nov 2017 07:49:28 +0000 Subject: [PATCH 1/8] Update boto3 from 1.4.7 to 1.4.8 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 8d69902c5..e410286b2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ -boto3==1.4.7 +boto3==1.4.8 cffi==1.11.0 # pyup: != 1.11.1, != 1.11.2 # These versions are missing .whl celery==3.1.25 # pyup: <4 docopt==0.6.2 From 86098041fa4435e5d7008a5b3a1c43d98403958e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 27 Nov 2017 13:39:35 +0000 Subject: [PATCH 2/8] Add reply_to_text to Notification model. This was missed out of the previous PR and needs to be there before the next PR is merged. --- app/models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models.py b/app/models.py index a1322cf75..a95c09a6f 100644 --- a/app/models.py +++ b/app/models.py @@ -978,6 +978,8 @@ class Notification(db.Model): created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=True) + reply_to_text = db.Column(db.String, nullable=True) + __table_args__ = ( db.ForeignKeyConstraint( ['template_id', 'template_version'], From bcee95214ecb0bc7a9da51f6b7a3a3b339345c73 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 27 Nov 2017 13:46:39 +0000 Subject: [PATCH 3/8] Add sleep after stopping the old app instances on deploy sleep for 10 seconds to try and make sure that all worker threads (either web api or celery) have finished before we delete when we delete the DB is unbound from the app, which can cause "permission denied for relation" psycopg2 errors. --- Makefile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Makefile b/Makefile index 8ac0f2429..b4a03d2eb 100644 --- a/Makefile +++ b/Makefile @@ -277,6 +277,10 @@ cf-deploy: ## Deploys the app to Cloud Foundry cf rename ${CF_APP} ${CF_APP}-rollback cf push ${CF_APP} -f ${CF_MANIFEST_FILE} cf scale -i $$(cf curl /v2/apps/$$(cf app --guid ${CF_APP}-rollback) | jq -r ".entity.instances" 2>/dev/null || echo "1") ${CF_APP} + cf stop ${CF_APP}-rollback + # sleep for 10 seconds to try and make sure that all worker threads (either web api or celery) have finished before we delete + # when we delete the DB is unbound from the app, which can cause "permission denied for relation" psycopg2 errors. + sleep 10 cf delete -f ${CF_APP}-rollback .PHONY: cf-deploy-api-db-migration From b6ac7f074de59573f82661bf72ed2efd1122356d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 23 Nov 2017 13:49:52 +0000 Subject: [PATCH 4/8] celery.task.retry exc param should be a throwable. This causes an issue when it hits the max retry limit, and tries to throw your exception to let you deal with it - at this point it was moaning because we pass in a string if it's not defined, and we're inside an exception block celery uses that instead. --- app/celery/callback_tasks.py | 4 ++-- app/celery/tasks.py | 9 ++++----- tests/app/celery/test_tasks.py | 11 +++++------ 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/app/celery/callback_tasks.py b/app/celery/callback_tasks.py index 3de4b746e..6f59a33f5 100644 --- a/app/celery/callback_tasks.py +++ b/app/celery/callback_tasks.py @@ -13,6 +13,6 @@ def process_ses_results(self, response): errors = process_ses_response(response) if errors: current_app.logger.error(errors) - except Exception: + except Exception as exc: current_app.logger.exception('Error processing SES results') - self.retry(queue=QueueNames.RETRY, exc="SES responses processed with error") + self.retry(queue=QueueNames.RETRY) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index e96d7e3d8..7cfb06385 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -323,8 +323,9 @@ def build_dvla_file(self, job_id): ) dao_update_job_status(job_id, JOB_STATUS_READY_TO_SEND) else: - current_app.logger.info("All notifications for job {} are not persisted".format(job_id)) - self.retry(queue=QueueNames.RETRY, exc="All notifications for job {} are not persisted".format(job_id)) + msg = "All notifications for job {} are not persisted".format(job_id) + current_app.logger.info(msg) + self.retry(queue=QueueNames.RETRY, exc=Exception(msg)) except Exception as e: # ? should this retry? current_app.logger.exception("build_dvla_file threw exception") @@ -520,9 +521,7 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): ) if not isinstance(e, HTTPError) or e.response.status_code >= 500: try: - self.retry(queue=QueueNames.RETRY, - exc='Unable to send_inbound_sms_to_service for service_id: {} and url: {}. \n{}'.format( - service_id, inbound_api.url, e)) + self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index b3a046897..16498c40c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,8 +1,7 @@ -import codecs import json import uuid from datetime import datetime, timedelta -from unittest.mock import Mock +from unittest.mock import Mock, ANY import pytest import requests_mock from flask import current_app @@ -1049,8 +1048,10 @@ def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_let build_dvla_file(job.id) mocked.assert_not_called() - tasks.build_dvla_file.retry.assert_called_with(queue="retry-tasks", - exc="All notifications for job {} are not persisted".format(job.id)) + tasks.build_dvla_file.retry.assert_called_with( + queue="retry-tasks", + exc=ANY + ) assert Job.query.get(job.id).job_status == 'in progress' mocked_send_task.assert_not_called() @@ -1157,7 +1158,6 @@ def test_send_inbound_sms_to_service_retries_if_request_returns_500(notify_api, ) assert mocked.call_count == 1 assert mocked.call_args[1]['queue'] == 'retry-tasks' - assert exc_msg in mocked.call_args[1]['exc'] def test_send_inbound_sms_to_service_retries_if_request_throws_unknown(notify_api, sample_service, mocker): @@ -1177,7 +1177,6 @@ def test_send_inbound_sms_to_service_retries_if_request_throws_unknown(notify_ap ) assert mocked.call_count == 1 assert mocked.call_args[1]['queue'] == 'retry-tasks' - assert exc_msg in mocked.call_args[1]['exc'] def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(notify_api, sample_service, mocker): From 1e4a4802989605eee9740023619d8ecd018bcb07 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 27 Nov 2017 15:11:58 +0000 Subject: [PATCH 5/8] only catch s3 and db errors in build-dvla-file-for-job this reduces the amount of error messages we log (we'll no longer log at error level when build-dvla-file-for-job retries while waiting for the task to finish), and make sure we retry in those cases above - db or s3 having temporary troubl --- app/celery/tasks.py | 10 ++++++---- tests/app/celery/test_tasks.py | 29 ++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 7cfb06385..3a146cdea 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -18,6 +18,8 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError +from botocore.exceptions import ClientError as BotoClientError + from app import ( create_uuid, create_random_identifier, @@ -325,11 +327,11 @@ def build_dvla_file(self, job_id): else: msg = "All notifications for job {} are not persisted".format(job_id) current_app.logger.info(msg) - self.retry(queue=QueueNames.RETRY, exc=Exception(msg)) - except Exception as e: - # ? should this retry? + self.retry(queue=QueueNames.RETRY) + # specifically don't catch celery.retry errors + except (SQLAlchemyError, BotoClientError): current_app.logger.exception("build_dvla_file threw exception") - raise e + self.retry(queue=QueueNames.RETRY) @notify_celery.task(bind=True, name='update-letter-job-to-sent') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 16498c40c..ad516a5e4 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,15 +1,17 @@ import json import uuid from datetime import datetime, timedelta -from unittest.mock import Mock, ANY +from unittest.mock import Mock + import pytest import requests_mock from flask import current_app from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError -from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from celery.exceptions import Retry +from botocore.exceptions import ClientError +from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks @@ -1023,9 +1025,7 @@ def test_build_dvla_file(sample_letter_template, mocker): create_notification(template=job.template, job=job) mocked_upload = mocker.patch("app.celery.tasks.s3upload") mocked_send_task = mocker.patch("app.celery.tasks.notify_celery.send_task") - mocked_letter_template = mocker.patch("app.celery.tasks.LetterDVLATemplate") - mocked_letter_template_instance = mocked_letter_template.return_value - mocked_letter_template_instance.__str__.return_value = "dvla|string" + mocker.patch("app.celery.tasks.LetterDVLATemplate", return_value='dvla|string') build_dvla_file(job.id) mocked_upload.assert_called_once_with( @@ -1048,14 +1048,25 @@ def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_let build_dvla_file(job.id) mocked.assert_not_called() - tasks.build_dvla_file.retry.assert_called_with( - queue="retry-tasks", - exc=ANY - ) + tasks.build_dvla_file.retry.assert_called_with(queue="retry-tasks") assert Job.query.get(job.id).job_status == 'in progress' mocked_send_task.assert_not_called() +def test_build_dvla_file_retries_if_s3_err(sample_letter_template, mocker): + job = create_job(sample_letter_template, notification_count=1) + create_notification(job.template, job=job) + + mocker.patch('app.celery.tasks.LetterDVLATemplate', return_value='dvla|string') + mocker.patch('app.celery.tasks.s3upload', side_effect=ClientError({}, 'operation_name')) + retry_mock = mocker.patch('app.celery.tasks.build_dvla_file.retry', side_effect=Retry) + + with pytest.raises(Retry): + build_dvla_file(job.id) + + retry_mock.assert_called_once_with(queue='retry-tasks') + + def test_create_dvla_file_contents(notify_db_session, mocker): service = create_service(service_permissions=SERVICE_PERMISSION_TYPES) create_letter_contact(service=service, contact_block='London,\nNW1A 1AA') From ab5b7c20a7516d81442c08962b634ea26cc06688 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 27 Nov 2017 16:52:52 +0000 Subject: [PATCH 6/8] Use sms sender or reply to email address of the Notify service in create_2fa_code depending on message type. --- app/user/rest.py | 8 ++++++-- tests/app/user/test_rest_verify.py | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 6a264e7fe..4b4e9c943 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -214,7 +214,11 @@ def create_2fa_code(template_id, user_to_send_to, secret_code, recipient, person # save the code in the VerifyCode table create_user_code(user_to_send_to, secret_code, template.template_type) - + reply_to = None + if template.template_type == SMS_TYPE: + reply_to = template.service.get_default_sms_sender() + elif template.template_type == EMAIL_TYPE: + reply_to = template.service.get_default_reply_to_email_address() saved_notification = persist_notification( template_id=template.id, template_version=template.version, @@ -224,7 +228,7 @@ def create_2fa_code(template_id, user_to_send_to, secret_code, recipient, person notification_type=template.template_type, api_key_id=None, key_type=KEY_TYPE_NORMAL, - reply_to_text=template.service.get_default_sms_sender() + reply_to_text=reply_to ) # 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 diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index adf39aa96..d361987d6 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -357,6 +357,7 @@ def test_send_user_email_code(admin_request, mocker, sample_user, email_2fa_code _expected_status=204 ) noti = Notification.query.one() + assert noti.reply_to_text == email_2fa_code_template.service.get_default_reply_to_email_address() assert noti.to == sample_user.email_address assert str(noti.template_id) == current_app.config['EMAIL_2FA_TEMPLATE_ID'] assert noti.personalisation['name'] == 'Test User' From 2b54c2308b2bfa7e237d765fb4da5582febc19de Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Nov 2017 10:42:54 +0000 Subject: [PATCH 7/8] fix test that did not depend on the notify-db-session fixture, so left a messy db --- tests/app/v2/notifications/test_get_notifications.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index de80073bd..6bef10a57 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -339,10 +339,9 @@ def test_get_all_notifications_no_notifications_if_no_notifications(client, samp assert len(json_response['notifications']) == 0 -def test_get_all_notifications_filter_by_template_type(client): - service = create_service() - email_template = create_template(service=service, template_type="email") - sms_template = create_template(service=service, template_type="sms") +def test_get_all_notifications_filter_by_template_type(client, sample_service): + email_template = create_template(service=sample_service, template_type="email") + sms_template = create_template(service=sample_service, template_type="sms") notification = create_notification(template=email_template, to_field="don.draper@scdp.biz") create_notification(template=sms_template) From b0d88ec08c3725f730021b5b9a5598d18d572ed1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 28 Nov 2017 11:05:56 +0000 Subject: [PATCH 8/8] Updates as per review comments --- app/service/send_notification.py | 7 +++++-- tests/app/celery/test_tasks.py | 4 ++-- tests/app/conftest.py | 14 +++++++++++--- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 068331167..5274d5e2b 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -4,7 +4,8 @@ from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.notifications.validators import ( check_service_over_daily_message_limit, validate_and_format_recipient, - validate_template, check_service_sms_sender_id) + validate_template +) from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, @@ -15,7 +16,9 @@ from app.models import ( KEY_TYPE_NORMAL, PRIORITY, SMS_TYPE, - EMAIL_TYPE, LETTER_TYPE) + EMAIL_TYPE, + LETTER_TYPE +) from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id_and_service_id from app.dao.users_dao import get_user_by_id diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 444469731..df1bf92f4 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -991,7 +991,7 @@ def test_save_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample def test_save_letter_saves_letter_to_database(mocker, notify_db_session): service = create_service() - create_letter_contact(service=service, contact_block="Address contract", is_default=True) + create_letter_contact(service=service, contact_block="Address contact", is_default=True) template = create_template(service=service, template_type=LETTER_TYPE) job = create_job(template=template) @@ -1035,7 +1035,7 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): assert notification_db.sent_by is None assert notification_db.personalisation == personalisation assert notification_db.reference == "this-is-random-in-real-life" - assert notification_db.reply_to_text == "Address contract" + assert notification_db.reply_to_text == "Address contact" def test_should_cancel_job_if_service_is_inactive(sample_service, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 611baf810..79a56ed10 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -27,9 +27,17 @@ from app.models import ( ProviderRates, ScheduledNotification, ServiceWhitelist, - KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, - MOBILE_TYPE, EMAIL_TYPE, INBOUND_SMS_TYPE, SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, - SERVICE_PERMISSION_TYPES, ServiceEmailReplyTo + KEY_TYPE_NORMAL, + KEY_TYPE_TEST, + KEY_TYPE_TEAM, + MOBILE_TYPE, + EMAIL_TYPE, + INBOUND_SMS_TYPE, + SMS_TYPE, + LETTER_TYPE, + NOTIFICATION_STATUS_TYPES_COMPLETED, + SERVICE_PERMISSION_TYPES, + ServiceEmailReplyTo ) from app.dao.users_dao import (create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service)