Fix: assign BasicMember role in members API so members can access draft project details#1140
Draft
mohamedelabbas1996 wants to merge 1 commit intomainfrom
Draft
Fix: assign BasicMember role in members API so members can access draft project details#1140mohamedelabbas1996 wants to merge 1 commit intomainfrom
mohamedelabbas1996 wants to merge 1 commit intomainfrom
Conversation
…ft project details Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for antenna-ssec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughUpdated membership API views to ensure BasicMember role is consistently assigned alongside specific project roles during create and update operations. Added test coverage verifying draft project access control for members with different role types. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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).
| self.project.save() | ||
| create_roles_for_project(self.project) | ||
|
|
||
| self.superuser = User.objects.create_superuser(email="super@insectai.org", password="x") |
Contributor
There was a problem hiding this comment.
Static analysis flag S106 (hardcoded test password) — consistent with existing pattern
Ruff flags password="x" at this line. The identical credential is already present on line 16 in the same file (TestUserProjectMembershipAPI.setUp), so this is consistent. No production risk; flagging for awareness since the linter will surface it in CI.
🧰 Tools
🪛 Ruff (0.15.1)
[error] 260-260: Possible hardcoded password assigned to argument: "password"
(S106)
🤖 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` at 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.
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
1 participant
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 receive 403 error when viewing draft project details. The fix manually assigns the BasicMember role when creating or updating members via the API.
List of Changes
UserProjectMembershipViewSetinami/users/api/views.pyto assign BasicMember inperform_createandperform_update, and to unassign all roles (including BasicMember) inperform_destroyTestMembersApiDraftProjectAccessinami/users/tests/test_membership_management_api.pyto verify all roles can access draft project detailsRelated Issues
TBD
Detailed Description
Draft project permission checks use
BasicMember.has_role(). Previously, when members were added via the Members API, only the requested role (Identifier, Researcher, etc.) was assigned, so users without BasicMember got 403 when accessing draft project details.How to Test the Changes
TBD
Screenshots
N/A
Deployment Notes
TBD
Checklist
Summary by CodeRabbit
New Features
Tests