[change] Make batch user creation organization field readonly#618
[change] Make batch user creation organization field readonly#618Eeshu-Yadav wants to merge 3 commits intoopenwisp:masterfrom
Conversation
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
from
017eddb to
b5023ad
Compare
|
@pandafy kindly review this |
|
@Eeshu-Yadav what's the point of reviewing if you don't follow up? Please follow up with #621. |
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
from
b5023ad to
1c7eba9
Compare
ok |
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
6 times, most recently
from
7ba30e3 to
1402441
Compare
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
from
1402441 to
a7e3f33
Compare
nemesifier
changed the title
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav we're ready to proceed with this one, please rebase on the latest master when you can and trigger the coderabbitai review.
…wisp#609 This change prevents modification of the organization field for existing RadiusBatch objects in both admin interface and REST API. When editing existing batch objects, the organization field is now readonly to maintain data integrity and prevent accidental organization changes. Changes: - Added organization field to readonly_fields in RadiusBatchAdmin for existing objects - Created RadiusBatchUpdateSerializer with organization as readonly field - Added BatchUpdateView for handling batch detail API operations - Added comprehensive tests for both admin and API readonly functionality Fixes openwisp#609
- Document GET, PUT, and PATCH methods for batch retrieve/update - Clarify that organization field is read-only for existing objects - List all read-only and updatable fields
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
from
b120d32 to
6797b23
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a detail API endpoint at /api/v1/radius/batch// supporting GET, PUT, and PATCH to retrieve or update existing RadiusBatch objects. Introduces RadiusBatchUpdateSerializer that makes the model's organization read-only for updates and enforces strategy-specific validation. Adds BatchUpdateView (RetrieveUpdateAPIView) with organization-scoped queryset selection and registers the new URL. Admin readonly fields logic for RadiusBatch was adjusted to prevent changing organization when status is not "pending". Documentation and tests for retrieval, updates, permissions, and readonly enforcement were added. Sequence DiagramsequenceDiagram
participant Client as Client
participant View as BatchUpdateView
participant Serializer as RadiusBatchSerializer / RadiusBatchUpdateSerializer
participant Instance as RadiusBatch Instance
participant DB as Database
Client->>View: GET /api/v1/radius/batch/<id>/
View->>View: get_queryset() (org-scoped)
View->>DB: fetch RadiusBatch by id
DB-->>View: RadiusBatch instance
View->>Serializer: serialize instance (read)
Serializer-->>Client: 200 JSON
Client->>View: PUT/PATCH /api/v1/radius/batch/<id>/ (payload)
View->>View: get_queryset() (org-scoped)
View->>DB: fetch RadiusBatch by id
DB-->>View: RadiusBatch instance
View->>Serializer: use RadiusBatchUpdateSerializer (validate)
Serializer->>Serializer: enforce organization read-only, validate strategy/fields
Serializer->>Instance: apply validated values (in-memory)
Instance->>Instance: full_clean()
View->>DB: save updated RadiusBatch
Serializer-->>Client: 200 JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 review |
✅ Actions performedReview triggered.
|
Eeshu-Yadav
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent changing the organization of an existing RadiusBatch, enforcing immutability in both the Django admin and the REST API to maintain data integrity (per issue #609).
Changes:
- Admin: updated
RadiusBatchAdmin.get_readonly_fields()soorganizationbecomes read-only for existing batches. - REST API: added a batch detail endpoint for retrieve/update and introduced an update serializer intended to keep
organizationread-only. - Tests/docs: added new admin/API tests and documented the new retrieve/update endpoint.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
openwisp_radius/admin.py |
Makes organization read-only for existing RadiusBatch objects in admin. |
openwisp_radius/api/views.py |
Adds BatchUpdateView (retrieve/update) for RadiusBatch detail operations. |
openwisp_radius/api/urls.py |
Adds /radius/batch/<uuid:pk>/ route for the new detail endpoint. |
openwisp_radius/api/serializers.py |
Adds RadiusBatchUpdateSerializer intended to prevent organization changes on update. |
openwisp_radius/tests/test_api/test_api.py |
Adds API tests for retrieve/update and organization immutability. |
openwisp_radius/tests/test_admin.py |
Adds admin tests for read-only/editable organization behavior; also removes an unrelated RadiusGroup admin test. |
docs/user/rest-api.rst |
Documents the new batch retrieve/update endpoint and read-only fields. |
openwisp_radius/integrations/monitoring/tasks.py |
Ensures registration metrics include all methods (fills missing methods with zero). |
openwisp_radius/integrations/monitoring/tests/test_metrics.py |
Adjusts metrics test setup for registered user method handling. |
tests/openwisp2/sample_radius/api/views.py |
Exposes the new batch detail view in the sample app. |
tests/openwisp2/settings.py |
Formatting/robustness tweaks in test settings imports and long strings. |
tests/openwisp2/sample_radius/migrations/0021_*.py |
Formatting of long help_text strings. |
pyproject.toml |
Adds a trailing newline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
openwisp_radius/api/serializers.py
Outdated
| Simplified validation for partial updates. | ||
| Only validates essential fields based on strategy. | ||
| Ignores organization_slug if provided since organization is readonly. | ||
| """ | ||
| # Remove organization_slug from data if provided (should not be changeable) | ||
| data.pop("organization_slug", None) | ||
|
|
||
| strategy = data.get("strategy") or (self.instance and self.instance.strategy) | ||
|
|
||
| if ( | ||
| strategy == "prefix" | ||
| and "number_of_users" in data | ||
| and not data.get("number_of_users") | ||
| ): | ||
| raise serializers.ValidationError( | ||
| {"number_of_users": _("The field number_of_users cannot be empty")} | ||
| ) | ||
|
|
||
| return serializers.ModelSerializer.validate(self, data) | ||
|
|
There was a problem hiding this comment.
RadiusBatchUpdateSerializer.validate bypasses RadiusBatchSerializer.validate by calling serializers.ModelSerializer.validate, which skips the model full_clean() and other validation logic implemented in the parent serializer. This can allow invalid updates (eg, inconsistent strategy/prefix/csv constraints) to be saved. Consider reusing the parent validation while avoiding the KeyError on missing strategy (eg, derive strategy from self.instance when absent) and keep the full_clean()-based validation path.
| Simplified validation for partial updates. | |
| Only validates essential fields based on strategy. | |
| Ignores organization_slug if provided since organization is readonly. | |
| """ | |
| # Remove organization_slug from data if provided (should not be changeable) | |
| data.pop("organization_slug", None) | |
| strategy = data.get("strategy") or (self.instance and self.instance.strategy) | |
| if ( | |
| strategy == "prefix" | |
| and "number_of_users" in data | |
| and not data.get("number_of_users") | |
| ): | |
| raise serializers.ValidationError( | |
| {"number_of_users": _("The field number_of_users cannot be empty")} | |
| ) | |
| return serializers.ModelSerializer.validate(self, data) | |
| Run the parent serializer validation for updates as well, while | |
| remaining compatible with partial updates. | |
| ``organization_slug`` is ignored because organization is read-only on | |
| existing objects. Missing fields are backfilled from ``self.instance`` | |
| only for validation so cross-field checks and model ``full_clean()`` | |
| continue to run without requiring PATCH requests to resend unchanged | |
| values. | |
| """ | |
| data.pop("organization_slug", None) | |
| original_keys = set(data.keys()) | |
| validation_data = data.copy() | |
| if self.instance is not None: | |
| for field_name, field in self.fields.items(): | |
| if ( | |
| field_name not in validation_data | |
| and field_name != "organization_slug" | |
| and not field.read_only | |
| and hasattr(self.instance, field_name) | |
| ): | |
| validation_data[field_name] = getattr(self.instance, field_name) | |
| validated_data = super().validate(validation_data) | |
| return { | |
| key: value for key, value in validated_data.items() if key in original_keys | |
| } |
docs/user/rest-api.rst
Outdated
| Parameters for **PUT**/**PATCH** (only certain fields can be updated): | ||
|
|
||
| =============== ================================================ | ||
| Param Description | ||
| =============== ================================================ | ||
| name Name of the operation | ||
| expiration_date Date of expiration of the users (can be updated) | ||
| =============== ================================================ | ||
|
|
||
| Fields that are **read-only** and cannot be updated: | ||
|
|
||
| - ``organization`` - Cannot be changed after creation | ||
| - ``strategy`` - Cannot be changed after creation | ||
| - ``csvfile`` - Cannot be changed after creation | ||
| - ``prefix`` - Cannot be changed after creation | ||
| - ``users`` - Managed automatically | ||
| - ``user_credentials`` - Generated automatically | ||
| - ``created``, ``modified`` - Timestamps | ||
|
|
There was a problem hiding this comment.
The new REST API docs claim only a limited set of fields can be updated (and list several fields as read-only), but RadiusBatchUpdateSerializer currently exposes fields = "__all__" and only makes a few fields read-only. Either tighten the update serializer to match the documented contract, or adjust the documentation to reflect what the endpoint actually allows updating.
| Parameters for **PUT**/**PATCH** (only certain fields can be updated): | |
| =============== ================================================ | |
| Param Description | |
| =============== ================================================ | |
| name Name of the operation | |
| expiration_date Date of expiration of the users (can be updated) | |
| =============== ================================================ | |
| Fields that are **read-only** and cannot be updated: | |
| - ``organization`` - Cannot be changed after creation | |
| - ``strategy`` - Cannot be changed after creation | |
| - ``csvfile`` - Cannot be changed after creation | |
| - ``prefix`` - Cannot be changed after creation | |
| - ``users`` - Managed automatically | |
| - ``user_credentials`` - Generated automatically | |
| - ``created``, ``modified`` - Timestamps | |
| Parameters for **PUT**/**PATCH**: | |
| The batch update endpoint accepts the fields exposed by the update | |
| serializer for the running version of the API. It is **not** limited to | |
| just ``name`` and ``expiration_date``. | |
| Use the live API schema/documentation for your deployment to determine | |
| the exact set of writable and read-only fields supported by that version. |
openwisp_radius/tests/test_admin.py
Outdated
| response = self.client.get(url) | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
||
| self.assertContains(response, "readonly") |
There was a problem hiding this comment.
These admin tests don't assert that the organization field specifically is read-only for existing batches: the response will contain "readonly" even if only other fields (eg, status) are read-only. Consider asserting that the organization form widget is not present (or is disabled) on the change form, e.g. by parsing HTML and checking the #id_organization element/readonly-field rendering.
| self.assertContains(response, "readonly") | |
| form_html = response.content.decode() | |
| document = lxml_html.fromstring(form_html) | |
| organization_widgets = document.cssselect( | |
| "input#id_organization, select#id_organization, textarea#id_organization" | |
| ) | |
| self.assertEqual( | |
| organization_widgets, | |
| [], | |
| "Organization should not be rendered as an editable widget for existing batches.", | |
| ) |
openwisp_radius/tests/test_admin.py
Outdated
| def test_radiusbatch_organization_readonly_for_existing_objects(self): | ||
| """ | ||
| Test that organization field is readonly for existing RadiusBatch objects | ||
| """ | ||
| batch = self._create_radius_batch( | ||
| name="test-batch", strategy="prefix", prefix="test-prefix" | ||
| ) | ||
| url = reverse(f"admin:{self.app_label}_radiusbatch_change", args=[batch.pk]) | ||
|
|
There was a problem hiding this comment.
This PR removes the previous TestRadiusGroupAdmin test case (add RadiusGroup with inline check/reply). That deletion looks unrelated to making RadiusBatch.organization read-only and reduces admin coverage. If it was removed intentionally due to changed behavior, it should be replaced with equivalent coverage; otherwise, consider restoring it.
| login_payload = {"username": "admin", "password": "tester"} | ||
| login_url = reverse("radius:user_auth_token", args=[self.default_org.slug]) | ||
| login_response = self.client.post(login_url, data=login_payload) | ||
| return f"Bearer {login_response.json()['key']}" |
There was a problem hiding this comment.
_get_admin_auth_header assumes the login request succeeds and returns JSON with a key. If authentication fails, this will raise a KeyError and obscure the real failure. Consider asserting login_response.status_code == 200 (and/or checking the presence of key) to make failures clearer.
| return f"Bearer {login_response.json()['key']}" | |
| self.assertEqual(login_response.status_code, status.HTTP_200_OK) | |
| login_response_data = login_response.json() | |
| self.assertIn("key", login_response_data) | |
| return f"Bearer {login_response_data['key']}" |
| for m in all_methods: | ||
| existing_methods = [ | ||
| clean_registration_method(k) for k in total_registered_users.keys() | ||
| ] | ||
| if m not in existing_methods: | ||
| total_registered_users[m] = 0 |
There was a problem hiding this comment.
In _write_user_signup_metric_for_all, existing_methods is recomputed inside the loop for each m, making this O(n^2) over the number of methods. Compute a single set of existing cleaned methods once, then add missing ones, to keep the task efficient (it runs hourly).
| for m in all_methods: | |
| existing_methods = [ | |
| clean_registration_method(k) for k in total_registered_users.keys() | |
| ] | |
| if m not in existing_methods: | |
| total_registered_users[m] = 0 | |
| existing_methods = { | |
| clean_registration_method(k) for k in total_registered_users.keys() | |
| } | |
| for m in all_methods: | |
| if m not in existing_methods: | |
| total_registered_users[m] = 0 | |
| existing_methods.add(m) |
![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 the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/api/serializers.py`:
- Line 552: RadiusBatchUpdateSerializer.validate() currently returns
serializers.ModelSerializer.validate(self, data) and thus bypasses
RadiusBatchSerializer.validate() (which calls instance.full_clean()), causing
PATCH/PUT to skip model-level validation; fix by restoring the parent validation
chain or explicitly invoking full_clean() during validation: call
super().validate(data) (ensuring RadiusBatchSerializer.validate runs) or, if
instance may be partial, call self.instance.full_clean() with appropriate
update-merge of fields before returning data so strategy-specific checks
(csvfile/prefix), CSV content validation, and prefix character validation are
enforced; reference RadiusBatchUpdateSerializer.validate(),
RadiusBatchSerializer.validate(), and AbstractRadiusBatch.full_clean()/save()
when making the change.
In `@openwisp_radius/integrations/monitoring/tasks.py`:
- Around line 98-106: The loop recomputes existing_methods on every iteration
which is unnecessary; compute existing_methods once before iterating
REGISTRATION_METHOD_CHOICES. Specifically, before the for m in all_methods loop,
build existing_methods = [clean_registration_method(k) for k in
total_registered_users.keys()] and then iterate all_methods (derived from
REGISTRATION_METHOD_CHOICES and clean_registration_method) checking if m not in
existing_methods to set total_registered_users[m] = 0; keep the same variable
names (REGISTRATION_METHOD_CHOICES, clean_registration_method,
total_registered_users, all_methods) so the change is local and clear.
In `@openwisp_radius/integrations/monitoring/tests/test_metrics.py`:
- Around line 389-398: Fix the grammar in the subtest description string used in
the self.subTest call in tests (the string "User does not has OrganizationUser
and RegisteredUser object"); change "does not has" to "does not have" (and
optionally pluralize "object" to "objects") so the description reads e.g. "User
does not have OrganizationUser and RegisteredUser objects".
In `@openwisp_radius/tests/test_admin.py`:
- Around line 1513-1524: The
test_radiusbatch_organization_editable_for_new_objects test contains a redundant
assertion: remove the duplicate check using assertIn on form_html (lines
referencing form_html variable and the assertIn call) because
assertContains(response, 'name="organization"') already verifies the presence;
keep the existing assertContains and delete the subsequent form_html =
response.content.decode() and self.assertIn('name="organization"', form_html) to
eliminate duplication.
- Around line 1498-1511: The current test
test_radiusbatch_organization_readonly_for_existing_objects uses a generic
assertContains(response, "readonly"); update it to specifically assert the
organization field is readonly by checking the form does not contain an editable
input for organization (e.g., assert there's no <input ... name="organization"
...> or no select with name="organization") and instead contains the
organization value (batch.organization.name) rendered as plain text or within a
readonly element; modify the assertions after creating batch (via
_create_radius_batch) and fetching the change URL
(reverse(..._radiusbatch_change, args=[batch.pk])) to target the organization
input/select by name or id and assert its absence and that
batch.organization.name is present.
In `@tests/openwisp2/settings.py`:
- Around line 474-480: Remove the redundant nested try/except in
tests/openwisp2/settings.py by collapsing the two ImportError handlers into a
single try/except around the import statement (the "from .local_settings import
* # noqa: F403,F401" line); keep one try block that attempts the import and one
except ImportError: pass handler, deleting the outer try/except so only a single
ImportError catch remains.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fb9d25b-f64a-46da-ac08-8f4da6c155c7
📒 Files selected for processing (13)
docs/user/rest-api.rstopenwisp_radius/admin.pyopenwisp_radius/api/serializers.pyopenwisp_radius/api/urls.pyopenwisp_radius/api/views.pyopenwisp_radius/integrations/monitoring/tasks.pyopenwisp_radius/integrations/monitoring/tests/test_metrics.pyopenwisp_radius/tests/test_admin.pyopenwisp_radius/tests/test_api/test_api.pypyproject.tomltests/openwisp2/sample_radius/api/views.pytests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Agent
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (1)
**/{docs,documentation}/**/*.{md,rst,adoc,html}
📄 CodeRabbit inference engine (Custom checks)
**/{docs,documentation}/**/*.{md,rst,adoc,html}: Documentation must be updated if a change affects behavior that is already documented
Feature documentation must mention the new feature. If the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/user/rest-api.rst
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
📚 Learning: 2026-03-02T16:47:07.405Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
Applied to files:
docs/user/rest-api.rst
🔇 Additional comments (13)
pyproject.toml (1)
23-23: No actionable change here.Trailing newline at EOF is harmless and keeps file formatting clean.
openwisp_radius/admin.py (1)
465-481: LGTM! Correct implementation of readonly organization field for existing RadiusBatch objects.The
get_readonly_fieldsmethod correctly makesorganizationreadonly for all existingRadiusBatchobjects (both pending and non-pending status), while keeping it editable for new objects. Usingsuper().get_readonly_fields(request, obj)follows proper inheritance patterns.tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py (1)
19-47: LGTM! Cosmetic string formatting improvement.The help_text strings are reformatted using parenthesized expressions for better readability. No functional changes.
tests/openwisp2/settings.py (1)
409-412: LGTM! Correct task path for the monitoring integration.The multi-line string concatenation correctly produces the full task path
openwisp_radius.integrations.monitoring.tasks.write_user_registration_metrics, which matches the@shared_taskdecorated function inopenwisp_radius/integrations/monitoring/tasks.py.openwisp_radius/api/urls.py (1)
81-83: LGTM! New batch detail endpoint correctly added.The URL pattern
/radius/batch/<uuid:pk>/is well-structured and consistent with the existing API URL conventions. Usinguuid:pkis appropriate for theRadiusBatchmodel.tests/openwisp2/sample_radius/api/views.py (1)
4-4: LGTM! Sample app correctly mirrors the new batch update view.The implementation follows the established pattern in this file, importing the base view and creating a local subclass that can be extended if needed. The
batch_detailview is properly instantiated.Also applies to: 108-110, 130-130
openwisp_radius/api/views.py (1)
162-188: LGTM! TheBatchUpdateViewimplementation correctly enforces organization-scoped access.The view properly:
- Filters batches by the user's managed organizations for non-superusers
- Uses the appropriate serializer (
RadiusBatchUpdateSerializer) for update operations to enforce the read-only organization field- Inherits throttling and uses appropriate authentication mechanisms
docs/user/rest-api.rst (1)
859-902: Documentation is comprehensive and accurately reflects the API behavior.The new section properly documents:
- The read-only nature of the
organizationfield for existing batches- The list of updatable vs read-only fields
- All supported HTTP methods (GET, PUT, PATCH)
One minor suggestion for consistency with other endpoint documentation: consider adding a curl example showing how to retrieve or update a batch.
openwisp_radius/tests/test_api/test_api.py (4)
1597-1628: Test correctly validates retrieve and update functionality.The test properly verifies:
- GET returns correct batch data
- PUT successfully updates the
namefieldNote: The PUT request includes
strategy,prefix, andorganization_slugin the payload, but these are either read-only or ignored by theRadiusBatchUpdateSerializer. This is acceptable for testing since the serializer should gracefully handle/ignore these fields, but for clarity, you might consider testing with only the updatable fields (name,expiration_date).
1559-1564: Helper method is well-structured.The
_get_admin_auth_header()method cleanly encapsulates the admin authentication flow for reuse across multiple test methods.
1566-1595: Test correctly verifies organization field is read-only.The test properly:
- Creates a batch with original organization
- Attempts to update with a different organization
- Verifies organization remains unchanged while name is updated
This is the core test for the PR objective.
1630-1662: Permission tests provide good coverage of access control.The test verifies the expected permission hierarchy:
- Unauthenticated → 401
- Regular user → 403
- Admin → 200
openwisp_radius/api/serializers.py (1)
518-558: The serializer correctly enforces organization as read-only for updates.The implementation properly:
- Removes any submitted
organization_slugfrom the data- Marks
organizationinread_only_fields- Handles partial updates by deriving
strategyfrom the instance when not provided
openwisp_radius/tests/test_admin.py
Outdated
| def test_radiusbatch_organization_readonly_for_existing_objects(self): | ||
| """ | ||
| Test that organization field is readonly for existing RadiusBatch objects | ||
| """ | ||
| batch = self._create_radius_batch( | ||
| name="test-batch", strategy="prefix", prefix="test-prefix" | ||
| ) | ||
| url = reverse(f"admin:{self.app_label}_radiusbatch_change", args=[batch.pk]) | ||
|
|
||
| class TestRadiusGroupAdmin(BaseTestCase): | ||
| def setUp(self): | ||
| self.organization = self._create_org() | ||
| self.admin = self._create_admin() | ||
| self.organization.add_user(self.admin, is_admin=True) | ||
| self.client.force_login(self.admin) | ||
|
|
||
| def test_add_radiusgroup_with_inline_check_succeeds(self): | ||
| add_url = reverse("admin:openwisp_radius_radiusgroup_add") | ||
|
|
||
| post_data = { | ||
| # Main RadiusGroup form | ||
| "organization": self.organization.pk, | ||
| "name": "test-group-with-inline", | ||
| "description": "A test group created with an inline check", | ||
| # Inline RadiusGroupCheck formset | ||
| "radiusgroupcheck_set-TOTAL_FORMS": "1", | ||
| "radiusgroupcheck_set-INITIAL_FORMS": "0", | ||
| "radiusgroupcheck_set-0-attribute": "Max-Daily-Session", | ||
| "radiusgroupcheck_set-0-op": ":=", | ||
| "radiusgroupcheck_set-0-value": "3600", | ||
| # Inline RadiusGroupReply formset | ||
| "radiusgroupreply_set-TOTAL_FORMS": "1", | ||
| "radiusgroupreply_set-INITIAL_FORMS": "0", | ||
| "radiusgroupreply_set-0-attribute": "Session-Timeout", | ||
| "radiusgroupreply_set-0-op": "=", | ||
| "radiusgroupreply_set-0-value": "1800", | ||
| } | ||
| response = self.client.get(url) | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
||
| self.assertContains(response, "readonly") | ||
| self.assertContains(response, batch.organization.name) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test assertion is too generic for validating organization field is readonly.
The assertion self.assertContains(response, "readonly") could match any readonly field on the change form (e.g., created, modified, status), not specifically the organization field. Consider a more targeted assertion that verifies the organization field specifically is rendered as readonly.
♻️ Suggested improvement for more precise assertion
response = self.client.get(url)
self.assertEqual(response.status_code, 200)
- self.assertContains(response, "readonly")
- self.assertContains(response, batch.organization.name)
+ # Verify organization is displayed as readonly (not as an editable input)
+ self.assertNotContains(response, 'name="organization"')
+ # Verify the organization name is displayed in the readonly field
+ self.assertContains(response, batch.organization.name)This approach verifies that there's no editable organization input field, which is a more reliable indicator that the field is readonly.
📝 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.
| def test_radiusbatch_organization_readonly_for_existing_objects(self): | |
| """ | |
| Test that organization field is readonly for existing RadiusBatch objects | |
| """ | |
| batch = self._create_radius_batch( | |
| name="test-batch", strategy="prefix", prefix="test-prefix" | |
| ) | |
| url = reverse(f"admin:{self.app_label}_radiusbatch_change", args=[batch.pk]) | |
| class TestRadiusGroupAdmin(BaseTestCase): | |
| def setUp(self): | |
| self.organization = self._create_org() | |
| self.admin = self._create_admin() | |
| self.organization.add_user(self.admin, is_admin=True) | |
| self.client.force_login(self.admin) | |
| def test_add_radiusgroup_with_inline_check_succeeds(self): | |
| add_url = reverse("admin:openwisp_radius_radiusgroup_add") | |
| post_data = { | |
| # Main RadiusGroup form | |
| "organization": self.organization.pk, | |
| "name": "test-group-with-inline", | |
| "description": "A test group created with an inline check", | |
| # Inline RadiusGroupCheck formset | |
| "radiusgroupcheck_set-TOTAL_FORMS": "1", | |
| "radiusgroupcheck_set-INITIAL_FORMS": "0", | |
| "radiusgroupcheck_set-0-attribute": "Max-Daily-Session", | |
| "radiusgroupcheck_set-0-op": ":=", | |
| "radiusgroupcheck_set-0-value": "3600", | |
| # Inline RadiusGroupReply formset | |
| "radiusgroupreply_set-TOTAL_FORMS": "1", | |
| "radiusgroupreply_set-INITIAL_FORMS": "0", | |
| "radiusgroupreply_set-0-attribute": "Session-Timeout", | |
| "radiusgroupreply_set-0-op": "=", | |
| "radiusgroupreply_set-0-value": "1800", | |
| } | |
| response = self.client.get(url) | |
| self.assertEqual(response.status_code, 200) | |
| self.assertContains(response, "readonly") | |
| self.assertContains(response, batch.organization.name) | |
| def test_radiusbatch_organization_readonly_for_existing_objects(self): | |
| """ | |
| Test that organization field is readonly for existing RadiusBatch objects | |
| """ | |
| batch = self._create_radius_batch( | |
| name="test-batch", strategy="prefix", prefix="test-prefix" | |
| ) | |
| url = reverse(f"admin:{self.app_label}_radiusbatch_change", args=[batch.pk]) | |
| response = self.client.get(url) | |
| self.assertEqual(response.status_code, 200) | |
| # Verify organization is displayed as readonly (not as an editable input) | |
| self.assertNotContains(response, 'name="organization"') | |
| # Verify the organization name is displayed in the readonly field | |
| self.assertContains(response, batch.organization.name) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/tests/test_admin.py` around lines 1498 - 1511, The current
test test_radiusbatch_organization_readonly_for_existing_objects uses a generic
assertContains(response, "readonly"); update it to specifically assert the
organization field is readonly by checking the form does not contain an editable
input for organization (e.g., assert there's no <input ... name="organization"
...> or no select with name="organization") and instead contains the
organization value (batch.organization.name) rendered as plain text or within a
readonly element; modify the assertions after creating batch (via
_create_radius_batch) and fetching the change URL
(reverse(..._radiusbatch_change, args=[batch.pk])) to target the organization
input/select by name or id and assert its absence and that
batch.organization.name is present.
Multiple Test Failures in
|
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
from
6797b23 to
9b6d9a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_radius/api/serializers.py (1)
532-553:⚠️ Potential issue | 🔴 CriticalLine 552 bypasses the parent validation chain and skips update-time model checks.
Calling
serializers.ModelSerializer.validate(self, data)at Line 552 skipsRadiusBatchSerializer.validate(), so update requests bypass batch-specific validation logic andfull_clean()execution path.Suggested fix
def validate(self, data): @@ - return serializers.ModelSerializer.validate(self, data) + validated_data = serializers.ModelSerializer.validate(self, data) + # keep organization immutable + validated_data.pop("organization_slug", None) + + # validate updated state, not only persisted instance state + if self.instance is not None: + for field, value in validated_data.items(): + if field in {"number_of_users"}: + continue + setattr(self.instance, field, value) + self.instance.full_clean() + + return validated_data#!/bin/bash # Verify current validation chain in serializers rg -n -C4 "class RadiusBatchSerializer|def validate\(|class RadiusBatchUpdateSerializer|return serializers.ModelSerializer.validate" openwisp_radius/api/serializers.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/api/serializers.py` around lines 532 - 553, The current validate method calls serializers.ModelSerializer.validate(self, data), which bypasses the parent validation chain (e.g., RadiusBatchSerializer.validate) and skips update-time checks; change that call to invoke the proper parent validator via the MRO (use super().validate(data)) so the RadiusBatchSerializer.validate and any full_clean/update logic run for updates, ensuring batch-specific validation is executed; locate the validate method shown and replace the direct ModelSerializer call with a super() call (or explicitly call RadiusBatchSerializer.validate(self, data) if the class hierarchy requires it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/api/serializers.py`:
- Around line 554-558: RadiusBatchUpdateSerializer currently declares
Meta.read_only_fields as ("created", "modified", "user_credentials",
"organization") but must also mark "strategy", "csvfile", "prefix", and "users"
read-only to match the API contract; update RadiusBatchUpdateSerializer.Meta to
include those four fields in the read_only_fields tuple (e.g., ("created",
"modified", "user_credentials", "organization", "strategy", "csvfile", "prefix",
"users")) so the serializer enforces immutability after creation.
---
Duplicate comments:
In `@openwisp_radius/api/serializers.py`:
- Around line 532-553: The current validate method calls
serializers.ModelSerializer.validate(self, data), which bypasses the parent
validation chain (e.g., RadiusBatchSerializer.validate) and skips update-time
checks; change that call to invoke the proper parent validator via the MRO (use
super().validate(data)) so the RadiusBatchSerializer.validate and any
full_clean/update logic run for updates, ensuring batch-specific validation is
executed; locate the validate method shown and replace the direct
ModelSerializer call with a super() call (or explicitly call
RadiusBatchSerializer.validate(self, data) if the class hierarchy requires it).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad051131-26a2-4d5b-903b-863729c182ba
📒 Files selected for processing (3)
docs/user/rest-api.rstopenwisp_radius/admin.pyopenwisp_radius/api/serializers.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | 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.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/{docs,documentation}/**/*.{md,rst,adoc,html}
📄 CodeRabbit inference engine (Custom checks)
**/{docs,documentation}/**/*.{md,rst,adoc,html}: Documentation must be updated if a change affects behavior that is already documented
Feature documentation must mention the new feature. If the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/user/rest-api.rst
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
📚 Learning: 2026-03-02T16:47:07.405Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
Applied to files:
docs/user/rest-api.rst
🔇 Additional comments (1)
openwisp_radius/admin.py (1)
465-481: Readonly scoping fororganizationis correctly implemented.This now keeps
organizationeditable on create and readonly on existingRadiusBatchobjects, which matches the stated objective.
| class Meta: | ||
| model = RadiusBatch | ||
| fields = "__all__" | ||
| read_only_fields = ("created", "modified", "user_credentials", "organization") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Cross-check serializer mutability vs documented read-only contract
rg -n -C3 "class RadiusBatchUpdateSerializer|read_only_fields" openwisp_radius/api/serializers.py
rg -n -C3 "Parameters for \\*\\*PUT\\*\\*/\\*\\*PATCH\\*\\*|Fields that are \\*\\*read-only\\*\\*" docs/user/rest-api.rstRepository: openwisp/openwisp-radius
Length of output: 2179
🏁 Script executed:
# Get full context of RadiusBatch API documentation
sed -n '880,920p' docs/user/rest-api.rstRepository: openwisp/openwisp-radius
Length of output: 1424
🏁 Script executed:
# Check RadiusBatch model definition and fields
rg -n "class RadiusBatch" openwisp_radius/models.py -A 50Repository: openwisp/openwisp-radius
Length of output: 1129
🏁 Script executed:
# Verify the parent RadiusBatchSerializer definition to understand inheritance
sed -n '512,520p' openwisp_radius/api/serializers.pyRepository: openwisp/openwisp-radius
Length of output: 336
RadiusBatchUpdateSerializer's read_only_fields is incomplete and does not enforce the documented API contract.
The serializer declares only ("created", "modified", "user_credentials", "organization") as read-only, but the API documentation specifies that strategy, csvfile, prefix, and users are also immutable after creation. This mismatch allows these fields to remain writable at the serializer level, contradicting the documented behavior.
Suggested fix
class Meta:
model = RadiusBatch
fields = "__all__"
- read_only_fields = ("created", "modified", "user_credentials", "organization")
+ read_only_fields = (
+ "created",
+ "modified",
+ "user_credentials",
+ "organization",
+ "strategy",
+ "csvfile",
+ "prefix",
+ "users",
+ "status",
+ )📝 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.
| class Meta: | |
| model = RadiusBatch | |
| fields = "__all__" | |
| read_only_fields = ("created", "modified", "user_credentials", "organization") | |
| class Meta: | |
| model = RadiusBatch | |
| fields = "__all__" | |
| read_only_fields = ( | |
| "created", | |
| "modified", | |
| "user_credentials", | |
| "organization", | |
| "strategy", | |
| "csvfile", | |
| "prefix", | |
| "users", | |
| "status", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/api/serializers.py` around lines 554 - 558,
RadiusBatchUpdateSerializer currently declares Meta.read_only_fields as
("created", "modified", "user_credentials", "organization") but must also mark
"strategy", "csvfile", "prefix", and "users" read-only to match the API
contract; update RadiusBatchUpdateSerializer.Meta to include those four fields
in the read_only_fields tuple (e.g., ("created", "modified", "user_credentials",
"organization", "strategy", "csvfile", "prefix", "users")) so the serializer
enforces immutability after creation.
9c476d5 to
17639fc
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_radius/api/serializers.py (1)
560-564:⚠️ Potential issue | 🟠 MajorEnforce immutable batch fields in update serializer.
RadiusBatchUpdateSerializer.Meta.read_only_fieldsstill leavesstrategy,csvfile,prefix, anduserswritable, despite the update contract documenting them as non-updatable. This allows unintended post-creation mutation.🔧 Proposed fix
class Meta: model = RadiusBatch fields = "__all__" - read_only_fields = ("created", "modified", "user_credentials", "organization") + read_only_fields = ( + "created", + "modified", + "user_credentials", + "organization", + "strategy", + "csvfile", + "prefix", + "users", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/api/serializers.py` around lines 560 - 564, RadiusBatchUpdateSerializer currently allows mutation of fields that should be immutable post-creation; update RadiusBatchUpdateSerializer.Meta.read_only_fields to include "strategy", "csvfile", "prefix", and "users" so they become read-only on updates (i.e., add those names to the existing tuple alongside "created", "modified", "user_credentials", "organization"), ensuring the serializer enforces the documented non-updatable contract for RadiusBatch instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_radius/api/serializers.py`:
- Around line 560-564: RadiusBatchUpdateSerializer currently allows mutation of
fields that should be immutable post-creation; update
RadiusBatchUpdateSerializer.Meta.read_only_fields to include "strategy",
"csvfile", "prefix", and "users" so they become read-only on updates (i.e., add
those names to the existing tuple alongside "created", "modified",
"user_credentials", "organization"), ensuring the serializer enforces the
documented non-updatable contract for RadiusBatch instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43150fc7-7b1f-46ec-a961-cb0f5dad761c
📒 Files selected for processing (7)
docs/user/rest-api.rstopenwisp_radius/admin.pyopenwisp_radius/api/serializers.pyopenwisp_radius/integrations/monitoring/tasks.pyopenwisp_radius/integrations/monitoring/tests/test_metrics.pyopenwisp_radius/tests/test_api/test_api.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (1)
**/{docs,documentation}/**/*.{md,rst,adoc,html}
📄 CodeRabbit inference engine (Custom checks)
**/{docs,documentation}/**/*.{md,rst,adoc,html}: Documentation must be updated if a change affects behavior that is already documented
Feature documentation must mention the new feature. If the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/user/rest-api.rst
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
📚 Learning: 2026-03-02T16:47:07.405Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
Applied to files:
docs/user/rest-api.rst
🔇 Additional comments (6)
openwisp_radius/integrations/monitoring/tasks.py (1)
100-106: Good optimization and completeness fix.Computing
existing_methodsonce and backfilling missing methods with0is clean and avoids repeated work in the loop.tests/openwisp2/settings.py (1)
474-477: Local settings import handling is clean.Single guarded import with explicit
# noqa: F403,F401is clear and lint-safe.openwisp_radius/integrations/monitoring/tests/test_metrics.py (1)
390-398: Nice test hardening for optional RegisteredUser state.The
try/except RegisteredUser.DoesNotExistbranch makes this subtest resilient across setup variations.docs/user/rest-api.rst (1)
859-902: Documentation update is clear and complete for the new batch detail endpoint.The new retrieve/update section and explicit read-only field list make the API behavior easy to consume.
openwisp_radius/admin.py (1)
465-481: Admin readonly behavior now aligns with the objective.Making
organizationreadonly for existingRadiusBatchobjects while keeping it editable on create is correct.openwisp_radius/tests/test_api/test_api.py (1)
1567-1663: Good API test coverage for batch detail update path.These tests cover organization immutability, retrieve/update flow, and permission boundaries for the new endpoint.
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
2 times, most recently
from
0131dea to
0cb10de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/api/serializers.py`:
- Around line 524-558: The serializer still declares a writable
organization_slug field and accepts number_of_users in validate even though both
are ignored for updates; update the serializer to make organization_slug and
number_of_users write_only only on create (or remove them from update) by
changing their declaration or behavior and adjust validate(self, data) to not
accept these keys for update: specifically, ensure RadiusOrganizationField
(organization_slug) is not considered writable when self.instance is present and
that number_of_users is removed from validated_data for updates (or marked
read_only for existing instances) so the browsable/OpenAPI docs and PUT/PATCH
paths no longer advertise these as editable; keep existing model-level
full_clean/apply logic (ModelSerializer.validate and instance.full_clean) for
other fields.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90050783-477b-4bf7-83cf-134367eff24b
📒 Files selected for processing (4)
docs/user/rest-api.rstopenwisp_radius/admin.pyopenwisp_radius/api/serializers.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/{docs,documentation}/**/*.{md,rst,adoc,html}
📄 CodeRabbit inference engine (Custom checks)
**/{docs,documentation}/**/*.{md,rst,adoc,html}: Documentation must be updated if a change affects behavior that is already documented
Feature documentation must mention the new feature. If the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/user/rest-api.rst
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
📚 Learning: 2026-03-02T16:47:07.405Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
Applied to files:
docs/user/rest-api.rst
ReStructuredText Formatting FailureHello @Eeshu-Yadav, The CI failed because of a ReStructuredText formatting issue in the documentation. Fix: |
- Resolve remaining git conflict markers in admin.py - Fix whitespace issue in serializers.py - Format rest-api.rst documentation
Eeshu-Yadav
force-pushed
the
issues/609-batch-user-org-field-readonly
branch
from
0cb10de to
e2aea2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/rest-api.rst`:
- Around line 876-886: Update the REST API docs for the batch GET/PUT/PATCH
endpoints to explicitly list which fields are updatable: add a parameter table
for PUT/PATCH (similar to the POST table) that names each field (organization,
strategy, csvfile, prefix, users, status, id, etc.) and marks whether it is
read-only, write-on-create-only, or writable (and any conditions, e.g.,
status/strategy/csvfile/users immutable once batch is not pending). Reference
the same field names used in the API/serializer (organization, strategy,
csvfile, prefix, users, status) and document allowed transitions or constraints
(e.g., status changes only via admin actions or when batch is pending) so
PATCH/PUT behavior is unambiguous.
In `@openwisp_radius/api/serializers.py`:
- Around line 560-563: The serializer's Meta.read_only_fields currently only
includes ("created", "modified", "user_credentials", "organization") which
allows API edits that the admin UI forbids for non-pending RadiusBatch records;
update the RadiusBatch serializer (referencing the Meta class and the
RadiusBatch model) to block changes to the same fields the admin makes readonly
for non-pending batches — strategy, prefix, csvfile, number_of_users, users,
expiration_date, name, and status — by overriding the serializer's update() (and
optionally validate()/create() if needed) to check instance.status (compare
against the Pending status constant on RadiusBatch) and either remove those keys
from validated_data or raise a ValidationError when an attempt is made to change
them for non-pending batches, leaving organization and the existing
created/modified/user_credentials read-only as before.
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 1631-1663: Add a cross-organization case to
test_batch_update_permissions: create a second Organization and a RadiusBatch
that belongs to that other organization, then authenticate as an organization
manager who has permissions only for the first org (use whatever helper you have
for manager auth, e.g. _get_org_manager_auth_header or create a manager user and
Token) and attempt to PATCH the other-org batch URL; assert the response is 403
to validate BatchUpdateView.get_queryset() filtering. Ensure you reference the
existing test method test_batch_update_permissions, the RadiusBatch instance for
the other organization, and BatchUpdateView.get_queryset() behavior when adding
this assertion.
- Around line 1559-1565: The _get_admin_auth_header helper assumes an admin
exists; make it self-contained by ensuring the admin user is created before
attempting login: call self._get_admin() (or otherwise create the admin) at the
start of _get_admin_auth_header so the POST to reverse("radius:user_auth_token",
args=[self.default_org.slug]) never fails with 400; keep the rest of the logic
(login_payload, client.post, status check, return token) intact and do not rely
on _radius_batch_post_request being called by callers.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35cf0550-af31-4970-bfe6-adcf3b328558
📒 Files selected for processing (4)
docs/user/rest-api.rstopenwisp_radius/admin.pyopenwisp_radius/api/serializers.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/{docs,documentation}/**/*.{md,rst,adoc,html}
📄 CodeRabbit inference engine (Custom checks)
**/{docs,documentation}/**/*.{md,rst,adoc,html}: Documentation must be updated if a change affects behavior that is already documented
Feature documentation must mention the new feature. If the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/user/rest-api.rst
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
📚 Learning: 2026-03-02T16:47:07.405Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
Applied to files:
docs/user/rest-api.rst
🔇 Additional comments (2)
openwisp_radius/api/serializers.py (1)
524-530:organization_slugfield is misleading in API documentation.While
organization_slugis correctly ignored during validation (line 537), declaring it as a field means it still appears in the browsable API and OpenAPI schema as an accepted input for PUT/PATCH requests. This contradicts the documentation stating organization is read-only.Consider removing it from the serializer entirely for updates:
💡 Suggested fix
class RadiusBatchUpdateSerializer(RadiusBatchSerializer): """ Serializer for updating RadiusBatch objects. Makes the organization field readonly for existing objects. """ - organization_slug = RadiusOrganizationField( - help_text=("Slug of the organization for creating radius batch."), - required=False, - label="organization", - slug_field="slug", - write_only=True, - ) + def get_fields(self): + fields = super().get_fields() + fields.pop("organization_slug", None) + return fieldsopenwisp_radius/admin.py (1)
465-481: LGTM!The
get_readonly_fieldsimplementation correctly makes theorganizationfield readonly for all existingRadiusBatchobjects (both pending and non-pending), while keeping it editable for new objects. This aligns with the PR objective to prevent post-creation changes to the organization field.
| Parameters for **GET**: | ||
|
|
||
| ===== ================= | ||
| Param Description | ||
| ===== ================= | ||
| id UUID of the batch | ||
| ===== ================= | ||
|
|
||
| The ``organization`` field is the only field that is explicitly | ||
| **read-only** and cannot be updated via this endpoint. All other editable | ||
| fields can be modified through **PUT** or **PATCH** requests. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Documentation could be more specific about updatable fields.
The documentation states that organization is read-only but implies "all other editable fields" can be modified. However, per the admin implementation and likely serializer constraints, fields like strategy, csvfile, prefix, users, and status may also be read-only after creation (especially for non-pending batches). Consider adding a parameter table for PUT/PATCH similar to the POST section, clarifying which fields are actually updatable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/user/rest-api.rst` around lines 876 - 886, Update the REST API docs for
the batch GET/PUT/PATCH endpoints to explicitly list which fields are updatable:
add a parameter table for PUT/PATCH (similar to the POST table) that names each
field (organization, strategy, csvfile, prefix, users, status, id, etc.) and
marks whether it is read-only, write-on-create-only, or writable (and any
conditions, e.g., status/strategy/csvfile/users immutable once batch is not
pending). Reference the same field names used in the API/serializer
(organization, strategy, csvfile, prefix, users, status) and document allowed
transitions or constraints (e.g., status changes only via admin actions or when
batch is pending) so PATCH/PUT behavior is unambiguous.
| class Meta: | ||
| model = RadiusBatch | ||
| fields = "__all__" | ||
| read_only_fields = ("created", "modified", "user_credentials", "organization") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check admin readonly fields vs serializer readonly fields
echo "=== Admin readonly fields for non-pending batches ==="
rg -n "strategy|organization|prefix|csvfile|number_of_users|users|expiration_date|name|status" openwisp_radius/admin.py | grep -A2 -B2 "get_readonly"
echo ""
echo "=== Serializer read_only_fields ==="
rg -n "read_only_fields" openwisp_radius/api/serializers.pyRepository: openwisp/openwisp-radius
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Search for RadiusBatchAdmin and get_readonly_fields
echo "=== Search for RadiusBatchAdmin class ==="
rg -A 30 "class RadiusBatchAdmin" openwisp_radius/admin.py
echo ""
echo "=== Search for get_readonly_fields method ==="
rg -B 5 -A 20 "def get_readonly_fields" openwisp_radius/admin.py | head -100Repository: openwisp/openwisp-radius
Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Find RadiusBatchUpdateSerializer and check for status handling
echo "=== RadiusBatchUpdateSerializer full implementation ==="
rg -B 50 "class RadiusBatchUpdateSerializer" openwisp_radius/api/serializers.py | tail -60
echo ""
echo "=== Check validate method for status handling ==="
rg -A 40 "def validate" openwisp_radius/api/serializers.py | grep -A 40 "class RadiusBatchUpdateSerializer" | tail -45Repository: openwisp/openwisp-radius
Length of output: 3563
🏁 Script executed:
#!/bin/bash
# Check RadiusBatch model for clean() method
echo "=== RadiusBatch model clean() method ==="
rg -B 5 -A 30 "def clean" openwisp_radius/models.py | grep -A 35 "class RadiusBatch" | tail -50
echo ""
echo "=== Check RadiusBatch status field and related logic ==="
rg -B 3 -A 8 "status.*=" openwisp_radius/models.py | grep -A 10 -B 3 "RadiusBatch\|pending\|COMPLETED" | head -60Repository: openwisp/openwisp-radius
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Find RadiusBatch model
echo "=== Find RadiusBatch model ==="
fd "models.py" openwisp_radius/ | head -5
echo ""
echo "=== Look for RadiusBatch class definition ==="
rg -A 50 "class RadiusBatch" openwisp_radius/models.py | head -80
echo ""
echo "=== Check if there's a clean method anywhere in the file ==="
rg "def clean" openwisp_radius/models.pyRepository: openwisp/openwisp-radius
Length of output: 1272
🏁 Script executed:
#!/bin/bash
# Find AbstractRadiusBatch and check for clean() method
echo "=== Look for AbstractRadiusBatch class ==="
rg -B 5 -A 100 "class AbstractRadiusBatch" openwisp_radius/base/models.py | head -150
echo ""
echo "=== Check for clean method ==="
rg -B 3 -A 20 "def clean" openwisp_radius/base/models.py | grep -A 22 -B 3 "RadiusBatch"Repository: openwisp/openwisp-radius
Length of output: 3419
🏁 Script executed:
#!/bin/bash
# Get the full clean method from AbstractRadiusBatch
rg -A 40 "def clean\(self\):" openwisp_radius/base/models.pyRepository: openwisp/openwisp-radius
Length of output: 9623
read_only_fields does not match admin readonly behavior for non-pending batches.
The admin's get_readonly_fields makes strategy, prefix, csvfile, number_of_users, users, expiration_date, name, and status all readonly for non-pending batches. However, this serializer only marks organization as readonly (plus the standard created, modified, user_credentials). The model's clean() method does not enforce this restriction either.
This creates an inconsistency: the admin prevents editing these fields for completed batches, but the API allows it. Consider making these fields conditionally readonly based on batch status to match admin behavior, or document this difference explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/api/serializers.py` around lines 560 - 563, The serializer's
Meta.read_only_fields currently only includes ("created", "modified",
"user_credentials", "organization") which allows API edits that the admin UI
forbids for non-pending RadiusBatch records; update the RadiusBatch serializer
(referencing the Meta class and the RadiusBatch model) to block changes to the
same fields the admin makes readonly for non-pending batches — strategy, prefix,
csvfile, number_of_users, users, expiration_date, name, and status — by
overriding the serializer's update() (and optionally validate()/create() if
needed) to check instance.status (compare against the Pending status constant on
RadiusBatch) and either remove those keys from validated_data or raise a
ValidationError when an attempt is made to change them for non-pending batches,
leaving organization and the existing created/modified/user_credentials
read-only as before.
| def _get_admin_auth_header(self): | ||
| """Helper method to get admin authentication header""" | ||
| login_payload = {"username": "admin", "password": "tester"} | ||
| login_url = reverse("radius:user_auth_token", args=[self.default_org.slug]) | ||
| login_response = self.client.post(login_url, data=login_payload) | ||
| self.assertEqual(login_response.status_code, status.HTTP_200_OK) | ||
| return f"Bearer {login_response.json()['key']}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential issue: _get_admin() not called before authentication.
The helper method authenticates as "admin"/"tester" but doesn't call self._get_admin() to ensure the admin user exists first. This works in the current tests because they all call _radius_batch_post_request() before _get_admin_auth_header(), which internally creates the admin user. However, if this helper is used in future tests without that precondition, it will fail with a 400 response.
Consider making the helper self-contained:
💡 Suggested improvement
def _get_admin_auth_header(self):
"""Helper method to get admin authentication header"""
+ self._get_admin() # Ensure admin user exists
login_payload = {"username": "admin", "password": "tester"}
login_url = reverse("radius:user_auth_token", args=[self.default_org.slug])
login_response = self.client.post(login_url, data=login_payload)
self.assertEqual(login_response.status_code, status.HTTP_200_OK)
return f"Bearer {login_response.json()['key']}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/tests/test_api/test_api.py` around lines 1559 - 1565, The
_get_admin_auth_header helper assumes an admin exists; make it self-contained by
ensuring the admin user is created before attempting login: call
self._get_admin() (or otherwise create the admin) at the start of
_get_admin_auth_header so the POST to reverse("radius:user_auth_token",
args=[self.default_org.slug]) never fails with 400; keep the rest of the logic
(login_payload, client.post, status check, return token) intact and do not rely
on _radius_batch_post_request being called by callers.
| def test_batch_update_permissions(self): | ||
| """ | ||
| Test that proper permissions are required for updating RadiusBatch objects | ||
| """ | ||
| data = self._radius_batch_prefix_data() | ||
| response = self._radius_batch_post_request(data) | ||
| self.assertEqual(response.status_code, 201) | ||
| batch = RadiusBatch.objects.get() | ||
|
|
||
| url = reverse("radius:batch_detail", args=[batch.pk]) | ||
|
|
||
| response = self.client.patch(url, {"name": "new-name"}) | ||
| self.assertEqual(response.status_code, 401) | ||
|
|
||
| user = self._get_user() | ||
| user_token = Token.objects.create(user=user) | ||
| header = f"Bearer {user_token.key}" | ||
| response = self.client.patch( | ||
| url, | ||
| json.dumps({"name": "new-name"}), | ||
| HTTP_AUTHORIZATION=header, | ||
| content_type="application/json", | ||
| ) | ||
| self.assertEqual(response.status_code, 403) | ||
|
|
||
| header = self._get_admin_auth_header() | ||
| response = self.client.patch( | ||
| url, | ||
| json.dumps({"name": "new-name"}), | ||
| HTTP_AUTHORIZATION=header, | ||
| content_type="application/json", | ||
| ) | ||
| self.assertEqual(response.status_code, 200) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding cross-organization permission test.
The permission tests cover unauthenticated, unprivileged user, and admin access. Consider adding a test case for an organization manager who has permissions but tries to update a batch belonging to a different organization they don't manage - this would verify the queryset filtering in BatchUpdateView.get_queryset().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/tests/test_api/test_api.py` around lines 1631 - 1663, Add a
cross-organization case to test_batch_update_permissions: create a second
Organization and a RadiusBatch that belongs to that other organization, then
authenticate as an organization manager who has permissions only for the first
org (use whatever helper you have for manager auth, e.g.
_get_org_manager_auth_header or create a manager user and Token) and attempt to
PATCH the other-org batch URL; assert the response is 403 to validate
BatchUpdateView.get_queryset() filtering. Ensure you reference the existing test
method test_batch_update_permissions, the RadiusBatch instance for the other
organization, and BatchUpdateView.get_queryset() behavior when adding this
assertion.
Description
This PR makes the organization field readonly for existing RadiusBatch objects in both the admin interface and REST API, addressing issue #609.
When the batch user creation object has been created, there's no point in changing the organization field, and this change disallows modification to maintain data integrity.
Changes
Admin Interface
RadiusBatchAdmin.get_readonly_fields()to includeorganizationfield for existing objectsREST API
RadiusBatchUpdateSerializerwith organization as readonly fieldBatchUpdateViewfor handling batch detail operations (retrieve/update)/radius/batch/<uuid:pk>/)Testing
test_radiusbatch_organization_readonly_for_existing_objectstest_radiusbatch_organization_editable_for_new_objectstest_batch_update_organization_readonlytest_batch_retrieve_and_update_apitest_batch_update_permissionsChecklist
Fixes #609