[change] Added generic enforcement of deactivated devices #1338#1365
Draft
pandafy wants to merge 2 commits into
Draft
[change] Added generic enforcement of deactivated devices #1338#1365pandafy wants to merge 2 commits into
pandafy wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR systematically enforces deactivation checks across the OpenWISP controller. It adds AbstractDevice.is_fully_deactivated() and applies early-return or rejection guards so deactivated or deactivating devices/configs are skipped in bulk flows (templates, WHOIS, subnet provisioning, background tasks) or rejected in explicit operations (registration, connect, command execution). Tests and test utilities were added/adjusted to validate the behavior. Sequence Diagram(s)sequenceDiagram
participant DeviceRegisterView
participant Device
participant Config
DeviceRegisterView->>Device: select_related("config").get(key)
Device->>Device: is_deactivated() / is_fully_deactivated()
alt deactivated
DeviceRegisterView->>DeviceRegisterView: return 403 "error: device deactivated"
else operable
DeviceRegisterView->>Config: proceed with registration updates
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3 tasks
![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: 5
🤖 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 `@openwisp_controller/config/base/device_group.py`:
- Line 113: The loop currently iterates over
device_group.device_set.exclude(_is_deactivated=True).iterator() which misses
other non-operable states; update the query on device_group.device_set to use
the project's canonical operability filter/manager (e.g., the operable()
queryset or the is_operable/is_active boolean field or provided manager method)
instead of only excluding _is_deactivated, so the iterator only yields truly
operable devices (replace the exclude(...) call with the canonical operability
filter and keep the iterator()).
In `@openwisp_controller/config/controller/views.py`:
- Around line 400-402: The guard currently checks device.is_deactivated() which
allows config-level deactivations to be bypassed; change the condition to call
the canonical strict helper device.is_fully_deactivated() instead so the
ControllerResponse("error: device deactivated", status=403) is returned for
fully-deactivated devices; locate this in the view where device =
self.model.objects.select_related("config").get(key=key) and replace the
is_deactivated() call with is_fully_deactivated() (no other behavior changes
needed).
In `@openwisp_controller/connection/base/models.py`:
- Around line 349-350: The hardcoded error message in the RuntimeError raised
after the self.device.is_fully_deactivated() check must be marked for
translation; import Django's i18n helper (e.g. from django.utils.translation
import gettext_lazy as _) in openwisp_controller/connection/base/models.py and
replace RuntimeError("Device is deactivated") with RuntimeError(_("Device is
deactivated")) so the failure_reason stored can be translated when surfaced.
- Around line 470-471: Replace the call to self.device.is_deactivated() with the
stricter self.device.is_fully_deactivated() to match other deactivation checks
(e.g., the ones around is_fully_deactivated() used elsewhere in this class), and
add a defensive guard to ensure self.device is not None before calling the
method so the validation block handles cases where device isn't set yet; update
the ValidationError to remain the same field ("device") and message if the
fully-deactivated check returns True.
In `@openwisp_controller/geo/estimated_location/handlers.py`:
- Around line 71-75: The condition in the handler uses the method object
device.is_deactivated instead of calling it, causing the if to always be truthy;
update the condition to invoke the method (device.is_deactivated()) so it
behaves like the other callsites, and keep the rest of the check
(EstimatedLocationService.check_estimated_location_enabled(device.organization_id))
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7694feac-56b3-419e-bd3e-36f00e30d24b
📒 Files selected for processing (12)
openwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Analyze (javascript-typescript)
🧰 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/connection/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/geo/estimated_location/handlers.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/connection/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/geo/estimated_location/handlers.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/connection/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/geo/estimated_location/handlers.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/connection/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/geo/estimated_location/handlers.py
🧠 Learnings (4)
📚 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/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/geo/estimated_location/handlers.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/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/geo/estimated_location/handlers.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/connection/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/geo/estimated_location/handlers.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/handlers.py
🔇 Additional comments (10)
openwisp_controller/connection/tests/test_api.py (1)
290-290: LGTM!openwisp_controller/config/base/device.py (1)
188-191: LGTM!Also applies to: 307-307, 484-484
openwisp_controller/config/base/config.py (1)
999-1000: LGTM!openwisp_controller/config/whois/service.py (1)
213-214: LGTM!openwisp_controller/config/whois/tasks.py (1)
75-77: LGTM!Also applies to: 120-121
openwisp_controller/geo/estimated_location/tasks.py (1)
124-128: LGTM!openwisp_controller/geo/estimated_location/handlers.py (1)
46-46: LGTM!openwisp_controller/connection/base/models.py (1)
144-145: LGTM!Also applies to: 271-274, 384-386
openwisp_controller/connection/tasks.py (1)
49-51: LGTM!openwisp_controller/connection/apps.py (1)
92-93: LGTM!
| templates = Template.objects.filter(pk__in=template_ids) | ||
| old_templates = Template.objects.filter(pk__in=old_template_ids) | ||
| for device in device_group.device_set.iterator(): | ||
| for device in device_group.device_set.exclude(_is_deactivated=True).iterator(): |
Contributor
There was a problem hiding this comment.
Use the canonical operability criteria for device filtering.
At Line 113, filtering only on _is_deactivated=True can still include devices whose config is already non-operable (for example, deactivating/deactivated), so group template updates may run on devices that should be skipped.
Suggested change
- for device in device_group.device_set.exclude(_is_deactivated=True).iterator():
+ for device in (
+ device_group.device_set.exclude(_is_deactivated=True)
+ .exclude(config__status__in=["deactivating", "deactivated"])
+ .iterator()
+ ):🤖 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/base/device_group.py` at line 113, The loop
currently iterates over
device_group.device_set.exclude(_is_deactivated=True).iterator() which misses
other non-operable states; update the query on device_group.device_set to use
the project's canonical operability filter/manager (e.g., the operable()
queryset or the is_operable/is_active boolean field or provided manager method)
instead of only excluding _is_deactivated, so the iterator only yields truly
operable devices (replace the exclude(...) call with the canonical operability
filter and keep the iterator()).
Comment on lines
+400
to
+402
| device = self.model.objects.select_related("config").get(key=key) | ||
| if device.is_deactivated(): | ||
| return ControllerResponse("error: device deactivated", status=403) |
Contributor
There was a problem hiding this comment.
Use the canonical strict deactivation helper here
At Line 401, this guard should call the shared strict helper (is_fully_deactivated()) rather than is_deactivated() to keep behavior aligned with the stack’s canonical enforcement contract and avoid allowing re-registration on config-level deactivation paths.
Suggested fix
- if device.is_deactivated():
+ if device.is_fully_deactivated():
return ControllerResponse("error: device deactivated", status=403)🤖 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/controller/views.py` around lines 400 - 402, The
guard currently checks device.is_deactivated() which allows config-level
deactivations to be bypassed; change the condition to call the canonical strict
helper device.is_fully_deactivated() instead so the ControllerResponse("error:
device deactivated", status=403) is returned for fully-deactivated devices;
locate this in the view where device =
self.model.objects.select_related("config").get(key=key) and replace the
is_deactivated() call with is_fully_deactivated() (no other behavior changes
needed).
Comment on lines
+349
to
+350
| if self.device.is_fully_deactivated(): | ||
| raise RuntimeError("Device is deactivated") |
Contributor
There was a problem hiding this comment.
Make the deactivation failure message translatable.
At Line 350, the new string is stored in failure_reason and can surface in notifications; mark it for i18n.
Proposed fix
- raise RuntimeError("Device is deactivated")
+ raise RuntimeError(gettext("Device is deactivated."))As per coding guidelines: "For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework".
🤖 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/connection/base/models.py` around lines 349 - 350, The
hardcoded error message in the RuntimeError raised after the
self.device.is_fully_deactivated() check must be marked for translation; import
Django's i18n helper (e.g. from django.utils.translation import gettext_lazy as
_) in openwisp_controller/connection/base/models.py and replace
RuntimeError("Device is deactivated") with RuntimeError(_("Device is
deactivated")) so the failure_reason stored can be translated when surfaced.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
pandafy
force-pushed
the
issues/1338-generic-deactivated-device
branch
from
635306e to
dccd050
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openwisp_controller/connection/base/models.py (2)
360-364:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark the deactivation error message as translatable.
The error message at line 364 will be stored in
failure_reason(line 368) and surfaced to users in notifications. It must be translatable. Additionally, add a period for consistency with the error message at line 485.🌐 Proposed fix
- if self.device.is_fully_deactivated(): - raise RuntimeError("Device is deactivated") + if self.device.is_fully_deactivated(): + raise RuntimeError(gettext("Device is deactivated."))As per coding guidelines: "For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework".
🤖 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/connection/base/models.py` around lines 360 - 364, The RuntimeError message raised in the is_fully_deactivated check is not marked for translation and lacks a trailing period; import Django's i18n (e.g. from django.utils.translation import gettext_lazy as _) and replace the literal string in the RuntimeError raised in the method containing self.device.is_fully_deactivated() with a translatable string including a period, e.g. RuntimeError(_("Device is deactivated.")), ensuring the unique symbols self.device.is_fully_deactivated and the RuntimeError raise are updated accordingly.
484-485:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
is_fully_deactivated()for consistency and add defensive check.Line 484 uses
is_deactivated(), while the same class usesis_fully_deactivated()at lines 363 and 398. For consistency with the operability contract elsewhere in this file, use the stricteris_fully_deactivated()check.Additionally, while the
devicefield is required (nonull=Trueat line 416), adding a defensivedevice_idcheck prevents potentialAttributeErrorifclean()is called before the device is fully set during form validation.♻️ Proposed fix
def clean(self): - if self.device.is_deactivated(): + if self.device_id and self.device.is_fully_deactivated(): raise ValidationError({"device": _("Device is deactivated.")})🤖 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/connection/base/models.py` around lines 484 - 485, Replace the use of is_deactivated() with is_fully_deactivated() to match the class's operability contract and add a defensive check for device_id before calling any device methods inside the clean() method: if self.device_id is falsy, skip the deactivation check; otherwise call self.device.is_fully_deactivated() and raise ValidationError({"device": _("Device is deactivated.")}) when true. Update the clause that currently references is_deactivated() accordingly to use is_fully_deactivated() and guard with a device_id existence check.
🤖 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 `@openwisp_controller/geo/tests/utils.py`:
- Line 14: Replace the inefficient count-based existence check with .exists():
change the condition in the utility where
Organization.objects.filter(**options).count() is used to use
Organization.objects.filter(**options).exists() instead (i.e., update the line
starting with if not Organization.objects.filter(**options).count(): to if not
Organization.objects.filter(**options).exists():), keeping the same surrounding
logic and using the same variables (Organization, options).
In `@openwisp_controller/subnet_division/rule_types/device.py`:
- Around line 57-60: The filter call using the lookup device___is_deactivated is
correct but non-obvious; update the iterator/filter in the code that builds the
queryset (the call with
.filter(device__organization_id=rule_obj.organization_id,
device___is_deactivated=False).iterator()) to add a concise inline comment
explaining the triple underscore: the pattern is device__{related_field_name}
where the related field itself is named _is_deactivated, so the extra underscore
is part of the field name; place the comment immediately next to
device___is_deactivated to prevent future regressions.
---
Duplicate comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 360-364: The RuntimeError message raised in the
is_fully_deactivated check is not marked for translation and lacks a trailing
period; import Django's i18n (e.g. from django.utils.translation import
gettext_lazy as _) and replace the literal string in the RuntimeError raised in
the method containing self.device.is_fully_deactivated() with a translatable
string including a period, e.g. RuntimeError(_("Device is deactivated.")),
ensuring the unique symbols self.device.is_fully_deactivated and the
RuntimeError raise are updated accordingly.
- Around line 484-485: Replace the use of is_deactivated() with
is_fully_deactivated() to match the class's operability contract and add a
defensive check for device_id before calling any device methods inside the
clean() method: if self.device_id is falsy, skip the deactivation check;
otherwise call self.device.is_fully_deactivated() and raise
ValidationError({"device": _("Device is deactivated.")}) when true. Update the
clause that currently references is_deactivated() accordingly to use
is_fully_deactivated() and guard with a device_id existence check.
🪄 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: 4794e788-09aa-4fad-bdce-7f5720a509f0
📒 Files selected for processing (18)
openwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/connection/apps.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/base/device_group.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/connection/apps.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/base/device_group.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/connection/apps.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/base/device_group.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/connection/apps.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/base/device_group.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/connection/apps.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (4)
📚 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/base/device_group.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/connection/apps.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/base/device_group.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/connection/apps.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/base/device_group.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/geo/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/connection/apps.pyopenwisp_controller/subnet_division/rule_types/device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tasks.py
🔇 Additional comments (19)
openwisp_controller/config/base/device_group.py (1)
113-113: Use the canonical operability criteria for device filtering.The current filter only excludes devices where
_is_deactivated=True, but does not account for devices whose config status isdeactivatingordeactivated. This means group template updates may still attempt to process devices that are not fully operable.Based on the past review comment, consider excluding devices with non-operable config states as well.
openwisp_controller/config/whois/service.py (1)
213-214: LGTM!openwisp_controller/connection/tasks.py (1)
49-51: LGTM!Also applies to: 82-89
openwisp_controller/geo/tests/utils.py (1)
47-50: LGTM!openwisp_controller/connection/tests/test_tasks.py (1)
166-183: LGTM!openwisp_controller/geo/estimated_location/tasks.py (1)
124-128: LGTM!openwisp_controller/config/tests/test_device.py (1)
515-531: LGTM!openwisp_controller/connection/tests/test_models.py (1)
352-360: LGTM!Also applies to: 1176-1196
openwisp_controller/geo/estimated_location/handlers.py (1)
46-49: LGTM!Also applies to: 71-77
openwisp_controller/config/whois/tasks.py (1)
75-77: LGTM!Also applies to: 120-126
openwisp_controller/config/controller/views.py (1)
124-126: LGTM!Also applies to: 137-139, 402-404
openwisp_controller/subnet_division/tests/test_models.py (1)
818-844: LGTM!openwisp_controller/connection/apps.py (1)
92-93: LGTM!openwisp_controller/subnet_division/rule_types/device.py (1)
36-42: LGTM!openwisp_controller/config/base/config.py (1)
485-490: LGTM!Also applies to: 1004-1005
openwisp_controller/config/base/device.py (1)
188-191: LGTM!Also applies to: 307-308, 484-485, 504-508
openwisp_controller/connection/tests/test_api.py (1)
290-290: LGTM!openwisp_controller/connection/base/models.py (2)
144-147: LGTM!Also applies to: 186-188, 276-280
387-388: LGTM!Also applies to: 398-399
| options.update(kwargs) | ||
| options.setdefault("slug", slugify(options["name"])) | ||
| if not Organization.objects.filter(**kwargs).count(): | ||
| if not Organization.objects.filter(**options).count(): |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Prefer .exists() over .count() for boolean checks.
Line 14 uses .count() to check if an organization exists. The .exists() method is more efficient as it stops at the first match rather than counting all matching rows.
⚡ Suggested refactor
- if not Organization.objects.filter(**options).count():
+ if not Organization.objects.filter(**options).exists():📝 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
| if not Organization.objects.filter(**options).count(): | |
| if not Organization.objects.filter(**options).exists(): |
🤖 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/utils.py` at line 14, Replace the inefficient
count-based existence check with .exists(): change the condition in the utility
where Organization.objects.filter(**options).count() is used to use
Organization.objects.filter(**options).exists() instead (i.e., update the line
starting with if not Organization.objects.filter(**options).count(): to if not
Organization.objects.filter(**options).exists():), keeping the same surrounding
logic and using the same variables (Organization, options).
Comment on lines
+57
to
+60
| .filter( | ||
| device__organization_id=rule_obj.organization_id, | ||
| device___is_deactivated=False, | ||
| ).iterator() |
Contributor
There was a problem hiding this comment.
Clarify the triple-underscore ORM lookup at Line 59.
device___is_deactivated is valid but non-obvious. Add a short inline note that the third underscore comes from the _is_deactivated field name to prevent accidental regressions.
Suggested patch
.filter(
device__organization_id=rule_obj.organization_id,
+ # Triple underscore is intentional:
+ # `device__` relation lookup + `_is_deactivated` field name.
device___is_deactivated=False,
).iterator()🤖 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/subnet_division/rule_types/device.py` around lines 57 -
60, The filter call using the lookup device___is_deactivated is correct but
non-obvious; update the iterator/filter in the code that builds the queryset
(the call with .filter(device__organization_id=rule_obj.organization_id,
device___is_deactivated=False).iterator()) to add a concise inline comment
explaining the triple underscore: the pattern is device__{related_field_name}
where the related field itself is named _is_deactivated, so the extra underscore
is part of the field name; place the comment immediately next to
device___is_deactivated to prevent future regressions.
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
1 participant
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
Reference to Existing Issue
Closes #1338
Description of Changes
Added generic enforcement of deactivated device