Merge pull request #1139 from thunderstore-io/refactor-team-form-views

Refactor team disband and create views(TS-2426)

Author: Roffenlund

django/thunderstore/api/cyberstorm/services/team.py

@@ -1,36 +1,28 @@
-from django.core.exceptions import ValidationError
 from django.db import transaction
-from django.shortcuts import get_object_or_404
 
 from thunderstore.account.models import ServiceAccount
 from thunderstore.core.exceptions import PermissionValidationError
 from thunderstore.core.types import UserType
-from thunderstore.repository.models import Namespace, Team, TeamMember
+from thunderstore.repository.models import Team, TeamMember
 from thunderstore.repository.models.team import TeamMemberRole
 
 
 @transaction.atomic
-def disband_team(user: UserType, team_name: str) -> None:
-    teams = Team.objects.exclude(is_active=False)
-    team = get_object_or_404(teams, name=team_name)
-    team.ensure_user_can_access(user)
-    team.ensure_user_can_disband(user)
+def disband_team(agent: UserType, team: Team) -> None:
+    team.ensure_user_can_access(agent)
+    team.ensure_user_can_disband(agent)
     team.delete()
 
 
 @transaction.atomic
-def create_team(user: UserType, team_name: str) -> Team:
-    if not user or not user.is_authenticated or not user.is_active:
+def create_team(agent: UserType, team_name: str) -> Team:
+    if not agent or not agent.is_authenticated or not agent.is_active:
         raise PermissionValidationError("Must be authenticated to create teams")
-    if getattr(user, "service_account", None) is not None:
+    if getattr(agent, "service_account", None) is not None:
         raise PermissionValidationError("Service accounts cannot create teams")
-    if Team.objects.filter(name=team_name).exists():
-        raise ValidationError("A team with the provided name already exists")
-    if Namespace.objects.filter(name=team_name).exists():
-        raise ValidationError("A namespace with the provided name already exists")
 
-    team = Team.objects.create(name=team_name)
-    team.add_member(user=user, role=TeamMemberRole.owner)
+    team = Team.create(name=team_name)
+    team.add_member(user=agent, role=TeamMemberRole.owner)
     return team
 
 

django/thunderstore/api/cyberstorm/tests/services/test_team_services.py

@@ -1,6 +1,5 @@
 import pytest
 from django.core.exceptions import ValidationError
-from django.http import Http404
 
 from conftest import TestUserTypes
 from thunderstore.api.cyberstorm.services import team as team_services
@@ -13,60 +12,91 @@
 @pytest.mark.django_db
 def test_disband_team_success(team_owner):
     team_pk = team_owner.team.pk
-    team_services.disband_team(team_owner.user, team_owner.team.name)
+    team_services.disband_team(agent=team_owner.user, team=team_owner.team)
     assert not Team.objects.filter(pk=team_pk).exists()
 
 
-@pytest.mark.django_db
-def test_disband_team_team_not_found(user):
-    with pytest.raises(Http404):
-        team_services.disband_team(user, "NonExistentTeam")
-
-
 @pytest.mark.django_db
 def test_disband_team_user_cannot_access_team(user):
     team = Team.objects.create(name="TestTeam")
-    with pytest.raises(ValidationError, match="Must be a member to access team"):
-        team_services.disband_team(user, team.name)
+    error_msg = "Must be a member to access team"
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.disband_team(agent=user, team=team)
 
 
 @pytest.mark.django_db
 def test_disband_team_user_cannot_disband(team_member):
-    with pytest.raises(ValidationError, match="Must be an owner to disband team"):
-        team_services.disband_team(team_member.user, team_member.team.name)
+    error_msg = "Must be an owner to disband team"
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.disband_team(agent=team_member.user, team=team_member.team)
+
+
+@pytest.mark.django_db
+def test_disband_team_with_packages(package, team_owner):
+    team = team_owner.team
+    package.owner = team
+    package.save()
+
+    with pytest.raises(ValidationError, match="Unable to disband teams with packages"):
+        team_services.disband_team(agent=team_owner.user, team=team)
+
+
+@pytest.mark.django_db
+def test_disband_team_user_is_service_account(service_account, team):
+    service_account_user = service_account.user
+    error_msg = "Service accounts are unable to perform this action"
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.disband_team(agent=service_account_user, team=team)
+
+
+@pytest.mark.django_db
+def test_disband_team_user_not_authenticated(team):
+    error_msg = "Must be authenticated"
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.disband_team(agent=None, team=team)
+
+
+@pytest.mark.django_db
+def test_disband_team_user_not_active(user, team):
+    user.is_active = False
+    user.save()
+
+    error_msg = "User has been deactivated"
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.disband_team(agent=user, team=team)
 
 
 @pytest.mark.django_db
 def test_create_team_name_exists_in_team(user):
     Team.objects.create(name="existing_team")
 
-    error_msg = "A team with the provided name already exists"
+    error_msg = "Team with this name already exists"
     with pytest.raises(ValidationError, match=error_msg):
-        team_services.create_team(user, "existing_team")
+        team_services.create_team(agent=user, team_name="existing_team")
 
 
 @pytest.mark.django_db
 def test_create_team_name_exists_in_namespace(user):
     Namespace.objects.create(name="existing_namespace")
 
-    error_msg = "A namespace with the provided name already exists"
+    error_msg = "Namespace with this name already exists"
     with pytest.raises(ValidationError, match=error_msg):
-        team_services.create_team(user, "existing_namespace")
+        team_services.create_team(agent=user, team_name="existing_namespace")
 
 
 @pytest.mark.django_db
 def test_create_team_user_is_service_account(service_account):
     service_account_user = service_account.user
 
     error_msg = "Service accounts cannot create teams"
-    with pytest.raises(ValidationError, match=error_msg):
-        team_services.create_team(service_account_user, "new_team")
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.create_team(agent=service_account_user, team_name="new_team")
 
 
 @pytest.mark.django_db
 def test_create_team_success(user):
     team_name = "new_team"
-    team = team_services.create_team(user, team_name)
+    team = team_services.create_team(agent=user, team_name=team_name)
 
     assert Team.objects.filter(name=team_name).exists()
     assert team.name == team_name
@@ -330,3 +360,40 @@ def test_update_team_member_cannot_remove_last_owner(team_owner):
         team_services.update_team_member(
             team_owner.user, team_owner, TeamMemberRole.member
         )
+
+
+@pytest.mark.django_db
+def test_create_team_user_not_authenticated():
+    error_msg = "Must be authenticated to create teams"
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.create_team(agent=None, team_name="new_team")
+
+
+@pytest.mark.django_db
+def test_create_team_user_not_active(user):
+    user.is_active = False
+    user.save()
+
+    error_msg = "Must be authenticated to create teams"
+    with pytest.raises(PermissionValidationError, match=error_msg):
+        team_services.create_team(agent=user, team_name="new_team")
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    ("name1", "name2", "should_fail"),
+    (
+        ("Team", "team", True),
+        ("Team", "t_eam", False),
+        ("team", "teaM", True),
+        ("team", "team", True),
+    ),
+)
+def test_create_team_name_conflict(user, name1: str, name2: str, should_fail: bool):
+    Team.create(name=name1)
+    if should_fail:
+        with pytest.raises(ValidationError):
+            team_services.create_team(agent=user, team_name=name2)
+    else:
+        team = team_services.create_team(agent=user, team_name=name2)
+        assert team.name == name2

django/thunderstore/api/cyberstorm/tests/test_create_team.py

@@ -53,9 +53,7 @@ def test_create_team__fail_because_team_with_provided_name_exists(
 ):
     api_client.force_authenticate(user)
     response = make_request(api_client, team.name)
-    expected_response = {
-        "non_field_errors": ["A team with the provided name already exists"]
-    }
+    expected_response = {"non_field_errors": ["Team with this name already exists"]}
 
     assert response.status_code == 400
     assert response.json() == expected_response
@@ -70,7 +68,7 @@ def test_create_team__fail_because_team_with_provided_namespace_exists(
     NamespaceFactory(name="CoolestTeamNameEver")
     response = make_request(api_client, "CoolestTeamNameEver")
     expected_response = {
-        "non_field_errors": ["A namespace with the provided name already exists"]
+        "non_field_errors": ["Namespace with this name already exists"]
     }
 
     assert response.status_code == 400

django/thunderstore/api/cyberstorm/views/team.py

@@ -75,8 +75,10 @@ class TeamCreateAPIView(APIView):
     def post(self, request, *args, **kwargs):
         serializer = CyberstormCreateTeamSerializer(data=request.data)
         serializer.is_valid(raise_exception=True)
+
         team_name = serializer.validated_data["name"]
-        team = create_team(user=request.user, team_name=team_name)
+        team = create_team(agent=request.user, team_name=team_name)
+
         return_data = CyberstormTeamSerializer(team).data
         return Response(return_data, status=status.HTTP_201_CREATED)
 
@@ -166,8 +168,8 @@ class DisbandTeamAPIView(APIView):
         responses={status.HTTP_204_NO_CONTENT: ""},
     )
     def delete(self, request, *args, **kwargs):
-        team_name = kwargs["team_name"]
-        disband_team(user=request.user, team_name=team_name)
+        team = get_object_or_404(Team, name=kwargs["team_name"])
+        disband_team(agent=request.user, team=team)
         return Response(status=status.HTTP_204_NO_CONTENT)
 
 

django/thunderstore/repository/forms/team.py

@@ -3,21 +3,17 @@
 from django import forms
 from django.contrib.auth import get_user_model
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
+from django.db import transaction
 
 from thunderstore.api.cyberstorm.services.team import (
+    create_team,
+    disband_team,
     remove_team_member,
     update_team,
     update_team_member,
 )
-from thunderstore.core.exceptions import PermissionValidationError
 from thunderstore.core.types import UserType
-from thunderstore.repository.models import (
-    Namespace,
-    Team,
-    TeamMember,
-    TeamMemberRole,
-    transaction,
-)
+from thunderstore.repository.models import Team, TeamMember, TeamMemberRole
 from thunderstore.repository.validators import PackageReferenceComponentValidator
 
 User = get_user_model()
@@ -38,23 +34,22 @@ def __init__(self, user: UserType, *args, **kwargs):
 
     def clean_name(self):
         name = self.cleaned_data["name"]
-        if Team.objects.filter(name__iexact=name.lower()).exists():
-            raise ValidationError(f"A team with the provided name already exists")
-        if Namespace.objects.filter(name__iexact=name.lower()).exists():
-            raise ValidationError("A namespace with the provided name already exists")
+        if Team.objects.filter(name__iexact=name).exists():
+            raise ValidationError("Team with this name already exists")
         return name
 
-    def clean(self):
-        if not self.user or not self.user.is_authenticated or not self.user.is_active:
-            raise PermissionValidationError("Must be authenticated to create teams")
-        if getattr(self.user, "service_account", None) is not None:
-            raise PermissionValidationError("Service accounts cannot create teams")
-        return super().clean()
-
     @transaction.atomic
     def save(self, *args, **kwargs) -> Team:
-        instance = super().save()
-        instance.add_member(user=self.user, role=TeamMemberRole.owner)
+        if self.errors:
+            raise ValidationError(self.errors)
+
+        try:
+            team_name = self.cleaned_data["name"]
+            instance = create_team(agent=self.user, team_name=team_name)
+        except ValidationError as e:
+            self.add_error(None, e)
+            raise ValidationError(self.errors)
+
         return instance
 
 
@@ -169,13 +164,17 @@ def clean_verification(self):
     def clean(self):
         if not self.instance.pk:
             raise ValidationError("Missing team instance")
-        self.instance.ensure_user_can_disband(self.user)
         return super().clean()
 
     @transaction.atomic
     def save(self, **kwargs):
-        self.instance.ensure_user_can_disband(self.user)
-        self.instance.delete()
+        if self.errors:
+            raise ValidationError(self.errors)
+        try:
+            disband_team(agent=self.user, team=self.instance)
+        except ValidationError as e:
+            self.add_error(None, e)
+            raise ValidationError(self.errors)
 
 
 class DonationLinkTeamForm(forms.ModelForm):

django/thunderstore/repository/models/team.py

@@ -187,13 +187,9 @@ def add_member(self, user: UserType, role: str) -> TeamMember:
     @classmethod
     @transaction.atomic
     def create(cls, name, **kwargs):
-        existing_ns = Namespace.objects.filter(name__iexact=name).first()
-        if existing_ns:
-            raise ValidationError("Namespace with the Teams name exists")
-        else:
-            team = cls.objects.create(name=name, **kwargs)
-            Namespace.objects.create(name=name, team=team)
-            return team
+        team = cls.objects.create(name=name, **kwargs)
+        Namespace.objects.create(name=name, team=team)
+        return team
 
     @classmethod
     @transaction.atomic

django/thunderstore/repository/tests/test_team.py

@@ -75,7 +75,7 @@ def test_team_create_namespace_creation() -> None:
     ns.save()
     with pytest.raises(ValidationError) as e:
         Team.create(name="taken_namespace")
-    assert "Namespace with the Teams name exists" in str(e.value)
+    assert "Namespace with this name already exists" in str(e.value)
 
 
 @pytest.mark.django_db
@@ -769,3 +769,39 @@ def test_team_ensure_user_can_manage_packages(
     else:
         assert team.can_user_manage_packages(user) is True
         assert team.ensure_user_can_manage_packages(user) is None
+
+
+@pytest.mark.django_db
+def test_team_create__success():
+    Team.create(name="TestTeam")
+    assert Team.objects.filter(name="TestTeam").count() == 1
+    assert Namespace.objects.filter(name="TestTeam").count() == 1
+
+
+@pytest.mark.django_db
+def test_team_create__team_exists_fail(team):
+    with pytest.raises(ValidationError) as e:
+        Team.create(name=team.name)
+    assert "Team with this name already exists" in str(e.value)
+    assert Team.objects.filter(name=team.name).count() == 1
+    assert Namespace.objects.filter(name=team.name).count() == 1
+
+
+@pytest.mark.django_db
+def test_team_create__namespace_exists_fail():
+    NamespaceFactory.create(name="TestTeam")
+    with pytest.raises(ValidationError) as e:
+        Team.create(name="TestTeam")
+    assert "Namespace with this name already exists" in str(e.value)
+    assert Team.objects.filter(name="TestTeam").count() == 0
+    assert Namespace.objects.filter(name="TestTeam").count() == 1
+
+
+@pytest.mark.django_db
+def test_team_create__team_name_read_only_fail(team):
+    with pytest.raises(ValidationError) as e:
+        team.name = "NewName"
+        team.save()
+    assert "Team name is read only" in str(e.value)
+    team.refresh_from_db()
+    assert team.name != "NewName"