fix(geo): skip WHOIS and estimated location on deactivated devices#1335
fix(geo): skip WHOIS and estimated location on deactivated devices#1335tomsebastian10 wants to merge 2 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds early-return checks in Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: 1 Critical Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Fix Reverted: The incremental diff shows that the As a result, this PR currently does NOT fix issue #1325. The WHOIS and Estimated Location lookups will still be triggered for deactivated devices. The PR description states this is a fix, but the actual code changes have been reverted to the original state. Please either:
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.5-0127 · 300,139 tokens |
Test Failures in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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: The code calls the misspelled method
self.device.is_deactived() in process_ip_data_and_location() and
update_whois_info(); replace both occurrences with the correct Device method
name self.device.is_deactivated() so the AttributeError is avoided and device
save operations proceed normally.
🪄 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: d6131e02-bd6f-4afd-ad99-d39c9fdc1ed2
📒 Files selected for processing (1)
openwisp_controller/config/whois/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.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | 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~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | 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.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.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.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.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.
📚 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.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.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.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.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.py
| if self.device.is_deactived(): | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the method definition(s) on the Device model to confirm the spelling.
rg -nP '\bdef\s+is_deactiv\w*\s*\(' --type=py -C2
# Show all call sites for both spellings to see which one is canonical in the codebase.
rg -nP '\bis_deactiv\w*\s*\(' --type=py -C1Repository: openwisp/openwisp-controller
Length of output: 13256
Fix method name: is_deactived() should be is_deactivated().
The method is misspelled at lines 213 and 234. The correct method name on the Device model is is_deactivated() (with an "a"), not is_deactived(). These calls will raise AttributeError whenever process_ip_data_and_location() or update_whois_info() are invoked, breaking device save operations.
🤖 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, The code
calls the misspelled method self.device.is_deactived() in
process_ip_data_and_location() and update_whois_info(); replace both occurrences
with the correct Device method name self.device.is_deactivated() so the
AttributeError is avoided and device save operations proceed normally.
|
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