code review feedback

tests/app/aws/test_s3.py

@@ -219,6 +219,20 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker):
             2,
             "5555555552",
         ),
+        (
+            # simulate file saved with utf8withbom
+            "\\ufeffPHONE NUMBER\n",
+            "eee",
+            2,
+            "5555555552",
+        ),
+        (
+            # simulate file saved without utf8withbom
+            "\\PHONE NUMBER\n",
+            "eee",
+            2,
+            "5555555552",
+        ),
     ],
 )
 def test_get_phone_number_from_s3(

tests/app/celery/test_provider_tasks.py

@@ -1,4 +1,5 @@
 import json
+from unittest.mock import ANY
 
 import pytest
 from botocore.exceptions import ClientError
@@ -148,7 +149,7 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task
     deliver_sms(notification_id)
     app.delivery.send_to_providers.send_sms_to_provider.assert_not_called()
     app.celery.provider_tasks.deliver_sms.retry.assert_called_with(
-        queue="retry-tasks", countdown=0
+        queue="retry-tasks", countdown=0, expires=ANY
     )
 
 
@@ -208,7 +209,7 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(
     assert str(sample_notification.id) in str(e.value)
 
     provider_tasks.deliver_sms.retry.assert_called_with(
-        queue="retry-tasks", countdown=0
+        queue="retry-tasks", countdown=0, expires=ANY
     )
 
     assert sample_notification.status == NotificationStatus.TEMPORARY_FAILURE
@@ -240,7 +241,7 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_ta
     deliver_email(notification_id)
     app.delivery.send_to_providers.send_email_to_provider.assert_not_called()
     app.celery.provider_tasks.deliver_email.retry.assert_called_with(
-        queue="retry-tasks"
+        queue="retry-tasks", expires=ANY
     )
 
 
@@ -268,7 +269,9 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task
         deliver_email(sample_notification.id)
     assert str(sample_notification.id) in str(e.value)
 
-    provider_tasks.deliver_email.retry.assert_called_with(queue="retry-tasks")
+    provider_tasks.deliver_email.retry.assert_called_with(
+        queue="retry-tasks", expires=ANY
+    )
     assert sample_notification.status == NotificationStatus.TECHNICAL_FAILURE
 
 

tests/app/celery/test_scheduled_tasks.py

@@ -415,6 +415,7 @@ def test_check_for_missing_rows_in_completed_jobs_calls_save_email(
         ),
         {},
         queue="database-tasks",
+        expires=ANY,
     )
 
 

tests/app/celery/test_tasks.py

@@ -1,7 +1,7 @@
 import json
 import uuid
 from datetime import datetime, timedelta
-from unittest.mock import Mock, call
+from unittest.mock import ANY, Mock, call
 
 import pytest
 import requests_mock
@@ -115,6 +115,7 @@ def test_should_process_sms_job(sample_job, mocker):
         (str(sample_job.service_id), "uuid", "something_encrypted"),
         {},
         queue="database-tasks",
+        expires=ANY,
     )
     job = jobs_dao.dao_get_job_by_id(sample_job.id)
     assert job.job_status == JobStatus.FINISHED
@@ -135,6 +136,7 @@ def test_should_process_sms_job_with_sender_id(sample_job, mocker, fake_uuid):
         (str(sample_job.service_id), "uuid", "something_encrypted"),
         {"sender_id": fake_uuid},
         queue="database-tasks",
+        expires=ANY,
     )
 
 
@@ -179,6 +181,7 @@ def test_should_process_job_if_send_limits_are_not_exceeded(
         ),
         {},
         queue="database-tasks",
+        expires=ANY,
     )
 
 
@@ -237,6 +240,7 @@ def test_should_process_email_job(email_job_with_placeholders, mocker):
         ),
         {},
         queue="database-tasks",
+        expires=ANY,
     )
     job = jobs_dao.dao_get_job_by_id(email_job_with_placeholders.id)
     assert job.job_status == JobStatus.FINISHED
@@ -262,6 +266,7 @@ def test_should_process_email_job_with_sender_id(
         (str(email_job_with_placeholders.service_id), "uuid", "something_encrypted"),
         {"sender_id": fake_uuid},
         queue="database-tasks",
+        expires=ANY,
     )
 
 
@@ -351,6 +356,7 @@ def test_process_row_sends_letter_task(
         ),
         {},
         queue=expected_queue,
+        expires=ANY,
     )
 
 
@@ -387,6 +393,7 @@ def test_process_row_when_sender_id_is_provided(mocker, fake_uuid):
         ),
         {"sender_id": fake_uuid},
         queue="database-tasks",
+        expires=ANY,
     )
 
 
@@ -849,7 +856,9 @@ def test_save_sms_should_go_to_retry_queue_if_database_errors(sample_template, m
             encryption.encrypt(notification),
         )
     assert provider_tasks.deliver_sms.apply_async.called is False
-    tasks.save_sms.retry.assert_called_with(exc=expected_exception, queue="retry-tasks")
+    tasks.save_sms.retry.assert_called_with(
+        exc=expected_exception, queue="retry-tasks", expires=ANY
+    )
 
     assert _get_notification_query_count() == 0
 
@@ -878,7 +887,7 @@ def test_save_email_should_go_to_retry_queue_if_database_errors(
         )
     assert not provider_tasks.deliver_email.apply_async.called
     tasks.save_email.retry.assert_called_with(
-        exc=expected_exception, queue="retry-tasks"
+        exc=expected_exception, queue="retry-tasks", expires=ANY
     )
 
     assert _get_notification_query_count() == 0

tests/app/delivery/test_send_to_providers.py

@@ -94,6 +94,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
     mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
     mock_s3.return_value = "2028675309"
 
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
     mock_personalisation = mocker.patch(
         "app.delivery.send_to_providers.get_personalisation_from_s3"
     )
@@ -242,6 +243,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
     mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
     mock_s3.return_value = "2028675309"
 
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
     mock_s3_p = mocker.patch(
         "app.delivery.send_to_providers.get_personalisation_from_s3"
     )
@@ -336,6 +338,7 @@ def test_should_send_sms_with_downgraded_content(notify_db_session, mocker):
     # ī, grapes, tabs, zero width space and ellipsis are not
     # ó isn't in GSM, but it is in the welsh alphabet so will still be sent
 
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
     mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None)
     mocker.patch(
         "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"]
@@ -374,6 +377,7 @@ def test_send_sms_should_use_service_sms_sender(
 
     mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None)
     mocker.patch("app.aws_sns_client.send_sms")
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
 
     sms_sender = create_service_sms_sender(
         service=sample_service, sms_sender="123456", is_default=False
@@ -414,6 +418,8 @@ def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_c
     )
     mocker.patch("app.aws_ses_client.send_email")
     mocker.patch("app.delivery.send_to_providers.send_email_response")
+
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
     mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
     mock_phone.return_value = "15555555555"
 
@@ -636,6 +642,10 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_
 ):
 
     mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None)
+    mocker.patch(
+        "app.delivery.send_to_providers.update_notification_message_id",
+        return_value=None,
+    )
     mocker.patch(
         "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"]
     )
@@ -646,6 +656,11 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_
         key_type=key_type,
         reply_to_text="testing",
     )
+
+    mocker.patch(
+        "app.delivery.send_to_providers.update_notification_message_id",
+        return_value=None,
+    )
     mocker.patch("app.aws_sns_client.send_sms")
     mocker.patch(
         "app.delivery.send_to_providers.send_sms_response",
@@ -656,6 +671,8 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_
         sample_template.service.research_mode = True
 
     mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
     mock_phone.return_value = "15555555555"
 
     mock_personalisation = mocker.patch(
@@ -679,6 +696,8 @@ def test_should_set_notification_billable_units_and_reduces_provider_priority_if
     assert sample_notification.sent_by is None
 
     mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
     mock_phone.return_value = "15555555555"
 
     mock_personalisation = mocker.patch(
@@ -714,8 +733,14 @@ def test_should_send_sms_to_international_providers(
     )
 
     mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+
+    mocker.patch("app.delivery.send_to_providers.update_notification_message_id")
     mock_s3.return_value = "601117224412"
 
+    mocker.patch(
+        "app.delivery.send_to_providers.update_notification_message_id",
+        return_value=None,
+    )
     mock_personalisation = mocker.patch(
         "app.delivery.send_to_providers.get_personalisation_from_s3"
     )
@@ -753,6 +778,11 @@ def test_should_handle_sms_sender_and_prefix_message(
 
     mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None)
     mocker.patch("app.aws_sns_client.send_sms")
+
+    mocker.patch(
+        "app.delivery.send_to_providers.update_notification_message_id",
+        return_value=None,
+    )
     service = create_service_with_defined_sms_sender(
         sms_sender_value=sms_sender, prefix_sms=prefix_sms
     )
@@ -812,6 +842,11 @@ def test_send_sms_to_provider_should_use_normalised_to(mocker, client, sample_te
     mocker.patch(
         "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"]
     )
+
+    mocker.patch(
+        "app.delivery.send_to_providers.update_notification_message_id",
+        return_value=None,
+    )
     send_mock = mocker.patch("app.aws_sns_client.send_sms")
     notification = create_notification(
         template=sample_template,
@@ -875,6 +910,11 @@ def test_send_sms_to_provider_should_return_template_if_found_in_redis(
     mocker.patch(
         "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"]
     )
+
+    mocker.patch(
+        "app.delivery.send_to_providers.update_notification_message_id",
+        return_value=None,
+    )
     from app.schemas import service_schema, template_schema
 
     service_dict = service_schema.dump(sample_template.service)

tests/app/service/test_sender.py

@@ -58,6 +58,7 @@ def test_send_notification_to_service_users_includes_user_fields_in_personalisat
 ):
     persist_mock = mocker.patch("app.service.sender.persist_notification")
     mocker.patch("app.service.sender.send_notification_to_queue")
+    mocker.patch("app.service.sender.redis_store")
 
     user = sample_service.users[0]
 
@@ -82,13 +83,16 @@ def test_send_notification_to_service_users_sends_to_active_users_only(
     notify_service, mocker
 ):
     mocker.patch("app.service.sender.send_notification_to_queue")
+    mocker.patch("app.service.sender.redis_store", autospec=True)
 
     first_active_user = create_user(email="foo@bar.com", state="active")
     second_active_user = create_user(email="foo1@bar.com", state="active")
     pending_user = create_user(email="foo2@bar.com", state="pending")
     service = create_service(user=first_active_user)
     dao_add_user_to_service(service, second_active_user)
+
     dao_add_user_to_service(service, pending_user)
+
     template = create_template(service, template_type=TemplateType.EMAIL)
 
     send_notification_to_service_users(service_id=service.id, template_id=template.id)