From e791aa3aaebb4c2d389e8e89f099df7aff9f077a Mon Sep 17 00:00:00 2001
From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com>
Date: Tue, 18 Jun 2024 01:25:03 +0000
Subject: [PATCH 01/32] Bump urllib3 from 1.26.18 to 1.26.19

Bumps [urllib3](https://github.com/urllib3/urllib3) from 1.26.18 to 1.26.19.
- [Release notes](https://github.com/urllib3/urllib3/releases)
- [Changelog](https://github.com/urllib3/urllib3/blob/1.26.19/CHANGES.rst)
- [Commits](https://github.com/urllib3/urllib3/compare/1.26.18...1.26.19)

---
updated-dependencies:
- dependency-name: urllib3
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
---
 requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/requirements.txt b/requirements.txt
index 2a0fd77be9..c6f98afee2 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -241,7 +241,7 @@ tzdata==2024.1
     # via celery
 uri-template==1.2.0
     # via jsonschema
-urllib3==1.26.18
+urllib3==1.26.19
     # via
     #   botocore
     #   celery

From 41ebc35e016c9face8fcb6b43e3d9d858bf6eda8 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 23 Jul 2024 16:19:47 +0100
Subject: [PATCH 02/32] Add a note about how to bump utils version
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

You wouldn’t know about this without looking in the `Makefile`, and it’s
easier to use the command than do it manually.
---
 requirements.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/requirements.in b/requirements.in
index b02d55429f..b71a132dfe 100644
--- a/requirements.in
+++ b/requirements.in
@@ -23,6 +23,7 @@ lxml==4.9.3
 
 notifications-python-client==8.0.1
 
+# Run `make bump-utils` to update to the latest version
 notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.0.0
 
 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains

From 276824301c16f3947516c2d531f7fbd10cc263bc Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Tue, 23 Jul 2024 19:09:15 +0100
Subject: [PATCH 03/32] downgrade callback log message

We changed this from warning to error in june[^1], however, in practice this just massively obscures other errors in our logs to the order of tens of thousands, and in practice we aren't doing anything about these errors. We're not routinely contacting services or otherwise tracking these issues to see if they're growing/shrinking/etc.

Our solution should perhaps involve some way to alert services automatically to their failures. Either proactively (eg via email) or reactively (an alert on their dashboard).

[^1] https://github.com/alphagov/notifications-api/commit/3600d123e5e2c061d357312b34a93a892d1aa7ef
---
 app/celery/service_callback_tasks.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py
index de4165411e..c7da3b7907 100644
--- a/app/celery/service_callback_tasks.py
+++ b/app/celery/service_callback_tasks.py
@@ -109,7 +109,7 @@ def _send_data_to_service_callback_api(self, data, service_callback_url, token,
             try:
                 self.retry(queue=QueueNames.CALLBACKS_RETRY)
             except self.MaxRetriesExceededError as e:
-                current_app.logger.error(
+                current_app.logger.warning(
                     "Retry: %s has retried the max num of times for callback url %s and id: %s",
                     function_name,
                     service_callback_url,

From 03989606902ce270e3ccfc2f0e5a276bb2f47124 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Thu, 25 Jul 2024 10:37:54 +0100
Subject: [PATCH 04/32] Add process_unsubscribe_request_report endpoint  This
 endpoint "processes" an unsubscribe_request_report by updating it's
 processed_by_service_at date

---
 app/service/rest.py            | 13 +++++++++++++
 tests/app/service/test_rest.py | 23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/app/service/rest.py b/app/service/rest.py
index e5b60f4728..dd35cb79e0 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -1106,6 +1106,19 @@ def get_unsubscribe_requests_statistics(service_id):
     return jsonify(data), 200
 
 
+@service_blueprint.route("/<uuid:service_id>/process-unsubscribe-request-report/<uuid:batch_id>", methods=["POST"])
+def process_unsubscribe_request_report(service_id, batch_id):
+    """
+    This endpoint processes unsubscribe_request_reports by updating the processed_by_service_at
+    field
+    """
+    report = get_unsubscribe_request_reports_by_id_dao(batch_id)
+    report.processed_by_service_at = datetime.utcnow()
+    update_unsubscribe_request_report_dao(report)
+
+    return "", 204
+
+
 @service_blueprint.route("/<uuid:service_id>/contact-list", methods=["GET"])
 def get_contact_list(service_id):
     contact_lists = dao_get_contact_lists(service_id)
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index fc74b48dce..5f81335ff4 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3676,6 +3676,29 @@ def test_get_unsubscribe_requests_statistics_for_no_unsubscribe_requests(admin_r
     assert response == {}
 
 
+@freeze_time("2024-07-17")
+def test_process_unsubscribe_request_report(admin_request, sample_service):
+    unsubscribe_request_report = UnsubscribeRequestReport(
+        id=uuid.uuid4(),
+        count=242,
+        earliest_timestamp=datetime.utcnow() + timedelta(days=-5),
+        latest_timestamp=datetime.utcnow() + timedelta(days=-4),
+        processed_by_service_at=None,
+        service_id=sample_service.id,
+    )
+    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+    admin_request.post("service.process_unsubscribe_request_report",
+                       service_id=sample_service.id,
+                       batch_id=unsubscribe_request_report.id,
+                       _expected_status=204,
+                       _data=None,
+                       )
+    updated_unsubscribe_request_report = UnsubscribeRequestReport.query.filter_by(id=unsubscribe_request_report.id)\
+        .one()
+    assert updated_unsubscribe_request_report.id == unsubscribe_request_report.id
+    assert updated_unsubscribe_request_report.processed_by_service_at == datetime.utcnow()
+
+
 @pytest.mark.parametrize(
     "new_custom_email_sender_name, expected_email_sender_local_part",
     [

From 26164693ba1950bf155a90d1b331ad1c5c228f65 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Thu, 25 Jul 2024 11:04:57 +0100
Subject: [PATCH 05/32] Add get_unsubscribe_request_report_by_id_dao and
 update_unsubscribe_request_report_processed_by_date_dao

---
 app/dao/unsubscribe_request_dao.py | 12 ++++++++++++
 app/service/rest.py                | 13 ++++++++++---
 tests/app/service/test_rest.py     | 29 +++++++++++++++++++++--------
 3 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py
index e2147f82fe..f1f29f9636 100644
--- a/app/dao/unsubscribe_request_dao.py
+++ b/app/dao/unsubscribe_request_dao.py
@@ -1,3 +1,5 @@
+from datetime import datetime
+
 from sqlalchemy import desc, func
 
 from app import db
@@ -57,6 +59,10 @@ def get_unsubscribe_request_reports_dao(service_id):
     )
 
 
+def get_unsubscribe_request_report_by_id_dao(batch_id):
+    return UnsubscribeRequestReport.query.filter_by(id=batch_id).one_or_none()
+
+
 def get_unbatched_unsubscribe_requests_dao(service_id):
     return (
         UnsubscribeRequest.query.filter_by(service_id=service_id, unsubscribe_request_report_id=None)
@@ -68,3 +74,9 @@ def get_unbatched_unsubscribe_requests_dao(service_id):
 @autocommit
 def create_unsubscribe_request_reports_dao(unsubscribe_request_report):
     db.session.add(unsubscribe_request_report)
+
+
+@autocommit
+def update_unsubscribe_request_report_processed_by_date_dao(report):
+    report.processed_by_service_at = datetime.utcnow()
+    db.session.add(report)
diff --git a/app/service/rest.py b/app/service/rest.py
index dd35cb79e0..78f767feab 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -100,7 +100,9 @@
 from app.dao.templates_dao import dao_get_template_by_id
 from app.dao.unsubscribe_request_dao import (
     get_latest_unsubscribe_request_date_dao,
+    get_unsubscribe_request_report_by_id_dao,
     get_unsubscribe_requests_statistics_dao,
+    update_unsubscribe_request_report_processed_by_date_dao,
 )
 from app.dao.users_dao import get_user_by_id
 from app.errors import InvalidRequest, register_errors
@@ -1112,9 +1114,14 @@ def process_unsubscribe_request_report(service_id, batch_id):
     This endpoint processes unsubscribe_request_reports by updating the processed_by_service_at
     field
     """
-    report = get_unsubscribe_request_reports_by_id_dao(batch_id)
-    report.processed_by_service_at = datetime.utcnow()
-    update_unsubscribe_request_report_dao(report)
+    report = get_unsubscribe_request_report_by_id_dao(batch_id)
+    if report:
+        update_unsubscribe_request_report_processed_by_date_dao(report)
+    else:
+        raise InvalidRequest(
+            message={"batch_id": f"No UnsubscribeRequestReport found for id:{batch_id}"},
+            status_code=400,
+        )
 
     return "", 204
 
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 5f81335ff4..afdbbd847e 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3687,18 +3687,31 @@ def test_process_unsubscribe_request_report(admin_request, sample_service):
         service_id=sample_service.id,
     )
     create_unsubscribe_request_reports_dao(unsubscribe_request_report)
-    admin_request.post("service.process_unsubscribe_request_report",
-                       service_id=sample_service.id,
-                       batch_id=unsubscribe_request_report.id,
-                       _expected_status=204,
-                       _data=None,
-                       )
-    updated_unsubscribe_request_report = UnsubscribeRequestReport.query.filter_by(id=unsubscribe_request_report.id)\
-        .one()
+    admin_request.post(
+        "service.process_unsubscribe_request_report",
+        service_id=sample_service.id,
+        batch_id=unsubscribe_request_report.id,
+        _expected_status=204,
+        _data=None,
+    )
+    updated_unsubscribe_request_report = UnsubscribeRequestReport.query.filter_by(
+        id=unsubscribe_request_report.id
+    ).one()
     assert updated_unsubscribe_request_report.id == unsubscribe_request_report.id
     assert updated_unsubscribe_request_report.processed_by_service_at == datetime.utcnow()
 
 
+def test_process_unsubscribe_request_report_raises_error_for_invalid_batch_id(admin_request, sample_service):
+    random_batch_id = "258de158-07d3-457b-8eec-3e0e3bdab3bf"
+    admin_request.post(
+        "service.process_unsubscribe_request_report",
+        service_id=sample_service.id,
+        batch_id=random_batch_id,
+        _expected_status=400,
+        _data=None,
+    )
+
+
 @pytest.mark.parametrize(
     "new_custom_email_sender_name, expected_email_sender_local_part",
     [

From bc1aca67c27e20cdf641c0c4fa2003e17b14d1b8 Mon Sep 17 00:00:00 2001
From: Katie Smith <klssmith@users.noreply.github.com>
Date: Thu, 25 Jul 2024 11:54:42 +0100
Subject: [PATCH 06/32] Update callback params when sending letters to DVLA

This changes the JSON keys to match the new names that the DVLA will
use. It also changes the value of the retry window to be the default
value for their API.
---
 app/clients/letter/dvla.py     | 5 ++++-
 tests/app/clients/test_dvla.py | 8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/app/clients/letter/dvla.py b/app/clients/letter/dvla.py
index b57e6beed1..be83b00b66 100644
--- a/app/clients/letter/dvla.py
+++ b/app/clients/letter/dvla.py
@@ -332,7 +332,10 @@ def _format_create_print_job_json(
         }
 
         if callback_url:
-            json_payload["callback"] = {"target": callback_url, "retry": {"enabled": True, "maxRetryWindow": 3600}}
+            json_payload["callbackParams"] = {
+                "target": callback_url,
+                "retryParams": {"enabled": True, "maxRetryWindow": 10800},
+            }
 
         # `despatchMethod` should not be added for second class letters
         if postage == FIRST_CLASS:
diff --git a/tests/app/clients/test_dvla.py b/tests/app/clients/test_dvla.py
index f8e0d58c17..f156a64453 100644
--- a/tests/app/clients/test_dvla.py
+++ b/tests/app/clients/test_dvla.py
@@ -455,9 +455,9 @@ def test_format_create_print_job_json_adds_callback_key_if_url_provided(dvla_cli
         callback_url="https://www.example.com?token=1234",
     )
 
-    assert formatted_json["callback"] == {
+    assert formatted_json["callbackParams"] == {
         "target": "https://www.example.com?token=1234",
-        "retry": {"enabled": True, "maxRetryWindow": 3600},
+        "retryParams": {"enabled": True, "maxRetryWindow": 10800},
     }
 
 
@@ -571,9 +571,9 @@ def test_send_domestic_letter(dvla_client, dvla_authenticate, rmock):
             {"key": "organisationIdentifier", "value": "org_id"},
             {"key": "serviceIdentifier", "value": "service_id"},
         ],
-        "callback": {
+        "callbackParams": {
             "target": "https://www.example.com?token=1234",
-            "retry": {"enabled": True, "maxRetryWindow": 3600},
+            "retryParams": {"enabled": True, "maxRetryWindow": 10800},
         },
     }
 

From ec94af7246ffc81db181094afbc603d5e0f2c002 Mon Sep 17 00:00:00 2001
From: Katie Smith <klssmith@users.noreply.github.com>
Date: Thu, 25 Jul 2024 15:23:11 +0100
Subject: [PATCH 07/32] Fix DVLA callback endpoint

This is only temporary code, since we have a separate story to handle
callbacks properly. However, this change stops the endpoint from giving
an error while testing it.
---
 app/notifications/notifications_letter_callback.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py
index 26a30f8761..0fd613839e 100644
--- a/app/notifications/notifications_letter_callback.py
+++ b/app/notifications/notifications_letter_callback.py
@@ -71,5 +71,7 @@ def process_letter_callback():
         notification_id = signing.decode(token)
     except BadSignature:
         current_app.logger.info("Letter callback with invalid token of %s received", token)
+    else:
+        current_app.logger.info("Letter callback for notification id %s received", notification_id)
 
-    current_app.logger.info("Letter callback for notification id %s received", notification_id)
+    return jsonify(result="success"), 200

From b1d677bbfad135e2c7f78bccfa2b7e8dcae621cd Mon Sep 17 00:00:00 2001
From: Katie Smith <klssmith@users.noreply.github.com>
Date: Thu, 25 Jul 2024 15:24:48 +0100
Subject: [PATCH 08/32] Fix flaky tests by freezing time

---
 tests/app/service/test_rest.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index fc74b48dce..0af691521a 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3489,6 +3489,7 @@ def test_get_returned_letter(admin_request, sample_letter_template):
     assert response[4]["uploaded_letter_file_name"] == "filename.pdf"
 
 
+@freeze_time("2024-07-01 12:00")
 def test_get_unsubscribe_request_report_summary_for_initial_unsubscribe_requests(admin_request, sample_service, mocker):
     """
     Test case covers when the initial unsubscribe requests have been received and have not yet been batched.
@@ -3544,6 +3545,7 @@ def test_get_unsubscribe_request_report_summary_for_initial_unsubscribe_requests
     assert response == expected_reports_summary
 
 
+@freeze_time("2024-07-01 12:00")
 def test_get_unsubscribe_request_reports_summary(admin_request, sample_service, mocker):
     date_format = "%a, %d %b %Y %H:%M:%S %Z"
     # Create 2 unbatched unsubscribe requests

From c0c270b6c650a8402c3121a908470fd2957d0ee1 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 26 Jul 2024 16:01:27 +0100
Subject: [PATCH 09/32] =?UTF-8?q?Fix=20reference=20to=20=E2=80=98unprocess?=
 =?UTF-8?q?ed=E2=80=99=20unsubscribe=20requests?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We changed the query and renamed this column, but missed changing the
reference here.
---
 app/service/rest.py            | 2 +-
 tests/app/service/test_rest.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/service/rest.py b/app/service/rest.py
index e5b60f4728..91c98c3377 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -1094,7 +1094,7 @@ def get_unsubscribe_requests_statistics(service_id):
     data = {}
     if unsubscribe_statistics := get_unsubscribe_requests_statistics_dao(service_id):
         data = {
-            "unsubscribe_requests_count": unsubscribe_statistics.unprocessed_unsubscribe_requests_count,
+            "unsubscribe_requests_count": unsubscribe_statistics.unsubscribe_requests_count,
             "datetime_of_latest_unsubscribe_request": unsubscribe_statistics.datetime_of_latest_unsubscribe_request,
         }
     elif latest_unsubscribe_request := get_latest_unsubscribe_request_date_dao(service_id):
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 0af691521a..7093e6aa9b 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3645,13 +3645,13 @@ def test_get_unsubscribe_request_reports_summary(admin_request, sample_service,
 def test_get_unsubscribe_requests_statistics(admin_request, sample_service, mocker):
     MockUnsubscribeRequest = namedtuple(
         "MockUnsubscribeRequest",
-        ["unprocessed_unsubscribe_requests_count", "service_id", "datetime_of_latest_unsubscribe_request"],
+        ["unsubscribe_requests_count", "service_id", "datetime_of_latest_unsubscribe_request"],
     )
     test_data = MockUnsubscribeRequest(0, "2fed1b45-66e1-4682-a389-85d0d50a916f", "Thu, 18 Jul 2024 15:32:28 GMT")
 
     mocker.patch("app.service.rest.get_unsubscribe_requests_statistics_dao", return_value=test_data)
     response = admin_request.get("service.get_unsubscribe_requests_statistics", service_id=sample_service.id)
-    assert response["unsubscribe_requests_count"] == test_data.unprocessed_unsubscribe_requests_count
+    assert response["unsubscribe_requests_count"] == test_data.unsubscribe_requests_count
     assert response["datetime_of_latest_unsubscribe_request"] == test_data.datetime_of_latest_unsubscribe_request
 
 

From d04310ea46191cbdfeb835e3cfd2f0bc78b922fa Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Mon, 29 Jul 2024 14:30:27 +0100
Subject: [PATCH 10/32] Update
 update_unsubscribe_request_report_processed_by_date_dao 
 processed_by_service_at can now be set back to None. This is to facilitate a
 user  clearing the "Mark as completed" checkbox to change a report's
 processed status.

---
 app/dao/unsubscribe_request_dao.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py
index f1f29f9636..1f1149fd19 100644
--- a/app/dao/unsubscribe_request_dao.py
+++ b/app/dao/unsubscribe_request_dao.py
@@ -77,6 +77,6 @@ def create_unsubscribe_request_reports_dao(unsubscribe_request_report):
 
 
 @autocommit
-def update_unsubscribe_request_report_processed_by_date_dao(report):
-    report.processed_by_service_at = datetime.utcnow()
+def update_unsubscribe_request_report_processed_by_date_dao(report, report_has_been_processed):
+    report.processed_by_service_at = datetime.utcnow() if report_has_been_processed else None
     db.session.add(report)

From 6f18158c141e3ecec388d2b2752075a139adbd76 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Mon, 29 Jul 2024 14:32:27 +0100
Subject: [PATCH 11/32] Update process_unsubscribe_request_report to handle
 setting UnsubscribeRequestReport.processed_by_service_at back to None

---
 app/service/rest.py            | 12 +++++++++---
 tests/app/service/test_rest.py | 29 +++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/app/service/rest.py b/app/service/rest.py
index 78f767feab..867fd9ce25 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -1114,9 +1114,15 @@ def process_unsubscribe_request_report(service_id, batch_id):
     This endpoint processes unsubscribe_request_reports by updating the processed_by_service_at
     field
     """
-    report = get_unsubscribe_request_report_by_id_dao(batch_id)
-    if report:
-        update_unsubscribe_request_report_processed_by_date_dao(report)
+    if data := request.get_json():
+        report_has_been_processed = data["report_has_been_processed"]
+    else:
+        raise InvalidRequest(
+            message={"marked_as_completed": "missing data for required field"},
+            status_code=400,
+        )
+    if report := get_unsubscribe_request_report_by_id_dao(batch_id):
+        update_unsubscribe_request_report_processed_by_date_dao(report, report_has_been_processed)
     else:
         raise InvalidRequest(
             message={"batch_id": f"No UnsubscribeRequestReport found for id:{batch_id}"},
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index afdbbd847e..0f415e7f9c 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3692,7 +3692,7 @@ def test_process_unsubscribe_request_report(admin_request, sample_service):
         service_id=sample_service.id,
         batch_id=unsubscribe_request_report.id,
         _expected_status=204,
-        _data=None,
+        _data={"report_has_been_processed": True},
     )
     updated_unsubscribe_request_report = UnsubscribeRequestReport.query.filter_by(
         id=unsubscribe_request_report.id
@@ -3701,6 +3701,31 @@ def test_process_unsubscribe_request_report(admin_request, sample_service):
     assert updated_unsubscribe_request_report.processed_by_service_at == datetime.utcnow()
 
 
+@freeze_time("2024-07-17")
+def test_process_unsubscribe_request_report_set_processed_by_date_back_to_none(admin_request, sample_service):
+    unsubscribe_request_report = UnsubscribeRequestReport(
+        id=uuid.uuid4(),
+        count=242,
+        earliest_timestamp=datetime.utcnow() + timedelta(days=-5),
+        latest_timestamp=datetime.utcnow() + timedelta(days=-4),
+        processed_by_service_at=datetime.utcnow() + timedelta(days=-2),
+        service_id=sample_service.id,
+    )
+    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+    admin_request.post(
+        "service.process_unsubscribe_request_report",
+        service_id=sample_service.id,
+        batch_id=unsubscribe_request_report.id,
+        _expected_status=204,
+        _data={"report_has_been_processed": False},
+    )
+    updated_unsubscribe_request_report = UnsubscribeRequestReport.query.filter_by(
+        id=unsubscribe_request_report.id
+    ).one()
+    assert updated_unsubscribe_request_report.id == unsubscribe_request_report.id
+    assert updated_unsubscribe_request_report.processed_by_service_at is None
+
+
 def test_process_unsubscribe_request_report_raises_error_for_invalid_batch_id(admin_request, sample_service):
     random_batch_id = "258de158-07d3-457b-8eec-3e0e3bdab3bf"
     admin_request.post(
@@ -3708,7 +3733,7 @@ def test_process_unsubscribe_request_report_raises_error_for_invalid_batch_id(ad
         service_id=sample_service.id,
         batch_id=random_batch_id,
         _expected_status=400,
-        _data=None,
+        _data={"report_has_been_processed": False},
     )
 
 

From 08586b7b7e208b7dcc1eaa1b5d908c2a4bbb397c Mon Sep 17 00:00:00 2001
From: Ben Corlett <ben.corlett@digital.cabinet-office.gov.uk>
Date: Mon, 29 Jul 2024 15:25:51 +0100
Subject: [PATCH 12/32] Set postage correctly for functional test fixtures

---
 app/functional_tests_fixtures/__init__.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py
index cbcc882941..0d3ade86e3 100644
--- a/app/functional_tests_fixtures/__init__.py
+++ b/app/functional_tests_fixtures/__init__.py
@@ -10,7 +10,6 @@
     EMAIL_AUTH,
     EXTRA_LETTER_FORMATTING,
     INBOUND_SMS_TYPE,
-    LETTER_TYPE,
     SECOND_CLASS,
     SEND_EMAILS,
     SEND_LETTERS,
@@ -356,9 +355,6 @@ def _create_email_template(service, user_id):
 
     new_template = template_schema.load(data)
 
-    if not new_template.postage and new_template.template_type == LETTER_TYPE:
-        new_template.postage = SECOND_CLASS
-
     new_template.service = service
 
     dao_create_template(new_template)
@@ -412,6 +408,7 @@ def _create_letter_template(service, user_id):
     new_template = template_schema.load(data)
 
     new_template.service = service
+    new_template.postage = SECOND_CLASS
 
     dao_create_template(new_template)
 

From f1bb9ec5863eb8f5bba6e6539e233d0cc899606c Mon Sep 17 00:00:00 2001
From: Ben Corlett <ben.corlett@digital.cabinet-office.gov.uk>
Date: Mon, 29 Jul 2024 15:26:47 +0100
Subject: [PATCH 13/32] Add contact_link to functional test fixtures service.
 Required for document download tests

---
 app/functional_tests_fixtures/__init__.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py
index 0d3ade86e3..7539d0cdd5 100644
--- a/app/functional_tests_fixtures/__init__.py
+++ b/app/functional_tests_fixtures/__init__.py
@@ -279,6 +279,7 @@ def _create_service(org_id, user, service_name="Functional Tests"):
             "sms_message_limit": 1000,
             "letter_message_limit": 1000,
             "email_message_limit": 1000,
+            "contact_link": current_app.config["ADMIN_BASE_URL"],
         }
         service = Service.from_json(data)
         dao_create_service(service, user)

From 34a59cb29263dac18075c2ca00dae1f1fdfba436 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 30 Jul 2024 16:37:45 +0100
Subject: [PATCH 14/32] Purge additional Redis cache when user unsubscribes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When a user unsubscribes we delete the cahce of statistics which
populates the dashboard.

We also cache the summary of unsubscribe reports that the person running
the service has batched up.

When we get a new request to unsubscribe it goes into the ‘unbatched’
report, so we need to delete the cache of reports for it to appear.
---
 app/one_click_unsubscribe/rest.py                           | 1 +
 .../app/one_click_unsubscribe/test_one_click_unsubscribe.py | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/app/one_click_unsubscribe/rest.py b/app/one_click_unsubscribe/rest.py
index f9d8f43347..dfc1aa0afc 100644
--- a/app/one_click_unsubscribe/rest.py
+++ b/app/one_click_unsubscribe/rest.py
@@ -46,6 +46,7 @@ def one_click_unsubscribe(notification_id, token):
 
     create_unsubscribe_request_dao(unsubscribe_data)
     redis_store.delete(f"service-{unsubscribe_data['service_id']}-unsubscribe-request-statistics")
+    redis_store.delete(f"service-{unsubscribe_data['service_id']}-unsubscribe-request-reports-summary")
 
     current_app.logger.debug("Received unsubscribe request for notification_id: %s", notification_id)
 
diff --git a/tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py b/tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py
index 3ffb7286d2..0d765dd9c8 100644
--- a/tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py
+++ b/tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py
@@ -1,4 +1,5 @@
 import uuid
+from unittest.mock import call
 
 from flask import current_app
 from notifications_utils.url_safe_token import generate_token
@@ -29,7 +30,10 @@ def test_valid_one_click_unsubscribe_url(mocker, client, sample_email_notificati
     assert created_unsubscribe_request.template_version == sample_email_notification.template_version
     assert created_unsubscribe_request.service_id == sample_email_notification.service_id
     assert created_unsubscribe_request.email_address == sample_email_notification.to
-    mock_redis.assert_called_once_with(f"service-{sample_email_notification.service.id}-unsubscribe-request-statistics")
+    assert mock_redis.call_args_list == [
+        call(f"service-{sample_email_notification.service.id}-unsubscribe-request-statistics"),
+        call(f"service-{sample_email_notification.service.id}-unsubscribe-request-reports-summary"),
+    ]
 
 
 def test_unsubscribe_request_object_refers_to_correct_template_version_after_template_updated(client, sample_service):

From 47a12738c3458d690a7662bf0834a3d74af542c6 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Thu, 25 Jul 2024 16:44:26 +0100
Subject: [PATCH 15/32] Add create_unsubscribe_request_report endpoint  This
 endpoint converts an unbatched unsubscribe request report to a batched
 unsubscribe request report.  It also batches all unbatched unsubscribe
 requests received within the reports earliest and latest time_stamps to it.

---
 app/service/rest.py            | 31 ++++++++++++++++++++++++++++++-
 tests/app/service/test_rest.py | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/app/service/rest.py b/app/service/rest.py
index d3540978bd..c727bc3547 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -1,4 +1,5 @@
 import itertools
+import uuid
 from datetime import datetime
 
 from flask import Blueprint, current_app, jsonify, request
@@ -103,6 +104,7 @@
     get_unsubscribe_request_report_by_id_dao,
     get_unsubscribe_requests_statistics_dao,
     update_unsubscribe_request_report_processed_by_date_dao,
+    get_unsubscribe_requests_statistics_dao, create_unsubscribe_request_reports_dao,
 )
 from app.dao.users_dao import get_user_by_id
 from app.errors import InvalidRequest, register_errors
@@ -112,7 +114,7 @@
     LetterBranding,
     Permission,
     Service,
-    ServiceContactList,
+    ServiceContactList, UnsubscribeRequestReport,
 )
 from app.notifications.process_notifications import (
     persist_notification,
@@ -1132,6 +1134,33 @@ def process_unsubscribe_request_report(service_id, batch_id):
     return "", 204
 
 
+@service_blueprint.route("/<uuid:service_id>/create-unsubscribe-request-report", methods=["POST"])
+def create_unsubscribe_request_report(service_id):
+    summary_data = request.get_json()
+    unsubscribe_request_report = UnsubscribeRequestReport(
+        id=uuid.uuid4(),
+        count=summary_data["count"],
+        earliest_timestamp=summary_data["earliest_timestamp"],
+        latest_timestamp=summary_data["latest_timestamp"],
+        processed_by_service_at=summary_data["processed_by_service_at"],
+        service_id=service_id,
+    )
+    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+    assign_unbatched_unsubscribe_requests_to_report_dao(
+        report_id=unsubscribe_request_report.id,
+        service_id=unsubscribe_request_report.service_id,
+        latest_timestamp=unsubscribe_request_report.latest_timestamp,
+    )
+    return (
+        jsonify(
+            {
+                "report_id": unsubscribe_request_report.id,
+            }
+        ),
+        201,
+    )
+
+
 @service_blueprint.route("/<uuid:service_id>/contact-list", methods=["GET"])
 def get_contact_list(service_id):
     contact_lists = dao_get_contact_lists(service_id)
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 7a02b6d36f..0b18f99e6d 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3739,6 +3739,40 @@ def test_process_unsubscribe_request_report_raises_error_for_invalid_batch_id(ad
     )
 
 
+def test_create_unsubscribe_request_report(sample_service, admin_request, mocker):
+    date_format = "%Y-%m-%d %H:%M:%S"
+    test_id = "2802262c-b6ac-4254-93c3-a83ae7180d96"
+    summary_data = {
+        "batch_id": None,
+        "count": 2,
+        "earliest_timestamp": "2024-07-03 13:30:00",
+        "latest_timestamp": "2024-07-09 21:13:11",
+        "processed_by_service_at": None,
+        "is_a_batched_report": False,
+    }
+    mocker.patch("app.service.rest.uuid.uuid4", return_value=test_id)
+    mock_assign_unbatched_requests = mocker.patch(
+        "app.service.rest.assign_unbatched_unsubscribe_requests_to_report_dao"
+    )
+    response = admin_request.post(
+        "service.create_unsubscribe_request_report", service_id=sample_service.id, _data=summary_data,
+        _expected_status=201
+    )
+    created_unsubscribe_request_report = UnsubscribeRequestReport.query.filter_by(id=test_id).one()
+    assert response == {"report_id": str(created_unsubscribe_request_report.id)}
+    assert summary_data["count"] == created_unsubscribe_request_report.count
+    assert summary_data["earliest_timestamp"] == created_unsubscribe_request_report.earliest_timestamp.strftime(
+        date_format
+    )
+    assert summary_data["latest_timestamp"] == created_unsubscribe_request_report.latest_timestamp.strftime(date_format)
+    assert summary_data["processed_by_service_at"] == created_unsubscribe_request_report.processed_by_service_at
+    mock_assign_unbatched_requests.assert_called_once_with(
+        report_id=created_unsubscribe_request_report.id,
+        service_id=created_unsubscribe_request_report.service_id,
+        latest_timestamp=created_unsubscribe_request_report.latest_timestamp,
+    )
+
+
 @pytest.mark.parametrize(
     "new_custom_email_sender_name, expected_email_sender_local_part",
     [

From 6e0fcd0945191215b8b6e9790457eab3afaa3ddf Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Thu, 25 Jul 2024 17:26:21 +0100
Subject: [PATCH 16/32] Add assign_unbatched_unsubscribe_requests_to_report_dao

---
 app/dao/unsubscribe_request_dao.py            | 20 +++++++
 app/service/rest.py                           | 52 ++++++++++--------
 tests/app/dao/test_unsubscribe_request_dao.py | 54 ++++++++++++++++++-
 tests/app/service/test_rest.py                |  8 +++
 4 files changed, 111 insertions(+), 23 deletions(-)

diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py
index 1f1149fd19..9c345329d6 100644
--- a/app/dao/unsubscribe_request_dao.py
+++ b/app/dao/unsubscribe_request_dao.py
@@ -1,6 +1,7 @@
 from datetime import datetime
 
 from sqlalchemy import desc, func
+from sqlalchemy.sql.operators import is_
 
 from app import db
 from app.dao.dao_utils import autocommit
@@ -80,3 +81,22 @@ def create_unsubscribe_request_reports_dao(unsubscribe_request_report):
 def update_unsubscribe_request_report_processed_by_date_dao(report, report_has_been_processed):
     report.processed_by_service_at = datetime.utcnow() if report_has_been_processed else None
     db.session.add(report)
+
+
+@autocommit
+def assign_unbatched_unsubscribe_requests_to_report_dao(report_id, service_id, earliest_timestamp,
+                                                        latest_timestamp):
+    """
+    This method retrieves all the un-batched unsubscribe requests received before or on
+    the report's latest_timestamp and sets their unsubscribe_request_report_id to the report_i
+    """
+    unbatched_unsubscribe_requests = UnsubscribeRequest.query.filter(
+        UnsubscribeRequest.unsubscribe_request_report_id.is_(None),
+        UnsubscribeRequest.service_id == service_id,
+        UnsubscribeRequest.created_at >= earliest_timestamp,
+        UnsubscribeRequest.created_at <= latest_timestamp,
+    ).all()
+    for unsubscribe_request in unbatched_unsubscribe_requests:
+        unsubscribe_request.unsubscribe_request_report_id = report_id
+        db.session.add(unsubscribe_request)
+
diff --git a/app/service/rest.py b/app/service/rest.py
index c727bc3547..ee06674764 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -105,6 +105,7 @@
     get_unsubscribe_requests_statistics_dao,
     update_unsubscribe_request_report_processed_by_date_dao,
     get_unsubscribe_requests_statistics_dao, create_unsubscribe_request_reports_dao,
+    assign_unbatched_unsubscribe_requests_to_report_dao,
 )
 from app.dao.users_dao import get_user_by_id
 from app.errors import InvalidRequest, register_errors
@@ -1137,28 +1138,35 @@ def process_unsubscribe_request_report(service_id, batch_id):
 @service_blueprint.route("/<uuid:service_id>/create-unsubscribe-request-report", methods=["POST"])
 def create_unsubscribe_request_report(service_id):
     summary_data = request.get_json()
-    unsubscribe_request_report = UnsubscribeRequestReport(
-        id=uuid.uuid4(),
-        count=summary_data["count"],
-        earliest_timestamp=summary_data["earliest_timestamp"],
-        latest_timestamp=summary_data["latest_timestamp"],
-        processed_by_service_at=summary_data["processed_by_service_at"],
-        service_id=service_id,
-    )
-    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
-    assign_unbatched_unsubscribe_requests_to_report_dao(
-        report_id=unsubscribe_request_report.id,
-        service_id=unsubscribe_request_report.service_id,
-        latest_timestamp=unsubscribe_request_report.latest_timestamp,
-    )
-    return (
-        jsonify(
-            {
-                "report_id": unsubscribe_request_report.id,
-            }
-        ),
-        201,
-    )
+    if summary_data:
+        unsubscribe_request_report = UnsubscribeRequestReport(
+            id=uuid.uuid4(),
+            count=summary_data["count"],
+            earliest_timestamp=summary_data["earliest_timestamp"],
+            latest_timestamp=summary_data["latest_timestamp"],
+            processed_by_service_at=summary_data["processed_by_service_at"],
+            service_id=service_id,
+        )
+        create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+        assign_unbatched_unsubscribe_requests_to_report_dao(
+            report_id=unsubscribe_request_report.id,
+            service_id=unsubscribe_request_report.service_id,
+            earliest_timestamp=unsubscribe_request_report.earliest_timestamp,
+            latest_timestamp=unsubscribe_request_report.latest_timestamp,
+        )
+        return (
+            jsonify(
+                {
+                    "report_id": unsubscribe_request_report.id,
+                }
+            ),
+            201,
+        )
+    else:
+        raise InvalidRequest(
+            message=f"summary data needed to create an unsubscribe request report is missing",
+            status_code=400,
+        )
 
 
 @service_blueprint.route("/<uuid:service_id>/contact-list", methods=["GET"])
diff --git a/tests/app/dao/test_unsubscribe_request_dao.py b/tests/app/dao/test_unsubscribe_request_dao.py
index fb2c109939..8031fa8f95 100644
--- a/tests/app/dao/test_unsubscribe_request_dao.py
+++ b/tests/app/dao/test_unsubscribe_request_dao.py
@@ -4,7 +4,7 @@
     create_unsubscribe_request_reports_dao,
     get_latest_unsubscribe_request_date_dao,
     get_unsubscribe_request_by_notification_id_dao,
-    get_unsubscribe_requests_statistics_dao,
+    get_unsubscribe_requests_statistics_dao, assign_unbatched_unsubscribe_requests_to_report_dao,
 )
 from app.models import UnsubscribeRequest, UnsubscribeRequestReport
 from app.one_click_unsubscribe.rest import get_unsubscribe_request_data
@@ -234,3 +234,55 @@ def test_get_latest_unsubscribe_request_dao(sample_service):
 def test_get_latest_unsubscribe_request_dao_if_no_unsubscribe_request_exists(sample_service):
     result = get_latest_unsubscribe_request_date_dao(sample_service.id)
     assert result is None
+
+
+def test_assign_unbatched_unsubscribe_requests_to_report_dao(sample_service):
+    template = create_template(
+        sample_service,
+        template_type=EMAIL_TYPE,
+    )
+    notification_1 = create_notification(template=template)
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_1.id,
+            "template_id": notification_1.template_id,
+            "template_version": notification_1.template_version,
+            "service_id": notification_1.service_id,
+            "email_address": notification_1.to,
+            "created_at": midnight_n_days_ago(0),
+        }
+    )
+    notification_2 = create_notification(template=template)
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_2.id,
+            "template_id": notification_2.template_id,
+            "template_version": notification_2.template_version,
+            "service_id": notification_2.service_id,
+            "email_address": notification_2.to,
+            "created_at": midnight_n_days_ago(2),
+        }
+    )
+
+    unsubscribe_request_report = UnsubscribeRequestReport(
+        id="7536fd15-3d9c-494b-9053-0fd9822bcae6",
+        count=141,
+        earliest_timestamp=midnight_n_days_ago(4),
+        latest_timestamp=midnight_n_days_ago(0),
+        service_id=sample_service.id,
+    )
+    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+
+    assign_unbatched_unsubscribe_requests_to_report_dao(
+        report_id=unsubscribe_request_report.id,
+        service_id=unsubscribe_request_report.service_id,
+        earliest_timestamp=unsubscribe_request_report.earliest_timestamp,
+        latest_timestamp=unsubscribe_request_report.latest_timestamp,
+    )
+    unsubscribe_requests = UnsubscribeRequest.query.filter_by(
+        unsubscribe_request_report_id=unsubscribe_request_report.id
+    ).all()
+
+    for unsubscribe_request in unsubscribe_requests:
+        assert unsubscribe_request.unsubscribe_request_report_id == unsubscribe_request_report.id
+    assert len(unsubscribe_requests) == 2
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 0b18f99e6d..2a9bf2bfd3 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3769,10 +3769,18 @@ def test_create_unsubscribe_request_report(sample_service, admin_request, mocker
     mock_assign_unbatched_requests.assert_called_once_with(
         report_id=created_unsubscribe_request_report.id,
         service_id=created_unsubscribe_request_report.service_id,
+        earliest_timestamp=created_unsubscribe_request_report.earliest_timestamp,
         latest_timestamp=created_unsubscribe_request_report.latest_timestamp,
     )
 
 
+def test_create_unsubscribe_request_report_raises_error_for_no_summary_data(sample_service, admin_request, mocker):
+    response = admin_request.post(
+        "service.create_unsubscribe_request_report", service_id=sample_service.id, _data=None,
+        _expected_status=400
+    )
+
+
 @pytest.mark.parametrize(
     "new_custom_email_sender_name, expected_email_sender_local_part",
     [

From c64d6df999e6e644544a8065c0a66f0b29e424a9 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Thu, 25 Jul 2024 18:00:24 +0100
Subject: [PATCH 17/32] Add get_unsubscribe_request_report_for_download

---
 app/service/rest.py            |  25 +++++++++
 tests/app/service/test_rest.py | 100 ++++++++++++++++++++++++++++++++-
 2 files changed, 123 insertions(+), 2 deletions(-)

diff --git a/app/service/rest.py b/app/service/rest.py
index ee06674764..52e70b37ac 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -1169,6 +1169,31 @@ def create_unsubscribe_request_report(service_id):
         )
 
 
+@service_blueprint.route("/<uuid:service_id>/unsubscribe-request-report/<uuid:batch_id>", methods=["GET"])
+def get_unsubscribe_request_report_for_download(service_id, batch_id):
+    report = get_unsubscribe_request_reports_by_id_dao(batch_id)
+    data = {
+        "batch_id": report.id,
+        "earliest_timestamp": report.earliest_timestamp,
+        "latest_timestamp": report.latest_timestamp,
+        "unsubscribe_requests": sorted(
+            [
+                {
+                    "email_address": unsubscribe_request.email_address,
+                    "template_name": unsubscribe_request.template.name,
+                    "sent_at": unsubscribe_request.notification.sent_at,
+                }
+                for unsubscribe_request in report.unsubscribe_requests
+            ],
+            key=lambda row: row["sent_at"],
+            reverse=True,
+        ),
+    }
+    return jsonify(data), 200
+
+
+
+
 @service_blueprint.route("/<uuid:service_id>/contact-list", methods=["GET"])
 def get_contact_list(service_id):
     contact_lists = dao_get_contact_lists(service_id)
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 2a9bf2bfd3..7aa8102821 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -31,7 +31,8 @@
     dao_remove_user_from_service,
 )
 from app.dao.templates_dao import dao_redact_template
-from app.dao.unsubscribe_request_dao import create_unsubscribe_request_dao, create_unsubscribe_request_reports_dao
+from app.dao.unsubscribe_request_dao import create_unsubscribe_request_dao, create_unsubscribe_request_reports_dao, \
+    assign_unbatched_unsubscribe_requests_to_report_dao
 from app.dao.users_dao import save_model_user
 from app.models import (
     AnnualBilling,
@@ -44,8 +45,9 @@
     ServicePermission,
     ServiceSmsSender,
     UnsubscribeRequestReport,
-    User,
+    User, UnsubscribeRequest,
 )
+from app.utils import midnight_n_days_ago
 from tests import create_admin_authorization_header
 from tests.app.db import (
     create_annual_billing,
@@ -3781,6 +3783,100 @@ def test_create_unsubscribe_request_report_raises_error_for_no_summary_data(samp
     )
 
 
+def test_get_unsubscribe_request_report_for_download(admin_request, sample_service, mocker):
+    # Create 3 un-batched unsubscribe requests
+    template_1 = create_template(sample_service, template_type=EMAIL_TYPE, template_name="First Template")
+    notification_1 = create_notification(template=template_1, sent_at=midnight_n_days_ago(1))
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_1.id,
+            "template_id": notification_1.template_id,
+            "template_version": notification_1.template_version,
+            "service_id": notification_1.service_id,
+            "email_address": notification_1.to,
+            "created_at": midnight_n_days_ago(0),
+        }
+    )
+
+    notification_2 = create_notification(template=template_1, sent_at=midnight_n_days_ago(3))
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_2.id,
+            "template_id": notification_2.template_id,
+            "template_version": notification_2.template_version,
+            "service_id": notification_2.service_id,
+            "email_address": notification_2.to,
+            "created_at": midnight_n_days_ago(2),
+        }
+    )
+
+    template_2 = create_template(sample_service, template_type=EMAIL_TYPE, template_name="Second Template")
+    notification_3 = create_notification(template=template_2, sent_at=midnight_n_days_ago(5))
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_3.id,
+            "template_id": notification_3.template_id,
+            "template_version": notification_3.template_version,
+            "service_id": notification_3.service_id,
+            "email_address": notification_3.to,
+            "created_at": midnight_n_days_ago(4),
+        }
+    )
+
+    # Create unsubscribe_request_report
+    unsubscribe_request_report = UnsubscribeRequestReport(
+        id="7536fd15-3d9c-494b-9053-0fd9822bcae6",
+        count=141,
+        earliest_timestamp=midnight_n_days_ago(4),
+        latest_timestamp=midnight_n_days_ago(0),
+        service_id=sample_service.id,
+    )
+
+    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+
+    # batch the unsubscribe_requests
+    assign_unbatched_unsubscribe_requests_to_report_dao(
+        report_id=unsubscribe_request_report.id,
+        service_id=unsubscribe_request_report.service_id,
+        latest_timestamp=unsubscribe_request_report.latest_timestamp,
+    )
+
+    response = admin_request.get(
+        "service.get_unsubscribe_request_report_for_download",
+        service_id=sample_service.id,
+        batch_id=unsubscribe_request_report.id,
+    )
+    unsubscribe_requests = sorted(
+        UnsubscribeRequest.query.filter_by(service_id=sample_service.id).all(),
+        key=lambda row: row.notification.sent_at,
+        reverse=True,
+    )
+    date_format = "%a, %d %b %Y %H:%M:%S"
+    assert response["batch_id"] == str(unsubscribe_request_report.id)
+    assert (
+        response["earliest_timestamp"] == unsubscribe_request_report.earliest_timestamp.strftime(date_format) + " GMT"
+    )
+    assert response["latest_timestamp"] == unsubscribe_request_report.latest_timestamp.strftime(date_format) + " GMT"
+    assert response["unsubscribe_requests"][0]["email_address"] == unsubscribe_requests[0].email_address
+    assert response["unsubscribe_requests"][0]["template_name"] == unsubscribe_requests[0].template.name
+    assert (
+        response["unsubscribe_requests"][0]["sent_at"]
+        == unsubscribe_requests[0].notification.sent_at.strftime(date_format) + " GMT"
+    )
+    assert response["unsubscribe_requests"][1]["email_address"] == unsubscribe_requests[1].email_address
+    assert response["unsubscribe_requests"][1]["template_name"] == unsubscribe_requests[1].template.name
+    assert (
+        response["unsubscribe_requests"][1]["sent_at"]
+        == unsubscribe_requests[1].notification.sent_at.strftime(date_format) + " GMT"
+    )
+    assert response["unsubscribe_requests"][2]["email_address"] == unsubscribe_requests[2].email_address
+    assert response["unsubscribe_requests"][2]["template_name"] == unsubscribe_requests[2].template.name
+    assert (
+        response["unsubscribe_requests"][2]["sent_at"]
+        == unsubscribe_requests[2].notification.sent_at.strftime(date_format) + " GMT"
+    )
+
+
 @pytest.mark.parametrize(
     "new_custom_email_sender_name, expected_email_sender_local_part",
     [

From ffde1f38415d4b001e5b0e76fab2b73dff85f368 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Thu, 25 Jul 2024 18:04:50 +0100
Subject: [PATCH 18/32] Add get_unsubscribe_request_reports_by_id_dao

---
 app/dao/unsubscribe_request_dao.py            | 30 +++++++++--
 app/service/rest.py                           | 54 +++++++++++--------
 tests/app/dao/test_unsubscribe_request_dao.py |  3 +-
 tests/app/service/test_rest.py                | 30 ++++++-----
 4 files changed, 78 insertions(+), 39 deletions(-)

diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py
index 9c345329d6..f0f165551a 100644
--- a/app/dao/unsubscribe_request_dao.py
+++ b/app/dao/unsubscribe_request_dao.py
@@ -1,11 +1,10 @@
 from datetime import datetime
 
 from sqlalchemy import desc, func
-from sqlalchemy.sql.operators import is_
 
 from app import db
 from app.dao.dao_utils import autocommit
-from app.models import UnsubscribeRequest, UnsubscribeRequestReport
+from app.models import UnsubscribeRequest, UnsubscribeRequestReport, Notification, NotificationHistory, Template, Job
 from app.utils import midnight_n_days_ago
 
 
@@ -64,6 +63,32 @@ def get_unsubscribe_request_report_by_id_dao(batch_id):
     return UnsubscribeRequestReport.query.filter_by(id=batch_id).one_or_none()
 
 
+def get_unsubscribe_request_report_for_download_dao(service_id, batch_id):
+    results = []
+    for table in [Notification, NotificationHistory]:
+        query = (
+            db.session.query(
+                UnsubscribeRequest.notification_id,
+                UnsubscribeRequest.email_address,
+                Template.name.label("template_name"),
+                table.template_id,
+                Job.original_file_name,
+                table.sent_at.label("template_sent_at")
+            )
+            .outerjoin(Job, table.job_id == Job.id)
+            .filter(
+                UnsubscribeRequest.service_id == service_id,
+                UnsubscribeRequest.unsubscribe_request_report_id == batch_id,
+                UnsubscribeRequest.notification_id == table.id,
+                table.template_id == Template.id,
+            )
+            .order_by(desc(Template.name), desc(Job.original_file_name), desc(table.sent_at))
+        )
+        results = results + query.all()
+    results = sorted(results, key=lambda i: i.created_at, reverse=True)
+    return results
+
+
 def get_unbatched_unsubscribe_requests_dao(service_id):
     return (
         UnsubscribeRequest.query.filter_by(service_id=service_id, unsubscribe_request_report_id=None)
@@ -99,4 +124,3 @@ def assign_unbatched_unsubscribe_requests_to_report_dao(report_id, service_id, e
     for unsubscribe_request in unbatched_unsubscribe_requests:
         unsubscribe_request.unsubscribe_request_report_id = report_id
         db.session.add(unsubscribe_request)
-
diff --git a/app/service/rest.py b/app/service/rest.py
index 52e70b37ac..7deac307c6 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -106,6 +106,10 @@
     update_unsubscribe_request_report_processed_by_date_dao,
     get_unsubscribe_requests_statistics_dao, create_unsubscribe_request_reports_dao,
     assign_unbatched_unsubscribe_requests_to_report_dao,
+    create_unsubscribe_request_reports_dao,
+    get_latest_unsubscribe_request_date_dao,
+    get_unsubscribe_request_reports_by_id_dao,
+    get_unsubscribe_requests_statistics_dao,
 )
 from app.dao.users_dao import get_user_by_id
 from app.errors import InvalidRequest, register_errors
@@ -115,7 +119,8 @@
     LetterBranding,
     Permission,
     Service,
-    ServiceContactList, UnsubscribeRequestReport,
+    ServiceContactList,
+    UnsubscribeRequestReport,
 )
 from app.notifications.process_notifications import (
     persist_notification,
@@ -1164,34 +1169,37 @@ def create_unsubscribe_request_report(service_id):
         )
     else:
         raise InvalidRequest(
-            message=f"summary data needed to create an unsubscribe request report is missing",
+            message="summary data needed to create an unsubscribe request report is missing",
             status_code=400,
         )
 
 
 @service_blueprint.route("/<uuid:service_id>/unsubscribe-request-report/<uuid:batch_id>", methods=["GET"])
 def get_unsubscribe_request_report_for_download(service_id, batch_id):
-    report = get_unsubscribe_request_reports_by_id_dao(batch_id)
-    data = {
-        "batch_id": report.id,
-        "earliest_timestamp": report.earliest_timestamp,
-        "latest_timestamp": report.latest_timestamp,
-        "unsubscribe_requests": sorted(
-            [
-                {
-                    "email_address": unsubscribe_request.email_address,
-                    "template_name": unsubscribe_request.template.name,
-                    "sent_at": unsubscribe_request.notification.sent_at,
-                }
-                for unsubscribe_request in report.unsubscribe_requests
-            ],
-            key=lambda row: row["sent_at"],
-            reverse=True,
-        ),
-    }
-    return jsonify(data), 200
-
-
+    if report := get_unsubscribe_request_reports_by_id_dao(batch_id):
+        data = {
+            "batch_id": report.id,
+            "earliest_timestamp": report.earliest_timestamp,
+            "latest_timestamp": report.latest_timestamp,
+            "unsubscribe_requests": sorted(
+                [
+                    {
+                        "email_address": unsubscribe_request.email_address,
+                        "template_name": unsubscribe_request.template.name,
+                        "template_sent_at": unsubscribe_request.notification.sent_at,
+                    }
+                    for get_unsubscribe_request_report_for_download_dao(unsubscribe_request.id) in report.unsubscribe_requests
+                ],
+                key=lambda row: row["template_sent_at"],
+                reverse=True,
+            ),
+        }
+        return jsonify(data), 200
+    else:
+        raise InvalidRequest(
+            message=f"No report available for {batch_id}",
+            status_code=400,
+        )
 
 
 @service_blueprint.route("/<uuid:service_id>/contact-list", methods=["GET"])
diff --git a/tests/app/dao/test_unsubscribe_request_dao.py b/tests/app/dao/test_unsubscribe_request_dao.py
index 8031fa8f95..666ddc9f46 100644
--- a/tests/app/dao/test_unsubscribe_request_dao.py
+++ b/tests/app/dao/test_unsubscribe_request_dao.py
@@ -1,10 +1,11 @@
 from app.constants import EMAIL_TYPE
 from app.dao.unsubscribe_request_dao import (
+    assign_unbatched_unsubscribe_requests_to_report_dao,
     create_unsubscribe_request_dao,
     create_unsubscribe_request_reports_dao,
     get_latest_unsubscribe_request_date_dao,
     get_unsubscribe_request_by_notification_id_dao,
-    get_unsubscribe_requests_statistics_dao, assign_unbatched_unsubscribe_requests_to_report_dao,
+    get_unsubscribe_requests_statistics_dao,
 )
 from app.models import UnsubscribeRequest, UnsubscribeRequestReport
 from app.one_click_unsubscribe.rest import get_unsubscribe_request_data
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 7aa8102821..7e0306e7b4 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -31,8 +31,11 @@
     dao_remove_user_from_service,
 )
 from app.dao.templates_dao import dao_redact_template
-from app.dao.unsubscribe_request_dao import create_unsubscribe_request_dao, create_unsubscribe_request_reports_dao, \
-    assign_unbatched_unsubscribe_requests_to_report_dao
+from app.dao.unsubscribe_request_dao import (
+    assign_unbatched_unsubscribe_requests_to_report_dao,
+    create_unsubscribe_request_dao,
+    create_unsubscribe_request_reports_dao,
+)
 from app.dao.users_dao import save_model_user
 from app.models import (
     AnnualBilling,
@@ -44,8 +47,9 @@
     ServiceLetterContact,
     ServicePermission,
     ServiceSmsSender,
+    UnsubscribeRequest,
     UnsubscribeRequestReport,
-    User, UnsubscribeRequest,
+    User,
 )
 from app.utils import midnight_n_days_ago
 from tests import create_admin_authorization_header
@@ -3757,8 +3761,10 @@ def test_create_unsubscribe_request_report(sample_service, admin_request, mocker
         "app.service.rest.assign_unbatched_unsubscribe_requests_to_report_dao"
     )
     response = admin_request.post(
-        "service.create_unsubscribe_request_report", service_id=sample_service.id, _data=summary_data,
-        _expected_status=201
+        "service.create_unsubscribe_request_report",
+        service_id=sample_service.id,
+        _data=summary_data,
+        _expected_status=201,
     )
     created_unsubscribe_request_report = UnsubscribeRequestReport.query.filter_by(id=test_id).one()
     assert response == {"report_id": str(created_unsubscribe_request_report.id)}
@@ -3777,13 +3783,12 @@ def test_create_unsubscribe_request_report(sample_service, admin_request, mocker
 
 
 def test_create_unsubscribe_request_report_raises_error_for_no_summary_data(sample_service, admin_request, mocker):
-    response = admin_request.post(
-        "service.create_unsubscribe_request_report", service_id=sample_service.id, _data=None,
-        _expected_status=400
+    admin_request.post(
+        "service.create_unsubscribe_request_report", service_id=sample_service.id, _data=None, _expected_status=400
     )
 
 
-def test_get_unsubscribe_request_report_for_download(admin_request, sample_service, mocker):
+def test_get_unsubscribe_request_report_for_download(admin_request, sample_service):
     # Create 3 un-batched unsubscribe requests
     template_1 = create_template(sample_service, template_type=EMAIL_TYPE, template_name="First Template")
     notification_1 = create_notification(template=template_1, sent_at=midnight_n_days_ago(1))
@@ -3838,6 +3843,7 @@ def test_get_unsubscribe_request_report_for_download(admin_request, sample_servi
     assign_unbatched_unsubscribe_requests_to_report_dao(
         report_id=unsubscribe_request_report.id,
         service_id=unsubscribe_request_report.service_id,
+        earliest_timestamp=unsubscribe_request_report.earliest_timestamp,
         latest_timestamp=unsubscribe_request_report.latest_timestamp,
     )
 
@@ -3860,19 +3866,19 @@ def test_get_unsubscribe_request_report_for_download(admin_request, sample_servi
     assert response["unsubscribe_requests"][0]["email_address"] == unsubscribe_requests[0].email_address
     assert response["unsubscribe_requests"][0]["template_name"] == unsubscribe_requests[0].template.name
     assert (
-        response["unsubscribe_requests"][0]["sent_at"]
+        response["unsubscribe_requests"][0]["template_sent_at"]
         == unsubscribe_requests[0].notification.sent_at.strftime(date_format) + " GMT"
     )
     assert response["unsubscribe_requests"][1]["email_address"] == unsubscribe_requests[1].email_address
     assert response["unsubscribe_requests"][1]["template_name"] == unsubscribe_requests[1].template.name
     assert (
-        response["unsubscribe_requests"][1]["sent_at"]
+        response["unsubscribe_requests"][1]["template_sent_at"]
         == unsubscribe_requests[1].notification.sent_at.strftime(date_format) + " GMT"
     )
     assert response["unsubscribe_requests"][2]["email_address"] == unsubscribe_requests[2].email_address
     assert response["unsubscribe_requests"][2]["template_name"] == unsubscribe_requests[2].template.name
     assert (
-        response["unsubscribe_requests"][2]["sent_at"]
+        response["unsubscribe_requests"][2]["template_sent_at"]
         == unsubscribe_requests[2].notification.sent_at.strftime(date_format) + " GMT"
     )
 

From 9662517408a5f2af4e0d935e3dc66e2905e9c850 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Fri, 26 Jul 2024 14:18:00 +0100
Subject: [PATCH 19/32] Add get_unsubscribe_requests_data_for_download_dao 
 This method takes service_id and batch_id as parameters and retrieves the
 required download info  for all the batched unsubscribe requests associated
 with a batched report.

---
 app/dao/unsubscribe_request_dao.py            |  3 +-
 tests/app/dao/test_unsubscribe_request_dao.py | 96 ++++++++++++++++++-
 2 files changed, 95 insertions(+), 4 deletions(-)

diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py
index f0f165551a..7acb9b0b5b 100644
--- a/app/dao/unsubscribe_request_dao.py
+++ b/app/dao/unsubscribe_request_dao.py
@@ -63,7 +63,7 @@ def get_unsubscribe_request_report_by_id_dao(batch_id):
     return UnsubscribeRequestReport.query.filter_by(id=batch_id).one_or_none()
 
 
-def get_unsubscribe_request_report_for_download_dao(service_id, batch_id):
+def get_unsubscribe_requests_data_for_download_dao(service_id, batch_id):
     results = []
     for table in [Notification, NotificationHistory]:
         query = (
@@ -85,7 +85,6 @@ def get_unsubscribe_request_report_for_download_dao(service_id, batch_id):
             .order_by(desc(Template.name), desc(Job.original_file_name), desc(table.sent_at))
         )
         results = results + query.all()
-    results = sorted(results, key=lambda i: i.created_at, reverse=True)
     return results
 
 
diff --git a/tests/app/dao/test_unsubscribe_request_dao.py b/tests/app/dao/test_unsubscribe_request_dao.py
index 666ddc9f46..7556ee4ef8 100644
--- a/tests/app/dao/test_unsubscribe_request_dao.py
+++ b/tests/app/dao/test_unsubscribe_request_dao.py
@@ -5,12 +5,12 @@
     create_unsubscribe_request_reports_dao,
     get_latest_unsubscribe_request_date_dao,
     get_unsubscribe_request_by_notification_id_dao,
-    get_unsubscribe_requests_statistics_dao,
+    get_unsubscribe_requests_statistics_dao, get_unsubscribe_requests_data_for_download_dao,
 )
 from app.models import UnsubscribeRequest, UnsubscribeRequestReport
 from app.one_click_unsubscribe.rest import get_unsubscribe_request_data
 from app.utils import midnight_n_days_ago
-from tests.app.db import create_notification, create_service, create_template
+from tests.app.db import create_notification, create_service, create_template, create_job
 
 
 def test_create_unsubscribe_request_dao(sample_email_notification):
@@ -287,3 +287,95 @@ def test_assign_unbatched_unsubscribe_requests_to_report_dao(sample_service):
     for unsubscribe_request in unsubscribe_requests:
         assert unsubscribe_request.unsubscribe_request_report_id == unsubscribe_request_report.id
     assert len(unsubscribe_requests) == 2
+
+
+def test_get_unsubscribe_request_data_for_download_dao(sample_service):
+    unsubscribe_request_report = UnsubscribeRequestReport(
+        id="7536fd15-3d9c-494b-9053-0fd9822bcae6",
+        count=141,
+        earliest_timestamp=midnight_n_days_ago(4),
+        latest_timestamp=midnight_n_days_ago(0),
+        service_id=sample_service.id,
+    )
+    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+    template_1 = create_template(
+        sample_service,
+        template_type=EMAIL_TYPE,
+    )
+    job_1 = create_job(template=template_1, original_file_name="contact list")
+    notification_1 = create_notification(template=template_1, job=job_1,
+                                         to_field="foo@bar.com", sent_at=midnight_n_days_ago(1))
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_1.id,
+            "template_id": notification_1.template_id,
+            "template_version": notification_1.template_version,
+            "service_id": notification_1.service_id,
+            "email_address": notification_1.to,
+            "created_at": midnight_n_days_ago(1),
+            "unsubscribe_request_report_id": unsubscribe_request_report.id,
+        }
+    )
+    notification_2 = create_notification(template=template_1, job=job_1, to_field="fizz@bar.com",
+                                         sent_at=midnight_n_days_ago(2))
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_2.id,
+            "template_id": notification_2.template_id,
+            "template_version": notification_2.template_version,
+            "service_id": notification_2.service_id,
+            "email_address": notification_2.to,
+            "created_at": midnight_n_days_ago(2),
+            "unsubscribe_request_report_id": unsubscribe_request_report.id,
+        }
+    )
+    template_2 = create_template(
+        service=sample_service,
+        template_name="Another Service",
+        template_type=EMAIL_TYPE,
+    )
+    job_2 = create_job(template=template_2, original_file_name="another contact list")
+    notification_3 = create_notification(template=template_2, job=job_2, to_field="buzz@bar.com",
+                                         sent_at=midnight_n_days_ago(3))
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_3.id,
+            "template_id": notification_3.template_id,
+            "template_version": notification_3.template_version,
+            "service_id": notification_3.service_id,
+            "email_address": notification_3.to,
+            "created_at": midnight_n_days_ago(3),
+            "unsubscribe_request_report_id": unsubscribe_request_report.id,
+        }
+    )
+    notification_4 = create_notification(template=template_2, to_field="fizzbuzz@bar.com",
+                                         sent_at=midnight_n_days_ago(4))
+    create_unsubscribe_request_dao(
+        {  # noqa
+            "notification_id": notification_4.id,
+            "template_id": notification_4.template_id,
+            "template_version": notification_4.template_version,
+            "service_id": notification_4.service_id,
+            "email_address": notification_4.to,
+            "created_at": midnight_n_days_ago(4),
+            "unsubscribe_request_report_id": unsubscribe_request_report.id,
+        }
+    )
+
+    result = get_unsubscribe_requests_data_for_download_dao(sample_service.id, unsubscribe_request_report.id)
+    result[0].email_address == notification_1.to
+    result[0].template_name == notification_1.template.name
+    result[0].original_file_name == notification_1.job.original_file_name
+    result[0].template_sent_at == notification_1.sent_at
+    result[1].email_address == notification_2.to
+    result[1].template_name == notification_2.template.name
+    result[1].original_file_name == notification_2.job.original_file_name
+    result[1].template_sent_at == notification_2.sent_at
+    result[2].email_address == notification_3.to
+    result[2].template_name == notification_3.template.name
+    result[2].original_file_name == notification_3.job.original_file_name
+    result[2].template_sent_at == notification_3.sent_at
+    result[3].email_address == notification_4.to
+    result[3].template_name == notification_4.template.name
+    result[3].original_file_name == None
+    result[3].template_sent_at == notification_4.sent_at

From 7ecaa6fa41118be36fdb3f6d8d59efe751af41df Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Fri, 26 Jul 2024 14:46:27 +0100
Subject: [PATCH 20/32] Refactor get_unsubscribe_request_report_for_download

---
 app/service/rest.py            | 14 ++++++--------
 tests/app/service/test_rest.py |  3 +--
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/app/service/rest.py b/app/service/rest.py
index 7deac307c6..d2433d35c2 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -109,7 +109,7 @@
     create_unsubscribe_request_reports_dao,
     get_latest_unsubscribe_request_date_dao,
     get_unsubscribe_request_reports_by_id_dao,
-    get_unsubscribe_requests_statistics_dao,
+    get_unsubscribe_requests_statistics_dao, get_unsubscribe_requests_data_for_download_dao,
 )
 from app.dao.users_dao import get_user_by_id
 from app.errors import InvalidRequest, register_errors
@@ -1181,18 +1181,16 @@ def get_unsubscribe_request_report_for_download(service_id, batch_id):
             "batch_id": report.id,
             "earliest_timestamp": report.earliest_timestamp,
             "latest_timestamp": report.latest_timestamp,
-            "unsubscribe_requests": sorted(
+            "unsubscribe_requests":
                 [
                     {
                         "email_address": unsubscribe_request.email_address,
-                        "template_name": unsubscribe_request.template.name,
-                        "template_sent_at": unsubscribe_request.notification.sent_at,
+                        "template_name": unsubscribe_request.template_name,
+                        "original_file_name": unsubscribe_request.original_file_name,
+                        "template_sent_at": unsubscribe_request.template_sent_at,
                     }
-                    for get_unsubscribe_request_report_for_download_dao(unsubscribe_request.id) in report.unsubscribe_requests
+                    for unsubscribe_request in get_unsubscribe_requests_data_for_download_dao(service_id, report.id)
                 ],
-                key=lambda row: row["template_sent_at"],
-                reverse=True,
-            ),
         }
         return jsonify(data), 200
     else:
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 7e0306e7b4..e29ee59c2d 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3854,8 +3854,7 @@ def test_get_unsubscribe_request_report_for_download(admin_request, sample_servi
     )
     unsubscribe_requests = sorted(
         UnsubscribeRequest.query.filter_by(service_id=sample_service.id).all(),
-        key=lambda row: row.notification.sent_at,
-        reverse=True,
+        key=lambda row: row.template.name, reverse=True,
     )
     date_format = "%a, %d %b %Y %H:%M:%S"
     assert response["batch_id"] == str(unsubscribe_request_report.id)

From 8a5c3e9394c95efc7ae44f9c844bff1078d9b9bb Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Fri, 26 Jul 2024 15:58:40 +0100
Subject: [PATCH 21/32] Add
 test_get_unsubscribe_request_reports_by_id_dao,test_get_unsubscribe_request_report_for_download

---
 app/dao/unsubscribe_request_dao.py            |   9 +-
 app/service/rest.py                           |  32 ++---
 tests/app/dao/test_unsubscribe_request_dao.py |  89 +++++++++----
 tests/app/service/test_rest.py                | 124 ++++++------------
 4 files changed, 122 insertions(+), 132 deletions(-)

diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py
index 7acb9b0b5b..7264c99734 100644
--- a/app/dao/unsubscribe_request_dao.py
+++ b/app/dao/unsubscribe_request_dao.py
@@ -4,7 +4,7 @@
 
 from app import db
 from app.dao.dao_utils import autocommit
-from app.models import UnsubscribeRequest, UnsubscribeRequestReport, Notification, NotificationHistory, Template, Job
+from app.models import Job, Notification, NotificationHistory, Template, UnsubscribeRequest, UnsubscribeRequestReport
 from app.utils import midnight_n_days_ago
 
 
@@ -72,8 +72,8 @@ def get_unsubscribe_requests_data_for_download_dao(service_id, batch_id):
                 UnsubscribeRequest.email_address,
                 Template.name.label("template_name"),
                 table.template_id,
-                Job.original_file_name,
-                table.sent_at.label("template_sent_at")
+                func.coalesce(Job.original_file_name, "N/A").label("original_file_name"),
+                table.sent_at.label("template_sent_at"),
             )
             .outerjoin(Job, table.job_id == Job.id)
             .filter(
@@ -108,8 +108,7 @@ def update_unsubscribe_request_report_processed_by_date_dao(report, report_has_b
 
 
 @autocommit
-def assign_unbatched_unsubscribe_requests_to_report_dao(report_id, service_id, earliest_timestamp,
-                                                        latest_timestamp):
+def assign_unbatched_unsubscribe_requests_to_report_dao(report_id, service_id, earliest_timestamp, latest_timestamp):
     """
     This method retrieves all the un-batched unsubscribe requests received before or on
     the report's latest_timestamp and sets their unsubscribe_request_report_id to the report_i
diff --git a/app/service/rest.py b/app/service/rest.py
index d2433d35c2..0886b97962 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -100,16 +100,13 @@
 )
 from app.dao.templates_dao import dao_get_template_by_id
 from app.dao.unsubscribe_request_dao import (
+    assign_unbatched_unsubscribe_requests_to_report_dao,
+    create_unsubscribe_request_reports_dao,
     get_latest_unsubscribe_request_date_dao,
     get_unsubscribe_request_report_by_id_dao,
+    get_unsubscribe_requests_data_for_download_dao,
     get_unsubscribe_requests_statistics_dao,
     update_unsubscribe_request_report_processed_by_date_dao,
-    get_unsubscribe_requests_statistics_dao, create_unsubscribe_request_reports_dao,
-    assign_unbatched_unsubscribe_requests_to_report_dao,
-    create_unsubscribe_request_reports_dao,
-    get_latest_unsubscribe_request_date_dao,
-    get_unsubscribe_request_reports_by_id_dao,
-    get_unsubscribe_requests_statistics_dao, get_unsubscribe_requests_data_for_download_dao,
 )
 from app.dao.users_dao import get_user_by_id
 from app.errors import InvalidRequest, register_errors
@@ -1169,28 +1166,27 @@ def create_unsubscribe_request_report(service_id):
         )
     else:
         raise InvalidRequest(
-            message="summary data needed to create an unsubscribe request report is missing",
+            message={"summary_data": "summary data needed to create an unsubscribe request report is missing"},
             status_code=400,
         )
 
 
 @service_blueprint.route("/<uuid:service_id>/unsubscribe-request-report/<uuid:batch_id>", methods=["GET"])
 def get_unsubscribe_request_report_for_download(service_id, batch_id):
-    if report := get_unsubscribe_request_reports_by_id_dao(batch_id):
+    if report := get_unsubscribe_request_report_by_id_dao(batch_id):
         data = {
             "batch_id": report.id,
             "earliest_timestamp": report.earliest_timestamp,
             "latest_timestamp": report.latest_timestamp,
-            "unsubscribe_requests":
-                [
-                    {
-                        "email_address": unsubscribe_request.email_address,
-                        "template_name": unsubscribe_request.template_name,
-                        "original_file_name": unsubscribe_request.original_file_name,
-                        "template_sent_at": unsubscribe_request.template_sent_at,
-                    }
-                    for unsubscribe_request in get_unsubscribe_requests_data_for_download_dao(service_id, report.id)
-                ],
+            "unsubscribe_requests": [
+                {
+                    "email_address": unsubscribe_request.email_address,
+                    "template_name": unsubscribe_request.template_name,
+                    "original_file_name": unsubscribe_request.original_file_name,
+                    "template_sent_at": unsubscribe_request.template_sent_at,
+                }
+                for unsubscribe_request in get_unsubscribe_requests_data_for_download_dao(service_id, report.id)
+            ],
         }
         return jsonify(data), 200
     else:
diff --git a/tests/app/dao/test_unsubscribe_request_dao.py b/tests/app/dao/test_unsubscribe_request_dao.py
index 7556ee4ef8..67ce20687d 100644
--- a/tests/app/dao/test_unsubscribe_request_dao.py
+++ b/tests/app/dao/test_unsubscribe_request_dao.py
@@ -5,12 +5,14 @@
     create_unsubscribe_request_reports_dao,
     get_latest_unsubscribe_request_date_dao,
     get_unsubscribe_request_by_notification_id_dao,
-    get_unsubscribe_requests_statistics_dao, get_unsubscribe_requests_data_for_download_dao,
+    get_unsubscribe_request_report_by_id_dao,
+    get_unsubscribe_requests_data_for_download_dao,
+    get_unsubscribe_requests_statistics_dao,
 )
 from app.models import UnsubscribeRequest, UnsubscribeRequestReport
 from app.one_click_unsubscribe.rest import get_unsubscribe_request_data
 from app.utils import midnight_n_days_ago
-from tests.app.db import create_notification, create_service, create_template, create_job
+from tests.app.db import create_job, create_notification, create_service, create_template
 
 
 def test_create_unsubscribe_request_dao(sample_email_notification):
@@ -300,11 +302,13 @@ def test_get_unsubscribe_request_data_for_download_dao(sample_service):
     create_unsubscribe_request_reports_dao(unsubscribe_request_report)
     template_1 = create_template(
         sample_service,
+        template_name="first Template",
         template_type=EMAIL_TYPE,
     )
     job_1 = create_job(template=template_1, original_file_name="contact list")
-    notification_1 = create_notification(template=template_1, job=job_1,
-                                         to_field="foo@bar.com", sent_at=midnight_n_days_ago(1))
+    notification_1 = create_notification(
+        template=template_1, job=job_1, to_field="foo@bar.com", sent_at=midnight_n_days_ago(1)
+    )
     create_unsubscribe_request_dao(
         {  # noqa
             "notification_id": notification_1.id,
@@ -316,8 +320,9 @@ def test_get_unsubscribe_request_data_for_download_dao(sample_service):
             "unsubscribe_request_report_id": unsubscribe_request_report.id,
         }
     )
-    notification_2 = create_notification(template=template_1, job=job_1, to_field="fizz@bar.com",
-                                         sent_at=midnight_n_days_ago(2))
+    notification_2 = create_notification(
+        template=template_1, job=job_1, to_field="fizz@bar.com", sent_at=midnight_n_days_ago(2)
+    )
     create_unsubscribe_request_dao(
         {  # noqa
             "notification_id": notification_2.id,
@@ -331,12 +336,13 @@ def test_get_unsubscribe_request_data_for_download_dao(sample_service):
     )
     template_2 = create_template(
         service=sample_service,
-        template_name="Another Service",
+        template_name="Another Template",
         template_type=EMAIL_TYPE,
     )
     job_2 = create_job(template=template_2, original_file_name="another contact list")
-    notification_3 = create_notification(template=template_2, job=job_2, to_field="buzz@bar.com",
-                                         sent_at=midnight_n_days_ago(3))
+    notification_3 = create_notification(
+        template=template_2, job=job_2, to_field="buzz@bar.com", sent_at=midnight_n_days_ago(3)
+    )
     create_unsubscribe_request_dao(
         {  # noqa
             "notification_id": notification_3.id,
@@ -348,8 +354,9 @@ def test_get_unsubscribe_request_data_for_download_dao(sample_service):
             "unsubscribe_request_report_id": unsubscribe_request_report.id,
         }
     )
-    notification_4 = create_notification(template=template_2, to_field="fizzbuzz@bar.com",
-                                         sent_at=midnight_n_days_ago(4))
+    notification_4 = create_notification(
+        template=template_2, to_field="fizzbuzz@bar.com", sent_at=midnight_n_days_ago(4)
+    )
     create_unsubscribe_request_dao(
         {  # noqa
             "notification_id": notification_4.id,
@@ -363,19 +370,47 @@ def test_get_unsubscribe_request_data_for_download_dao(sample_service):
     )
 
     result = get_unsubscribe_requests_data_for_download_dao(sample_service.id, unsubscribe_request_report.id)
-    result[0].email_address == notification_1.to
-    result[0].template_name == notification_1.template.name
-    result[0].original_file_name == notification_1.job.original_file_name
-    result[0].template_sent_at == notification_1.sent_at
-    result[1].email_address == notification_2.to
-    result[1].template_name == notification_2.template.name
-    result[1].original_file_name == notification_2.job.original_file_name
-    result[1].template_sent_at == notification_2.sent_at
-    result[2].email_address == notification_3.to
-    result[2].template_name == notification_3.template.name
-    result[2].original_file_name == notification_3.job.original_file_name
-    result[2].template_sent_at == notification_3.sent_at
-    result[3].email_address == notification_4.to
-    result[3].template_name == notification_4.template.name
-    result[3].original_file_name == None
-    result[3].template_sent_at == notification_4.sent_at
+
+    assert result[0].email_address == notification_1.to
+    assert result[0].template_name == notification_1.template.name
+    assert result[0].original_file_name == notification_1.job.original_file_name
+    assert result[0].template_sent_at == notification_1.sent_at
+    assert result[1].email_address == notification_2.to
+    assert result[1].template_name == notification_2.template.name
+    assert result[1].original_file_name == notification_2.job.original_file_name
+    assert result[1].template_sent_at == notification_2.sent_at
+    assert result[2].email_address == notification_4.to
+    assert result[2].template_name == notification_4.template.name
+    assert result[2].original_file_name == "N/A"
+    assert result[2].template_sent_at == notification_4.sent_at
+    assert result[3].email_address == notification_3.to
+    assert result[3].template_name == notification_3.template.name
+    assert result[3].original_file_name == notification_3.job.original_file_name
+    assert result[3].template_sent_at == notification_3.sent_at
+
+
+def test_get_unsubscribe_request_data_for_download_dao_invalid_batch_id(sample_service):
+    result = get_unsubscribe_requests_data_for_download_dao(sample_service.id, "c5019907-656a-4adf-9c02-da422529e507")
+    assert result == []
+
+
+def test_get_unsubscribe_request_report_by_id_dao(sample_service):
+    unsubscribe_request_report = UnsubscribeRequestReport(
+        id="7536fd15-3d9c-494b-9053-0fd9822bcae6",
+        count=141,
+        earliest_timestamp=midnight_n_days_ago(4),
+        latest_timestamp=midnight_n_days_ago(0),
+        service_id=sample_service.id,
+    )
+    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
+    result = get_unsubscribe_request_report_by_id_dao(unsubscribe_request_report.id)
+    assert result.id == unsubscribe_request_report.id
+    assert result.count == unsubscribe_request_report.count
+    assert result.earliest_timestamp == unsubscribe_request_report.earliest_timestamp
+    assert result.latest_timestamp == unsubscribe_request_report.latest_timestamp
+    assert result.service_id == unsubscribe_request_report.service_id
+
+
+def test_get_unsubscribe_request_report_by_id_dao_invalid_service_id(sample_service):
+    result = get_unsubscribe_request_report_by_id_dao("c5019907-656a-4adf-9c02-da422529e507")
+    assert result is None
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index e29ee59c2d..bdff009988 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -32,7 +32,6 @@
 )
 from app.dao.templates_dao import dao_redact_template
 from app.dao.unsubscribe_request_dao import (
-    assign_unbatched_unsubscribe_requests_to_report_dao,
     create_unsubscribe_request_dao,
     create_unsubscribe_request_reports_dao,
 )
@@ -47,11 +46,9 @@
     ServiceLetterContact,
     ServicePermission,
     ServiceSmsSender,
-    UnsubscribeRequest,
     UnsubscribeRequestReport,
     User,
 )
-from app.utils import midnight_n_days_ago
 from tests import create_admin_authorization_header
 from tests.app.db import (
     create_annual_billing,
@@ -3788,97 +3785,60 @@ def test_create_unsubscribe_request_report_raises_error_for_no_summary_data(samp
     )
 
 
-def test_get_unsubscribe_request_report_for_download(admin_request, sample_service):
-    # Create 3 un-batched unsubscribe requests
-    template_1 = create_template(sample_service, template_type=EMAIL_TYPE, template_name="First Template")
-    notification_1 = create_notification(template=template_1, sent_at=midnight_n_days_ago(1))
-    create_unsubscribe_request_dao(
-        {  # noqa
-            "notification_id": notification_1.id,
-            "template_id": notification_1.template_id,
-            "template_version": notification_1.template_version,
-            "service_id": notification_1.service_id,
-            "email_address": notification_1.to,
-            "created_at": midnight_n_days_ago(0),
-        }
+def test_get_unsubscribe_request_report_for_download(admin_request, sample_service, mocker):
+    # test_data
+    UnsubscribeRequestReport = namedtuple(
+        "UnsubscribeRequestReport", ["id", "earliest_timestamp", "latest_timestamp", "unsubscribe_requests"]
     )
-
-    notification_2 = create_notification(template=template_1, sent_at=midnight_n_days_ago(3))
-    create_unsubscribe_request_dao(
-        {  # noqa
-            "notification_id": notification_2.id,
-            "template_id": notification_2.template_id,
-            "template_version": notification_2.template_version,
-            "service_id": notification_2.service_id,
-            "email_address": notification_2.to,
-            "created_at": midnight_n_days_ago(2),
-        }
+    UnsubscribeRequest = namedtuple(
+        "UnsubscribeRequest", ["email_address", "template_name", "original_file_name", "template_sent_at"]
     )
 
-    template_2 = create_template(sample_service, template_type=EMAIL_TYPE, template_name="Second Template")
-    notification_3 = create_notification(template=template_2, sent_at=midnight_n_days_ago(5))
-    create_unsubscribe_request_dao(
-        {  # noqa
-            "notification_id": notification_3.id,
-            "template_id": notification_3.template_id,
-            "template_version": notification_3.template_version,
-            "service_id": notification_3.service_id,
-            "email_address": notification_3.to,
-            "created_at": midnight_n_days_ago(4),
-        }
+    unsubscribe_request_1 = UnsubscribeRequest(
+        "foo@bar.com", "email Template Name", "contact list", "2024-07-23 13:30:00"
+    )
+    unsubscribe_request_2 = UnsubscribeRequest(
+        "fizz@bar.com", "email Template Name", "contact list", "2024-07-21 11:04:00"
+    )
+    unsubscribe_request_3 = UnsubscribeRequest("fizzbuzz@bar.com", "Another Service", None, "2024-07-19 23:45:00")
+    unsubscribe_request_4 = UnsubscribeRequest(
+        "buzz@bar.com", "Another Service", "another contact list", "2024-07-17 09:42:00"
     )
-
-    # Create unsubscribe_request_report
     unsubscribe_request_report = UnsubscribeRequestReport(
-        id="7536fd15-3d9c-494b-9053-0fd9822bcae6",
-        count=141,
-        earliest_timestamp=midnight_n_days_ago(4),
-        latest_timestamp=midnight_n_days_ago(0),
-        service_id=sample_service.id,
+        "e6c02a98-8e64-4ab3-b176-271274517c21",
+        "2024-07-17 09:42:00",
+        "2024-07-23 13:30:00",
+        [unsubscribe_request_1, unsubscribe_request_2, unsubscribe_request_3, unsubscribe_request_4],
     )
-
-    create_unsubscribe_request_reports_dao(unsubscribe_request_report)
-
-    # batch the unsubscribe_requests
-    assign_unbatched_unsubscribe_requests_to_report_dao(
-        report_id=unsubscribe_request_report.id,
-        service_id=unsubscribe_request_report.service_id,
-        earliest_timestamp=unsubscribe_request_report.earliest_timestamp,
-        latest_timestamp=unsubscribe_request_report.latest_timestamp,
+    mocker.patch("app.service.rest.get_unsubscribe_request_report_by_id_dao", return_value=unsubscribe_request_report)
+    mocker.patch(
+        "app.service.rest.get_unsubscribe_requests_data_for_download_dao",
+        return_value=unsubscribe_request_report.unsubscribe_requests,
     )
-
     response = admin_request.get(
         "service.get_unsubscribe_request_report_for_download",
         service_id=sample_service.id,
         batch_id=unsubscribe_request_report.id,
     )
-    unsubscribe_requests = sorted(
-        UnsubscribeRequest.query.filter_by(service_id=sample_service.id).all(),
-        key=lambda row: row.template.name, reverse=True,
-    )
-    date_format = "%a, %d %b %Y %H:%M:%S"
-    assert response["batch_id"] == str(unsubscribe_request_report.id)
-    assert (
-        response["earliest_timestamp"] == unsubscribe_request_report.earliest_timestamp.strftime(date_format) + " GMT"
-    )
-    assert response["latest_timestamp"] == unsubscribe_request_report.latest_timestamp.strftime(date_format) + " GMT"
-    assert response["unsubscribe_requests"][0]["email_address"] == unsubscribe_requests[0].email_address
-    assert response["unsubscribe_requests"][0]["template_name"] == unsubscribe_requests[0].template.name
-    assert (
-        response["unsubscribe_requests"][0]["template_sent_at"]
-        == unsubscribe_requests[0].notification.sent_at.strftime(date_format) + " GMT"
-    )
-    assert response["unsubscribe_requests"][1]["email_address"] == unsubscribe_requests[1].email_address
-    assert response["unsubscribe_requests"][1]["template_name"] == unsubscribe_requests[1].template.name
-    assert (
-        response["unsubscribe_requests"][1]["template_sent_at"]
-        == unsubscribe_requests[1].notification.sent_at.strftime(date_format) + " GMT"
-    )
-    assert response["unsubscribe_requests"][2]["email_address"] == unsubscribe_requests[2].email_address
-    assert response["unsubscribe_requests"][2]["template_name"] == unsubscribe_requests[2].template.name
-    assert (
-        response["unsubscribe_requests"][2]["template_sent_at"]
-        == unsubscribe_requests[2].notification.sent_at.strftime(date_format) + " GMT"
+
+    assert response["batch_id"] == unsubscribe_request_report.id
+    assert response["earliest_timestamp"] == unsubscribe_request_report.earliest_timestamp
+    assert response["latest_timestamp"] == unsubscribe_request_report.latest_timestamp
+
+    for i, row in enumerate(unsubscribe_request_report.unsubscribe_requests):
+        assert response["unsubscribe_requests"][i]["email_address"] == row.email_address
+        assert response["unsubscribe_requests"][i]["template_name"] == row.template_name
+        assert response["unsubscribe_requests"][i]["original_file_name"] == row.original_file_name
+        assert response["unsubscribe_requests"][i]["template_sent_at"] == row.template_sent_at
+
+
+def test_get_unsubscribe_request_report_for_download_400_error(admin_request, sample_service):
+    invalid_batch_id = "c92de771-32a0-49ec-b398-75b1308c7142"
+    admin_request.get(
+        "service.get_unsubscribe_request_report_for_download",
+        service_id=sample_service.id,
+        batch_id=invalid_batch_id,
+        _expected_status=400,
     )
 
 

From 9bfb3fa15f0d73c573729b5a298cc2b23499943d Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 1 Aug 2024 10:39:41 +0100
Subject: [PATCH 22/32] Bump utils to 82.2.0

 ## 82.2.0

* Add `unsubscribe_link` argument to email templates

 ## 82.1.2

* Write updated version number to `requirements.txt` if no `requirements.in` file found

 ## 82.1.1
*  Fix the way we log the request_size. Accessing the data at this point can trigger a validation error early and cause a 500 error

 ## 82.1.0
*  Adds new logging fields to request logging. Namely environment_name, request_size and response_size

***

Complete changes: https://github.com/alphagov/notifications-utils/compare/82.0.0...82.2.0
---
 requirements.in  | 2 +-
 requirements.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/requirements.in b/requirements.in
index b71a132dfe..31ea0b54d7 100644
--- a/requirements.in
+++ b/requirements.in
@@ -24,7 +24,7 @@ lxml==4.9.3
 notifications-python-client==8.0.1
 
 # Run `make bump-utils` to update to the latest version
-notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.0.0
+notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.2.0
 
 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
 prometheus-client==0.14.1
diff --git a/requirements.txt b/requirements.txt
index 11a36b0ea1..48ccaae16c 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -151,7 +151,7 @@ mistune==0.8.4
     # via notifications-utils
 notifications-python-client==8.0.1
     # via -r requirements.in
-notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.0.0
+notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.2.0
     # via -r requirements.in
 ordered-set==4.1.0
     # via notifications-utils

From 029764c81c575fe6c77f6b1e2fa7c8d11a83e54e Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 26 Jul 2024 15:24:13 +0100
Subject: [PATCH 23/32] Generate unsubscribe link at time of sending
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We need to store the unsubscribe link where it is something that a user
has provided to us through an API call.

But where we are generating the link we don’t need to store it, because
we already store all the information needed to generate it.

By not storing it we make it easy to differentiate which notifications
have user-supplied unsubscribe links and which have ones which we are
generating.
---
 app/delivery/send_to_providers.py             | 29 ++++++-----
 app/models.py                                 | 15 +++++-
 app/notifications/process_notifications.py    |  6 ---
 tests/app/delivery/test_send_to_providers.py  | 49 +++++++++++++++++--
 .../test_process_notification.py              | 12 +----
 5 files changed, 78 insertions(+), 33 deletions(-)

diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py
index 6d4ced569c..2e22b56230 100644
--- a/app/delivery/send_to_providers.py
+++ b/app/delivery/send_to_providers.py
@@ -100,16 +100,16 @@ def send_sms_to_provider(notification):
             statsd_client.timing("sms.live-key.total-time", delta_seconds)
 
 
-def _get_email_headers(notification: Notification) -> list[dict[str, str]]:
-    headers = []
-
-    if notification.unsubscribe_link:
-        headers += [
-            {"Name": "List-Unsubscribe", "Value": f"<{notification.unsubscribe_link}>"},
+def _get_email_headers(notification: Notification, template: SerialisedTemplate) -> list[dict[str, str]]:
+    if unsubscribe_link := notification.get_unsubscribe_link_for_headers(
+        template_has_unsubscribe_link=template.has_unsubscribe_link
+    ):
+        return [
+            {"Name": "List-Unsubscribe", "Value": f"<{unsubscribe_link}>"},
             {"Name": "List-Unsubscribe-Post", "Value": "List-Unsubscribe=One-Click"},
         ]
 
-    return headers
+    return []
 
 
 def send_email_to_provider(notification):
@@ -121,15 +121,20 @@ def send_email_to_provider(notification):
     if notification.status == "created":
         provider = provider_to_use(EMAIL_TYPE)
 
-        template_dict = SerialisedTemplate.from_id_and_service_id(
+        template = SerialisedTemplate.from_id_and_service_id(
             template_id=notification.template_id, service_id=service.id, version=notification.template_version
-        ).__dict__
+        )
 
         html_email = HTMLEmailTemplate(
-            template_dict, values=notification.personalisation, **get_html_email_options(service)
+            template.__dict__,
+            values=notification.personalisation,
+            **get_html_email_options(service),
         )
 
-        plain_text_email = PlainTextEmailTemplate(template_dict, values=notification.personalisation)
+        plain_text_email = PlainTextEmailTemplate(
+            template.__dict__,
+            values=notification.personalisation,
+        )
         created_at = notification.created_at
         key_type = notification.key_type
         if notification.key_type == KEY_TYPE_TEST:
@@ -149,7 +154,7 @@ def send_email_to_provider(notification):
                 body=str(plain_text_email),
                 html_body=str(html_email),
                 reply_to_address=notification.reply_to_text,
-                headers=_get_email_headers(notification),
+                headers=_get_email_headers(notification, template),
             )
             notification.reference = reference
             update_notification_to_sending(notification, provider)
diff --git a/app/models.py b/app/models.py
index e68ae597f5..1750bfe566 100644
--- a/app/models.py
+++ b/app/models.py
@@ -77,6 +77,7 @@
     DATETIME_FORMAT_NO_TIMEZONE,
     get_dt_string_or_none,
     get_uuid_string_or_none,
+    url_with_token,
 )
 
 
@@ -1622,7 +1623,9 @@ def serialize(self):
             "completed_at": self.completed_at(),
             "scheduled_for": None,
             "postage": self.postage,
-            "one_click_unsubscribe_url": self.unsubscribe_link,
+            "one_click_unsubscribe_url": self.get_unsubscribe_link_for_headers(
+                template_has_unsubscribe_link=self.template.has_unsubscribe_link
+            ),
         }
 
         if self.notification_type == LETTER_TYPE:
@@ -1748,6 +1751,16 @@ def _get_letter_cost(self):
 
         return letter_rate
 
+    def get_unsubscribe_link_for_headers(self, *, template_has_unsubscribe_link):
+        if self.unsubscribe_link:
+            return self.unsubscribe_link
+        if template_has_unsubscribe_link:
+            return url_with_token(
+                self.to,
+                url=f"/unsubscribe/{str(self.id)}/",
+                base_url=current_app.config["API_HOST_NAME"],
+            )
+
 
 class NotificationHistory(db.Model):
     __tablename__ = "notification_history"
diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py
index b51737e4ab..36d48381e0 100644
--- a/app/notifications/process_notifications.py
+++ b/app/notifications/process_notifications.py
@@ -36,7 +36,6 @@
     dao_delete_notifications_by_id,
 )
 from app.models import Notification
-from app.utils import url_with_token
 from app.v2.errors import BadRequestError, QrCodeTooLongError
 
 REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram(
@@ -125,11 +124,6 @@ def persist_notification(
     if not notification_id:
         notification_id = uuid.uuid4()
 
-    if template_has_unsubscribe_link and not unsubscribe_link:
-        base_url = current_app.config["API_HOST_NAME"]
-        url = f"/unsubscribe/{str(notification_id)}/"
-        unsubscribe_link = url_with_token(recipient, url=url, base_url=base_url)
-
     notification = Notification(
         id=notification_id,
         template_id=template_id,
diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py
index fecfc26a39..9220bee135 100644
--- a/tests/app/delivery/test_send_to_providers.py
+++ b/tests/app/delivery/test_send_to_providers.py
@@ -2,7 +2,7 @@
 import uuid
 from collections import namedtuple
 from datetime import datetime, timedelta
-from unittest.mock import ANY
+from unittest.mock import ANY, call
 
 import pytest
 from flask import current_app
@@ -843,10 +843,25 @@ def test_get_html_email_options_add_email_branding_from_service(sample_service):
     }
 
 
-def test_send_email_to_provider_sends_unsubscribe_link(sample_email_template, mocker):
+@pytest.mark.parametrize(
+    "template_has_unsubscribe_link",
+    (
+        # If the notification has an unsubscribe link it shouldn’t matter what the
+        # setting on the template is
+        True,
+        False,
+    ),
+)
+def test_send_email_to_provider_sends_unsubscribe_link(sample_service, mocker, template_has_unsubscribe_link):
     mocker.patch("app.aws_ses_client.send_email", return_value="reference")
 
-    db_notification = create_notification(template=sample_email_template, unsubscribe_link="https://example.com")
+    template = create_template(
+        service=sample_service,
+        template_type="email",
+        has_unsubscribe_link=template_has_unsubscribe_link,
+    )
+
+    db_notification = create_notification(template=template, unsubscribe_link="https://example.com")
 
     expected_headers = [
         {"Name": "List-Unsubscribe", "Value": "<https://example.com>"},
@@ -858,3 +873,31 @@ def test_send_email_to_provider_sends_unsubscribe_link(sample_email_template, mo
     )
     app.aws_ses_client.send_email.assert_called_once()
     assert app.aws_ses_client.send_email.call_args[1]["headers"] == expected_headers
+
+
+def test_send_email_to_provider_sends_unsubscribe_link_if_template_is_unsubscribable(sample_service, mocker):
+    mocker.patch("app.aws_ses_client.send_email", return_value="reference")
+    mock_url_with_token = mocker.patch(
+        "app.models.url_with_token",
+        return_value="https://api.notify.example.com",
+    )
+
+    template = create_template(
+        service=sample_service,
+        template_type="email",
+        has_unsubscribe_link=True,
+    )
+
+    db_notification = create_notification(template=template)
+
+    send_to_providers.send_email_to_provider(db_notification)
+    app.aws_ses_client.send_email.assert_called_once()
+
+    assert mock_url_with_token.call_args_list == [
+        call("test@example.com", url=f"/unsubscribe/{db_notification.id}/", base_url="http://localhost:6011"),
+    ]
+
+    assert app.aws_ses_client.send_email.call_args[1]["headers"] == [
+        {"Name": "List-Unsubscribe", "Value": "<https://api.notify.example.com>"},
+        {"Name": "List-Unsubscribe-Post", "Value": "List-Unsubscribe=One-Click"},
+    ]
diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py
index 8dd5218995..dc2e9e0cfd 100644
--- a/tests/app/notifications/test_process_notification.py
+++ b/tests/app/notifications/test_process_notification.py
@@ -612,11 +612,6 @@ def test_persist_notification_when_template_has_unsubscribe_link_is_false(
     sample_job.service = template.service
     recipient = "foo@bar.com"
 
-    mocker.patch(
-        "app.notifications.process_notifications.url_with_token",
-        return_value="https://notify-generated-super-unsubscribe-link.com/unsubscribe",
-    )
-
     persist_notification(
         notification_id=uuid.uuid4(),
         template_id=sample_job.template.id,
@@ -640,7 +635,7 @@ def test_persist_notification_when_template_has_unsubscribe_link_is_false(
     "unsubscribe_link, expected_unsubscribe_link",
     [
         ("https://please-unsubscribe-me.com/unsubscribe", "https://please-unsubscribe-me.com/unsubscribe"),
-        (None, "https://notify-generated-super-unsubscribe-link.com/unsubscribe"),
+        (None, None),
     ],
 )
 def test_persist_notification_when_template_has_unsubscribe_link_is_true(
@@ -661,11 +656,6 @@ def test_persist_notification_when_template_has_unsubscribe_link_is_true(
 
     recipient = "foo@bar.com"
 
-    mocker.patch(
-        "app.notifications.process_notifications.url_with_token",
-        return_value="https://notify-generated-super-unsubscribe-link.com/unsubscribe",
-    )
-
     persist_notification(
         notification_id=uuid.uuid4(),
         template_id=job.template.id,

From aa66974f2ccd8d34614fe47c8dbee13d15c7edd3 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 26 Jul 2024 15:24:13 +0100
Subject: [PATCH 24/32] Add unsubscribe link to body of emails
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We have started sending list-unsubscribe headers if a user marks a
template as unsubscribeable, for teams who aren’t using the API.

However we are already telling these teams to add unsubscribe links to
the body of their emails.

This means:

- there are two different ways to add unsubscribe links
- users will have to manage requests to unsubscribe coming from two
  different places

This will be confusing, and make it harder for us to write good guidance
that users will be able to follow, and harder to explain what the ‘let
users unsubscribe from these emails’ checkbox does.

If we have a confusing product proposition and users cant follow our
guidance then we risk our spam score rising.

This commit adds a way to generate a separate unsubscribe link which we
can add to the footer of every email (using the argument added in
https://github.com/alphagov/notifications-utils/pull/1136/files)

We can then report unsubscribes back to the team in the same way, no
matter if the recipient has clicked the button in their email client
or the link at the end of the email.

If the user has checked the checkbox then we will always show the link
in the body of the email (this feels like the most predictable
behaviour). If they have also passed in a one-click URL for the headers
then we will respect that too.

The only combination which isn’t possible is using Notify’s generated
URL for one-click, and your own in the body of the email. This feels
like the most unlikely combination.
---
 app/delivery/send_to_providers.py            |  6 ++++
 app/models.py                                | 17 +++++++---
 tests/app/delivery/test_send_to_providers.py | 35 ++++++++++++++++----
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py
index 2e22b56230..f6242e2a50 100644
--- a/app/delivery/send_to_providers.py
+++ b/app/delivery/send_to_providers.py
@@ -125,15 +125,21 @@ def send_email_to_provider(notification):
             template_id=notification.template_id, service_id=service.id, version=notification.template_version
         )
 
+        unsubscribe_link_for_body = notification.get_unsubscribe_link_for_body(
+            template_has_unsubscribe_link=template.has_unsubscribe_link
+        )
+
         html_email = HTMLEmailTemplate(
             template.__dict__,
             values=notification.personalisation,
+            unsubscribe_link=unsubscribe_link_for_body,
             **get_html_email_options(service),
         )
 
         plain_text_email = PlainTextEmailTemplate(
             template.__dict__,
             values=notification.personalisation,
+            unsubscribe_link=unsubscribe_link_for_body,
         )
         created_at = notification.created_at
         key_type = notification.key_type
diff --git a/app/models.py b/app/models.py
index 1750bfe566..226b91304b 100644
--- a/app/models.py
+++ b/app/models.py
@@ -1751,15 +1751,22 @@ def _get_letter_cost(self):
 
         return letter_rate
 
+    def _generate_unsubscribe_link(self, base_url):
+        return url_with_token(
+            self.to,
+            url=f"/unsubscribe/{str(self.id)}/",
+            base_url=base_url,
+        )
+
     def get_unsubscribe_link_for_headers(self, *, template_has_unsubscribe_link):
         if self.unsubscribe_link:
             return self.unsubscribe_link
         if template_has_unsubscribe_link:
-            return url_with_token(
-                self.to,
-                url=f"/unsubscribe/{str(self.id)}/",
-                base_url=current_app.config["API_HOST_NAME"],
-            )
+            return self._generate_unsubscribe_link(current_app.config["API_HOST_NAME"])
+
+    def get_unsubscribe_link_for_body(self, *, template_has_unsubscribe_link):
+        if template_has_unsubscribe_link:
+            return self._generate_unsubscribe_link(current_app.config["ADMIN_BASE_URL"])
 
 
 class NotificationHistory(db.Model):
diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py
index 9220bee135..a4b8d1f452 100644
--- a/tests/app/delivery/test_send_to_providers.py
+++ b/tests/app/delivery/test_send_to_providers.py
@@ -844,16 +844,25 @@ def test_get_html_email_options_add_email_branding_from_service(sample_service):
 
 
 @pytest.mark.parametrize(
-    "template_has_unsubscribe_link",
+    "template_has_unsubscribe_link, expected_link_in_email_body",
     (
-        # If the notification has an unsubscribe link it shouldn’t matter what the
-        # setting on the template is
-        True,
-        False,
+        (True, "https://www.notify.example.com"),
+        (False, None),
     ),
 )
-def test_send_email_to_provider_sends_unsubscribe_link(sample_service, mocker, template_has_unsubscribe_link):
+def test_send_email_to_provider_sends_unsubscribe_link(
+    sample_service,
+    mocker,
+    template_has_unsubscribe_link,
+    expected_link_in_email_body,
+):
     mocker.patch("app.aws_ses_client.send_email", return_value="reference")
+    mock_html_email = mocker.patch("app.delivery.send_to_providers.HTMLEmailTemplate")
+    mock_plain_text_email = mocker.patch("app.delivery.send_to_providers.PlainTextEmailTemplate")
+    mocker.patch(
+        "app.models.url_with_token",
+        return_value="https://www.notify.example.com",
+    )
 
     template = create_template(
         service=sample_service,
@@ -874,13 +883,21 @@ def test_send_email_to_provider_sends_unsubscribe_link(sample_service, mocker, t
     app.aws_ses_client.send_email.assert_called_once()
     assert app.aws_ses_client.send_email.call_args[1]["headers"] == expected_headers
 
+    assert mock_html_email.call_args[1]["unsubscribe_link"] == expected_link_in_email_body
+    assert mock_plain_text_email.call_args[1]["unsubscribe_link"] == expected_link_in_email_body
+
 
 def test_send_email_to_provider_sends_unsubscribe_link_if_template_is_unsubscribable(sample_service, mocker):
     mocker.patch("app.aws_ses_client.send_email", return_value="reference")
     mock_url_with_token = mocker.patch(
         "app.models.url_with_token",
-        return_value="https://api.notify.example.com",
+        side_effect=[
+            "https://www.notify.example.com",
+            "https://api.notify.example.com",
+        ],
     )
+    mock_html_email = mocker.patch("app.delivery.send_to_providers.HTMLEmailTemplate")
+    mock_plain_text_email = mocker.patch("app.delivery.send_to_providers.PlainTextEmailTemplate")
 
     template = create_template(
         service=sample_service,
@@ -894,6 +911,7 @@ def test_send_email_to_provider_sends_unsubscribe_link_if_template_is_unsubscrib
     app.aws_ses_client.send_email.assert_called_once()
 
     assert mock_url_with_token.call_args_list == [
+        call("test@example.com", url=f"/unsubscribe/{db_notification.id}/", base_url="http://localhost:6012"),
         call("test@example.com", url=f"/unsubscribe/{db_notification.id}/", base_url="http://localhost:6011"),
     ]
 
@@ -901,3 +919,6 @@ def test_send_email_to_provider_sends_unsubscribe_link_if_template_is_unsubscrib
         {"Name": "List-Unsubscribe", "Value": "<https://api.notify.example.com>"},
         {"Name": "List-Unsubscribe-Post", "Value": "List-Unsubscribe=One-Click"},
     ]
+
+    assert mock_html_email.call_args[1]["unsubscribe_link"] == "https://www.notify.example.com"
+    assert mock_plain_text_email.call_args[1]["unsubscribe_link"] == "https://www.notify.example.com"

From a02cb303e1e0bab7680972f640b27f513b8902b5 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 2 Aug 2024 10:30:41 +0100
Subject: [PATCH 25/32] Add suggested comments and docstrings

---
 app/models.py                                        | 7 +++++++
 tests/app/notifications/test_process_notification.py | 1 +
 2 files changed, 8 insertions(+)

diff --git a/app/models.py b/app/models.py
index 226b91304b..28a4231872 100644
--- a/app/models.py
+++ b/app/models.py
@@ -1759,12 +1759,19 @@ def _generate_unsubscribe_link(self, base_url):
         )
 
     def get_unsubscribe_link_for_headers(self, *, template_has_unsubscribe_link):
+        """
+        Generates a URL on the API domain, which accepts a POST request from an email client
+        """
         if self.unsubscribe_link:
             return self.unsubscribe_link
         if template_has_unsubscribe_link:
             return self._generate_unsubscribe_link(current_app.config["API_HOST_NAME"])
 
     def get_unsubscribe_link_for_body(self, *, template_has_unsubscribe_link):
+        """
+        Generates a URL on the admin domain, which serves a page telling the user they
+        have been unsubscribed
+        """
         if template_has_unsubscribe_link:
             return self._generate_unsubscribe_link(current_app.config["ADMIN_BASE_URL"])
 
diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py
index dc2e9e0cfd..5fb4658f85 100644
--- a/tests/app/notifications/test_process_notification.py
+++ b/tests/app/notifications/test_process_notification.py
@@ -635,6 +635,7 @@ def test_persist_notification_when_template_has_unsubscribe_link_is_false(
     "unsubscribe_link, expected_unsubscribe_link",
     [
         ("https://please-unsubscribe-me.com/unsubscribe", "https://please-unsubscribe-me.com/unsubscribe"),
+        # We don’t persist template-level unsubscribe links – they are generated at time of sending
         (None, None),
     ],
 )

From b4b7eb87d1757ca2909ba450087153835d083a79 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 1 Aug 2024 16:09:18 +0100
Subject: [PATCH 26/32] Tweak start and end of unsubscribe request reports
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Originally we thought it would be nice and clean to extend the end dates
of the reports so there were no gaps between reports.

On second thought I think this will be confusing where the latest report
claims to run up to the current time. In this case:
- the report, when downloaded might not even contain requests from today
  so it could look like requests are missing from the report
- the report might claim to have requests up until now, but if any new
  ones come in between the user clicking on the page and generating the
  report then they wouldn’t be included

In could also be confusing when a report is cleaned up by data
retention. Adjacent reports would then appear to expand to fill the gap,
even though they gain no new requests.
---
 app/one_click_unsubscribe/rest.py | 20 +++-------
 app/service/rest.py               |  9 ++---
 tests/app/service/test_rest.py    | 66 ++++++++++++++-----------------
 3 files changed, 39 insertions(+), 56 deletions(-)

diff --git a/app/one_click_unsubscribe/rest.py b/app/one_click_unsubscribe/rest.py
index f9d8f43347..5482faf82d 100644
--- a/app/one_click_unsubscribe/rest.py
+++ b/app/one_click_unsubscribe/rest.py
@@ -1,5 +1,3 @@
-from datetime import datetime
-
 from flask import Blueprint, current_app, jsonify
 from itsdangerous import BadData
 from notifications_utils.url_safe_token import check_token
@@ -74,14 +72,8 @@ def create_unsubscribe_request_reports_summary(service_id):
         batched_unsubscribe_reports_summary = batched_unsubscribe_reports_summary
     # Check for unsubscribe requests and create a summary for them
     if unbatched_unsubscribe_requests := get_unbatched_unsubscribe_requests_dao(service_id):
-        if batched_unsubscribe_reports_summary:
-            earliest_timestamp = batched_unsubscribe_reports_summary[0]["latest_timestamp"]
-        else:
-
-            earliest_timestamp = unbatched_unsubscribe_requests[0].created_at
-
         unbatched_unsubscribe_report_summary.append(
-            _create_unbatched_unsubscribe_request_report_summary(unbatched_unsubscribe_requests, earliest_timestamp)
+            _create_unbatched_unsubscribe_request_report_summary(unbatched_unsubscribe_requests)
         )
     reports_summary = unbatched_unsubscribe_report_summary + batched_unsubscribe_reports_summary
 
@@ -94,9 +86,9 @@ def _create_batched_unsubscribe_request_reports_summary(unsubscribe_request_repo
         report_summary = {
             "batch_id": str(report.id),
             "count": report.count,
-            "earliest_timestamp": report.earliest_timestamp,
-            "latest_timestamp": report.latest_timestamp,
-            "processed_by_service_at": report.processed_by_service_at,
+            "earliest_timestamp": report.earliest_timestamp.isoformat(),
+            "latest_timestamp": report.latest_timestamp.isoformat(),
+            "processed_by_service_at": report.processed_by_service_at.isoformat(),
             "is_a_batched_report": True,
         }
         report_summaries.append(report_summary)
@@ -110,8 +102,8 @@ def _create_unbatched_unsubscribe_request_report_summary(
     report_summary = {
         "batch_id": None,
         "count": count,
-        "earliest_timestamp": earliest_timestamp,
-        "latest_timestamp": datetime.utcnow(),
+        "earliest_timestamp": unbatched_unsubscribe_requests[-1].created_at.isoformat(),
+        "latest_timestamp": unbatched_unsubscribe_requests[0].created_at.isoformat(),
         "processed_by_service_at": None,
         "is_a_batched_report": False,
     }
diff --git a/app/service/rest.py b/app/service/rest.py
index d3540978bd..3babbde234 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -1075,12 +1075,9 @@ def get_unsubscribe_request_reports_summary(service_id):
     This returns report summaries for both batched and un-batched unsubscribe requests.
 
     In the case of un-batched unsubscribe requests:
-    is_a_batched_result has a value of False.
-    The latest earliest_timestamp value is the date the user views the summary
-    The earliest_timestamp value is either:
-        i. the latest_timestamp of the last existing unsubscribe_request_report
-        or
-        ii. the date of the earliest unsubscribe request in the report.
+    - is_a_batched_result has a value of False.
+    - the latest_timestamp value is the date of the newest unsubscribe request in the report
+    - the earliest_timestamp value is the date of the oldest unsubscribe request in the report
 
     parameter: uuid service_id
 
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 7a02b6d36f..8c7907857e 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3520,7 +3520,7 @@ def test_get_unsubscribe_request_report_summary_for_initial_unsubscribe_requests
         template=template_2, created_at=(datetime.utcnow() + timedelta(days=-1)).strftime(date_format) + " GMT"
     )
     create_unsubscribe_request_dao(
-        {  # noqa
+        {
             "notification_id": notification_2.id,
             "template_id": notification_2.template_id,
             "template_version": notification_2.template_version,
@@ -3533,8 +3533,8 @@ def test_get_unsubscribe_request_report_summary_for_initial_unsubscribe_requests
     expected_unbatched_unsubscribe_request_summary = {
         "batch_id": None,
         "count": 2,
-        "earliest_timestamp": notification_2.created_at.strftime(date_format) + " GMT",
-        "latest_timestamp": (datetime.utcnow()).strftime(date_format) + " GMT",
+        "earliest_timestamp": notification_1.created_at.isoformat(),
+        "latest_timestamp": notification_2.created_at.isoformat(),
         "processed_by_service_at": None,
         "is_a_batched_report": False,
     }
@@ -3547,38 +3547,35 @@ def test_get_unsubscribe_request_report_summary_for_initial_unsubscribe_requests
 
 @freeze_time("2024-07-01 12:00")
 def test_get_unsubscribe_request_reports_summary(admin_request, sample_service, mocker):
-    date_format = "%a, %d %b %Y %H:%M:%S %Z"
     # Create 2 unbatched unsubscribe requests
     template_1 = create_template(
         sample_service,
         template_type=EMAIL_TYPE,
     )
     notification_1 = create_notification(template=template_1)
-    create_unsubscribe_request_dao(
-        {  # noqa
-            "notification_id": notification_1.id,
-            "template_id": notification_1.template_id,
-            "template_version": notification_1.template_version,
-            "service_id": notification_1.service_id,
-            "email_address": notification_1.to,
-            "created_at": (datetime.utcnow() + timedelta(days=-2)).strftime(date_format),
-        }
-    )
+    unbatched_request_1_data = {
+        "notification_id": notification_1.id,
+        "template_id": notification_1.template_id,
+        "template_version": notification_1.template_version,
+        "service_id": notification_1.service_id,
+        "email_address": notification_1.to,
+        "created_at": (datetime.utcnow() + timedelta(days=-2)),
+    }
+    create_unsubscribe_request_dao(unbatched_request_1_data)
     template_2 = create_template(
         sample_service,
         template_type=EMAIL_TYPE,
     )
     notification_2 = create_notification(template=template_2)
-    create_unsubscribe_request_dao(
-        {  # noqa
-            "notification_id": notification_2.id,
-            "template_id": notification_2.template_id,
-            "template_version": notification_2.template_version,
-            "service_id": notification_2.service_id,
-            "email_address": notification_2.to,
-            "created_at": datetime.utcnow() + timedelta(days=-1),
-        }
-    )
+    unbatched_request_2_data = {
+        "notification_id": notification_2.id,
+        "template_id": notification_2.template_id,
+        "template_version": notification_2.template_version,
+        "service_id": notification_2.service_id,
+        "email_address": notification_2.to,
+        "created_at": datetime.utcnow() + timedelta(days=-1),
+    }
+    create_unsubscribe_request_dao(unbatched_request_2_data)
 
     # Create 2 unsubscribe_request_reports
     unsubscribe_request_report_1 = UnsubscribeRequestReport(
@@ -3605,9 +3602,9 @@ def test_get_unsubscribe_request_reports_summary(admin_request, sample_service,
         {
             "batch_id": str(report.id),
             "count": report.count,
-            "earliest_timestamp": report.earliest_timestamp.strftime(date_format),
-            "latest_timestamp": report.latest_timestamp.strftime(date_format),
-            "processed_by_service_at": report.processed_by_service_at.strftime(date_format),
+            "earliest_timestamp": report.earliest_timestamp.isoformat(),
+            "latest_timestamp": report.latest_timestamp.isoformat(),
+            "processed_by_service_at": report.processed_by_service_at.isoformat(),
             "is_a_batched_report": True,
         }
         for report in [unsubscribe_request_report_2, unsubscribe_request_report_1]
@@ -3615,8 +3612,8 @@ def test_get_unsubscribe_request_reports_summary(admin_request, sample_service,
     expected_unbatched_unsubscribe_request_summary = {
         "batch_id": None,
         "count": 2,
-        "earliest_timestamp": unsubscribe_request_report_2.latest_timestamp.strftime(date_format),
-        "latest_timestamp": datetime.utcnow().strftime(date_format),
+        "earliest_timestamp": unbatched_request_1_data["created_at"].isoformat(),
+        "latest_timestamp": unbatched_request_2_data["created_at"].isoformat(),
         "processed_by_service_at": None,
         "is_a_batched_report": False,
     }
@@ -3626,20 +3623,17 @@ def test_get_unsubscribe_request_reports_summary(admin_request, sample_service,
     ] + expected_batched_unsubscribe_request_reports_summary
 
     response = admin_request.get("service.get_unsubscribe_request_reports_summary", service_id=sample_service.id)
-    assert datetime.strptime(response[0]["earliest_timestamp"], date_format) == datetime.strptime(
-        response[1]["latest_timestamp"], date_format
-    )
 
     for index, summary in enumerate(response):
         assert summary["batch_id"] == expected_reports_summary[index]["batch_id"]
         assert summary["count"] == expected_reports_summary[index]["count"]
-        assert summary["earliest_timestamp"] == expected_reports_summary[index]["earliest_timestamp"] + "GMT"
-        assert summary["latest_timestamp"] == expected_reports_summary[index]["latest_timestamp"] + "GMT"
+        assert summary["earliest_timestamp"] == expected_reports_summary[index]["earliest_timestamp"]
+        assert summary["latest_timestamp"] == expected_reports_summary[index]["latest_timestamp"]
         assert summary["is_a_batched_report"] == expected_reports_summary[index]["is_a_batched_report"]
 
     assert response[0]["processed_by_service_at"] == expected_reports_summary[0]["processed_by_service_at"]
-    assert response[1]["processed_by_service_at"] == expected_reports_summary[1]["processed_by_service_at"] + "GMT"
-    assert response[2]["processed_by_service_at"] == expected_reports_summary[2]["processed_by_service_at"] + "GMT"
+    assert response[1]["processed_by_service_at"] == expected_reports_summary[1]["processed_by_service_at"]
+    assert response[2]["processed_by_service_at"] == expected_reports_summary[2]["processed_by_service_at"]
 
 
 def test_get_unsubscribe_requests_statistics(admin_request, sample_service, mocker):

From d3eabdb636a6a028e3b56654bb955d118ccd658f Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 5 Aug 2024 09:56:33 +0100
Subject: [PATCH 27/32] Refactor serialisation into model layer

The pattern we tend to use for turning a database object into a `dict`
is to add a `serialize` method to the model class
---
 app/models.py                     | 21 ++++++++++++++++++++
 app/one_click_unsubscribe/rest.py | 33 ++++++-------------------------
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/app/models.py b/app/models.py
index e68ae597f5..abf3aee2dc 100644
--- a/app/models.py
+++ b/app/models.py
@@ -2676,6 +2676,27 @@ class UnsubscribeRequestReport(db.Model):
     processed_by_service_at = db.Column(db.DateTime, nullable=True)
     count = db.Column(db.BigInteger, nullable=False)
 
+    def serialize(self):
+        return {
+            "batch_id": str(self.id),
+            "count": self.count,
+            "earliest_timestamp": self.earliest_timestamp.isoformat(),
+            "latest_timestamp": self.latest_timestamp.isoformat(),
+            "processed_by_service_at": self.processed_by_service_at.isoformat(),
+            "is_a_batched_report": True,
+        }
+
+    @staticmethod
+    def serialize_unbatched_requests(self, unbatched_unsubscribe_requests):
+        return {
+            "batch_id": None,
+            "count": len(unbatched_unsubscribe_requests),
+            "earliest_timestamp": unbatched_unsubscribe_requests[-1].created_at.isoformat(),
+            "latest_timestamp": unbatched_unsubscribe_requests[0].created_at.isoformat(),
+            "processed_by_service_at": None,
+            "is_a_batched_report": False,
+        }
+
 
 class UnsubscribeRequest(db.Model):
     __tablename__ = "unsubscribe_request"
diff --git a/app/one_click_unsubscribe/rest.py b/app/one_click_unsubscribe/rest.py
index 5482faf82d..38576be042 100644
--- a/app/one_click_unsubscribe/rest.py
+++ b/app/one_click_unsubscribe/rest.py
@@ -11,6 +11,7 @@
     get_unsubscribe_request_reports_dao,
 )
 from app.errors import InvalidRequest, register_errors
+from app.models import UnsubscribeRequestReport
 
 one_click_unsubscribe_blueprint = Blueprint("one_click_unsubscribe", __name__)
 
@@ -81,30 +82,8 @@ def create_unsubscribe_request_reports_summary(service_id):
 
 
 def _create_batched_unsubscribe_request_reports_summary(unsubscribe_request_reports: list) -> list:
-    report_summaries = []
-    for report in unsubscribe_request_reports:
-        report_summary = {
-            "batch_id": str(report.id),
-            "count": report.count,
-            "earliest_timestamp": report.earliest_timestamp.isoformat(),
-            "latest_timestamp": report.latest_timestamp.isoformat(),
-            "processed_by_service_at": report.processed_by_service_at.isoformat(),
-            "is_a_batched_report": True,
-        }
-        report_summaries.append(report_summary)
-    return report_summaries
-
-
-def _create_unbatched_unsubscribe_request_report_summary(
-    unbatched_unsubscribe_requests: list, earliest_timestamp: str = None
-) -> dict:
-    count = len(unbatched_unsubscribe_requests)
-    report_summary = {
-        "batch_id": None,
-        "count": count,
-        "earliest_timestamp": unbatched_unsubscribe_requests[-1].created_at.isoformat(),
-        "latest_timestamp": unbatched_unsubscribe_requests[0].created_at.isoformat(),
-        "processed_by_service_at": None,
-        "is_a_batched_report": False,
-    }
-    return report_summary
+    return [report.serialize() for report in unsubscribe_request_reports]
+
+
+def _create_unbatched_unsubscribe_request_report_summary(unbatched_unsubscribe_requests: list) -> dict:
+    return UnsubscribeRequestReport.serialize_unbatched_requests(unbatched_unsubscribe_requests)

From 304511010be004a2559c202e8357accbe5989fe0 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 5 Aug 2024 10:01:54 +0100
Subject: [PATCH 28/32] Refactor to reduce assignments
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Building up lists by appending isn’t very Pythonic. Leaning on list
comprehesion is genrally preferred.
---
 app/models.py                     |  2 +-
 app/one_click_unsubscribe/rest.py | 29 ++++++-----------------------
 app/service/rest.py               |  3 +--
 3 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/app/models.py b/app/models.py
index abf3aee2dc..98eebba7e2 100644
--- a/app/models.py
+++ b/app/models.py
@@ -2687,7 +2687,7 @@ def serialize(self):
         }
 
     @staticmethod
-    def serialize_unbatched_requests(self, unbatched_unsubscribe_requests):
+    def serialize_unbatched_requests(unbatched_unsubscribe_requests):
         return {
             "batch_id": None,
             "count": len(unbatched_unsubscribe_requests),
diff --git a/app/one_click_unsubscribe/rest.py b/app/one_click_unsubscribe/rest.py
index 38576be042..7cbf4e5b4d 100644
--- a/app/one_click_unsubscribe/rest.py
+++ b/app/one_click_unsubscribe/rest.py
@@ -62,28 +62,11 @@ def get_unsubscribe_request_data(notification, email_address):
 
 
 def create_unsubscribe_request_reports_summary(service_id):
-    reports_summary = []
-    batched_unsubscribe_reports_summary = []
-    unbatched_unsubscribe_report_summary = []
-    # Check for existing unsubscribe reports and create their summaries
-    if unsubscribe_request_reports := get_unsubscribe_request_reports_dao(service_id):
-        batched_unsubscribe_reports_summary = _create_batched_unsubscribe_request_reports_summary(
-            unsubscribe_request_reports,
-        )
-        batched_unsubscribe_reports_summary = batched_unsubscribe_reports_summary
-    # Check for unsubscribe requests and create a summary for them
-    if unbatched_unsubscribe_requests := get_unbatched_unsubscribe_requests_dao(service_id):
-        unbatched_unsubscribe_report_summary.append(
-            _create_unbatched_unsubscribe_request_report_summary(unbatched_unsubscribe_requests)
-        )
-    reports_summary = unbatched_unsubscribe_report_summary + batched_unsubscribe_reports_summary
-
-    return reports_summary
-
-
-def _create_batched_unsubscribe_request_reports_summary(unsubscribe_request_reports: list) -> list:
-    return [report.serialize() for report in unsubscribe_request_reports]
+    unsubscribe_request_reports = [report.serialize() for report in get_unsubscribe_request_reports_dao(service_id)]
 
+    if unbatched_unsubscribe_requests := get_unbatched_unsubscribe_requests_dao(service_id):
+        return [
+            UnsubscribeRequestReport.serialize_unbatched_requests(unbatched_unsubscribe_requests)
+        ] + unsubscribe_request_reports
 
-def _create_unbatched_unsubscribe_request_report_summary(unbatched_unsubscribe_requests: list) -> dict:
-    return UnsubscribeRequestReport.serialize_unbatched_requests(unbatched_unsubscribe_requests)
+    return unsubscribe_request_reports
diff --git a/app/service/rest.py b/app/service/rest.py
index 3babbde234..f23d97b62b 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -1084,8 +1084,7 @@ def get_unsubscribe_request_reports_summary(service_id):
     return: reports_summary = []
 
     """
-    reports_summary = create_unsubscribe_request_reports_summary(service_id)
-    return jsonify(reports_summary)
+    return jsonify(create_unsubscribe_request_reports_summary(service_id))
 
 
 @service_blueprint.route("/<uuid:service_id>/unsubscribe-request-statistics", methods=["GET"])

From 76c4ff4d7d108d35bf0c88a291871323e1945423 Mon Sep 17 00:00:00 2001
From: Chukwugozie Mbeledogu
 <chukwugozie.mbeledogu@digital.cabinet-office.gov.uk>
Date: Mon, 5 Aug 2024 12:33:32 +0100
Subject: [PATCH 29/32] streamline
 assign_unbatched_unsubscribe_requests_to_report_dao to be more efficient

---
 app/dao/unsubscribe_request_dao.py | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py
index 7264c99734..06fbe5f9b7 100644
--- a/app/dao/unsubscribe_request_dao.py
+++ b/app/dao/unsubscribe_request_dao.py
@@ -110,15 +110,12 @@ def update_unsubscribe_request_report_processed_by_date_dao(report, report_has_b
 @autocommit
 def assign_unbatched_unsubscribe_requests_to_report_dao(report_id, service_id, earliest_timestamp, latest_timestamp):
     """
-    This method retrieves all the un-batched unsubscribe requests received before or on
-    the report's latest_timestamp and sets their unsubscribe_request_report_id to the report_i
+    This method updates the unsubscribe_request_report_id of all un-batched unsubscribe requests that fall
+    within the earliest_timestamp and latest_timestamp values to report_id
     """
-    unbatched_unsubscribe_requests = UnsubscribeRequest.query.filter(
+    UnsubscribeRequest.query.filter(
         UnsubscribeRequest.unsubscribe_request_report_id.is_(None),
         UnsubscribeRequest.service_id == service_id,
         UnsubscribeRequest.created_at >= earliest_timestamp,
         UnsubscribeRequest.created_at <= latest_timestamp,
-    ).all()
-    for unsubscribe_request in unbatched_unsubscribe_requests:
-        unsubscribe_request.unsubscribe_request_report_id = report_id
-        db.session.add(unsubscribe_request)
+    ).update({"unsubscribe_request_report_id": report_id})

From 6308f69eaf45c6b00cb2a72c0379ffa5ea1466de Mon Sep 17 00:00:00 2001
From: Katie Smith <klssmith@users.noreply.github.com>
Date: Mon, 5 Aug 2024 14:01:17 +0100
Subject: [PATCH 30/32] Add log line for letters that failed validation due to
 no fixed abode

This adds a log line for precompiled API letters that were sent to addresses
which were no fixed abode addresses. This will allow us to look up the
address of the notification and check for false positives.

For templated letters sent via the API, we can already see from the logs if
any failed validation due to having a no fixed abode address. We won't
be able to see the address that was used, but can check which services are
sending them.
---
 app/celery/letters_pdf_tasks.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py
index 10f541564d..d9534a4ae2 100644
--- a/app/celery/letters_pdf_tasks.py
+++ b/app/celery/letters_pdf_tasks.py
@@ -303,6 +303,12 @@ def process_sanitised_letter(self, sanitise_data):
                 "Processing invalid precompiled pdf with id %s (file %s)", notification_id, filename
             )
 
+            # Log letters that fail with no fixed abode error so we can check for false positives
+            if letter_details["message"] == "no-fixed-abode-address":
+                current_app.logger.info(
+                    "Precomiled PDF with id %s was invalid due to no fixed abode address", notification_id
+                )
+
             _move_invalid_letter_and_update_status(
                 notification=notification,
                 filename=filename,

From 3f8c50fdd2a184b56a9c25703fbc7e0d5988731c Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 5 Aug 2024 12:12:04 +0100
Subject: [PATCH 31/32] Account for reports which have not been processed

Serialisation needs to be able to handle reports with a
`processed_by_service` value of `None`
---
 app/models.py                  | 4 +++-
 tests/app/service/test_rest.py | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/app/models.py b/app/models.py
index 98eebba7e2..ed321d9238 100644
--- a/app/models.py
+++ b/app/models.py
@@ -2682,7 +2682,9 @@ def serialize(self):
             "count": self.count,
             "earliest_timestamp": self.earliest_timestamp.isoformat(),
             "latest_timestamp": self.latest_timestamp.isoformat(),
-            "processed_by_service_at": self.processed_by_service_at.isoformat(),
+            "processed_by_service_at": (
+                self.processed_by_service_at.isoformat() if self.processed_by_service_at else None
+            ),
             "is_a_batched_report": True,
         }
 
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 8c7907857e..3b95c414f1 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -3583,7 +3583,7 @@ def test_get_unsubscribe_request_reports_summary(admin_request, sample_service,
         count=141,
         earliest_timestamp=datetime.utcnow() + timedelta(days=-8),
         latest_timestamp=datetime.utcnow() + timedelta(days=-7),
-        processed_by_service_at=datetime.utcnow() + timedelta(days=-6),
+        processed_by_service_at=None,
         service_id=sample_service.id,
     )
     create_unsubscribe_request_reports_dao(unsubscribe_request_report_1)
@@ -3604,7 +3604,9 @@ def test_get_unsubscribe_request_reports_summary(admin_request, sample_service,
             "count": report.count,
             "earliest_timestamp": report.earliest_timestamp.isoformat(),
             "latest_timestamp": report.latest_timestamp.isoformat(),
-            "processed_by_service_at": report.processed_by_service_at.isoformat(),
+            "processed_by_service_at": (
+                report.processed_by_service_at.isoformat() if report.processed_by_service_at else None
+            ),
             "is_a_batched_report": True,
         }
         for report in [unsubscribe_request_report_2, unsubscribe_request_report_1]

From 20fd53e8fbf7a640a96c99851b746cb1b7503526 Mon Sep 17 00:00:00 2001
From: Richard Parke <richard.parke@digital.cabinet-office.gov.uk>
Date: Sat, 27 Jul 2024 13:58:55 +0100
Subject: [PATCH 32/32] Makes a change to app/notifications/validators.py so
 that services with "sms_to_uk_landlines" permission use new validation logic

We made this change because services with the "sms_to_uk_landlines" permission were failing when trying to send SMS notifications to UK landlines, because they were hitting the old validation code inadvertantly.

This makes a change to app/notifications/validators.py ensuring that services with this permission hit the right validation functions
---
 app/notifications/validators.py               | 16 +++-
 requirements.in                               |  2 +-
 requirements.txt                              |  2 +-
 .../notifications/test_post_notifications.py  | 89 +++++++++++++++++--
 4 files changed, 98 insertions(+), 11 deletions(-)

diff --git a/app/notifications/validators.py b/app/notifications/validators.py
index 12e1ffaae4..9fe38f7b86 100644
--- a/app/notifications/validators.py
+++ b/app/notifications/validators.py
@@ -6,7 +6,9 @@
     rate_limit_cache_key,
 )
 from notifications_utils.recipient_validation.email_address import validate_and_format_email_address
+from notifications_utils.recipient_validation.errors import InvalidPhoneError
 from notifications_utils.recipient_validation.phone_number import (
+    PhoneNumber,
     get_international_phone_info,
     validate_and_format_phone_number,
 )
@@ -21,6 +23,7 @@
     KEY_TYPE_TEAM,
     KEY_TYPE_TEST,
     LETTER_TYPE,
+    SMS_TO_UK_LANDLINES,
     SMS_TYPE,
 )
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
@@ -138,7 +141,18 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type,
     service_can_send_to_recipient(send_to, key_type, service, allow_guest_list_recipients)
 
     if notification_type == SMS_TYPE:
-        international_phone_info = check_if_service_can_send_to_number(service, send_to)
+        if service.has_permission(SMS_TO_UK_LANDLINES):
+            try:
+                phone_number = PhoneNumber(send_to, allow_international=service.has_permission(INTERNATIONAL_SMS_TYPE))
+                return phone_number.get_normalised_format()
+            except InvalidPhoneError as e:
+                if e.code == InvalidPhoneError.Codes.NOT_A_UK_MOBILE:
+                    raise BadRequestError(message="Cannot send to international mobile numbers") from e
+                else:
+                    raise
+
+        else:
+            international_phone_info = check_if_service_can_send_to_number(service, send_to)
 
         return validate_and_format_phone_number(number=send_to, international=international_phone_info.international)
     elif notification_type == EMAIL_TYPE:
diff --git a/requirements.in b/requirements.in
index 31ea0b54d7..9cff0c2c6d 100644
--- a/requirements.in
+++ b/requirements.in
@@ -24,7 +24,7 @@ lxml==4.9.3
 notifications-python-client==8.0.1
 
 # Run `make bump-utils` to update to the latest version
-notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.2.0
+notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.2.1
 
 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
 prometheus-client==0.14.1
diff --git a/requirements.txt b/requirements.txt
index 48ccaae16c..1072fc1f22 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -151,7 +151,7 @@ mistune==0.8.4
     # via notifications-utils
 notifications-python-client==8.0.1
     # via -r requirements.in
-notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.2.0
+notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.2.1
     # via -r requirements.in
 ordered-set==4.1.0
     # via notifications-utils
diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py
index 13caf43f24..a1a0506f53 100644
--- a/tests/app/v2/notifications/test_post_notifications.py
+++ b/tests/app/v2/notifications/test_post_notifications.py
@@ -9,6 +9,7 @@
     INTERNATIONAL_SMS_TYPE,
     LETTER_TYPE,
     NOTIFICATION_CREATED,
+    SMS_TO_UK_LANDLINES,
     SMS_TYPE,
 )
 from app.dao import templates_dao
@@ -765,19 +766,26 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(
     assert not deliver_mock.called
 
 
+@pytest.mark.parametrize(
+    "permissions",
+    [
+        [SMS_TYPE],
+        [SMS_TYPE, SMS_TO_UK_LANDLINES],
+    ],
+)
 def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(
     api_client_request,
     notify_db_session,
+    permissions,
 ):
-    service = create_service(service_permissions=[SMS_TYPE])
+    service = create_service(service_permissions=permissions)
     template = create_template(service=service)
 
-    data = {"phone_number": "20-12-1234-1234", "template_id": template.id}
+    data = {"phone_number": "+14158961600", "template_id": template.id}
 
     error_json = api_client_request.post(
         service.id, "v2_notifications.post_notification", notification_type="sms", _data=data, _expected_status=400
     )
-
     assert error_json["status_code"] == 400
     assert error_json["errors"] == [
         {"error": "BadRequestError", "message": "Cannot send to international mobile numbers"}
@@ -859,24 +867,89 @@ def test_post_sms_notification_returns_400_if_number_not_in_guest_list(
     ]
 
 
+@pytest.mark.parametrize(
+    "permissions",
+    [
+        [SMS_TYPE, INTERNATIONAL_SMS_TYPE],
+        [SMS_TYPE, SMS_TO_UK_LANDLINES, INTERNATIONAL_SMS_TYPE],
+    ],
+)
 def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(
-    sample_service,
-    sample_template,
+    notify_db_session,
     api_client_request,
     mocker,
+    permissions,
 ):
     mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")
 
-    data = {"phone_number": "20-12-1234-1234", "template_id": sample_template.id}
-
+    service = create_service(service_permissions=permissions)
+    template = create_template(service=service)
+    data = {"phone_number": "20-12-1234-1234", "template_id": template.id}
     api_client_request.post(
-        sample_service.id,
+        service.id,
         "v2_notifications.post_notification",
         notification_type="sms",
         _data=data,
     )
 
 
+@pytest.mark.parametrize(
+    "permissions",
+    [
+        [SMS_TYPE, SMS_TO_UK_LANDLINES],
+        [SMS_TYPE, SMS_TO_UK_LANDLINES, INTERNATIONAL_SMS_TYPE],
+    ],
+)
+def test_post_sms_notification_returns_201_if_allowed_to_send_to_uk_landlines(
+    notify_db_session,
+    api_client_request,
+    mocker,
+    permissions,
+):
+    mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")
+
+    service = create_service(service_permissions=permissions)
+    template = create_template(service=service)
+    data = {"phone_number": "01709510122", "template_id": template.id}
+    resp_json = api_client_request.post(
+        service.id,
+        "v2_notifications.post_notification",
+        notification_type="sms",
+        _data=data,
+    )
+    notifications = Notification.query.all()
+    assert len(notifications) == 1
+    notification_id = notifications[0].id
+    assert "01709510122" == notifications[0].to
+    assert resp_json["id"] == str(notification_id)
+
+
+@pytest.mark.parametrize(
+    "permissions",
+    [
+        [SMS_TYPE],
+        [SMS_TYPE, INTERNATIONAL_SMS_TYPE],
+    ],
+)
+def test_post_sms_notification_returns_400_if_not_allowed_to_send_to_uk_landlines(
+    notify_db_session,
+    api_client_request,
+    mocker,
+    permissions,
+):
+    mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")
+
+    service = create_service(service_permissions=permissions)
+    template = create_template(service=service)
+    data = {"phone_number": "01709510122", "template_id": template.id}
+    error_json = api_client_request.post(
+        service.id, "v2_notifications.post_notification", notification_type="sms", _data=data, _expected_status=400
+    )
+
+    assert error_json["status_code"] == 400
+    assert error_json["errors"] == [{"error": "InvalidPhoneError", "message": "Not a UK mobile number"}]
+
+
 def test_post_sms_should_persist_supplied_sms_number(api_client_request, sample_template_with_placeholders, mocker):
     mocked = mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")
     data = {