[fix] Disallow changing configuration backend from UI #789#1293
[fix] Disallow changing configuration backend from UI #789#1293hypnoastic wants to merge 1 commit intoopenwisp:masterfrom
Conversation
|
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:
📝 WalkthroughWalkthroughMakes the configuration "backend" field readonly on admin change forms except for recover views. Adds server-side detection ( Sequence Diagram(s)sequenceDiagram
participant Browser as Admin UI (browser + JS)
participant DjangoAdmin as Django Admin View
participant ConfigForm as ConfigForm (server)
participant Views as get_relevant_templates
participant DB as Database
Browser->>DjangoAdmin: GET change-form for object
DjangoAdmin->>ConfigForm: instantiate (__init__) / request check
ConfigForm-->>DjangoAdmin: mark backend field disabled if not recover view
DjangoAdmin-->>Browser: Render form (backend input or readonly label)
Browser->>Browser: JS reads backend from input or readonly label
Browser->>Views: GET relevant templates (org, backend or config_id)
Views->>DB: if backend missing and config_id present -> fetch Config.backend
DB-->>Views: return backend (or none)
Views-->>Browser: Return filtered templates
Browser->>DjangoAdmin: POST change-form attempting backend change
DjangoAdmin->>ConfigForm: validate/save (backend ignored when readonly)
ConfigForm->>DB: Save object (backend preserved)
DB-->>DjangoAdmin: Saved object
DjangoAdmin-->>Browser: Redirect / success (backend unchanged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 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_controller/config/tests/test_admin.py`:
- Around line 1545-1568: In
test_template_backend_cannot_be_changed_from_change_form replace the hardcoded
backend assertion with a comparison to the template's original backend value:
capture the original value (e.g., original_backend = template.backend) before
posting, then after response and refresh_from_db assert template.backend ==
original_backend instead of "netjsonconfig.OpenWrt"; update the similar
assertions in the other test referenced (lines 1688-1712) to use the template
variable's original backend value as well to avoid fixture-specific constants.
🪄 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: eaec9162-5a0d-49a9-bf38-d7e62a735b8d
📒 Files selected for processing (2)
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.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.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
🧠 Learnings (3)
📚 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/admin.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.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/admin.pyopenwisp_controller/config/tests/test_admin.py
🔇 Additional comments (7)
openwisp_controller/config/admin.py (3)
470-474: Good conditional lock for device config backend on change.This correctly keeps backend immutable only when an existing config is present, while not affecting add-time behavior.
1087-1092: Template backend readonly behavior is implemented cleanly.The guard on
objpreserves backend selection on add and enforces immutability on change.
1272-1277: VPN backend readonly behavior is consistent with TemplateAdmin.The implementation is minimal and correctly scoped to change forms.
openwisp_controller/config/tests/test_admin.py (4)
1438-1445: Good assertion strategy for readonly rendering on device change form.Checking presence of the field wrapper and absence of the editable input captures the intended UI lock.
1446-1470: Great regression test for backend tampering via POST on device change.This correctly verifies that submitted backend edits are ignored on change forms.
1531-1544: Template readonly UI coverage looks good.The test cleanly validates non-editability in the admin change form.
1681-1687: VPN readonly rendering test is correct and focused.This verifies backend is displayed but not editable in change context.
Commit Message and Test FailuresHello @hypnoastic,
For example:
|
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
6298af4 to
242c45b
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_controller/config/tests/test_admin.py`:
- Around line 1466-1469: The test posts the change form but never asserts the
submission succeeded, so it can miss validation failures; after calling
self.client.post(path, params) assert the response indicates success (e.g., a
302 redirect or expected status_code) before calling config.refresh_from_db()
and asserting immutability of config.backend against template.backend; update
the block around self.client.post(path, params), response,
config.refresh_from_db(), and self.assertEqual(config.backend, template.backend)
to first assert the redirect/status and then reload and compare backend to
ensure the form was accepted and the backend remained unchanged.
🪄 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: a162074c-a008-49d3-bf31-2dc9b194eaea
📒 Files selected for processing (2)
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.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~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | 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.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
🧠 Learnings (5)
📚 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/admin.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.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/admin.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_admin.py
🔇 Additional comments (8)
openwisp_controller/config/admin.py (3)
470-474: Good conditional readonly enforcement for device config backend.This correctly keeps backend editable when creating config and locks it once config already exists on change forms.
1087-1092: Template backend immutability is implemented correctly.The
objguard ensures backend remains selectable on add and readonly on change.
1272-1277: VPN backend readonly behavior aligns with the intended UX.This change is consistent with the Template and Device config behavior.
openwisp_controller/config/tests/test_admin.py (5)
1438-1445: Strong readonly rendering assertion for device backend.This test correctly verifies that change forms display backend as readonly instead of editable input.
1531-1544: Template readonly UI test is well-scoped and clear.It validates the intended admin rendering contract for backend immutability.
1545-1569: Great immutability test pattern for template backend.Checking both unchanged backend and updated name after a 302 makes this test resilient and behavior-accurate.
1682-1688: VPN change-form readonly assertion looks correct.This properly confirms backend is displayed but not editable in the change UI.
1689-1714: VPN backend immutability test is solid.This verifies successful form processing while ensuring backend cannot be altered via crafted POST data.
Commit Message and Test FailuresHello @hypnoastic,
Fix: Please reformat your commit message. A correct format looks like this: For example:
Fix: Please investigate the test |
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
242c45b to
d723890
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_controller/config/tests/test_admin.py`:
- Around line 1438-1471: Add a new test that mirrors the existing change-form
tests but targets the admin "recover" view to lock the branch where readonly
enforcement is skipped (i.e., where is_recover_view(request) is True). Use the
same helpers (_create_device, _create_config, Template.objects.first,
_get_device_params) and build a path with
reverse("admin:{app_label}_device_recover", args=[device.pk]) or the actual
recover-view name used in the app; POST params should attempt to change
config-0-backend to an invalid value and assert the outcome differs from the
normal change-view (e.g., readonly enforcement is skipped or validation behaves
as expected), refresh config with config.refresh_from_db() and assert
config.backend remained or changed per the intended recover-view behavior; add
at least one test function named test_device_backend_recover_view_behavior to
cover this branch and replicate the assertions pattern used in
test_device_backend_cannot_be_changed_from_change_form.
🪄 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: 8bf82ffa-6360-4286-b5d9-81ef8970ef16
📒 Files selected for processing (2)
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.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.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.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~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.py
🧠 Learnings (5)
📚 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/admin.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.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/admin.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_admin.py
🔇 Additional comments (10)
openwisp_controller/config/admin.py (4)
69-75:is_recover_viewhelper is clean and safe.The fallback on
request._recover_viewplus URL-name suffix check is a solid way to keep recover flows exempt from readonly enforcement.
480-487: Config inline backend immutability logic looks correct.The condition correctly targets existing device configs only and preserves recover behavior.
1101-1105: Template admin readonly handling is correctly scoped to change forms.
objgating keeps backend selectable on add forms while blocking edits on change forms.
1286-1290: VPN admin readonly handling matches intended behavior.This is consistent with template/device behavior and keeps recover view exceptions intact.
openwisp_controller/config/tests/test_admin.py (6)
1438-1445: Device change-form readonly assertion is solid.The test verifies backend is displayed but not editable in inline form markup.
1446-1471: Device backend immutability POST test is well hardened.Good use of successful submission assertion (
302) plus persisted backend and mutable-field change checks.
1533-1546: Template readonly rendering test is correct.This correctly checks that backend appears on change form without an editable input.
1547-1571: Template immutability test implementation is robust.Using
original_backendavoids fixture-specific constants and improves test stability.
1684-1690: VPN readonly rendering test is correct.The assertion pattern is consistent with template/device and validates expected UI behavior.
1691-1715: VPN backend immutability POST test is correctly structured.It validates successful save and confirms backend remains unchanged while other fields update.
Test Failures in CI RunHello @hypnoastic,
Fix: Review the test case and the corresponding admin code to ensure the expected redirection (status code 302) occurs after attempting to change the device backend. This might involve checking form processing logic and redirect URLs. |
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
d723890 to
991e13f
Compare
hypnoastic
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_controller/config/static/config/js/widget.js`:
- Around line 416-440: Add a short explanatory comment above the
getReadonlySchemaKey function describing the fuzzy matching strategy: explain
that backendLabel is normalized via the inner normalize function (to lowercase
and stripping non-alphanumerics) and that keys from schemas are compared using
the same normalization on the last dot-segment (normalizedBackendKey), then
matched via exact equality or substring containment both ways to tolerate
variations like "OpenWRT" vs "netjsonconfig.OpenWrt"; reference the normalize
helper, normalizedBackendLabel and normalizedBackendKey in the comment so future
maintainers understand why the flexible matching is required.
In `@openwisp_controller/config/tests/test_admin.py`:
- Around line 1492-1494: The test currently grabs the inline via
django_admin.site._registry[Device].inlines[0], which is brittle; instead
iterate the inlines on django_admin.site._registry[Device] and pick the inline
class whose model is Config (or whose class name is ConfigInline) and
instantiate that for config_inline; update the test to locate the inline by
checking inline.model is Config (referencing Device, Config, and
django_admin.site._registry) so it no longer depends on index position.
🪄 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: 0e29369d-6d2a-43b3-b639-1b8ee6ce5f43
📒 Files selected for processing (8)
openwisp_controller/config/admin.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/views.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.1.0
- 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.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.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
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.py
🧠 Learnings (5)
📚 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/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.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/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🔇 Additional comments (23)
openwisp_controller/config/static/config/js/vpn.js (2)
36-43: LGTM!The
getBackendValuehelper correctly handles both scenarios: extracting the value from the editable#id_backendinput when present, and falling back to the readonly field text when the backend field is rendered as readonly. The.field-backend .readonlyselector matches the Django admin pattern used inVpnAdmin.get_readonly_fields.
71-76: LGTM!The guard check ensures the change handler is only attached when the
#id_backendelement exists, preventing JavaScript errors on change forms where the backend field is readonly.openwisp_controller/config/static/config/js/widget.js (1)
468-473: LGTM!The fallback to
getReadonlySchemaKeywhen no selector element exists ensures the JSON schema editor loads correctly on change forms where the backend field is readonly.openwisp_controller/config/admin.py (6)
69-76: LGTM!The
is_recover_viewfunction properly detects recover views via both the_recover_viewattribute (set inDeviceAdmin.recover_view) and the URL name pattern. The fallback to checkingresolver_match.url_namehandles edge cases where the attribute might not be set.
359-366: LGTM!The
readonly_backendclass attribute with__init__logic to disable the backend field provides a clean way to make the field read-only at the form level, complementing theget_readonly_fieldsapproach in the admin.
475-482: Dynamic form class creation is correct but worth noting.Creating a dynamic form class with
type()is a valid pattern for injecting thereadonly_backendflag per-request. This ensures each formset instance gets the appropriate readonly state based on whether the device has an existing config.
494-503: LGTM!The
get_readonly_fieldsoverride correctly appends "backend" when:
- An object exists (
obj)- The object has a config (
obj._has_config())- It's not a recover view
- "backend" isn't already in readonly fields
This ensures backend is editable on add forms and recover views, but readonly on change forms.
1116-1120: LGTM!The
TemplateAdmin.get_readonly_fieldsfollows the same pattern as the other admin classes, making backend readonly on change forms while allowing edits on add forms and recover views.
1301-1305: LGTM!The
VpnAdmin.get_readonly_fieldsis consistent with the pattern used inTemplateAdminandConfigInline.openwisp_controller/config/views.py (1)
72-78: LGTM!The fallback logic correctly derives the backend from the Config when the backend parameter is missing but
config_idis present. This supports the readonly backend scenario where the frontend can't send the backend value. Using.only("backend")minimizes the query overhead.openwisp_controller/config/tests/test_views.py (1)
174-198: LGTM!The test properly validates the backend inference fallback by:
- Creating a config with a specific backend (OpenWrt)
- Creating templates with different backends
- Requesting templates with only
config_id(nobackendparam)- Asserting that only matching-backend templates are returned
This provides good coverage for the new logic in
get_relevant_templates.openwisp_controller/config/tests/test_selenium.py (3)
273-279: LGTM!The updated test properly validates that:
- The backend field is not editable on the device change form (no
config-0-backendinput)- Templates still load correctly using the stored backend value
The use of
subTestblocks improves test organization and failure diagnosis.
668-674: LGTM!These subTests effectively verify that:
- The backend field renders as readonly (
.field-backend .readonlyvisible,#id_backendabsent)- WireGuard-related fields remain visible despite the readonly backend
This ensures the VPN change form displays correctly.
676-681: LGTM!The new test validates that the backend field remains editable on the VPN add form and that selecting OpenVPN correctly hides the WireGuard-specific fields (webhook_endpoint, auth_token). This confirms the add form behavior is preserved.
openwisp_controller/config/static/config/js/relevant_templates.js (3)
13-17: LGTM!The updated
isAddingNewObjectfunction correctly handles both contexts:
- Device groups: checks for
.add-formclass- Device configs: checks if
config-0-idfield exists and has a valueThis distinguishes between add and change views properly.
128-133: LGTM!The logic correctly handles the readonly backend scenario:
- For device groups, backend is empty string (no backend filtering)
- For device configs, uses the backend value if available, otherwise empty string
- The early return now includes
configIDcheck, allowing template loading when backend is readonly but config exists
199-215: LGTM!The
bindDefaultTemplateLoadinglogic correctly handles all scenarios:
- Editable backend field present → attach change handler
- Non-device-group with existing config (readonly backend) → call
showRelevantTemplatesdirectly, server infers backend fromconfig_id- Device group → initialize and show templates
- Add view → wait for config section to be added
This ensures templates load correctly regardless of backend editability.
openwisp_controller/config/tests/test_admin.py (6)
4-4: Good test-only imports for recover-view coverage.These imports are focused and used by the new admin behavior tests.
Also applies to: 9-9, 17-17
1440-1479: Device backend readonly + immutability tests are solid.Nice combination of UI-readonly assertion and POST immutability check with successful submission validation.
1560-1599: Template backend readonly and immutable-on-change tests look good.Coverage clearly verifies both rendered readonly behavior and persistence safety on change POST.
1600-1614: Template recover-view branch coverage is well targeted.This test locks the change-vs-recover readonly branching in admin logic.
1727-1759: VPN backend readonly + immutability checks are comprehensive.Good parity with device/template tests and successful form submission assertion.
1760-1773: VPN recover-view behavior test is clear and valuable.This prevents regressions in the recover exception path for backend editability.
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
991e13f to
e802b19
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 `@openwisp_controller/config/static/config/js/relevant_templates.js`:
- Around line 13-17: The isAddingNewObject() logic is inverted for device
groups: currently it returns true when $(".add-form") is absent; update the
branch for isDeviceGroup() so it returns true when an add form is present (e.g.,
$(".add-form").length > 0) so add forms are detected as adding and change forms
as not adding; leave the fallback that checks $('input[name="config-0-id"]')
(config-0-id) unchanged.
In `@openwisp_controller/config/tests/test_admin.py`:
- Around line 1486-1493: Extract the repeated RequestFactory setup into a helper
(e.g., a test utility function like build_request or make_recover_request) that
accepts parameters for user (use self._get_admin()), path/method (default "/"),
and resolver_match url_name (compose using self.app_label + "_device_recover" or
other recover names); replace the inline RequestFactory + resolver_match
assignments in the recover-view tests for device/template/vpn with calls to this
helper to eliminate duplication and avoid unused/redundant code.
In `@openwisp_controller/config/tests/test_views.py`:
- Around line 192-197: The test calls response.json() without asserting the HTTP
status first, which can obscure failures; update the test in
openwisp_controller/config/tests/test_views.py (the test that does
reverse("admin:get_relevant_templates", args=[org1.pk]) and assigns response) to
assert the expected status (e.g., self.assertEqual(response.status_code, 200))
immediately after obtaining response and before calling response.json(), then
proceed to parse JSON and assert that str(t1.pk) is in templates.
In `@openwisp_controller/config/views.py`:
- Around line 74-77: The lookup that sets backend from
Config.objects.only("backend").get(pk=config_id) is not tenant-scoped; change
the query to include the same tenant/organization filter used elsewhere in this
view (e.g. add the organization/tenant field or use the view's request-scoped
queryset such as self.request.organization or request.user.organization or an
existing Config queryset helper) so the lookup becomes
Config.objects.only("backend").get(pk=config_id, <tenant_field>=current_tenant)
(or use the view's get_queryset()/for_tenant helper) and keep the existing
exception handling for Config.DoesNotExist/ValueError to avoid leaking other
tenants' backends.
🪄 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: 1fd05826-da54-4c24-990d-209c1a7b64a5
📒 Files selected for processing (8)
openwisp_controller/config/admin.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/views.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.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.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
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.py
🧠 Learnings (5)
📚 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/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.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/views.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_admin.py
🔇 Additional comments (8)
openwisp_controller/config/static/config/js/widget.js (1)
416-443: Readonly schema fallback is well-handled.This correctly covers the no-selector path and keeps schema loading functional on readonly backend change forms.
Also applies to: 471-476
openwisp_controller/config/static/config/js/vpn.js (1)
36-43: Backend fallback and guarded binding are solid.Good hardening for readonly backend pages without regressing editable add-form behavior.
Also applies to: 71-76
openwisp_controller/config/tests/test_selenium.py (1)
273-280: Coverage updates align well with readonly-backend behavior.The new assertions validate both non-editability on change forms and backend-driven field toggling on add forms.
Also applies to: 668-682
openwisp_controller/config/admin.py (1)
69-75: Readonly backend enforcement is correctly implemented across admin surfaces.This cleanly preserves add/recover behavior while preventing backend edits on standard change forms.
Also applies to: 361-367, 475-505, 1118-1122, 1303-1307
openwisp_controller/config/tests/test_admin.py (4)
4-17: Import additions are clean and purposeful.The new imports are all exercised by the added admin readonly/recover tests; no dead import introduced.
1440-1506: Device backend test coverage is strong.This block correctly validates readonly UI state, POST immutability, and recover-view behavior split for admin logic.
1568-1624: Template backend tests are well-targeted.Good validation of readonly behavior on change forms and backend immutability while preserving recover-view editability.
1737-1783: VPN backend protection tests look complete.These assertions correctly verify change-form immutability and preserve the recover-view exception path.
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
e802b19 to
076d8cd
Compare
CI Failure: Test Failures and WHOIS Lookup IssuesHello @hypnoastic, There are multiple failures in the CI run:
Please address these issues to resolve the CI failures. |
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
076d8cd to
6bebb4e
Compare
|
Left a comment, please take a look. Other than that address the comments left by kilo bot and code rabbit and the PR should be good for the most part |
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
156fd4f to
6fece20
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Hi, I addressed the bot comments and all review conversations are now resolved, but I still can’t see the manual review comment you mentioned on my side. GitHub is only showing resolved conversations for me. Could you please mention it again here or point me to the file/line? Also, the PR is still showing a stale changes requested status even though all visible comments have been addressed. |
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 `@openwisp_controller/config/static/config/js/vpn.js`:
- Around line 36-43: getBackendValue currently falls back to the visible label
text (".field-backend .readonly") which is not a stable identifier for
toggleRelatedFields; change the readonly branch to read a stable backend
identifier from a data attribute on the readonly element (e.g. use
$(".field-backend .readonly").first().data("backend") or a similar data-*
attribute) and normalize that value (toLocaleLowerCase) so toggleRelatedFields
receives actual backend keys like "wireguard", "openvpn", "vxlan" instead of the
display label; ensure the code still checks backendInput.val() first and only
uses the data attribute when backendInput is absent.
In `@openwisp_controller/config/static/config/js/widget.js`:
- Around line 416-443: The readonly schema inference in getReadonlySchemaKey
relies on fuzzy-matching the human label from .field-backend .readonly which
fails for custom labels; change the DOM to expose the actual backend value
(e.g., add a data-backend or value attribute to the .readonly element where
BACKENDS/VPN_BACKENDS are rendered) and update getReadonlySchemaKey to read that
attribute (use jQuery .data('backend') or .attr('data-backend')) and directly
return schemas[backendValue] (or the matching key) instead of normalizing
labels; keep the existing normalize/fuzzy logic only as a fallback when the
data-backend attribute is absent and apply the same change to the other
identical block referenced around the second occurrence (the similar
readonly-to-schema lookup later in the file).
In `@openwisp_controller/config/tests/test_selenium.py`:
- Around line 676-682: The test test_vpn_add_backend_switches_related_fields is
selecting "OpenVPN" when the form already defaults to OpenVPN, so it doesn't
exercise the change handler; update the test to first select a different backend
option (e.g., "WireGuard" or any option not equal to "OpenVPN") using the Select
on id_backend and wait for the related fields (field-webhook_endpoint,
field-auth_token) to become visible, then select "OpenVPN" again and assert
those fields become invisible using the existing wait_for_invisibility calls;
locate and modify the test_vpn_add_backend_switches_related_fields function to
implement the two-step switch (other backend -> OpenVPN) and the corresponding
visibility checks.
In `@openwisp_controller/config/views.py`:
- Around line 74-82: The exception handlers in
openwisp_controller/config/views.py currently catch only Config.DoesNotExist and
ValueError when reading Config.objects.get(pk=config_id); add
django.core.exceptions.ValidationError to the imports and include
ValidationError in both except tuples (the one handling the get at the block
around backend = config_queryset.get(pk=config_id).backend and the similar
handler later at the other get) so malformed UUIDs are caught and handled
gracefully.
🪄 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: bc31b709-b4a5-4c66-9d58-954e2f884d86
📒 Files selected for processing (8)
openwisp_controller/config/admin.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/views.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~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | 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.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/static/config/js/vpn.jsopenwisp_controller/config/static/config/js/widget.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.py
🧠 Learnings (5)
📚 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/views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.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/views.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_views.pyopenwisp_controller/config/admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_admin.py
🔇 Additional comments (10)
openwisp_controller/config/static/config/js/vpn.js (1)
71-76: Nice guard for readonly change forms.Skipping the change binding when
#id_backendis absent avoids pointless selector work and JS edge cases on the new readonly pages.openwisp_controller/config/tests/test_views.py (1)
174-261: Good coverage for config-backed template inference.These cases pin down same-org, foreign-org, and org-change behavior around the
config_idfallback, which should make future refactors ofget_relevant_templates()much safer.openwisp_controller/config/tests/test_admin.py (2)
1440-1478: Strong change-form immutability coverage.The GET + POST checks validate both halves of the contract: the backend input disappears on change forms, and backend mutations are ignored while ordinary fields still save.
Also applies to: 1571-1767
1480-1509: Recover-view branch is now well pinned down.The helper-based assertions make the intended exception explicit for device, template, and vpn admins, which is exactly the kind of branch that tends to regress later.
Also applies to: 1611-1623, 1769-1777
openwisp_controller/config/tests/test_selenium.py (1)
273-279: Readonly Selenium checks look good.Checking both the missing backend input and the still-visible backend-dependent UI gives good end-to-end coverage for the new change-form behavior.
Also applies to: 668-674
openwisp_controller/config/static/config/js/relevant_templates.js (1)
13-17: The readonly-backend fallback here looks solid.Using
config-0-idto distinguish add vs change and letting change pages fetch templates without an editable backend keeps the sortedm2m flow aligned with the new admin UI.Also applies to: 128-133, 199-215
openwisp_controller/config/admin.py (4)
69-75: Recover-view detection is nicely centralized.This keeps the readonly exception aligned for both real admin requests and the
resolver_match.url_name-based test setup.
361-366: Good defense-in-depth on the form layer.Disabling
backendhere complements the admin readonly rendering and keeps POSTed values from changing the field if this form is reused outside the standard inline path.
475-484: The inline formset gate matches the add-vs-change requirement.Using
obj._has_config()here keeps backend editable when a device is getting its first config, while making existing configs immutable outside recover flows.
497-504: Readonly gating is consistent across Device, Template, and VPN admins.The
objcheck preserves editable add forms, and the shared recover exception avoids blocking legitimate rollback/recovery flows.Also applies to: 1118-1122, 1303-1307
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
2 times, most recently
from
374058e to
d868824
Compare
Make the backend read-only on Django admin change forms for device configuration, template, and VPN server, while keeping it selectable on create forms. Also add recover-view regression coverage and ensure device inline backend changes are ignored on change-form submissions. Fixes openwisp#789
hypnoastic
force-pushed
the
issues/789-disallow-changing-configuration-backend-ui
branch
from
94578f9 to
4651ba3
Compare
|
also lets avoid force pushing, unless the maintainers ask to do so 👍 |
|
Thanks, I will keep that in mind |
3 participants
What does this PR do?
This PR makes backend read-only on Django admin change forms for device configuration, template, and VPN server, while keeping
it selectable on create forms.
Why the scope is bigger than the issue title
The admin change made backend non-editable, which removed selectors used by existing JS and Selenium flows on change pages. The extra updates are regression/compatibility fixes to keep current admin behavior working with the new readonly contract, not additional feature work.
What changed
Files updated (and why)
openwisp_controller/config/admin.pycore readonly-on-change behavior.openwisp_controller/config/tests/test_admin.pycreate/change immutability + recover-view coverage.openwisp_controller/config/static/config/js/relevant_templates.jstemplate loading on readonly change forms.openwisp_controller/config/views.pybackend inference fallback for readonly flows with tenant safety.openwisp_controller/config/tests/test_views.pycoverage for fallback and cross-org safety.openwisp_controller/config/static/config/js/vpn.jsVPN related-field behavior with readonly backend.openwisp_controller/config/static/config/js/widget.jsschema selection fallback when backend is readonly.openwisp_controller/config/tests/test_selenium.pyaligned Selenium expectations with readonly-on-change behavior.UI Behaviour
Manually verified in admin UI for create vs edit flows for VPN, devices, and templates.
VPN
Devices
Templates
Self-review checklist
Closes #789