-
Notifications
You must be signed in to change notification settings - Fork 29
Add validation checks to create_team service #1192
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.
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
base: master
Are you sure you want to change the base?
Conversation
Add validation checks for checking if Team and Namespace already exists to create_team service in order to catch these potential validation errors before reaching the model. Refs. TS-2739
WalkthroughThe Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1192 +/- ##
=======================================
Coverage 92.81% 92.81%
=======================================
Files 337 337
Lines 10355 10361 +6
Branches 937 939 +2
=======================================
+ Hits 9611 9617 +6
Misses 617 617
Partials 127 127 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
django/thunderstore/api/cyberstorm/services/team.py (1)
25-28: Consider whether service-layer validation duplication is necessary.The validation checks you've added duplicate the logic in
Team.validate()(lines 127-131 inteam.py), which is already called viaTeam.create() → save() → validate()at line 30. While your PR objectives state this is intentional to catch errors before the model layer, consider whether:
- Relying solely on the model validation would be sufficient
- If service-layer validation is required, extracting the validation logic into a shared method would reduce duplication and maintenance burden
The current implementation works correctly but violates DRY. The trade-off is defense-in-depth vs. code duplication.
Static analysis note: Ruff flags the inline error messages (TRY003). If you proceed with service-layer validation, consider extracting messages to constants or custom exception classes for consistency, though this is consistent with the existing model-layer code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
django/thunderstore/api/cyberstorm/services/team.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
django/thunderstore/api/cyberstorm/services/team.py (4)
django/thunderstore/account/models/service_account.py (1)
ServiceAccount(20-93)django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/repository/models/team.py (2)
Team(92-380)TeamMember(42-79)django/thunderstore/repository/models/namespace.py (1)
Namespace(10-44)
🪛 Ruff (0.14.0)
django/thunderstore/api/cyberstorm/services/team.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
28-28: 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 (1)
django/thunderstore/api/cyberstorm/services/team.py (1)
1-1: LGTM!Imports are correct and necessary for the validation logic.
Also applies to: 8-8
Add validation checks for checking if Team and Namespace already exists to create_team service in order to catch these potential validation errors before reaching the model.
Refs. TS-2739