fix: prevent cascade deletes from users deleting change requests (#3580)

api/features/workflows/core/exceptions.py

@@ -12,3 +12,7 @@ class ChangeRequestNotApprovedError(FeatureWorkflowError):
 
 class CannotApproveOwnChangeRequest(FeatureWorkflowError):
     status_code = status.HTTP_400_BAD_REQUEST
+
+
+class ChangeRequestDeletionError(FeatureWorkflowError):
+    status_code = status.HTTP_400_BAD_REQUEST

api/features/workflows/core/migrations/0009_prevent_cascade_delete_from_user_delete.py

@@ -0,0 +1,21 @@
+# Generated by Django 3.2.24 on 2024-03-07 18:04
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('workflows_core', '0008_remove_redundant_column'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='changerequest',
+            name='user',
+            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='change_requests', to=settings.AUTH_USER_MODEL),
+        ),
+    ]

api/features/workflows/core/models.py

@@ -17,6 +17,7 @@
     AFTER_CREATE,
     AFTER_SAVE,
     AFTER_UPDATE,
+    BEFORE_DELETE,
     LifecycleModel,
     LifecycleModelMixin,
     hook,
@@ -39,6 +40,7 @@
 from ...versioning.models import EnvironmentFeatureVersion
 from .exceptions import (
     CannotApproveOwnChangeRequest,
+    ChangeRequestDeletionError,
     ChangeRequestNotApprovedError,
 )
 
@@ -63,10 +65,14 @@ class ChangeRequest(
 
     title = models.CharField(max_length=500)
     description = models.TextField(blank=True, null=True)
+
+    # We allow null here so that deleting users does not cascade to deleting change
+    # requests which can be used for historical purposes.
     user = models.ForeignKey(
         settings.AUTH_USER_MODEL,
-        on_delete=models.CASCADE,
+        on_delete=models.SET_NULL,
         related_name="change_requests",
+        null=True,
     )
 
     environment = models.ForeignKey(
@@ -228,6 +234,16 @@ def create_audit_log_for_related_feature_state(self):
                     args=(feature_state.id,)
                 )
 
+    @hook(BEFORE_DELETE)
+    def prevent_change_request_delete_if_committed(self) -> None:
+        # In the workflows-logic module, we prevent change requests from being
+        # deleted but, since this can have unexpected effects on published
+        # feature states, we also want to prevent it at the ORM level.
+        if self.committed_at and not self.environment.deleted_at:
+            raise ChangeRequestDeletionError(
+                "Cannot delete a Change Request that has been committed."
+            )
+
 
 class ChangeRequestApproval(LifecycleModel, abstract_base_auditable_model_factory()):
     related_object_type = RelatedObjectType.CHANGE_REQUEST

api/tests/unit/features/workflows/core/test_unit_workflows_models.py

@@ -18,6 +18,7 @@
 from features.versioning.versioning_service import get_environment_flags_list
 from features.workflows.core.exceptions import (
     CannotApproveOwnChangeRequest,
+    ChangeRequestDeletionError,
     ChangeRequestNotApprovedError,
 )
 from features.workflows.core.models import (
@@ -610,3 +611,18 @@ def test_commit_change_request_publishes_environment_feature_versions(
     assert environment_feature_version.published
     assert environment_feature_version.published_by == admin_user
     assert environment_feature_version.live_from == now
+
+
+def test_cannot_delete_committed_change_request(
+    change_request: ChangeRequest, admin_user: FFAdminUser
+) -> None:
+    # Given
+    change_request.commit(admin_user)
+    change_request.save()
+
+    # When
+    with pytest.raises(ChangeRequestDeletionError):
+        change_request.delete()
+
+    # Then
+    # exception raised

frontend/web/components/pages/ChangeRequestPage.js

@@ -238,7 +238,7 @@ const ChangeRequestsPage = class extends Component {
       this.fetchFeature(featureId)
     }
     const user =
-      changeRequest && orgUsers.find((v) => v.id === changeRequest.user)
+      changeRequest && changeRequest.user && orgUsers.find((v) => v.id === changeRequest.user)
     const committedBy =
       (changeRequest.committed_by &&
         orgUsers &&

frontend/web/components/pages/ChangeRequestsPage.js

@@ -203,8 +203,8 @@ const ChangeRequestsPage = class extends Component {
                             <div className='list-item-subtitle mt-1'>
                               Created{' '}
                               {moment(created_at).format('Do MMM YYYY HH:mma')}{' '}
-                              by {user && user.first_name}{' '}
-                              {user && user.last_name}
+                              by {user && user.first_name || 'Unknown'}{' '}
+                              {user && user.last_name || 'user'}
                               {description ? ` - ${description}` : ''}
                             </div>
                           </Flex>
@@ -283,8 +283,8 @@ const ChangeRequestsPage = class extends Component {
                             <div className='list-item-subtitle mt-1'>
                               Live from{' '}
                               {moment(created_at).format('Do MMM YYYY HH:mma')}{' '}
-                              by {user && user.first_name}{' '}
-                              {user && user.last_name}
+                              by {user && user.first_name || 'Unknown'}{' '}
+                              {user && user.last_name || 'user'}
                             </div>
                           </Flex>
                           <div className='table-column'>