From 12a54840033bef31d89c81563576aa785dadedde Mon Sep 17 00:00:00 2001 From: kitos Date: Fri, 3 Jul 2026 10:48:42 +0200 Subject: [PATCH] fix(campaigns): preserve modification-request audit row after test deletion Approving a remove_test modification request deletes the underlying Test row, which was cascading through test_id's ON DELETE CASCADE and wiping out the request row itself before it could be read back. Changed to ON DELETE SET NULL so the audit record (justification, reviewer, decision) survives. Adds regression coverage plus double-approve/reject idempotency tests. --- .../b056_mod_request_test_id_set_null.py | 50 +++++++++++++++++++ backend/app/models/campaign.py | 8 ++- backend/tests/test_campaign_approval.py | 47 ++++++++++++++++- 3 files changed, 101 insertions(+), 4 deletions(-) create mode 100644 backend/alembic/versions/b056_mod_request_test_id_set_null.py diff --git a/backend/alembic/versions/b056_mod_request_test_id_set_null.py b/backend/alembic/versions/b056_mod_request_test_id_set_null.py new file mode 100644 index 0000000..1ed76c2 --- /dev/null +++ b/backend/alembic/versions/b056_mod_request_test_id_set_null.py @@ -0,0 +1,50 @@ +"""Change campaign_modification_requests.test_id to nullable with ON DELETE SET NULL. + +Approving a "remove_test" modification request deletes the underlying Test +row. With the original ON DELETE CASCADE, that delete cascaded and wiped +out the modification-request row itself, destroying the audit record +(justification, reviewer, decision) the request exists to preserve. + +Revision ID: b056 +Revises: b055 +Create Date: 2026-07-03 +""" +from alembic import op +import sqlalchemy as sa + +revision = "b056" +down_revision = "b055" +branch_labels = None +depends_on = None + +_FK_NAME = "campaign_modification_requests_test_id_fkey" + + +def upgrade() -> None: + op.alter_column( + "campaign_modification_requests", "test_id", + existing_type=sa.dialects.postgresql.UUID(as_uuid=True), + nullable=True, + ) + op.drop_constraint(_FK_NAME, "campaign_modification_requests", type_="foreignkey") + op.create_foreign_key( + _FK_NAME, + "campaign_modification_requests", "tests", + ["test_id"], ["id"], + ondelete="SET NULL", + ) + + +def downgrade() -> None: + op.drop_constraint(_FK_NAME, "campaign_modification_requests", type_="foreignkey") + op.create_foreign_key( + _FK_NAME, + "campaign_modification_requests", "tests", + ["test_id"], ["id"], + ondelete="CASCADE", + ) + op.alter_column( + "campaign_modification_requests", "test_id", + existing_type=sa.dialects.postgresql.UUID(as_uuid=True), + nullable=False, + ) diff --git a/backend/app/models/campaign.py b/backend/app/models/campaign.py index ae06722..f033494 100644 --- a/backend/app/models/campaign.py +++ b/backend/app/models/campaign.py @@ -259,10 +259,14 @@ class CampaignModificationRequest(Base): nullable=True, ) action = Column(String, nullable=False) # add_test, remove_test + # SET NULL (not CASCADE): approving a "remove_test" request deletes the + # underlying Test row (see remove_test_from_campaign), and this request + # row is the audit record of that decision — it must survive the test's + # deletion, not be wiped out along with it. test_id = Column( UUID(as_uuid=True), - ForeignKey("tests.id", ondelete="CASCADE"), - nullable=False, + ForeignKey("tests.id", ondelete="SET NULL"), + nullable=True, ) order_index = Column(Integer, nullable=True) phase = Column(String, nullable=True) diff --git a/backend/tests/test_campaign_approval.py b/backend/tests/test_campaign_approval.py index c128382..0165543 100644 --- a/backend/tests/test_campaign_approval.py +++ b/backend/tests/test_campaign_approval.py @@ -4,7 +4,7 @@ import uuid import pytest -from app.models.campaign import Campaign, CampaignTest +from app.models.campaign import Campaign, CampaignModificationRequest, CampaignTest from app.models.technique import Technique from app.models.test import Test from app.models.enums import TestState @@ -275,12 +275,41 @@ class TestModificationRequests: test_id=str(ct.test_id), justification="Test superseded", ) db.commit() + request_id = request.id - approve_modification_request(db, str(request.id), reviewer_id=admin_user.id) + approve_modification_request(db, str(request_id), reviewer_id=admin_user.id) cts = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).all() assert len(cts) == 0 + # The request row must survive the cascade-delete of the underlying Test + # (ondelete="CASCADE" on CampaignModificationRequest.test_id) — this locks + # in the fix where the request's status is persisted BEFORE the test is + # removed, not after. + reloaded = ( + db.query(CampaignModificationRequest) + .filter(CampaignModificationRequest.id == request_id) + .first() + ) + assert reloaded is not None + assert reloaded.status == "approved" + assert reloaded.reviewed_by == admin_user.id + assert reloaded.reviewed_at is not None + + def test_approve_already_decided_request_raises(self, db, active_campaign_with_test, admin_user): + ct = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).first() + request = create_modification_request( + db, str(active_campaign_with_test.id), + requester_id=admin_user.id, action="remove_test", + test_id=str(ct.test_id), justification="Test superseded", + ) + db.commit() + approve_modification_request(db, str(request.id), reviewer_id=admin_user.id) + db.commit() + + with pytest.raises(BusinessRuleViolation, match="Only pending"): + approve_modification_request(db, str(request.id), reviewer_id=admin_user.id) + def test_reject_request_leaves_campaign_untouched(self, db, active_campaign_with_test, admin_user): ct = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).first() request = create_modification_request( @@ -310,6 +339,20 @@ class TestModificationRequests: with pytest.raises(BusinessRuleViolation, match="Review notes are required"): reject_modification_request(db, str(request.id), reviewer_id=admin_user.id, review_notes="") + def test_reject_already_decided_request_raises(self, db, active_campaign_with_test, admin_user): + ct = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).first() + request = create_modification_request( + db, str(active_campaign_with_test.id), + requester_id=admin_user.id, action="remove_test", + test_id=str(ct.test_id), justification="Test superseded", + ) + db.commit() + reject_modification_request(db, str(request.id), reviewer_id=admin_user.id, review_notes="Not needed") + db.commit() + + with pytest.raises(BusinessRuleViolation, match="Only pending"): + reject_modification_request(db, str(request.id), reviewer_id=admin_user.id, review_notes="Again") + def test_list_modification_requests_filters_by_status(self, db, active_campaign_with_test, admin_user): ct = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).first() create_modification_request(