diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 3ae0486bc..f6d6f7847 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -11,7 +11,7 @@ from app.clients.sms import SmsClientResponseException from app.config import QueueNames from app.dao import notifications_dao from app.dao.notifications_dao import ( - sanitize_notifications_by_id, + sanitize_successful_notification_by_id, update_notification_status_by_id, ) from app.delivery import send_to_providers @@ -37,14 +37,16 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): status, provider_response = aws_cloudwatch_client.check_sms(message_id, notification_id, sent_at) if status == 'success': status = NOTIFICATION_DELIVERED - else: + elif status == 'failure': status = NOTIFICATION_FAILED - update_notification_status_by_id(notification_id, status, provider_response=provider_response) - current_app.logger.info(f"Updated notification {notification_id} with response '{provider_response}'") + # if status is not success or failure the client raised an exception and this method will retry if status == NOTIFICATION_DELIVERED: - sanitize_notifications_by_id(notification_id) + sanitize_successful_notification_by_id(notification_id) current_app.logger.info(f"Sanitized notification {notification_id} that was successfully delivered") + else: + update_notification_status_by_id(notification_id, status, provider_response=provider_response) + current_app.logger.info(f"Updated notification {notification_id} with response '{provider_response}'") @notify_celery.task(bind=True, name="deliver_sms", max_retries=48, default_retry_delay=300) diff --git a/app/clients/cloudwatch/aws_cloudwatch.py b/app/clients/cloudwatch/aws_cloudwatch.py index 97de58219..532472295 100644 --- a/app/clients/cloudwatch/aws_cloudwatch.py +++ b/app/clients/cloudwatch/aws_cloudwatch.py @@ -84,6 +84,6 @@ class AwsCloudwatchClient(Client): if all_failed_events and len(all_failed_events) > 0: event = all_failed_events[0] message = json.loads(event['message']) - return "fail", message['delivery']['providerResponse'] + return "failure", message['delivery']['providerResponse'] raise Exception(f'No event found for message_id {message_id} notification_id {notification_id}') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6969874ad..fd4518b34 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -272,7 +272,7 @@ def _filter_query(query, filter_dict=None): @autocommit -def sanitize_notifications_by_id( +def sanitize_successful_notification_by_id( notification_id ): # TODO what to do for international? @@ -280,12 +280,10 @@ def sanitize_notifications_by_id( Notification.query.filter( Notification.id.in_([notification_id]), ).update( - {'to': phone_prefix, 'normalised_to': phone_prefix}, + {'to': phone_prefix, 'normalised_to': phone_prefix, 'status': 'delivered'}, synchronize_session=False ) - db.session.commit() - @autocommit def insert_notification_history_delete_notifications( diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 2ca9b5032..23fcb426c 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -24,6 +24,7 @@ from app.dao.notifications_dao import ( get_notifications_for_service, get_service_ids_with_notifications_on_date, notifications_not_yet_sent, + sanitize_successful_notification_by_id, update_notification_status_by_id, update_notification_status_by_reference, ) @@ -84,6 +85,26 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, sns_ assert notification.status == 'delivered' +def test_should_be_able_to_sanitize_successful_notification(sample_template, sample_job, sns_provider): + with freeze_time('2000-01-01 12:00:00'): + data = _notification_json(sample_template, job_id=sample_job.id, status='sending') + notification = Notification(**data) + notification.to = '15555555555' + notification.normalised_to = '15555555555' + dao_create_notification(notification) + assert notification.status == 'sending' + assert notification.normalised_to == '15555555555' + assert notification.to == '15555555555' + + assert Notification.query.get(notification.id).status == 'sending' + + with freeze_time('2000-01-02 12:00:00'): + sanitize_successful_notification_by_id(notification.id) + assert Notification.query.get(notification.id).status == 'delivered' + assert Notification.query.get(notification.id).normalised_to == '1' + assert Notification.query.get(notification.id).to == '1' + + def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(sample_job): notification = create_notification(template=sample_job.template, status='delivered', job=sample_job) assert Notification.query.get(notification.id).status == 'delivered'