From 052bd5adc2a08faf7fb14bca589a9b6f1765b550 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 24 Jan 2025 12:27:47 +0000 Subject: [PATCH 1/5] Make project name unique --- ...ke_project_name_unique_per_organisation.py | 46 +++++++++++++++++++ api/projects/models.py | 1 + ...ons.py => test_unit_project_migrations.py} | 34 ++++++++++++++ 3 files changed, 81 insertions(+) create mode 100644 api/projects/migrations/0027_make_project_name_unique_per_organisation.py rename api/tests/unit/projects/{test_migrations.py => test_unit_project_migrations.py} (72%) diff --git a/api/projects/migrations/0027_make_project_name_unique_per_organisation.py b/api/projects/migrations/0027_make_project_name_unique_per_organisation.py new file mode 100644 index 000000000000..884d4c0343e1 --- /dev/null +++ b/api/projects/migrations/0027_make_project_name_unique_per_organisation.py @@ -0,0 +1,46 @@ +# Generated by Django 4.2.18 on 2025-01-24 11:29 +from django.apps.registry import Apps +from django.db import migrations +from django.db.backends.base.schema import BaseDatabaseSchemaEditor +from django.db.models import Count + + +def handle_duplicate_project_name( + apps: Apps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + Project = apps.get_model("projects", "Project") + + # Get all projects, grouped by organisation and name + projects_grouped = ( + Project.objects.values("organisation", "name") + .annotate(name_count=Count("name")) + .filter(name_count__gt=1) + ) + + # Iterate over organisations that have duplicate project names + for org in projects_grouped: + # Get projects with the same name under the same organisation + duplicate_projects = Project.objects.filter( + name=org["name"], organisation=org["organisation"] + ) + + # Skip first project and rename remaining duplicates + for i, project in enumerate(duplicate_projects[1:], start=1): + project.name = f"{project.name} ({i})" + project.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ("organisations", "0058_update_audit_and_history_limits_in_sub_cache"), + ("projects", "0026_add_change_request_approval_limit_to_projects"), + ] + + operations = [ + migrations.RunPython(handle_duplicate_project_name, reverse_code=migrations.RunPython.noop), + migrations.AlterUniqueTogether( + name="project", + unique_together={("name", "organisation")}, + ), + ] diff --git a/api/projects/models.py b/api/projects/models.py index 0eee8a630b66..f0797e96b4c7 100644 --- a/api/projects/models.py +++ b/api/projects/models.py @@ -112,6 +112,7 @@ class Project(LifecycleModelMixin, SoftDeleteExportableModel): class Meta: ordering = ["id"] + unique_together = (("name", "organisation"),) def __str__(self): return "Project %s" % self.name diff --git a/api/tests/unit/projects/test_migrations.py b/api/tests/unit/projects/test_unit_project_migrations.py similarity index 72% rename from api/tests/unit/projects/test_migrations.py rename to api/tests/unit/projects/test_unit_project_migrations.py index 20f3ef4b19dd..d1b062bc0e2c 100644 --- a/api/tests/unit/projects/test_migrations.py +++ b/api/tests/unit/projects/test_unit_project_migrations.py @@ -96,3 +96,37 @@ def test_merge_duplicate_permissions_migration(migrator): # and non_duplicate permission still exists assert UserProjectPermission.objects.filter(id=non_duplicate_permission.id).exists() + + +@pytest.mark.skipif( + settings.SKIP_MIGRATION_TESTS is True, + reason="Skip migration tests to speed up tests where necessary", +) +def test_make_project_name_unique_per_organisation_migration(migrator): + # Given - the migration state is at 0026 (before the migration we want to test) + old_state = migrator.apply_initial_migration( + ("projects", "0026_add_change_request_approval_limit_to_projects") + ) + + Organisation = old_state.apps.get_model("organisations", "Organisation") + Project = old_state.apps.get_model("projects", "Project") + + organisation = Organisation.objects.create(name="Test Organisation") + + duplicate_project_name = "Duplicate project" + project_1 = Project.objects.create(name=duplicate_project_name, organisation=organisation) + duplicate_project = Project.objects.create(name=duplicate_project_name, organisation=organisation) + + non_duplicate_project_name = "Non duplicate project" + non_duplicate_project = Project.objects.create(name=non_duplicate_project_name, organisation=organisation) + + # When - we run the migration + new_state = migrator.apply_tested_migration( + ("projects", "0027_make_project_name_unique_per_organisation") + ) + NewProject = new_state.apps.get_model("projects", "Project") + + # Then + assert NewProject.objects.get(id=project_1.id).name == duplicate_project_name + assert NewProject.objects.get(id=duplicate_project.id).name == f"{duplicate_project_name} (1)" + assert NewProject.objects.get(id=non_duplicate_project.id).name == non_duplicate_project_name From b8d13d9fa497b640fe5d450b9e071b1600d1258e Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 24 Jan 2025 14:35:53 +0000 Subject: [PATCH 2/5] Add test to verify that it's not possible to create a project with a duplicate name via the API --- api/projects/serializers.py | 9 ++++++++ .../unit/projects/test_unit_projects_views.py | 23 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/api/projects/serializers.py b/api/projects/serializers.py index 339b1b8fa90a..85440cde3cce 100644 --- a/api/projects/serializers.py +++ b/api/projects/serializers.py @@ -74,6 +74,15 @@ class ProjectCreateSerializer(ReadOnlyIfNotValidPlanMixin, ProjectListSerializer invalid_plans_regex = r"^(free|startup.*|scale-up.*)$" field_names = ("stale_flags_limit_days", "enable_realtime_updates") + class Meta(ProjectListSerializer.Meta): + validators = [ + serializers.UniqueTogetherValidator( + queryset=ProjectListSerializer.Meta.model.objects.all(), + fields=("name", "organisation"), + message="A project with this name already exists.", + ) + ] + def get_subscription(self) -> typing.Optional[Subscription]: view = self.context["view"] diff --git a/api/tests/unit/projects/test_unit_projects_views.py b/api/tests/unit/projects/test_unit_projects_views.py index 71f078356dbc..bf27b207dccd 100644 --- a/api/tests/unit/projects/test_unit_projects_views.py +++ b/api/tests/unit/projects/test_unit_projects_views.py @@ -875,3 +875,26 @@ def test_delete_project_delete_handles_cascade_delete( mocked_handle_cascade_delete.delay.assert_called_once_with( kwargs={"project_id": project.id} ) + + +def test_cannot_create_duplicate_project_name( + admin_client: APIClient, + project: Project, +) -> None: + # Given + data = { + "name": project.name, + "organisation": project.organisation_id, + } + url = reverse("api-v1:projects:project-list") + + # When + response = admin_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == { + "non_field_errors": ["A project with this name already exists."] + } From b1ed967dc24c7ec3fe02ff428481049b1f6f5e69 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 24 Jan 2025 14:38:48 +0000 Subject: [PATCH 3/5] Revert "Make project name unique" This reverts commit 052bd5adc2a08faf7fb14bca589a9b6f1765b550. --- ...ke_project_name_unique_per_organisation.py | 46 ------------------- api/projects/models.py | 1 - ...oject_migrations.py => test_migrations.py} | 34 -------------- 3 files changed, 81 deletions(-) delete mode 100644 api/projects/migrations/0027_make_project_name_unique_per_organisation.py rename api/tests/unit/projects/{test_unit_project_migrations.py => test_migrations.py} (72%) diff --git a/api/projects/migrations/0027_make_project_name_unique_per_organisation.py b/api/projects/migrations/0027_make_project_name_unique_per_organisation.py deleted file mode 100644 index 884d4c0343e1..000000000000 --- a/api/projects/migrations/0027_make_project_name_unique_per_organisation.py +++ /dev/null @@ -1,46 +0,0 @@ -# Generated by Django 4.2.18 on 2025-01-24 11:29 -from django.apps.registry import Apps -from django.db import migrations -from django.db.backends.base.schema import BaseDatabaseSchemaEditor -from django.db.models import Count - - -def handle_duplicate_project_name( - apps: Apps, schema_editor: BaseDatabaseSchemaEditor -) -> None: - Project = apps.get_model("projects", "Project") - - # Get all projects, grouped by organisation and name - projects_grouped = ( - Project.objects.values("organisation", "name") - .annotate(name_count=Count("name")) - .filter(name_count__gt=1) - ) - - # Iterate over organisations that have duplicate project names - for org in projects_grouped: - # Get projects with the same name under the same organisation - duplicate_projects = Project.objects.filter( - name=org["name"], organisation=org["organisation"] - ) - - # Skip first project and rename remaining duplicates - for i, project in enumerate(duplicate_projects[1:], start=1): - project.name = f"{project.name} ({i})" - project.save() - - -class Migration(migrations.Migration): - - dependencies = [ - ("organisations", "0058_update_audit_and_history_limits_in_sub_cache"), - ("projects", "0026_add_change_request_approval_limit_to_projects"), - ] - - operations = [ - migrations.RunPython(handle_duplicate_project_name, reverse_code=migrations.RunPython.noop), - migrations.AlterUniqueTogether( - name="project", - unique_together={("name", "organisation")}, - ), - ] diff --git a/api/projects/models.py b/api/projects/models.py index f0797e96b4c7..0eee8a630b66 100644 --- a/api/projects/models.py +++ b/api/projects/models.py @@ -112,7 +112,6 @@ class Project(LifecycleModelMixin, SoftDeleteExportableModel): class Meta: ordering = ["id"] - unique_together = (("name", "organisation"),) def __str__(self): return "Project %s" % self.name diff --git a/api/tests/unit/projects/test_unit_project_migrations.py b/api/tests/unit/projects/test_migrations.py similarity index 72% rename from api/tests/unit/projects/test_unit_project_migrations.py rename to api/tests/unit/projects/test_migrations.py index d1b062bc0e2c..20f3ef4b19dd 100644 --- a/api/tests/unit/projects/test_unit_project_migrations.py +++ b/api/tests/unit/projects/test_migrations.py @@ -96,37 +96,3 @@ def test_merge_duplicate_permissions_migration(migrator): # and non_duplicate permission still exists assert UserProjectPermission.objects.filter(id=non_duplicate_permission.id).exists() - - -@pytest.mark.skipif( - settings.SKIP_MIGRATION_TESTS is True, - reason="Skip migration tests to speed up tests where necessary", -) -def test_make_project_name_unique_per_organisation_migration(migrator): - # Given - the migration state is at 0026 (before the migration we want to test) - old_state = migrator.apply_initial_migration( - ("projects", "0026_add_change_request_approval_limit_to_projects") - ) - - Organisation = old_state.apps.get_model("organisations", "Organisation") - Project = old_state.apps.get_model("projects", "Project") - - organisation = Organisation.objects.create(name="Test Organisation") - - duplicate_project_name = "Duplicate project" - project_1 = Project.objects.create(name=duplicate_project_name, organisation=organisation) - duplicate_project = Project.objects.create(name=duplicate_project_name, organisation=organisation) - - non_duplicate_project_name = "Non duplicate project" - non_duplicate_project = Project.objects.create(name=non_duplicate_project_name, organisation=organisation) - - # When - we run the migration - new_state = migrator.apply_tested_migration( - ("projects", "0027_make_project_name_unique_per_organisation") - ) - NewProject = new_state.apps.get_model("projects", "Project") - - # Then - assert NewProject.objects.get(id=project_1.id).name == duplicate_project_name - assert NewProject.objects.get(id=duplicate_project.id).name == f"{duplicate_project_name} (1)" - assert NewProject.objects.get(id=non_duplicate_project.id).name == non_duplicate_project_name From 999d88233a9f0bed11f37bc6e33b5762293b1403 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 24 Jan 2025 14:47:32 +0000 Subject: [PATCH 4/5] Add test to verify it's possible to create a duplicate project in another organisation --- .../unit/projects/test_unit_projects_views.py | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/api/tests/unit/projects/test_unit_projects_views.py b/api/tests/unit/projects/test_unit_projects_views.py index bf27b207dccd..2c7243fb9125 100644 --- a/api/tests/unit/projects/test_unit_projects_views.py +++ b/api/tests/unit/projects/test_unit_projects_views.py @@ -20,7 +20,7 @@ from environments.dynamodb.types import ProjectIdentityMigrationStatus from environments.identities.models import Identity from features.models import Feature, FeatureSegment -from organisations.models import Organisation, Subscription +from organisations.models import Organisation, OrganisationRole, Subscription from organisations.permissions.models import ( OrganisationPermissionModel, UserOrganisationPermission, @@ -898,3 +898,28 @@ def test_cannot_create_duplicate_project_name( assert response.json() == { "non_field_errors": ["A project with this name already exists."] } + + +def test_can_create_project_with_duplicate_name_in_another_organisation( + admin_user: FFAdminUser, + admin_client: APIClient, + project: Project, + organisation_two: Organisation, +) -> None: + # Given + assert project.organisation != organisation_two + admin_user.add_organisation(organisation_two, OrganisationRole.ADMIN) + + data = { + "name": project.name, + "organisation": organisation_two.id, + } + url = reverse("api-v1:projects:project-list") + + # When + response = admin_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED From 68a83a0b0516eafcd125225e5fc9ae82d34d92bb Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 24 Jan 2025 15:21:39 +0000 Subject: [PATCH 5/5] Fix fixture --- api/tests/integration/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index 0a0cf6495c04..18a5e6be4443 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -61,7 +61,7 @@ def dynamo_enabled_project( ): settings.EDGE_ENABLED = True project_data = { - "name": "Test Project", + "name": "Dynamo Enabled Project", "organisation": organisation, } url = reverse("api-v1:projects:project-list")