fix(geo): skip WHOIS and estimated location on deactivated devices#1340
fix(geo): skip WHOIS and estimated location on deactivated devices#1340tomsebastian10 wants to merge 6 commits intoopenwisp:masterfrom
Conversation
…openwisp#1325 This commit adds regression tests to ensure that the WHOIS handling for deactivated devices works as expected.
📝 WalkthroughWalkthroughAdds deactivation guards to WHOIS and Estimated Location flows so processing stops immediately when a device is deactivated. WHOISService.process_ip_data_and_location and WHOISService.update_whois_info now return early for deactivated devices, avoiding DB updates being committed, skipping scheduling of the Celery fetch_whois_details task and emission of the whois_lookup_skipped signal. trigger_estimated_location_task similarly aborts without sending the estimated-location task for deactivated devices. Tests and a management command projection were adjusted accordingly. Sequence Diagram(s)sequenceDiagram
participant Device
participant WHOISService
participant Database
participant CeleryTask as "Celery (fetch_whois_details)"
participant SignalBus as "Signal (whois_lookup_skipped)"
Device->>WHOISService: process_ip_data_and_location(ip, location)
WHOISService->>WHOISService: check device._is_deactivated
alt device deactivated
WHOISService-->>Device: return (skip DB, task, signal)
else device active
WHOISService->>Database: update last_ip / location (in transaction)
Database-->>WHOISService: commit hooks scheduled
WHOISService->>SignalBus: maybe emit whois_lookup_skipped
WHOISService->>CeleryTask: schedule fetch_whois_details.delay on commit
end
sequenceDiagram
participant Device
participant WHOISService
participant Database
participant CeleryTask as "Celery (fetch_whois_details)"
Device->>WHOISService: update_whois_info(device, data)
WHOISService->>WHOISService: check device._is_deactivated
alt device deactivated
WHOISService-->>Device: return (skip validation, staleness, scheduling)
else device active
WHOISService->>WHOISService: validate / check staleness
WHOISService->>Database: persist whois info (transaction)
WHOISService->>CeleryTask: schedule fetch_whois_details.delay if needed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)The PR description states it fixes issue #1325 (preventing WHOIS lookups for deactivated devices), but all changes have been reverted again. The current code does NOT check if a device is deactivated before triggering WHOIS lookups in:
The fix needs to add early returns in both methods: if self.device.is_deactivated():
returnRegression tests for deactivated devices were also removed from the PR. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.5-0127 · 114,601 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.
| @@ -0,0 +1 @@ | |||
| fatal: no rebase in progress | |||
There was a problem hiding this comment.
Critical: Invalid file content - Git error message instead of code.
This file contains only a Git error message (fatal: no rebase in progress) rather than actual code changes. The PR objectives indicate that guard conditions should be added to process_ip_data_and_location and update_whois_info functions to skip WHOIS lookups for deactivated devices, but no such code is present in the files provided for review.
Please verify the PR contents and ensure the actual code changes are included.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@null` at 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.
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||
| 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() | ||
|
|
||
| self.assertEqual(mock_task.call_count, 0) | ||
|
|
||
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||
| def test_process_ip_runs_when_active(self, mock_task): | ||
| self.device._is_deactivated = False | ||
|
|
||
| service = WHOISService(self.device) | ||
| service.process_ip_data_and_location() | ||
|
|
||
| self.assertEqual(mock_task.call_count, 1) |
There was a problem hiding this comment.
Active-path test is not a reliable regression guard.
Two problems with test_process_ip_runs_when_active:
- It only patches
WHOIS_CONFIGURED=Truebut does not enablewhois_enabledon the organization's config settings.is_whois_enabledfalls back to the globalWHOIS_ENABLED, so whetherfetch_whois_details.delayis scheduled depends on a global default unrelated to the deactivation fix. Explicitly enable it (org.config_settings.whois_enabled = True; org.config_settings.save()) to make the test deterministic — see the pattern intest_whois_task_calledat Lines 483-485. - Calling
self.device.save()insetUpwithlast_ip="8.8.8.8"already triggersprocess_ip_data_and_locationvia the post-save flow; by the time the test body runs, the mock patch isn't active yet. Create the device (or change its IP) inside the patched block somock_task.call_count == 1actually reflects the call under test.
Additionally, test_process_ip_skips_when_deactivated sets _is_deactivated = True in-memory without saving; this happens to work because WHOISService holds the same instance, but a brief comment would prevent future confusion (or use self.device.deactivate() for realism).
🤖 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 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.
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.
| if self.device.is_deactivated(): | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the estimated_location service also has a deactivation guard,
# since issue `#1325` requires both WHOIS and Estimated Location to be skipped.
fd -t f 'service\.py$' openwisp_controller/geo/estimated_location --exec cat {} \;
echo "---"
# Look for any is_deactivated / _is_deactivated usage inside estimated_location
rg -nP -C3 '(is_deactivated|_is_deactivated)' openwisp_controller/geo/estimated_locationRepository: openwisp/openwisp-controller
Length of output: 5427
Estimated Location service is missing the deactivation guard—fix is incomplete.
The WHOIS service guard is correct: is_deactivated() is called before _need_whois_lookup and transaction.on_commit, properly suppressing the Celery task and signal for deactivated devices.
However, issue #1325 explicitly requires the same protection for both WHOIS and Estimated Location. The EstimatedLocationService lacks an equivalent guard in trigger_estimated_location_task(). Deactivated devices will still enqueue estimated location tasks, leaving the fix incomplete.
Add an is_deactivated() guard at the entry point of EstimatedLocationService.trigger_estimated_location_task() (or the method that calls it) to match the WHOIS service implementation.
🤖 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 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.
|
|
||
| with self.assertNumQueries(7): | ||
| call_command("clear_last_ip", *args, stdout=out, stderr=StringIO()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect clear_last_ip to see if .only() is used and whether _is_deactivated is included.
fd -t f 'clear_last_ip.py$' openwisp_controller --exec cat {} \;
echo "---"
# Also check any other callers of process_ip_data_and_location / update_whois_info that may load deferred devices
rg -nP -C4 '\b(process_ip_data_and_location|update_whois_info)\s*\(' --type=pyRepository: openwisp/openwisp-controller
Length of output: 5544
🏁 Script executed:
fd -t f 'commands.py' openwisp_controller/config/whois --exec cat {} \;Repository: openwisp/openwisp-controller
Length of output: 2598
🏁 Script executed:
# Find is_deactivated method
rg -nP 'def is_deactivated' --type=py openwisp_controllerRepository: openwisp/openwisp-controller
Length of output: 224
🏁 Script executed:
sed -n '185,200p' openwisp_controller/config/base/device.pyRepository: openwisp/openwisp-controller
Length of output: 628
🏁 Script executed:
# Find process_ip_data_and_location and check for the guard
rg -nP -A15 'def process_ip_data_and_location' openwisp_controller/config/whois/service.pyRepository: openwisp/openwisp-controller
Length of output: 803
🏁 Script executed:
# Find the signal handler that connects process_ip_data_and_location to post_save
rg -nP 'post_save.*device|process_ip_data_and_location' --type=py openwisp_controller/config/whoisRepository: openwisp/openwisp-controller
Length of output: 500
🏁 Script executed:
# Check the test at line 351 to see the baseline
sed -n '340,360p' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 887
🏁 Script executed:
# Find the method that calls process_ip_data_and_location on post_save
sed -n '530,550p' openwisp_controller/config/base/device.pyRepository: openwisp/openwisp-controller
Length of output: 642
🏁 Script executed:
# Find where _check_last_ip is called
rg -nP '_check_last_ip' --type=py openwisp_controller/config/base/device.pyRepository: openwisp/openwisp-controller
Length of output: 181
🏁 Script executed:
# Check context around line 292
sed -n '280,310p' openwisp_controller/config/base/device.pyRepository: openwisp/openwisp-controller
Length of output: 1535
Include _is_deactivated in the .only() call to avoid N+1 deferred-field access.
The clear_last_ip_command loads devices with .only("last_ip", "organization", "key") without _is_deactivated. When each device is saved, it triggers _check_last_ip() → process_ip_data_and_location() → is_deactivated(), which dereferences the deferred field and causes one SELECT per device. With 3 test devices, this produces exactly 3 extra queries (4→7), confirming the N+1 regression. Add _is_deactivated to the .only() call so the guard becomes a no-op lookup on an already-loaded field, and the test will remain at 4 queries.
🤖 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 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.
Flake8 and Commit Message FailuresHello @tomsebastian10, There are two issues with your commit:
Here's an example of the correct format: |
…isp#1325 - add guard in WHOISService to skip processing for deactivated devices - add guard in EstimatedLocationService to prevent task scheduling - fix N+1 query by including _is_deactivated in clear_last_ip_command queryset - add regression tests ensuring tasks are skipped when device is deactivated - ensure deterministic tests by enabling whois on organization settings Fixes openwisp#1325
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: |
1 participant
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