From 13919b0445e71e6b62dc81b4f16c7be64e033e36 Mon Sep 17 00:00:00 2001 From: BlessedDev <32799090+BlessedDev@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:49:38 +0000 Subject: [PATCH 1/2] Redeploy changes to prevent duplicate unsubscribe requests The earlier changes to prevent duplicate unsubscribe requests were reverted in order to update the functional tests for the unsubscribe request journey. This is a redeployment of those changes. --- app/dao/unsubscribe_request_dao.py | 2 +- app/one_click_unsubscribe/rest.py | 25 +++++++++++ tests/app/db.py | 36 ++++++++++++++++ .../test_one_click_unsubscribe.py | 43 ++++++++++++++++++- 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/app/dao/unsubscribe_request_dao.py b/app/dao/unsubscribe_request_dao.py index 0a03ffacef..43ef48be64 100644 --- a/app/dao/unsubscribe_request_dao.py +++ b/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): diff --git a/app/one_click_unsubscribe/rest.py b/app/one_click_unsubscribe/rest.py index 4beaa4525e..b3664e20d5 100644 --- a/app/one_click_unsubscribe/rest.py +++ b/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 diff --git a/tests/app/db.py b/tests/app/db.py index 74f2ca0f81..fa719014b9 100644 --- a/tests/app/db.py +++ b/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="example@example.com", + 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 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 0d765dd9c8..cf6b8097ff 100644 --- a/tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py +++ b/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_requets(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 From 151a64546011050e316dd78b92277c17ad1287d6 Mon Sep 17 00:00:00 2001 From: Chukwugozie Mbeledogu Date: Thu, 19 Dec 2024 10:53:08 +0000 Subject: [PATCH 2/2] fix typo in duplicate unsubscribe requests test name --- tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 cf6b8097ff..d42973e0bf 100644 --- a/tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py +++ b/tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py @@ -39,7 +39,7 @@ def test_valid_one_click_unsubscribe_url(mocker, client, sample_email_notificati ] -def test_duplicate_unsubscribe_requets(mocker, client, sample_email_notification): +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"] )