Refactor team disband and create views(TS-2426) #1139
Conversation
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
e0e34bd to
356c7e9
Compare
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
356c7e9 to
3713125
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
3713125 to
de16554
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1139 +/- ##
==========================================
+ Coverage 92.80% 92.81% +0.01%
==========================================
Files 337 337
Lines 10367 10358 -9
Branches 942 938 -4
==========================================
- Hits 9621 9614 -7
+ Misses 618 617 -1
+ Partials 128 127 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Roffenlund
force-pushed
the
refactor-team-form-views
branch
2 times, most recently
from
db4e908 to
86e5e4c
Compare
Roffenlund
force-pushed
the
refactor-team-form-views
branch
2 times, most recently
from
838cd63 to
9df9daa
Compare
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py
Outdated
Show resolved
Hide resolved
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
9df9daa to
833d2ed
Compare
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
833d2ed to
7abdd7f
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughService API changed: Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (1)
85-91: Expect PermissionValidationError for create_team by service accountService enforces this as a permission failure.
- error_msg = "Service accounts cannot create teams" - with pytest.raises(ValidationError, match=error_msg): + error_msg = "Service accounts cannot create teams" + with pytest.raises(PermissionValidationError, match=error_msg): team_services.create_team(agent=service_account_user, team_name="new_team")django/thunderstore/repository/forms/team.py (1)
7-15: Import missing service functions (fix F821) and use consistently
create_teamanddisband_teamare used below but not imported.-from thunderstore.api.cyberstorm.services.team import ( - remove_team_member, - update_team, - update_team_member, -) +from thunderstore.api.cyberstorm.services.team import ( + create_team, + disband_team, + remove_team_member, + update_team, + update_team_member, +)
♻️ Duplicate comments (3)
django/thunderstore/repository/tests/test_team_forms.py (2)
75-78: Same note as above for error capture in save()Relies on form.save() not raising. Please ensure both PermissionValidationError and ValidationError are handled in the form save.
468-471: Same note: DisbandTeamForm.save() should swallow service errorsTo make this pattern work without
pytest.raises, catch PermissionValidationError and ValidationError in DisbandTeamForm.save and add them toform.errors.django/thunderstore/repository/forms/team.py (1)
35-41: CreateTeamForm.save should catch permission errors tooCapture PermissionValidationError so errors are added to the form instead of bubbling.
- try: + try: team_name = self.cleaned_data["name"] self.instance = create_team(agent=self.user, team_name=team_name) - except ValidationError as e: + except (PermissionValidationError, ValidationError) as e: self.add_error(None, e)
🧹 Nitpick comments (1)
django/thunderstore/repository/tests/test_team_forms.py (1)
507-510: OK, but ensure DisbandTeamForm.save catches both exception typesDisband will raise ValidationError (packages exist) today; also handle PermissionValidationError for symmetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
django/thunderstore/api/cyberstorm/services/team.py(1 hunks)django/thunderstore/api/cyberstorm/tests/services/test_team_services.py(5 hunks)django/thunderstore/api/cyberstorm/views/team.py(2 hunks)django/thunderstore/repository/forms/team.py(2 hunks)django/thunderstore/repository/tests/test_team_forms.py(6 hunks)django/thunderstore/repository/views/team_settings.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
django/thunderstore/repository/tests/test_team_forms.py (1)
django/thunderstore/repository/forms/team.py (5)
save(31-41)save(84-94)save(117-131)save(157-164)save(184-198)
django/thunderstore/repository/views/team_settings.py (1)
django/thunderstore/repository/forms/team.py (5)
save(31-41)save(84-94)save(117-131)save(157-164)save(184-198)
django/thunderstore/api/cyberstorm/services/team.py (3)
django/thunderstore/repository/models/team.py (5)
Team(92-384)ensure_user_can_access(292-295)ensure_user_can_disband(338-344)add_member(180-185)TeamMemberRole(19-21)django/thunderstore/api/cyberstorm/views/team.py (3)
delete(138-148)delete(170-173)delete(215-218)django/thunderstore/repository/models/namespace.py (1)
Namespace(10-44)
django/thunderstore/repository/forms/team.py (3)
django/thunderstore/repository/models/team.py (4)
Team(92-384)TeamMember(42-79)TeamMemberRole(19-21)save(146-148)django/thunderstore/account/forms.py (3)
save(25-46)save(57-67)save(85-89)django/thunderstore/api/cyberstorm/services/team.py (2)
create_team(19-31)disband_team(12-15)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (4)
django/thunderstore/api/cyberstorm/services/team.py (2)
disband_team(12-15)create_team(19-31)django/conftest.py (4)
team_owner(165-170)user(116-121)team_member(156-161)service_account(422-433)django/thunderstore/repository/models/team.py (1)
Team(92-384)django/thunderstore/repository/forms/team.py (5)
save(31-41)save(84-94)save(117-131)save(157-164)save(184-198)
django/thunderstore/api/cyberstorm/views/team.py (2)
django/thunderstore/api/cyberstorm/services/team.py (2)
create_team(19-31)disband_team(12-15)django/thunderstore/repository/models/team.py (1)
Team(92-384)
🪛 Flake8 (7.3.0)
django/thunderstore/repository/views/team_settings.py
[error] 19-19: 'thunderstore.api.cyberstorm.services.team as team_service' imported but unused
(F401)
django/thunderstore/repository/forms/team.py
[error] 37-37: undefined name 'create_team'
(F821)
[error] 162-162: undefined name 'disband_team'
(F821)
🪛 Ruff (0.13.3)
django/thunderstore/api/cyberstorm/services/team.py
21-21: Avoid specifying long messages outside the exception class
(TRY003)
23-23: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
django/thunderstore/repository/forms/team.py
31-31: Unused method argument: args
(ARG002)
31-31: Unused method argument: kwargs
(ARG002)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
37-37: Undefined name create_team
(F821)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Unused method argument: kwargs
(ARG002)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Undefined name disband_team
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build docker image
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
🔇 Additional comments (8)
django/thunderstore/repository/tests/test_team_forms.py (2)
108-110: LGTM: explicit ValueError for invalid field errorsThis aligns with save() raising on pre-existing field errors.
487-489: LGTM: expecting ValueError for invalid verificationConsistent with raising on pre-existing form errors.
django/thunderstore/api/cyberstorm/services/team.py (2)
12-15: LGTM: disband_team aligns with domain checks and new signatureSignature and checks are correct; deletion gated by access and disband permissions.
19-31: LGTM: create_team validates agent and conflicts, assigns ownerCase-insensitive checks and owner assignment look good.
django/thunderstore/repository/views/team_settings.py (2)
157-161: Pattern works with forms that add errors in save()Fine to keep; just ensure CreateTeamForm.save doesn’t raise on service errors and uses
form.add_error.
182-184: Ditto for Disband: rely on save() to add errorsOK, provided DisbandTeamForm.save catches PermissionValidationError/ValidationError and adds errors.
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (1)
389-396: LGTM: corrected type annotation and conflict parametrizationAnnotation
should_fail: booland keyword calls align with new API.django/thunderstore/repository/forms/team.py (1)
147-151: LGTM: verification check is correctExact match against instance name is appropriate.
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
7abdd7f to
4762ef0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
django/thunderstore/api/cyberstorm/services/team.py(1 hunks)django/thunderstore/api/cyberstorm/tests/services/test_team_services.py(3 hunks)django/thunderstore/repository/forms/team.py(3 hunks)django/thunderstore/repository/tests/test_team_forms.py(4 hunks)django/thunderstore/repository/views/team_settings.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- django/thunderstore/repository/tests/test_team_forms.py
🧰 Additional context used
🧬 Code graph analysis (4)
django/thunderstore/api/cyberstorm/services/team.py (4)
django/thunderstore/repository/models/team.py (5)
Team(92-384)ensure_user_can_access(292-295)ensure_user_can_disband(338-344)add_member(180-185)TeamMemberRole(19-21)django/thunderstore/api/cyberstorm/views/team.py (3)
delete(138-148)delete(170-173)delete(215-218)django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/repository/models/namespace.py (1)
Namespace(10-44)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (3)
django/thunderstore/api/cyberstorm/services/team.py (2)
disband_team(12-15)create_team(19-31)django/conftest.py (4)
team_owner(165-170)user(116-121)team_member(156-161)service_account(422-433)django/thunderstore/repository/models/team.py (1)
Team(92-384)
django/thunderstore/repository/views/team_settings.py (2)
django/thunderstore/repository/forms/team.py (5)
save(35-46)save(89-99)save(122-136)save(163-170)save(190-204)django/thunderstore/repository/models/team.py (1)
save(146-148)
django/thunderstore/repository/forms/team.py (2)
django/thunderstore/api/cyberstorm/services/team.py (2)
create_team(19-31)disband_team(12-15)django/thunderstore/repository/models/team.py (3)
Team(92-384)TeamMember(42-79)TeamMemberRole(19-21)
🪛 Ruff (0.13.3)
django/thunderstore/api/cyberstorm/services/team.py
21-21: Avoid specifying long messages outside the exception class
(TRY003)
23-23: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
django/thunderstore/repository/forms/team.py
44-44: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
170-170: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
- GitHub Check: Build docker image
Roffenlund
force-pushed
the
refactor-team-form-views
branch
2 times, most recently
from
a0e817a to
beeac65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (3)
43-48: Fix exception type for service account disband testThe service raises
PermissionValidationError(viavalidate_userorensure_user_can_disband), notValidationError.- with pytest.raises( - ValidationError, match="Service accounts are unable to perform this action" - ): + with pytest.raises( + PermissionValidationError, match="Service accounts are unable to perform this action" + ): team_services.disband_team(agent=service_account_user, team=team)
363-366: Fix exception type for unauthenticated create_team testThe service explicitly raises
PermissionValidationErrorat line 20-21 for authentication checks.error_msg = "Must be authenticated to create teams" - with pytest.raises(ValidationError, match=error_msg): + with pytest.raises(PermissionValidationError, match=error_msg): team_services.create_team(agent=None, team_name="new_team")
370-376: Fix exception type for inactive user create_team testThe service explicitly raises
PermissionValidationErrorat line 20-21 for is_active checks.error_msg = "Must be authenticated to create teams" - with pytest.raises(ValidationError, match=error_msg): + with pytest.raises(PermissionValidationError, match=error_msg): team_services.create_team(agent=user, team_name="new_team")django/thunderstore/repository/forms/team.py (2)
40-45: Catch both ValidationError and PermissionValidationErrorThe
create_teamservice raisesPermissionValidationErrorfor authentication and service account checks, in addition toValidationErrorfor name conflicts. Uncaught permission errors will propagate up instead of being added to the form.try: team_name = self.cleaned_data["name"] instance = create_team(agent=self.user, team_name=team_name) - except ValidationError as e: + except (PermissionValidationError, ValidationError) as e: self.add_error(None, e) raise ValidationError(self.errors)
167-171: Catch both ValidationError and PermissionValidationErrorThe
disband_teamservice raisesPermissionValidationErrorfor access and ownership checks, in addition toValidationErrorfor package checks. Uncaught permission errors will propagate up instead of being added to the form.try: disband_team(agent=self.user, team=self.instance) - except ValidationError as e: + except (PermissionValidationError, ValidationError) as e: self.add_error(None, e) raise ValidationError(self.errors)
🧹 Nitpick comments (2)
django/thunderstore/repository/forms/team.py (2)
45-45: Consider exception chainingUse
raise ... from errto preserve the exception chain for better debugging.- raise ValidationError(self.errors) + raise ValidationError(self.errors) from e
171-171: Consider exception chainingUse
raise ... from errto preserve the exception chain for better debugging.- raise ValidationError(self.errors) + raise ValidationError(self.errors) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
django/thunderstore/api/cyberstorm/services/team.py(1 hunks)django/thunderstore/api/cyberstorm/tests/services/test_team_services.py(3 hunks)django/thunderstore/api/cyberstorm/tests/test_create_team.py(2 hunks)django/thunderstore/repository/forms/team.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
django/thunderstore/repository/forms/team.py (2)
django/thunderstore/api/cyberstorm/services/team.py (2)
create_team(19-27)disband_team(12-15)django/thunderstore/repository/models/team.py (3)
Team(92-384)TeamMember(42-79)TeamMemberRole(19-21)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (2)
django/thunderstore/api/cyberstorm/services/team.py (2)
disband_team(12-15)create_team(19-27)django/thunderstore/repository/models/team.py (1)
Team(92-384)
django/thunderstore/api/cyberstorm/services/team.py (2)
django/thunderstore/repository/models/team.py (5)
Team(92-384)ensure_user_can_access(292-295)ensure_user_can_disband(338-344)add_member(180-185)TeamMemberRole(19-21)django/thunderstore/api/cyberstorm/views/team.py (3)
delete(138-148)delete(170-173)delete(215-218)
🪛 Ruff (0.13.3)
django/thunderstore/repository/forms/team.py
45-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
171-171: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
django/thunderstore/api/cyberstorm/services/team.py
21-21: Avoid specifying long messages outside the exception class
(TRY003)
23-23: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build docker image
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
🔇 Additional comments (3)
django/thunderstore/api/cyberstorm/services/team.py (1)
25-26: LGTM - Namespace creation issue resolvedUsing
Team.createensures the namespace is created atomically with the team. The subsequentadd_membercall is protected by the@transaction.atomicdecorator, so any failure will roll back the entire operation.django/thunderstore/api/cyberstorm/tests/test_create_team.py (2)
56-56: LGTMError message correctly updated to match the domain layer's ValidationError from
Team.create.
70-70: LGTMError message correctly updated to match the domain layer's ValidationError from
Team.create.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (2)
43-49: Expect PermissionValidationError for service account disbandService accounts fail via PermissionValidationError from validate_user.
with pytest.raises( - ValidationError, match="Service accounts are unable to perform this action" + PermissionValidationError, match="Service accounts are unable to perform this action" ): team_services.disband_team(agent=service_account_user, team=team)
363-377: Expect PermissionValidationError for unauthenticated/inactive create_teamcreate_team enforces auth/active via PermissionValidationError.
def test_create_team_user_not_authenticated(): error_msg = "Must be authenticated to create teams" - with pytest.raises(ValidationError, match=error_msg): + with pytest.raises(PermissionValidationError, match=error_msg): team_services.create_team(agent=None, team_name="new_team") @@ def test_create_team_user_not_active(user): @@ - with pytest.raises(ValidationError, match=error_msg): + with pytest.raises(PermissionValidationError, match=error_msg): team_services.create_team(agent=user, team_name="new_team")
🧹 Nitpick comments (3)
django/thunderstore/repository/forms/team.py (3)
35-46: Preserve exception cause; optionally set self.instanceUse exception chaining and keep instance on the form.
def save(self, *args, **kwargs) -> Team: 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) + instance = create_team(agent=self.user, team_name=team_name) + except ValidationError as e: + self.add_error(None, e) + raise ValidationError(self.errors) from e - return instance + self.instance = instance + return self.instance
163-172: Preserve exception cause in DisbandTeamForm.saveChain the original error for easier debugging.
def save(self, **kwargs): 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) + except ValidationError as e: + self.add_error(None, e) + raise ValidationError(self.errors) from e
35-46: Optional: avoid nested atomic blocksServices already wrap operations in @transaction.atomic. Dropping form-level @transaction.atomic reduces savepoint overhead without changing behavior.
Also applies to: 163-172, 190-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
django/thunderstore/api/cyberstorm/services/team.py(1 hunks)django/thunderstore/api/cyberstorm/tests/services/test_team_services.py(3 hunks)django/thunderstore/api/cyberstorm/tests/test_create_team.py(2 hunks)django/thunderstore/repository/forms/team.py(3 hunks)django/thunderstore/repository/tests/test_team_forms.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (1)
django/thunderstore/api/cyberstorm/services/team.py (2)
disband_team(12-15)create_team(19-27)
django/thunderstore/repository/tests/test_team_forms.py (1)
django/thunderstore/repository/forms/team.py (5)
save(36-47)save(90-100)save(123-137)save(164-171)save(191-205)
django/thunderstore/repository/forms/team.py (2)
django/thunderstore/api/cyberstorm/services/team.py (2)
create_team(19-27)disband_team(12-15)django/thunderstore/repository/models/team.py (3)
Team(92-384)TeamMember(42-79)TeamMemberRole(19-21)
django/thunderstore/api/cyberstorm/services/team.py (3)
django/thunderstore/repository/models/team.py (5)
Team(92-384)ensure_user_can_access(292-295)ensure_user_can_disband(338-344)add_member(180-185)TeamMemberRole(19-21)django/thunderstore/api/cyberstorm/views/team.py (3)
delete(138-148)delete(170-173)delete(215-218)django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)
🪛 Ruff (0.13.3)
django/thunderstore/repository/forms/team.py
45-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
171-171: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
django/thunderstore/api/cyberstorm/services/team.py
21-21: Avoid specifying long messages outside the exception class
(TRY003)
23-23: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build docker image
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
🔇 Additional comments (7)
django/thunderstore/api/cyberstorm/tests/test_create_team.py (1)
56-56: LGTM: expected error string aligned with Team.createMatches the domain message “Namespace with the Teams name exists”.
Also applies to: 70-70
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (3)
33-40: LGTM: package guard raises ValidationErrorExpecting ValidationError with “Unable to disband teams with packages” is correct.
96-99: LGTM: create_team call and assertionsKeyword args and membership checks match the service API.
389-396: LGTM: name-conflict parametrized test uses Team.createUsing Team.create ensures namespace is created, aligning with the service’s collision checks.
django/thunderstore/repository/tests/test_team_forms.py (2)
43-46: LGTM: failure paths now assert form.save raises and check errorsWrapping save() with pytest.raises(ValidationError) matches the form’s behavior and surfaces service errors.
Also applies to: 76-80
468-471: LGTM: disband error paths captured via form.save()Both permission/validation failures bubble as ValidationError and are asserted correctly.
Also applies to: 506-509
django/thunderstore/api/cyberstorm/services/team.py (1)
11-15: LGTM: service API and invariants
- disband_team: access/owner checks then delete.
- create_team: auth and service-account guards, uses Team.create, then adds owner membership.
Matches domain rules and preserves namespace invariant.
Also applies to: 19-27
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
beeac65 to
e28c3f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
django/thunderstore/repository/forms/team.py (2)
40-45: Preserve exception chain when re-raising.Line 45 re-raises without preserving the original exception context. Use
from eto maintain the exception chain for better debugging.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) + raise ValidationError(self.errors) from eNote: The past review comment suggesting to catch
PermissionValidationErrorseparately is unnecessary since it's a subclass ofValidationErrorand is already caught by the current exception handler.
167-171: Preserve exception chain when re-raising.Line 171 re-raises without preserving the original exception context. Use
from eto maintain the exception chain for better debugging.try: disband_team(agent=self.user, team=self.instance) except ValidationError as e: self.add_error(None, e) - raise ValidationError(self.errors) + raise ValidationError(self.errors) from eNote: The past review comment suggesting to catch
PermissionValidationErrorseparately is unnecessary since it's a subclass ofValidationErrorand is already caught by the current exception handler.
🧹 Nitpick comments (1)
django/thunderstore/api/cyberstorm/services/team.py (1)
17-26: LGTM - Namespace creation issue resolved.The function now correctly uses
Team.create()(line 24), which creates both the Team and its associated Namespace atomically, addressing the previous review concern.Minor: Static analysis flags long exception messages (lines 20, 22) as a style issue (TRY003), but this doesn't affect correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
django/thunderstore/api/cyberstorm/services/team.py(1 hunks)django/thunderstore/repository/forms/team.py(3 hunks)django/thunderstore/repository/models/team.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
django/thunderstore/repository/models/team.py (2)
django/conftest.py (1)
team(151-152)django/thunderstore/repository/models/namespace.py (1)
Namespace(10-44)
django/thunderstore/api/cyberstorm/services/team.py (4)
django/thunderstore/repository/models/team.py (6)
Team(92-380)TeamMember(42-79)TeamMemberRole(19-21)ensure_user_can_access(288-291)ensure_user_can_disband(334-340)add_member(180-185)django/thunderstore/core/types.py (1)
UserType(13-16)django/thunderstore/api/cyberstorm/views/team.py (3)
delete(138-148)delete(170-173)delete(215-218)django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)
django/thunderstore/repository/forms/team.py (2)
django/thunderstore/api/cyberstorm/services/team.py (2)
create_team(18-26)disband_team(11-14)django/thunderstore/repository/models/team.py (3)
Team(92-380)TeamMember(42-79)TeamMemberRole(19-21)
🪛 Ruff (0.13.3)
django/thunderstore/api/cyberstorm/services/team.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
22-22: Avoid specifying long messages outside the exception class
(TRY003)
django/thunderstore/repository/forms/team.py
45-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
171-171: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
- GitHub Check: Build docker image
🔇 Additional comments (2)
django/thunderstore/repository/models/team.py (1)
189-192: LGTM - Clean delegation to validation flow.The refactored
create()method correctly relies onsave()'s validation (lines 140-141 still check namespace conflicts) while maintaining atomicity. This removes duplication and keeps the flow straightforward.django/thunderstore/api/cyberstorm/services/team.py (1)
10-14: LGTM - Signature update aligns with PR objectives.The updated signature
(agent, team)correctly delegates authorization to the Team model'sensure_*methods before deletion.
Remove raising ValidationError in Team.create if namespace exists. This check is performed in Team.validation() and does not have to be checked in Team.create(). Do this change in order to have consistent error messaging when using the Team.create function. Refs. TS-2426
Update disband_team service and create_team service. Utilize Team.create() in create_team service and remove redundant validation errors. Use objects as parameters in the services instead of trying to fetch and raise 404 errors in services. Refs. TS-2426
Update the create team and disband team views since the parameter names changed of the services. Refs. TS-2426
Use the create_team and disband_team services in the forms and views responsible for disband teams and creating teams. Update tests accordingly. Update the form for creating teams to clean the name manually in order to keep the error messaging consistent, otherwise tests will fail since because it is a ModelForm which throws a different formatted error by default. Refs. TS-2426
Roffenlund
force-pushed
the
refactor-team-form-views
branch
from
e28c3f8 to
70cede3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
django/thunderstore/repository/forms/team.py (2)
46-51: Catch PermissionValidationError in addition to ValidationError.The
create_teamservice raisesPermissionValidationErrorfor authentication and service account checks. Currently, onlyValidationErroris caught, so permission errors will propagate uncaught instead of being added to the form.Import
PermissionValidationErrorand update the exception handler:+from thunderstore.core.exceptions import PermissionValidationErrortry: team_name = self.cleaned_data["name"] instance = create_team(agent=self.user, team_name=team_name) - except ValidationError as e: + except (PermissionValidationError, ValidationError) as e: self.add_error(None, e) raise ValidationError(self.errors)Based on past review comments.
173-177: Catch PermissionValidationError in addition to ValidationError.The
disband_teamservice callsensure_user_can_accessandensure_user_can_disband, which raisePermissionValidationErrorfor authentication and permission failures. Only catchingValidationErrorwill leave these errors uncaught.Update the exception handler to catch both:
try: disband_team(agent=self.user, team=self.instance) - except ValidationError as e: + except (PermissionValidationError, ValidationError) as e: self.add_error(None, e) raise ValidationError(self.errors)Don't forget to import
PermissionValidationErroras noted in the previous comment.Based on past review comments.
🧹 Nitpick comments (4)
django/thunderstore/repository/models/team.py (1)
190-192: Harden create() against DB race conditions (map IntegrityError to ValidationError).Under concurrency, unique constraints may raise IntegrityError despite pre-validation. Map it to deterministic ValidationError for consistent API errors.
Apply this diff:
@@ @classmethod @transaction.atomic def create(cls, name, **kwargs): - team = cls.objects.create(name=name, **kwargs) - Namespace.objects.create(name=name, team=team) - return team + try: + team = cls.objects.create(name=name, **kwargs) + Namespace.objects.create(name=name, team=team) + return team + except IntegrityError: + if cls.objects.filter(name__iexact=name).exists(): + raise ValidationError("Team with this name already exists") + if Namespace.objects.filter(name__iexact=name).exists(): + raise ValidationError("Namespace with this name already exists") + raiseAnd add:
from django.db import IntegrityErrornear the imports.
django/thunderstore/api/cyberstorm/services/team.py (1)
11-14: Drop redundant ensure_user_can_access (ensure_user_can_disband already implies it).Saves one permission check/query without changing semantics.
def disband_team(agent: UserType, team: Team) -> None: - team.ensure_user_can_access(agent) team.ensure_user_can_disband(agent) team.delete()django/thunderstore/repository/forms/team.py (2)
37-39: Consider removing duplicate team name validation.Per the PR objectives to remove redundant form-side validations, this check duplicates the validation already performed by the
create_teamservice throughTeam.validate(). The service will catch and surface this error duringsave().If you prefer to keep early field-level validation for UX, the current approach is acceptable.
51-51: Preserve exception chain when re-raising.Use
raise ... from eto maintain the exception chain for better debugging:except (PermissionValidationError, ValidationError) as e: self.add_error(None, e) - raise ValidationError(self.errors) + raise ValidationError(self.errors) from eThe same pattern applies to line 177 in
DisbandTeamForm.save().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
django/thunderstore/api/cyberstorm/services/team.py(1 hunks)django/thunderstore/api/cyberstorm/tests/services/test_team_services.py(3 hunks)django/thunderstore/api/cyberstorm/tests/test_create_team.py(2 hunks)django/thunderstore/api/cyberstorm/views/team.py(2 hunks)django/thunderstore/repository/forms/team.py(3 hunks)django/thunderstore/repository/models/team.py(1 hunks)django/thunderstore/repository/tests/test_team.py(2 hunks)django/thunderstore/repository/tests/test_team_forms.py(4 hunks)django/thunderstore/repository/views/team_settings.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- django/thunderstore/api/cyberstorm/views/team.py
- django/thunderstore/repository/views/team_settings.py
🧰 Additional context used
🧬 Code graph analysis (6)
django/thunderstore/repository/tests/test_team.py (4)
django/thunderstore/repository/models/team.py (3)
Team(92-380)create(189-192)save(146-148)django/thunderstore/repository/models/namespace.py (2)
Namespace(10-44)save(42-44)django/conftest.py (1)
team(151-152)django/thunderstore/repository/factories.py (1)
NamespaceFactory(22-26)
django/thunderstore/repository/tests/test_team_forms.py (1)
django/thunderstore/repository/forms/team.py (5)
save(42-53)save(96-106)save(129-143)save(170-177)save(197-211)
django/thunderstore/repository/forms/team.py (3)
django/thunderstore/api/cyberstorm/services/team.py (5)
create_team(18-26)disband_team(11-14)remove_team_member(41-45)update_team(30-37)update_team_member(71-85)django/thunderstore/repository/models/team.py (4)
Team(92-380)TeamMember(42-79)TeamMemberRole(19-21)save(146-148)django/thunderstore/account/forms.py (3)
save(25-46)save(57-67)save(85-89)
django/thunderstore/repository/models/team.py (2)
django/conftest.py (1)
team(151-152)django/thunderstore/repository/models/namespace.py (1)
Namespace(10-44)
django/thunderstore/api/cyberstorm/tests/services/test_team_services.py (3)
django/thunderstore/api/cyberstorm/services/team.py (2)
disband_team(11-14)create_team(18-26)django/conftest.py (4)
team_owner(165-170)user(116-121)team_member(156-161)service_account(422-433)django/thunderstore/repository/models/team.py (1)
Team(92-380)
django/thunderstore/api/cyberstorm/services/team.py (2)
django/thunderstore/repository/models/team.py (6)
Team(92-380)TeamMember(42-79)TeamMemberRole(19-21)ensure_user_can_access(288-291)ensure_user_can_disband(334-340)add_member(180-185)django/thunderstore/api/cyberstorm/views/team.py (3)
delete(138-148)delete(170-173)delete(215-218)
🪛 Ruff (0.13.3)
django/thunderstore/repository/forms/team.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
42-42: Unused method argument: args
(ARG002)
42-42: Unused method argument: kwargs
(ARG002)
51-51: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
177-177: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
django/thunderstore/api/cyberstorm/services/team.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
22-22: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build docker image
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
🔇 Additional comments (3)
django/thunderstore/api/cyberstorm/tests/test_create_team.py (1)
56-56: LGTM — messages now match model validation.Also applies to: 71-72
django/thunderstore/repository/tests/test_team_forms.py (1)
76-80: LGTM — conflict message aligned to model (“Team with this name already exists”).django/thunderstore/repository/tests/test_team.py (1)
774-807: LGTM! Comprehensive test coverage for Team.create validation.The four new tests effectively cover success and failure scenarios for team creation, including duplicate detection and read-only name enforcement.
Some actions raise PermissionValidationError instead of ValidationError. Although PermissionValidationError inherits from ValidationError, tests that specifically expect these errors should use PermissionValidationError to ensure accurate validation. Refs. TS-2426
Use the team services create_team and disband_team in the form views responsible for creating and disbanding teams. Catch the errors manually which are raised in the services and add them to the form.
Remove redunant functionality from the forms, such as validations that are handled in the services and remove the save functions from the forms since this is handled by the services.
Refs. TS-2426