[SK-2671] feat(roles): add update_default_roles and list_dependent_roles#135
Merged
hrishikesh-p merged 7 commits intomainfrom Mar 24, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds RoleClient methods to update default roles and list dependent roles, and UserClient methods to list a user's roles and permissions; includes new integration tests exercising these APIs and small import/whitespace adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as UserClient/RoleClient
participant Core as CoreClient.grpc_exec
participant Service as gRPC Service
participant Backend as Backend Store
Client->>Core: build Request (UpdateDefaultRolesRequest / ListDependentRolesRequest / ListUserRolesRequest / ListUserPermissionsRequest)
Core->>Service: invoke RPC (UpdateDefaultRoles / ListDependentRoles / ListUserRoles / ListUserPermissions)
Service->>Backend: read/update role & permission data
Backend-->>Service: return payload
Service-->>Core: RPC response
Core-->>Client: parsed Response object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
scalekit/role.py (1)
372-375: Use explicitNonechecks when populating optional request fields.At Line 372 and Line 374, truthiness checks make empty-string input a silent no-op. Prefer
is not Noneso behavior is explicit.Suggested patch
- if default_creator_role: + if default_creator_role is not None: req.default_creator_role = default_creator_role - if default_member_role: + if default_member_role is not None: req.default_member_role = default_member_role🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scalekit/role.py` around lines 372 - 375, The checks that populate req.default_creator_role and req.default_member_role currently use truthiness (if default_creator_role / if default_member_role) which silently skips empty-string values; update these to explicit None checks (if default_creator_role is not None / if default_member_role is not None) in scalekit/role.py so empty strings are treated as valid values while still skipping absent (None) inputs—locate the assignment sites referencing req.default_creator_role and req.default_member_role and replace the conditional expressions accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scalekit/users.py`:
- Around line 395-433: Add explicit imports for the four protobuf symbols Ruff
flags as undefined: ListUserPermissionsRequest, ListUserPermissionsResponse,
ListUserRolesRequest, and ListUserRolesResponse from
scalekit.v1.users.users_pb2; these symbols are referenced in the methods
list_user_permissions and list_user_roles (and their use of
core_client.grpc_exec and user_service.ListUserPermissions.with_call /
ListUserRoles.with_call), so replace the star import by adding those four names
to the module imports to satisfy the linter.
In `@tests/test_role_defaults_dependent.py`:
- Around line 15-21: The test test_update_default_roles_with_member_role mutates
global environment state by calling
role.update_default_roles(default_member_role="member") without restoring the
previous default; modify the test to read and save the existing
default_member_role via role.get_default_roles (or the existing API that returns
current defaults) before calling update_default_roles, then restore the saved
value after the assertion—either using a try/finally block or self.addCleanup to
call role.update_default_roles with the original value—so the environment
default is always reverted even on test failure.
---
Nitpick comments:
In `@scalekit/role.py`:
- Around line 372-375: The checks that populate req.default_creator_role and
req.default_member_role currently use truthiness (if default_creator_role / if
default_member_role) which silently skips empty-string values; update these to
explicit None checks (if default_creator_role is not None / if
default_member_role is not None) in scalekit/role.py so empty strings are
treated as valid values while still skipping absent (None) inputs—locate the
assignment sites referencing req.default_creator_role and
req.default_member_role and replace the conditional expressions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91dfc5d8-b5fd-4fdc-9446-ee7f3759f835
📒 Files selected for processing (4)
scalekit/role.pyscalekit/users.pytests/test_role_defaults_dependent.pytests/test_user_roles_permissions.py
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
hrishikesh-p
changed the title
…ervice-update-default-roles-list-dependent
- role.py: use 'is not None' instead of truthiness for default_creator_role and default_member_role to allow empty-string values through explicitly - users.py: replace star import with explicit named imports for all pb2 symbols used in the module (Ruff F401/F403) - test_role_defaults_dependent.py: add addCleanup to restore default_member_role after test_update_default_roles_with_member_role to avoid mutating global environment state
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scalekit/role.py (1)
355-394: Resolve F405 linting by adding explicit imports for protobuf symbols.Ruff's F405 rule flags undefined names from wildcard imports. Add explicit imports for the four protobuf symbols used in this file:
Import fix
from scalekit.core import CoreClient from scalekit.v1.roles.roles_pb2 import * +from scalekit.v1.roles.roles_pb2 import ( + ListDependentRolesRequest, + ListDependentRolesResponse, + UpdateDefaultRolesRequest, + UpdateDefaultRolesResponse, +) from scalekit.v1.roles.roles_pb2_grpc import RolesServiceStub🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scalekit/role.py` around lines 355 - 394, The file is triggering F405 due to wildcard imports for protobuf symbols used in update_default_roles and list_dependent_roles; add explicit imports for UpdateDefaultRolesRequest, UpdateDefaultRolesResponse, ListDependentRolesRequest, and ListDependentRolesResponse from the generated protobuf module (rather than using a star import) so the names referenced in update_default_roles, list_dependent_roles, and the role_service.grpc calls are defined; update the import statement at the top of the module to import those four symbols explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scalekit/role.py`:
- Around line 355-394: The file is triggering F405 due to wildcard imports for
protobuf symbols used in update_default_roles and list_dependent_roles; add
explicit imports for UpdateDefaultRolesRequest, UpdateDefaultRolesResponse,
ListDependentRolesRequest, and ListDependentRolesResponse from the generated
protobuf module (rather than using a star import) so the names referenced in
update_default_roles, list_dependent_roles, and the role_service.grpc calls are
defined; update the import statement at the top of the module to import those
four symbols explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79f2becf-b69c-4d15-b576-f5e408ecac02
📒 Files selected for processing (3)
scalekit/role.pyscalekit/users.pytests/test_role_defaults_dependent.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scalekit/users.py
- tests/test_role_defaults_dependent.py
jranaskit
approved these changes
Mar 23, 2026
ScalekitClient exposes the RoleClient as self.roles (plural), not self.role. Tests were referencing the non-existent .role attribute, causing AttributeError in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
update_default_roles(default_creator_role, default_member_role)— sets environment-level default roles (PATCH /api/v1/roles:set_defaults)list_dependent_roles(role_name)— lists roles that inherit from a base role (GET /api/v1/roles/{role_name}/dependents)tests/test_role_defaults_dependent.pyNotion: (SK-2671)
Test plan
update_default_roles()— accepts no paramsupdate_default_roles(default_member_role="member")— sets member rolelist_dependent_roles("member")— returns responselist_dependent_roles("admin")— returns responseSummary by CodeRabbit
New Features
Tests