diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index 640630733..df8676512 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -97,8 +97,9 @@ def _send_data_to_service_callback_api(self, data, service_callback_url, token, self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: current_app.logger.error( - "Retry: {} has retried the max num of times for notification: {}".format( + "Retry: {} has retried the max num of times for callback url {} and notification_id: {}".format( function_name, + service_callback_url, notification_id ) ) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 95896e4da..f170c2410 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -143,7 +143,7 @@ def _update_notification_status(notification, status): @statsd(namespace="dao") @transactional -def update_notification_status_by_id(notification_id, status): +def update_notification_status_by_id(notification_id, status, sent_by=None): notification = Notification.query.with_lockmode("update").filter( Notification.id == notification_id, or_( @@ -158,7 +158,8 @@ def update_notification_status_by_id(notification_id, status): if notification.international and not country_records_delivery(notification.phone_prefix): return None - + if not notification.sent_by and sent_by: + notification.sent_by = sent_by return _update_notification_status( notification=notification, status=status diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index ce2f023e5..d45c3cc92 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -15,6 +15,7 @@ from app.celery.service_callback_tasks import ( from app.config import QueueNames from app.dao.notifications_dao import dao_update_notification from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service +from app.models import NOTIFICATION_PENDING sms_response_mapper = { 'MMG': get_mmg_responses, @@ -74,7 +75,11 @@ def process_sms_client_response(status, provider_reference, client_name): def _process_for_status(notification_status, client_name, provider_reference): # record stats - notification = notifications_dao.update_notification_status_by_id(provider_reference, notification_status) + notification = notifications_dao.update_notification_status_by_id( + notification_id=provider_reference, + status=notification_status, + sent_by=client_name.lower() + ) if not notification: current_app.logger.warning("{} callback failed: notification {} either not found or already updated " "from sending. Status {}".format(client_name, @@ -84,9 +89,6 @@ def _process_for_status(notification_status, client_name, provider_reference): statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status)) - if not notification.sent_by: - set_notification_sent_by(notification, client_name.lower()) - if notification.sent_at: statsd_client.timing_with_dates( 'callback.{}.elapsed-time'.format(client_name.lower()), @@ -94,13 +96,13 @@ def _process_for_status(notification_status, client_name, provider_reference): notification.sent_at ) - # queue callback task only if the service_callback_api exists - service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id) - - if service_callback_api: - encrypted_notification = create_delivery_status_callback_data(notification, service_callback_api) - send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification], - queue=QueueNames.CALLBACKS) + if notification_status != NOTIFICATION_PENDING: + service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id) + # queue callback task only if the service_callback_api exists + if service_callback_api: + encrypted_notification = create_delivery_status_callback_data(notification, service_callback_api) + send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification], + queue=QueueNames.CALLBACKS) success = "{} callback succeeded. reference {} updated".format(client_name, provider_reference) return success diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 0c3eafd11..ad3a9aa41 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -141,6 +141,14 @@ def test_should_update_status_by_id_if_created(notify_db, notify_db_session): assert updated.status == 'failed' +def test_should_update_status_by_id_and_set_sent_by(notify_db, notify_db_session): + notification = sample_notification(notify_db, notify_db_session, status='sending') + + updated = update_notification_status_by_id(notification.id, 'delivered', sent_by='mmg') + assert updated.status == 'delivered' + assert updated.sent_by == 'mmg' + + def test_should_not_update_status_by_reference_if_from_country_with_no_delivery_receipts(sample_template): notification = create_notification( sample_template, diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 6a22a2b03..474aa732b 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -89,6 +89,13 @@ def test_process_sms_response_return_success_for_send_sms_code_reference(mocker) assert error is None +def test_process_sms_response_does_not_send_status_update_for_pending(sample_notification, mocker): + send_mock = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async') + process_sms_client_response( + status='2', provider_reference=str(sample_notification.id), client_name='firetext') + send_mock.assert_not_called() + + def test_process_sms_updates_sent_by_with_client_name_if_not_in_noti(notify_db, sample_notification): sample_notification.sent_by = None success, error = process_sms_client_response(