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 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 a35e2e457..250f786eb 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, @@ -326,12 +328,13 @@ 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)) - except Exception as e: - # ? should this retry? + msg = "All notifications for job {} are not persisted".format(job_id) + current_app.logger.info(msg) + 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') @@ -523,9 +526,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/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/requirements.txt b/requirements.txt index 93743ed58..c186de052 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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 73b0b376a..98428f0b1 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,16 +1,17 @@ -import codecs import json import uuid from datetime import datetime, timedelta 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 @@ -991,7 +992,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 +1036,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, @@ -1071,9 +1072,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( @@ -1096,12 +1095,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="All notifications for job {} are not persisted".format(job.id)) + 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') @@ -1205,7 +1217,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): @@ -1225,7 +1236,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): 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) 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)