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(