From f59da3a32530955fc0edd2e0669362ff7c95528b Mon Sep 17 00:00:00 2001 From: Srijan Tripathi Date: Fri, 14 Aug 2026 19:27:04 +0000 Subject: [PATCH 1/6] fix: prevent system segments from being overwritten via change request drafts and add unit test --- api/core/workflows_services.py | 5 +++++ .../core/test_unit_workflows_models.py | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 2547681e56a2..050e00e25f54 100644 --- a/api/core/workflows_services.py +++ b/api/core/workflows_services.py @@ -106,6 +106,7 @@ def _publish_change_sets(self, published_by: "FFAdminUser") -> None: for change_set in self.change_request.change_sets.all(): change_set.publish(user=published_by) + @transaction.atomic @transaction.atomic def _publish_segments(self) -> None: for draft_segment in self.change_request.segments.all(): @@ -114,6 +115,10 @@ def _publish_segments(self) -> None: logger.warning("missing-live-segment", draft_segment=draft_segment.uuid) continue + # Prevent overwriting system segments + if getattr(live_segment, 'is_system_segment', False): + raise ValueError("System segments cannot be overwritten via change request drafts.") + # Make a revision of the live segment revision = live_segment.clone(is_revision=True) logger.info( diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index dce1054a8169..7db68c23e50d 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -1257,3 +1257,24 @@ def test_change_request_commit__v1_segment_override_draft__inherits_mv_hashing_s # Then the draft carries the superseded override's id as its bucketing salt draft_feature_state.refresh_from_db() assert draft_feature_state.mv_hashing_salt == live_override.id +def test_change_request_commit__system_segment_draft__raises_value_error( + segment: Segment, + change_request: ChangeRequest, + admin_user: FFAdminUser, +) -> None: + # Given + segment.is_system_segment = True + segment.save() + + Segment.objects.create( + name="system-segment-draft", + change_request=change_request, + project=segment.project, + version_of=segment, + ) + + # When / Then + with pytest.raises( + ValueError, match="System segments cannot be overwritten via change request drafts." + ): + change_request.commit(admin_user) \ No newline at end of file From 0b5ab68ba956d00671da59e07ca6971d6854d8b2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:29:20 +0000 Subject: [PATCH 2/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/core/workflows_services.py | 6 ++++-- .../features/workflows/core/test_unit_workflows_models.py | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 050e00e25f54..9a48746ba561 100644 --- a/api/core/workflows_services.py +++ b/api/core/workflows_services.py @@ -116,8 +116,10 @@ def _publish_segments(self) -> None: continue # Prevent overwriting system segments - if getattr(live_segment, 'is_system_segment', False): - raise ValueError("System segments cannot be overwritten via change request drafts.") + if getattr(live_segment, "is_system_segment", False): + raise ValueError( + "System segments cannot be overwritten via change request drafts." + ) # Make a revision of the live segment revision = live_segment.clone(is_revision=True) diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index 7db68c23e50d..ab4b93686fb3 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -1257,6 +1257,8 @@ def test_change_request_commit__v1_segment_override_draft__inherits_mv_hashing_s # Then the draft carries the superseded override's id as its bucketing salt draft_feature_state.refresh_from_db() assert draft_feature_state.mv_hashing_salt == live_override.id + + def test_change_request_commit__system_segment_draft__raises_value_error( segment: Segment, change_request: ChangeRequest, @@ -1275,6 +1277,7 @@ def test_change_request_commit__system_segment_draft__raises_value_error( # When / Then with pytest.raises( - ValueError, match="System segments cannot be overwritten via change request drafts." + ValueError, + match="System segments cannot be overwritten via change request drafts.", ): - change_request.commit(admin_user) \ No newline at end of file + change_request.commit(admin_user) From 00269fc60c362cf9fa214bb231fed7ab670f9e50 Mon Sep 17 00:00:00 2001 From: Srijan Tripathi Date: Fri, 14 Aug 2026 19:46:20 +0000 Subject: [PATCH 3/6] fix: make change request commit fully atomic and add rollback assertion to test --- api/core/workflows_services.py | 6 +++--- .../core/test_unit_workflows_models.py | 21 ++++++++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 9a48746ba561..d7cceb4c2b89 100644 --- a/api/core/workflows_services.py +++ b/api/core/workflows_services.py @@ -20,7 +20,7 @@ class ChangeRequestCommitService: def __init__(self, change_request: "ChangeRequest") -> None: self.change_request = change_request - + @transaction.atomic def commit(self, committed_by: "FFAdminUser") -> None: if not self.change_request.is_approved(): raise ChangeRequestNotApprovedError( @@ -106,8 +106,8 @@ def _publish_change_sets(self, published_by: "FFAdminUser") -> None: for change_set in self.change_request.change_sets.all(): change_set.publish(user=published_by) - @transaction.atomic - @transaction.atomic + + def _publish_segments(self) -> None: for draft_segment in self.change_request.segments.all(): live_segment = draft_segment.version_of diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index ab4b93686fb3..b94f9602baa8 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -1258,11 +1258,12 @@ def test_change_request_commit__v1_segment_override_draft__inherits_mv_hashing_s draft_feature_state.refresh_from_db() assert draft_feature_state.mv_hashing_salt == live_override.id - def test_change_request_commit__system_segment_draft__raises_value_error( segment: Segment, change_request: ChangeRequest, admin_user: FFAdminUser, + feature: Feature, + environment: Environment, ) -> None: # Given segment.is_system_segment = True @@ -1275,9 +1276,23 @@ def test_change_request_commit__system_segment_draft__raises_value_error( version_of=segment, ) + # Add a feature state to test transaction rollback behavior + feature_state = FeatureState.objects.create( + feature=feature, + environment=environment, + change_request=change_request, + version=None, + ) + initial_version = feature_state.version + # When / Then with pytest.raises( - ValueError, - match="System segments cannot be overwritten via change request drafts.", + ValueError, match="System segments cannot be overwritten via change request drafts." ): change_request.commit(admin_user) + + # Assert that the transaction rolled back successfully + feature_state.refresh_from_db() + change_request.refresh_from_db() + assert feature_state.version == initial_version + assert change_request.committed_at is None \ No newline at end of file From c3c40a9cb718856acf2d764d4a9188ad921b9e88 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:47:44 +0000 Subject: [PATCH 4/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/core/workflows_services.py | 3 +-- .../features/workflows/core/test_unit_workflows_models.py | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 5efd8c9b24cd..9d615443c6db 100644 --- a/api/core/workflows_services.py +++ b/api/core/workflows_services.py @@ -23,6 +23,7 @@ class ChangeRequestCommitService: def __init__(self, change_request: "ChangeRequest") -> None: self.change_request = change_request + @transaction.atomic def commit(self, committed_by: "FFAdminUser") -> None: if not self.change_request.is_approved(): @@ -112,8 +113,6 @@ def _publish_change_sets(self, published_by: "FFAdminUser") -> None: for change_set in self.change_request.change_sets.all(): change_set.publish(user=published_by) - - def _validate_segments_are_not_cohort_managed(self) -> None: for draft_segment in self.change_request.segments.all(): if ( diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index 85b07cae6a02..667b4541f7b9 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -1284,6 +1284,7 @@ def test_change_request_commit__v1_segment_override_draft__inherits_mv_hashing_s draft_feature_state.refresh_from_db() assert draft_feature_state.mv_hashing_salt == live_override.id + def test_change_request_commit__system_segment_draft__raises_value_error( segment: Segment, change_request: ChangeRequest, @@ -1313,7 +1314,8 @@ def test_change_request_commit__system_segment_draft__raises_value_error( # When / Then with pytest.raises( - ValueError, match="System segments cannot be overwritten via change request drafts." + ValueError, + match="System segments cannot be overwritten via change request drafts.", ): change_request.commit(admin_user) @@ -1321,4 +1323,4 @@ def test_change_request_commit__system_segment_draft__raises_value_error( feature_state.refresh_from_db() change_request.refresh_from_db() assert feature_state.version == initial_version - assert change_request.committed_at is None \ No newline at end of file + assert change_request.committed_at is None From 3e15279a0af68949621881ef5226d96ba8b9f061 Mon Sep 17 00:00:00 2001 From: Srijan Tripathi Date: Fri, 14 Aug 2026 19:58:23 +0000 Subject: [PATCH 5/6] style: add trailing newline to pass ruff linter --- .../unit/features/workflows/core/test_unit_workflows_models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index 667b4541f7b9..5ccedf5e3701 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -1324,3 +1324,4 @@ def test_change_request_commit__system_segment_draft__raises_value_error( change_request.refresh_from_db() assert feature_state.version == initial_version assert change_request.committed_at is None + From 3d0b1ba367baba03b1facf69625baf777755b75e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:59:22 +0000 Subject: [PATCH 6/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../unit/features/workflows/core/test_unit_workflows_models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index 5ccedf5e3701..667b4541f7b9 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -1324,4 +1324,3 @@ def test_change_request_commit__system_segment_draft__raises_value_error( change_request.refresh_from_db() assert feature_state.version == initial_version assert change_request.committed_at is None -