From fd47db7beacd4a59f6c0d5cd63d17416d1aec5cd Mon Sep 17 00:00:00 2001 From: kitos Date: Fri, 3 Jul 2026 11:04:20 +0200 Subject: [PATCH] fix(campaigns): null-safe test_id serialization, correct stale comments serialize_modification_request crashed toward a string 'None' instead of JSON null for requests whose test_id was nulled by the SET NULL cascade. Also corrects two comments that still described the debunked ordering theory instead of the actual SET NULL fix, and fixes a test that asserted post-cascade state without committing first (the ORM only picks up out-of-band DB-side SET NULL on already-loaded objects after expire_on_commit forces a re-read, matching real router behavior). --- backend/app/services/campaign_crud_service.py | 10 +++++----- backend/tests/test_campaign_approval.py | 20 +++++++++++++++---- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/backend/app/services/campaign_crud_service.py b/backend/app/services/campaign_crud_service.py index ae8f5f4..c1599bf 100644 --- a/backend/app/services/campaign_crud_service.py +++ b/backend/app/services/campaign_crud_service.py @@ -957,7 +957,7 @@ def serialize_modification_request(db: Session, request: CampaignModificationReq "campaign_name": request.campaign.name if request.campaign else None, "requested_by": str(request.requested_by) if request.requested_by else None, "action": request.action, - "test_id": str(request.test_id), + "test_id": str(request.test_id) if request.test_id else None, "test_name": test.name if test else None, "order_index": request.order_index, "phase": request.phase, @@ -1049,10 +1049,10 @@ def approve_modification_request( if request.status != "pending": raise BusinessRuleViolation("Only pending modification requests can be approved") - # Mark the request approved *before* applying the underlying change: a - # "remove_test" approval deletes the Test row, which cascades (ON DELETE - # CASCADE on test_id) and would otherwise wipe out this very request row - # before we get a chance to persist its reviewed/approved fields. + # A "remove_test" approval deletes the underlying Test row below, which + # nulls this row's test_id (test_id uses ON DELETE SET NULL specifically + # so this audit record survives that delete instead of being cascaded + # away with it — see CampaignModificationRequest.test_id). request.status = "approved" request.reviewed_by = reviewer_id request.reviewed_at = datetime.utcnow() diff --git a/backend/tests/test_campaign_approval.py b/backend/tests/test_campaign_approval.py index 0165543..3d7abf2 100644 --- a/backend/tests/test_campaign_approval.py +++ b/backend/tests/test_campaign_approval.py @@ -177,6 +177,7 @@ from app.services.campaign_crud_service import ( approve_modification_request, reject_modification_request, list_modification_requests, + serialize_modification_request, ) @@ -278,14 +279,18 @@ class TestModificationRequests: request_id = request.id approve_modification_request(db, str(request_id), reviewer_id=admin_user.id) + db.commit() 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. + # The request row must survive deletion of the underlying Test — this + # locks in test_id's ondelete="SET NULL" (not CASCADE), which is what + # keeps this audit record intact after the test it referenced is gone. + # (Commit above first: ON DELETE SET NULL is applied DB-side by the + # cascade, and the ORM only picks that up on already-loaded objects + # after expire_on_commit forces a fresh read — exactly what happens + # for real in the router, which commits before serializing.) reloaded = ( db.query(CampaignModificationRequest) .filter(CampaignModificationRequest.id == request_id) @@ -296,6 +301,13 @@ class TestModificationRequests: assert reloaded.reviewed_by == admin_user.id assert reloaded.reviewed_at is not None + # Serialization must not crash and must emit a real null, not the + # string "None", for a request whose test_id has been nulled out. + assert reloaded.test_id is None + serialized = serialize_modification_request(db, reloaded) + assert serialized["test_id"] is None + assert serialized["test_name"] is 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(