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.
This commit is contained in:
kitos
2026-07-03 10:48:42 +02:00
parent dc5f206bf5
commit 12a5484003
3 changed files with 101 additions and 4 deletions
@@ -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,
)
+6 -2
View File
@@ -259,10 +259,14 @@ class CampaignModificationRequest(Base):
nullable=True, nullable=True,
) )
action = Column(String, nullable=False) # add_test, remove_test 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( test_id = Column(
UUID(as_uuid=True), UUID(as_uuid=True),
ForeignKey("tests.id", ondelete="CASCADE"), ForeignKey("tests.id", ondelete="SET NULL"),
nullable=False, nullable=True,
) )
order_index = Column(Integer, nullable=True) order_index = Column(Integer, nullable=True)
phase = Column(String, nullable=True) phase = Column(String, nullable=True)
+45 -2
View File
@@ -4,7 +4,7 @@ import uuid
import pytest 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.technique import Technique
from app.models.test import Test from app.models.test import Test
from app.models.enums import TestState from app.models.enums import TestState
@@ -275,12 +275,41 @@ class TestModificationRequests:
test_id=str(ct.test_id), justification="Test superseded", test_id=str(ct.test_id), justification="Test superseded",
) )
db.commit() 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() cts = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).all()
assert len(cts) == 0 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): 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() ct = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).first()
request = create_modification_request( request = create_modification_request(
@@ -310,6 +339,20 @@ class TestModificationRequests:
with pytest.raises(BusinessRuleViolation, match="Review notes are required"): with pytest.raises(BusinessRuleViolation, match="Review notes are required"):
reject_modification_request(db, str(request.id), reviewer_id=admin_user.id, review_notes="") 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): 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() ct = db.query(CampaignTest).filter(CampaignTest.campaign_id == active_campaign_with_test.id).first()
create_modification_request( create_modification_request(