Some minor refactoring.

- Updated notifications_dao.update_notification_status_by_id with an optional parameter to set the sent_by, this will eliminate a separate update to notifcaitons.
- Added the callback url to the log message, that way we can see if it's the same url failing.
- Stop sending the status callbacks for PENDING status.

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
                     )
                 )

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

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
+    if notification_status != NOTIFICATION_PENDING:
-    service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id)
+        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:
+        if service_callback_api:
-        encrypted_notification = create_delivery_status_callback_data(notification, 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],
+            send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification],
-                                                    queue=QueueNames.CALLBACKS)
+                                                        queue=QueueNames.CALLBACKS)
 
     success = "{} callback succeeded. reference {} updated".format(client_name, provider_reference)
     return success

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,

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(