baseapp-profiles: edit member role #192
Conversation
…BA-1863-be-multiple-profiles-edit-member-role
…BA-1863-be-multiple-profiles-edit-member-role
hpmoreira05
requested review from
nossila,
Hercilio1,
anicioalexandre and
vitorguima
WalkthroughThis pull request introduces a new mutation class Changes
Possibly related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
baseapp-profiles/baseapp_profiles/permissions.py (3)
Line range hint
23-31: Consider adding docstring to clarify permission hierarchy.The permission logic handles two different cases:
- Profile owners can always modify their profiles
- Users with explicit permissions can modify any profile
Consider documenting this hierarchy for better maintainability.
if perm in ["baseapp_profiles.change_profile", "baseapp_profiles.delete_profile"]: + """ + Permission hierarchy for profile modification: + 1. Profile owners can always modify their own profiles + 2. Users with explicit permissions can modify any profile + """ if user_obj.is_authenticated and isinstance(obj, Profile):🧰 Tools
🪛 Ruff (0.8.0)
33-34: Use a single
ifstatement instead of nestedifstatements(SIM102)
40-41: Use a single
ifstatement instead of nestedifstatements(SIM102)
45-46: Use a single
ifstatement instead of nestedifstatements(SIM102)
33-38: Simplify nested conditions while maintaining readability.The permission check is logically correct but can be more concise.
- if perm == "baseapp_profiles.use_profile" and obj: - if isinstance(obj, Profile): + if perm == "baseapp_profiles.use_profile" and isinstance(obj, Profile): return ( obj.owner_id == user_obj.id or obj.members.filter(user_id=user_obj.id).exists() )🧰 Tools
🪛 Ruff (0.8.0)
33-34: Use a single
ifstatement instead of nestedifstatements(SIM102)
Line range hint
1-59: Consider implementing a permission registry pattern.As the number of permissions grows, maintaining them in conditional blocks becomes harder to manage and test. Consider implementing a permission registry pattern where permissions are declared declaratively and can be easily tested in isolation.
This would:
- Make permissions more maintainable
- Reduce the risk of inconsistencies (like the duplicate delete permission)
- Make it easier to test each permission independently
Would you like me to provide an example implementation of a permission registry pattern?
🧰 Tools
🪛 Ruff (0.8.0)
33-34: Use a single
ifstatement instead of nestedifstatements(SIM102)
40-41: Use a single
ifstatement instead of nestedifstatements(SIM102)
45-46: Use a single
ifstatement instead of nestedifstatements(SIM102)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (1)
130-142: Consider enhancing the ordering logic robustnessWhile the implementation is functional, consider these improvements:
- Use enum values from
ProfileRoleStatusinstead of hardcoding status values- Add validation for
order_by_statusparameter valuesHere's a suggested improvement:
def resolve_members(cls, instance, info, order_by_status=None, **kwargs): if not info.context.user.has_perm("baseapp_profiles.view_profile_members", instance): return instance.members.none() members_queryset = instance.members.all() - if order_by_status == "custom": + VALID_ORDER_BY_STATUS = ["custom"] + if order_by_status and order_by_status not in VALID_ORDER_BY_STATUS: + raise ValueError(f"Invalid order_by_status value. Allowed values: {VALID_ORDER_BY_STATUS}") + + if order_by_status == VALID_ORDER_BY_STATUS[0]: # "custom" status_order = Case( - When(status=ProfileUserRole.ProfileRoleStatus.PENDING.value, then=Value(1)), - When(status=ProfileUserRole.ProfileRoleStatus.INACTIVE.value, then=Value(2)), - When(status=ProfileUserRole.ProfileRoleStatus.ACTIVE.value, then=Value(3)), + When(status=ProfileUserRole.ProfileRoleStatus.PENDING, then=Value(1)), + When(status=ProfileUserRole.ProfileRoleStatus.INACTIVE, then=Value(2)), + When(status=ProfileUserRole.ProfileRoleStatus.ACTIVE, then=Value(3)), default=Value(4), output_field=IntegerField(), ) members_queryset = members_queryset.order_by(status_order) return members_querysetbaseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (3)
37-63: Remove commented out GraphQL mutation.The commented out version of
PROFILE_ROLE_UPDATE_GRAPHQLshould be removed to maintain code cleanliness. If this represents an alternative implementation that might be needed later, consider tracking it in a separate issue or documentation.
248-269: Enhance test robustness with additional assertions.The test should verify:
- The initial role state before the update
- That the role was actually persisted in the database after the update
def test_user_profile_owner_can_update_role(django_user_client, graphql_user_client): perm = Permission.objects.get( content_type__app_label="baseapp_profiles", codename="change_profileuserrole" ) user = django_user_client.user user_2 = UserFactory() user.user_permissions.add(perm) profile = ProfileFactory(owner=user) - ProfileUserRoleFactory(profile=profile, user=user_2, role=ProfileUserRole.ProfileRoles.MANAGER) + profile_user_role = ProfileUserRoleFactory(profile=profile, user=user_2, role=ProfileUserRole.ProfileRoles.MANAGER) + + # Verify initial state + assert profile_user_role.role == ProfileUserRole.ProfileRoles.MANAGER response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={"input": {"userId": user_2.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"}}, ) content = response.json() assert content["data"]["roleUpdate"]["profileUserRole"]["role"] == "ADMIN" - profile.refresh_from_db() + profile_user_role.refresh_from_db() + assert profile_user_role.role == ProfileUserRole.ProfileRoles.ADMIN
270-293: Remove unnecessary database refresh and enhance assertions.
- The
profile.refresh_from_db()call is unnecessary as we're not modifying the profile- Similar to the previous test, we should verify the initial state and database persistence
def test_user_with_permission_can_update_role(django_user_client, graphql_user_client): perm = Permission.objects.get( content_type__app_label="baseapp_profiles", codename="change_profileuserrole" ) user = django_user_client.user user.user_permissions.add(perm) user_2 = UserFactory() user_3 = UserFactory() profile = ProfileFactory(owner=user_2) ProfileUserRoleFactory(profile=profile, user=user, role=ProfileUserRole.ProfileRoles.ADMIN) - ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER) + profile_user_role = ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER) + + # Verify initial state + assert profile_user_role.role == ProfileUserRole.ProfileRoles.MANAGER response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={"input": {"userId": user_3.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"}}, ) content = response.json() assert content["data"]["roleUpdate"]["profileUserRole"]["role"] == "ADMIN" - profile.refresh_from_db() + profile_user_role.refresh_from_db() + assert profile_user_role.role == ProfileUserRole.ProfileRoles.ADMIN
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
baseapp-profiles/baseapp_profiles/graphql/mutations.py(2 hunks)baseapp-profiles/baseapp_profiles/graphql/object_types.py(3 hunks)baseapp-profiles/baseapp_profiles/permissions.py(4 hunks)baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
baseapp-profiles/baseapp_profiles/graphql/mutations.py
138-138: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
baseapp-profiles/baseapp_profiles/permissions.py
33-34: Use a single if statement instead of nested if statements
(SIM102)
40-41: Use a single if statement instead of nested if statements
(SIM102)
54-55: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (7)
baseapp-profiles/baseapp_profiles/permissions.py (3)
3-3: LGTM!
The import is necessary for the new role-based permission checks.
Line range hint 45-52: LGTM!
The permission check correctly handles all required cases for viewing profile members:
- Profile owners
- Superusers
- Profile members
🧰 Tools
🪛 Ruff (0.8.0)
54-55: Use a single if statement instead of nested if statements
(SIM102)
54-58: Verify role constants and consider edge cases.
The permission check correctly restricts role changes to profile owners and admin members. However:
- Let's verify the role constant exists
- Consider preventing users from changing their own role to avoid privilege escalation
✅ Verification successful
Role constants verified, consider adding self-role change protection
The ProfileUserRole.ProfileRoles.ADMIN constant is correctly defined and used in the codebase. The permission check implementation is sound, but could be improved:
- The
ADMINrole is properly defined asADMIN = 1, _("admin")in theProfileRolesclass - The constant is consistently used across the codebase for admin role checks
- Consider adding a check to prevent users from modifying their own role to avoid privilege escalation:
if isinstance(obj, ProfileUserRole) and obj.user_id == user_obj.id:
return False🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify ProfileUserRole.ProfileRoles.ADMIN constant exists
ast-grep --pattern 'class ProfileRoles:
$$$
ADMIN = $_
$$$' baseapp_profiles/models.py
Length of output: 168
Script:
#!/bin/bash
# Let's first find the correct file containing ProfileUserRole
fd -t f "ProfileUserRole" --exec grep -l "ProfileUserRole"
# Then search for ProfileRoles and ADMIN constant
fd -t f "ProfileRoles" --exec grep -A 5 "ProfileRoles"
Length of output: 109
Script:
#!/bin/bash
# Try using ripgrep to find ProfileUserRole and ProfileRoles
rg "class ProfileUserRole|class ProfileRoles" -A 5
# Also search for ADMIN constant usage
rg "ProfileUserRole.*ADMIN|ProfileRoles.*ADMIN"
Length of output: 2698
🧰 Tools
🪛 Ruff (0.8.0)
54-55: Use a single if statement instead of nested if statements
(SIM102)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (2)
11-11: LGTM: Import additions are appropriate
The new imports are necessary for implementing the custom ordering functionality and are correctly grouped with related Django model imports.
126-129: LGTM: Permission check is properly implemented
The permission check ensures that only authorized users can view profile members, which aligns with the PR's security requirements.
baseapp-profiles/baseapp_profiles/graphql/mutations.py (2)
140-144: Permission check is appropriate
The permission check correctly ensures that only users with the change_profileuserrole permission can perform this action.
235-235: Added role_update mutation to ProfilesMutations
The role_update mutation field is properly added to the ProfilesMutations class.
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Outdated
Show resolved
Hide resolved
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
baseapp-profiles/baseapp_profiles/graphql/mutations.py (1)
142-146: Consider adding specific error codes for different scenariosTo support the UI requirements (confirmation dialog for Admin role changes), consider adding specific error codes that the frontend can use to handle different scenarios appropriately.
Example implementation:
if not info.context.user.has_perm("baseapp_profiles.change_profileuserrole", obj.profile): raise GraphQLError( str(_("You don't have permission to perform this action")), - extensions={"code": "permission_required"}, + extensions={ + "code": "permission_denied", + "reason": "insufficient_permissions" + }, )Consider adding these error codes:
- insufficient_permissions
- self_modification_denied
- invalid_role_transition
- role_not_found
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (2)
37-63: Remove commented GraphQL query.The commented-out version of
PROFILE_ROLE_UPDATE_GRAPHQLshould be removed to maintain code cleanliness. If this query variation is needed for future reference, consider documenting it in the project's documentation instead.
250-273: Enhance test_user_profile_owner_can_update_role with state validation.The test should verify the initial state before the role update and ensure the transition is correct.
def test_user_profile_owner_can_update_role(django_user_client, graphql_user_client): perm = Permission.objects.get( content_type__app_label="baseapp_profiles", codename="change_profileuserrole" ) user = django_user_client.user user_2 = UserFactory() user.user_permissions.add(perm) profile = ProfileFactory(owner=user) - ProfileUserRoleFactory(profile=profile, user=user_2, role=ProfileUserRole.ProfileRoles.MANAGER) + profile_user_role = ProfileUserRoleFactory( + profile=profile, + user=user_2, + role=ProfileUserRole.ProfileRoles.MANAGER + ) + + # Verify initial state + assert profile_user_role.role == ProfileUserRole.ProfileRoles.MANAGER response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={ "input": {"userId": user_2.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"} }, ) content = response.json() assert content["data"]["roleUpdate"]["profileUserRole"]["role"] == "ADMIN" - profile.refresh_from_db() + + # Verify final state directly through the role object + profile_user_role.refresh_from_db() + assert profile_user_role.role == ProfileUserRole.ProfileRoles.ADMIN
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
baseapp-profiles/baseapp_profiles/graphql/mutations.py(2 hunks)baseapp-profiles/baseapp_profiles/permissions.py(2 hunks)baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
baseapp-profiles/baseapp_profiles/graphql/mutations.py
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
baseapp-profiles/baseapp_profiles/permissions.py
51-52: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (9)
baseapp-profiles/baseapp_profiles/permissions.py (2)
4-4: LGTM!
The import is necessary for the new role-based permission checks.
Line range hint 39-41: Remove duplicate delete permission check.
The delete permission check here conflicts with the more comprehensive check in lines 24-32, potentially leading to inconsistent behavior.
- if perm == "baseapp_profiles.delete_profile" and obj:
- if isinstance(obj, Profile):
- return obj.owner_id == user_obj.idbaseapp-profiles/baseapp_profiles/graphql/mutations.py (4)
117-117: LGTM! Good use of model enum
The GraphQL enum is correctly derived from the model's ProfileRoles enum, ensuring consistency between the API schema and database.
124-126: Make role_type a required input field
The role_type field should be required since it's essential for role updates.
237-237: LGTM! Mutation properly exposed
The role_update mutation is correctly added to the ProfilesMutations class.
139-140: 🛠️ Refactor suggestion
Improve exception handling
Use raise ... from err to preserve the exception context for better debugging.
- except ProfileUserRole.DoesNotExist:
- raise ValueError(_("Role not found"))
+ except ProfileUserRole.DoesNotExist as err:
+ raise ValueError(_("Role not found")) from errLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (3)
324-324: Remove debug print statement.
Remove the print(content) statement as it was likely used for debugging purposes.
250-326: Add missing test cases for edge scenarios.
The test suite should be expanded to include the following scenarios:
- Attempting to update role with invalid role type
- Attempting to update one's own role
- Attempting to update role for non-existent user/profile
- Attempting to update role for user not in profile
Would you like me to provide implementations for these test cases?
302-326: 🛠️ Refactor suggestion
Enhance negative test case with role state validation.
The test should verify that the role remains unchanged after the failed update attempt.
def test_user_without_permission_cant_update_role(django_user_client, graphql_user_client):
perm = Permission.objects.get(
content_type__app_label="baseapp_profiles", codename="change_profileuserrole"
)
user = django_user_client.user
user.user_permissions.add(perm)
user_2 = UserFactory()
user_3 = UserFactory()
profile = ProfileFactory(owner=user_2)
ProfileUserRoleFactory(profile=profile, user=user, role=ProfileUserRole.ProfileRoles.MANAGER)
- ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER)
+ profile_user_role = ProfileUserRoleFactory(
+ profile=profile,
+ user=user_3,
+ role=ProfileUserRole.ProfileRoles.MANAGER
+ )
+ initial_role = profile_user_role.role
response = graphql_user_client(
PROFILE_ROLE_UPDATE_GRAPHQL,
variables={
"input": {"userId": user_3.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"}
},
)
content = response.json()
- print(content)
assert content["errors"][0]["message"] == "You don't have permission to perform this action"
- profile.refresh_from_db()
+
+ # Verify role remains unchanged
+ profile_user_role.refresh_from_db()
+ assert profile_user_role.role == initial_roleLikely invalid or redundant comment.
…BA-1863-be-multiple-profiles-edit-member-role
…BA-1863-be-multiple-profiles-edit-member-role
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (1)
Line range hint
134-138: Implement theorder_by_statusparameter logicThe
order_by_statusparameter is accepted but not utilized in the implementation. Based on the PR objectives for role management, proper ordering of members by status is important for a better user experience.Consider implementing the ordering logic like this:
@classmethod def resolve_members(cls, instance, info, order_by_status=None, **kwargs): if not info.context.user.has_perm("baseapp_profiles.view_profile_members", instance): return instance.members.none() - return instance.members.all() + members = instance.members.all() + if order_by_status: + status_order = { + 'ACTIVE': 1, + 'PENDING': 2, + 'INACTIVE': 3 + } + members = sorted( + members, + key=lambda x: status_order.get(x.status, 999) + ) + return members
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
baseapp-profiles/baseapp_profiles/graphql/object_types.py(1 hunks)
🔇 Additional comments (1)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (1)
Line range hint 134-138: Missing test coverage for order_by_status parameter
While the resolver implementation shows proper permission handling, there appears to be no test coverage validating the order_by_status functionality.
Let's verify the test coverage:
#!/bin/bash
# Description: Check for test coverage of the order_by_status parameter
# Look for test files containing order_by_status tests
rg -g "*test*.py" "order_by_status.*test"
# Look for GraphQL query definitions testing member ordering
rg -g "*test*.py" "members.*order"
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
baseapp-profiles/baseapp_profiles/graphql/mutations.py(3 hunks)baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py(3 hunks)baseapp-profiles/setup.cfg(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- baseapp-profiles/setup.cfg
🧰 Additional context used
🪛 Ruff (0.8.2)
baseapp-profiles/baseapp_profiles/graphql/mutations.py
139-139: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (4)
baseapp-profiles/baseapp_profiles/graphql/mutations.py (3)
122-125:
Make role_type a required field
The role_type field should be required since it's necessary for role updates.
Apply this diff:
- role_type = graphene.Field(ProfileRoleTypesEnum)
+ role_type = graphene.Field(ProfileRoleTypesEnum, required=True)Likely invalid or redundant comment.
138-139: 🛠️ Refactor suggestion
Preserve exception context using raise from
When re-raising exceptions, use raise ... from to maintain the traceback.
Apply this diff:
- raise ValueError(_("Role not found"))
+ raise ValueError(_("Role not found")) from NoneLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
139-139: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
146-148:
Add validation for role_type value and self-modification
The current implementation lacks important validations:
- No check if role_type is None
- No prevention of self-role modification
- No validation of role_type value against allowed transitions
Apply this diff:
+ if role_type is None:
+ raise ValueError(_("Role type is required"))
+
+ if obj.user_id == info.context.user.id:
+ raise GraphQLError(
+ str(_("You cannot modify your own role")),
+ extensions={"code": "permission_denied"},
+ )
+
obj.role = role_type
obj.save()Likely invalid or redundant comment.
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (1)
274-298: 🛠️ Refactor suggestion
Remove debugging print and enhance error case validation
The test should be improved by:
- Removing the
print(content)debugging statement - Adding assertion to verify the role remains unchanged
Apply this diff:
profile = ProfileFactory(owner=user_2)
ProfileUserRoleFactory(profile=profile, user=user, role=ProfileUserRole.ProfileRoles.MANAGER)
- ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER)
+ profile_user_role = ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER)
+ initial_role = profile_user_role.role
response = graphql_user_client(
PROFILE_ROLE_UPDATE_GRAPHQL,
variables={"input": {"userId": user_3.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"}},
)
content = response.json()
- print(content)
assert content["errors"][0]["message"] == "You don't have permission to perform this action"
- profile.refresh_from_db()
+
+ # Verify role remains unchanged
+ profile_user_role.refresh_from_db()
+ assert profile_user_role.role == initial_roleLikely invalid or redundant comment.
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Outdated
Show resolved
Hide resolved
nossila
changed the title
nossila
changed the title
…BA-1863-be-multiple-profiles-edit-member-role
…BA-1863-be-multiple-profiles-edit-member-role
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (2)
37-51: 🛠️ Refactor suggestionAdd role type validation in test cases
The GraphQL mutation structure looks good, but based on the PR objectives, we should add test cases to validate that:
- Only 'ADMIN' and 'MANAGER' role types are accepted
- Invalid role types are properly rejected
222-297:⚠️ Potential issueAdd missing test scenarios
Add test cases for the following scenarios:
- Member removal functionality
- Attempting to modify one's own role
- Attempting to modify roles for non-existent users
- Attempting to modify roles for users not in the profile
Example test structure for member removal:
def test_admin_can_remove_member(django_user_client, graphql_user_client): user = django_user_client.user user_2 = UserFactory() profile = ProfileFactory(owner=user) profile_user_role = ProfileUserRoleFactory( profile=profile, user=user_2, role=ProfileUserRole.ProfileRoles.MANAGER ) response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={ "input": { "userId": user_2.relay_id, "profileId": profile.relay_id, "action": "REMOVE" } }, ) assert content["data"]["roleUpdate"]["profileUserRole"]["status"] == "REMOVED"
🧹 Nitpick comments (2)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (2)
243-244: Remove unnecessary database refreshesThe
profile.refresh_from_db()calls after the assertions are unnecessary since we're not verifying any profile changes. Consider removing these lines or adding specific assertions if there are profile-level changes to verify.assert content["data"]["roleUpdate"]["profileUserRole"]["role"] == "ADMIN" - profile.refresh_from_db()Also applies to: 270-271, 296-297
222-297: Enhance test assertionsThe current tests could benefit from more specific assertions to verify:
- The complete state of the
profileUserRoleobject (including status)- That other members' roles remain unchanged
- That the appropriate audit trail is created (if applicable)
Example of enhanced assertions:
def test_user_profile_owner_can_update_role(django_user_client, graphql_user_client): # ... existing setup ... response = graphql_user_client(PROFILE_ROLE_UPDATE_GRAPHQL, variables={...}) content = response.json() profile_user_role = ProfileUserRole.objects.get( profile=profile, user=user_2 ) # Verify complete response assert content["data"]["roleUpdate"]["profileUserRole"]["role"] == "ADMIN" assert content["data"]["roleUpdate"]["profileUserRole"]["status"] == "ACTIVE" # Verify database state assert profile_user_role.role == ProfileUserRole.ProfileRoles.ADMIN assert profile_user_role.status == ProfileUserRole.Status.ACTIVE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py(1 hunks)baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py(3 hunks)
🔇 Additional comments (2)
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (2)
11-11: LGTM! Model loading is correctly implemented.
The addition of ProfileUserRole model loading using swapper is consistent with the existing pattern and necessary for the role management feature.
Line range hint 1-248: Additional test coverage needed for role management feature.
Given that this PR implements role management functionality, the following test scenarios should be added:
-
Role-specific permissions:
- Test that Admins can modify member roles
- Test that Managers cannot modify member roles
- Test permission checks in member queries
-
Role update operations:
- Test successful role updates
- Test validation of allowed role transitions
- Test confirmation requirements for Admin role changes
-
Member management:
- Test member removal permissions
- Test role-based access control for member list modifications
Let's verify if these test scenarios are covered elsewhere:
Consider organizing these tests in a separate test file (e.g., test_role_management.py) to maintain clear separation of concerns.
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (1)
222-297:⚠️ Potential issueAdd missing test cases for comprehensive coverage
Several critical test cases are missing according to the PR objectives and edge cases.
Add the following test cases:
- Test that managers cannot modify roles:
def test_manager_cannot_modify_roles(django_user_client, graphql_user_client): user = django_user_client.user profile = ProfileFactory() ProfileUserRoleFactory( profile=profile, user=user, role=ProfileUserRole.ProfileRoles.MANAGER ) target_user = UserFactory() profile_user_role = ProfileUserRoleFactory( profile=profile, user=target_user, role=ProfileUserRole.ProfileRoles.MANAGER ) response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={ "input": { "userId": target_user.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN" } }, ) content = response.json() assert "insufficient_permissions" in content["errors"][0]["message"]
- Test invalid role type:
def test_invalid_role_type(django_user_client, graphql_user_client): user = django_user_client.user profile = ProfileFactory(owner=user) target_user = UserFactory() ProfileUserRoleFactory( profile=profile, user=target_user, role=ProfileUserRole.ProfileRoles.MANAGER ) response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={ "input": { "userId": target_user.relay_id, "profileId": profile.relay_id, "roleType": "INVALID_ROLE" } }, ) content = response.json() assert "invalid_role_type" in content["errors"][0]["message"]
- Test preventing self-role modification:
def test_cannot_modify_own_role(django_user_client, graphql_user_client): user = django_user_client.user profile = ProfileFactory(owner=user) profile_user_role = ProfileUserRoleFactory( profile=profile, user=user, role=ProfileUserRole.ProfileRoles.ADMIN ) response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={ "input": { "userId": user.relay_id, "profileId": profile.relay_id, "roleType": "MANAGER" } }, ) content = response.json() assert "cannot_modify_own_role" in content["errors"][0]["message"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py(3 hunks)
🔇 Additional comments (1)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (1)
274-297: 🛠️ Refactor suggestion
Remove unnecessary refresh_from_db() and enhance error validation
The profile.refresh_from_db() call is unnecessary as we're not checking any profile attributes. Also, we should verify that the role remains unchanged after a failed update.
Apply these changes:
def test_user_without_permission_cant_update_role(django_user_client, graphql_user_client):
# ... existing setup ...
- ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER)
+ profile_user_role = ProfileUserRoleFactory(
+ profile=profile,
+ user=user_3,
+ role=ProfileUserRole.ProfileRoles.MANAGER
+ )
+ initial_role = profile_user_role.role
response = graphql_user_client(
PROFILE_ROLE_UPDATE_GRAPHQL,
variables={"input": {"userId": user_3.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"}},
)
content = response.json()
assert content["errors"][0]["message"] == "You don't have permission to perform this action"
- profile.refresh_from_db()
+
+ # Verify role remains unchanged
+ profile_user_role.refresh_from_db()
+ assert profile_user_role.role == initial_roleLikely invalid or redundant comment.
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Show resolved
Hide resolved
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (2)
222-245:⚠️ Potential issueAdd confirmation flag for Admin role changes
According to the PR objectives, changing a member's role to Admin requires confirmation. The test needs to verify both confirmed and unconfirmed scenarios.
The test should verify:
- Role change to Admin without confirmation should fail
- Role change to Admin with confirmation should succeed
- Role change to Manager doesn't require confirmation
274-297: 🛠️ Refactor suggestionVerify role remains unchanged and remove unnecessary DB refresh
The test should verify that the role remains unchanged after a failed update attempt.
def test_user_without_permission_cant_update_role(django_user_client, graphql_user_client): # ... existing setup ... - ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER) + profile_user_role = ProfileUserRoleFactory( + profile=profile, + user=user_3, + role=ProfileUserRole.ProfileRoles.MANAGER + ) + initial_role = profile_user_role.role response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={ "input": {"userId": user_3.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"} }, ) content = response.json() assert content["errors"][0]["message"] == "You don't have permission to perform this action" - profile.refresh_from_db() + + # Verify role remains unchanged + profile_user_role.refresh_from_db() + assert profile_user_role.role == initial_role
🧹 Nitpick comments (1)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (1)
247-272: Improve test robustness and remove unnecessary DB refreshThe test can be improved by:
- Verifying the initial role before update
- Removing unnecessary
profile.refresh_from_db()call as we're not checking any profile attributesdef test_user_with_permission_can_update_role(django_user_client, graphql_user_client): # ... existing setup ... - ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER) + profile_user_role = ProfileUserRoleFactory( + profile=profile, + user=user_3, + role=ProfileUserRole.ProfileRoles.MANAGER + ) + + # Verify initial state + assert profile_user_role.role == ProfileUserRole.ProfileRoles.MANAGER response = graphql_user_client( PROFILE_ROLE_UPDATE_GRAPHQL, variables={ "input": {"userId": user_3.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"} }, ) content = response.json() assert content["data"]["profileRoleUpdate"]["profileUserRole"]["role"] == "ADMIN" - profile.refresh_from_db()
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py(3 hunks)
🔇 Additional comments (3)
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py (3)
37-51:
Add confirmation flag to mutation input schema
According to the PR objectives, a confirmation dialog is required when changing a member's role to Admin. The mutation input schema should include a confirmed boolean field to support this requirement.
Update the mutation schema:
mutation ProfileRoleUpdateMutation($input: RoleUpdateInput!) {
profileRoleUpdate(input: $input) {
profileUserRole {
id
role
status
}
errors {
field
messages
}
}
}Likely invalid or redundant comment.
222-245:
Add test cases for Admin role confirmation requirement
The test should verify that changing a role to Admin requires confirmation.
Add test cases for both confirmed and unconfirmed role changes:
def test_user_profile_owner_can_update_role(django_user_client, graphql_user_client):
# ... existing setup ...
+ # Attempt without confirmation should fail
response = graphql_user_client(
PROFILE_ROLE_UPDATE_GRAPHQL,
variables={
- "input": {"userId": user_2.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"}
+ "input": {
+ "userId": user_2.relay_id,
+ "profileId": profile.relay_id,
+ "roleType": "ADMIN",
+ "confirmed": False
+ }
},
)
content = response.json()
+ assert "confirmation_required" in content["errors"][0]["message"]
+ # Attempt with confirmation should succeed
+ response = graphql_user_client(
+ PROFILE_ROLE_UPDATE_GRAPHQL,
+ variables={
+ "input": {
+ "userId": user_2.relay_id,
+ "profileId": profile.relay_id,
+ "roleType": "ADMIN",
+ "confirmed": True
+ }
+ },
+ )
+ content = response.json()
assert content["data"]["profileRoleUpdate"]["profileUserRole"]["role"] == "ADMIN"
- profile.refresh_from_db()Likely invalid or redundant comment.
274-297: 🛠️ Refactor suggestion
Improve test for user without permission
The test should verify that the role remains unchanged after a failed update attempt.
Update the test to verify role state:
def test_user_without_permission_cant_update_role(django_user_client, graphql_user_client):
# ... existing setup ...
profile = ProfileFactory(owner=user_2)
ProfileUserRoleFactory(profile=profile, user=user, role=ProfileUserRole.ProfileRoles.MANAGER)
- ProfileUserRoleFactory(profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER)
+ profile_user_role = ProfileUserRoleFactory(
+ profile=profile,
+ user=user_3,
+ role=ProfileUserRole.ProfileRoles.MANAGER
+ )
+ initial_role = profile_user_role.role
response = graphql_user_client(
PROFILE_ROLE_UPDATE_GRAPHQL,
variables={
"input": {"userId": user_3.relay_id, "profileId": profile.relay_id, "roleType": "ADMIN"}
},
)
content = response.json()
assert content["errors"][0]["message"] == "You don't have permission to perform this action"
- profile.refresh_from_db()
+
+ # Verify role remains unchanged
+ profile_user_role.refresh_from_db()
+ assert profile_user_role.role == initial_roleLikely invalid or redundant comment.
baseapp-profiles/baseapp_profiles/tests/test_graphql_mutations_update.py
Show resolved
Hide resolved
|
Description
As a user, on the BaseApp Profile,I would like to Change the role of a member, In order to to give them permissions for different managing actions inside the profile.
Original Story:
Acceptance Criteria
Context
In this story we want to enable to set permissions to members in an organization.
Business Rules - Edit Member Role - 3
Given a user wants to change the role or remove a member from their organization, they will be able to click on a dropdown next to the members name in the Members List
The user is able to change between Admin and Manager Role
Given the user has selected a new role for that member, display a confirmation dialog to make sure he intends to do this change.
Only show this confirmation dialog when changing to Admin
Given the user has confirmed the change of role, then the member's role should now reflect it in the FE and BE.
Only admin roles are able to interact with the Member's editing dropdown
Managers will have this dropdown disable.
Add the permission the following permissions to the Admin role:
Editing the role of a member
Removing a member
Copy Text
Role Change Confirmation: Are you sure you want to assign the [Role] role to this user?
Removal Confirmation: Are you sure you want to remove this member? This action will revoke access to the organizational profile
Current behavior
Expected behavior
Code Snippet
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3790-91705&node-type=instance&t=4tZzXNioOJs6iIeL-0
Template: https://bitbucket.org/silverlogic/baseapp-frontend-template/pull-requests/122
BA FE Package: silverlogic/baseapp-frontend#143
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
baseapp_profilespackage from 0.3.5 to 0.3.6.