Refactor PackageListing ensure_x functions (TS-2580) #1172
Conversation
|
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. WalkthroughRefactors permission handling to a validator pattern: adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Service as API Service
participant Listing as PackageListing
participant Perm as Permissions Utils
User->>Service: request update_categories(agent, categories)
Service->>Listing: validate_update_categories_permissions(agent)
Listing->>Perm: check_user_permissions(agent)
Perm-->>Listing: PermissionResult(error?, is_public)
Listing-->>Service: PermissionResult
alt invalid
Service->>Service: validation_result.raise_if_invalid()
Service-->>User: PermissionValidationError -> error response
else valid
Service->>Listing: update_categories(agent, categories)
Service->>Service: clear cache (get_package_listing_or_404.clear_cache_with_args)
Service-->>User: success
end
sequenceDiagram
autonumber
actor Viewer as User
participant Listing as PackageListing
participant Perm as Permissions Utils
Viewer->>Listing: can_be_viewed_by_user(user)
Listing->>Listing: validate_can_be_viewed_by_user(user)
Listing->>Perm: check_user_permissions(user)
Perm-->>Listing: PermissionResult(error?, is_public)
Listing-->>Viewer: boolean (derived from PermissionResult.is_valid)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1172 +/- ##
==========================================
+ Coverage 92.23% 92.28% +0.04%
==========================================
Files 331 331
Lines 10087 10119 +32
Branches 927 933 +6
==========================================
+ Hits 9304 9338 +34
+ Misses 657 654 -3
- Partials 126 127 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
django/thunderstore/community/models/package_listing.py (2)
379-409: Consistently apply check_user_permissions and is_public semantics in view validatorUnlike the other validators, validate_can_be_viewed_by_user does not call check_user_permissions and always returns is_public=True. This can:
- Bypass the deactivated-user/service-account checks for viewing, if get_has_perms returns True for such users.
- Break consistency in how is_public is propagated to callers.
Align with the other validators by delegating to check_user_permissions first and propagating is_public.
Apply this diff:
def validate_can_be_viewed_by_user( self, user: Optional[UserType] ) -> Tuple[List[str], bool]: - errors = [] - is_public = True + errors, is_public = check_user_permissions(user) + if errors: + return errors, is_public def get_has_perms() -> bool: return ( user is not None and user.is_authenticated and ( self.community.can_user_manage_packages(user) or self.package.owner.can_user_access(user) ) ) if self.community.require_package_listing_approval: if ( self.review_status != PackageListingReviewStatus.approved and not get_has_perms() ): errors.append("Insufficient permissions to view") else: if ( self.review_status == PackageListingReviewStatus.rejected and not get_has_perms() ): errors.append("Insufficient permissions to view") return errors, is_public
321-328: Allupdate_categoriesCalls Updated; Audit Coverage Needs Review
- Verified that every call to the model’s
update_categoriesnow only passescategories—noagentargument remains.- Confirmed there are no references to legacy permission methods (
ensure_update_categories_permission,check_update_categories_permission, etc.).- Next step: if you relied on the dropped
agentparameter for audit logging of category changes, consider implementing a dedicated audit event or log entry now that the parameter is gone.
🧹 Nitpick comments (9)
django/thunderstore/permissions/utils.py (2)
21-35: Centralized permission check looks good; nit: renamepublic_errortois_publicfor consistencyEverywhere else the tuple’s second item is named/treated as
is_public. Renaming improves readability and consistency with validators returning(errors, is_public).Apply this localized rename:
def check_user_permissions( user: Optional[UserType], allow_serviceaccount: bool = False ) -> Tuple[List[str], bool]: - errors = [] - public_error = True # Set to False if you want to hide certain errors + errors: List[str] = [] + is_public = True # Set to False if you want to hide certain errors if not user or not user.is_authenticated: errors.append("Must be authenticated") elif not user.is_active: errors.append("User has been deactivated") - public_error = False + is_public = False elif hasattr(user, "service_account") and not allow_serviceaccount: errors.append("Service accounts are unable to perform this action") - return errors, public_error + return errors, is_public
7-18: DRY: Implementvalidate_userviacheck_user_permissionsAvoid duplicating the business rules and ensure error visibility stays in sync with the new validator flow.
def validate_user( user: Optional[UserType], allow_serviceaccount: bool = False ) -> UserType: - if not user or not user.is_authenticated: - raise PermissionValidationError("Must be authenticated") - if not user.is_active: - raise PermissionValidationError("User has been deactivated", is_public=False) - if hasattr(user, "service_account") and not allow_serviceaccount: - raise PermissionValidationError( - "Service accounts are unable to perform this action" - ) - return user + errors, is_public = check_user_permissions( + user, allow_serviceaccount=allow_serviceaccount + ) + if errors: + raise PermissionValidationError(errors, is_public=is_public) + # At this point user is authenticated and active + assert user is not None + return userdjango/thunderstore/api/cyberstorm/services/package_listing.py (1)
15-21: Correctly surface validator failures asPermissionValidationErrorThis mirrors the new pattern: collect
(errors, is_public)and raise when needed, else proceed to update.Optional: Strengthen type hints for
categoriesfor better static analysis.
- Signature:
# Add near the top of the file: from typing import List from thunderstore.community.models import PackageCategory # type: ignore[import] # Then update signature: def update_categories( agent: UserType, categories: List[PackageCategory], listing: PackageListing ) -> None: ...django/thunderstore/repository/tests/test_permissions_checker.py (1)
48-58: Rename mock variable to reflect the patched targetThe variable still uses the old “ensure” naming. Renaming improves clarity and avoids confusion with the deprecated API.
@patch( "thunderstore.community.models.PackageListing.validate_update_categories_permissions" ) def test_can_manage_categories( - mock_ensure_update_categories_permission, return_val, permissions_checker + mock_validate_update_categories_permissions, return_val, permissions_checker ): expected_response = len(return_val) == 0 - mock_ensure_update_categories_permission.return_value = return_val, True + mock_validate_update_categories_permissions.return_value = return_val, True assert permissions_checker.can_manage_categories == expected_responsedjango/thunderstore/community/tests/test_package_listing.py (1)
258-314: Nit: Rename test to match the new APIThe function is still named
test_package_listing_ensure_update_categories_permission, but it now exercisescan_update_categories_permission/validate_update_categories_permissions. Consider renaming totest_package_listing_update_categories_permissionsfor consistency.django/thunderstore/community/models/package_listing.py (4)
336-353: Minor: drop redundant initialization of errorserrors is immediately reassigned from check_user_permissions. Remove the unused initialization.
Apply this diff:
def validate_user_can_manage_listing( self, user: Optional[UserType] ) -> Tuple[List[str], bool]: - errors = [] - errors, is_public = check_user_permissions(user) if errors: return errors, is_public
354-371: Minor: drop redundant initialization of errorsSame as above; the local list is overwritten by check_user_permissions.
Apply this diff:
def validate_update_categories_permissions( self, user: Optional[UserType] ) -> Tuple[List[str], bool]: - errors = [] - errors, is_public = check_user_permissions(user) if errors: return errors, is_public
372-375: Naming consistency: consider simplifying can_update_categories_permissionThe method name reads a bit awkwardly. Consider a clearer boolean alias like can_update_categories or has_update_categories_permission. Keeping the current name is fine; if you rename, ensure callsites are updated or add a temporary alias for backward-compat.
410-413: Nit: simplify boolean checkReturn not errors is a bit cleaner and idiomatic.
Apply this diff:
def can_be_viewed_by_user(self, user: Optional[UserType]) -> bool: - errors, _ = self.validate_can_be_viewed_by_user(user) - return len(errors) == 0 + errors, _ = self.validate_can_be_viewed_by_user(user) + return not errors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
django/thunderstore/api/cyberstorm/services/package_listing.py(2 hunks)django/thunderstore/community/models/package_listing.py(5 hunks)django/thunderstore/community/tests/test_package_listing.py(6 hunks)django/thunderstore/permissions/utils.py(2 hunks)django/thunderstore/repository/tests/test_permissions_checker.py(1 hunks)django/thunderstore/repository/views/package/detail.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
django/thunderstore/api/cyberstorm/services/package_listing.py (2)
django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/community/models/package_listing.py (1)
validate_update_categories_permissions(354-370)
django/thunderstore/repository/tests/test_permissions_checker.py (2)
django/thunderstore/repository/views/package/detail.py (2)
permissions_checker(88-89)can_manage_categories(40-41)django/conftest.py (1)
permissions_checker(534-535)
django/thunderstore/permissions/utils.py (2)
django/conftest.py (1)
user(115-120)django/thunderstore/core/types.py (1)
UserType(13-16)
django/thunderstore/repository/views/package/detail.py (2)
django/thunderstore/community/models/package_listing.py (1)
can_update_categories_permission(372-374)django/conftest.py (1)
user(115-120)
django/thunderstore/community/tests/test_package_listing.py (3)
django/thunderstore/community/models/package_listing.py (5)
validate_can_be_viewed_by_user(379-408)can_update_categories_permission(372-374)validate_update_categories_permissions(354-370)update_categories(321-327)validate_user_can_manage_listing(336-352)django/conftest.py (6)
user(115-120)active_package_listing(229-233)package_category(272-277)TestUserTypes(550-585)get_user_by_type(564-585)service_account(406-417)django/thunderstore/community/factories.py (1)
categories(97-102)
django/thunderstore/community/models/package_listing.py (5)
django/thunderstore/permissions/utils.py (1)
check_user_permissions(21-35)django/thunderstore/core/types.py (1)
UserType(13-16)django/thunderstore/repository/models/team.py (1)
can_user_manage_packages(355-356)django/thunderstore/community/models/community.py (2)
can_user_manage_packages(283-284)can_user_manage_categories(287-288)django/thunderstore/community/consts.py (1)
PackageListingReviewStatus(4-7)
⏰ 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). (8)
- GitHub Check: Test pytest (4)
- GitHub Check: Test pytest (5)
- GitHub Check: Test pytest (6)
- GitHub Check: Test pytest (3)
- GitHub Check: Test pytest (1)
- GitHub Check: Test pytest (2)
- GitHub Check: Build docker image
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
django/thunderstore/repository/views/package/detail.py (1)
39-41: LGTM: Swap to boolean helper aligns with new validator patternUsing
can_update_categories_permission(self.user)keeps the view logic clean and matches the tuple-based validators introduced in the model.django/thunderstore/api/cyberstorm/services/package_listing.py (1)
15-21: Verify no lingering calls to removed ensure/check APIsTo avoid dead paths, scan for old API usages that should now be using the new validators or boolean helpers. Update the script so both searches run even if no matches are found:
#!/bin/bash # Expect: no matches for legacy permission helpers or obsolete validity wrappers. set -euo pipefail echo "Searching for legacy permission helpers..." rg --exit-zero -nP -C2 '\bensure_(user_can_manage_listing|update_categories_permission|can_be_viewed_by_user)\b|\bcheck_update_categories_permission\b' echo "Searching for remaining check_validity wrappers that may be obsolete..." rg --exit-zero -nP -C2 '\bcheck_validity\s*\('Rerun this locally and confirm that neither search produces output.
django/thunderstore/community/tests/test_package_listing.py (4)
187-219: Good coverage for view-permission validatorExhaustively validates
validate_can_be_viewed_by_useracross review-status, approval requirement, and role combinations. Assertions line up with the underlying rules.
281-283: Boolean helper and validator used together properlyDeriving the boolean via
can_update_categories_permissionand asserting errors fromvalidate_update_categories_permissionsmatches the new API design.
325-339: update_categories tests reflect new signature and invalid-category errorCalling
update_categories(categories=[...])and asserting the community mismatch ValidationError ensures both the happy path and guardrail are covered.
420-444: Solid validation matrix for listing management permissionsThe error and visibility mapping per user type matches
check_user_permissionssemantics (includingis_public=Falsefor deactivated users). Nicely exercises the newvalidate_user_can_manage_listing.django/thunderstore/community/models/package_listing.py (2)
1-1: Typing import updates look goodImporting Tuple aligns with the new validator return types. No issues.
19-19: Good move: centralizing auth checks via check_user_permissionsAdopting the common helper is consistent with the new validator pattern and keeps is_public semantics centralized.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
django/thunderstore/community/tests/test_package_listing.py (1)
187-236: Add missing else-assertions to cover negative paths in view-permissions testSeveral parameter combinations fall through without any assertions (e.g., authenticated regular users without roles when approval is required and the listing is not approved). This weakens the test and can hide regressions.
Add an explicit else branch in both require_approval paths to assert the negative expectation.
Apply this diff:
@@ - elif listing.package.owner.can_user_access(user): - assert result is True - assert not errors + elif listing.package.owner.can_user_access(user): + assert result is True + assert not errors + else: + assert result is False + assert expected_error in errors @@ - elif listing.package.owner.can_user_access(user): - assert result is True - assert not errors + elif listing.package.owner.can_user_access(user): + assert result is True + assert not errors + else: + assert result is False + assert expected_error in errorsAlso applies to: 237-252
♻️ Duplicate comments (1)
django/thunderstore/community/tests/test_package_listing.py (1)
258-264: Rename test to match the pluralized validator nameThe validator is
validate_update_categories_permissions(plural), but the test is named singular. Aligning the name reduces confusion when grepping and keeps parity with the public API.Apply this diff:
-def test_package_listing_validate_update_categories_permission( +def test_package_listing_validate_update_categories_permissions(Also, per earlier feedback about removing “ensure” naming from tests, please verify there are no lingering “ensure_*” usages or references to the old
check_update_categories_permissionAPI elsewhere.#!/bin/bash # Find lingering 'ensure' style names and old API in tests/repo rg -n -C2 -P '(test_.*ensure|ensure_update_categories_permission|ensure_can_be_viewed_by_user|check_update_categories_permission)'
🧹 Nitpick comments (2)
django/thunderstore/community/tests/test_package_listing.py (2)
325-339: Assert atomicity after failed category updateGreat adaptation to the new
update_categories(categories=[...])signature. Consider asserting the categories remain unchanged after the invalid update to lock in atomic behavior.Apply this diff:
with pytest.raises( ValidationError, match="Community mismatch between package listing and category", ): active_package_listing.update_categories(categories=[invalid_category]) + # Ensure categories remain unchanged after a failed update + assert active_package_listing.categories.count() == 1 + assert package_category in active_package_listing.categories.all()
420-444: Solid coverage of user-type matrix; consider adding a positive role-based caseThis validates the mapping returned from
validate_user_can_manage_listingfor all user types. To fully cover the positive permission path for regular users, add a case where a regular user has a qualifying role (e.g., team owner or community moderator) and expecterrors == []andis_public is True.Example additional test (add alongside this one):
@pytest.mark.django_db def test_package_listing_validate_user_can_manage_listing_with_roles(active_package_listing, user_factory=UserFactory): user = user_factory.create() # Grant team ownership (safe positive case) TeamMember.objects.create( user=user, team=active_package_listing.package.owner, role=TeamMemberRole.owner, ) errors, is_public = active_package_listing.validate_user_can_manage_listing(user) assert errors == [] assert is_public is True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
django/thunderstore/community/tests/test_package_listing.py(7 hunks)django/thunderstore/repository/tests/test_permissions_checker.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- django/thunderstore/repository/tests/test_permissions_checker.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
django/thunderstore/community/tests/test_package_listing.py (1)
django/thunderstore/community/models/package_listing.py (5)
validate_can_be_viewed_by_user(379-408)can_update_categories_permission(372-374)validate_update_categories_permissions(354-370)update_categories(321-327)validate_user_can_manage_listing(336-352)
⏰ 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
🔇 Additional comments (1)
django/thunderstore/community/tests/test_package_listing.py (1)
281-283: Good: dual assertions cover both boolean helper and validator outputsAsserting both
can_update_categories_permission(user)and(errors, _)fromvalidate_update_categories_permissions(user)ensures the convenience method and validator remain in sync.
Roffenlund
force-pushed
the
refactor-package-listing-ensure-x-functions
branch
from
13d7581 to
7df7b67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
django/thunderstore/community/tests/test_package_listing.py (1)
187-194: Rename propagation: ensure_ → validate_ across the repo**This file is updated, but please ensure the rename has been propagated elsewhere (e.g., tests still mentioning ensure_* as noted earlier).
Run to verify no lingering ensure_* names:
#!/bin/bash set -euo pipefail rg -nP -S --type=py -C2 '\bensure_(can|update|user|.*)\b|ensure_[a-zA-Z_]*\('
🧹 Nitpick comments (10)
django/thunderstore/permissions/utils.py (3)
8-16: Type correctness and dataclass ergonomics (use Optional[str] and slots).Make the error field Optional and enable slots for lighter instances.
-from dataclasses import dataclass +from dataclasses import dataclass -from typing import Optional +from typing import Optional, Type -@dataclass +@dataclass(slots=True) class PermissionResult: - error: str = None + error: Optional[str] = None is_public: bool = True
17-25: Minor typing nit on raise_if_invalid() signature.Annotate exc_cls for better IDE/static-analysis support.
- def raise_if_invalid(self, exc_cls=PermissionValidationError) -> None: + def raise_if_invalid(self, exc_cls: Type[Exception] = PermissionValidationError) -> None:
41-55: Optional: Delegate to validate_user for a single source of truth.This keeps messages and visibility flags in sync with validate_user and removes duplication.
-def check_user_permissions( - user: Optional[UserType], allow_serviceaccount: bool = False -) -> PermissionResult: - - result = PermissionResult() - - if not user or not user.is_authenticated: - result.error = "Must be authenticated" - elif not user.is_active: - result.error = "User has been deactivated" - result.is_public = False - elif hasattr(user, "service_account") and not allow_serviceaccount: - result.error = "Service accounts are unable to perform this action" - - return result +def check_user_permissions( + user: Optional[UserType], allow_serviceaccount: bool = False +) -> PermissionResult: + try: + validate_user(user, allow_serviceaccount=allow_serviceaccount) + return PermissionResult() + except PermissionValidationError as e: + return PermissionResult( + error=str(e), + is_public=getattr(e, "is_public", True), + )If you adopt this, please run the permissions-related test suite to confirm message text parity.
django/thunderstore/api/cyberstorm/services/package_listing.py (1)
14-18: Migration to validator pattern looks correct.Permission validated via validator + raise_if_invalid, then pure update_categories(categories=...). Matches the new API.
Consider widening the categories type to Sequence[str] if callers may pass tuples/iterables.
django/thunderstore/community/tests/test_package_listing.py (2)
220-236: Prefer strict equality for error assertionsUse exact string equality for determinism instead of substring checks.
- assert expected_error in validation_result.error + assert validation_result.error == expected_error @@ - assert expected_error in validation_result.error + assert validation_result.error == expected_errorAlso applies to: 239-252
256-257: Minor style: avoid list concatenation in parametrizeRuff RUF005: use splat to append None.
-@pytest.mark.parametrize("team_role", TeamMemberRole.options() + [None]) -@pytest.mark.parametrize("community_role", CommunityMemberRole.options() + [None]) +@pytest.mark.parametrize("team_role", [*TeamMemberRole.options(), None]) +@pytest.mark.parametrize("community_role", [*CommunityMemberRole.options(), None])django/thunderstore/permissions/tests/test_validate_user.py (1)
3-3: Remove unused import (Flake8 F401)PermissionDenied is unused.
-from django.core.exceptions import PermissionDenied, ValidationError +from django.core.exceptions import ValidationErrordjango/thunderstore/community/models/package_listing.py (3)
1-1: Drop unused Tuple importMinor cleanup per Flake8.
-from typing import TYPE_CHECKING, List, Optional, Tuple +from typing import TYPE_CHECKING, List, Optional
336-352: Validator reads clean; consistent error for insufficient listing permsConsider hoisting the error message to a shared constant to reduce string coupling with tests.
372-374: Naming nit: can_update_categories_permission → can_update_categoriesThe suffix is redundant given the return type; consider adding a friendlier alias and deprecating the current name later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
django/thunderstore/api/cyberstorm/services/package_listing.py(1 hunks)django/thunderstore/community/models/package_listing.py(5 hunks)django/thunderstore/community/tests/test_package_listing.py(6 hunks)django/thunderstore/permissions/tests/test_validate_user.py(2 hunks)django/thunderstore/permissions/utils.py(2 hunks)django/thunderstore/repository/tests/test_permissions_checker.py(2 hunks)django/thunderstore/repository/views/package/detail.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- django/thunderstore/repository/views/package/detail.py
🧰 Additional context used
🧬 Code graph analysis (6)
django/thunderstore/repository/tests/test_permissions_checker.py (3)
django/thunderstore/permissions/utils.py (1)
PermissionResult(9-24)django/thunderstore/repository/views/package/detail.py (2)
permissions_checker(87-88)can_manage_categories(39-40)django/conftest.py (1)
permissions_checker(534-535)
django/thunderstore/permissions/tests/test_validate_user.py (4)
django/thunderstore/account/models/service_account.py (1)
ServiceAccount(20-93)django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/permissions/utils.py (5)
PermissionResult(9-24)check_user_permissions(41-55)validate_user(27-38)is_valid(14-15)raise_if_invalid(17-24)django/conftest.py (2)
user(115-120)service_account(406-417)
django/thunderstore/community/tests/test_package_listing.py (2)
django/thunderstore/community/models/package_listing.py (7)
validate_can_be_viewed_by_user(378-404)approved(38-39)can_update_categories_permission(372-373)validate_update_categories_permissions(353-370)PackageListing(60-454)update_categories(321-327)validate_user_can_manage_listing(336-351)django/conftest.py (10)
user(115-120)community(267-268)package(186-187)TestUserTypes(550-585)active_package_listing(229-233)package_category(272-277)team_owner(149-154)team(135-136)get_user_by_type(564-585)service_account(406-417)
django/thunderstore/api/cyberstorm/services/package_listing.py (2)
django/thunderstore/community/models/package_listing.py (1)
validate_update_categories_permissions(353-370)django/thunderstore/permissions/utils.py (1)
raise_if_invalid(17-24)
django/thunderstore/permissions/utils.py (2)
django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/core/types.py (1)
UserType(13-16)
django/thunderstore/community/models/package_listing.py (3)
django/thunderstore/permissions/utils.py (3)
PermissionResult(9-24)check_user_permissions(41-55)is_valid(14-15)django/thunderstore/repository/models/team.py (1)
can_user_manage_packages(355-356)django/thunderstore/community/models/community.py (2)
can_user_manage_packages(283-284)can_user_manage_categories(287-288)
🪛 GitHub Actions: Build & Test
django/thunderstore/repository/tests/test_permissions_checker.py
[error] 59-59: PermissionsChecker.can_manage_categories test failed: expected '' (empty string) but got False. The test asserts can_manage_categories equals error == '' (empty).
[error] 59-59: PermissionsChecker.can_manage_categories test failed for error 'Some error': expected can_manage_categories to equal 'Some error' but got False.
🪛 Flake8 (7.2.0)
django/thunderstore/permissions/tests/test_validate_user.py
[error] 3-3: 'django.core.exceptions.PermissionDenied' imported but unused
(F401)
django/thunderstore/community/models/package_listing.py
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
🪛 Ruff (0.12.2)
django/thunderstore/community/tests/test_package_listing.py
256-256: Consider [*TeamMemberRole.options(), None] instead of concatenation
Replace with [*TeamMemberRole.options(), None]
(RUF005)
257-257: Consider [*CommunityMemberRole.options(), None] instead of concatenation
Replace with [*CommunityMemberRole.options(), None]
(RUF005)
🔇 Additional comments (10)
django/thunderstore/permissions/utils.py (1)
1-1: Import looks good.django/thunderstore/repository/tests/test_permissions_checker.py (1)
6-7: Import is correct and necessary.django/thunderstore/community/tests/test_package_listing.py (3)
281-283: Validator + boolean helper coverage looks goodAsserting both the boolean shortcut and the PermissionResult contract is clear and sufficient.
Also applies to: 307-313
324-337: Update-categories flow LGTMSignature change is reflected correctly in tests, and the negative-path ValidationError assertion matches the model guard.
419-443: Good mapping coverage for validate_user_can_manage_listingThe matrix verifies error text and is_public semantics across user types; concise and accurate.
django/thunderstore/permissions/tests/test_validate_user.py (2)
45-78: check_user_permissions tests are clearCovers unauthenticated, inactive, and service account paths with is_public semantics. Good.
80-111: PermissionResult behavior tests are solidDefault/invalid cases, is_public propagation, and custom exception path are well covered.
django/thunderstore/community/models/package_listing.py (3)
353-371: Category-permission validator matches role matrixEarly return on user validity preserves is_public semantics. Message is generic, which is fine for public disclosure.
378-405: View validator logic is straightforwardGeneric “Insufficient permissions to view” avoids disclosure. If you later differentiate unauthenticated vs. unauthorized, keep is_public=True.
321-327: Noupdate_categoriescall sites detected
Grepping the repo forupdate_categories(,agent=, and related permission helpers returned no matches. It’s safe to update the signature to acceptIterable[PackageCategory]and removeagenthandling, but please manually verify any dynamic or indirect invocations before merging.
django/thunderstore/repository/tests/test_permissions_checker.py
Outdated
Show resolved
Hide resolved
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
django/thunderstore/repository/views/package/detail.py (1)
39-41: Optional: rename for clarity and consistency.Method name mixes “can_” with “…_permission”. Consider renaming the model helper to
can_update_categories(user)(and keeping a temporary alias) to align withvalidate_update_categories_permissions. If adopted, update this call:- return self.listing.can_update_categories_permission(self.user) + return self.listing.can_update_categories(self.user)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
django/thunderstore/repository/views/package/detail.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
django/thunderstore/repository/views/package/detail.py (1)
django/thunderstore/community/models/package_listing.py (1)
can_update_categories_permission(372-373)
⏰ 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
🔇 Additional comments (2)
django/thunderstore/repository/views/package/detail.py (2)
39-41: Good swap to boolean permission helper; matches new validator pattern.Using
can_update_categories_permission(user)here is appropriate for view-layer gating and aligns with the PermissionResult-based API.
39-41: Server-side enforcement confirmed. Theupdate_categoriesservice indjango/thunderstore/api/cyberstorm/services/package_listing.pycallslisting.validate_update_categories_permissions(agent).raise_if_invalid()and its tests inapi/cyberstorm/tests/services/test_package_listing_services.pycover both success and failure cases.
Roffenlund
force-pushed
the
refactor-package-listing-ensure-x-functions
branch
from
948b048 to
5bcde37
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
django/thunderstore/repository/tests/test_permissions_checker.py (1)
50-56: Good fix: explicit expected boolean removes the flaky chained comparison.Parametrize now drives a clear True/False expectation.
🧹 Nitpick comments (1)
django/thunderstore/repository/tests/test_permissions_checker.py (1)
61-65: Tighten assertion and assert call args for stronger coupling.Use identity for booleans and verify the validator received the user.
Apply:
- assert permissions_checker.can_manage_categories == expected + assert permissions_checker.can_manage_categories is expected + mock_validate_update_categories_permission.assert_called_once_with( + permissions_checker.user + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
django/thunderstore/repository/tests/test_permissions_checker.py(2 hunks)django/thunderstore/repository/views/package/detail.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- django/thunderstore/repository/views/package/detail.py
🧰 Additional context used
🧬 Code graph analysis (1)
django/thunderstore/repository/tests/test_permissions_checker.py (3)
django/thunderstore/permissions/utils.py (1)
PermissionResult(9-24)django/thunderstore/repository/views/package/detail.py (2)
permissions_checker(87-88)can_manage_categories(39-40)django/conftest.py (1)
permissions_checker(534-535)
⏰ 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
🔇 Additional comments (2)
django/thunderstore/repository/tests/test_permissions_checker.py (2)
6-7: Import aligns with new validator pattern.Using PermissionResult directly in the test matches the refactor direction.
58-58: Patching validate_update_categories_permissions on the PackageListing class is correct. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
django/thunderstore/api/cyberstorm/services/package_listing.py (1)
14-16: Good shift to validator pattern; consider avoiding double permission checksYou validate via
validate_update_categories_permissions(...).raise_if_invalid()and then call a model method that re-validates and raises a different exception (PermissionError). Either keep both intentionally (defense-in-depth) or unify the boundary to prevent inconsistent exceptions and redundant calls. Suggest updating the model method to use the same validator (see proposed diff in the model file comment).django/thunderstore/community/tests/test_package_listing.py (1)
255-258: Ruff RUF005: replace list concatenation with starred lists in parametrizeMinor style/alloc improvement. Apply here (and similarly above where used):
-@pytest.mark.parametrize("team_role", TeamMemberRole.options() + [None]) -@pytest.mark.parametrize("community_role", CommunityMemberRole.options() + [None]) +@pytest.mark.parametrize("team_role", [*TeamMemberRole.options(), None]) +@pytest.mark.parametrize("community_role", [*CommunityMemberRole.options(), None])django/thunderstore/community/models/package_listing.py (3)
1-1: Remove unused import (Tuple)
Tupleis unused; flake8 flags this.-from typing import TYPE_CHECKING, List, Optional, Tuple +from typing import TYPE_CHECKING, List, Optional
321-331: Consider wrapping category updates in a transaction
self.categories.set(...)performs multiple queries. Adding@transaction.atomichere makes the method safe when called outside service-layer transactions.- def update_categories(self, agent: UserType, categories: List["PackageCategory"]): + @transaction.atomic + def update_categories(self, agent: UserType, categories: List["PackageCategory"]):
381-407: Optional: align visibility of errors with user-state checks
validate_can_be_viewed_by_userskipscheck_user_permissions, so deactivated/service-account viewers get a public error by default. If you want error visibility to mirror other validators, consider invokingcheck_user_permissionsfirst and short-circuiting on invalid results (to setis_public=Falsefor deactivated).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
django/thunderstore/api/cyberstorm/services/package_listing.py(1 hunks)django/thunderstore/community/models/package_listing.py(5 hunks)django/thunderstore/community/tests/test_package_listing.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
django/thunderstore/api/cyberstorm/services/package_listing.py (2)
django/thunderstore/community/models/package_listing.py (1)
validate_update_categories_permissions(356-373)django/thunderstore/permissions/utils.py (1)
raise_if_invalid(17-24)
django/thunderstore/community/models/package_listing.py (3)
django/thunderstore/permissions/utils.py (3)
PermissionResult(9-24)check_user_permissions(41-55)is_valid(14-15)django/thunderstore/repository/models/team.py (1)
can_user_manage_packages(355-356)django/thunderstore/community/models/community.py (2)
can_user_manage_packages(283-284)can_user_manage_categories(287-288)
django/thunderstore/community/tests/test_package_listing.py (3)
django/thunderstore/community/models/package_listing.py (7)
validate_can_be_viewed_by_user(381-407)approved(38-39)can_update_categories_permission(375-376)validate_update_categories_permissions(356-373)PackageListing(60-457)update_categories(321-330)validate_user_can_manage_listing(339-354)django/conftest.py (6)
user(115-120)community(267-268)package(186-187)TestUserTypes(550-585)get_user_by_type(564-585)service_account(406-417)django/thunderstore/api/cyberstorm/services/package_listing.py (1)
update_categories(11-23)
🪛 Flake8 (7.2.0)
django/thunderstore/community/models/package_listing.py
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
🪛 Ruff (0.12.2)
django/thunderstore/community/tests/test_package_listing.py
256-256: Consider [*TeamMemberRole.options(), None] instead of concatenation
Replace with [*TeamMemberRole.options(), None]
(RUF005)
257-257: Consider [*CommunityMemberRole.options(), None] instead of concatenation
Replace with [*CommunityMemberRole.options(), None]
(RUF005)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
django/thunderstore/community/tests/test_package_listing.py (4)
187-236: LGTM: viewing-permission tests now assert exact error stringsSwitch to equality checks keeps tests precise and consistent with the new validator API.
281-313: LGTM: boolean helper + validator assertions cover both pathsThe combined assertions on
can_updateandvalidation_result.errorare clear and map to role permutations correctly.
321-341: Solid coverage for update flow and community-mismatch validationPositive path and mismatch failure are both exercised; nice.
423-447: LGTM: management-permission validator matrix is preciseChecking both
errorandis_publicper user type aligns withcheck_user_permissionssemantics.
Implement PermissionResult class to encapsulate permission validation results in order to provide a consistent pattern for returning and validating results from validation functions. Introduce check_user_permissions to return a PermissionResult instead of raising exceptions immediately, complementing the existing validate_user function. This provides flexibility in handling permission checks in both API and form views while keeping a shared validation pattern. Refs. TS-2580
Implement new functions responsible for returning error information instead of raising errors in the PackageListing model. Refs. TS-2580
Update the update_categories to use validate_update_categories_permissions. Refs. TS-2580
Update can_manage_categories to use can_update_categories_permission. Refs. TS-2580
Add the agent parameter back to PacakgeListing.update_categories and add a validation check. Raise a core PermissionError instead of raising any type of validation errors in order to defend against misusing this function. Refs. TS-2580
Roffenlund
force-pushed
the
refactor-package-listing-ensure-x-functions
branch
from
2f3c680 to
b75856c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
django/thunderstore/community/tests/test_package_listing.py (1)
187-236: Validator-based assertions and exact error matching look solidGood switch from exception-catching to validator results, and from substring checks to exact equality for errors. This aligns with prior feedback and prevents false positives.
Also applies to: 237-252
django/thunderstore/community/models/package_listing.py (1)
321-324: Unify permission enforcement with validator; avoid generic PermissionErrorCurrent code rechecks permissions and raises a bare
PermissionError, diverging from the new validator pattern and losing error context.Apply:
def update_categories(self, agent: UserType, categories: List["PackageCategory"]): - if not self.can_update_categories_permission(agent): - raise PermissionError() + # Enforce via validator to keep semantics and error messages consistent + self.validate_update_categories_permissions(agent).raise_if_invalid()
🧹 Nitpick comments (3)
django/thunderstore/community/tests/test_package_listing.py (1)
185-186: Adopt unpacking over list concatenation in parametrize decorators (RUF005)Use iterable unpacking to avoid creating a temporary list and to satisfy linters.
Apply this diff:
-@pytest.mark.parametrize("team_role", TeamMemberRole.options() + [None]) -@pytest.mark.parametrize("community_role", CommunityMemberRole.options() + [None]) +@pytest.mark.parametrize("team_role", [*TeamMemberRole.options(), None]) +@pytest.mark.parametrize("community_role", [*CommunityMemberRole.options(), None])And similarly here:
-@pytest.mark.parametrize("team_role", TeamMemberRole.options() + [None]) -@pytest.mark.parametrize("community_role", CommunityMemberRole.options() + [None]) +@pytest.mark.parametrize("team_role", [*TeamMemberRole.options(), None]) +@pytest.mark.parametrize("community_role", [*CommunityMemberRole.options(), None])Also applies to: 256-257
django/thunderstore/community/models/package_listing.py (2)
1-1: Remove unused Tuple import (F401)
Tupleisn’t used; drop it to appease flake8.-from typing import TYPE_CHECKING, List, Optional, Tuple +from typing import TYPE_CHECKING, List, Optional
375-377: Name consistency: singular vs plural
validate_update_categories_permissions(plural) vscan_update_categories_permission(singular) is slightly inconsistent. Consider aligning both to singular or both to plural for discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
django/thunderstore/api/cyberstorm/services/package_listing.py(1 hunks)django/thunderstore/community/models/package_listing.py(5 hunks)django/thunderstore/community/tests/test_package_listing.py(6 hunks)django/thunderstore/permissions/tests/test_validate_user.py(2 hunks)django/thunderstore/permissions/utils.py(2 hunks)django/thunderstore/repository/tests/test_permissions_checker.py(2 hunks)django/thunderstore/repository/views/package/detail.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- django/thunderstore/api/cyberstorm/services/package_listing.py
- django/thunderstore/repository/views/package/detail.py
- django/thunderstore/permissions/utils.py
- django/thunderstore/repository/tests/test_permissions_checker.py
🧰 Additional context used
🧬 Code graph analysis (3)
django/thunderstore/community/models/package_listing.py (3)
django/thunderstore/permissions/utils.py (3)
PermissionResult(9-24)check_user_permissions(41-55)is_valid(14-15)django/thunderstore/repository/models/team.py (1)
can_user_manage_packages(355-356)django/thunderstore/community/models/community.py (2)
can_user_manage_packages(283-284)can_user_manage_categories(287-288)
django/thunderstore/permissions/tests/test_validate_user.py (3)
django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/permissions/utils.py (5)
PermissionResult(9-24)check_user_permissions(41-55)validate_user(27-38)is_valid(14-15)raise_if_invalid(17-24)django/conftest.py (2)
user(115-120)service_account(406-417)
django/thunderstore/community/tests/test_package_listing.py (2)
django/thunderstore/community/models/package_listing.py (7)
validate_can_be_viewed_by_user(381-407)approved(38-39)can_update_categories_permission(375-376)validate_update_categories_permissions(356-373)PackageListing(60-457)update_categories(321-330)validate_user_can_manage_listing(339-354)django/conftest.py (9)
user(115-120)community(267-268)package(186-187)TestUserTypes(550-585)active_package_listing(229-233)team_owner(149-154)team(135-136)get_user_by_type(564-585)service_account(406-417)
🪛 Flake8 (7.2.0)
django/thunderstore/community/models/package_listing.py
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
django/thunderstore/permissions/tests/test_validate_user.py
[error] 3-3: 'django.core.exceptions.PermissionDenied' imported but unused
(F401)
🪛 Ruff (0.12.2)
django/thunderstore/community/tests/test_package_listing.py
256-256: Consider [*TeamMemberRole.options(), None] instead of concatenation
Replace with [*TeamMemberRole.options(), None]
(RUF005)
257-257: Consider [*CommunityMemberRole.options(), None] instead of concatenation
Replace with [*CommunityMemberRole.options(), None]
(RUF005)
⏰ 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
🔇 Additional comments (18)
django/thunderstore/permissions/tests/test_validate_user.py (10)
6-11: Imports for new permission API look correct.The tests exercise
PermissionResult,check_user_permissions, andvalidate_useras intended. No issues.
45-51: Good coverage for unauthenticated users.Covers both
NoneandAnonymousUser()and assertsis_publicstays True.
53-62: Inactive user path validated, including is_public=False.Asserts the privacy flag; nice.
64-70: Service account allowed case looks good.Confirms no error and public visibility.
72-78: Service account disallowed case looks good.Asserts error and default public visibility.
80-85: Default PermissionResult state verified.Checks validity, error, and visibility defaults.
94-97: Valid-case no-raise behavior is correct.Covers the non-exception path.
99-105: Exception propagation preserves is_public correctly.Good assertion on
is_publicparity.
107-110: Custom exception path verified.Confirms message forwarding when using non-
PermissionValidationError.
87-92: No action required — PermissionResult already accepts kwargsdjango/thunderstore/permissions/utils.py declares @DataClass class PermissionResult with error: Optional[str] = None and is_public: bool = True, so PermissionResult(error="...") is valid.
django/thunderstore/community/tests/test_package_listing.py (4)
258-313: Permission test covers both boolean and detailed error pathsExercising both
can_update_categories_permissionandvalidate_update_categories_permissionswith an explicit error map is clear and robust.
321-341: Happy path and community mismatch checks look correctUsing a team owner to pass permissions and asserting the community mismatch ValidationError matches model behavior.
423-447: Good coverage ofis_publicsemanticsAsserting both
errorandis_publicper user type ensures the validator contract remains stable.
187-187: Re-run repo-wide search for lingering "ensure" references (rg skipped files)Run this from the repo root and paste the output:
#!/bin/bash set -euo pipefail # Include ignored/hidden files so ripgrep doesn't skip matches rg -nP --hidden -uu -S '(?i)test_.*ensure|ensure_(can|update|user)|ensure.*permission' || true # Check specific prior mentions rg -n --hidden -uu -S 'test_package_listing_ensure_update_categories_permission' || true rg -n --hidden -uu -S 'test_permissions_checker\.py' || true rg -n --hidden -uu -S 'test_durations' || truedjango/thunderstore/community/models/package_listing.py (4)
339-355: Validator for “manage listing” reads clean and preserves detailed errorsThe flow
check_user_permissions→ role checks → specific message is consistent with the new pattern.
356-377: Category-permission validator matches policy and test expectationsDenies unauth, deactivated, and service accounts (by default), and allows owner/member/janitor/moderator paths. Looks good.
381-407: Clarify viewing policy for deactivated/service accountsThis validator doesn’t call
check_user_permissions, so a deactivated authenticated user with roles could still be allowed to view. If that’s intentional, consider a brief docstring note; if not, reusecheck_user_permissionshere too.Would you like this validator to respect
is_active/service-account restrictions as well?
409-411: Boolean helper correctly delegates to validatorGood thin wrapper.
|
|
||
| def check_update_categories_permission(self, user: Optional[UserType]) -> bool: | ||
| return check_validity(lambda: self.ensure_update_categories_permission(user)) | ||
| def can_update_categories_permission(self, user: Optional[UserType]) -> bool: |
There was a problem hiding this comment.
This should probably be called can_update_categories(), as can_update_categories_permission() could imply that it's determining whether the user can update who has that permission.
Refactor ensure_x functions in PackageListing. Update logic and tests that depended on the old ensure_x
implementations.
Refs. TS-2580
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests