Merge pull request #470 from NHSDigital/remove-delay-from-retry-command

Remove delay from retry command

Author: steventux

manage_breast_screening/config/.env.tpl

@@ -24,7 +24,6 @@ CIS2_CLIENT_PUBLIC_KEY="paste-pem-public-key-here"
 CIS2_DEBUG=False
 
 NOTIFICATIONS_BATCH_RETRY_LIMIT=5
-NOTIFICATIONS_BATCH_RETRY_DELAY=10
 
 BASIC_AUTH_ENABLED=False
 BASIC_AUTH_USERNAME=changeme

manage_breast_screening/notifications/management/commands/retry_failed_message_batch.py

@@ -1,6 +1,5 @@
 import json
 import os
-import time
 from logging import getLogger
 
 from django.core.management.base import BaseCommand, CommandError
@@ -41,19 +40,21 @@ def handle(self, *args, **options):
 
         if message_batch is None:
             raise CommandError(
-                f"Message Batch with id {message_batch_id} and status of '{MessageBatchStatusChoices.FAILED_RECOVERABLE.value}' not found"
+                (
+                    f"Message Batch with id {message_batch_id} and status of "
+                    f"'{MessageBatchStatusChoices.FAILED_RECOVERABLE.value}' not found"
+                )
             )
 
         queue.delete(queue_message)
-        logger.info(f"Message Batch with id {message_batch_id} deleted from queue")
+        logger.info("Message Batch with id %s deleted from queue", message_batch_id)
 
         retry_count = int(json.loads(queue_message.content)["retry_count"])
         if retry_count < int(os.getenv("NOTIFICATIONS_BATCH_RETRY_LIMIT", "5")):
             logger.info(
-                f"Retrying Message Batch with id {message_batch_id} with retry count {retry_count}"
-            )
-            time.sleep(
-                int(os.getenv("NOTIFICATIONS_BATCH_RETRY_DELAY", "0")) * retry_count
+                "Retrying Message Batch with id %s with retry count %s",
+                message_batch_id,
+                retry_count,
             )
 
             try:
@@ -73,8 +74,9 @@ def handle(self, *args, **options):
             except Exception as e:
                 raise CommandError(e)
         else:
-            logger.info(
-                f"Failed Message Batch with id {message_batch_id} not sent: Retry limit exceeded"
+            logger.error(
+                "Failed Message Batch with id %s not sent: Retry limit exceeded",
+                message_batch_id,
             )
             message_batch.status = MessageBatchStatusChoices.FAILED_UNRECOVERABLE.value
             message_batch.save()

manage_breast_screening/notifications/tests/management/commands/test_retry_failed_message_batch.py

@@ -29,12 +29,10 @@ def setup(monkeypatch):
 @patch.object(Queue, "RetryMessageBatches", return_value=MagicMock(spec=Queue))
 @patch.object(MessageBatchHelpers, "mark_batch_as_sent")
 @patch.object(MessageBatchHelpers, "mark_batch_as_failed")
-@patch("time.sleep", return_value=MagicMock())
 class TestRetryFailedMessageBatch:
     @pytest.mark.django_db
     def test_handle_batch_not_found(
         self,
-        mock_sleep,
         mock_mark_batch_as_failed,
         mock_mark_batch_as_sent,
         mock_retry_message_batches,
@@ -59,7 +57,6 @@ def test_handle_batch_not_found(
     @pytest.mark.django_db
     def test_not_failed_yet(
         self,
-        mock_sleep,
         mock_mark_batch_as_failed,
         mock_mark_batch_as_sent,
         mock_retry_message_batches,
@@ -83,7 +80,6 @@ def test_not_failed_yet(
     @pytest.mark.django_db
     def test_server_error_when_resending_batch(
         self,
-        mock_sleep,
         mock_mark_batch_as_failed,
         mock_mark_batch_as_sent,
         mock_retry_message_batches,
@@ -110,7 +106,6 @@ def test_server_error_when_resending_batch(
     @pytest.mark.django_db
     def test_successfully_resends_batch(
         self,
-        mock_sleep,
         mock_mark_batch_as_failed,
         mock_mark_batch_as_sent,
         mock_retry_message_batches,
@@ -133,12 +128,10 @@ def test_successfully_resends_batch(
             message_batch=failed_batch, response_json=ANY
         )
         mock_retry_message_batches.return_value.delete.assert_called_with(mocked_item())
-        mock_sleep.assert_called_with(10)
 
     @pytest.mark.django_db
     def test_no_batches_in_queue(
         self,
-        mock_sleep,
         mock_mark_batch_as_failed,
         mock_mark_batch_as_sent,
         mock_retry_message_batches,
@@ -157,7 +150,6 @@ def test_no_batches_in_queue(
     @pytest.mark.django_db
     def test_batch_with_retry_count_more_than_5_is_marked_as_failed_unrecoverable(
         self,
-        mock_sleep,
         mock_mark_batch_as_failed,
         mock_mark_batch_as_sent,
         mock_retry_message_batches,
@@ -178,24 +170,3 @@ def test_batch_with_retry_count_more_than_5_is_marked_as_failed_unrecoverable(
             str(error.value)
             == f"Message Batch with id {batch_id} not sent: Retry limit exceeded"
         )
-
-    @pytest.mark.django_db
-    def test_sleeps_based_on_retry_count(
-        self,
-        mock_sleep,
-        mock_mark_batch_as_failed,
-        mock_mark_batch_as_sent,
-        mock_retry_message_batches,
-        mock_send_message_batch,
-    ):
-        mock_send_message_batch.return_value.status_code = 201
-        subject = Command()
-        batch_id = uuid.uuid4()
-        _failed_batch = MessageBatchFactory(id=batch_id, status="failed_recoverable")
-        mock_retry_message_batches.return_value.item.return_value.content = json.dumps(
-            {"message_batch_id": str(batch_id), "retry_count": 3}
-        )
-
-        subject.handle()
-
-        mock_sleep.assert_called_with(30)