Skip to content

Consolidate team creation logic #1153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Consolidate team creation logic #1153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion django/thunderstore/api/cyberstorm/services/team.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@
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 = Team.create(name=team_name)

Check warning on line 31 in django/thunderstore/api/cyberstorm/services/team.py

View check run for this annotation

Codecov / codecov/patch

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

Added line #L31 was not covered by tests
team.add_member(user=user, role=TeamMemberRole.owner)
return team
2 changes: 1 addition & 1 deletion django/thunderstore/api/cyberstorm/tests/test_package_listing_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ def test_listing_by_namespace_view__returns_only_packages_listed_in_community_be
community: Community,
team: Team,
) -> None:
namespace = team.namespaces.get()
namespace = team.get_namespace()
expected = PackageListingFactory(
community_=community,
package_kwargs={"namespace": namespace},
Expand Down
9 changes: 2 additions & 7 deletions django/thunderstore/repository/models/team.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,13 +185,8 @@ 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)
return team
Comment on lines -188 to +189
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not entirely sure why we want to move or remove the Namespace validation from the Team.create() function. Can you clarify the motivation behind this refactor?

I’m wondering why we’re using a @classmethod directly on the model, instead of the more idiomatic Django pattern of defining custom creation logic on the model's manager. Using the manager would keep the model cleaner and make creation more consistent with Django conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The namespace validation is redundant because Team.create() will always call save() which will call validate() which also has namespace validation.

I'm pretty sure we could just remove Team.create() entirely and use Team.objects.create() like normal, I just tried to keep my changes minimal since it's used in a lot of tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that the namespace validation in Team.create() was redundant since it's already handled in validate(). However, now that the custom validation logic has been removed, the Team.create() method itself has become redundant, it's just a wrapper around Team.objects.create() with no added functionality.

I think the cleanest solution would be to remove the Team.create() classmethod entirely and update all call sites to use the standard Team.objects.create(). Otherwise we're just creating technical debt, and using Team.objects.create() is more in line with Django conventions.


@classmethod
@transaction.atomic
Expand Down
19 changes: 0 additions & 19 deletions django/thunderstore/repository/tests/test_team.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,6 @@ def test_team_create(name: str, should_fail: bool) -> None:
assert team.name == name


@pytest.mark.django_db
def test_team_create_namespace_creation() -> None:
team = Team.create(name="Test_Team")
assert len(Namespace.objects.filter(name="Test_Team")) == 1
ns = Namespace(name="taken_namespace", team=team)
ns.save()
with pytest.raises(ValidationError) as e:
Team.create(name="taken_namespace")
assert "Namespace with the Teams name exists" in str(e.value)


@pytest.mark.django_db
@pytest.mark.parametrize(
("username", "expected_name"),
Expand Down Expand Up @@ -673,14 +662,6 @@ def test_team_ensure_can_create_service_account(
assert team.ensure_can_create_service_account(user) is None


@pytest.mark.django_db
def test_team_save():
team = Team.create(name="TestTeam")
team.save()
assert team.namespaces is not None
assert team.namespaces.first().name == team.name


@pytest.mark.django_db
def test_team_get_namespace(team):
team_named_ns = Namespace.objects.get_or_create(name=team.name, team=team)[0]
Expand Down
Loading