1.2 fix migration#1368
Closed
okraits wants to merge 17 commits into
Closed
Conversation
Bug: The `Vpn.auto_client` method was incorrectly using the Vpn.host field when generating the client configuration for the OpenVPN backend. This resulted in the `remote` directive being hardcoded in the `Template.config`, instead of being rendered from the provided context variables. Fix: The `Vpn.auto_client` method has been updated to use the correct context-based values. [backport 1.2] (cherry picked from commit dbf1a05)
The first call to vpn.save() was generting a real HTTP request, which failed in the CI of ansible-openwisp2. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit 582e6a7) # Conflicts: # openwisp_controller/config/tests/test_vpn.py
The previous help texts were confusing and too technical. [backport 1.2] (cherry picked from commit c540d38)
Fixed a bug in the configuration update task that caused Celery workers to incorrectly skip execution. The task would sometimes falsely detect a collision with another worker, leading to missed configuration updates. [backport 1.2] Fixes #1204 Co-authored-by: Piyush Bafna <130243298+piyushdev04@users.noreply.github.com> (cherry picked from commit 45b24b6)
Bug: Previously, the `post_provision_handler` in BaseSubnetDivisionRuleType did not update the associated Config checksum after subnets or IP addresses were provisioned. This could lead to inconsistent state where the Config's checksum and checksum_db diverged, especially when VPN templates were switched or updated, causing change detection and dependent logic to fail. Fix: - Converted `post_provision_handler` to a classmethod so subclasses can extend it safely. - Invalidate the backend instance cache for the Config. - Update `checksum_db` if the current checksum differs. - In VPNSubnetDivisionRuleType, call the superclass handler to ensure checksum and cache are updated automatically when a VPN template is provisioned. [backport 1.2] (cherry picked from commit 76e1527)
(cherry picked from commit af0d99b)
Bug: The JS logic in relevant_templates.js assumed that the last `ul.sortedm2m-items` element belonged to the empty inline form used by Django admin formsets. This assumption breaks when the user does not have permission to add Config objects: in that case the ConfigInlineAdmin does not render the empty form. As a result, both selectors ended up referencing the same list and the script appended template elements twice to the same `sortedm2m` list, causing duplicate entries and issues when saving the form. Fix: Select the empty form container explicitly using `#config-empty ul.sortedm2m-items` instead of relying on the last occurrence of `ul.sortedm2m-items`. This ensures the logic works correctly regardless of whether the empty inline form is rendered. [backport 1.2] (cherry picked from commit b73e83a)
Fixed incorrect initialization of `_initial_<field>` values when a Device instance is loaded with deferred fields, which could break change detection logic. [backport 1.2]
We need to use the historical model for the data migration, otherwise the migration will fail. See https://docs.djangoproject.com/en/6.0/topics/migrations/#data-migrations [backport 1.2] Signed-off-by: Oliver Kraitschy <ok@dev.tdt.de>
Contributor
📝 WalkthroughWalkthroughThis PR is a v1.2.3 patch release containing five critical bug fixes, corresponding test coverage, and field documentation improvements across the codebase. It addresses Config admin form validation when organization data is missing, restores deferred Device fields correctly after database refresh, invalidates Config checksums after subnet provisioning, prevents concurrent update_config tasks from self-blocking via Celery task binding, and fixes Device admin template list duplication via DOM selector correction. Additionally, it clarifies field help text across multiple models to better document intent for configuration variables and metadata, updates all related migrations, and includes integration tests validating the fixes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This is a backport PR from the master branch to the 1.2 branch containing bug fixes. OverviewThe PR addresses several issues:
Regression TestsAll bug fixes include appropriate regression tests:
Files Reviewed (32 files)Key files reviewed:
All changes follow the existing codebase patterns and the fixes are correctly implemented. The migration fix is particularly important as it ensures data migrations use historical models rather than current model definitions. Reviewed by kimi-k2.5-0127 · 528,744 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGES.rst`:
- Around line 4-5: Update the release date for "Version 1.2.3" in CHANGES.rst
(the line containing "Version 1.2.3 [2026-04-09]") to reflect the correct
intent: either change the bracketed date to the actual release date (e.g.,
current date), annotate it as an "anticipated" or "backdated" entry by adding a
short parenthetical note, or add a one-line comment below the heading clarifying
that the entry was written earlier than the release; pick the appropriate
approach and make the single-line change so the date or note unambiguously
communicates whether it was backdated, pre-written, or should be updated to the
real release date.
In `@openwisp_controller/config/tests/test_selenium.py`:
- Around line 459-464: The test currently looks up checkboxes by positional IDs
(id_config-templates_{idx}) which couples assertions to render order; instead,
for each template (template1.id, template2.id) use a value-based lookup (e.g.,
find_element with a selector targeting
input[type="checkbox"][value="{template_id}"] or an XPath that matches the
input's value) and then assert checkbox.get_attribute("data-required") ==
"false" and checkbox.get_attribute("value") == str(template_id); update usages
of find_element/By.ID and remove reliance on id_config-templates_{idx} so each
checkbox is located directly by its value attribute.
In `@openwisp_controller/config/tests/test_vpn.py`:
- Around line 834-842: The test creates a mock requests.Response named
success_response but the patch for requests.post still returns HttpResponse(),
leaving the prepared mock unused; update the mock.patch that targets
"requests.post" to use return_value=success_response (ensuring
success_response.status_code and raise_for_status are used) and remove the
now-redundant HttpResponse() setup so the success branch exercises the
requests.Response mock instead.
In `@openwisp_controller/geo/tests/test_api.py`:
- Around line 1041-1056: The test test_device_location_view_parent_permission is
currently getting a 404 because device1 has no DeviceLocation, so update the
test to create a DeviceLocation linked to device1 before calling
reverse("geo_api:device_location", args=[device1.pk]) to ensure the 404 (or
allowed access) is due to permission filtering rather than a missing object;
specifically instantiate the related DeviceLocation for device1 (using the
existing factory/helper used elsewhere in tests) and then perform the GET as
manager_org2 to validate parent-org permission behavior.
🪄 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: aaa24ae8-879b-4b40-a089-7be0f1eea35e
📒 Files selected for processing (32)
.github/workflows/ci.ymlCHANGES.rstopenwisp_controller/__init__.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/tests/test_admin.pytests/openwisp2/sample_config/migrations/0001_initial.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). (1)
- GitHub Check: Kilo Code Review
🧰 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/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/__init__.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/config.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0001_initial.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/subnet_division/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/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/__init__.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/config.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0001_initial.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/subnet_division/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/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/__init__.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/config.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0001_initial.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/subnet_division/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/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/__init__.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/config.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0001_initial.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/subnet_division/tests/test_admin.py
🧠 Learnings (6)
📚 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/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/__init__.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/config.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/base/template.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/subnet_division/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/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/__init__.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/config.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/base/template.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/subnet_division/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/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/__init__.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/config.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0001_initial.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/subnet_division/tests/test_admin.py
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-01-12T22:27:40.078Z
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:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0001_initial.py
🔇 Additional comments (36)
.github/workflows/ci.yml (1)
8-8: LGTM!Also applies to: 13-13, 77-77
openwisp_controller/config/base/device.py (1)
328-330: LGTM!openwisp_controller/config/tests/test_device.py (1)
532-545: LGTM!openwisp_controller/subnet_division/rule_types/base.py (1)
82-111: LGTM!openwisp_controller/subnet_division/rule_types/vpn.py (1)
44-46: LGTM!openwisp_controller/subnet_division/tests/test_admin.py (2)
3-18: LGTM!
267-393: LGTM!openwisp_controller/config/static/config/js/relevant_templates.js (1)
141-141: LGTM!openwisp_controller/config/base/config.py (1)
93-104: LGTM!openwisp_controller/config/base/template.py (1)
95-106: LGTM!openwisp_controller/config/base/device_group.py (1)
39-60: LGTM!openwisp_controller/config/base/multitenancy.py (1)
34-44: LGTM!openwisp_controller/config/migrations/0023_update_context.py (1)
13-29: LGTM!openwisp_controller/config/migrations/0028_template_default_values.py (1)
13-29: LGTM!openwisp_controller/config/migrations/0036_device_group.py (1)
58-73: LGTM!openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
14-30: LGTM!openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py (1)
23-24: LGTM!openwisp_controller/config/migrations/0061_config_checksum_db.py (1)
15-15: LGTM!tests/openwisp2/sample_config/migrations/0001_initial.py (5)
124-128: LGTM!
583-587: LGTM!
714-716: LGTM!
770-775: LGTM!
786-788: LGTM!openwisp_controller/config/base/vpn.py (1)
651-654: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
6-6: LGTM!Also applies to: 12-12, 202-205, 235-238, 843-853, 854-861
openwisp_controller/connection/tasks.py (1)
19-33: LGTM!Also applies to: 36-37, 56-56
openwisp_controller/connection/tests/test_models.py (1)
4-4: LGTM!Also applies to: 1030-1080, 1093-1101
openwisp_controller/connection/tests/test_tasks.py (1)
22-71: LGTM!openwisp_controller/config/admin.py (1)
355-362: LGTM!Also applies to: 375-380
openwisp_controller/config/tests/test_admin.py (1)
2282-2310: LGTM!openwisp_controller/geo/api/views.py (1)
118-118: LGTM!openwisp_controller/geo/tests/test_admin.py (1)
32-36: LGTM!openwisp_controller/geo/tests/test_api.py (1)
12-12: LGTM!openwisp_controller/geo/tests/test_selenium.py (1)
94-98: LGTM!openwisp_controller/__init__.py (1)
1-1: LGTM!CHANGES.rst (1)
7-14: Potential inconsistency between AI summary and changelog.The AI-generated summary states this PR includes "five critical bug fixes," but the changelog lists only 4 bugfix entries:
- Corrected initial field value assignment in AbstractDevice class
- Fixed duplicate template entries in Device admin
- Invalidate Config checksum after subnet provisioning
- Fixed concurrent update_config detection
Based on the review stack context, layer 2 (Config admin form validation for missing organization) appears to address the MultiValueDictKeyError issue mentioned in version 1.2.1 (line 37-38). Verify whether this fix should be listed in the 1.2.3 changelog or if it's already adequately documented in 1.2.1.
Comment on lines
+4
to
+5
| Version 1.2.3 [2026-04-09] | ||
| -------------------------- |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the CHANGES.rst file to see the actual content
cat -n CHANGES.rst | head -30Repository: openwisp/openwisp-controller
Length of output: 918
🏁 Script executed:
# Check git log to understand PR creation date and commit details
git log --oneline -20Repository: openwisp/openwisp-controller
Length of output: 132
🏁 Script executed:
# Check if there's git metadata that shows when this was created
git log --all --date=short --format='%h %ad %s' | head -20Repository: openwisp/openwisp-controller
Length of output: 1648
Clarify the release date for version 1.2.3.
The changelog shows version 1.2.3 dated [2026-04-09], but recent commits in the repository are dated 2026-05-19 (approximately 6 weeks later). Please confirm whether this date reflects:
- An intentional backdated release for historical record-keeping
- An advance changelog entry written before the actual release
- The actual release date that should be updated to the current date
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGES.rst` around lines 4 - 5, Update the release date for "Version 1.2.3"
in CHANGES.rst (the line containing "Version 1.2.3 [2026-04-09]") to reflect the
correct intent: either change the bracketed date to the actual release date
(e.g., current date), annotate it as an "anticipated" or "backdated" entry by
adding a short parenthetical note, or add a one-line comment below the heading
clarifying that the entry was written earlier than the release; pick the
appropriate approach and make the single-line change so the date or note
unambiguously communicates whether it was backdated, pre-written, or should be
updated to the real release date.
Comment on lines
+459
to
+464
| for idx, template_id in enumerate([template1.id, template2.id]): | ||
| checkbox = self.find_element( | ||
| by=By.ID, value=f"id_config-templates_{idx}" | ||
| ) | ||
| self.assertEqual(checkbox.get_attribute("value"), str(template_id)) | ||
| self.assertEqual(checkbox.get_attribute("data-required"), "false") |
Contributor
There was a problem hiding this comment.
Avoid order-coupled assertions for checkbox lookup.
Line 459 ties expected template IDs to positional IDs (id_config-templates_{idx}), which can become flaky if template render order changes. Prefer selecting each checkbox by value and asserting attributes on that element.
Suggested stable assertion
- for idx, template_id in enumerate([template1.id, template2.id]):
- checkbox = self.find_element(
- by=By.ID, value=f"id_config-templates_{idx}"
- )
+ for template_id in [template1.id, template2.id]:
+ checkbox = self.find_element(
+ by=By.CSS_SELECTOR,
+ value=f'input[type="checkbox"][value="{template_id}"]',
+ )
self.assertEqual(checkbox.get_attribute("value"), str(template_id))
self.assertEqual(checkbox.get_attribute("data-required"), "false")📝 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.
Suggested change
| for idx, template_id in enumerate([template1.id, template2.id]): | |
| checkbox = self.find_element( | |
| by=By.ID, value=f"id_config-templates_{idx}" | |
| ) | |
| self.assertEqual(checkbox.get_attribute("value"), str(template_id)) | |
| self.assertEqual(checkbox.get_attribute("data-required"), "false") | |
| for template_id in [template1.id, template2.id]: | |
| checkbox = self.find_element( | |
| by=By.CSS_SELECTOR, | |
| value=f'input[type="checkbox"][value="{template_id}"]', | |
| ) | |
| self.assertEqual(checkbox.get_attribute("value"), str(template_id)) | |
| self.assertEqual(checkbox.get_attribute("data-required"), "false") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_controller/config/tests/test_selenium.py` around lines 459 - 464,
The test currently looks up checkboxes by positional IDs
(id_config-templates_{idx}) which couples assertions to render order; instead,
for each template (template1.id, template2.id) use a value-based lookup (e.g.,
find_element with a selector targeting
input[type="checkbox"][value="{template_id}"] or an XPath that matches the
input's value) and then assert checkbox.get_attribute("data-required") ==
"false" and checkbox.get_attribute("value") == str(template_id); update usages
of find_element/By.ID and remove reliance on id_config-templates_{idx} so each
checkbox is located directly by its value attribute.
Comment on lines
+834
to
842
| success_response = mock.Mock(spec=requests.Response) | ||
| success_response.status_code = 200 | ||
| success_response.raise_for_status = mock.Mock() | ||
|
|
||
| with mock.patch( | ||
| "openwisp_controller.config.tasks.logger.info" | ||
| ) as mocked_logger, mock.patch( | ||
| "requests.post", return_value=HttpResponse() | ||
| ): |
Contributor
There was a problem hiding this comment.
Use the prepared requests.Response mock in the success path (and remove dead setup).
Line 834 creates success_response, but Line 841 still returns HttpResponse(). This leaves redundant code and keeps the success branch on a non-requests response type.
Proposed fix
- with mock.patch(
- "openwisp_controller.config.tasks.logger.info"
- ) as mocked_logger, mock.patch(
- "requests.post", return_value=HttpResponse()
- ):
+ with mock.patch(
+ "openwisp_controller.config.tasks.logger.info"
+ ) as mocked_logger, mock.patch(
+ "requests.post", return_value=success_response
+ ):As per coding guidelines: "Flag unused or redundant code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_controller/config/tests/test_vpn.py` around lines 834 - 842, The
test creates a mock requests.Response named success_response but the patch for
requests.post still returns HttpResponse(), leaving the prepared mock unused;
update the mock.patch that targets "requests.post" to use
return_value=success_response (ensuring success_response.status_code and
raise_for_status are used) and remove the now-redundant HttpResponse() setup so
the success branch exercises the requests.Response mock instead.
Comment on lines
+1041
to
+1056
| def test_device_location_view_parent_permission(self): | ||
| org1 = self._create_org(name="Org One") | ||
| device1 = self._create_device(organization=org1) | ||
| org2 = self._create_org(name="Org Two") | ||
| manager_org2 = self._create_administrator( | ||
| organizations=[org2], | ||
| username="manager_org2", | ||
| password="test_password", | ||
| is_superuser=False, | ||
| is_staff=True, | ||
| ) | ||
| self.client.force_login(manager_org2) | ||
| url = reverse("geo_api:device_location", args=[device1.pk]) | ||
| response = self.client.get(url) | ||
| self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) | ||
| self.client.logout() |
Contributor
There was a problem hiding this comment.
Test does not exercise the permission path it claims to validate.
device1 has no DeviceLocation, so the 404 can come from a missing object rather than parent-org permission filtering. Create the related DeviceLocation first to make this a real authorization regression test.
✅ Minimal fix
def test_device_location_view_parent_permission(self):
org1 = self._create_org(name="Org One")
device1 = self._create_device(organization=org1)
+ location1 = self._create_location(organization=org1)
+ self._create_device_location(content_object=device1, location=location1)
org2 = self._create_org(name="Org Two")
manager_org2 = self._create_administrator(
organizations=[org2],📝 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.
Suggested change
| def test_device_location_view_parent_permission(self): | |
| org1 = self._create_org(name="Org One") | |
| device1 = self._create_device(organization=org1) | |
| org2 = self._create_org(name="Org Two") | |
| manager_org2 = self._create_administrator( | |
| organizations=[org2], | |
| username="manager_org2", | |
| password="test_password", | |
| is_superuser=False, | |
| is_staff=True, | |
| ) | |
| self.client.force_login(manager_org2) | |
| url = reverse("geo_api:device_location", args=[device1.pk]) | |
| response = self.client.get(url) | |
| self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) | |
| self.client.logout() | |
| def test_device_location_view_parent_permission(self): | |
| org1 = self._create_org(name="Org One") | |
| device1 = self._create_device(organization=org1) | |
| location1 = self._create_location(organization=org1) | |
| self._create_device_location(content_object=device1, location=location1) | |
| org2 = self._create_org(name="Org Two") | |
| manager_org2 = self._create_administrator( | |
| organizations=[org2], | |
| username="manager_org2", | |
| password="test_password", | |
| is_superuser=False, | |
| is_staff=True, | |
| ) | |
| self.client.force_login(manager_org2) | |
| url = reverse("geo_api:device_location", args=[device1.pk]) | |
| response = self.client.get(url) | |
| self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) | |
| self.client.logout() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_controller/geo/tests/test_api.py` around lines 1041 - 1056, The test
test_device_location_view_parent_permission is currently getting a 404 because
device1 has no DeviceLocation, so update the test to create a DeviceLocation
linked to device1 before calling reverse("geo_api:device_location",
args=[device1.pk]) to ensure the 404 (or allowed access) is due to permission
filtering rather than a missing object; specifically instantiate the related
DeviceLocation for device1 (using the existing factory/helper used elsewhere in
tests) and then perform the GET as manager_org2 to validate parent-org
permission behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Description of Changes
We need to use the historical model for the data migration, otherwise
the migration will fail.
See https://docs.djangoproject.com/en/6.0/topics/migrations/#data-migrations
Backport of an unrelated fix in 703f288#diff-98cce51566ddba844a98687e46b38dab90bc90650372525bc2a1cc590db9c2d5 for 1.2.