[fix] Skip WHOIS and estimated location for deactivated devices #1325#1340
Conversation
…openwisp#1325 This commit adds regression tests to ensure that the WHOIS handling for deactivated devices works as expected.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds deactivation guards to prevent WHOIS and estimated-location features from triggering on deactivated devices. It introduces early-return checks in service methods ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tomsebastian10
changed the title
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| Trigger WHOIS lookup based on the conditions of `_need_whois_lookup`. | ||
| Tasks are triggered on commit to ensure redundant data is not created. | ||
| """ | ||
| if self.device.is_deactived(): |
There was a problem hiding this comment.
CRITICAL: Method name typo - is_deactived() should be is_deactivated() (missing 'a'). This will cause an AttributeError at runtime since the method doesn't exist.
| if self.device.is_deactived(): | |
| if self.device.is_deactivated(): |
| when the data is older than | ||
| ``OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS``. | ||
| """ | ||
| if self.device.is_deactived(): |
There was a problem hiding this comment.
CRITICAL: Method name typo - is_deactived() should be is_deactivated() (missing 'a'). This will cause an AttributeError at runtime.
| if self.device.is_deactived(): | |
| if self.device.is_deactivated(): |
| from datetime import timedelta | ||
| from io import StringIO | ||
| from unittest import mock | ||
| from unittest import mock, mocke |
There was a problem hiding this comment.
TYPO: mocke should be removed - this is an unused import typo.
| from unittest import mock, mocke | |
| from unittest import mock |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Changes ReviewedSince commit
VerificationThese incremental changes provide defense-in-depth for issue #1325:
All modified methods now properly guard against processing deactivated devices. Files Reviewed (2 files)
Reviewed by kimi-k2.5 · 120,072 tokens |
…openwisp#1325 This commit adds regression tests to ensure that the WHOIS handling for deactivated devices works as expected.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_controller/config/whois/tests/tests.py (1)
24-32:⚠️ Potential issue | 🟡 MinorDuplicate imports —
app_settingsandWHOISServiceare each imported twice.Lines 24/30 both import
app_settings, and Lines 26/32 both importWHOISService. Keep one — the existing relative imports on Lines 30 and 32 are consistent with the rest of the file, so the newly-added absolute-path imports on Lines 24 and 26 should be removed.🧹 Proposed fix
-from openwisp_controller.config import settings as app_settings from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped -from openwisp_controller.config.whois.service import WHOISService from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin from ... import settings as app_settings from ..handlers import connect_whois_handlers from ..service import WHOISService🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 24 - 32, Remove the duplicate absolute imports: delete the top-level imports "from openwisp_controller.config import settings as app_settings" and "from openwisp_controller.config.whois.service import WHOISService", keeping the existing relative imports "from ... import settings as app_settings" and "from ..service import WHOISService" (so only one app_settings and one WHOISService import remain consistent with the file).openwisp_controller/config/whois/service.py (1)
228-248:⚠️ Potential issue | 🟠 MajorMissing test coverage for the
update_whois_infoguard.The PR description and issue
#1325call out both WHOIS code paths, and this method has the same early-return. The new test class only exercisesprocess_ip_data_and_location. Please add a symmetric regression test that seeds a staleWHOISInforecord for the device'slast_ip, marks the device deactivated, callsupdate_whois_info(), and assertsfetch_whois_details.delayis not scheduled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/service.py` around lines 228 - 248, Add a regression test for WHOIS early-return: create a stale WHOISInfo record for the device.last_ip (modified older than OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS), set device.is_deactived() to True (or mark the device deactivated state used by the code), ensure device.last_ip is a valid public IP and is_whois_enabled is set appropriately, then call WHOISService.update_whois_info() and assert that fetch_whois_details.delay was NOT scheduled; locate logic in update_whois_info and WHOISService._get_whois_info_from_db to seed the DB and mock/assert calls to fetch_whois_details.delay.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@null`:
- Line 1: The PR contains a Git error message instead of code — replace the
erroneous file content with the actual implementation changes and include guard
checks in the two functions mentioned: in process_ip_data_and_location and in
update_whois_info add an early-return guard that checks the device's
active/deactivated state (e.g., device.is_active, device.status, or by loading
device by device_id and checking its active flag) and skip performing WHOIS
lookups when the device is deactivated; ensure these guards run before any
network/WHOIS calls and update tests/fixtures accordingly so the PR actually
contains the modified functions rather than the git error text.
In `@openwisp_controller/config/whois/service.py`:
- Around line 213-214: The guard is calling a non-existent method is_deactived()
which raises AttributeError and prevents WHOIS saves; replace both calls (in
process_ip_data_and_location and in update_whois_info) to use the correct Device
method is_deactivated() so the deactivated-device check actually runs; update
the occurrences of self.device.is_deactived() to self.device.is_deactivated() in
the methods process_ip_data_and_location and update_whois_info.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1199-1217: The active-path test test_process_ip_runs_when_active
is nondeterministic: enable the organization's whois flag explicitly (set
org.config_settings.whois_enabled = True and save) before invoking WHOISService
so the check doesn't rely on global WHOIS_CONFIGURED; also ensure the device
creation/IP change that triggers process_ip_data_and_location happens inside the
patched context (or create/change the device after applying the `@mock.patch` for
fetch_whois_details.delay) so the mock observes the call; for
test_process_ip_skips_when_deactivated either call self.device.deactivate() or
add a brief comment explaining why setting self.device._is_deactivated = True
in-memory is acceptable, and keep assertions against
fetch_whois_details.delay.call_count as-is.
- Line 5: The import line incorrectly includes a non-existent symbol `mocke`,
causing ImportError during test collection; update the import to only import the
valid `mock` symbol (e.g., change `from unittest import mock, mocke` to `from
unittest import mock`) so the test module can load; reference the incorrect
symbol `mocke` and the import statement to locate and correct the issue.
- Around line 1193-1198: TestWHOISDeactivated fails because it calls
self._create_device() but does not inherit CreateWHOISMixin; update the class
declaration to include CreateWHOISMixin alongside TransactionTestCase (mirror
TestWHOISTransaction's bases) so the _create_device helper is available, then
run the tests to verify setUp runs without AttributeError.
---
Outside diff comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 228-248: Add a regression test for WHOIS early-return: create a
stale WHOISInfo record for the device.last_ip (modified older than
OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS), set device.is_deactived() to
True (or mark the device deactivated state used by the code), ensure
device.last_ip is a valid public IP and is_whois_enabled is set appropriately,
then call WHOISService.update_whois_info() and assert that
fetch_whois_details.delay was NOT scheduled; locate logic in update_whois_info
and WHOISService._get_whois_info_from_db to seed the DB and mock/assert calls to
fetch_whois_details.delay.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 24-32: Remove the duplicate absolute imports: delete the top-level
imports "from openwisp_controller.config import settings as app_settings" and
"from openwisp_controller.config.whois.service import WHOISService", keeping the
existing relative imports "from ... import settings as app_settings" and "from
..service import WHOISService" (so only one app_settings and one WHOISService
import remain consistent with the file).
🪄 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: 4efda95c-48aa-4af4-a43f-55d36ba1b331
📒 Files selected for processing (3)
nullopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.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.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- 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/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
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.
📚 Learning: 2026-01-15T14:06:44.800Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:44.800Z
Learning: In openwisp_controller/geo/base/models.py, ensure that in BaseLocation.save(), when the estimated location feature is disabled, the is_estimated field is not overwritten by user input; instead, revert to its initial database value. This preserves the invariant that is_estimated can only change when the feature is enabled. Consider documenting this behavior and refactoring into a guard or helper to reduce future regressions.
Applied to files:
null
📚 Learning: 2026-03-17T09:20:04.547Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:04.547Z
Learning: In openwisp_controller/connection/tasks.py, the update_config Celery task is defined to take a single argument device_id, which is consistently passed as a string (via str(device.pk)) from openwisp_controller/connection/apps.py. Do not flag str(device_id) in task["args"] as unreliable due to typed arguments, because the task arguments are always strings. When reviewing, treat this as a specific, file-scoped guideline: do not raise issues about string conversion or type of the device_id argument for this task; if needed, add a note in the codebase explaining the argument is intentionally a single string ID and that the calling site ensures string conversion.
Applied to files:
null
📚 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/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (2)
openwisp_controller/config/whois/tests/tests.py (1)
351-352: LGTM on the query-count bump.Increase from 4 → 7 is consistent with the extra
_is_deactivated=Falsefilter now applied across theclear_last_ipcommand path (seeopenwisp_controller/config/whois/tasks.py:116-122).openwisp_controller/config/whois/service.py (1)
208-226: The early return for deactivated devices is intentional design—deactivated devices should not trigger WHOIS processing or signal emission. The pattern is consistent across multiple methods in WHOISService (bothprocess_ip_data_and_locationandupdate_whois_infohave identical early returns), and the existing test confirms this behavior is expected. Signal receivers should not expect notifications from deactivated devices as the intent is to suppress all related background tasks entirely.
Flake8 Fails on Unused ImportsHello @tomsebastian10, The CI build failed due to the following issues:
The test suite also encountered an issue, but it appears to be related to the code style failures. Please address the code style issues and push a new commit. |
…an10/openwisp-controller into fix-whois-deactivated
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/whois/tests/tests.py (1)
24-32: 🛠️ Refactor suggestion | 🟠 MajorDuplicate imports — remove the newly added absolute imports.
Lines 24 and 26 duplicate lines 30 and 32 (the relative imports
from ... import settings as app_settingsandfrom ..service import WHOISServiceresolve to the exact same symbols). The later relative imports shadow the new absolute ones, making lines 24 and 26 redundant.♻️ Proposed fix
-from openwisp_controller.config import settings as app_settings from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped -from openwisp_controller.config.whois.service import WHOISService from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin from ... import settings as app_settings from ..handlers import connect_whois_handlers from ..service import WHOISServiceAs per coding guidelines: "Flag unused or redundant code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 24 - 32, The test file imports app_settings and WHOISService twice: the absolute imports "from openwisp_controller.config import settings as app_settings" and "from openwisp_controller.config.whois.service import WHOISService" are duplicated and shadowed by the later relative imports "from ... import settings as app_settings" and "from ..service import WHOISService"; remove the earlier absolute import statements so only the relative imports for app_settings and WHOISService remain to eliminate redundancy.
♻️ Duplicate comments (1)
openwisp_controller/config/whois/tests/tests.py (1)
1205-1211: 🧹 Nitpick | 🔵 TrivialPrefer
deactivate()over in-memory flag flip for realism.Setting
self.device._is_deactivated = Truewithout persisting works only becauseWHOISService(self.device)below holds the same in-memory instance. Usingself.device.deactivate()exercises the real deactivation path (save + signal) and would also catch regressions where a caller reloads the device from DB before invoking the service.♻️ Proposed change
- def test_process_ip_skips_when_deactivated(self, mock_task): - self.device._is_deactivated = True - - service = WHOISService(self.device) - service.process_ip_data_and_location() + def test_process_ip_skips_when_deactivated(self, mock_task): + self.device.deactivate() + mock_task.reset_mock() # deactivate() may trigger save-side effects + + service = WHOISService(self.device) + service.process_ip_data_and_location()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 1205 - 1211, The test currently flips the private in-memory flag self.device._is_deactivated; instead call the public deactivation path by invoking self.device.deactivate() (which persists and emits signals) before instantiating WHOISService(self.device) to more realistically exercise deactivation and catch reload/regression issues; update the test method test_process_ip_skips_when_deactivated to use self.device.deactivate() and keep the assertion that mock_task.call_count == 0 after service.process_ip_data_and_location().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 213-214: Add the same deactivation guard used in WHOIS: at the
start of EstimatedLocationService.trigger_estimated_location_task (or the method
that invokes it), call self.device.is_deactivated() and immediately return if
True so no Celery task or signals are enqueued; mirror the WHOIS pattern that
prevents _need_whois_lookup and transaction.on_commit from running for
deactivated devices.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1213-1222: The test test_process_ip_runs_when_active currently
only asserts mock_task.call_count; update it to assert that
fetch_whois_details.delay was called with the expected device and IP so the
scheduled task is for the correct device/initial IP. After creating
WHOISService(self.device) and calling service.process_ip_data_and_location(),
replace or augment the count assertion with an assertion on
mock_task.assert_called_once_with(...) (or inspect mock_task.call_args/kwargs)
verifying the device identifier (e.g., self.device.pk or device.id as used by
the service) and the IP "8.8.8.8" are passed to fetch_whois_details.delay so the
call target and payload are validated.
- Around line 351-353: The test triggers an N+1 due to deferred access of the
_is_deactivated field: the clear_last_ip command loads devices with
.only("last_ip", "organization", "key") but saving each device calls
_check_last_ip() → process_ip_data_and_location() → is_deactivated(), which
dereferences _is_deactivated and issues one SELECT per device; update the device
queryset used by the clear_last_ip command (the .only(...) call) to include
"_is_deactivated" so the field is preloaded and avoids the extra queries.
---
Outside diff comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 24-32: The test file imports app_settings and WHOISService twice:
the absolute imports "from openwisp_controller.config import settings as
app_settings" and "from openwisp_controller.config.whois.service import
WHOISService" are duplicated and shadowed by the later relative imports "from
... import settings as app_settings" and "from ..service import WHOISService";
remove the earlier absolute import statements so only the relative imports for
app_settings and WHOISService remain to eliminate redundancy.
---
Duplicate comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1205-1211: The test currently flips the private in-memory flag
self.device._is_deactivated; instead call the public deactivation path by
invoking self.device.deactivate() (which persists and emits signals) before
instantiating WHOISService(self.device) to more realistically exercise
deactivation and catch reload/regression issues; update the test method
test_process_ip_skips_when_deactivated to use self.device.deactivate() and keep
the assertion that mock_task.call_count == 0 after
service.process_ip_data_and_location().
🪄 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: a686e353-0710-4a86-9b42-c70f282c810f
📒 Files selected for processing (2)
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.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). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
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.
📚 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/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (1)
openwisp_controller/config/whois/service.py (1)
234-235: LGTM on the second guard.Placed before the public-IP/enabled/staleness checks so no redundant work runs, and consistent with the guard in
process_ip_data_and_location.
Flake8 and Commit Message FailuresHello @tomsebastian10, There are two issues with your commit:
Here's an example of the correct format: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/whois/tests/tests.py (1)
24-32:⚠️ Potential issue | 🟡 MinorDuplicate imports for
app_settingsandWHOISService.Lines 24 and 26 are equivalent to the existing relative imports on lines 30 and 32 —
...resolves toopenwisp_controller.configand..serviceresolves toopenwisp_controller.config.whois.servicefrom this test module. The second import rebinds the same name, which flake8 will report as F811. Drop the new absolute imports (keep the existing relative ones to stay consistent with the surrounding imports) and keep only the genuinely newwhois_fetched, whois_lookup_skippedimport.🧹 Proposed fix
-from openwisp_controller.config import settings as app_settings from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped -from openwisp_controller.config.whois.service import WHOISService from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin from ... import settings as app_settings from ..handlers import connect_whois_handlers from ..service import WHOISServiceAs per coding guidelines: "Flag unused or redundant code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 24 - 32, The test file has duplicate imports rebinding app_settings and WHOISService (the absolute imports at the top shadow the existing relative imports from lines that import app_settings and WHOISService); remove the redundant absolute imports that rebind app_settings and WHOISService and keep the existing relative imports (retain the relative imports of app_settings and WHOISService from the current package) while keeping only the new signal imports (whois_fetched, whois_lookup_skipped) from openwisp_controller.config.signals so there is no F811 duplicate-import error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 24-32: The test file has duplicate imports rebinding app_settings
and WHOISService (the absolute imports at the top shadow the existing relative
imports from lines that import app_settings and WHOISService); remove the
redundant absolute imports that rebind app_settings and WHOISService and keep
the existing relative imports (retain the relative imports of app_settings and
WHOISService from the current package) while keeping only the new signal imports
(whois_fetched, whois_lookup_skipped) from openwisp_controller.config.signals so
there is no F811 duplicate-import error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21a6e4db-6832-429e-946d-a61a2386ed64
📒 Files selected for processing (3)
openwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/service.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.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.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.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- 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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/service.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (3)
openwisp_controller/geo/estimated_location/service.py (1)
65-67: LGTM — defense-in-depth guard for deactivated devices.The early return mirrors the guard added in
WHOISServiceand correctly short-circuitswhois_fetched_handler/whois_lookup_skipped_handlerpaths that eventually calltrigger_estimated_location_task. Even thoughprocess_ip_data_and_locationalready guards at the source, having this guard here prevents regressions if the task is ever triggered from a different code path.openwisp_controller/config/whois/tests/tests.py (1)
1193-1225: LGTM — prior review feedback fully addressed.
- The class now correctly inherits
CreateWHOISMixinandWHOISTransactionMixinand invokessuper().setUp(), so_create_device()is available.setUpexplicitly enableswhois_enabledon the organization'sconfig_settings, making the active-path test deterministic rather than relying on the globalWHOIS_ENABLEDdefault.test_process_ip_runs_when_activenow asserts kwargs viaassert_called_once_with(...), providing a stronger regression signal than a bare call count.One residual note (not a blocker):
test_process_ip_skips_when_deactivatedstill mutates_is_deactivatedin memory — this works becauseWHOISServiceholds the same instance, butself.device.deactivate()would be more faithful to the production flow.openwisp_controller/config/whois/commands.py (1)
42-42: LGTM — correctly prevents the N+1 regression.Including
_is_deactivatedin.only()preloads the field so thatdevice.save()→_check_last_ip()→process_ip_data_and_location()→is_deactivated()doesn't trigger a deferred-field SELECT per device during iteration. This keepstest_last_ip_management_command_queriesat the expected 4 queries regardless of device count.Based on learnings: "In Django projects, when using select_related() to traverse relations ... the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field in the .only() list" — while this specific addition is for a performance reason rather than a traversal conflict, it's consistent with keeping
.only()aligned with all fields accessed downstream of the queryset.
Flake8 F811 Redefinition ErrorsHello @tomsebastian10, There are Error: Error: Fix: |
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
7a02dde to
75273f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/geo/estimated_location/service.py (1)
65-79:⚠️ Potential issue | 🟡 MinorAdd a test for the deactivation guard in
trigger_estimated_location_task.The guard is exercised only indirectly through WHOIS signal handlers; no test in
openwisp_controller/geo/estimated_location/tests/directly verifies thatcurrent_app.send_taskis not called when a device is deactivated. A dedicated test should deactivate a device and assert the task is not enqueued, ensuring that future refactors (e.g., the handler being called from another code path) do not silently bypass this safety check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/estimated_location/service.py` around lines 65 - 79, Write a new unit test that directly exercises EstimatedLocationService.trigger_estimated_location_task to assert it does not enqueue a task when the device is deactivated: instantiate or retrieve a Device, call device.deactivate() (or set the state so device.is_deactivated() returns True), call EstimatedLocationService(device).trigger_estimated_location_task(ip_address), and assert current_app.send_task was not called (use patch/mock on current_app.send_task or task sending mechanism); reference the service method trigger_estimated_location_task, the guard device.is_deactivated, and the call to current_app.send_task (task name whois_estimated_location_task) so the test fails if the guard is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/commands.py`:
- Around line 40-42: The comment should be extended to explain why
"_is_deactivated" is included in the .only(...) call: because the per-device
device.save() within the loop flows into process_ip_data_and_location(), which
calls is_deactivated() and would otherwise trigger a SELECT per device for a
deferred field; update the comment near the .only("last_ip", "organization",
"key", "_is_deactivated") line to note that "_is_deactivated" is required to
avoid reintroducing an N+1 query (see is_deactivated(),
process_ip_data_and_location(), and test_last_ip_management_command_queries for
context).
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1203-1204: The test test_process_ip_skips_when_deactivated mutates
the private flag self.device._is_deactivated instead of exercising the real
deactivation flow; replace that in-memory assignment with a call to
self.device.deactivate() so the device's last_ip/management_ip are cleared and
the model is persisted (this mirrors production behavior when WHOISService may
fetch a fresh instance), or if you intentionally must keep the in-memory toggle,
add a short comment explaining why the real deactivate() cannot be used.
- Around line 1191-1223: Test coverage missing for the deactivation guard in
WHOISService.update_whois_info: add a test that (1) creates/updates a WHOISInfo
for the device's public last_ip with a timestamp older than
WHOIS_REFRESH_THRESHOLD_DAYS, (2) sets self.device._is_deactivated = True, (3)
instantiates WHOISService(self.device) and calls service.update_whois_info(),
and (4) asserts that fetch_whois_details.delay was not called (mocking
"openwisp_controller.config.whois.service.fetch_whois_details.delay"); reference
WHOISService.update_whois_info, WHOISInfo, and WHOIS_REFRESH_THRESHOLD_DAYS to
locate the logic to exercise.
- Around line 1201-1209: The test test_process_ip_skips_when_deactivated is not
asserting the deactivation guard because the device created in setUp has
last_ip=None; set a public last_ip on the device and save it before
instantiating WHOISService so _need_whois_lookup sees a valid IP, then mark the
device deactivated (self.device._is_deactivated = True or use is_deactivated)
and call WHOISService(self.device).process_ip_data_and_location(); this mirrors
test_process_ip_runs_when_active and ensures the only reason the task is not
scheduled is the deactivation guard in
WHOISService.process_ip_data_and_location/_need_whois_lookup.
---
Outside diff comments:
In `@openwisp_controller/geo/estimated_location/service.py`:
- Around line 65-79: Write a new unit test that directly exercises
EstimatedLocationService.trigger_estimated_location_task to assert it does not
enqueue a task when the device is deactivated: instantiate or retrieve a Device,
call device.deactivate() (or set the state so device.is_deactivated() returns
True), call
EstimatedLocationService(device).trigger_estimated_location_task(ip_address),
and assert current_app.send_task was not called (use patch/mock on
current_app.send_task or task sending mechanism); reference the service method
trigger_estimated_location_task, the guard device.is_deactivated, and the call
to current_app.send_task (task name whois_estimated_location_task) so the test
fails if the guard is removed.
🪄 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: 859dd8f1-01f0-4df2-9986-15fb37eec4a3
📒 Files selected for processing (3)
openwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/service.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.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | 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.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- 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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
Test Failure in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
75273f8 to
2eeff23
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openwisp_controller/config/whois/tests/tests.py (1)
1242-1243:⚠️ Potential issue | 🔴 CriticalTypo
ervice→NameErrorat runtime; this test cannot pass.Line 1242 defines
ervice(missings) while line 1243 referencesservice.service.update_whois_info()will raiseNameError: name 'service' is not defined, so this test never reaches itsassert_not_called()check.🐛 Proposed fix
- ervice = WHOISService(self.device) + service = WHOISService(self.device) service.update_whois_info()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 1242 - 1243, There's a typo: the variable is created as "ervice = WHOISService(self.device)" but later referenced as "service.update_whois_info()", causing a NameError; rename the declared variable to "service" (or update the later reference to "ervice") so the WHOISService instance assigned to service is the one whose update_whois_info() method is called, ensuring the test proceeds to the subsequent assert_not_called() check (look for WHOISService and update_whois_info in this test).openwisp_controller/config/whois/commands.py (1)
40-42: 🧹 Nitpick | 🔵 TrivialExtend the comment to also justify
_is_deactivated.The existing comment only motivates the
organizationentry. Without a note, a future cleanup could silently drop_is_deactivatedand reintroduce the N+1 on thedevice.save()→process_ip_data_and_location()→is_deactivated()path thattest_last_ip_management_command_queriespins at 4 queries.♻️ Proposed comment tweak
- # include the FK field 'organization' in .only() so the related - # `organization__config_settings` traversal is not deferred + # Include the FK 'organization' so the `organization__config_settings` + # traversal is not deferred, and '_is_deactivated' so the per-device + # save() -> process_ip_data_and_location() -> is_deactivated() path + # does not trigger an N+1 on the deferred field. .only("last_ip", "organization", "key", "_is_deactivated")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/commands.py` around lines 40 - 42, The comment explaining the .only("last_ip", "organization", "key", "_is_deactivated") call is incomplete; extend it to also justify including "_is_deactivated" so future maintainers don't remove it and reintroduce an N+1 during device.save() → process_ip_data_and_location() → is_deactivated(); update the comment to state that "_is_deactivated" is eagerly loaded to avoid extra DB hits observed by test_last_ip_management_command_queries and to preserve the intended query count when traversing organization__config_settings as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1219-1231: Add a one-line clarifying comment in the
test_process_ip_runs_when_active test explaining that because this test class
uses TransactionTestCase, transaction.on_commit callbacks run immediately (there
is no enclosing atomic block) and that saving the device synchronizes
_initial_last_ip with last_ip, which is why the mock_task assertion succeeds;
reference the test name (test_process_ip_runs_when_active),
WHOISService.process_ip_data_and_location, and the _initial_last_ip/on_commit
behavior to make future migration to TestCase (which defers on_commit) obvious.
- Around line 1233-1245: The test test_update_whois_skips_when_deactivated is
invalid because it never exercises the deactivation branch; fix it by creating a
WHOISInfo row for the device IP that is expired (so update_whois_info would
attempt to schedule work), correct the typo "ervice" → "service", then assert
fetch_whois_details.delay is called for the active device and after calling
self.device.deactivate() call WHOISService(self.device).update_whois_info()
again and assert delay is not called; target WHOISService.update_whois_info, the
WHOISInfo test fixture/creation, and the mocked fetch_whois_details.delay in
your changes.
---
Duplicate comments:
In `@openwisp_controller/config/whois/commands.py`:
- Around line 40-42: The comment explaining the .only("last_ip", "organization",
"key", "_is_deactivated") call is incomplete; extend it to also justify
including "_is_deactivated" so future maintainers don't remove it and
reintroduce an N+1 during device.save() → process_ip_data_and_location() →
is_deactivated(); update the comment to state that "_is_deactivated" is eagerly
loaded to avoid extra DB hits observed by
test_last_ip_management_command_queries and to preserve the intended query count
when traversing organization__config_settings as well.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1242-1243: There's a typo: the variable is created as "ervice =
WHOISService(self.device)" but later referenced as
"service.update_whois_info()", causing a NameError; rename the declared variable
to "service" (or update the later reference to "ervice") so the WHOISService
instance assigned to service is the one whose update_whois_info() method is
called, ensuring the test proceeds to the subsequent assert_not_called() check
(look for WHOISService and update_whois_info in this test).
🪄 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: ae9ec19c-6928-40c3-bee7-ba3c3a28eab1
📒 Files selected for processing (3)
openwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/service.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
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.
📚 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/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.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/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.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/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (1)
openwisp_controller/geo/estimated_location/service.py (1)
66-67: LGTM!The deactivation guard at the entry of
trigger_estimated_location_taskcorrectly short-circuits task enqueueing and mirrors the same pattern introduced inopenwisp_controller/config/whois/service.py(process_ip_data_and_locationandupdate_whois_info). Placing the check before thetry/except send_taskblock is the right spot — callers inhandlers.py(whois_fetched_handlerandwhois_lookup_skipped_handler) don't need to duplicate the check, and no Celery task is scheduled for deactivated devices.
Flake8 Errors in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
2eeff23 to
eadb209
Compare
Test Failures in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
eadb209 to
8633a01
Compare
Flake8 F821 Error in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
8633a01 to
6e6f41e
Compare
Test Failure in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
6e6f41e to
2504f10
Compare
|
The remaining CI failure appears unrelated to the changes in this PR. |
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai full review! |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR diff
gh pr view 1340 --repo openwisp/openwisp-controller --json files,body,title
# Get the diff
gh pr diff 1340 --repo openwisp/openwisp-controllerLength of output: 20396 🏁 Script executed: #!/bin/bash
# Check which test class the new tests are added to
# Look at lines around test_get_whois_info_when_not_configured
rg -n "class Test|def test_get_whois_info_when_not_configured|def test_process_ip_skips|def test_process_ip_runs|def test_update_whois_skips|TransactionTestCase|TestCase" openwisp_controller/config/whois/tests/tests.py | head -80
# Check service.py for _need_whois_lookup implementation
rg -n "_need_whois_lookup|_initial_last_ip|is_deactivated" openwisp_controller/config/whois/service.py -A3
# Check estimated_location/service.py for is_deactivated usage
rg -n "is_deactivated|_is_deactivated" openwisp_controller/geo/estimated_location/service.py
# Check views.py for how save() triggers signals related to is_deactivated
rg -n "is_deactivated|_is_deactivated" openwisp_controller/config/controller/views.py
# Check if there are similar .only() queries that might be missing _is_deactivated
rg -n "\.only(" openwisp_controller/config/whois/ openwisp_controller/geo/estimated_location/ -A2
# Check the whois_lookup_skipped signal handler
rg -n "whois_lookup_skipped|estimated_location" openwisp_controller/config/whois/service.py -A5
# Check what fields _is_deactivated is a property/field
rg -n "_is_deactivated|is_deactivated" openwisp_controller/config/models.py -A3 2>/dev/null | head -40
rg -rn "def is_deactivated" openwisp_controller/ -A5
# Check if estimated_location tasks.py has .only() queries that might need _is_deactivated
rg -n "\.only(" openwisp_controller/geo/estimated_location/tasks.py -A3Length of output: 5527 🏁 Script executed: #!/bin/bash
# Check which class test_process_ip_skips_when_deactivated belongs to (lines 1090-1170)
sed -n '1090,1170p' openwisp_controller/config/whois/tests/tests.py
# Check the TestWHOISTransaction class definition and what it inherits
sed -n '427,440p' openwisp_controller/config/whois/tests/tests.py
# Check _need_whois_lookup implementation
sed -n '177,210p' openwisp_controller/config/whois/service.py
# Check is_deactivated in device.py
rg -n "is_deactivated|_is_deactivated" openwisp_controller/config/base/device.py -A3 | head -40
# Check process_ip_data_and_location - the full lambda to see what initial_ip is passed
sed -n '208,235p' openwisp_controller/config/whois/service.py
# Check if any .only() queries in estimated_location tasks are missing _is_deactivated
rg -n "\.only\(" openwisp_controller/geo/estimated_location/tasks.py -A2
# Check if any .only() queries in whois are missing _is_deactivated
rg -n "\.only\(" openwisp_controller/config/whois/ -A2
# Check estimated location task manage_estimated_locations for full context
sed -n '110,145p' openwisp_controller/geo/estimated_location/tasks.py
# Check if test_process_ip_runs_when_active uses a TransactionTestCase
# by checking the full class containing those tests
sed -n '427,435p' openwisp_controller/config/whois/tests/tests.pyLength of output: 12310
Review of PR
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openwisp_controller/config/whois/commands.py (1)
40-42: 🧹 Nitpick | 🔵 TrivialComment should explain why
_is_deactivatedis included.The comment explains why
organizationis in.only(), but not why_is_deactivatedis included. Without it, the per-devicesave()→ signal handlers →is_deactivated()path would trigger an N+1 query on the deferred field.🤖 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/whois/commands.py` around lines 40 - 42, The comment on the QuerySet .only("last_ip", "organization", "key", "_is_deactivated") is missing why "_is_deactivated" is included; update the comment to state that including the model field "_is_deactivated" prevents deferred-loading during the per-device save() → signal handlers → is_deactivated() path and avoids an N+1 query when signal handlers access that attribute (also keep the existing note about including the FK 'organization' to prevent deferring organization__config_settings).
🤖 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/controller/views.py`:
- Line 96: Add a concise inline comment next to the queryset.only("pk", "key",
"management_ip", "_is_deactivated") call explaining that _is_deactivated is
included even though it's not referenced in the loop because dupe.save()
triggers signal handlers that call is_deactivated(), and omitting the field
would cause a SELECT per save (N+1 query); reference the symbols queryset.only,
"_is_deactivated", dupe.save(), and is_deactivated() in the comment for clarity.
- Line 111: Add a brief inline comment next to the queryset.only("pk", "key",
"last_ip", "_is_deactivated") call explaining that "_is_deactivated" is included
to avoid N+1 queries because calling dupe.save() triggers signal handlers that
call is_deactivated(), so the flag is prefetched; reference the loop variable
"dupe", the "is_deactivated()" method, and the save-triggered signal handlers in
the comment.
---
Duplicate comments:
In `@openwisp_controller/config/whois/commands.py`:
- Around line 40-42: The comment on the QuerySet .only("last_ip",
"organization", "key", "_is_deactivated") is missing why "_is_deactivated" is
included; update the comment to state that including the model field
"_is_deactivated" prevents deferred-loading during the per-device save() →
signal handlers → is_deactivated() path and avoids an N+1 query when signal
handlers access that attribute (also keep the existing note about including the
FK 'organization' to prevent deferring organization__config_settings).
🪄 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: 95e882a5-28bb-4575-8351-bd94b4a94c9a
📒 Files selected for processing (8)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📜 Review details
🧰 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/geo/estimated_location/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.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/geo/estimated_location/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
🪛 GitHub Actions: OpenWISP Controller CI Build / 1_Python==3.10 django~=5.2.0.txt
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task.
🪛 GitHub Actions: OpenWISP Controller CI Build / 2_Python==3.12 django~=4.2.0.txt
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: Celery warning: AlwaysEagerIgnored: task_always_eager has no effect on send_task.
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.10 django~=4.2.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task (current_app.send_task...).
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.10 django~=5.1.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task (current_app.send_task).
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.10 django~=5.2.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task.
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.11 django~=4.2.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task (current_app.send_task(...)).
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.12 django~=4.2.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task.
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.12 django~=5.1.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task.
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.12 django~=5.2.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task.
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.13 django~=5.1.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: AlwaysEagerIgnored: task_always_eager has no effect on send_task
🪛 GitHub Actions: OpenWISP Controller CI Build / Python==3.13 django~=5.2.0
openwisp_controller/geo/estimated_location/service.py
[warning] 70-70: Celery warning AlwaysEagerIgnored: task_always_eager has no effect on send_task.
🔇 Additional comments (12)
openwisp_controller/config/whois/service.py (2)
211-218: LGTM!
239-241: LGTM!openwisp_controller/config/whois/tasks.py (1)
75-79: LGTM!openwisp_controller/config/whois/tests/tests.py (5)
180-184: LGTM!Also applies to: 487-492, 506-509, 831-843, 881-886, 960-963, 979-984
1012-1024: LGTM!
1108-1124: LGTM!
1126-1144: LGTM!
1146-1165: LGTM!openwisp_controller/geo/estimated_location/service.py (1)
66-68: LGTM!openwisp_controller/geo/estimated_location/tasks.py (1)
124-130: LGTM!openwisp_controller/geo/estimated_location/tests/tests.py (2)
197-211: LGTM!
816-827: LGTM!
|
@coderabbitai any outstanding issue to resolve? full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the current state of the test assertion in test_process_ip_runs_when_active
rg -n "test_process_ip_runs_when_active|initial_ip_address|_initial_last_ip|assert_called_once_with" openwisp_controller/config/whois/tests/tests.py -A3 | head -60
# Check views.py for the _is_deactivated .only() comment
rg -n "_is_deactivated|is_deactivated" openwisp_controller/config/controller/views.py -B1 -A1
# Check reset_mock placement relative to deactivate() in test_process_ip_skips_when_deactivated
rg -n "test_process_ip_skips_when_deactivated|reset_mock|deactivate" openwisp_controller/config/whois/tests/tests.py -A2 | head -40Length of output: 4750
The minor observation about No outstanding blocking issues. The PR looks good to merge. ✅ Actions performedFull review triggered. |
|
Proposed change log entry: |
nemesifier
dismissed stale reviews from coderabbitai[bot] and themself
via
6f207f8
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
LGTM ✔️
Thanks @tomsebastian10 @pandafy
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
nemesifier
dismissed stale reviews from coderabbitai[bot] and themself
via
6bc1b2c
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
4 participants
Checklist
Reference to Existing Issue
Closes #1325.
Description of Changes
This PR fixes an issue where WHOIS and Estimated Location lookups were still being triggered for deactivated devices.
Previously, the logic responsible for scheduling WHOIS lookup tasks (process_ip_data_and_location and update_whois_info) did not check whether the device was active. As a result, background tasks continued to run even when a device was deactivated, causing WHOIS and location data to be repopulated unintentionally.
This change introduces a guard condition to skip these operations when the device is inactive:
Added an early return in process_ip_data_and_location
Added an early return in update_whois_info
This ensures that:
No WHOIS lookup tasks are scheduled for deactivated devices
No unnecessary background processing occurs
Device state is respected consistently