From 72abe04c72b3985de637b9ff8b11550eb0e13593 Mon Sep 17 00:00:00 2001 From: Quentin BEY Date: Wed, 23 Oct 2024 17:21:03 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=83=EF=B8=8F(teams)=20remove=20`slug`?= =?UTF-8?q?=20field?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After some reflexion, the use of a slug field raises to many problems without being really needed. One problem is the slug is made from the group name, but we don't have unicity on this, so a user might be blocked without any clue. We also want to allow group names to be reused (which is already allowed except for the automatic slug). The unique ID that will be shared with Service Providers will be the PK/UUID. --- CHANGELOG.md | 4 + src/backend/core/admin.py | 1 - src/backend/core/api/serializers.py | 7 -- .../migrations/0003_team_slug_nullable.py | 18 +++++ src/backend/core/models.py | 11 --- .../tests/teams/test_core_api_teams_create.py | 79 +------------------ .../teams/test_core_api_teams_retrieve.py | 1 - .../tests/teams/test_core_api_teams_update.py | 33 +------- src/backend/core/tests/test_models_teams.py | 6 -- .../demo/management/commands/create_demo.py | 2 - .../__tests__/app-desk/teams-create.spec.ts | 21 ----- 11 files changed, 25 insertions(+), 158 deletions(-) create mode 100644 src/backend/core/migrations/0003_team_slug_nullable.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 537512f..9440b7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,10 @@ and this project adheres to - 🔇(backend) remove Sentry duplicated warning/errors - 👷(ci) add sharding e2e tests #467 +### Removed + +- 🗃️(teams) remove `slug` field + ## [1.4.1] - 2024-10-23 ### Fixed diff --git a/src/backend/core/admin.py b/src/backend/core/admin.py index 7af7c8d..c0b3d4a 100644 --- a/src/backend/core/admin.py +++ b/src/backend/core/admin.py @@ -115,7 +115,6 @@ class TeamAdmin(admin.ModelAdmin): inlines = (TeamAccessInline, TeamWebhookInline) list_display = ( "name", - "slug", "created_at", "updated_at", ) diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index f42668b..f9d776c 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -172,7 +172,6 @@ class TeamSerializer(serializers.ModelSerializer): """Serialize teams.""" abilities = serializers.SerializerMethodField(read_only=True) - slug = serializers.SerializerMethodField() class Meta: model = models.Team @@ -181,7 +180,6 @@ class TeamSerializer(serializers.ModelSerializer): "name", "accesses", "abilities", - "slug", "created_at", "updated_at", ] @@ -189,7 +187,6 @@ class TeamSerializer(serializers.ModelSerializer): "id", "accesses", "abilities", - "slug", "created_at", "updated_at", ] @@ -209,10 +206,6 @@ class TeamSerializer(serializers.ModelSerializer): return team.get_abilities(request.user) return {} - def get_slug(self, instance): - """Return slug from the team's name.""" - return instance.get_slug() - class InvitationSerializer(serializers.ModelSerializer): """Serialize invitations.""" diff --git a/src/backend/core/migrations/0003_team_slug_nullable.py b/src/backend/core/migrations/0003_team_slug_nullable.py new file mode 100644 index 0000000..1ed60a4 --- /dev/null +++ b/src/backend/core/migrations/0003_team_slug_nullable.py @@ -0,0 +1,18 @@ +# Generated by Django 5.1.2 on 2024-11-04 14:25 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0002_add_organization_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='team', + name='slug', + field=models.SlugField(max_length=100, null=True), + ), + ] diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 7bc1299..d4a7e5a 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -22,7 +22,6 @@ from django.db import models, transaction from django.template.loader import render_to_string from django.utils import timezone from django.utils.functional import lazy -from django.utils.text import slugify from django.utils.translation import gettext_lazy as _ from django.utils.translation import override @@ -486,7 +485,6 @@ class Team(BaseModel): """ name = models.CharField(max_length=100) - slug = models.SlugField(max_length=100, unique=True, null=False, editable=False) users = models.ManyToManyField( User, @@ -511,15 +509,6 @@ class Team(BaseModel): def __str__(self): return self.name - def save(self, *args, **kwargs): - """Override save function to compute the slug.""" - self.slug = self.get_slug() - return super().save(*args, **kwargs) - - def get_slug(self): - """Compute slug value from name.""" - return slugify(self.name) - def get_abilities(self, user): """ Compute and return abilities for a given user on the team. diff --git a/src/backend/core/tests/teams/test_core_api_teams_create.py b/src/backend/core/tests/teams/test_core_api_teams_create.py index a2e0b8a..54c40e0 100644 --- a/src/backend/core/tests/teams/test_core_api_teams_create.py +++ b/src/backend/core/tests/teams/test_core_api_teams_create.py @@ -5,12 +5,11 @@ Tests for Teams API endpoint in People's core app: create import pytest from rest_framework.status import ( HTTP_201_CREATED, - HTTP_400_BAD_REQUEST, HTTP_401_UNAUTHORIZED, ) from rest_framework.test import APIClient -from core.factories import OrganizationFactory, TeamFactory, UserFactory +from core.factories import OrganizationFactory, UserFactory from core.models import Team pytestmark = pytest.mark.django_db @@ -55,82 +54,6 @@ def test_api_teams_create_authenticated(): assert team.accesses.filter(role="owner", user=user).exists() -def test_api_teams_create_authenticated_slugify_name(): - """ - Creating teams should automatically generate a slug. - """ - organization = OrganizationFactory(with_registration_id=True) - user = UserFactory(organization=organization) - client = APIClient() - client.force_login(user) - - response = client.post( - "/api/v1.0/teams/", - {"name": "my team"}, - ) - - assert response.status_code == HTTP_201_CREATED - team = Team.objects.get() - assert team.name == "my team" - assert team.slug == "my-team" - assert team.organization == organization - - -@pytest.mark.parametrize( - "param", - [ - ("my team", "my-team"), - ("my team", "my-team"), - ("MY TEAM TOO", "my-team-too"), - ("mon équipe", "mon-equipe"), - ("front devs & UX", "front-devs-ux"), - ], -) -def test_api_teams_create_authenticated_expected_slug(param): - """ - Creating teams should automatically create unaccented, no unicode, lower-case slug. - """ - organization = OrganizationFactory(with_registration_id=True) - user = UserFactory(organization=organization) - - client = APIClient() - client.force_login(user) - - response = client.post( - "/api/v1.0/teams/", - { - "name": param[0], - }, - ) - - assert response.status_code == HTTP_201_CREATED - team = Team.objects.get() - assert team.name == param[0] - assert team.slug == param[1] - assert team.organization == organization - - -def test_api_teams_create_authenticated_unique_slugs(): - """ - Creating teams should raise an error if already existing slug. - """ - TeamFactory(name="existing team") - user = UserFactory() - - client = APIClient() - client.force_login(user) - - response = client.post( - "/api/v1.0/teams/", - { - "name": "èxisting team", - }, - ) - - assert response.status_code == HTTP_400_BAD_REQUEST - assert response.json()["slug"] == ["Team with this Slug already exists."] - - def test_api_teams_create_cannot_override_organization(): """ Authenticated users should be able to create teams and not diff --git a/src/backend/core/tests/teams/test_core_api_teams_retrieve.py b/src/backend/core/tests/teams/test_core_api_teams_retrieve.py index eff3276..2edd1fa 100644 --- a/src/backend/core/tests/teams/test_core_api_teams_retrieve.py +++ b/src/backend/core/tests/teams/test_core_api_teams_retrieve.py @@ -69,7 +69,6 @@ def test_api_teams_retrieve_authenticated_related(): assert response.json() == { "id": str(team.id), "name": team.name, - "slug": team.slug, "abilities": team.get_abilities(user), "created_at": team.created_at.isoformat().replace("+00:00", "Z"), "updated_at": team.updated_at.isoformat().replace("+00:00", "Z"), diff --git a/src/backend/core/tests/teams/test_core_api_teams_update.py b/src/backend/core/tests/teams/test_core_api_teams_update.py index 0f11c09..ace2b53 100644 --- a/src/backend/core/tests/teams/test_core_api_teams_update.py +++ b/src/backend/core/tests/teams/test_core_api_teams_update.py @@ -7,7 +7,6 @@ import random import pytest from rest_framework.status import ( HTTP_200_OK, - HTTP_400_BAD_REQUEST, HTTP_401_UNAUTHORIZED, HTTP_403_FORBIDDEN, HTTP_404_NOT_FOUND, @@ -125,7 +124,7 @@ def test_api_teams_update_authenticated_administrators(): elif key == "updated_at": assert value > initial_values[key] else: - # name, slug and abilities successfully modified + # name and abilities successfully modified assert value == new_values[key] @@ -158,7 +157,7 @@ def test_api_teams_update_authenticated_owners(): elif key == "updated_at": assert value > old_team_values[key] else: - # name, slug and abilities successfully modified + # name and abilities successfully modified assert value == new_team_values[key] @@ -189,31 +188,3 @@ def test_api_teams_update_administrator_or_owner_of_another(): team.refresh_from_db() team_values = serializers.TeamSerializer(instance=team).data assert team_values == old_team_values - - -def test_api_teams_update_existing_slug_should_return_error(): - """ - Updating a team's name to an existing slug should return a bad request, - instead of creating a duplicate. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - factories.TeamFactory(name="Existing team", users=[(user, "administrator")]) - my_team = factories.TeamFactory(name="New team", users=[(user, "administrator")]) - - updated_values = serializers.TeamSerializer(instance=my_team).data - # Update my team's name for existing team. Creates a duplicate slug - updated_values["name"] = "existing team" - response = client.put( - f"/api/v1.0/teams/{my_team.id!s}/", - updated_values, - format="json", - ) - assert response.status_code == HTTP_400_BAD_REQUEST - assert response.json()["slug"] == ["Team with this Slug already exists."] - # Both teams names and slugs should be unchanged - assert my_team.name == "New team" - assert my_team.slug == "new-team" diff --git a/src/backend/core/tests/test_models_teams.py b/src/backend/core/tests/test_models_teams.py index 05ab0cb..c2f1995 100644 --- a/src/backend/core/tests/test_models_teams.py +++ b/src/backend/core/tests/test_models_teams.py @@ -47,12 +47,6 @@ def test_models_teams_name_max_length(): factories.TeamFactory(name="a " * 51) -def test_models_teams_slug_empty(): - """Slug field should not be empty.""" - with pytest.raises(ValidationError, match="This field cannot be blank."): - models.Team.objects.create(slug="") - - # get_abilities diff --git a/src/backend/demo/management/commands/create_demo.py b/src/backend/demo/management/commands/create_demo.py index a4d2b4f..2198915 100755 --- a/src/backend/demo/management/commands/create_demo.py +++ b/src/backend/demo/management/commands/create_demo.py @@ -174,8 +174,6 @@ def create_demo(stdout): queue.push( models.Team( name=f"Team {i:d}", - # slug should be automatic but bulk_create doesn't use save - slug=f"team-{i:d}", ) ) queue.flush() diff --git a/src/frontend/apps/e2e/__tests__/app-desk/teams-create.spec.ts b/src/frontend/apps/e2e/__tests__/app-desk/teams-create.spec.ts index 5018d5c..afdd530 100644 --- a/src/frontend/apps/e2e/__tests__/app-desk/teams-create.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-desk/teams-create.spec.ts @@ -93,27 +93,6 @@ test.describe('Teams Create', () => { await expect(page).toHaveURL(/\/teams\//); }); - test('checks error when duplicate team', async ({ page, browserName }) => { - const panel = page.getByLabel('Teams panel').first(); - - await panel.getByRole('link', { name: 'Add a team' }).click(); - - const teamName = `My duplicate team ${browserName}-${Math.floor(Math.random() * 1000)}`; - await page.getByText('Team name').fill(teamName); - await page.getByRole('button', { name: 'Create the team' }).click(); - - await panel.getByRole('link', { name: 'Add a team' }).click(); - - await page.getByText('Team name').fill(teamName); - await page.getByRole('button', { name: 'Create the team' }).click(); - - await expect( - page.getByText( - 'This name is already used for another group. Please enter another one.', - ), - ).toBeVisible(); - }); - test('checks 404 on teams/[id] page', async ({ page }) => { await page.goto('/teams/some-unknown-team'); await expect(