Merge pull request #4304 from alphagov/revert-4299-revert-4298-handli…

…ng_duplicate_unsubscribe_requests

Prevent duplicate unsubscribe requests

app/dao/unsubscribe_request_dao.py

@@ -22,7 +22,7 @@ def create_unsubscribe_request_dao(unsubscribe_data):
 
 
 def get_unsubscribe_request_by_notification_id_dao(notification_id):
-    return UnsubscribeRequest.query.filter_by(notification_id=notification_id).one()
+    return UnsubscribeRequest.query.filter_by(notification_id=notification_id).one_or_none()
 
 
 def get_unsubscribe_requests_statistics_dao(service_id):

app/one_click_unsubscribe/rest.py

@@ -8,6 +8,8 @@
 from app.dao.unsubscribe_request_dao import (
     create_unsubscribe_request_dao,
     get_unbatched_unsubscribe_requests_dao,
+    get_unsubscribe_request_by_notification_id_dao,
+    get_unsubscribe_request_report_by_id_dao,
     get_unsubscribe_request_reports_dao,
 )
 from app.errors import InvalidRequest, register_errors
@@ -30,6 +32,10 @@ def one_click_unsubscribe(notification_id, token):
         errors = {"unsubscribe request": "This is not a valid unsubscribe link."}
         raise InvalidRequest(errors, status_code=404) from e
 
+    # Check if the unsubscribe request is a duplicate
+    if is_duplicate_unsubscribe_request(notification_id):
+        return jsonify(result="success", message="Unsubscribe successful"), 200
+
     if notification := get_notification_by_id(notification_id):
         unsubscribe_data = get_unsubscribe_request_data(notification, email_address)
 
@@ -69,3 +75,22 @@ def create_unsubscribe_request_reports_summary(service_id):
             UnsubscribeRequestReport.serialize_unbatched_requests(unbatched_unsubscribe_requests)
         ] + unsubscribe_request_reports
     return unsubscribe_request_reports
+
+
+def is_duplicate_unsubscribe_request(notification_id):
+    """
+    A duplicate unsubscribe request is being defined as an unsubscribe_request that has
+    the same notification_id of a previously received unsubscribe request that has not yet been processed
+    by the service that initiated the notification.
+    """
+    unsubscribe_request = get_unsubscribe_request_by_notification_id_dao(notification_id)
+
+    if not unsubscribe_request:
+        return False
+
+    report_id = unsubscribe_request.unsubscribe_request_report_id
+
+    if report_id and get_unsubscribe_request_report_by_id_dao(report_id).processed_by_service_at:
+        return False
+
+    return True

tests/app/db.py

@@ -80,6 +80,7 @@
     User,
     WebauthnCredential,
 )
+from app.utils import midnight_n_days_ago
 
 
 def create_user(*, mobile_number="+447700900986", email=None, state="active", id_=None, name="Test User", **kwargs):
@@ -1322,3 +1323,38 @@ def create_unsubscribe_request_report(
     )
     create_unsubscribe_request_reports_dao(report)
     return report
+
+
+def create_unsubscribe_request_and_return_the_notification_id(
+    sample_service, sample_template, is_batched, processed_by_service
+):
+    notification = create_notification(
+        template=sample_template,
+        to_field="[email protected]",
+        sent_at=datetime.now() - timedelta(days=4),
+    )
+
+    unsubscribe_request_report_id = None
+    if is_batched:
+        # Create processed unsubscribe request report
+        unsubscribe_request_report = create_unsubscribe_request_report(
+            sample_service,
+            earliest_timestamp=midnight_n_days_ago(4),
+            latest_timestamp=midnight_n_days_ago(2),
+            processed_by_service_at=midnight_n_days_ago(1) if processed_by_service else None,
+        )
+        unsubscribe_request_report_id = unsubscribe_request_report.id
+
+    # Create an unsubscribe request
+    create_unsubscribe_request_dao(
+        {
+            "notification_id": notification.id,
+            "template_id": notification.template_id,
+            "template_version": notification.template_version,
+            "service_id": sample_service.id,
+            "email_address": notification.to,
+            "created_at": datetime.now(),
+            "unsubscribe_request_report_id": unsubscribe_request_report_id,
+        }
+    )
+    return notification.id

tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py

@@ -1,13 +1,16 @@
 import uuid
 from unittest.mock import call
 
+import pytest
 from flask import current_app
 from notifications_utils.url_safe_token import generate_token
 
 from app.constants import EMAIL_TYPE
 from app.dao.templates_dao import dao_update_template
 from app.dao.unsubscribe_request_dao import get_unsubscribe_request_by_notification_id_dao
-from tests.app.db import create_notification, create_template
+from app.models import UnsubscribeRequest
+from app.one_click_unsubscribe.rest import is_duplicate_unsubscribe_request
+from tests.app.db import create_notification, create_template, create_unsubscribe_request_and_return_the_notification_id
 
 
 def unsubscribe_url_post(client, notification_id, token):
@@ -36,6 +39,21 @@ def test_valid_one_click_unsubscribe_url(mocker, client, sample_email_notificati
     ]
 
 
+def test_duplicate_unsubscribe_requests(mocker, client, sample_email_notification):
+    token = generate_token(
+        sample_email_notification.to, current_app.config["SECRET_KEY"], current_app.config["DANGEROUS_SALT"]
+    )
+    # first unsubscribe request
+    unsubscribe_url_post(client, sample_email_notification.id, token)
+    # duplicate unsubscribe request
+    unsubscribe_url_post(client, sample_email_notification.id, token)
+
+    result = UnsubscribeRequest.query.filter_by(notification_id=sample_email_notification.id).all()
+
+    # The duplicate unsubscribe request shouldn't be added to the unsubscribe_request table
+    assert len(result) == 1
+
+
 def test_unsubscribe_request_object_refers_to_correct_template_version_after_template_updated(client, sample_service):
     test_template = create_template(
         sample_service,
@@ -119,3 +137,26 @@ def test_invalid_one_click_unsubscribe_url_notification_id(client, sample_email_
     response_json_data = response.get_json()
     assert response.status_code == 404
     assert response_json_data["message"] == {"unsubscribe request": "This is not a valid unsubscribe link."}
+
+
+@pytest.mark.parametrize(
+    "create_previous_unsubscribe_request,is_batched, processed_by_service, expected_result",
+    [(False, False, False, False), (True, True, True, False), (True, False, False, True), (True, True, False, True)],
+)
+def test_is_duplicate_unsubscribe_request(
+    sample_service,
+    sample_email_template,
+    create_previous_unsubscribe_request,
+    is_batched,
+    processed_by_service,
+    expected_result,
+):
+    if create_previous_unsubscribe_request:
+        notification_id = create_unsubscribe_request_and_return_the_notification_id(
+            sample_service, sample_email_template, is_batched, processed_by_service
+        )
+    else:
+        notification_id = uuid.uuid4()
+
+    result = is_duplicate_unsubscribe_request(notification_id)
+    assert result == expected_result