[feature] Added REST API endpoints for organization memberships #543 - #555
[feature] Added REST API endpoints for organization memberships #543#555BHARATH0153 wants to merge 1 commit into
Conversation
…isp#543 Added dedicated REST API endpoints to manage user organization memberships: list/create under users/user/{id}/organization-membership/ and retrieve/update/delete under users/user/{id}/organization-membership/{org_id}/. Memberships can be managed by superusers and by organization managers of the organizations involved; managers are restricted to the organizations they manage and cannot manage superusers. Closes openwisp#543
📝 WalkthroughWalkthroughAdds dedicated REST API endpoints to list, create, retrieve, update, and delete user organization memberships. The implementation adds serializer validation, organization-scoped queries, organization-manager and superuser permission checks, duplicate prevention, ownership protection, and partial updates. Tests cover endpoint behavior, authorization, invalid UUIDs, and URL mappings. Documentation describes the endpoints and the Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MembershipView
participant MembershipSerializer
participant OrganizationUser
Client->>MembershipView: Submit membership request
MembershipView->>OrganizationUser: Filter by user and organization
MembershipView->>MembershipSerializer: Validate request data
MembershipSerializer->>OrganizationUser: Create or update membership
MembershipView-->>Client: Return membership response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/rest-api.rst`:
- Around line 206-252: In the organization membership endpoint section, add a
concise note documenting that superusers can manage any membership, organization
managers can manage memberships only for organizations they manage, and
organization managers cannot manage superusers.
In `@openwisp_users/api/views.py`:
- Around line 317-319: Document in docs/user/rest-api.rst that this endpoint’s
PUT performs a partial update like PATCH, and add an inline comment above the
forced partial update in update() explaining that organization is required but
immutable.
- Around line 268-285: Extract initial, get_parent_queryset, and
get_organization_queryset into a shared mixin, preserving the existing
tenant-scoping and superuser behavior. Remove the duplicate implementations from
both view bases and have each inherit from the new mixin so both endpoints use
the same authorization logic.
- Around line 287-292: Update get_serializer_context() so the swagger_fake_view
branch returns the base serializer context dictionary with user set to None,
rather than returning None; preserve the existing
super().get_serializer_context() result and normal get_parent_queryset().first()
behavior for non-swagger requests.
In `@openwisp_users/tests/test_api/test_api.py`:
- Around line 697-743: Add regression coverage for DELETE in the
organization-manager API tests: verify the manager can delete a membership in
their managed organization with HTTP 204, and cannot delete a membership
belonging to an unmanaged organization, which must return HTTP 404. Reuse the
existing setup patterns and target the organization membership detail endpoint
handled by OrganizationMembershipDetailView.
- Around line 569-589: Merge test_organization_membership_put_api and
test_organization_membership_patch_api into a single test method using subTest
to iterate over PUT and PATCH. Keep the shared setup, payload, query-count
assertion, status assertion, and is_admin assertion unchanged for each HTTP
method.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 253c3ecb-39dd-4875-a3ae-572f13c4ec24
📒 Files selected for processing (7)
docs/user/rest-api.rstopenwisp_users/api/serializers.pyopenwisp_users/api/urls.pyopenwisp_users/api/views.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers.
Place imports at the top of the file; defer imports only when necessary, such as Django model imports before app readiness.
Avoid unnecessary blank lines inside function and method bodies.
Use concise names that do not unnecessarily repeat context already provided by the containing module, class, or function.
Prefer method decorators for context managers covering an entire test method when this avoids unnecessary nesting.
Build internal Django URLs with named URL patterns and reverse() or reverse_lazy(), including in tests, using the appropriate namespace and URL arguments.
Organization-owned, parent, and related-object lookups must begin with objects managed by the requester; filters may only narrow that queryset, and writes must reject cross-organization relations.
Cached lookups must check permission and organization scope on every request.
Preserve swappable model support, public APIs, migrations, and multi-tenant permission behavior unless explicitly required.
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/*.{py,js,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Add comments or docstrings only when they explain non-obvious reasons, constraints, compatibility or security requirements, side effects, unavoidable complexity, or opaque behavior; do not restate adjacent code.
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/test*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test*.py: Group separate tests with nearly identical database preparation and varying inputs or outcomes into one test method using subTest; retain separate methods for genuinely distinct behavior.
Leave one blank line immediately before each self.subTest(...) call.
Keep helpers and classes used by only one test method inside that method; promote them only when genuinely reused.
Include assertNumQueries() with representative data in main behavior tests for non-trivial, frequently called views; use AssertNumQueriesSubTestMixin where available.
Run focused tests and directly affected suites during development instead of routinely running the full suite.
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_api.py
openwisp_users/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_users/tests/**/*.py: Prefer in-process tests so coverage tools can measure changed code.
Changes to swapped-model behavior, tenant isolation, authentication flows, or admin/API permissions require package-level regression tests.
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_api.py
**/*.{py,js,html}
📄 CodeRabbit inference engine (AGENTS.md)
Update documentation when behavior, settings, public APIs, setup steps, supported versions, or documented feature behavior changes.
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: For UI-impacting changes, the pull request description must include before-and-after screen recordings or screenshots.
Changes, new features, and bug fixes must include at least one regression test.
New features must be documented.
Changes to documented features require corresponding documentation updates.
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pydocs/user/rest-api.rstopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
⚙️ CodeRabbit configuration file
**/*: - Flag potential security vulnerabilities
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Flag unused or redundant code
Flag outdated or incorrect comments/docstrings
Ensure new code handles errors properly:
- Log errors that cannot be resolved by the user with error level
- Log unusual conditions with warning level
- Log important background actions with info level
- Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pydocs/user/rest-api.rstopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/*tests*/**
⚙️ CodeRabbit configuration file
**/*tests*/**: Ensure tests cover relevant success, error, boundary, and unusual
input scenarios.Flag tests that depend on arbitrary sleeps, uncontrolled system time,
specific timezones, unseeded randomness, network access, external
services, execution order, shared mutable state, hardcoded ports, or
asynchronous operations that are not properly awaited.
Files:
openwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_api.py
**/*.{md,rst}
⚙️ CodeRabbit configuration file
**/*.{md,rst}: Verify that documentation remains consistent with the implemented
behavior and does not reference deprecated or removed functionality.
Files:
docs/user/rest-api.rst
openwisp_users/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_users/api/**/*.py: API authentication, permissions, throttling, filters, serializers, and views must preserve authentication and tenant-security behavior.
Changes to HTTP REST endpoints or DRF serializers must include tests for permissions, input validation, filtering or pagination when supported, and organization or tenant boundaries where applicable.
Files:
openwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/api/views.py
🔇 Additional comments (8)
openwisp_users/api/serializers.py (1)
205-242: LGTM!openwisp_users/api/views.py (2)
295-301: LGTM!
28-28: LGTM!Also applies to: 332-333
openwisp_users/api/urls.py (1)
49-58: LGTM!openwisp_users/tests/test_api/test_api.py (2)
14-17: LGTM!Also applies to: 42-48
518-568: LGTM!Also applies to: 591-636
openwisp_users/tests/test_api/test_urls.py (1)
22-23: LGTM!openwisp_users/tests/test_api/test_views.py (1)
35-36: LGTM!
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Reviewed by step-3.7-flash · Input: 121.9K · Output: 27.6K · Cached: 2M |
|
@nemesifier @pandafy once check this whenever you are free thanks! |
|
@coderabbitai full review! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_users/tests/test_api/test_api.py`:
- Around line 537-542: Extend the test assertions after the POST to retrieve the
created OrganizationUser and verify its user_id equals user1.pk. Keep the
existing response and count assertions, using the persisted membership rather
than response data to validate serializer context assignment.
In `@openwisp_users/tests/test_api/test_views.py`:
- Around line 35-36: Add a test case alongside the existing invalid-UUID URL
cases using a valid user UUID for the <uuid:pk> segment and an invalid
organization UUID for <uuid:org_id>. Ensure the request reaches and
independently validates the organization-membership route’s org_id converter.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a9afb327-87af-4ada-b024-2d3254e0f0b4
📒 Files selected for processing (7)
docs/user/rest-api.rstopenwisp_users/api/serializers.pyopenwisp_users/api/urls.pyopenwisp_users/api/views.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_views.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers.
Place imports at the top of the file; defer imports only when necessary, such as Django model imports before app readiness.
Avoid unnecessary blank lines inside function and method bodies.
Use concise names that do not unnecessarily repeat context already provided by the containing module, class, or function.
Prefer method decorators for context managers covering an entire test method when this avoids unnecessary nesting.
Build internal Django URLs with named URL patterns and reverse() or reverse_lazy(), including in tests, using the appropriate namespace and URL arguments.
Organization-owned, parent, and related-object lookups must begin with objects managed by the requester; filters may only narrow that queryset, and writes must reject cross-organization relations.
Cached lookups must check permission and organization scope on every request.
Preserve swappable model support, public APIs, migrations, and multi-tenant permission behavior unless explicitly required.
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/*.{py,js,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Add comments or docstrings only when they explain non-obvious reasons, constraints, compatibility or security requirements, side effects, unavoidable complexity, or opaque behavior; do not restate adjacent code.
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/test*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test*.py: Group separate tests with nearly identical database preparation and varying inputs or outcomes into one test method using subTest; retain separate methods for genuinely distinct behavior.
Leave one blank line immediately before each self.subTest(...) call.
Keep helpers and classes used by only one test method inside that method; promote them only when genuinely reused.
Include assertNumQueries() with representative data in main behavior tests for non-trivial, frequently called views; use AssertNumQueriesSubTestMixin where available.
Run focused tests and directly affected suites during development instead of routinely running the full suite.
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_api.py
openwisp_users/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_users/tests/**/*.py: Prefer in-process tests so coverage tools can measure changed code.
Changes to swapped-model behavior, tenant isolation, authentication flows, or admin/API permissions require package-level regression tests.
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_api.py
**/*.{py,js,html}
📄 CodeRabbit inference engine (AGENTS.md)
Update documentation when behavior, settings, public APIs, setup steps, supported versions, or documented feature behavior changes.
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: For UI-impacting changes, the pull request description must include before-and-after screen recordings or screenshots.
Changes, new features, and bug fixes must include at least one regression test.
New features must be documented.
Changes to documented features require corresponding documentation updates.
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pydocs/user/rest-api.rstopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
⚙️ CodeRabbit configuration file
**/*: - Flag potential security vulnerabilities
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Flag unused or redundant code
Flag outdated or incorrect comments/docstrings
Ensure new code handles errors properly:
- Log errors that cannot be resolved by the user with error level
- Log unusual conditions with warning level
- Log important background actions with info level
- Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pydocs/user/rest-api.rstopenwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/views.py
**/*tests*/**
⚙️ CodeRabbit configuration file
**/*tests*/**: Ensure tests cover relevant success, error, boundary, and unusual
input scenarios.Flag tests that depend on arbitrary sleeps, uncontrolled system time,
specific timezones, unseeded randomness, network access, external
services, execution order, shared mutable state, hardcoded ports, or
asynchronous operations that are not properly awaited.
Files:
openwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_urls.pyopenwisp_users/tests/test_api/test_api.py
**/*.{md,rst}
⚙️ CodeRabbit configuration file
**/*.{md,rst}: Verify that documentation remains consistent with the implemented
behavior and does not reference deprecated or removed functionality.
Files:
docs/user/rest-api.rst
openwisp_users/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_users/api/**/*.py: API authentication, permissions, throttling, filters, serializers, and views must preserve authentication and tenant-security behavior.
Changes to HTTP REST endpoints or DRF serializers must include tests for permissions, input validation, filtering or pagination when supported, and organization or tenant boundaries where applicable.
Files:
openwisp_users/api/urls.pyopenwisp_users/api/serializers.pyopenwisp_users/api/views.py
🔇 Additional comments (11)
openwisp_users/tests/test_api/test_api.py (3)
569-589: Merge the identical PUT and PATCH tests.This concern was reported previously and still applies.
697-743: Add organization-manager DELETE authorization coverage.This concern was reported previously and still applies.
14-17: LGTM!Also applies to: 42-49, 518-530, 544-568, 591-636, 638-696
openwisp_users/api/views.py (4)
256-285: Extract the duplicated parent-user scoping logic.This concern was reported previously and still applies.
287-292: Return a serializer context dictionary during schema generation.This concern was reported previously and still applies.
317-319: Document that PUT performs a partial update.This concern was reported previously and still applies.
28-28: LGTM!Also applies to: 295-315, 332-333
docs/user/rest-api.rst (1)
206-251: Document membership permission rules.This concern was reported previously and still applies.
openwisp_users/api/serializers.py (1)
205-242: LGTM!openwisp_users/api/urls.py (1)
49-58: LGTM!openwisp_users/tests/test_api/test_urls.py (1)
22-23: LGTM!
| with self.assertNumQueries(9): | ||
| r = self.client.post(path, data, content_type="application/json") | ||
| self.assertEqual(r.status_code, 201) | ||
| self.assertEqual(OrganizationUser.objects.count(), 1) | ||
| self.assertEqual(r.data["organization"], org1.pk) | ||
| self.assertFalse(r.data["is_admin"]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the created membership belongs to the URL user.
Assert that the persisted OrganizationUser.user_id equals user1.pk. The response does not contain the user ID, so the current assertions do not verify the serializer context assignment.
Proposed test addition
self.assertEqual(OrganizationUser.objects.count(), 1)
+ self.assertEqual(
+ OrganizationUser.objects.get(organization=org1).user_id, user1.pk
+ )
self.assertEqual(r.data["organization"], org1.pk)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with self.assertNumQueries(9): | |
| r = self.client.post(path, data, content_type="application/json") | |
| self.assertEqual(r.status_code, 201) | |
| self.assertEqual(OrganizationUser.objects.count(), 1) | |
| self.assertEqual(r.data["organization"], org1.pk) | |
| self.assertFalse(r.data["is_admin"]) | |
| with self.assertNumQueries(9): | |
| r = self.client.post(path, data, content_type="application/json") | |
| self.assertEqual(r.status_code, 201) | |
| self.assertEqual(OrganizationUser.objects.count(), 1) | |
| self.assertEqual( | |
| OrganizationUser.objects.get(organization=org1).user_id, user1.pk | |
| ) | |
| self.assertEqual(r.data["organization"], org1.pk) | |
| self.assertFalse(r.data["is_admin"]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/tests/test_api/test_api.py` around lines 537 - 542, Extend the
test assertions after the POST to retrieve the created OrganizationUser and
verify its user_id equals user1.pk. Keep the existing response and count
assertions, using the persisted membership rather than response data to validate
serializer context assignment.
Source: Coding guidelines
| "/api/v1/users/user/not-a-uuid/organization-membership/", | ||
| "/api/v1/users/user/not-a-uuid/organization-membership/not-a-uuid/", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the org_id UUID converter independently.
Line 36 fails at <uuid:pk> before Django evaluates <uuid:org_id>. Add a case with a valid user UUID and an invalid organization UUID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/tests/test_api/test_views.py` around lines 35 - 36, Add a test
case alongside the existing invalid-UUID URL cases using a valid user UUID for
the <uuid:pk> segment and an invalid organization UUID for <uuid:org_id>. Ensure
the request reaches and independently validates the organization-membership
route’s org_id converter.
Source: Path instructions
2 participants
changes
Added dedicated REST API endpoints to manage user organization memberships
Memberships can be managed by superusers and by organization managers of the organizations involved; managers are restricted to the organizations they manage and cannot manage superusers.
Closes #543
Checklist