Fix #848: Improve JSON rendering for read-only admin users#1327
Fix #848: Improve JSON rendering for read-only admin users#1327vivinakashrangarajan wants to merge 4 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds a ReadonlyPrettyJsonMixin to render JSON/dict fields as formatted, read-only HTML blocks in the Django admin for users with view-only permissions and integrates it into multiple admin classes (config and connection). CSS styling for Sequence Diagram(s)(Skipped — conditions for generating a sequence diagram are not met) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi maintainers, This PR addresses issue #848 by improving JSON rendering for users with view-only permissions. The solution ensures:
I’ve also cleaned the PR by removing unnecessary files and updating .gitignore. Looking forward to your feedback. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 76-77: Remove the duplicate gitignore entries by deleting the
redundant "__pycache__/" and "*.pyc" lines; keep the original "__pycache__/"
rule already declared earlier and the broader "*.py[cod]" rule (which covers
"*.pyc") so only one canonical rule remains for Python cache artifacts.
In `@myproject/myproject/settings.py`:
- Around line 22-23: Replace the hardcoded SECRET_KEY in settings.py with a
value loaded from the environment: read os.environ (e.g., os.environ.get or
os.environ[]) for a SECRET_KEY and assign it to the SECRET_KEY symbol; provide a
safe fallback only for local/dev (or explicitly raise an exception when
SECRET_KEY is missing in production by checking DEBUG or a custom flag) and
ensure you import os at the top of the file and remove the hardcoded
'django-insecure-...' literal.
In `@openwisp_controller/config/static/config/css/admin.css`:
- Around line 34-47: The .readonly-json CSS rule uses deprecated word-wrap and
unnecessarily quotes the single-word font family "Monaco"; update the rule by
replacing word-wrap with overflow-wrap and remove the quotes around Monaco in
the font-family list (target the .readonly-json selector to find the block) so
stylelint passes.
In `@patch_config_tests.py`:
- Around line 2-62: The TestReadonlyJsonIssues class in patch_config_tests.py is
a duplicate of the existing TestReadonlyJsonIssues in config/tests/test_admin.py
and also contains missing imports and a filename that prevents pytest discovery;
remove this patch artifact (patch_config_tests.py) or consolidate its assertions
into the canonical TestReadonlyJsonIssues in TestAdminMixin/TestCase-based test
file (config/tests/test_admin.py), adding any missing imports (TestAdminMixin,
TestCase, reverse, OrganizationConfigSettings) only in the canonical file and
deleting the redundant class and file so tests are not duplicated and are
discoverable by pytest.
In `@patch_conn_tests.py`:
- Around line 7-47: Delete the redundant test module patch_conn_tests.py (which
contains the TestConnectionReadonlyJsonIssues class) because it duplicates the
tests already defined in the project's test_admin.py and won't be discovered by
pytest; also check for and delete patch_config_tests.py if it has the same
duplicate pattern; ensure no other imports/reference rely on these patch_*.py
files before removing them.
In `@patch_inline.py`:
- Around line 1-40: This patch script (patch_inline.py) that searches for and
replaces the ConfigSettingsInline class is a development-only utility and should
not be committed; inspect openwisp_controller/config/admin.py for the updated
ConfigSettingsInline (look for symbols: class ConfigSettingsInline,
readonly_json_fields, pretty_context, _format_json_field) and if the file
already contains those changes delete this script; if you still need automation,
move this script to a dev/tools directory, add documentation and tests, and
replace the brittle exact-string replacement with a robust approach (e.g., edit
via AST or a small migration/management command) so it doesn’t rely on exact
whitespace/formatting.
In `@test_readonly.py`:
- Line 12: Remove the unused import of User from django.contrib.auth.models in
this test_readonly.py file; locate the line that imports "User" and delete that
import (or consolidate imports) so the module no longer imports an unused symbol
"User".
- Around line 1-34: This file is a standalone debug script and should be removed
or converted to a proper automated test: either delete test_readonly.py if
equivalent coverage exists in openwisp_controller/config/tests/test_admin.py, or
convert it into a Django TestCase by replacing the top-level script with a
class-based test (subclass django.test.TestCase), move RequestFactory usage into
setUp, instantiate MockAdmin(Template, site) and Template() inside tests,
replace print() calls with assertions (e.g., assertEqual/assertTrue/assertFalse)
against admin.fields, admin.get_fields(request, obj) and
admin.get_readonly_fields(request, obj), and ensure request.user attributes
(is_superuser, has_perm) are set via a proper user or Mock within the test
methods.
🪄 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: a6be5642-f77b-450b-ae41-75724122e9d6
📒 Files selected for processing (19)
.gitignoremyproject/db.sqlite3myproject/manage.pymyproject/myproject/__init__.pymyproject/myproject/asgi.pymyproject/myproject/settings.pymyproject/myproject/urls.pymyproject/myproject/wsgi.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/static/config/css/admin.cssopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/connection/admin.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/connection/tests/test_api.pypatch_config_tests.pypatch_conn_tests.pypatch_inline.pytest_readonly.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). (8)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.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:
myproject/myproject/urls.pymyproject/myproject/asgi.pymyproject/myproject/wsgi.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pymyproject/manage.pyopenwisp_controller/connection/tests/test_admin.pymyproject/myproject/settings.pypatch_conn_tests.pypatch_config_tests.pyopenwisp_controller/config/tests/test_admin.pytest_readonly.pyopenwisp_controller/connection/admin.pypatch_inline.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:
myproject/myproject/urls.pymyproject/myproject/asgi.pymyproject/myproject/wsgi.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pymyproject/manage.pyopenwisp_controller/connection/tests/test_admin.pymyproject/myproject/settings.pypatch_conn_tests.pypatch_config_tests.pyopenwisp_controller/config/tests/test_admin.pytest_readonly.pyopenwisp_controller/connection/admin.pypatch_inline.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:
myproject/myproject/urls.pymyproject/myproject/asgi.pymyproject/myproject/wsgi.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pymyproject/manage.pyopenwisp_controller/connection/tests/test_admin.pymyproject/myproject/settings.pypatch_conn_tests.pypatch_config_tests.pyopenwisp_controller/config/tests/test_admin.pytest_readonly.pyopenwisp_controller/connection/admin.pypatch_inline.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:
myproject/myproject/urls.pymyproject/myproject/asgi.pymyproject/myproject/wsgi.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pymyproject/manage.pyopenwisp_controller/connection/tests/test_admin.pymyproject/myproject/settings.pypatch_conn_tests.pypatch_config_tests.pyopenwisp_controller/config/tests/test_admin.pytest_readonly.pyopenwisp_controller/connection/admin.pypatch_inline.pyopenwisp_controller/config/admin.py
🧠 Learnings (5)
📚 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:
myproject/myproject/urls.pymyproject/myproject/asgi.pymyproject/myproject/wsgi.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pymyproject/manage.pyopenwisp_controller/connection/tests/test_admin.pymyproject/myproject/settings.pypatch_conn_tests.pypatch_config_tests.pyopenwisp_controller/config/tests/test_admin.pytest_readonly.pyopenwisp_controller/connection/admin.pypatch_inline.pyopenwisp_controller/config/admin.py
📚 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/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/connection/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/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/connection/admin.pyopenwisp_controller/config/admin.py
📚 Learning: 2026-01-16T10:20:30.847Z
Learnt from: atif09
Repo: openwisp/openwisp-controller PR: 1164
File: openwisp_controller/connection/static/connection/css/command-inline.css:121-134
Timestamp: 2026-01-16T10:20:30.847Z
Learning: In openwisp-controller, when replacing hardcoded colors with CSS variables, it's acceptable to use the same variable for both default and hover/focus states even if the original code had different colors for these states. UX improvements like restoring hover state visual feedback are considered out of scope for color replacement PRs and can be addressed separately.
Applied to files:
openwisp_controller/config/static/config/css/admin.css
📚 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
🪛 Stylelint (17.5.0)
openwisp_controller/config/static/config/css/admin.css
[error] 46-46: Unexpected quotes around "Monaco" (font-family-name-quotes)
(font-family-name-quotes)
[error] 38-38: Expected "word-wrap" to be "overflow-wrap" (property-no-deprecated)
(property-no-deprecated)
🔇 Additional comments (18)
.gitignore (1)
75-78: Good addition for local environment and secrets artifacts.Ignoring
venv/and.envis a solid safety improvement for local/dev workflows.myproject/myproject/urls.py (1)
1-22: LGTM!Standard Django URL configuration. The admin route is properly configured.
myproject/myproject/asgi.py (1)
1-16: LGTM!Standard Django ASGI configuration.
myproject/myproject/wsgi.py (1)
1-16: LGTM!Standard Django WSGI configuration.
myproject/manage.py (1)
1-22: LGTM!Standard Django management script with proper error handling for missing Django installation.
myproject/myproject/settings.py (1)
33-40: This review comment is based on an incorrect assumption about myproject's purpose.myproject is not used for testing. The actual test suite uses
openwisp2.settings(configured in pytest.ini withDJANGO_SETTINGS_MODULE = openwisp2.settings), which already includes all necessary openwisp_controller apps (openwisp_controller.config,openwisp_controller.connection,openwisp_controller.pki,openwisp_controller.geo,openwisp_controller.subnet_division). These apps register admin classes usingReadonlyPrettyJsonMixin, so they are already being tested through the openwisp2 test project.myproject appears to be a minimal Django starter project unrelated to testing the mixin.
> Likely an incorrect or invalid review comment.openwisp_controller/connection/api/serializers.py (1)
81-86: Security gate is nicely scoped.Redacting
paramsonly at representation time keeps writes untouched while preventing shared credential secrets from leaking to non-superusers.openwisp_controller/connection/tests/test_api.py (1)
536-543: Good serializer-level regression test.This exercises the exact redaction branch in
CredentialSerializer.to_representation()without needing the full view stack.openwisp_controller/config/tests/test_admin.py (1)
2785-2830: Coverage hits the main view-only admin surfaces.These assertions validate device, template, VPN, and device-group pages against the new
<pre class="readonly-json">rendering instead of only unit-testing the mixin in isolation.Also applies to: 2911-2932
openwisp_controller/connection/tests/test_admin.py (1)
135-153: Nice admin regression coverage for credential params.The added assertions pin both sides of the change: pretty JSON for view-only admins and hidden params for shared credentials.
Also applies to: 350-359
openwisp_controller/connection/admin.py (1)
42-69: Nice separation between readonly rendering and hidden-field policy.Mapping
paramsthroughreadonly_json_fieldspreserves the schema widget for editors, while_get_hidden_readonly_fields()cleanly removes shared credential params for view-only non-superusers.Also applies to: 85-93
openwisp_controller/config/admin.py (7)
83-141: Well-structured mixin implementation with proper security handling.The
ReadonlyPrettyJsonMixincorrectly usesformat_htmlto prevent XSS when rendering JSON data. The logic for determining readonly fields based onhas_change_permissionis sound, and the field mapping throughreadonly_json_fieldsintegrates cleanly with Django admin's field rendering.
493-500: Correct mixin integration with ConfigInline.The
ReadonlyPrettyJsonMixinis properly positioned in the MRO before other parent classes, ensuring itsget_fieldsandget_readonly_fieldsmethods are called first.
550-558: Properly implemented pretty field methods.The
pretty_contextandpretty_configmethods correctly delegate to_format_json_fieldand include translatableshort_descriptionattributes.
1119-1124: Consistent mixin integration with TemplateAdmin.The inheritance chain is correctly structured with
ReadonlyPrettyJsonMixinfirst, followed by other mixins andBaseConfigAdmin.
1313-1316: Correct mixin integration with VpnAdmin.Appropriately adds
ReadonlyPrettyJsonMixinand configures only theconfigfield for pretty JSON rendering.
1393-1393: Consistent mixin integration with DeviceGroupAdmin.Both
contextandmeta_datafields are properly configured for pretty JSON rendering.
1502-1517: Good refactoring of ConfigSettingsInline.The switch from
get_fields()override to a class-levelfieldslist is correct. The mixin'sget_fields()will use this list viasuper().get_fields()and apply the readonly field transformations. Theapp_settingsvalues are evaluated at module import time (as confirmed byopenwisp_controller/config/settings.py), so the conditional field inclusion works correctly.
| .readonly-json { | ||
| margin: 0; | ||
| padding: 15px; | ||
| white-space: pre-wrap; | ||
| word-wrap: break-word; | ||
| overflow: auto; | ||
| background-color: #fff; | ||
| border: 1px solid rgba(0, 0, 0, 0.08); | ||
| color: #333; | ||
| line-height: 1.6; | ||
| font-size: 1em; | ||
| font-family: | ||
| "Bitstream Vera Sans Mono", "Monaco", "Droid Sans Mono", "DejaVu Sans Mono", | ||
| "Ubuntu Mono", "Courier New", Courier, monospace; |
There was a problem hiding this comment.
Fix the new .readonly-json rule so stylelint passes.
This hunk still uses deprecated word-wrap and quotes the single-word Monaco family name, which matches the stylelint failures attached to this file.
♻️ Minimal fix
.readonly-json {
margin: 0;
padding: 15px;
white-space: pre-wrap;
- word-wrap: break-word;
+ overflow-wrap: break-word;
overflow: auto;
background-color: `#fff`;
border: 1px solid rgba(0, 0, 0, 0.08);
color: `#333`;
line-height: 1.6;
font-size: 1em;
font-family:
- "Bitstream Vera Sans Mono", "Monaco", "Droid Sans Mono", "DejaVu Sans Mono",
+ "Bitstream Vera Sans Mono", Monaco, "Droid Sans Mono", "DejaVu Sans Mono",
"Ubuntu Mono", "Courier New", Courier, monospace;
}📝 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.
| .readonly-json { | |
| margin: 0; | |
| padding: 15px; | |
| white-space: pre-wrap; | |
| word-wrap: break-word; | |
| overflow: auto; | |
| background-color: #fff; | |
| border: 1px solid rgba(0, 0, 0, 0.08); | |
| color: #333; | |
| line-height: 1.6; | |
| font-size: 1em; | |
| font-family: | |
| "Bitstream Vera Sans Mono", "Monaco", "Droid Sans Mono", "DejaVu Sans Mono", | |
| "Ubuntu Mono", "Courier New", Courier, monospace; | |
| .readonly-json { | |
| margin: 0; | |
| padding: 15px; | |
| white-space: pre-wrap; | |
| overflow-wrap: break-word; | |
| overflow: auto; | |
| background-color: `#fff`; | |
| border: 1px solid rgba(0, 0, 0, 0.08); | |
| color: `#333`; | |
| line-height: 1.6; | |
| font-size: 1em; | |
| font-family: | |
| "Bitstream Vera Sans Mono", Monaco, "Droid Sans Mono", "DejaVu Sans Mono", | |
| "Ubuntu Mono", "Courier New", Courier, monospace; | |
| } |
🧰 Tools
🪛 Stylelint (17.5.0)
[error] 46-46: Unexpected quotes around "Monaco" (font-family-name-quotes)
(font-family-name-quotes)
[error] 38-38: Expected "word-wrap" to be "overflow-wrap" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/static/config/css/admin.css` around lines 34 - 47,
The .readonly-json CSS rule uses deprecated word-wrap and unnecessarily quotes
the single-word font family "Monaco"; update the rule by replacing word-wrap
with overflow-wrap and remove the quotes around Monaco in the font-family list
(target the .readonly-json selector to find the block) so stylelint passes.
| import os | ||
| import django | ||
| from django.conf import settings | ||
| from unittest.mock import Mock | ||
|
|
||
| os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'tests.settings') | ||
| django.setup() | ||
|
|
||
| from django.contrib.admin import site | ||
| from openwisp_controller.config.admin import TemplateAdmin | ||
| from openwisp_controller.config.models import Template | ||
| from django.contrib.auth.models import User | ||
| from django.test import RequestFactory | ||
|
|
||
| request = RequestFactory().get('/admin/') | ||
| request.user = Mock() | ||
| request.user.is_superuser = False | ||
| request.user.has_perm = Mock(return_value=False) | ||
| request._recover_view = False | ||
|
|
||
| class MockAdmin(TemplateAdmin): | ||
| def has_change_permission(self, request, obj=None): | ||
| return False | ||
| def has_view_permission(self, request, obj=None): | ||
| return True | ||
| def has_add_permission(self, request): | ||
| return False | ||
|
|
||
| admin = MockAdmin(Template, site) | ||
| obj = Template() | ||
|
|
||
| print("fields list:", admin.fields) | ||
| print("get_fields:", admin.get_fields(request, obj)) | ||
| print("get_readonly_fields:", admin.get_readonly_fields(request, obj)) |
There was a problem hiding this comment.
Debug script should be removed or converted to a proper test case.
This file appears to be a standalone debugging script rather than an automated test. It uses print() statements instead of assertions and is not structured as a Django TestCase. According to the AI summary, proper test coverage exists in openwisp_controller/config/tests/test_admin.py.
Consider removing this file or converting it to a proper test if the functionality isn't already covered.
If keeping, convert to a proper test:
-import os
-import django
-from django.conf import settings
-from unittest.mock import Mock
-
-os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'tests.settings')
-django.setup()
-
-from django.contrib.admin import site
-from openwisp_controller.config.admin import TemplateAdmin
-from openwisp_controller.config.models import Template
-from django.contrib.auth.models import User
-from django.test import RequestFactory
-
-request = RequestFactory().get('/admin/')
-request.user = Mock()
-request.user.is_superuser = False
-request.user.has_perm = Mock(return_value=False)
-request._recover_view = False
-
-class MockAdmin(TemplateAdmin):
- def has_change_permission(self, request, obj=None):
- return False
- def has_view_permission(self, request, obj=None):
- return True
- def has_add_permission(self, request):
- return False
-
-admin = MockAdmin(Template, site)
-obj = Template()
-
-print("fields list:", admin.fields)
-print("get_fields:", admin.get_fields(request, obj))
-print("get_readonly_fields:", admin.get_readonly_fields(request, obj))
+from django.test import TestCase, RequestFactory
+from django.contrib.admin import site
+from unittest.mock import Mock
+
+from openwisp_controller.config.admin import TemplateAdmin
+from openwisp_controller.config.models import Template
+
+
+class TestReadonlyJsonFields(TestCase):
+ def test_readonly_fields_for_view_only_user(self):
+ request = RequestFactory().get('/admin/')
+ request.user = Mock()
+ request.user.is_superuser = False
+ request.user.has_perm = Mock(return_value=False)
+ request._recover_view = False
+
+ class MockAdmin(TemplateAdmin):
+ def has_change_permission(self, request, obj=None):
+ return False
+ def has_view_permission(self, request, obj=None):
+ return True
+ def has_add_permission(self, request):
+ return False
+
+ admin = MockAdmin(Template, site)
+ obj = Template()
+
+ readonly_fields = admin.get_readonly_fields(request, obj)
+ self.assertIn('pretty_config', readonly_fields)
+ self.assertIn('pretty_default_values', readonly_fields)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_readonly.py` around lines 1 - 34, This file is a standalone debug script
and should be removed or converted to a proper automated test: either delete
test_readonly.py if equivalent coverage exists in
openwisp_controller/config/tests/test_admin.py, or convert it into a Django
TestCase by replacing the top-level script with a class-based test (subclass
django.test.TestCase), move RequestFactory usage into setUp, instantiate
MockAdmin(Template, site) and Template() inside tests, replace print() calls
with assertions (e.g., assertEqual/assertTrue/assertFalse) against admin.fields,
admin.get_fields(request, obj) and admin.get_readonly_fields(request, obj), and
ensure request.user attributes (is_superuser, has_perm) are set via a proper
user or Mock within the test methods.
| from django.contrib.admin import site | ||
| from openwisp_controller.config.admin import TemplateAdmin | ||
| from openwisp_controller.config.models import Template | ||
| from django.contrib.auth.models import User |
There was a problem hiding this comment.
Unused import: User.
The User model is imported but never used in this script.
-from django.contrib.auth.models import User📝 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.
| from django.contrib.auth.models import User |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_readonly.py` at line 12, Remove the unused import of User from
django.contrib.auth.models in this test_readonly.py file; locate the line that
imports "User" and delete that import (or consolidate imports) so the module no
longer imports an unused symbol "User".
Multiple CI Failures DetectedHello @vivinakashrangarajan,
|
…ncies, resolve GDAL and Django settings issues
Multiple Code Style and Commit Message FailuresHello @vivinakashrangarajan,
For example:
|
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@anti_gravity.py`:
- Around line 1-47: This script (anti_gravity.py) is a throwaway dev utility
that should not live in production code; move it to a scripts/ or tools/
directory, add a module docstring describing purpose and intended usage, and
guard all destructive operations: replace direct sys.modules assignments and
writing of conftest_path with an opt-in check (e.g., require an env var or CLI
flag), verify os.path.exists(conftest_path) and back it up before overwriting,
constrain shutil.rmtree calls by checking each folder is inside the repo root
and exists (use os.path.abspath and root checks) and catch/log exceptions
explicitly rather than bare except, and avoid unguarded mutation of
os.environ["GDAL_LIBRARY_PATH"]; limit that to opt-in behavior and document it.
- Around line 19-23: The current block that opens conftest_path with open(...,
"w") and writes hardcoded content will unconditionally overwrite any existing
conftest.py; change this to first check for an existing file
(os.path.exists(conftest_path)), and if it exists create a backup (e.g., rename
or copy to conftest_path + ".bak") or open in append mode and merge safely,
otherwise create the file; ensure the logic around conftest_path and the write
operation handles errors (raise/log) and does not blindly destroy existing test
fixtures.
- Around line 33-34: Replace the bare "except:" that currently does nothing by
catching only regular exceptions (use "except Exception as e") and log the error
(e.g., logger.exception(...) or print with traceback) instead of silently
passing; if the code cannot handle the error then re-raise the exception after
logging so that KeyboardInterrupt/SystemExit are not swallowed and failures are
visible.
- Around line 29-34: The hardcoded loop that removes three specific
"__pycache__" folders is fragile; replace it with a recursive discovery +
removal (e.g., use os.walk or pathlib.Path.rglob to find all "__pycache__"
directories) instead of the current for loop over folder, and for each
discovered path call shutil.rmtree; avoid a bare except—catch and log exceptions
(or print) with the path and error to aid debugging (refer to the existing
folder variable and shutil.rmtree usage to locate where to change).
In `@conftest.py`:
- Around line 1-3: Remove the manual sys.modules stubbing in conftest.py: delete
the three lines that set sys.modules['django.contrib.gis'] and
sys.modules['django.contrib.gis.gdal'] to None, because this breaks imports
during Django app registry; rely on the existing conditional in settings.py that
removes 'django.contrib.gis' from INSTALLED_APPS when pytest runs (leave that
conditional intact) so Django initializes correctly.
In `@myproject/manage.py`:
- Around line 3-6: The comment above the sys.modules assignments is too terse;
update it to explain why Django GIS is being disabled by assigning None to
sys.modules['django.contrib.gis'] and sys.modules['django.contrib.gis.gdal'] —
e.g., note that these entries are set to None to prevent Django from importing
the optional GIS/GDAL packages at startup because the runtime environment lacks
GDAL libraries (or to avoid import-time side-effects in non-GIS deployments),
and mention any runtime conditions or platform-specific issues this work-around
addresses so future maintainers understand the reason.
- Around line 1-11: The shebang is misplaced and sys is imported twice: move the
shebang (#!/usr/bin/env python) to be the very first line of the file, remove
the duplicate import of sys (keep a single import sys), and ensure the django
GIS disabling lines (sys.modules['django.contrib.gis'] = None and
sys.modules['django.contrib.gis.gdal'] = None) come after that single import;
update/manage the top of the file so only one import sys exists and the shebang
is the first line.
In `@myproject/myproject/settings.py`:
- Around line 1-13: Move the module docstring so it is the very first statement
in the file (before any imports or executable code like the os.environ
assignment), ensure the initial import of os remains only once by removing the
duplicate import, and keep the os.environ["GDAL_LIBRARY_PATH"] assignment after
the imports; update references to the module docstring, the import os statement,
and the os.environ["GDAL_LIBRARY_PATH"] line accordingly.
In `@openwisp_controller/conftest.py`:
- Around line 1-4: The test conftest is setting
sys.modules['django.contrib.gis'] and ['django.contrib.gis.gdal'] to None which
breaks imports used by openwisp_controller/geo/base/models.py (it imports from
django.contrib.gis.db) and tests that import Point/GEOSGeometry; remove those
None assignments and instead either remove this global blocking, or replace them
with lightweight mocks (e.g., inject a types.ModuleType with the minimal
attributes like a .db submodule and any used classes) using monkeypatch or
pytest fixtures so imports of 'django.contrib.gis' and 'django.contrib.gis.gdal'
don't raise ModuleNotFoundError while keeping tests isolated.
In `@pytest_err2.txt`:
- Around line 1-265: This file is a local pytest traceback/console log that must
not be committed; remove pytest_err*.txt and pytest_output*.txt from the PR, add
patterns like "pytest_err*.txt" and "pytest_output*.txt" to .gitignore, and if
already committed remove them from the index (e.g., stop tracking with git rm
--cached) before committing the .gitignore change so the sensitive local paths
(shown in pytest_err2.txt) are not stored in the repository.
In `@tests/manage.py`:
- Around line 1-10: Move the shebang (#!/usr/bin/env python) to line 1, remove
the duplicate import sys so there is only a single "import sys", and eliminate
the unsafe sys.modules assignments (sys.modules['django.contrib.gis'] and
sys.modules['django.contrib.gis.gdal']); instead, handle optional GIS by
catching ImportError where django.contrib.gis is imported (or guard GIS usage
with try/except) so imports fail gracefully rather than forcing
ModuleNotFoundError.
In `@tests/openwisp2/settings.py`:
- Around line 76-81: The current pytest branch only removes GIS apps from
INSTALLED_APPS but the spatial DB backend
(openwisp_utils.db.backends.spatialite) and rest_framework_gis still cause GDAL
imports; update the pytest-only configuration to (1) switch
DATABASES['default']['ENGINE'] to 'django.db.backends.sqlite3' (or another
non-spatial backend) so the spatialite backend is not imported, (2) ensure
'rest_framework_gis' is removed from INSTALLED_APPS in the same pytest
conditional, and (3) optionally mark or skip geo-dependent tests; locate and
modify the DATABASES and INSTALLED_APPS handling (symbols: DATABASES,
INSTALLED_APPS, openwisp_utils.db.backends.spatialite, rest_framework_gis) to
implement these changes.
🪄 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: 33cf985b-08b5-4ba7-8d68-2bbfc0183210
📒 Files selected for processing (17)
.gitignoreanti_gravity.pyconftest.pymyproject/manage.pymyproject/myproject/settings.pyopenwisp_controller/conftest.pypytest.inipytest_err.txtpytest_err2.txtpytest_err_utf8.txtpytest_final_out.txtpytest_out.txtpytest_output_step.txtpytest_output_step_utf8.txtpytest_result.txttests/manage.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Python==3.10 | django~=5.2.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.2.0
- GitHub Check: Python==3.10 | 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.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.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:
conftest.pyopenwisp_controller/conftest.pytests/manage.pytests/openwisp2/settings.pymyproject/manage.pyanti_gravity.pymyproject/myproject/settings.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:
conftest.pyopenwisp_controller/conftest.pytests/manage.pytests/openwisp2/settings.pymyproject/manage.pyanti_gravity.pymyproject/myproject/settings.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:
conftest.pyopenwisp_controller/conftest.pytests/manage.pytests/openwisp2/settings.pymyproject/manage.pyanti_gravity.pymyproject/myproject/settings.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:
conftest.pyopenwisp_controller/conftest.pytests/manage.pytests/openwisp2/settings.pymyproject/manage.pyanti_gravity.pymyproject/myproject/settings.py
🧠 Learnings (4)
📚 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:
conftest.pyopenwisp_controller/conftest.pytests/manage.pytests/openwisp2/settings.pymyproject/manage.pyanti_gravity.pymyproject/myproject/settings.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/conftest.pytests/openwisp2/settings.py
📚 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/conftest.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/conftest.py
🔇 Additional comments (3)
.gitignore (1)
75-76: Good addition for local dev hygiene.Ignoring
venv/and.envis appropriate and helps prevent accidental commits of local environments/secrets.pytest_output_step_utf8.txt (1)
1-543: Debug log file should not be committed.Same issue as
pytest_err2.txt- this error log contains local development paths and should be removed from the repository and added to.gitignore.pytest.ini (1)
3-3: LGTM!The settings module path change aligns with the test directory structure and is correctly supported by
pythonpath = testson line 6.
| import sys | ||
| import os | ||
| import shutil | ||
|
|
||
| print("🚀 Initiating anti-gravity protocol...") | ||
|
|
||
| # Step 1: Disable Django GIS (the real villain) | ||
|
|
||
| sys.modules['django.contrib.gis'] = None | ||
| sys.modules['django.contrib.gis.gdal'] = None | ||
|
|
||
| print("✅ GIS gravity disabled") | ||
|
|
||
| # Step 2: Ensure conftest.py is in root | ||
|
|
||
| root_path = os.getcwd() | ||
| conftest_path = os.path.join(root_path, "conftest.py") | ||
|
|
||
| with open(conftest_path, "w") as f: | ||
| f.write("""import sys | ||
| sys.modules['django.contrib.gis'] = None | ||
| sys.modules['django.contrib.gis.gdal'] = None | ||
| """) | ||
|
|
||
| print("✅ conftest.py created at root") | ||
|
|
||
| # Step 3: Clean cache (remove Python dust) | ||
|
|
||
| for folder in ["__pycache__", "openwisp_controller/__pycache__", "myproject/__pycache__"]: | ||
| try: | ||
| shutil.rmtree(folder) | ||
| print(f"🧹 Removed {folder}") | ||
| except: | ||
| pass | ||
|
|
||
| print("✨ Cache cleared") | ||
|
|
||
| # Step 4: Set environment variable (extra safety) | ||
|
|
||
| os.environ["GDAL_LIBRARY_PATH"] = "" | ||
|
|
||
| print("🔧 Environment stabilized") | ||
|
|
||
| print("\n🚀 Now run this in terminal:") | ||
| print("pytest") | ||
|
|
||
| print("\n🎯 Outcome: GDAL error disappears, tests start running, PR moves forward!") |
There was a problem hiding this comment.
This appears to be a development/debugging utility that should not be committed.
This script has characteristics of a throwaway development tool (emoji messages, casual naming like "anti-gravity protocol", "Python dust") rather than production code. It modifies the filesystem by overwriting conftest.py and deleting directories without safeguards.
If this functionality is needed for CI/test setup, consider:
- Moving it to a
scripts/ortools/directory with proper documentation - Adding a clear docstring explaining its purpose
- Adding safety checks before file operations
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@anti_gravity.py` around lines 1 - 47, This script (anti_gravity.py) is a
throwaway dev utility that should not live in production code; move it to a
scripts/ or tools/ directory, add a module docstring describing purpose and
intended usage, and guard all destructive operations: replace direct sys.modules
assignments and writing of conftest_path with an opt-in check (e.g., require an
env var or CLI flag), verify os.path.exists(conftest_path) and back it up before
overwriting, constrain shutil.rmtree calls by checking each folder is inside the
repo root and exists (use os.path.abspath and root checks) and catch/log
exceptions explicitly rather than bare except, and avoid unguarded mutation of
os.environ["GDAL_LIBRARY_PATH"]; limit that to opt-in behavior and document it.
| with open(conftest_path, "w") as f: | ||
| f.write("""import sys | ||
| sys.modules['django.contrib.gis'] = None | ||
| sys.modules['django.contrib.gis.gdal'] = None | ||
| """) |
There was a problem hiding this comment.
File overwrite without existence check or backup.
This overwrites conftest.py unconditionally. If a legitimate conftest.py already exists with important test fixtures or configuration, it will be destroyed without warning.
🛡️ Suggested safer approach if this script is kept
+if os.path.exists(conftest_path):
+ print(f"⚠️ conftest.py already exists at {conftest_path}, skipping")
+else:
with open(conftest_path, "w") as f:
f.write("""import sys
sys.modules['django.contrib.gis'] = None
sys.modules['django.contrib.gis.gdal'] = None
""")
+ print("✅ conftest.py created at root")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@anti_gravity.py` around lines 19 - 23, The current block that opens
conftest_path with open(..., "w") and writes hardcoded content will
unconditionally overwrite any existing conftest.py; change this to first check
for an existing file (os.path.exists(conftest_path)), and if it exists create a
backup (e.g., rename or copy to conftest_path + ".bak") or open in append mode
and merge safely, otherwise create the file; ensure the logic around
conftest_path and the write operation handles errors (raise/log) and does not
blindly destroy existing test fixtures.
| for folder in ["__pycache__", "openwisp_controller/__pycache__", "myproject/__pycache__"]: | ||
| try: | ||
| shutil.rmtree(folder) | ||
| print(f"🧹 Removed {folder}") | ||
| except: | ||
| pass |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded cache paths are fragile and incomplete.
Only three specific __pycache__ directories are listed. Other packages and nested directories are ignored. If cache cleaning is necessary, use a recursive approach.
♻️ More robust cache cleaning
-for folder in ["__pycache__", "openwisp_controller/__pycache__", "myproject/__pycache__"]:
- try:
- shutil.rmtree(folder)
- print(f"🧹 Removed {folder}")
- except:
- pass
+# Recursively remove all __pycache__ directories
+for root, dirs, _ in os.walk(root_path):
+ for d in dirs:
+ if d == "__pycache__":
+ cache_path = os.path.join(root, d)
+ try:
+ shutil.rmtree(cache_path)
+ print(f"🧹 Removed {cache_path}")
+ except (FileNotFoundError, PermissionError):
+ pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@anti_gravity.py` around lines 29 - 34, The hardcoded loop that removes three
specific "__pycache__" folders is fragile; replace it with a recursive discovery
+ removal (e.g., use os.walk or pathlib.Path.rglob to find all "__pycache__"
directories) instead of the current for loop over folder, and for each
discovered path call shutil.rmtree; avoid a bare except—catch and log exceptions
(or print) with the path and error to aid debugging (refer to the existing
folder variable and shutil.rmtree usage to locate where to change).
| except: | ||
| pass |
There was a problem hiding this comment.
Bare except: clause silently swallows all exceptions.
Using except: without specifying an exception type catches everything including KeyboardInterrupt and SystemExit. Combined with pass, this violates error handling guidelines—errors that cannot be resolved should be logged.
🐛 Proposed fix
try:
shutil.rmtree(folder)
print(f"🧹 Removed {folder}")
- except:
- pass
+ except FileNotFoundError:
+ pass # Directory doesn't exist, nothing to remove
+ except PermissionError as e:
+ print(f"⚠️ Could not remove {folder}: {e}")📝 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.
| except: | |
| pass | |
| try: | |
| shutil.rmtree(folder) | |
| print(f"🧹 Removed {folder}") | |
| except FileNotFoundError: | |
| pass # Directory doesn't exist, nothing to remove | |
| except PermissionError as e: | |
| print(f"⚠️ Could not remove {folder}: {e}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@anti_gravity.py` around lines 33 - 34, Replace the bare "except:" that
currently does nothing by catching only regular exceptions (use "except
Exception as e") and log the error (e.g., logger.exception(...) or print with
traceback) instead of silently passing; if the code cannot handle the error then
re-raise the exception after logging so that KeyboardInterrupt/SystemExit are
not swallowed and failures are visible.
| import os | ||
| os.environ["GDAL_LIBRARY_PATH"] = "" | ||
| """ | ||
| Django settings for myproject project. | ||
|
|
||
| Generated by 'django-admin startproject' using Django 5.2.12. | ||
|
|
||
| For more information on this file, see | ||
| https://docs.djangoproject.com/en/5.2/topics/settings/ | ||
|
|
||
| For the full list of settings and their values, see | ||
| https://docs.djangoproject.com/en/5.2/ref/settings/ | ||
| """ |
There was a problem hiding this comment.
Module docstring placed after executable code; duplicate import of os.
The module docstring (lines 3-13) should be at the very top of the file before any imports or executable code. Additionally, os is imported twice (line 1 and line 24).
🔧 Proposed fix
+"""
+Django settings for myproject project.
+
+Generated by 'django-admin startproject' using Django 5.2.12.
+
+For more information on this file, see
+https://docs.djangoproject.com/en/5.2/topics/settings/
+
+For the full list of settings and their values, see
+https://docs.djangoproject.com/en/5.2/ref/settings/
+"""
+
import os
os.environ["GDAL_LIBRARY_PATH"] = ""
-"""
-Django settings for myproject project.
-
-Generated by 'django-admin startproject' using Django 5.2.12.
-
-For more information on this file, see
-https://docs.djangoproject.com/en/5.2/topics/settings/
-
-For the full list of settings and their values, see
-https://docs.djangoproject.com/en/5.2/ref/settings/
-"""
from pathlib import PathAnd remove the duplicate import at line 24:
# See https://docs.djangoproject.com/en/5.2/howto/deployment/checklist/
-import os
SECRET_KEY = os.environ.get("SECRET_KEY", "unsafe-secret-key")📝 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.
| import os | |
| os.environ["GDAL_LIBRARY_PATH"] = "" | |
| """ | |
| Django settings for myproject project. | |
| Generated by 'django-admin startproject' using Django 5.2.12. | |
| For more information on this file, see | |
| https://docs.djangoproject.com/en/5.2/topics/settings/ | |
| For the full list of settings and their values, see | |
| https://docs.djangoproject.com/en/5.2/ref/settings/ | |
| """ | |
| """ | |
| Django settings for myproject project. | |
| Generated by 'django-admin startproject' using Django 5.2.12. | |
| For more information on this file, see | |
| https://docs.djangoproject.com/en/5.2/topics/settings/ | |
| For the full list of settings and their values, see | |
| https://docs.djangoproject.com/en/5.2/ref/settings/ | |
| """ | |
| import os | |
| os.environ["GDAL_LIBRARY_PATH"] = "" | |
| from pathlib import Path |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@myproject/myproject/settings.py` around lines 1 - 13, Move the module
docstring so it is the very first statement in the file (before any imports or
executable code like the os.environ assignment), ensure the initial import of os
remains only once by removing the duplicate import, and keep the
os.environ["GDAL_LIBRARY_PATH"] assignment after the imports; update references
to the module docstring, the import os statement, and the
os.environ["GDAL_LIBRARY_PATH"] line accordingly.
| pytest : Traceback | ||
| (most recent call last): | ||
| At line:1 char:1 | ||
| + pytest 2> | ||
| pytest_err.log | ||
| + | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| + CategoryInfo | ||
| : NotSpecified | ||
| : (Traceback (most | ||
| recent call last):: | ||
| String) [], RemoteE | ||
| xception | ||
| + FullyQualifiedErr | ||
| orId : NativeComman | ||
| dError | ||
|
|
||
| File "<frozen | ||
| runpy>", line 198, in | ||
| _run_module_as_main | ||
| File "<frozen | ||
| runpy>", line 88, in | ||
| _run_code | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Scripts\pytest.exe\__mai | ||
| n__.py", line 7, in | ||
| <module> | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\_pytes | ||
| t\config\__init__.py", | ||
| line 223, in | ||
| console_main | ||
| code = main() | ||
| ^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\_pytes | ||
| t\config\__init__.py", | ||
| line 193, in main | ||
| config = _preparecon | ||
| fig(new_args, plugins) | ||
| ^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\_pytes | ||
| t\config\__init__.py", | ||
| line 361, in | ||
| _prepareconfig | ||
| config: Config = plu | ||
| ginmanager.hook.pytest_c | ||
| mdline_parse( | ||
| ^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_hooks.py", line 512, | ||
| in __call__ | ||
| return self._hookexe | ||
| c(self.name, | ||
| self._hookimpls.copy(), | ||
| kwargs, firstresult) | ||
| ^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_manager.py", line | ||
| 120, in _hookexec | ||
| return self._inner_h | ||
| ookexec(hook_name, | ||
| methods, kwargs, | ||
| firstresult) | ||
| ^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_callers.py", line | ||
| 167, in _multicall | ||
| raise exception | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_callers.py", line | ||
| 139, in _multicall | ||
| teardown.throw(excep | ||
| tion) | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\_pytes | ||
| t\helpconfig.py", line | ||
| 124, in | ||
| pytest_cmdline_parse | ||
| config = yield | ||
| ^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_callers.py", line | ||
| 121, in _multicall | ||
| res = hook_impl.func | ||
| tion(*args) | ||
| ^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\_pytes | ||
| t\config\__init__.py", | ||
| line 1186, in | ||
| pytest_cmdline_parse | ||
| self.parse(args) | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\_pytes | ||
| t\config\__init__.py", | ||
| line 1556, in parse | ||
| self.hook.pytest_loa | ||
| d_initial_conftests( | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_hooks.py", line 512, | ||
| in __call__ | ||
| return self._hookexe | ||
| c(self.name, | ||
| self._hookimpls.copy(), | ||
| kwargs, firstresult) | ||
| ^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_manager.py", line | ||
| 120, in _hookexec | ||
| return self._inner_h | ||
| ookexec(hook_name, | ||
| methods, kwargs, | ||
| firstresult) | ||
| ^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_callers.py", line | ||
| 167, in _multicall | ||
| raise exception | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_callers.py", line | ||
| 139, in _multicall | ||
| teardown.throw(excep | ||
| tion) | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\_pytes | ||
| t\capture.py", line | ||
| 173, in pytest_load_init | ||
| ial_conftests | ||
| yield | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pluggy | ||
| \_callers.py", line | ||
| 121, in _multicall | ||
| res = hook_impl.func | ||
| tion(*args) | ||
| ^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pytest | ||
| _django\plugin.py", | ||
| line 370, in pytest_load | ||
| _initial_conftests | ||
| _setup_django(early_ | ||
| config) | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\pytest | ||
| _django\plugin.py", | ||
| line 242, in | ||
| _setup_django | ||
| django.setup() | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\django | ||
| \__init__.py", line 24, | ||
| in setup | ||
| apps.populate(settin | ||
| gs.INSTALLED_APPS) | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\django | ||
| \apps\registry.py", | ||
| line 91, in populate | ||
| app_config = | ||
| AppConfig.create(entry) | ||
|
|
||
| ^^^^^^^^^^^^^^^^^^^^^^^ | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\site-packages\django | ||
| \apps\config.py", line | ||
| 193, in create | ||
| import_module(entry) | ||
| File "C:\Users\VIVIN | ||
| AKASH R\AppData\Local\Pr | ||
| ograms\Python\Python311\ | ||
| Lib\importlib\__init__.p | ||
| y", line 126, in | ||
| import_module | ||
| return _bootstrap._g | ||
| cd_import(name[level:], | ||
| package, level) | ||
| ^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^^^^^^^^^^^^ | ||
| File "<frozen | ||
| importlib._bootstrap>", | ||
| line 1204, in | ||
| _gcd_import | ||
| File "<frozen | ||
| importlib._bootstrap>", | ||
| line 1187, in | ||
| _find_and_load | ||
| ModuleNotFoundError: | ||
| import of | ||
| django.contrib.gis | ||
| halted; None in | ||
| sys.modules |
There was a problem hiding this comment.
Debug/error log files should not be committed to the repository.
This file contains a raw pytest traceback from local development. It includes local filesystem paths with a developer's username and should be added to .gitignore rather than committed. The same applies to pytest_output_step_utf8.txt.
Consider adding these patterns to .gitignore:
pytest_err*.txt
pytest_output*.txt
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pytest_err2.txt` around lines 1 - 265, This file is a local pytest
traceback/console log that must not be committed; remove pytest_err*.txt and
pytest_output*.txt from the PR, add patterns like "pytest_err*.txt" and
"pytest_output*.txt" to .gitignore, and if already committed remove them from
the index (e.g., stop tracking with git rm --cached) before committing the
.gitignore change so the sensitive local paths (shown in pytest_err2.txt) are
not stored in the repository.
| import sys | ||
|
|
||
| # Disable Django GIS before it loads | ||
|
|
||
| sys.modules['django.contrib.gis'] = None | ||
| sys.modules['django.contrib.gis.gdal'] = None | ||
|
|
||
| #!/usr/bin/env python | ||
| import os | ||
| import sys |
There was a problem hiding this comment.
Shebang must be on line 1; duplicate import sys.
The shebang #!/usr/bin/env python on line 8 won't be recognized by the OS since it must be the very first line of the file. Additionally, sys is imported twice (lines 1 and 10).
Note: The sys.modules blocking approach has the same issue as conftest.py - it will cause ModuleNotFoundError rather than gracefully disabling GIS.
🔧 Proposed fix for shebang and duplicate import
+#!/usr/bin/env python
import sys
# Disable Django GIS before it loads
-
sys.modules['django.contrib.gis'] = None
sys.modules['django.contrib.gis.gdal'] = None
-#!/usr/bin/env python
import os
-import sys
if __name__ == "__main__":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/manage.py` around lines 1 - 10, Move the shebang (#!/usr/bin/env
python) to line 1, remove the duplicate import sys so there is only a single
"import sys", and eliminate the unsafe sys.modules assignments
(sys.modules['django.contrib.gis'] and sys.modules['django.contrib.gis.gdal']);
instead, handle optional GIS by catching ImportError where django.contrib.gis is
imported (or guard GIS usage with try/except) so imports fail gracefully rather
than forcing ModuleNotFoundError.
tests/openwisp2/settings.py
Outdated
| # Disable GIS apps during pytest to avoid GDAL dependency | ||
| if 'pytest' in sys.modules: | ||
| INSTALLED_APPS = [ | ||
| app for app in INSTALLED_APPS | ||
| if app not in ['django.contrib.gis', 'openwisp_controller.geo'] | ||
| ] |
There was a problem hiding this comment.
Removing GIS apps from INSTALLED_APPS is insufficient; the database backend still requires GDAL.
The openwisp_utils.db.backends.spatialite engine (line 15) internally imports from django.contrib.gis.db.backends.spatialite, which requires GDAL regardless of INSTALLED_APPS content. The error log in pytest_output_step_utf8.txt confirms this chain.
Additionally, rest_framework_gis (line 68) also depends on Django GIS.
To run tests without GDAL, you would need to:
- Use a non-spatial database backend (e.g.,
django.db.backends.sqlite3) - Remove
rest_framework_gisfromINSTALLED_APPS - Skip or mock all tests that use geo functionality
Alternatively, ensure GDAL is available in the test environment (the standard approach for GeoDjango projects).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/openwisp2/settings.py` around lines 76 - 81, The current pytest branch
only removes GIS apps from INSTALLED_APPS but the spatial DB backend
(openwisp_utils.db.backends.spatialite) and rest_framework_gis still cause GDAL
imports; update the pytest-only configuration to (1) switch
DATABASES['default']['ENGINE'] to 'django.db.backends.sqlite3' (or another
non-spatial backend) so the spatialite backend is not imported, (2) ensure
'rest_framework_gis' is removed from INSTALLED_APPS in the same pytest
conditional, and (3) optionally mark or skip geo-dependent tests; locate and
modify the DATABASES and INSTALLED_APPS handling (symbols: DATABASES,
INSTALLED_APPS, openwisp_utils.db.backends.spatialite, rest_framework_gis) to
implement these changes.
CI Failures: Code Style and Commit MessageHello @vivinakashrangarajan,
Example:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/openwisp2/settings.py (2)
13-19:⚠️ Potential issue | 🟠 MajorRemoving GIS apps from
INSTALLED_APPSis insufficient—the database backend still requires GDAL.The
openwisp_utils.db.backends.spatialiteengine on line 15 internally imports fromdjango.contrib.gis.db.backends.spatialite, which requires GDAL regardless ofINSTALLED_APPS. To run tests without GDAL, you would need to switch to a non-spatial backend (e.g.,django.db.backends.sqlite3) or ensure GDAL is available in the test environment.#!/bin/bash # Verify if spatialite backend imports from django.contrib.gis rg -n "from django\.contrib\.gis" --type py -g '*spatialite*' 2>/dev/null || echo "Pattern not found in local files" # Check openwisp_utils backend source (if available as installed package info) python -c "import openwisp_utils.db.backends.spatialite; import inspect; print(inspect.getfile(openwisp_utils.db.backends.spatialite))" 2>/dev/null || echo "Cannot inspect openwisp_utils backend"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openwisp2/settings.py` around lines 13 - 19, The tests currently use DATABASES with ENGINE set to openwisp_utils.db.backends.spatialite which imports django.contrib.gis and thus requires GDAL; change the test settings to use a non-spatial backend (e.g., set DATABASES["default"]["ENGINE"] to "django.db.backends.sqlite3") or ensure GDAL is installed in the test environment, and keep any GIS app removals in INSTALLED_APPS only as a complement, not a substitute for switching the ENGINE.
77-81:⚠️ Potential issue | 🟠 Major
rest_framework_gisshould also be removed when disabling GIS apps.
rest_framework_gis(line 68) depends ondjango.contrib.gis. If you're removing GIS apps to avoid GDAL imports, this app should be excluded as well; otherwise it will still trigger GIS imports.🔧 Proposed fix
if os.environ.get("PYTEST_CURRENT_TEST") or os.environ.get("CI"): INSTALLED_APPS = [ app for app in INSTALLED_APPS - if app not in ['django.contrib.gis', 'openwisp_controller.geo'] + if app not in ['django.contrib.gis', 'openwisp_controller.geo', 'rest_framework_gis'] ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openwisp2/settings.py` around lines 77 - 81, When disabling GIS apps in the test/CI branch, ensure INSTALLED_APPS filtering also excludes 'rest_framework_gis' so it doesn't import GDAL via django.contrib.gis; update the list comprehension that mutates INSTALLED_APPS (the block referencing INSTALLED_APPS and the conditional checking PYTEST_CURRENT_TEST/CI) to filter out 'rest_framework_gis' in addition to 'django.contrib.gis' and 'openwisp_controller.geo'.tests/manage.py (1)
1-7:⚠️ Potential issue | 🟠 MajorShebang must be on line 1; duplicate
import sysand dead code remain.The shebang on line 5 won't be recognized by the OS—it must be the very first line. Additionally,
sysis imported twice (lines 1 and 7), and lines 1-4 appear to be leftover code that does nothing after thesys.modulespatching was removed.🔧 Proposed fix
-import sys - -# GIS apps are now disabled in settings.py using environment variables - #!/usr/bin/env python import os import sys🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/manage.py` around lines 1 - 7, Move the shebang (#!/usr/bin/env python) to be the very first line of tests/manage.py, remove the duplicated "import sys" so there is a single import, and delete the leftover dead lines (the initial stray comments and the unused sys.modules patching remnants) so the file starts with the shebang followed by the necessary imports only; ensure references to "import sys" and the shebang are the only changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conftest.py`:
- Around line 1-2: Delete the unused conftest.py file entirely; it only contains
an unused import of sys and a comment (no fixtures or hooks), so remove
conftest.py from the repo to clean up dead code and avoid confusion.
In `@tests/openwisp2/settings.py`:
- Around line 76-81: The SAMPLE_APP removal code fails if GIS apps were already
stripped from INSTALLED_APPS; update the removal to be idempotent by checking
membership before removing (e.g. wrap any
INSTALLED_APPS.remove('openwisp_controller.geo') calls with an if
'openwisp_controller.geo' in INSTALLED_APPS check or replace with a safe
list-comprehension filter), and do the same for any other explicit removes so
SAMPLE_APP logic no longer raises ValueError when GIS apps were previously
removed.
---
Duplicate comments:
In `@tests/manage.py`:
- Around line 1-7: Move the shebang (#!/usr/bin/env python) to be the very first
line of tests/manage.py, remove the duplicated "import sys" so there is a single
import, and delete the leftover dead lines (the initial stray comments and the
unused sys.modules patching remnants) so the file starts with the shebang
followed by the necessary imports only; ensure references to "import sys" and
the shebang are the only changes.
In `@tests/openwisp2/settings.py`:
- Around line 13-19: The tests currently use DATABASES with ENGINE set to
openwisp_utils.db.backends.spatialite which imports django.contrib.gis and thus
requires GDAL; change the test settings to use a non-spatial backend (e.g., set
DATABASES["default"]["ENGINE"] to "django.db.backends.sqlite3") or ensure GDAL
is installed in the test environment, and keep any GIS app removals in
INSTALLED_APPS only as a complement, not a substitute for switching the ENGINE.
- Around line 77-81: When disabling GIS apps in the test/CI branch, ensure
INSTALLED_APPS filtering also excludes 'rest_framework_gis' so it doesn't import
GDAL via django.contrib.gis; update the list comprehension that mutates
INSTALLED_APPS (the block referencing INSTALLED_APPS and the conditional
checking PYTEST_CURRENT_TEST/CI) to filter out 'rest_framework_gis' in addition
to 'django.contrib.gis' and 'openwisp_controller.geo'.
🪄 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: 70c5766c-aadf-48da-bfbb-1b4f7878d4a2
📒 Files selected for processing (3)
conftest.pytests/manage.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (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:
conftest.pytests/manage.pytests/openwisp2/settings.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:
conftest.pytests/manage.pytests/openwisp2/settings.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:
conftest.pytests/manage.pytests/openwisp2/settings.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:
conftest.pytests/manage.pytests/openwisp2/settings.py
🧠 Learnings (2)
📚 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:
conftest.pytests/manage.pytests/openwisp2/settings.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:
tests/openwisp2/settings.py
| import sys | ||
| # GIS apps are now disabled in settings.py using environment variables |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove this empty/unused conftest.py file.
After removing the sys.modules patching (per previous review), this file only imports sys (which is never used) and contains a comment. It serves no purpose and should be deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conftest.py` around lines 1 - 2, Delete the unused conftest.py file entirely;
it only contains an unused import of sys and a comment (no fixtures or hooks),
so remove conftest.py from the repo to clean up dead code and avoid confusion.
| # Disable GIS apps during tests (works in CI + local) | ||
| if os.environ.get("PYTEST_CURRENT_TEST") or os.environ.get("CI"): | ||
| INSTALLED_APPS = [ | ||
| app for app in INSTALLED_APPS | ||
| if app not in ['django.contrib.gis', 'openwisp_controller.geo'] | ||
| ] |
There was a problem hiding this comment.
SAMPLE_APP code will crash when GIS apps already removed.
When both PYTEST_CURRENT_TEST/CI and SAMPLE_APP environment variables are set, line 240 will raise ValueError: 'openwisp_controller.geo' is not in list because the GIS conditional on lines 77-81 already removed it from INSTALLED_APPS.
🐛 Proposed fix
# Disable GIS apps during tests (works in CI + local)
-if os.environ.get("PYTEST_CURRENT_TEST") or os.environ.get("CI"):
+_disable_gis = os.environ.get("PYTEST_CURRENT_TEST") or os.environ.get("CI")
+if _disable_gis and not os.environ.get("SAMPLE_APP", False):
INSTALLED_APPS = [
app for app in INSTALLED_APPS
if app not in ['django.contrib.gis', 'openwisp_controller.geo']
]📝 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.
| # Disable GIS apps during tests (works in CI + local) | |
| if os.environ.get("PYTEST_CURRENT_TEST") or os.environ.get("CI"): | |
| INSTALLED_APPS = [ | |
| app for app in INSTALLED_APPS | |
| if app not in ['django.contrib.gis', 'openwisp_controller.geo'] | |
| ] | |
| # Disable GIS apps during tests (works in CI + local) | |
| _disable_gis = os.environ.get("PYTEST_CURRENT_TEST") or os.environ.get("CI") | |
| if _disable_gis and not os.environ.get("SAMPLE_APP", False): | |
| INSTALLED_APPS = [ | |
| app for app in INSTALLED_APPS | |
| if app not in ['django.contrib.gis', 'openwisp_controller.geo'] | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/openwisp2/settings.py` around lines 76 - 81, The SAMPLE_APP removal
code fails if GIS apps were already stripped from INSTALLED_APPS; update the
removal to be idempotent by checking membership before removing (e.g. wrap any
INSTALLED_APPS.remove('openwisp_controller.geo') calls with an if
'openwisp_controller.geo' in INSTALLED_APPS check or replace with a safe
list-comprehension filter), and do the same for any other explicit removes so
SAMPLE_APP logic no longer raises ValueError when GIS apps were previously
removed.
1 participant
Description of Changes
This PR fixes Issue #848 by improving how JSONField data is rendered in the Django admin for users with read-only permissions.
Problem
When a user has only
viewpermission (and no change permission), Django renders fields as disabled inputs instead of usingreadonly_fields. As a result, JSON data is displayed as raw dictionary strings, and the formatting provided byReadonlyPrettyJsonMixinis bypassed.Solution
This PR enhances
ReadonlyPrettyJsonMixinto correctly handle view-only users:has_change_permissionreadonly_fields<pre class="readonly-json">The mixin has also been applied to
ConfigSettingsInlineto ensure consistent behavior in inline admin views.Implementation Details
get_readonly_fieldsto inject JSON fields for view-only usersget_fieldsbehavior in Django adminjson.loadsfor stringified valuesformat_htmlfor secure rendering (avoiding XSS risks)Edge Cases Handled
{}) without misrepresentationreadonly-jsonclass)Tests
Added test coverage to ensure:
Result
Closes #848