[config] Add API support for organization config settings #700 #1208
Conversation
📝 WalkthroughWalkthroughThis change introduces API support for managing organization configuration settings. It adds two new serializers for handling Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant View as OrganizationViewSet
participant Serializer as OrganizationSerializer
participant Org as Organization Model
participant ConfigSettings as OrganizationConfigSettings Model
participant DB as Database
Client->>View: POST /organization/ with config_settings
View->>Serializer: validate(data)
Serializer->>Serializer: create(validated_data)
Serializer->>Org: create(org_data)
Org->>DB: INSERT Organization
Serializer->>ConfigSettings: get_or_create(organization, config_data)
ConfigSettings->>DB: INSERT/UPDATE OrganizationConfigSettings
Serializer->>View: return created organization with config_settings
View->>Client: 201 Created with full organization object
sequenceDiagram
participant Client as API Client
participant View as OrganizationViewSet
participant Serializer as OrganizationSerializer
participant Org as Organization Model
participant ConfigSettings as OrganizationConfigSettings Model
participant DB as Database
Client->>View: PATCH /organization/{id}/ with config_settings
View->>Serializer: validate(data, instance)
Serializer->>Serializer: update(instance, validated_data)
Serializer->>Org: save() with org_fields
Org->>DB: UPDATE Organization
Serializer->>ConfigSettings: get_or_create(organization, config_data)
ConfigSettings->>DB: UPDATE/INSERT OrganizationConfigSettings
Serializer->>View: return updated organization
View->>Client: 200 OK with updated organization object
sequenceDiagram
participant Client as API Client
participant View as OrganizationViewSet
participant Serializer as OrganizationSerializer
participant Org as Organization Model
participant ConfigSettings as OrganizationConfigSettings Model
participant DB as Database
Client->>View: GET /organization/{id}/
View->>Org: retrieve(id)
Org->>DB: SELECT Organization with related config_settings
View->>Serializer: serialize(organization_instance)
Serializer->>ConfigSettings: nested serialize config_settings
Serializer->>View: return serialized organization
View->>Client: 200 OK with organization and config_settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/api/serializers.py`:
- Around line 383-388: The serializer's shared_secret CharField (named
shared_secret) lacks max_length validation and should match the model's KeyField
constraint; update the shared_secret field definition to include max_length=32
so the serializer enforces min_length=8 and max_length=32, keeping
required=False and allow_blank=False and leaving write_only as-is per tests.
- Around line 404-420: The create method's try/except around accessing
organization.config_settings is redundant because a newly created Organization
won't have a related OrganizationConfigSettings, so always taking the except
branch; update the create(self, validated_data) implementation to remove the
try/except and directly create (or update_or_create) the related
OrganizationConfigSettings when config_settings_data is present: replace the
try/except that touches organization.config_settings with a direct
OrganizationConfigSettings.objects.create(organization=organization,
**config_settings_data) or use
OrganizationConfigSettings.objects.update_or_create(organization=organization,
defaults=config_settings_data) if you need to handle existing instances.
In `@openwisp_controller/config/tests/test_api.py`:
- Around line 1342-1344: A debug print statement is present in the test where
the response is checked (the block using the variable response and the assertion
self.assertEqual(response.status_code, 201)); remove the conditional print line
that outputs response.data so the test does not produce noisy output in CI and
keep only the assertion (ensure the if-check and print are deleted, leaving the
assertion on response.status_code intact).
- Around line 1408-1410: Remove the temporary debug print inside the test:
delete the conditional branch that prints response.data (the "if
response.status_code != 200: print(f\"Error response: {response.data}\")" lines)
and leave the assertion self.assertEqual(response.status_code, 200) intact so
the test fails without printing debug output; locate this in the API test around
the response/status_code check in test_api.py.
🧹 Nitpick comments (7)
openwisp_controller/config/api/serializers.py (2)
380-394: Trailing whitespace and minor style issue.Line 380 has trailing whitespace after the class definition. Also, consider adding a trailing period to the
help_textfor consistency with other serializers in this file.🧹 Proposed cleanup
- -class OrganizationConfigSettingsSerializer(serializers.ModelSerializer): + + +class OrganizationConfigSettingsSerializer(serializers.ModelSerializer): registration_enabled = serializers.BooleanField(required=False, default=False) shared_secret = serializers.CharField( - required=False, - allow_blank=False, + required=False, + allow_blank=False, min_length=8, - help_text='Used for automatic registration of devices' + help_text='Used for automatic registration of devices.' ) context = serializers.JSONField(required=False, default=dict)
431-441: Inconsistent handling betweencreateandupdatemethods.In
update, whenconfig_settings_datais provided butconfig_settingsdoesn't exist, a newOrganizationConfigSettingsis created without the provided data (line 435-437), then the data is applied in a separate loop (lines 439-441). This works but results in two saves.In contrast,
createpasses**config_settings_datadirectly toOrganizationConfigSettings.objects.create().Consider aligning the approaches for consistency and efficiency:
♻️ Proposed refactor for update method
if config_settings_data is not None: try: config_settings = instance.config_settings except OrganizationConfigSettings.DoesNotExist: - config_settings = OrganizationConfigSettings.objects.create( - organization=instance + OrganizationConfigSettings.objects.create( + organization=instance, + **config_settings_data ) - - for attr, value in config_settings_data.items(): - setattr(config_settings, attr, value) - config_settings.save() + else: + for attr, value in config_settings_data.items(): + setattr(config_settings, attr, value) + config_settings.save()openwisp_controller/config/api/views.py (2)
292-300: Line length exceeds 88 characters and missingDestroyModelMixin.Line 298 exceeds the 88-character limit enforced by flake8 in this repository. Additionally, the viewset includes Create, Retrieve, Update, and List operations but notably excludes Delete (
DestroyModelMixin). Verify this is intentional.♻️ Proposed formatting fix
-class OrganizationViewSet(ProtectedAPIMixin, - mixins.CreateModelMixin, - mixins.RetrieveModelMixin, - mixins.UpdateModelMixin, - mixins.ListModelMixin, - viewsets.GenericViewSet): - queryset = Organization.objects.select_related('config_settings').order_by('-created') +class OrganizationViewSet( + ProtectedAPIMixin, + mixins.CreateModelMixin, + mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, + mixins.ListModelMixin, + viewsets.GenericViewSet, +): + queryset = Organization.objects.select_related( + 'config_settings' + ).order_by('-created') serializer_class = OrganizationSerializer pagination_class = ListViewPaginationBased on learnings: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88).
292-300: Consider adding filter backends for organization filtering.Other viewsets in this file (e.g.,
TemplateListCreateView,DeviceListCreateView) useDjangoFilterBackendfor filtering. Consider adding similar filtering capability toOrganizationViewSetfor consistency, especially if the list endpoint could return many organizations.openwisp_controller/config/tests/test_api.py (3)
1354-1357: Redundantload_modelcall inside test.
Organizationis already loaded viaself._get_org()andself._create_org()methods. Theload_modelcall here is redundant and adds unnecessary overhead.♻️ Proposed simplification
- Organization = load_model('openwisp_users', 'Organization') - org = Organization.objects.get(name='Test Org with Config') + from openwisp_users.models import Organization + org = Organization.objects.get(name='Test Org with Config')Or better, simply use the response data to verify:
- Organization = load_model('openwisp_users', 'Organization') - org = Organization.objects.get(name='Test Org with Config') - self.assertIsNotNone(org.config_settings) - self.assertEqual(org.config_settings.shared_secret, 'my-secret-123') + # The response already confirms config_settings were created correctly + # Additional DB verification is redundant given the assertions above
1362-1367: Consider importingOrganizationConfigSettingsat module level.The model is loaded inside the test method. For consistency with other models in this file (lines 26-34), consider importing it at the module level.
♻️ Proposed import addition at top of file
Add near line 34:
OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings")Then simplify the test methods by removing the inline
load_modelcalls.
1328-1421: Consider adding test coverage for edge cases and error scenarios.The tests cover the happy path well, but consider adding tests for:
- List endpoint:
test_organization_list_api- verify pagination and filtering work- Validation errors: Creating organization with invalid
shared_secret(less than 8 chars)- Permission tests: Non-admin users accessing organization endpoints
- Create without config_settings: Verify organization can be created without nested config_settings
- Partial update edge case: PATCH with empty
config_settings: {}vsconfig_settings: nullDo you want me to generate additional test cases for these scenarios?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/config/tests/test_api.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/config/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/config/tests/test_api.py
🧬 Code graph analysis (3)
openwisp_controller/config/api/serializers.py (2)
openwisp_controller/config/models.py (1)
OrganizationConfigSettings(95-103)tests/openwisp2/sample_config/models.py (1)
OrganizationConfigSettings(97-104)
openwisp_controller/config/api/views.py (4)
openwisp_controller/config/api/serializers.py (1)
OrganizationSerializer(396-443)openwisp_controller/mixins.py (1)
ProtectedAPIMixin(34-35)tests/openwisp2/sample_users/models.py (1)
Organization(32-36)openwisp_controller/geo/api/views.py (1)
ListViewPagination(54-57)
openwisp_controller/config/tests/test_api.py (4)
openwisp_controller/config/tests/test_controller.py (1)
_create_org(44-49)openwisp_controller/config/models.py (1)
OrganizationConfigSettings(95-103)tests/openwisp2/sample_config/models.py (1)
OrganizationConfigSettings(97-104)openwisp_controller/config/api/serializers.py (3)
create(259-265)create(371-374)create(405-421)
🪛 Ruff (0.14.14)
openwisp_controller/config/api/serializers.py
[warning] 393-393: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 394-394: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 401-401: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 402-402: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/tests/test_api.py
[error] 1370-1370: Possible hardcoded password assigned to: "shared_secret"
(S105)
[error] 1394-1394: Possible hardcoded password assigned to: "shared_secret"
(S105)
🔇 Additional comments (3)
openwisp_controller/config/api/serializers.py (1)
19-19: LGTM!The model loading for
OrganizationConfigSettingsfollows the existing pattern in this file using swapper'sload_model.openwisp_controller/config/api/views.py (1)
314-319: LGTM!The view function mappings correctly wire HTTP methods to viewset actions, consistent with the pattern used elsewhere in this file.
openwisp_controller/config/api/urls.py (1)
86-95: LGTM!The new URL patterns follow the established conventions in this file, using consistent naming (
organization_list,organization_detail) and path structure (controller/organization/).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| shared_secret = serializers.CharField( | ||
| required=False, | ||
| allow_blank=False, | ||
| min_length=8, | ||
| help_text='Used for automatic registration of devices' | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the model definition for AbstractOrganizationConfigSettings
rg -n 'shared_secret' --type py -B 2 -A 2
# Check for max_length constraints on shared_secret in model
rg -n 'shared_secret\s*=' --type py -B 1 -A 1 | head -50
# Find the config model file
fd -e py -path '*config*' | grep -i model | head -10Repository: openwisp/openwisp-controller
Length of output: 18330
🏁 Script executed:
#!/bin/bash
# Check KeyField definition
rg -n 'class KeyField' --type py -A 5
# Get full serializer context
sed -n '381,395p' openwisp_controller/config/api/serializers.pyRepository: openwisp/openwisp-controller
Length of output: 651
🏁 Script executed:
#!/bin/bash
# Check KeyField definition to understand the field type
rg -n 'class KeyField' --type py -A 10
# Also search for KeyField import
rg -n 'from.*KeyField|import.*KeyField' --type py | head -5
# Check multitenancy.py for full context
sed -n '20,35p' openwisp_controller/config/base/multitenancy.pyRepository: openwisp/openwisp-controller
Length of output: 838
Add max_length=32 to serializer field to match model constraint.
The shared_secret field in the model (KeyField) enforces max_length=32, but the serializer CharField only validates min_length=8. Add max_length=32 to the serializer field to ensure consistent validation:
shared_secret = serializers.CharField(
required=False,
allow_blank=False,
min_length=8,
max_length=32,
help_text='Used for automatic registration of devices'
)Regarding the write-only concern: the tests confirm that returning shared_secret in GET responses is intentional and aligns with the PR objectives, so write_only=True is not appropriate here.
🤖 Prompt for AI Agents
In `@openwisp_controller/config/api/serializers.py` around lines 383 - 388, The
serializer's shared_secret CharField (named shared_secret) lacks max_length
validation and should match the model's KeyField constraint; update the
shared_secret field definition to include max_length=32 so the serializer
enforces min_length=8 and max_length=32, keeping required=False and
allow_blank=False and leaving write_only as-is per tests.
| @transaction.atomic | ||
| def create(self, validated_data): | ||
| config_settings_data = validated_data.pop('config_settings', None) | ||
| organization = Organization.objects.create(**validated_data) | ||
|
|
||
| if config_settings_data: | ||
| try: | ||
| config_settings = organization.config_settings | ||
| for attr, value in config_settings_data.items(): | ||
| setattr(config_settings, attr, value) | ||
| config_settings.save() | ||
| except OrganizationConfigSettings.DoesNotExist: | ||
| OrganizationConfigSettings.objects.create( | ||
| organization=organization, | ||
| **config_settings_data | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for post_save signals related to Organization
rg -n 'post_save.*Organization|@receiver.*post_save' --type py | head -30Repository: openwisp/openwisp-controller
Length of output: 54
🏁 Script executed:
# Search for OrganizationConfigSettings creation patterns
rg -n 'OrganizationConfigSettings' --type py -B2 -A2 | head -60Repository: openwisp/openwisp-controller
Length of output: 4364
🏁 Script executed:
# Find the model definitions to understand the relationship
fd -e py | xargs grep -l 'class OrganizationConfigSettings\|class Organization' | grep modelsRepository: openwisp/openwisp-controller
Length of output: 187
🏁 Script executed:
# Read the Organization model definition and look for related fields
rg -n 'class Organization' openwisp_controller/config/models.py -A30 | head -50Repository: openwisp/openwisp-controller
Length of output: 754
🏁 Script executed:
# Read AbstractOrganizationConfigSettings to understand the relationship
rg -n 'class AbstractOrganizationConfigSettings' openwisp_controller/config/ -A30Repository: openwisp/openwisp-controller
Length of output: 2708
🏁 Script executed:
# Search for signals in the config module
fd -e py openwisp_controller/config | xargs rg -l 'post_save|receiver|signals' | head -10Repository: openwisp/openwisp-controller
Length of output: 1012
🏁 Script executed:
# Check if there's a signals.py file
find openwisp_controller/config -name 'signals.py' -o -name 'signal*'Repository: openwisp/openwisp-controller
Length of output: 109
🏁 Script executed:
# Read the signals.py file to check for Organization post_save handlers
cat openwisp_controller/config/signals.pyRepository: openwisp/openwisp-controller
Length of output: 2043
🏁 Script executed:
# Check the full OneToOneField definition to see auto_created setting
rg -n 'organization = models.OneToOneField' openwisp_controller/config/base/multitenancy.py -A10Repository: openwisp/openwisp-controller
Length of output: 939
🏁 Script executed:
# Check if there are any post_save signals for Organization in the config app
rg -n 'post_save.*Organization|@receiver' openwisp_controller/config -A5 --type pyRepository: openwisp/openwisp-controller
Length of output: 1004
🏁 Script executed:
# Check apps.py files for signal registration
rg -n 'ready\|post_save\|receiver' openwisp_controller/config/apps.py openwisp_controller/config/base/apps.py openwisp_users/apps.py 2>/dev/null | head -30Repository: openwisp/openwisp-controller
Length of output: 54
🏁 Script executed:
# Specifically check if the config app's apps.py sets up any signals
cat openwisp_controller/config/apps.pyRepository: openwisp/openwisp-controller
Length of output: 17474
🏁 Script executed:
# Check the OrganizationSerializer to understand the full context
rg -n 'class OrganizationSerializer' openwisp_controller/config/api/serializers.py -A40Repository: openwisp/openwisp-controller
Length of output: 1912
Simplify redundant try/except pattern in create method.
The try block will always raise OrganizationConfigSettings.DoesNotExist because Django OneToOneField does not auto-create related instances upon parent creation, and no signals auto-create OrganizationConfigSettings when an Organization is created. The except branch will always execute when config_settings_data is provided.
Simplify by removing the try/except and directly creating the instance:
if config_settings_data:
OrganizationConfigSettings.objects.create(
organization=organization,
**config_settings_data
)Or use update_or_create if you need to handle both new and existing instances.
🤖 Prompt for AI Agents
In `@openwisp_controller/config/api/serializers.py` around lines 404 - 420, The
create method's try/except around accessing organization.config_settings is
redundant because a newly created Organization won't have a related
OrganizationConfigSettings, so always taking the except branch; update the
create(self, validated_data) implementation to remove the try/except and
directly create (or update_or_create) the related OrganizationConfigSettings
when config_settings_data is present: replace the try/except that touches
organization.config_settings with a direct
OrganizationConfigSettings.objects.create(organization=organization,
**config_settings_data) or use
OrganizationConfigSettings.objects.update_or_create(organization=organization,
defaults=config_settings_data) if you need to handle existing instances.
| if response.status_code != 201: | ||
| print(f"Error response: {response.data}") | ||
| self.assertEqual(response.status_code, 201) |
There was a problem hiding this comment.
Remove debug print statements.
Debug print statements should be removed before merging. They add noise to test output and are not needed for CI/CD.
🧹 Proposed fix
response = self.client.post(path, data, content_type='application/json')
- if response.status_code != 201:
- print(f"Error response: {response.data}")
self.assertEqual(response.status_code, 201)📝 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.
| if response.status_code != 201: | |
| print(f"Error response: {response.data}") | |
| self.assertEqual(response.status_code, 201) | |
| self.assertEqual(response.status_code, 201) |
🤖 Prompt for AI Agents
In `@openwisp_controller/config/tests/test_api.py` around lines 1342 - 1344, A
debug print statement is present in the test where the response is checked (the
block using the variable response and the assertion
self.assertEqual(response.status_code, 201)); remove the conditional print line
that outputs response.data so the test does not produce noisy output in CI and
keep only the assertion (ensure the if-check and print are deleted, leaving the
assertion on response.status_code intact).
| if response.status_code != 200: | ||
| print(f"Error response: {response.data}") | ||
| self.assertEqual(response.status_code, 200) |
There was a problem hiding this comment.
Remove debug print statement.
Same as above - debug print should be removed.
🧹 Proposed fix
response = self.client.patch(path, data, content_type='application/json')
- if response.status_code != 200:
- print(f"Error response: {response.data}")
self.assertEqual(response.status_code, 200)📝 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.
| if response.status_code != 200: | |
| print(f"Error response: {response.data}") | |
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual(response.status_code, 200) |
🤖 Prompt for AI Agents
In `@openwisp_controller/config/tests/test_api.py` around lines 1408 - 1410,
Remove the temporary debug print inside the test: delete the conditional branch
that prints response.data (the "if response.status_code != 200: print(f\"Error
response: {response.data}\")" lines) and leave the assertion
self.assertEqual(response.status_code, 200) intact so the test fails without
printing debug output; locate this in the API test around the
response/status_code check in test_api.py.
2 participants
Checklist
Reference to Existing Issue
Closes #700
Description of Changes
I added REST API endpoints to manage OrganizationConfigSettings including shared_secret, registration_enabled, and context fields via the /api/v1/controller/organization/ endpoint. This resolves the issue where organization configuration management settings were not accessible through any API endpoint.
Changes:
Screenshot
Please include any relevant screenshots.