Conversation
…ft project details Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
Contributor
📝 WalkthroughWalkthroughMembership view logic now unassigns all role subclasses then assigns the chosen role and conditionally ensures Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MembershipView as "MembershipView\n(api/views.py)"
participant RoleManager as "RoleManager\n(unassign/assign helpers)"
participant DB as "Database\n(memberships, roles)"
Client->>MembershipView: POST /members/ (create) with role_id, user
MembershipView->>RoleManager: unassign_all_role_subclasses(old_user?)
RoleManager->>DB: remove role assignments
MembershipView->>RoleManager: assign role_cls to user
RoleManager->>DB: add role assignment
alt role_cls != BasicMember
MembershipView->>RoleManager: assign BasicMember to user
RoleManager->>DB: add BasicMember assignment
end
RoleManager-->>MembershipView: success
MembershipView-->>Client: 201 Created
Note over MembershipView,RoleManager: update flow additionally revokes roles from prior membership.user when user changed
Client->>MembershipView: DELETE /members/{id}
MembershipView->>RoleManager: unassign_all_role_subclasses(target_user incl BasicMember)
RoleManager->>DB: remove assignments
MembershipView->>DB: delete membership
MembershipView-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ami/users/api/views.py (1)
63-66: RedundantBasicMember.assign_usercall whenrole_clsisBasicMemberWhen the requested
role_idmaps toBasicMember, lines 65–66 (and 90–91 inperform_update) both callBasicMember.assign_user(user, project)back-to-back — the second call is a no-op due to Django's idempotent M2Madd, but it incurs an extra DB round-trip on everyBasicMembermembership creation or update.♻️ Suggested fix (apply to both
perform_createandperform_update)- role_cls.assign_user(user, project) - BasicMember.assign_user(user, project) + role_cls.assign_user(user, project) + if role_cls is not BasicMember: + BasicMember.assign_user(user, project)Also applies to: 88-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/users/api/views.py` around lines 63 - 66, The code unconditionally calls BasicMember.assign_user(user, project) after assigning role_cls, causing a redundant DB round-trip when role_cls is BasicMember; update both perform_create and perform_update to skip the extra call by checking if role_cls is not BasicMember (e.g., if role_cls is not BasicMember: BasicMember.assign_user(user, project)) or otherwise only call BasicMember.assign_user when role_cls differs, leaving the Role.__subclasses__ unassignment and role_cls.assign_user(user, project) logic unchanged.ami/users/tests/test_membership_management_api.py (1)
270-297: Test coverage gaps:perform_updateandperform_destroypaths not exercised for draft accessThe four parameterised tests only exercise
perform_create(POST → assert 200). Two additional paths are untested:
- Update path — after a role change via PATCH, the member should retain draft access (verifies
perform_updatere-assignsBasicMember).- Delete path — after membership deletion, the user should lose access (verifies
perform_destroycorrectly strips all roles includingBasicMember).💡 Suggested additions
def test_member_role_update_retains_draft_access(self): """After a role change via API, member should still access draft project.""" self._add_member_and_assert_can_access_draft(self.user_basic, BasicMember.__name__) self.client.force_authenticate(self.superuser) membership = UserProjectMembership.objects.get(project=self.project, user=self.user_basic) update_url = f"{self.members_url}{membership.pk}/" resp = self.client.patch(update_url, {"role_id": Researcher.__name__}, format="json") self.assertEqual(resp.status_code, 200) self.client.force_authenticate(self.user_basic) detail_resp = self.client.get(self.detail_url) self.assertEqual(detail_resp.status_code, 200, "Member should retain draft access after role update") def test_deleted_member_cannot_access_draft_project(self): """After membership deletion, former member should lose draft access.""" self._add_member_and_assert_can_access_draft(self.user_basic, BasicMember.__name__) self.client.force_authenticate(self.superuser) membership = UserProjectMembership.objects.get(project=self.project, user=self.user_basic) delete_url = f"{self.members_url}{membership.pk}/" self.client.delete(delete_url) self.client.force_authenticate(self.user_basic) detail_resp = self.client.get(self.detail_url) self.assertIn(detail_resp.status_code, (403, 404), "Removed member should not access draft project")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/users/tests/test_membership_management_api.py` around lines 270 - 297, The tests only exercise perform_create; add two tests to cover perform_update and perform_destroy: 1) implement test_member_role_update_retains_draft_access that uses _add_member_and_assert_can_access_draft(self.user_basic, BasicMember.__name__), then as self.superuser fetch the membership via UserProjectMembership.objects.get(project=self.project, user=self.user_basic), PATCH the membership at f"{self.members_url}{membership.pk}/" to change role_id to Researcher.__name__ and assert 200, then authenticate as the user and assert GET to self.detail_url returns 200 (verifying perform_update reassigns BasicMember); 2) implement test_deleted_member_cannot_access_draft_project that adds the member the same way, then as self.superuser delete the membership at f"{self.members_url}{membership.pk}/", then authenticate as the user and assert GET to self.detail_url returns 403 or 404 (verifying perform_destroy removes draft access).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/users/tests/test_membership_management_api.py`:
- Line 260: Ruff flags the hardcoded password literal used when creating the
superuser (User.objects.create_superuser(password="x")), so replace the literal
with a single shared test password constant (e.g., TEST_PASSWORD = "x") defined
at the top of the test module and use that constant in
TestUserProjectMembershipAPI.setUp and here, or alternatively add a local noqa
comment to silence S106 for this line; update usages to reference TEST_PASSWORD
and ensure create_superuser(...) uses that symbol instead of the raw string.
---
Nitpick comments:
In `@ami/users/api/views.py`:
- Around line 63-66: The code unconditionally calls
BasicMember.assign_user(user, project) after assigning role_cls, causing a
redundant DB round-trip when role_cls is BasicMember; update both perform_create
and perform_update to skip the extra call by checking if role_cls is not
BasicMember (e.g., if role_cls is not BasicMember: BasicMember.assign_user(user,
project)) or otherwise only call BasicMember.assign_user when role_cls differs,
leaving the Role.__subclasses__ unassignment and role_cls.assign_user(user,
project) logic unchanged.
In `@ami/users/tests/test_membership_management_api.py`:
- Around line 270-297: The tests only exercise perform_create; add two tests to
cover perform_update and perform_destroy: 1) implement
test_member_role_update_retains_draft_access that uses
_add_member_and_assert_can_access_draft(self.user_basic, BasicMember.__name__),
then as self.superuser fetch the membership via
UserProjectMembership.objects.get(project=self.project, user=self.user_basic),
PATCH the membership at f"{self.members_url}{membership.pk}/" to change role_id
to Researcher.__name__ and assert 200, then authenticate as the user and assert
GET to self.detail_url returns 200 (verifying perform_update reassigns
BasicMember); 2) implement test_deleted_member_cannot_access_draft_project that
adds the member the same way, then as self.superuser delete the membership at
f"{self.members_url}{membership.pk}/", then authenticate as the user and assert
GET to self.detail_url returns 403 or 404 (verifying perform_destroy removes
draft access).
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a permissions gap where members added/updated via the Members API could not view draft project details because they were missing the BasicMember role required by draft permission checks.
Changes:
- Assign
BasicMemberinUserProjectMembershipViewSet.perform_createandperform_updateafter assigning the requested role. - Ensure
perform_destroyrevokes all role groups before deleting the membership. - Add regression tests asserting members created via the API (across multiple roles) can access draft project details.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ami/users/api/views.py |
Ensures API-managed memberships also get BasicMember so draft-project retrieve permission checks succeed. |
ami/users/tests/test_membership_management_api.py |
Adds regression coverage for draft project access for members added via the Members API across roles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
Claude says: Addressed the review feedback in the latest push:
All 7 tests pass. |
- Revoke old user's roles in perform_update when membership user changes - Skip redundant BasicMember.assign_user when role_cls is already BasicMember - Add tests for role update retaining draft access and deletion revoking it Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ami/users/tests/test_membership_management_api.py (1)
287-337: Exercise the membership-reassignment branch too.These tests cover create, delete, and same-user role changes, but they never hit the new
perform_update()path wheremembership.userchanges and the old user's roles are revoked. Please add a PATCH that reassigns the membership to a different user and asserts the previous user loses draft access while the replacement user keeps it. If this suite is meant to cover all supported roles, addMLDataManagerhere as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/users/tests/test_membership_management_api.py` around lines 287 - 337, Add a new assertion branch that exercises the membership-reassignment path (the perform_update that changes membership.user): after creating a membership via _add_member_and_assert_can_access_draft (as in test_member_role_update_retains_draft_access), authenticate as self.superuser, fetch the UserProjectMembership for self.user_basic, then PATCH f"{self.members_url}{membership.pk}/" with a payload that reassigns "user" to a different user (e.g. self.user_identifier.pk) and optionally includes "role_id" (to cover role-preserving reassignment), assert 200, then authenticate as the original user and assert they now get 403/404 on self.detail_url, and authenticate as the new user and assert they get 200; also ensure MLDataManager is exercised either by adding a separate variant calling _add_member_and_assert_can_access_draft(self.user_ml_data_manager, MLDataManager.__name__) or by including MLDataManager in the tested roles so the reassignment case covers that role too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/users/api/views.py`:
- Around line 60-70: The view is temporarily disconnecting the global
m2m_changed receiver (manage_project_membership), which causes race conditions;
instead, implement a request-scoped skip flag (e.g., a thread-local or
contextvar) that manage_project_membership checks and returns early when set,
and set/clear that flag around the role update block (the section using
Role.__subclasses__(), role_cls.assign_user, BasicMember.assign_user and
touching Group.user_set.through) so you avoid m2m_changed.disconnect/reconnect
while keeping the signal registered globally.
---
Nitpick comments:
In `@ami/users/tests/test_membership_management_api.py`:
- Around line 287-337: Add a new assertion branch that exercises the
membership-reassignment path (the perform_update that changes membership.user):
after creating a membership via _add_member_and_assert_can_access_draft (as in
test_member_role_update_retains_draft_access), authenticate as self.superuser,
fetch the UserProjectMembership for self.user_basic, then PATCH
f"{self.members_url}{membership.pk}/" with a payload that reassigns "user" to a
different user (e.g. self.user_identifier.pk) and optionally includes "role_id"
(to cover role-preserving reassignment), assert 200, then authenticate as the
original user and assert they now get 403/404 on self.detail_url, and
authenticate as the new user and assert they get 200; also ensure MLDataManager
is exercised either by adding a separate variant calling
_add_member_and_assert_can_access_draft(self.user_ml_data_manager,
MLDataManager.__name__) or by including MLDataManager in the tested roles so the
reassignment case covers that role too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: baad5f32-6111-4c63-bf04-668dc9f55e18
📒 Files selected for processing (2)
ami/users/api/views.pyami/users/tests/test_membership_management_api.py
mihow
approved these changes
Apr 1, 2026
Collaborator
|
Thanks @mohamedelabbas1996 ! |
5 tasks
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
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
Members added via the Members API received 403 errors when viewing draft project details. Draft permission checks use
BasicMember.has_role(), but the API only assigned the requested role (Identifier, Researcher, etc.) without also assigning BasicMember.List of Changes
perform_createandperform_updateinUserProjectMembershipViewSetnow assign BasicMember alongside the requested role (skipped when the requested role is already BasicMember)perform_updaterevokes roles from the old user when a membership's user is changed, preventing permission leaksperform_destroyrevokes all roles (including BasicMember) before deleting the membershipTestMembersApiDraftProjectAccesswith 7 tests covering:How to Test
docker compose -f docker-compose.ci.yml run --rm django python manage.py test ami.users.tests.test_membership_management_api.TestMembersApiDraftProjectAccess --keepdbChecklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests