diff --git a/api/features/workflows/core/exceptions.py b/api/features/workflows/core/exceptions.py index 6dc4507743ff..e7a9c6b94311 100644 --- a/api/features/workflows/core/exceptions.py +++ b/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 diff --git a/api/features/workflows/core/migrations/0009_prevent_cascade_delete_from_user_delete.py b/api/features/workflows/core/migrations/0009_prevent_cascade_delete_from_user_delete.py new file mode 100644 index 000000000000..a950ff479f5d --- /dev/null +++ b/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), + ), + ] diff --git a/api/features/workflows/core/models.py b/api/features/workflows/core/models.py index 6b63bdb85fad..1a0ec84575f0 100644 --- a/api/features/workflows/core/models.py +++ b/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 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 04d0fd3cd8d0..96c4569470d4 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 @@ -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 diff --git a/frontend/web/components/pages/ChangeRequestPage.js b/frontend/web/components/pages/ChangeRequestPage.js index fb41c90bdaaf..da9d3769f846 100644 --- a/frontend/web/components/pages/ChangeRequestPage.js +++ b/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 && diff --git a/frontend/web/components/pages/ChangeRequestsPage.js b/frontend/web/components/pages/ChangeRequestsPage.js index a2fc75d0607d..d86d0bc066e8 100644 --- a/frontend/web/components/pages/ChangeRequestsPage.js +++ b/frontend/web/components/pages/ChangeRequestsPage.js @@ -203,8 +203,8 @@ const ChangeRequestsPage = class extends Component {
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}` : ''}
@@ -283,8 +283,8 @@ const ChangeRequestsPage = class extends Component {
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'}