increase code coverage

.ds.baseline

@@ -331,15 +331,15 @@
         "filename": "tests/app/user/test_rest.py",
         "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8",
         "is_verified": false,
-        "line_number": 110,
+        "line_number": 111,
         "is_secret": false
       },
       {
         "type": "Secret Keyword",
         "filename": "tests/app/user/test_rest.py",
         "hashed_secret": "0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33",
         "is_verified": false,
-        "line_number": 864,
+        "line_number": 875,
         "is_secret": false
       }
     ],
@@ -374,5 +374,5 @@
       }
     ]
   },
-  "generated_at": "2025-06-09T16:07:54Z"
+  "generated_at": "2025-07-01T16:40:56Z"
 }

app/aws/s3.py

@@ -13,6 +13,7 @@
 from app.clients import AWS_CLIENT_CONFIG
 
 # from app.service.rest import get_service_by_id
+from app.utils import hilite
 from notifications_utils import aware_utcnow
 
 FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv"
@@ -22,11 +23,6 @@
 ttl = 60 * 60 * 24 * 7
 
 
-# Global variable
-s3_client = None
-s3_resource = None
-
-
 def get_service_id_from_key(key):
     key = key.replace("service-", "")
     key = key.split("/")
@@ -69,32 +65,31 @@ def clean_cache():
 
 
 def get_s3_client():
-    global s3_client
-    if s3_client is None:
-        access_key = current_app.config["CSV_UPLOAD_BUCKET"]["access_key_id"]
-        secret_key = current_app.config["CSV_UPLOAD_BUCKET"]["secret_access_key"]
-        region = current_app.config["CSV_UPLOAD_BUCKET"]["region"]
-        session = Session(
-            aws_access_key_id=access_key,
-            aws_secret_access_key=secret_key,
-            region_name=region,
-        )
-        s3_client = session.client("s3", config=AWS_CLIENT_CONFIG)
+
+    access_key = current_app.config["CSV_UPLOAD_BUCKET"]["access_key_id"]
+    secret_key = current_app.config["CSV_UPLOAD_BUCKET"]["secret_access_key"]
+    region = current_app.config["CSV_UPLOAD_BUCKET"]["region"]
+    session = Session(
+        aws_access_key_id=access_key,
+        aws_secret_access_key=secret_key,
+        region_name=region,
+    )
+    current_app.logger.info(hilite("About to call session.client"))
+    s3_client = session.client("s3", config=AWS_CLIENT_CONFIG)
+    current_app.logger.info(hilite("SESSION CALLED"))
     return s3_client
 
 
 def get_s3_resource():
-    global s3_resource
-    if s3_resource is None:
-        access_key = current_app.config["CSV_UPLOAD_BUCKET"]["access_key_id"]
-        secret_key = current_app.config["CSV_UPLOAD_BUCKET"]["secret_access_key"]
-        region = current_app.config["CSV_UPLOAD_BUCKET"]["region"]
-        session = Session(
-            aws_access_key_id=access_key,
-            aws_secret_access_key=secret_key,
-            region_name=region,
-        )
-        s3_resource = session.resource("s3", config=AWS_CLIENT_CONFIG)
+    access_key = current_app.config["CSV_UPLOAD_BUCKET"]["access_key_id"]
+    secret_key = current_app.config["CSV_UPLOAD_BUCKET"]["secret_access_key"]
+    region = current_app.config["CSV_UPLOAD_BUCKET"]["region"]
+    session = Session(
+        aws_access_key_id=access_key,
+        aws_secret_access_key=secret_key,
+        region_name=region,
+    )
+    s3_resource = session.resource("s3", config=AWS_CLIENT_CONFIG)
     return s3_resource
 
 

tests/app/aws/test_s3.py

@@ -1,12 +1,15 @@
 import os
+import time
 from datetime import timedelta
 from os import getenv
-from unittest.mock import ANY, MagicMock, Mock, call, patch
+from unittest.mock import MagicMock, Mock, call
 
 import botocore
 import pytest
 from botocore.exceptions import ClientError
 
+from app import job_cache
+from app.aws import s3
 from app.aws.s3 import (
     cleanup_old_s3_objects,
     download_from_s3,
@@ -431,7 +434,6 @@ def test_get_s3_files_success(client, mocker):
     # mock_current_app.info.assert_any_call("job_cache length after regen: 0 #notify-debug-admin-1200")
 
 
-@patch("app.aws.s3.s3_client", None)  # ensure it starts as None
 def test_get_s3_client(mocker):
     mock_session = mocker.patch("app.aws.s3.Session")
     mock_current_app = mocker.patch("app.aws.s3.current_app")
@@ -447,12 +449,12 @@ def test_get_s3_client(mocker):
     mock_s3_client = MagicMock()
     mock_session.return_value.client.return_value = mock_s3_client
     result = get_s3_client()
-
-    mock_session.return_value.client.assert_called_once_with("s3", config=ANY)
-    assert result == mock_s3_client
+    assert result
+    mock_session.return_value.client.assert_called_once_with(
+        "s3", config=AWS_CLIENT_CONFIG
+    )
 
 
-@patch("app.aws.s3.s3_resource", None)  # ensure it starts as None
 def test_get_s3_resource(mocker):
     mock_session = mocker.patch("app.aws.s3.Session")
     mock_current_app = mocker.patch("app.aws.s3.current_app")
@@ -473,7 +475,7 @@ def test_get_s3_resource(mocker):
     mock_session.return_value.resource.assert_called_once_with(
         "s3", config=AWS_CLIENT_CONFIG
     )
-    assert result == mock_s3_resource
+    assert result
 
 
 def test_get_job_and_metadata_from_s3(mocker):
@@ -601,3 +603,72 @@ def test_get_s3_files_handles_exception(mocker):
     mock_current_app.logger.exception.assert_called_with(
         "Trouble reading file2.csv which is # 1 during cache regeneration"
     )
+
+
+def test_get_service_id_from_key_various_formats():
+    assert s3.get_service_id_from_key("service-123-notify/abc.csv") == "123"
+    assert s3.get_service_id_from_key("service-xyz-notify/def/ghi.csv") == "xyz"
+    assert s3.get_service_id_from_key("noservice-foo") == "nofoo"
+
+
+def test_set_and_get_job_cache_and_expiry(monkeypatch):
+    # isolate time for test
+    fake_time = time.time()
+    monkeypatch.setattr(time, "time", lambda: fake_time)
+
+    # set cache entry
+    s3.set_job_cache("k", "v")
+    tup = s3.get_job_cache("k")
+    assert tup is not None
+    value, expiry = tup
+    assert value == "v"
+    assert expiry == fake_time + (8 * 24 * 60 * 60)
+
+    # fast forward beyond expiry
+    monkeypatch.setattr(time, "time", lambda: fake_time + (9 * 24 * 60 * 60))
+
+    # clean_cache should remove expired entries
+    job_cache["other"] = ("foo", fake_time + 10)
+    s3.clean_cache()
+    assert "k" not in job_cache
+
+
+# def test_list_s3_objects_pagination_and_filtering(monkeypatch):
+#     fake_now = datetime.datetime(2025, 6, 29, tzinfo=timezone.utc)
+#     recent = fake_now - datetime.timedelta(days=1)
+#     old = fake_now - datetime.timedelta(days=10)
+#     client = MagicMock()
+#     client.list_objectes_v2.side_effect = [
+#         {
+#             "Contents": [{"Key": "new", "LastModified": recent}],
+#             "NextContinuationToken": "token",
+#         },
+#         {"Contents": [{"Key": "old", "LastModified": old}]},
+#     ]
+#     monkeypatch.setattr(s3, "get_s3_client", lambda: client)
+
+#     # set bucket name config
+#     s3.current_app = MagicMock()
+#     s3.current_app.config = {"CSV_UPLOAD_BUCKET": {"bucket": "b"}}
+#     s3.current_app.logger = MagicMock()
+#     keys = list(s3.list_s3_objects())
+#     assert "new" in keys and "old" not in keys
+
+
+def test_read_s3_file_populates_cache(monkeypatch):
+    fake_csv = "Phone number,Name\r\n+1-555-1234,Alice"
+    obj = MagicMock()
+    obj.get.return_value = {"Body": MagicMock(read=lambda: fake_csv.encode())}
+    s3res = MagicMock(Object=lambda b, o: obj)
+    monkeypatch.setattr(s3, "get_job_cache", lambda k: None)
+    monkeypatch.setattr(s3, "extract_phones", lambda job, sid, jid: {"0", "15551234"})
+    monkeypatch.setattr(
+        s3, "extract_personalisation", lambda job: {0: {"Name": "Alice"}}
+    )
+    monkeypatch.setattr(
+        s3, "set_job_cache", lambda k, v: job_cache.update({k: (v, time.time() + 1)})
+    )
+    s3.read_s3_file("bucket", "service-XX-notify/66.csv", s3res)
+    assert job_cache.get("66")[0].startswith("Phone number")
+    assert job_cache.get("66_phones")[0] == {"0", "15551234"}
+    assert job_cache.get("66_personalisation")[0] == {0: {"Name": "Alice"}}

tests/app/celery/test_nightly_tasks.py

@@ -1,5 +1,5 @@
 from datetime import date, datetime, timedelta
-from unittest.mock import ANY, call
+from unittest.mock import ANY, call, patch
 
 import pytest
 from freezegun import freeze_time
@@ -13,6 +13,7 @@
     cleanup_unfinished_jobs,
     delete_email_notifications_older_than_retention,
     delete_inbound_sms,
+    delete_notifications_for_service_and_type,
     delete_sms_notifications_older_than_retention,
     remove_sms_email_csv_files,
     s3,
@@ -431,3 +432,30 @@ def test_cleanup_unfinished_jobs(mocker):
     cleanup_unfinished_jobs()
     mock_s3.assert_called_once_with("blah")
     mock_dao_archive.assert_called_once_with(mock_job_unfinished)
+
+
+def test_delete_notifications_logs_when_deletion_occurs():
+    fake_start_time = datetime(2025, 1, 1, 12, 0, 0)
+    fake_end_time = fake_start_time + timedelta(seconds=10)
+    with patch(
+        "app.utils.utc_now", side_effect=[fake_start_time, fake_end_time]
+    ), patch(
+        "app.celery.nightly_tasks.move_notifications_to_notification_history",
+        return_value=5,
+    ) as mock_move, patch(
+        "app.celery.nightly_tasks.current_app.logger.info"
+    ) as mock_logger:
+
+        delete_notifications_for_service_and_type(
+            service_id="abc123",
+            notification_type="sms",
+            datetime_to_delete_before=datetime(2025, 1, 1, 0, 0, 0),
+        )
+        mock_move.assert_called_once_with(
+            "sms", "abc123", datetime(2025, 1, 1, 0, 0, 0)
+        )
+        mock_logger.assert_called_once()
+        log_message = mock_logger.call_args[0][0]
+        assert "service: abc123" in log_message
+        assert "notification_type: sms" in log_message
+        assert "count deleted: 5" in log_message

tests/app/celery/test_tasks.py

@@ -1,12 +1,13 @@
 import json
 import uuid
 from datetime import datetime, timedelta
-from unittest.mock import ANY, MagicMock, Mock, call
+from unittest.mock import ANY, MagicMock, Mock, call, patch
 
 import pytest
 import requests_mock
 from celery.exceptions import Retry
 from freezegun import freeze_time
+from psycopg2 import IntegrityError
 from requests import RequestException
 from sqlalchemy import func, select
 from sqlalchemy.exc import SQLAlchemyError
@@ -22,6 +23,7 @@
     process_row,
     s3,
     save_api_email,
+    save_api_email_or_sms,
     save_api_sms,
     save_email,
     save_sms,
@@ -1721,3 +1723,33 @@ def test_total_sending_limits_exceeded(mocker):
     assert mock_job.job_status == "sending limits exceeded"
     assert mock_job.processing_finished == datetime(2024, 11, 10, 12, 0, 0)
     mock_dao_update_job.assert_called_once_with(mock_job)
+
+
+def test_save_api_email_or_sms_integrity_error():
+    encrypted = MagicMock()
+    decrypted = {
+        "id": "notif-id",
+        "service_id": "service-id",
+        "notification_type": "email",
+        "template_id": "template-id",
+        "template_version": 1,
+        "to": "[email protected]",
+        "client_reference": None,
+        "created_at": "2025-01-01T00:00:00",
+        "reply_to_text": None,
+        "status": "created",
+        "document_download_count": 0,
+    }
+
+    with patch("app.celery.tasks.encryption.decrypt", return_value=decrypted), patch(
+        "app.celery.tasks.SerialisedService.from_id"
+    ), patch("app.celery.tasks.get_notification", return_value=None), patch(
+        "app.celery.tasks.persist_notification",
+        side_effect=IntegrityError("msg", None, None),
+    ), patch(
+        "app.celery.tasks.current_app.logger.warning"
+    ) as mock_log:
+
+        save_api_email_or_sms(encrypted)
+        mock_log.assert_called_once()
+        assert "already exists" in mock_log.call_args[0][0]

tests/app/dao/test_jobs_dao.py

@@ -562,6 +562,7 @@ def test_unique_key_on_job_id_and_job_row_number_no_error_if_row_number_for_diff
 
 def test_jobs_with_same_activity_time_are_sorted_by_id(sample_template):
     from datetime import datetime
+
     dt = datetime(2023, 1, 1, 12, 0, 0)
 
     job1 = create_job(sample_template, created_at=dt, processing_started=None)