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 7 commits intoopenwisp:masterfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds early-return checks in WhoIsService.process_ip_data_and_location() and WhoIsService.update_whois_info() to exit immediately when self.device.is_deactivated() is true, preventing WHOIS lookups, preventing scheduling of the fetch_whois_details Celery task, and stopping emission of the whois_lookup_skipped signal. Tests were added to assert that no Celery task is enqueued for deactivated devices and one existing test's expected query count was adjusted. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 and Tests Removed: The incremental diff shows:
Current state:
This PR claims to close #1325 but contains NO fix. Please restore both the implementation and tests. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.5-0127 · 342,364 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.
|
Black and Flake8 FailuresHello @tomsebastian10, There are two issues in the CI logs:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/whois/service.py (1)
213-218:⚠️ Potential issue | 🔴 CriticalResolve the merge conflict and keep
is_deactivated().The conflict markers make this file invalid Python, and the alternate branch calls the non-existent
is_deactived()method. Keep only the canonicalself.device.is_deactivated()guard in both methods.🐛 Proposed fix
-<<<<<<< HEAD if self.device.is_deactivated(): -======= - if self.device.is_deactived(): ->>>>>>> d3173be671ff5dbef364d55ec120a8c083eaf7d4 return-<<<<<<< HEAD if self.device.is_deactivated(): -======= - if self.device.is_deactived(): ->>>>>>> d3173be671ff5dbef364d55ec120a8c083eaf7d4 returnAlso applies to: 238-243
🤖 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 - 218, Remove the merge conflict markers and the incorrect method name and keep the canonical guard self.device.is_deactivated() in place; specifically, delete the <<<<<<<, =======, >>>>>>> blocks and replace any occurrence of self.device.is_deactived() with self.device.is_deactivated(), ensuring both locations of this guard are identical and the file is valid Python.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 213-218: Remove the merge conflict markers and the incorrect
method name and keep the canonical guard self.device.is_deactivated() in place;
specifically, delete the <<<<<<<, =======, >>>>>>> blocks and replace any
occurrence of self.device.is_deactived() with self.device.is_deactivated(),
ensuring both locations of this guard are identical and the file is valid
Python.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c386f38-65ad-4e41-91d1-8d9942022886
📒 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.11 | django~=5.2.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.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: publiccode_yml_validation
🧰 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
There was a problem hiding this comment.
Actionable comments posted: 1
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/service.py (1)
208-212: 🧹 Nitpick | 🔵 TrivialConsider documenting the deactivation guard in docstrings.
The docstrings for both
process_ip_data_and_locationandupdate_whois_infocould mention that these methods skip processing entirely when the device is deactivated. This would improve API documentation clarity.📝 Suggested docstring updates
For
process_ip_data_and_location:def process_ip_data_and_location(self, force_lookup=False): """ + Skip processing for deactivated devices. + Trigger WHOIS lookup based on the conditions of `_need_whois_lookup`. Tasks are triggered on commit to ensure redundant data is not created. """For
update_whois_info:def update_whois_info(self): """ + Skip processing for deactivated devices. + Update existing WHOIS data for the device when the data is older than ``OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS``. """Also applies to: 228-233
🤖 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 208 - 212, Update the docstrings for process_ip_data_and_location and update_whois_info to explicitly state that each method will skip processing when the device is deactivated (i.e., a deactivation guard prevents any WHOIS lookup or updates); locate the methods by name in openwisp_controller/config/whois/service.py and add a brief sentence in each docstring indicating the methods return/skip if the device is deactivated so the API behavior is clear to callers and documentation generators.
🤖 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 unit tests that assert both
process_ip_data_and_location and update_whois_info return early when
device.is_deactivated() is True: mock a device with is_deactivated() -> True,
call Service.process_ip_data_and_location and Service.update_whois_info, and
assert they do not perform work. Specifically verify that no WHOIS Celery task
is scheduled (mock the task scheduler or delay/apply_async) and that the
whois_lookup_skipped signal is not emitted (connect a test receiver or mock the
signal send) for deactivated devices; reference the Service methods
process_ip_data_and_location and update_whois_info and the whois_lookup_skipped
signal to locate the logic to test. Ensure tests cover both methods and assert
absence of task scheduling and signal emission.
---
Outside diff comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 208-212: Update the docstrings for process_ip_data_and_location
and update_whois_info to explicitly state that each method will skip processing
when the device is deactivated (i.e., a deactivation guard prevents any WHOIS
lookup or updates); locate the methods by name in
openwisp_controller/config/whois/service.py and add a brief sentence in each
docstring indicating the methods return/skip if the device is deactivated so the
API behavior is clear to callers and documentation generators.
🪄 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: 4af265a8-fbda-4c93-bb5c-67fab2aa7f64
📒 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.12 | 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~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | 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.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 (5)
📓 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-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
🔇 Additional comments (2)
openwisp_controller/config/whois/service.py (2)
213-214: LGTM! Guard correctly prevents WHOIS processing for deactivated devices.The early return is correctly placed and prevents unnecessary WHOIS lookup task scheduling and signal emission when the device is deactivated, which aligns with the PR objective to fix issue
#1325.
234-235: LGTM! Guard correctly prevents WHOIS updates for deactivated devices.The early return correctly prevents scheduling WHOIS refresh tasks for deactivated devices, ensuring stale WHOIS data isn't refreshed when the device is inactive.
| if self.device.is_deactivated(): | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests covering the deactivated device + WHOIS interaction
# Search for test methods that reference both deactivation and WHOIS
rg -nP 'def\s+test.*deactivat' --type=py -A 20 | rg -i 'whois|process_ip_data|update_whois'
# Search for tests that call the modified methods
rg -nP '(process_ip_data_and_location|update_whois_info)' --type=py --glob '*test*.py' -C 3
# Search for tests that mock or check is_deactivated in WHOIS context
rg -nP 'is_deactivated.*whois|whois.*is_deactivated' --type=py --glob '*test*.py' -i -C 3Repository: openwisp/openwisp-controller
Length of output: 1490
🏁 Script executed:
# First, locate and examine the service.py file to understand the guard placement
fd -n 'service.py' openwisp_controller/config/whois/ -x cat -n {}
# Then search for tests covering deactivated devices in WHOIS context
rg -n 'is_deactivated' openwisp_controller/config/whois/tests/ --type=py -B 2 -A 10
# Search for test classes and methods related to WHOIS
rg -n 'class.*Test|def test_' openwisp_controller/config/whois/tests/tests.py --type=py | head -50Repository: openwisp/openwisp-controller
Length of output: 2594
🏁 Script executed:
# View the service.py file around the mentioned lines
cat -n openwisp_controller/config/whois/service.py | sed -n '200,250p'
# Search for any reference to deactivated in the test file
rg -i 'deactivat' openwisp_controller/config/whois/tests/tests.py -n -B 2 -A 5
# Search for test methods that specifically test process_ip_data_and_location
rg -n 'def test.*process_ip_data|process_ip_data_and_location' openwisp_controller/config/whois/tests/tests.py -A 15Repository: openwisp/openwisp-controller
Length of output: 4237
🏁 Script executed:
# Search for test methods that use deactivate()
rg -n 'deactivate()' openwisp_controller/config/whois/tests/tests.py -B 5 -A 20
# Check the test_whois_task_called test method to see if it covers deactivated devices
sed -n '476,540p' openwisp_controller/config/whois/tests/tests.py
# Search for any assertions about is_deactivated in tests
rg -n 'is_deactivated|deactivate' openwisp_controller/config/whois/tests/tests.py -A 10Repository: openwisp/openwisp-controller
Length of output: 6499
🏁 Script executed:
# Get the full test_whois_update method
sed -n '741,820p' openwisp_controller/config/whois/tests/tests.py
# Search for any test that explicitly tests behavior with deactivated devices and WHOIS methods
rg -n 'def test.*' openwisp_controller/config/whois/tests/tests.py | xargs -I {} bash -c "grep -q 'is_deactivated\|deactivate()' openwisp_controller/config/whois/tests/tests.py && echo {}"Repository: openwisp/openwisp-controller
Length of output: 11670
🏁 Script executed:
# Final verification: confirm no other tests cover deactivated + WHOIS interaction
rg -n 'process_ip_data_and_location|update_whois_info' openwisp_controller/config/whois/tests/tests.py -B 3 -A 3Repository: openwisp/openwisp-controller
Length of output: 818
Add test coverage for deactivated device scenarios.
Tests are missing to verify:
- Both
process_ip_data_and_locationandupdate_whois_infoskip processing whendevice.is_deactivated()isTrue - No WHOIS tasks are scheduled for deactivated devices
- No
whois_lookup_skippedsignal is emitted for deactivated devices
The PR checklist indicates tests are not yet completed. These guards prevent critical side effects (task scheduling and signal emission) and require test coverage to prevent regressions.
🤖 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 unit
tests that assert both process_ip_data_and_location and update_whois_info return
early when device.is_deactivated() is True: mock a device with is_deactivated()
-> True, call Service.process_ip_data_and_location and
Service.update_whois_info, and assert they do not perform work. Specifically
verify that no WHOIS Celery task is scheduled (mock the task scheduler or
delay/apply_async) and that the whois_lookup_skipped signal is not emitted
(connect a test receiver or mock the signal send) for deactivated devices;
reference the Service methods process_ip_data_and_location and update_whois_info
and the whois_lookup_skipped signal to locate the logic to test. Ensure tests
cover both methods and assert absence of task scheduling and signal emission.
Test Failures in
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 1195-1196: The setUp currently calls
Device.objects.create(name="test-device") which fails because Device requires
organization and mac_address; replace that call with the test helper
self._create_device(...) provided by CreateWHOISMixin/TestOrganizationMixin
(pass at least a name and allow the helper to populate organization and
mac_address) so setUp uses self._create_device("test-device") (or equivalent
arguments) instead of Device.objects.create to avoid IntegrityError and follow
the pattern used by other tests.
- Around line 352-353: The inline comment describing the query breakdown is
stale and should be updated to match the current expectation of 7 queries; edit
the comment above the with self.assertNumQueries(7) assertion in tests.py to
either remove the old "4 queries (3 for ... + 1 for ...)" text or replace it
with a brief breakdown that reflects the added
select_related("organization__config_settings") traversal and any cache
invalidation caused by updating device.last_ip (so the comment mentions
select_related, organization__config_settings, last_ip updates and cache
invalidation as reasons for the extra queries).
- Around line 1194-1214: The tests in TestWhoisDeactivated are no-ops; update
them to actually exercise the deactivation guard by patching
app_settings.WHOIS_CONFIGURED=True, using a public last_ip on the Device, and
switching to TransactionTestCase (or asserting the transaction.on_commit
callback list) so the scheduled fetch_whois_details.delay call from
WhoIsService.process_ip_data_and_location / WhoIsService.update_whois_info is
observable; add a positive control where device._is_deactivated=False to assert
fetch_whois_details.delay is called, and add an assertion with
catch_signal(whois_lookup_skipped) to verify the signal is emitted/omitted for
deactivated devices.
- Around line 25-27: Remove the incorrect import of WhoIsService and the dead
import of Device; ensure the tests reference the existing WHOISService class
(use WHOISService where WhoIsService was used) and rely on load_model("config",
"Device") already used in the file. In TestWhoisDeactivated.setUp(), replace the
direct Device.objects.create(name="test-device") call with the mixin helper
self._create_device() or supply the required organization and mac_address fields
so creation succeeds. Finally, update the inline comment that currently says "4
queries" to match the test assertion (change it to "7 queries") so comment and
expectation align.
🪄 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: c2ae4413-2856-469d-9119-2b21594e0c61
📒 Files selected for processing (1)
openwisp_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.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.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/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/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/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/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-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-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/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/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/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/tests/tests.py
| from openwisp_controller.config.models import Device | ||
| from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped | ||
| from openwisp_controller.config.whois.service import WhoIsService |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the actual service class name exported from whois.service
rg -nP '^\s*class\s+\w*[Ww]ho[Ii][Ss]\w*\s*[:\(]' --type=py
# And check what's importable
rg -nP '\bfrom\s+openwisp_controller\.config\.whois\.service\s+import\b' --type=pyRepository: openwisp/openwisp-controller
Length of output: 279
🏁 Script executed:
# Check the actual service file for class definitions
cat openwisp_controller/config/whois/service.py | head -50
# Also search for any class definition with WHOIS in the name
rg -n 'class.*[Ww]ho' openwisp_controller/config/whois/service.pyRepository: openwisp/openwisp-controller
Length of output: 1569
🏁 Script executed:
# Check the test file imports and Device assignment
sed -n '1,50p' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 2099
🏁 Script executed:
# Check TestWhoisDeactivated class implementation
sed -n '1194,1230p' openwisp_controller/config/whois/tests/tests.py
# Check lines 350-355 for stale comment
sed -n '350,360p' openwisp_controller/config/whois/tests/tests.py
# Check if WhoIsService is used anywhere in the test file
rg -n 'WhoIsService' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 1540
🏁 Script executed:
# Check Device model to see required fields
rg -A 20 'class Device' openwisp_controller/config/models.py | head -50
# Search for organization field requirement in Device
rg -n 'organization.*required|organization.*null.*False' openwisp_controller/config/models.py | head -20
# Check lines 350-360 more carefully for the stale comment
sed -n '348,365p' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 1678
🏁 Script executed:
# Find AbstractDevice and its required fields
rg -A 50 'class AbstractDevice' openwisp_controller/config/models.py | head -80
# Check if organization is a required FK field
rg -n 'organization.*ForeignKey|organization.*models\.' openwisp_controller/config/models.py | head -10Repository: openwisp/openwisp-controller
Length of output: 54
🏁 Script executed:
# Search for AbstractDevice in abstract_models or base files
fd -t f AbstractDevice openwisp_controller/
# Find where AbstractDevice is defined
rg -l 'class AbstractDevice' --type=py
# Get device creation pattern from test utils
rg -B2 -A5 '_create_device' openwisp_controller/config/whois/tests/tests.py | head -40Repository: openwisp/openwisp-controller
Length of output: 1576
🏁 Script executed:
# Find the _create_device method definition in test utils
rg -B5 -A15 'def _create_device' openwisp_controller/config/whois/tests/tests.py | head -40
# Also check the CreateWHOISMixin that's imported
rg -B5 -A15 'def _create_device' openwisp_controller/config/whois/tests/utils.pyRepository: openwisp/openwisp-controller
Length of output: 54
🏁 Script executed:
# Check the test file imports to understand the inheritance chain
grep -n 'class.*TestCase\|CreateWHOISMixin\|TestAdminMixin' openwisp_controller/config/whois/tests/tests.py | head -20
# Check CreateWHOISMixin definition
rg -A20 'class CreateWHOISMixin' openwisp_controller/config/whois/tests/utils.py
# Also check TestAdminMixin
fd -t f 'TestAdminMixin' openwisp_controller/Repository: openwisp/openwisp-controller
Length of output: 1177
🏁 Script executed:
# Find CreateConfigMixin to see what _create_device does
rg -B5 -A15 'class CreateConfigMixin\|def _create_device' openwisp_controller/config/tests/utils.py | head -60
# Check AbstractDevice to see required fields
sed -n '1,100p' openwisp_controller/config/base/device.py | grep -A5 -B5 'organization\|mac_address'Repository: openwisp/openwisp-controller
Length of output: 1040
🏁 Script executed:
# Check OrgMixin to see organization field requirements
rg -B5 -A20 'class OrgMixin' openwisp_controller/config/base/device.py
# Check CreateConfigMixin to see _create_device implementation
rg -B2 -A20 'class CreateConfigMixin\|def _create_device' openwisp_controller/config/tests/utils.py | head -80Repository: openwisp/openwisp-controller
Length of output: 54
🏁 Script executed:
# Search for OrgMixin definition
rg -l 'class OrgMixin' --type=py
# Check the CreateConfigMixin file directly
head -150 openwisp_controller/config/tests/utils.py | tail -100
# Search for _create_device method
rg 'def _create_device' -A15 openwisp_controller/config/tests/utils.pyRepository: openwisp/openwisp-controller
Length of output: 4846
🏁 Script executed:
# Confirm the stale comment - check lines 350-365 again more carefully
sed -n '347,365p' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 1014
🏁 Script executed:
# Check if TestWhoisDeactivated has access to _get_org or _get_org() method
rg -B5 'class TestWhoisDeactivated' openwisp_controller/config/whois/tests/tests.py
# Check if any test uses Device.objects.create without organization
rg 'Device\.objects\.create.*name=' openwisp_controller/config/whois/tests/tests.py | head -20
# Verify that Device requires organization by checking model constraints
rg 'organization.*null.*False|null.*False.*organization' openwisp_controller/config/base/device.py | head -10
# Check if mac_address is required
rg 'mac_address.*blank.*False|mac_address.*required|blank=False.*mac_address' openwisp_controller/config/base/device.pyRepository: openwisp/openwisp-controller
Length of output: 351
🏁 Script executed:
# Check the Device model field definitions directly
sed -n '1,200p' openwisp_controller/config/base/device.py | grep -A3 'organization\|mac_address' | head -40
# Check if organization is inherited from OrgMixin
rg -B5 -A15 'class OrgMixin' --type=py
# Try to find where OrgMixin is defined
fd -t f '*.py' | xargs grep -l 'class OrgMixin' | head -5Repository: openwisp/openwisp-controller
Length of output: 1625
🏁 Script executed:
# Find where OrgMixin is defined
rg -l 'class OrgMixin' --type=py
# Check if Device.objects.create() call in TestWhoisDeactivated will fail
# First, find if organization field is null=True
rg 'organization.*ForeignKey' openwisp_controller/config/base/ -A3 | head -20
# Count how many tests use _create_device vs Device.objects.create directly
rg 'Device\.objects\.create' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 179
🏁 Script executed:
# Find OrgMixin definition
rg -B2 -A25 'class OrgMixin' --type=py | head -60
# Check if organization field has null=False or blank=False
rg 'organization.*models\.ForeignKey' openwisp_controller/ -A5 | head -30Repository: openwisp/openwisp-controller
Length of output: 54
Fix broken import: WhoIsService class does not exist; use WHOISService already imported on line 33.
Line 27 attempts to import WhoIsService, but the actual class name is WHOISService (defined in openwisp_controller/config/whois/service.py). This causes an ImportError at test collection time. Remove the incorrect import on line 27 and use the correct WHOISService already imported on line 33 in the test class.
Additionally:
- Remove the dead import of
Deviceon line 25 (immediately shadowed byload_model("config", "Device")on line 39). - Line 1196 in
TestWhoisDeactivated.setUp():Device.objects.create(name="test-device")is missing required fieldsorganizationandmac_address. Useself._create_device()inherited from the test mixin, or pass these required parameters. - Line 352 comment states "4 queries" but the assertion expects 7 queries; update the comment to match.
Suggested fix
-from openwisp_controller.config.models import Device
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_signalUpdate TestWhoisDeactivated to use WHOISService and fix device creation to include required fields.
Update line 352 comment to "7 queries" instead of "4 queries".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from openwisp_controller.config.models import Device | |
| from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped | |
| from openwisp_controller.config.whois.service import WhoIsService | |
| from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped | |
| from openwisp_utils.tests import SeleniumTestMixin, catch_signal |
🤖 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 25 - 27, Remove
the incorrect import of WhoIsService and the dead import of Device; ensure the
tests reference the existing WHOISService class (use WHOISService where
WhoIsService was used) and rely on load_model("config", "Device") already used
in the file. In TestWhoisDeactivated.setUp(), replace the direct
Device.objects.create(name="test-device") call with the mixin helper
self._create_device() or supply the required organization and mac_address fields
so creation succeeds. Finally, update the inline comment that currently says "4
queries" to match the test assertion (change it to "7 queries") so comment and
expectation align.
| def setUp(self): | ||
| self.device = Device.objects.create(name="test-device") |
There was a problem hiding this comment.
Device.objects.create(name="test-device") will fail validation.
Device requires at minimum organization and mac_address. Even if you bypass full_clean, the FK to organization is non-nullable and creation will raise IntegrityError. Use self._create_device(...) from CreateWHOISMixin / TestOrganizationMixin as every other test in this file does.
🤖 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 1195 - 1196,
The setUp currently calls Device.objects.create(name="test-device") which fails
because Device requires organization and mac_address; replace that call with the
test helper self._create_device(...) provided by
CreateWHOISMixin/TestOrganizationMixin (pass at least a name and allow the
helper to populate organization and mac_address) so setUp uses
self._create_device("test-device") (or equivalent arguments) instead of
Device.objects.create to avoid IntegrityError and follow the pattern used by
other tests.
Code Style and Commit Message FailuresHello @tomsebastian10, (Analysis for commit 8d798b4) Your commit failed due to:
Here's an example of the correct format: |
test(geo): add regression tests for deactivated device WHOIS handling
Code Style and Commit Message FailuresHello @tomsebastian10, (Analysis for commit 99173a5) There are several issues with your commit:
To fix these, please run
A correct commit message should look like this: For example: |
|
Closing this PR in favor of a cleaner version after rebasing and fixing CI issues. New PR: #1340 |
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