[gsoc 25] WHOIS Information and IP Address-Based Geolocation #1175
Conversation
Closes #1026 Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
- Estimated geographic location logic, closes #1034. - Added notification and admin warning, closes #1035. - Set location estimated flag to ``False`` on manual update, closes #1036. - Added admin and API filters for estimated locations, closes #1028. --------- Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
Added required checks for ensuring WHOIS records older than a certain configurable threshold will be updated along with the coordinates. Closes #1058 Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
pandafy
moved this to In Progress
in [GSoC25] WHOIS Information and IP Address-Based Geolocation
DragnEmperor
moved this from In Progress
to Needs Review
in [GSoC25] WHOIS Information and IP Address-Based Geolocation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds WHOIS lookup and estimated-location features: new WHOISInfo model and serializers; WHOISService with GeoIP integration; Celery tasks for fetching/deleting WHOIS and managing estimated Locations; signal handlers and admin UI assets; API mixins/filters to expose whois/estimated fields; Location model is_estimated support and estimated-location creation/update logic; migrations, tests, utilities, notification registration, a management command to clear last_ip, and related settings and static assets. Runtime toggles, validation, caching, and transactional task orchestration are introduced. Sequence Diagram(s)sequenceDiagram
autonumber
participant Device
participant View as DeviceChecksumView
participant Service as WHOISService
participant DB as Database
participant GeoIP as GeoIP2 (web)
participant Task as Celery:fetch_whois_details
participant GeoTask as Celery:manage_estimated_locations
participant Notif as Notifications
Device->>View: Report checksum / last_ip
View->>Service: update_whois_info() / process_ip_data_and_location()
Service->>DB: read org config (whois/estimated flags) & cached WHOISInfo
alt WHOIS lookup needed
Service->>Task: enqueue fetch_whois_details(device_pk, ip)
Task->>GeoIP: perform GeoIP lookup(ip)
alt GeoIP returns details
GeoIP-->>Task: whois_details
Task->>DB: create_or_update WHOISInfo
Task->>Service: trigger estimated-location decision
alt Estimated location enabled
Service->>GeoTask: enqueue manage_estimated_locations(device_pk, ip)
GeoTask->>DB: create/update Location (is_estimated=true) and DeviceLocation
GeoTask->>Notif: send estimated_location_info notification
end
else GeoIP error
Task->>Notif: send_whois_task_notification(error)
end
else skip WHOIS
end
View-->>Device: Respond checksum result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
openwisp_controller/geo/estimated_location/tests/utils.py (1)
8-15: Consider usingsuper().setUp()with flag override instead of skipping parent setUp.The current approach of calling
super(CreateWHOISMixin, self).setUp()to skipCreateWHOISMixin.setUp()works, but it's fragile if the inheritance chain changes.An alternative would be to add a flag to
CreateWHOISMixin.setUp()that allows subclasses to control whether to create theOrganizationConfigSettings, or to refactorCreateWHOISMixinto acceptestimated_location_enabledas a parameter.🔎 Alternative approach
class TestEstimatedLocationMixin(CreateWHOISMixin): + # Override to include estimated_location_enabled + estimated_location_enabled = True + def setUp(self): - super(CreateWHOISMixin, self).setUp() + # Skip CreateWHOISMixin.setUp() to avoid duplicate OrganizationConfigSettings + super(CreateWHOISMixin, self).setUp() # Call grandparent's setUp OrganizationConfigSettings.objects.create( organization=self._get_org(), whois_enabled=True, estimated_location_enabled=True, )Or refactor
CreateWHOISMixinto be more flexible with anestimated_location_enabledclass attribute.openwisp_controller/geo/api/filters.py (1)
25-26: Remove unused parameter or prefix with underscore.The
nameparameter is required by the django-filter method signature but not used in the implementation. Prefix it with an underscore to indicate it's intentionally unused.🔎 Proposed fix
- def filter_is_estimated(self, queryset, name, value): + def filter_is_estimated(self, queryset, _name, value): return queryset.filter(devicelocation__location__is_estimated=value)openwisp_controller/geo/estimated_location/tests/tests.py (1)
173-173: Consider removing unusednoqadirectives.Static analysis has identified several unused blanket
noqadirectives at lines 173, 287, 290, and 293. These can be safely removed for cleaner code.🔎 Proposed cleanup
- @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" # noqa - ) + @mock.patch( + "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" + )Apply similar changes to lines 287, 290, and 293.
Also applies to: 287-287, 290-290, 293-293
openwisp_controller/config/whois/utils.py (1)
44-55: Consider handlingObjectDoesNotExistexception.If
deviceis a primary key and the device doesn't exist in the database,Device.objects.get(pk=device)will raiseObjectDoesNotExist. Since this function is likely called from async tasks, unhandled exceptions could cause task failures without clear error messages.🔎 Proposed fix
def send_whois_task_notification(device, notify_type, actor=None): Device = load_model("config", "Device") if not isinstance(device, Device): - device = Device.objects.get(pk=device) + try: + device = Device.objects.get(pk=device) + except Device.DoesNotExist: + return notify_details = MESSAGE_MAP[notify_type] notify.send( sender=actor or device,openwisp_controller/geo/base/models.py (1)
19-19: Annotate mutable class attribute withClassVar.Per static analysis, mutable class attributes should be annotated with
typing.ClassVarto make it clear they're not instance attributes.🔎 Proposed fix
+from typing import ClassVar + class BaseLocation(OrgMixin, AbstractLocation): - _changed_checked_fields = ["is_estimated", "address", "geometry"] + _changed_checked_fields: ClassVar[list[str]] = ["is_estimated", "address", "geometry"]openwisp_controller/config/base/whois.py (2)
30-30: Redundantdb_index=Trueon primary key field.Primary key fields are automatically indexed, so
db_index=Trueis unnecessary.🔎 Proposed fix
- ip_address = models.GenericIPAddressField(db_index=True, primary_key=True) + ip_address = models.GenericIPAddressField(primary_key=True)
81-84: Use exception chaining for better debugging.Per static analysis, use
raise ... from eto preserve the original traceback when re-raising exceptions.🔎 Proposed fix
except ValueError as e: - raise ValidationError( + raise ValidationError( {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} - ) + ) from eopenwisp_controller/config/whois/tests/tests.py (1)
174-174: Replacegetattrwith direct attribute access.Per static analysis, using
getattrwith a constant attribute value provides no benefit over direct attribute access.🔎 Proposed fix
- self.assertEqual(getattr(org_settings_obj, "whois_enabled"), True) + self.assertEqual(org_settings_obj.whois_enabled, True)- self.assertEqual(getattr(org_settings_obj, "whois_enabled"), False) + self.assertEqual(org_settings_obj.whois_enabled, False)- self.assertEqual( - getattr(org_settings_obj, "whois_enabled"), - app_settings.WHOIS_ENABLED, - ) + self.assertEqual( + org_settings_obj.whois_enabled, + app_settings.WHOIS_ENABLED, + )Also applies to: 180-180, 191-191
openwisp_controller/config/whois/service.py (1)
167-179: Use exception chaining to preserve traceback context.The static analysis correctly identifies that the exception handling can be improved:
- Line 177: Use
raise ... from eto preserve the original exception context- Line 179: Use bare
raiseto re-raise the originalRequestExceptionunmodified🔎 Proposed fix
except ( errors.AddressNotFoundError, errors.AuthenticationError, errors.OutOfQueriesError, errors.PermissionRequiredError, ) as e: exc_type = type(e) message = EXCEPTION_MESSAGES.get(exc_type) if exc_type is errors.AddressNotFoundError: message = message.format(ip_address=ip_address) - raise exc_type(message) + raise exc_type(message) from e except requests.RequestException as e: - raise e + raise
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
openwisp_controller/config/static/whois/images/whois_globe.svgis excluded by!**/*.svg
📒 Files selected for processing (59)
docs/developer/extending.rstdocs/index.rstdocs/user/estimated-location.rstdocs/user/rest-api.rstdocs/user/settings.rstdocs/user/whois.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/management/__init__.pyopenwisp_controller/config/management/commands/__init__.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/migrations/0061_config_checksum_db.pyopenwisp_controller/config/migrations/0062_whoisinfo.pyopenwisp_controller/config/migrations/0063_organizationconfigsettings_approximate_location_enabled_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/static/whois/css/whois.cssopenwisp_controller/config/static/whois/js/whois.jsopenwisp_controller/config/templates/admin/config/change_form.htmlopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/whois/__init__.pyopenwisp_controller/config/whois/handlers.pyopenwisp_controller/config/whois/mixins.pyopenwisp_controller/config/whois/serializers.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/__init__.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/filters.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/__init__.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/__init__.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/0004_location_is_estimated.pyopenwisp_controller/geo/templates/admin/geo/location/change_form.htmlopenwisp_controller/geo/tests/test_admin.pyrequirements.txttests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.pytests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.pytests/openwisp2/sample_config/models.pytests/openwisp2/sample_geo/migrations/0004_location_is_estimated.pytests/openwisp2/settings.py
🧰 Additional context used
🧬 Code graph analysis (32)
openwisp_controller/config/models.py (2)
openwisp_controller/config/base/whois.py (1)
AbstractWHOISInfo(17-172)tests/openwisp2/sample_config/models.py (5)
WHOISInfo(117-123)Meta(22-23)Meta(31-32)Meta(40-41)Meta(49-50)
tests/openwisp2/sample_config/models.py (2)
openwisp_controller/config/base/whois.py (1)
AbstractWHOISInfo(17-172)openwisp_controller/config/models.py (10)
WHOISInfo(117-124)Meta(21-23)Meta(31-33)Meta(41-43)Meta(51-53)Meta(61-63)Meta(71-73)Meta(81-83)Meta(91-93)Meta(102-104)
openwisp_controller/config/api/serializers.py (1)
openwisp_controller/config/whois/mixins.py (2)
BriefWHOISMixin(23-24)WHOISMixin(5-20)
openwisp_controller/geo/api/views.py (1)
openwisp_controller/geo/admin.py (1)
queryset(168-182)
openwisp_controller/config/tests/test_device.py (1)
openwisp_controller/geo/base/models.py (1)
device(133-134)
openwisp_controller/geo/apps.py (1)
openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(8-41)
openwisp_controller/config/controller/views.py (3)
openwisp_controller/geo/base/models.py (1)
device(133-134)openwisp_controller/config/base/device.py (1)
whois_service(506-511)openwisp_controller/config/whois/service.py (1)
update_whois_info(269-280)
openwisp_controller/config/base/whois.py (2)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-351)get_cache_key(60-64)openwisp_controller/config/whois/tasks.py (1)
delete_whois_record(85-94)
openwisp_controller/geo/base/models.py (1)
openwisp_controller/config/base/device.py (1)
_set_initial_values_for_changed_checked_fields(130-135)
openwisp_controller/config/tests/test_api.py (1)
openwisp_controller/config/tests/utils.py (1)
assertNumQueries(390-394)
openwisp_controller/geo/estimated_location/tests/tests.py (6)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/geo/tests/utils.py (2)
TestGeoMixin(9-68)_create_location(35-37)openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(8-41)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(77-134)openwisp_controller/config/base/config.py (2)
full_clean(567-571)name(141-148)openwisp_controller/geo/base/models.py (2)
save(60-87)device(133-134)
openwisp_controller/geo/api/serializers.py (2)
openwisp_controller/geo/estimated_location/mixins.py (2)
EstimatedLocationGeoJsonSerializer(19-30)EstimatedLocationMixin(4-16)openwisp_controller/serializers.py (1)
BaseSerializer(5-12)
openwisp_controller/config/apps.py (1)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)
openwisp_controller/config/whois/mixins.py (4)
openwisp_controller/config/whois/serializers.py (2)
BriefWHOISSerializer(7-16)WHOISSerializer(19-28)openwisp_controller/config/base/device.py (1)
whois_service(506-511)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(233-242)openwisp_controller/config/whois/utils.py (1)
get_whois_info(58-78)
openwisp_controller/geo/estimated_location/tests/utils.py (1)
openwisp_controller/config/whois/tests/utils.py (2)
CreateWHOISMixin(14-41)setUp(37-41)
openwisp_controller/config/whois/tests/utils.py (3)
openwisp_controller/config/tests/utils.py (1)
CreateConfigMixin(56-73)openwisp_controller/config/base/device.py (1)
save(277-297)openwisp_controller/config/controller/views.py (4)
get(147-164)get(202-210)get(503-509)get(517-525)
openwisp_controller/geo/api/filters.py (2)
openwisp_controller/geo/admin.py (1)
queryset(168-182)openwisp_controller/config/base/config.py (1)
name(141-148)
openwisp_controller/config/base/device.py (1)
openwisp_controller/config/whois/service.py (3)
WHOISService(37-351)process_ip_data_and_location(244-267)update_whois_info(269-280)
tests/openwisp2/sample_geo/migrations/0004_location_is_estimated.py (2)
openwisp_controller/geo/migrations/0004_location_is_estimated.py (1)
Migration(6-21)tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py (1)
Migration(9-35)
openwisp_controller/config/whois/utils.py (3)
openwisp_controller/config/whois/mixins.py (1)
get_whois_info(16-20)openwisp_controller/config/whois/serializers.py (1)
WHOISSerializer(19-28)openwisp_controller/config/whois/service.py (2)
WHOISService(37-351)get_device_whois_info(233-242)
openwisp_controller/config/whois/tests/tests.py (3)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (4)
_create_whois_info(15-35)setUp(37-41)_task_called(60-155)_mocked_client_response(46-58)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(233-242)
openwisp_controller/config/whois/handlers.py (2)
openwisp_controller/config/models.py (3)
Device(16-23)WHOISInfo(117-124)OrganizationConfigSettings(96-104)openwisp_controller/config/base/whois.py (2)
device_whois_info_delete_handler(102-118)invalidate_org_settings_cache(123-129)
openwisp_controller/geo/estimated_location/mixins.py (2)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-351)check_estimate_location_configured(128-134)openwisp_controller/geo/base/models.py (1)
organization_id(137-138)
openwisp_controller/geo/tests/test_admin.py (2)
openwisp_controller/geo/tests/utils.py (1)
_create_location(35-37)openwisp_controller/tests/utils.py (1)
_login(17-18)
openwisp_controller/config/migrations/0062_whoisinfo.py (1)
openwisp_controller/config/migrations/0063_organizationconfigsettings_approximate_location_enabled_and_more.py (1)
Migration(9-35)
openwisp_controller/config/whois/service.py (2)
openwisp_controller/config/whois/tasks.py (1)
fetch_whois_details(40-81)openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(44-55)
openwisp_controller/config/migrations/0063_organizationconfigsettings_approximate_location_enabled_and_more.py (1)
openwisp_controller/config/migrations/0062_whoisinfo.py (1)
Migration(11-83)
tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py (2)
openwisp_controller/geo/migrations/0004_location_is_estimated.py (1)
Migration(6-21)tests/openwisp2/sample_geo/migrations/0004_location_is_estimated.py (1)
Migration(6-21)
openwisp_controller/config/base/multitenancy.py (2)
openwisp_controller/config/base/whois.py (1)
clean(66-99)openwisp_controller/geo/base/models.py (3)
clean(41-58)clean(96-101)clean(127-130)
openwisp_controller/geo/migrations/0004_location_is_estimated.py (1)
tests/openwisp2/sample_geo/migrations/0004_location_is_estimated.py (1)
Migration(6-21)
openwisp_controller/config/admin.py (2)
openwisp_controller/config/whois/utils.py (1)
get_whois_info(58-78)openwisp_controller/config/whois/mixins.py (1)
get_whois_info(16-20)
openwisp_controller/geo/admin.py (1)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-351)check_estimate_location_configured(128-134)
🪛 Biome (2.1.2)
openwisp_controller/config/static/whois/js/whois.js
[error] 3-3: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🪛 Ruff (0.14.10)
openwisp_controller/config/whois/tasks.py
40-40: Unused function argument: self
(ARG001)
openwisp_controller/config/base/whois.py
82-84: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
102-102: Unused static method argument: kwargs
(ARG004)
123-123: Unused static method argument: kwargs
(ARG004)
openwisp_controller/geo/base/models.py
19-19: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/management/commands/clear_last_ip.py
22-22: Unused method argument: args
(ARG002)
36-36: Avoid specifying long messages outside the exception class
(TRY003)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
13-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
17-79: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/geo/estimated_location/tests/tests.py
173-173: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
287-287: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
290-290: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
293-293: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
openwisp_controller/geo/api/filters.py
25-25: Unused method argument: name
(ARG002)
tests/openwisp2/sample_geo/migrations/0004_location_is_estimated.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/whois/tests/tests.py
174-174: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
180-180: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
191-191: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
openwisp_controller/config/migrations/0062_whoisinfo.py
12-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
16-83: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/whois/service.py
177-177: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
179-179: Use raise without specifying exception name
Remove exception name
(TRY201)
openwisp_controller/config/migrations/0063_organizationconfigsettings_approximate_location_enabled_and_more.py
11-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py
11-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/settings.py
83-85: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_controller/geo/migrations/0004_location_is_estimated.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/admin.py
570-574: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
1375-1375: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
1382-1382: Unused method argument: request
(ARG002)
1382-1382: Unused method argument: obj
(ARG002)
openwisp_controller/geo/admin.py
117-117: Consider (*fields, "is_estimated") instead of concatenation
Replace with (*fields, "is_estimated")
(RUF005)
155-155: Unused method argument: request
(ARG002)
155-155: Unused method argument: model_admin
(ARG002)
168-168: Unused method argument: request
(ARG002)
🔇 Additional comments (71)
requirements.txt (1)
19-19: Verify geoip2 version aligns with project Python version requirements.geoip2's latest version is 5.2.0, released Nov 20, 2025, but the PR pins version 5.1.0. Version 5.2.0 requires Python 3.10 or greater, while 5.1.0 requires Python 3.9+. No known security vulnerabilities were found in the geoip2 package, and the version specified here is safe.
If the project supports Python 3.10+, consider updating to
geoip2~=5.2.0to gain improvements including maxminddb 3.0.0 with free-threading support and a new build backend. Otherwise, the current constraint is acceptable for Python 3.9 environments.openwisp_controller/config/migrations/0061_config_checksum_db.py (1)
15-15: LGTM! Follows Django migration best practices.Using
apps.get_model()instead ofswapper.load_model()is the recommended Django approach for data migrations. This ensures the migration uses the historical model state and makes the migration more predictable and maintainable.openwisp_controller/config/apps.py (1)
32-32: LGTM!The WHOIS handlers are correctly integrated into the app initialization. The
connect_whois_handlers()function has an early return guard whenWHOIS_CONFIGUREDis False (as seen in the relevant snippet fromhandlers.py), so this won't affect deployments without WHOIS configured.Also applies to: 54-54
openwisp_controller/config/static/whois/css/whois.css (1)
1-113: LGTM!The CSS is well-structured with:
- CSS custom properties for maintainability
- Clean table styling with proper border-radius handling
- Accessible interactive states (hover on info icon)
- Modern mask-based icon approach for the globe
Note:
var(--border-color)on lines 38, 43, and 47 is assumed to be defined in the OpenWISP admin theme.openwisp_controller/config/management/commands/clear_last_ip.py (2)
22-22: The*argsparameter is conventional for Django management commands.The static analysis hint about unused
*argsis a false positive. This is the standard Django management command signature and maintaining it ensures compatibility with the base class contract.
38-48: Clarify the intended behavior: should the command clear last_ip for ALL active devices or selectively preserve those with WHOIS records?The implementation excludes devices with matching WHOISInfo records from the update, preserving their
last_ip. However, the help text states the command "Clears the last IP address (if set) for every active device across all organizations," which suggests it should clear all active devices regardless of WHOIS status.The test only covers devices without WHOIS data, so this selective behavior is untested. Either:
- Update the help text to document that devices with WHOIS records are preserved, or
- Remove the WHOIS exclusion (lines 40-46) to clear all active devices as the help text states.
docs/developer/extending.rst (1)
347-347: LGTM!The swapper configuration for
CONFIG_WHOISINFO_MODELis correctly documented, following the same pattern as other model configurations in this section.docs/index.rst (1)
51-52: LGTM!The new documentation pages for WHOIS and Estimated Location are correctly added to the table of contents, placed logically after feature documentation and before API/settings references.
openwisp_controller/config/tests/test_device.py (1)
527-532: LGTM!The test correctly extends the field change detection coverage to include
last_ip. The IP address used (172.217.22.14) is a valid public IP, which aligns with the WHOIS feature's requirement to track public IP addresses for geolocation lookup.openwisp_controller/config/controller/views.py (1)
156-158: LGTM. The code logic is correct. Theupdate_whois_info()method executes on every checksum request when the IP hasn't changed, and does query the database to fetch the existing WHOIS record. However, theis_whois_enabledcheck is cached (30-day timeout), and the method returns early if WHOIS is disabled. More importantly, the async task is only queued when the existing record is older than the refresh threshold (default 14 days), so the DB query impact in high-traffic scenarios is limited to a simple lookup that exits early in most cases.openwisp_controller/geo/apps.py (1)
11-11: LGTM!The import and registration call for estimated location notification types are properly placed. The function is invoked during app initialization in the
ready()method, which is the correct Django lifecycle hook for this type of setup.Also applies to: 26-26
tests/openwisp2/sample_config/models.py (1)
16-16: LGTM!The
WHOISInfotest model follows the same pattern as other concrete models in this file (Device, Config, Template, etc.), inheriting from bothDetailsModeland the abstract baseAbstractWHOISInfo. This is consistent with the swappable model pattern used throughout the codebase.Also applies to: 117-123
openwisp_controller/config/templates/admin/config/change_form.html (1)
22-24: LGTM!The conditional initialization of
deviceWHOISDetailsfollows the same pattern asowCommandApiEndpointabove it. The| safefilter is appropriate here since the value is serialized JSON from the server side, and the conditional check ensures the variable is only defined when the data is available.docs/user/estimated-location.rst (1)
1-87: LGTM!The documentation is comprehensive and well-structured. It clearly explains:
- Feature defaults and prerequisites
- Trigger conditions for the feature
- Behavior with different device/location scenarios
- Visibility of estimated status in UI and API
- Management of older records
The writing is clear and includes appropriate cross-references to other documentation sections.
openwisp_controller/geo/tests/test_admin.py (1)
140-148: LGTM!The test correctly verifies that the
is_estimatedcheckbox is not present in the Location admin change form when WHOIS is not configured. The test follows the established patterns in this file and uses appropriate helper methods.openwisp_controller/geo/migrations/0004_location_is_estimated.py (1)
6-21: LGTM!The migration correctly adds the
is_estimatedBooleanField to the Location model with appropriate defaults and help text. The structure follows standard Django migration patterns.Note: The Ruff static analysis hints about
ClassVarannotations are false positives—Django migrations use plain class attributes fordependenciesandoperations, and this is the standard and correct pattern.openwisp_controller/config/tests/test_api.py (1)
253-255: LGTM!The tests now consistently patch
WHOIS_CONFIGUREDtoFalsewhen checking query counts. This ensures the tests have predictable and stable query counts regardless of whether the WHOIS feature is enabled in the test environment, which is important for maintaining reliable test assertions.The pattern is applied consistently across all three affected tests.
Also applies to: 401-403, 413-415
docs/user/rest-api.rst (2)
71-86: LGTM! Clear documentation of new WHOIS and estimated location features.The documentation properly describes the conditional availability of WHOIS details and estimated location filters, with appropriate references to the feature documentation pages.
555-573: Good use of RST replacement directives to avoid duplication.The
|est_loc|replacement pattern allows consistent referencing of the Estimated Location feature across multiple endpoints without duplicating text.tests/openwisp2/settings.py (2)
211-217: LGTM! Proper configuration for GEOIP settings.The environment variable pattern with empty string defaults is appropriate for optional WHOIS feature configuration.
274-274: LGTM! Swappable model mapping follows existing pattern.The CONFIG_WHOISINFO_MODEL mapping is consistent with other swappable model configurations in this test settings file.
openwisp_controller/config/models.py (2)
13-13: LGTM! Import follows the established pattern.The import of
AbstractWHOISInfois consistent with other abstract model imports in this file.
117-124: LGTM! Swappable model implementation follows established pattern.The WHOISInfo model correctly implements the swappable pattern used throughout OpenWISP, consistent with other models like Device, Config, and Template.
docs/user/settings.rst (1)
765-842: LGTM! Comprehensive and well-structured documentation.The WHOIS-related settings documentation is thorough, including:
- Clear descriptions and default values
- Proper warnings about configuration requirements
- Visual aids (screenshots)
- Cross-references to related features
The documentation quality is consistent with the rest of the file.
openwisp_controller/geo/api/views.py (2)
13-13: LGTM! Import added for WHOIS configuration check.The import of
config_app_settingsis necessary for the conditional filter logic below.
49-54: Backward compatibility is not at risk; the__init__signature change is safe.DjangoFilterBackend (from
django_filters.rest_framework) instantiates filtersets internally using keyword arguments only. The filter is never directly instantiated with positional arguments—it appears exclusively asfilterset_classin DRF views. The*enforcement for keyword-only arguments is fully compatible with this integration pattern.The conditional addition of the
is_estimatedfilter when WHOIS is configured is a clean feature-toggling approach.docs/user/whois.rst (1)
1-128: LGTM! Excellent comprehensive documentation for the WHOIS feature.This documentation is well-structured and covers all essential aspects:
- Clear indication that the feature is disabled by default
- Detailed setup instructions with MaxMind integration
- Comprehensive trigger conditions and record management logic
- Multiple viewing locations with screenshots
- Proper cross-references to related settings
The documentation quality is excellent and will help users understand and configure this feature effectively.
openwisp_controller/geo/templates/admin/geo/location/change_form.html (1)
1-11: Theestimated_configuredcontext variable is properly provided.The variable is set in the
LocationAdmin.change_viewmethod atopenwisp_controller/geo/admin.py:123-125. It's calculated viaWHOISService.check_estimate_location_configured(org_id)and passed to the template throughextra_context, ensuring the template has access to it.openwisp_controller/geo/estimated_location/utils.py (1)
1-6: Acknowledge the reliance on private APIs from openwisp_notifications, but note this is an established pattern in the codebase.This function uses two private APIs from
openwisp_notifications:
_get_object_link()- private function fromopenwisp_notifications.utilsobj._related_object(field)- private method on notification objectsWhile the leading underscore indicates these are private/internal APIs, this same pattern is consistently used in other notification target URL functions across the codebase (
connection/utils.pyandconfig/utils.py), suggesting this is an intentional design. However, be aware that relying on private APIs carries the risk of breakage if theopenwisp_notificationspackage implementation changes.openwisp_controller/geo/api/filters.py (1)
28-37: LGTM! Clean conditional filter registration.The dynamic filter registration correctly gates the
geo_is_estimatedfilter behind theWHOIS_CONFIGUREDsetting, maintaining consistency with the admin filter implementation.openwisp_controller/config/api/serializers.py (1)
12-12: LGTM! Clean mixin integration.The WHOIS mixins are correctly integrated into the serializer inheritance chain, enabling conditional WHOIS data exposure in API responses without modifying existing serializer logic.
Also applies to: 224-224, 278-278
tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py (1)
9-35: LGTM! Standard Django migration.The migration correctly adds the
estimated_location_enabledtoggle andcoordinatesfield to support the estimated location feature. The Ruff ClassVar warnings are false positives—Django migration class attributes follow established framework patterns.tests/openwisp2/sample_geo/migrations/0004_location_is_estimated.py (1)
6-21: LGTM! Standard test migration.The migration correctly mirrors the main app's
is_estimatedfield addition. The Ruff ClassVar warnings are false positives for Django migrations.openwisp_controller/config/whois/handlers.py (1)
7-29: LGTM! Clean conditional signal wiring.The signal handler registration correctly:
- Guards against unnecessary wiring when WHOIS is not configured
- Uses unique
dispatch_uidvalues to prevent duplicate connections- Wires appropriate cleanup and cache invalidation handlers
openwisp_controller/config/migrations/0062_whoisinfo.py (1)
11-83: LGTM! Well-structured WHOIS model migration.The migration correctly:
- Creates the
WHOISInfomodel with appropriate fields for WHOIS data storage- Uses
ip_addressas the primary key, which is suitable for this use case- Implements the swappable pattern for extensibility
- Adds the
whois_enabledtoggle to organization settingsThe Ruff ClassVar warnings are false positives for Django migrations.
openwisp_controller/config/whois/mixins.py (1)
5-24: LGTM! Well-designed serializer mixins.The WHOIS mixins correctly:
- Conditionally inject
whois_infointo device representations based on configuration- Handle missing WHOIS data gracefully by returning
None- Provide both detailed (
WHOISMixin) and brief (BriefWHOISMixin) variants- Use proper double-gating (configuration + per-device enablement)
openwisp_controller/geo/estimated_location/mixins.py (2)
4-16: LGTM! Clean conditional field exposure.The
EstimatedLocationMixincorrectly conditionally includes theis_estimatedfield based on organization configuration, ensuring the field is only exposed when the estimated location feature is enabled.
19-30: LGTM! GeoJSON-specific mixin correctly handles properties structure.The
super(EstimatedLocationMixin, self)call on line 25 intentionally skipsEstimatedLocationMixin.to_representationto avoid double-processing, then correctly placesis_estimatedinto the GeoJSONpropertiesobject. This pattern accommodates the different structure of GeoJSON serialization where fields belong inpropertiesrather than at the top level.openwisp_controller/config/whois/utils.py (1)
58-78: LGTM!The
get_whois_infofunction has proper guard clauses for configuration checks, device existence, and organization settings. The use ofselect_relatedfor prefetching is appropriate.openwisp_controller/config/base/multitenancy.py (2)
37-46: LGTM!The
FallbackBooleanChoiceFieldusage with proper fallbacks to app settings is appropriate for organization-level overrides.
69-88: LGTM!The validation logic correctly enforces the dependency chain: WHOIS must be configured before enabling, and estimated location requires WHOIS to be enabled. The error messages are clear and actionable.
openwisp_controller/geo/base/models.py (1)
60-87: LGTM with note on the save logic.The save method correctly handles the estimated location flag, clearing it when address/geometry changes manually. The
_set_estimatedparameter provides a clean way to differentiate system vs manual updates.openwisp_controller/config/settings.py (1)
71-85: LGTM!The WHOIS configuration settings are well-structured with proper validation. The dependency chain (WHOIS_CONFIGURED → WHOIS_ENABLED → ESTIMATED_LOCATION_ENABLED) is correctly enforced at startup via
ImproperlyConfiguredexceptions.openwisp_controller/geo/estimated_location/tasks.py (2)
11-73: LGTM!The helper function correctly handles three scenarios: attaching existing location, updating current location, and creating new location. The explicit check
if attached_devices_exists is Falseis appropriate sinceNonehas different semantics.
76-134: LGTM!The task correctly handles edge cases including multiple devices with the same IP (proxy scenarios) with appropriate error logging and notifications. The logic flow is clear and well-documented.
openwisp_controller/config/base/whois.py (2)
17-99: LGTM on the model design.Using
ip_addressas the primary key is a good design choice to avoid duplicate WHOIS records. The validation inclean()properly handles private IPs, CIDR format, and coordinate ranges.
101-129: LGTM!The static handlers correctly manage WHOIS lifecycle. The
**kwargsparameters are required for Django signal handler compatibility even if unused.openwisp_controller/config/whois/tests/utils.py (2)
14-41: LGTM!The
CreateWHOISMixinprovides a clean factory method with sensible defaults, and thesetUpcorrectly initializes the organization config settings for WHOIS-enabled tests.
44-155: LGTM!The
WHOISTransactionMixinprovides comprehensive test coverage for various WHOIS task scenarios including public/private IPs, cache interactions, and DeviceChecksumView integrations. The subtests are well-organized and descriptive.openwisp_controller/config/whois/tests/tests.py (4)
33-126: LGTM!Excellent test coverage for WHOIS configuration settings including signal connection/disconnection, admin UI field visibility, and ImproperlyConfigured exception handling.
286-340: LGTM!Comprehensive validation tests for WHOISInfo model fields covering ISP length, private IP rejection, timezone length, CIDR format, ASN, and coordinate range validations.
342-680: LGTM!The transaction tests thoroughly cover WHOIS task triggering, creation, update, and deletion scenarios across multiple organizations. The failure notification tests properly verify error handling and logging.
682-753: LGTM!The Selenium tests properly verify UI visibility of WHOIS details under various configuration states. The
_assert_no_js_errorshelper ensures no JavaScript errors occur during UI interactions.openwisp_controller/config/admin.py (6)
34-34: LGTM!Import for
get_whois_infois correctly added to support WHOIS data in the admin context.
53-53: LGTM!The
whois_prefixvariable follows the same pattern as the existingprefixvariable for organizing static assets.
565-579: LGTM! Good approach for test isolation.Converting from
class Mediato a property allows runtime evaluation ofapp_settings.WHOIS_CONFIGURED, enabling proper test isolation. The conditional inclusion of WHOIS assets is clean.
933-936: LGTM!Clean implementation that embeds WHOIS details directly in the context, avoiding an extra frontend request. The walrus operator usage is appropriate here.
1373-1389: LGTM!The
ConfigSettingsInlinedynamically includes fields based on feature configuration, which is a clean approach. Theget_fieldsmethod correctly gates WHOIS-related fields behindWHOIS_CONFIGUREDand registration fields behindREGISTRATION_ENABLED.Note: The static analysis hints about unused
requestandobjparameters are false positives—these are required by Django's admin API signature.
1392-1394: LGTM!The
ConfigSettingsInlineis correctly inserted at position 0, andlimits_inline_positionis updated to maintain proper ordering.openwisp_controller/geo/admin.py (4)
15-16: LGTM!Imports for
config_app_settingsandWHOISServiceare correctly added to support the WHOIS-aware behavior in the geo admin.
150-154: LGTM!The
__init__override correctly updates the filter title when WHOIS is configured. The unused parameter warnings are false positives—these are required by Django'sSimpleListFilterAPI.
155-182: LGTM with a note on filter logic.The filter logic correctly handles:
- Estimated: Filters locations marked as estimated
- Indoor/Outdoor: Filters by type while excluding estimated locations
- No Location / False: Falls through to the existing
devicelocation__isnullcheckThe early return on lines 169-171 prevents processing when no filter value is selected, which is appropriate.
105-111: No issue. Django'sModelAdmin.get_fields()returns a list, not a tuple, so callingfields.remove()is safe and correct. Tests confirm this code works as intended.openwisp_controller/config/whois/service.py (8)
19-34: LGTM!The
EXCEPTION_MESSAGESmapping provides clear, actionable error messages for different GeoIP2 exceptions, which will help users diagnose configuration issues.
45-57: LGTM!The
get_geoip_clientfactory method correctly configures the GeoIP2 web service client with thegeolite.infohost for GeoLite2 database access.
66-74: LGTM!The
is_valid_public_ip_addressmethod correctly uses Python'sipaddressmodule to check if an IP is globally routable, with proper exception handling for invalid IP formats.
183-200: LGTM!The response data extraction correctly handles potentially
Nonevalues with appropriate fallbacks. ThePointis correctly constructed with SRID 4326 (WGS84), which is standard for GPS coordinates.
202-220: LGTM!The
_need_whois_lookupmethod correctly orders conditions from cheapest to most expensive:
- Validate IP address (in-memory check)
- Check if existing WHOIS data is fresh (single DB query)
- Check organization settings (potentially cached)
This optimization avoids unnecessary database queries.
244-267: LGTM!Using
transaction.on_commitensures tasks are only scheduled after the transaction successfully commits, preventing orphaned tasks or race conditions. The separation between WHOIS lookup and estimated location management is well-handled.
282-301: LGTM!The
_create_or_update_whoismethod correctly:
- Tracks which fields changed for selective updates
- Uses
save(update_fields=...)for efficient partial updates- Calls
full_clean()for validation on new instances- Uses
force_insert=Truefor new records
303-350: LGTM with observation on notification placement.The method correctly handles three scenarios:
- No location exists: Create new estimated location
- Shared estimated location: Create new location for this device
- Device's own estimated location: Update in place
The notification on line 345 is sent outside the
transaction.atomic()block, which means DB changes are committed even if notification fails. This is likely intentional to avoid rollback on notification failure.
| $parentDiv.after( | ||
| `<table class="whois-table"> | ||
| <tr> | ||
| <th> | ||
| <div> | ||
| <span>${gettext("ISP")}</span> | ||
| <span title="${tooltipText}" class="ow-info-icon icon"></span> | ||
| </div> | ||
| </th> | ||
| <th>${gettext("Country")}</th> | ||
| </tr> | ||
| <tr> | ||
| <td>${deviceWHOISDetails.isp}</td> | ||
| <td>${deviceWHOISDetails.address.country}</td> | ||
| </tr> | ||
| </table> | ||
| <details class="whois"> | ||
| <summary> | ||
| <span class="whois-globe"></span> | ||
| <div> | ||
| <span>${gettext("Additional Details")}</span><span class="mg-arrow"></span> | ||
| </div> | ||
| </summary> | ||
| <div> | ||
| <span class="additional-text">${gettext("ASN")}: ${deviceWHOISDetails.asn}</span> | ||
| <span class="additional-text">${gettext("Timezone")}: ${deviceWHOISDetails.timezone}</span> | ||
| <span class="additional-text">${gettext("Address")}: ${deviceWHOISDetails.formatted_address}</span> | ||
| <span class="additional-text">${gettext("CIDR")}: ${deviceWHOISDetails.cidr}</span> | ||
| </div> | ||
| </details>`, | ||
| ); |
There was a problem hiding this comment.
Potential XSS vulnerability: HTML-escape dynamic content.
The code directly interpolates deviceWHOISDetails fields into HTML using template literals without escaping. If any WHOIS data (ISP names, addresses, CIDR, etc.) contains HTML special characters or malicious content, this could lead to XSS vulnerabilities.
🔎 Recommended fix: HTML-escape the interpolated values
Add an HTML escape utility function and use it for all dynamic content:
+"use strict";
+
+if (typeof gettext === "undefined") {
+ var gettext = function (word) {
+ return word;
+ };
+}
+
+// HTML escape utility
+function escapeHtml(text) {
+ if (text == null) return '';
+ const div = document.createElement('div');
+ div.textContent = text;
+ return div.innerHTML;
+}
+
django.jQuery(function ($) {
const $addForm = $(".add-form");
const $deviceForm = $("#device_form");
if (
$addForm.length ||
!$deviceForm.length ||
typeof deviceWHOISDetails === "undefined"
) {
return;
}
const $parentDiv = $("#overview-group .field-last_ip div:last");
const tooltipText = gettext(
"This is the Organization associated with registered ASN",
);
$parentDiv.after(
`<table class="whois-table">
<tr>
<th>
<div>
<span>${gettext("ISP")}</span>
<span title="${tooltipText}" class="ow-info-icon icon"></span>
</div>
</th>
<th>${gettext("Country")}</th>
</tr>
<tr>
- <td>${deviceWHOISDetails.isp}</td>
- <td>${deviceWHOISDetails.address.country}</td>
+ <td>${escapeHtml(deviceWHOISDetails.isp)}</td>
+ <td>${escapeHtml(deviceWHOISDetails.address.country)}</td>
</tr>
</table>
<details class="whois">
<summary>
<span class="whois-globe"></span>
<div>
<span>${gettext("Additional Details")}</span><span class="mg-arrow"></span>
</div>
</summary>
<div>
- <span class="additional-text">${gettext("ASN")}: ${deviceWHOISDetails.asn}</span>
- <span class="additional-text">${gettext("Timezone")}: ${deviceWHOISDetails.timezone}</span>
- <span class="additional-text">${gettext("Address")}: ${deviceWHOISDetails.formatted_address}</span>
- <span class="additional-text">${gettext("CIDR")}: ${deviceWHOISDetails.cidr}</span>
+ <span class="additional-text">${gettext("ASN")}: ${escapeHtml(deviceWHOISDetails.asn)}</span>
+ <span class="additional-text">${gettext("Timezone")}: ${escapeHtml(deviceWHOISDetails.timezone)}</span>
+ <span class="additional-text">${gettext("Address")}: ${escapeHtml(deviceWHOISDetails.formatted_address)}</span>
+ <span class="additional-text">${gettext("CIDR")}: ${escapeHtml(deviceWHOISDetails.cidr)}</span>
</div>
</details>`,
);
});Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@DragnEmperor Are we sure HTML code cannot be injected in the fields of the WHOIS?
Can you add a specific security test for this? If we handle this in the backend we don't have to worry about it in the frontend.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def change_view(self, request, object_id, form_url="", extra_context=None): | ||
| obj = self.get_object(request, object_id) | ||
| org_id = obj.organization_id if obj else None | ||
| estimated_configured = WHOISService.check_estimate_location_configured(org_id) | ||
| extra_context = {"estimated_configured": estimated_configured} | ||
| return super().change_view(request, object_id, form_url, extra_context) |
There was a problem hiding this comment.
extra_context is overwritten instead of merged.
The method unconditionally assigns a new dict to extra_context, discarding any context passed by callers. This deviates from Django's convention and the parent class pattern.
🔎 Proposed fix
def change_view(self, request, object_id, form_url="", extra_context=None):
obj = self.get_object(request, object_id)
org_id = obj.organization_id if obj else None
estimated_configured = WHOISService.check_estimate_location_configured(org_id)
- extra_context = {"estimated_configured": estimated_configured}
+ extra_context = extra_context or {}
+ extra_context["estimated_configured"] = estimated_configured
return super().change_view(request, object_id, form_url, extra_context)🤖 Prompt for AI Agents
In openwisp_controller/geo/admin.py around lines 120 to 125, the change_view
implementation currently overwrites the incoming extra_context instead of
merging with it; preserve any existing keys by creating or copying the incoming
extra_context (e.g., ctx = dict(extra_context) if extra_context else {}),
add/overwrite the "estimated_configured" key on that dict, then pass the merged
ctx to super().change_view so caller-provided context is retained.
There was a problem hiding this comment.
@DragnEmperor I think it makes sense to use the proposed fix here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| estimated_string = gettext("Estimated Location") | ||
| if self.name and estimated_string in self.name: | ||
| # remove string starting with "(Estimated Location" | ||
| self.name = re.sub( | ||
| rf"\s\({estimated_string}.*", "", self.name, flags=re.IGNORECASE | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "Estimated Location" --type py -A 3 -B 3Repository: openwisp/openwisp-controller
Length of output: 21850
🏁 Script executed:
# Find the file and get more context around lines 79-84
fd "models.py" -p "*/geo/base/*" -x cat -n {} | sed -n '70,95p'Repository: openwisp/openwisp-controller
Length of output: 171
🏁 Script executed:
# Search for how the name field is set and where gettext is imported
rg -n "from.*gettext|import.*gettext" --type py openwisp_controller/geo/Repository: openwisp/openwisp-controller
Length of output: 740
🏁 Script executed:
# Check if there are tests related to this functionality
fd "test.*\.py" -p "*/geo/*" -x rg -l "Estimated Location" {}Repository: openwisp/openwisp-controller
Length of output: 166
🏁 Script executed:
# Read the models.py file around lines 79-84 to see full context
cat -n openwisp_controller/geo/base/models.py | sed -n '1,100p'Repository: openwisp/openwisp-controller
Length of output: 4422
🏁 Script executed:
# Check the whois.py file to see how the estimated location name is created
cat -n openwisp_controller/config/base/whois.py | sed -n '150,170p'Repository: openwisp/openwisp-controller
Length of output: 1019
🏁 Script executed:
# Check how location names are set in tests to confirm the pattern
rg -n "Estimated Location:" openwisp_controller/config/base/whois.py -B 5 -A 5Repository: openwisp/openwisp-controller
Length of output: 640
Fix locale-independent string matching for estimated location removal.
The location name is created with the full translated string: _(f"{location} (Estimated Location: {self.ip_address})"). When saved to the database, this is evaluated in the current locale. Later, when save() is called, the code tries to match the pattern using gettext("Estimated Location") in the current locale. If the name was created in a different locale (e.g., French), the stored name will contain the translated marker (e.g., "Localisation estimée"), and the regex won't match, leaving the estimated location marker in the name.
Use a locale-independent approach: either store an untranslated marker with the location name, or use a fixed character/token that doesn't depend on translation.
🤖 Prompt for AI Agents
openwisp_controller/geo/base/models.py around lines 79-84: the current removal
of the estimated-location suffix relies on gettext("Estimated Location") which
fails if the stored name was created under a different locale; change to a
locale-independent scheme by storing an untranslated/fixed marker when composing
the name (for example append a constant token/delimiter such as " [ESTIMATED]"
or embed the raw IP in a fixed format) and update the save() cleanup to strip
that fixed token via a regex that matches the token (not a translated string),
or alternatively store the estimated note in a separate DB field and stop
embedding localized text in the name; implement one of these changes and update
creation and save code paths so both writing and cleanup use the same
locale-independent marker.
There was a problem hiding this comment.
This is a good point actually.. Maybe we can look for something slightly different and language agnostic, eg:
We can write something like: ~Estimated Location~, then we can use a regex which simply removes anything between the tilde sign ~ (which is not as common as rounded parenthesis.
There was a problem hiding this comment.
@nemesifier That's a great solution! Using ~Estimated Location~ with tildes as delimiters is indeed a cleaner, locale-independent approach. The tilde character is uncommon in location names, making it a good choice for a marker.
For the implementation, you'll need to update two locations in the codebase:
-
Creating the marker (in
openwisp_controller/config/base/whois.pyin the_location_nameproperty):- Change from:
_(f"{location} (Estimated Location: {self.ip_address})") - To something like:
f"{location} ~Estimated Location: {self.ip_address}~" - Note: No translation needed for the marker text since it's being stripped anyway
- Change from:
-
Removing the marker (in
openwisp_controller/geo/base/models.pyin thesavemethod, lines 79-84):- Replace the current logic with a simple regex pattern:
if self.name: # Remove anything between tildes (estimated location marker) self.name = re.sub(r"~[^~]*~", "", self.name).strip()
- The pattern
~[^~]*~matches a tilde, followed by any characters except tilde, followed by another tilde - Alternatively, you could use
r"~.*?~"(non-greedy match), but[^~]*is more explicit and slightly more efficient
This approach completely avoids the locale dependency issue and is simpler to maintain! 🎯
nemesifier
requested review from
codesankalp,
devkapilbansal,
nemesifier and
pandafy
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openwisp_controller/config/admin.py (1)
574-588: LGTM!Converting
Mediato a property enables dynamic asset loading based onWHOIS_CONFIGURED, and the comment correctly explains the testability benefit.The static analysis tool suggests using iterable unpacking for better performance (line 579-583):
🔎 Optional refactor
@property def media(self): - js = BaseConfigAdmin.Media.js + [ + js = [ + *BaseConfigAdmin.Media.js, f"{prefix}js/tabs.js", f"{prefix}js/management_ip.js", f"{prefix}js/relevant_templates.js", ] css = BaseConfigAdmin.Media.css["all"] if app_settings.WHOIS_CONFIGURED: js.append(f"{whois_prefix}js/whois.js") css += (f"{whois_prefix}css/whois.css",) return super().media + forms.Media(js=js, css={"all": css})
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openwisp_controller/config/admin.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/controller/views.pyrequirements.txt
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_controller/config/controller/views.py (3)
openwisp_controller/geo/base/models.py (1)
device(133-134)openwisp_controller/config/base/device.py (1)
whois_service(525-530)openwisp_controller/config/whois/service.py (1)
update_whois_info(269-280)
openwisp_controller/config/base/device.py (1)
openwisp_controller/config/whois/service.py (3)
WHOISService(37-351)process_ip_data_and_location(244-267)update_whois_info(269-280)
openwisp_controller/config/admin.py (3)
openwisp_controller/config/whois/utils.py (1)
get_whois_info(58-78)openwisp_controller/config/whois/mixins.py (1)
get_whois_info(16-20)openwisp_controller/config/models.py (11)
Meta(21-23)Meta(31-33)Meta(41-43)Meta(51-53)Meta(61-63)Meta(71-73)Meta(81-83)Meta(91-93)Meta(102-104)Meta(112-114)OrganizationConfigSettings(96-104)
🪛 Ruff (0.14.10)
openwisp_controller/config/admin.py
579-583: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
1384-1384: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
1391-1391: Unused method argument: request
(ARG002)
1391-1391: Unused method argument: obj
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.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~=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.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (12)
requirements.txt (1)
19-19: Dependency addition looks good.The geoip2 package provides an API for the GeoIP2 and GeoLite2 web services and databases, which aligns with the PR's WHOIS and geolocation features. The version pinning strategy (~=5.1.0) is consistent with the rest of the file.
Note: Version 5.2.0 is available and newer, but requires Python 3.10+, while 5.1.0 requires Python 3.9 or greater. Verify your project's Python version support to determine if upgrading to 5.2.0 in the future would be beneficial.
openwisp_controller/config/base/device.py (6)
1-1: LGTM!The import is necessary for the
whois_servicecached property added later in the file.
22-22: LGTM!The import is necessary for the WHOIS integration added in this PR.
289-290: LGTM!The placement after
super().save()is correct, ensuring the device is persisted before processing WHOIS data. The conditional check andcreatingparameter are appropriate.
307-313: LGTM! Defensive improvement.The callable check prevents
AttributeErrorwhen dynamically invoking field-specific change handlers. This is a good defensive coding practice.
524-530: LGTM!The
cached_propertydecorator ensures theWHOISServiceinstance is created once and reused, which is appropriate for this stateless service wrapper.
532-543: LGTM!The method correctly:
- Guards against deferred fields
- Distinguishes between IP changes (triggers full processing) and no change (checks for stale data)
- Updates the initial value for subsequent comparisons
openwisp_controller/config/admin.py (4)
34-34: LGTM!The import is necessary for injecting WHOIS details into the admin context.
53-53: LGTM!The constant follows the existing pattern for organizing static asset paths.
942-945: LGTM!The walrus operator usage is appropriate, and the comment clearly explains the optimization of avoiding an additional AJAX request from JavaScript.
1382-1403: LGTM!The form and inline implementation correctly expose organization-level WHOIS and registration settings. The dynamic
get_fieldsmethod appropriately shows fields based on feature configuration.Note: The static analysis warnings about unused method parameters (line 1391) are false positives—these parameters are part of Django's admin method signature.
openwisp_controller/config/controller/views.py (1)
156-158: The checksum view is cached for 30 days via@cache_memoize, so the WHOIS check does not run frequently. The view logic only executes on cache misses (first request or after invalidation), not on every request. Additionally,update_whois_info()internally caches WHOIS data with the same 30-day timeout. The performance concern is not valid given the existing caching infrastructure.Likely an incorrect or invalid review comment.
| # Initial value for last_ip is required in WHOIS | ||
| # to remove WHOIS info related to that ip address. | ||
| if app_settings.WHOIS_CONFIGURED: | ||
| self._changed_checked_fields.append("last_ip") | ||
|
|
There was a problem hiding this comment.
Critical: Modifying class-level list causes shared state bug.
Line 126 appends to self._changed_checked_fields, but _changed_checked_fields is a class attribute (defined at line 33). Using .append() mutates the shared list across all instances, causing duplicate entries after multiple device instantiations.
🔎 Proposed fix
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
- # Initial value for last_ip is required in WHOIS
- # to remove WHOIS info related to that ip address.
- if app_settings.WHOIS_CONFIGURED:
- self._changed_checked_fields.append("last_ip")
-
self._set_initial_values_for_changed_checked_fields()Then update line 33 to conditionally include last_ip:
- _changed_checked_fields = ["name", "group_id", "management_ip", "organization_id"]
+ _changed_checked_fields = ["name", "group_id", "management_ip", "organization_id"]
+
+ def __init__(self, *args, **kwargs):
+ # Conditionally add last_ip to checked fields for this instance
+ if app_settings.WHOIS_CONFIGURED and "last_ip" not in self._changed_checked_fields:
+ self._changed_checked_fields = self._changed_checked_fields + ["last_ip"]
+ super().__init__(*args, **kwargs)
- self._set_initial_values_for_changed_checked_fields()
+ self._set_initial_values_for_changed_checked_fields()Or better yet, make it a class-level decision:
_changed_checked_fields = ["name", "group_id", "management_ip", "organization_id"]
if app_settings.WHOIS_CONFIGURED:
_changed_checked_fields = _changed_checked_fields + ["last_ip"]📝 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.
| # Initial value for last_ip is required in WHOIS | |
| # to remove WHOIS info related to that ip address. | |
| if app_settings.WHOIS_CONFIGURED: | |
| self._changed_checked_fields.append("last_ip") | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| # Create instance copy to avoid modifying class attribute | |
| if app_settings.WHOIS_CONFIGURED: | |
| self._changed_checked_fields = self._changed_checked_fields + ["last_ip"] | |
| else: | |
| self._changed_checked_fields = self._changed_checked_fields.copy() | |
| self._set_initial_values_for_changed_checked_fields() |
There was a problem hiding this comment.
This is also something to be wary of.
There was a problem hiding this comment.
@nemesifier, absolutely correct. This is a critical shared-state bug that will cause "last_ip" to be appended multiple times to the class-level list across all Device instances.
Recommended fix:
Since app_settings.WHOIS_CONFIGURED is determined at module load time (based on the settings in openwisp_controller/config/settings.py), the cleanest solution is to make this a class-level decision rather than instance-level:
_changed_checked_fields = ["name", "group_id", "management_ip", "organization_id"]
if app_settings.WHOIS_CONFIGURED:
_changed_checked_fields = _changed_checked_fields + ["last_ip"]This should be placed at the class level (around line 33), and the problematic __init__ code block (lines 121-126) should be removed entirely.
Why this approach:
- ✅ No mutation of shared state
- ✅ No per-instance overhead
- ✅ Single source of truth established at class definition time
- ✅ No conditional logic needed in
__init__
The alternative of creating instance copies would work but adds unnecessary overhead since WHOIS_CONFIGURED doesn't change per-instance or at runtime.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This is almost ready, we have tested it enough to give me confidence to deploy it on a prod system so we can see how it behaves with real data (doing this now).
I asked Code Rabbit to help me review this and I think it spotted 3 minor issues that will be good to double check before merging.
| $parentDiv.after( | ||
| `<table class="whois-table"> | ||
| <tr> | ||
| <th> | ||
| <div> | ||
| <span>${gettext("ISP")}</span> | ||
| <span title="${tooltipText}" class="ow-info-icon icon"></span> | ||
| </div> | ||
| </th> | ||
| <th>${gettext("Country")}</th> | ||
| </tr> | ||
| <tr> | ||
| <td>${deviceWHOISDetails.isp}</td> | ||
| <td>${deviceWHOISDetails.address.country}</td> | ||
| </tr> | ||
| </table> | ||
| <details class="whois"> | ||
| <summary> | ||
| <span class="whois-globe"></span> | ||
| <div> | ||
| <span>${gettext("Additional Details")}</span><span class="mg-arrow"></span> | ||
| </div> | ||
| </summary> | ||
| <div> | ||
| <span class="additional-text">${gettext("ASN")}: ${deviceWHOISDetails.asn}</span> | ||
| <span class="additional-text">${gettext("Timezone")}: ${deviceWHOISDetails.timezone}</span> | ||
| <span class="additional-text">${gettext("Address")}: ${deviceWHOISDetails.formatted_address}</span> | ||
| <span class="additional-text">${gettext("CIDR")}: ${deviceWHOISDetails.cidr}</span> | ||
| </div> | ||
| </details>`, | ||
| ); |
There was a problem hiding this comment.
@DragnEmperor Are we sure HTML code cannot be injected in the fields of the WHOIS?
Can you add a specific security test for this? If we handle this in the backend we don't have to worry about it in the frontend.
| estimated_string = gettext("Estimated Location") | ||
| if self.name and estimated_string in self.name: | ||
| # remove string starting with "(Estimated Location" | ||
| self.name = re.sub( | ||
| rf"\s\({estimated_string}.*", "", self.name, flags=re.IGNORECASE | ||
| ) |
There was a problem hiding this comment.
This is a good point actually.. Maybe we can look for something slightly different and language agnostic, eg:
We can write something like: ~Estimated Location~, then we can use a regex which simply removes anything between the tilde sign ~ (which is not as common as rounded parenthesis.
| def change_view(self, request, object_id, form_url="", extra_context=None): | ||
| obj = self.get_object(request, object_id) | ||
| org_id = obj.organization_id if obj else None | ||
| estimated_configured = WHOISService.check_estimate_location_configured(org_id) | ||
| extra_context = {"estimated_configured": estimated_configured} | ||
| return super().change_view(request, object_id, form_url, extra_context) |
There was a problem hiding this comment.
@DragnEmperor I think it makes sense to use the proposed fix here.
| .. code-block:: bash | ||
| source /opt/openwisp2/env/bin/activate | ||
| python /opt/openwisp2/src/manage.py clear_last_ip |
There was a problem hiding this comment.
| python /opt/openwisp2/src/manage.py clear_last_ip | |
| python /opt/openwisp2/manage.py clear_last_ip |
|
|
||
| .. _openwisp_controller_whois_estimated_location_enabled: | ||
|
|
||
| ``OPENWISP_CONTROLLER_WHOIS_ESTIMATED_LOCATION_ENABLED`` |
There was a problem hiding this comment.
This is outdated, we changed this to OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED
There was a problem hiding this comment.
Pull request overview
This PR implements the WHOIS Information and IP Address-Based Geolocation feature for OpenWISP as part of a GSoC 2025 project. The feature automatically fetches WHOIS information for devices based on their public IP addresses and creates/updates estimated geographic locations using MaxMind's GeoIP2 service.
Key Changes:
- Adds WHOIS lookup functionality with automatic refresh for stale records
- Implements estimated location creation/update based on IP geolocation data
- Adds organization-level settings to enable/disable features
- Exposes WHOIS and estimated location data via REST APIs and admin interface
Reviewed changes
Copilot reviewed 52 out of 60 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
openwisp_controller/config/whois/service.py |
Core WHOIS service handling GeoIP lookups, caching, and location management |
openwisp_controller/config/whois/tasks.py |
Celery tasks for async WHOIS fetching with retry logic |
openwisp_controller/config/base/whois.py |
WHOISInfo model with validation and signal handlers |
openwisp_controller/config/base/device.py |
Device model integration with WHOIS service |
openwisp_controller/geo/base/models.py |
Location model extended with is_estimated field and auto-update logic |
openwisp_controller/geo/estimated_location/tasks.py |
Tasks for managing estimated location creation/updates |
openwisp_controller/config/settings.py |
New settings for WHOIS and estimated location features with validation |
openwisp_controller/config/api/serializers.py |
Device serializers with WHOIS info mixins |
openwisp_controller/geo/api/serializers.py |
Location serializers with estimated status |
requirements.txt |
Added geoip2 dependency |
docs/user/whois.rst |
Comprehensive WHOIS feature documentation |
docs/user/estimated-location.rst |
Estimated location feature documentation |
| Various test files | Comprehensive test coverage for new features |
| Migration files | Database schema changes for WHOIS and estimated location |
| Admin files | Admin interface updates for WHOIS display and settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from .tasks import fetch_whois_details, manage_estimated_locations | ||
| from .utils import send_whois_task_notification |
There was a problem hiding this comment.
Circular import risk: This file imports from openwisp_controller.geo.estimated_location.tasks which in turn imports from the current module via openwisp_controller.config.whois.utils. This circular dependency could cause import issues at runtime. Consider restructuring to avoid the circular import, possibly by moving the notification utility function to a shared location or using lazy imports.
| with patch.object( | ||
| app_settings, "WHOIS_CONFIGURED", False | ||
| ), self.assertNumQueries(3): |
There was a problem hiding this comment.
Missing import statement for patch from unittest.mock. The test file uses patch.object() but doesn't show the import in the diff. While this may work if imported elsewhere, it's better to have explicit imports for test utilities.
| def save(self, *args, _set_estimated=False, **kwargs): | ||
| """ | ||
| Save the location object with special handling for estimated locations. | ||
| Parameters: | ||
| _set_estimated: Boolean flag to indicate if this save is being performed | ||
| by the estimated location system. When False (default), | ||
| manual edits will clear the estimated status. | ||
| *args, **kwargs: Arguments passed to the parent save method. | ||
| Returns: | ||
| The result of the parent save method. | ||
| """ | ||
| if WHOISService.check_estimate_location_configured(self.organization_id): | ||
| if not _set_estimated and ( | ||
| self._initial_address != self.address | ||
| or self._initial_geometry != self.geometry | ||
| ): | ||
| self.is_estimated = False | ||
| estimated_string = gettext("Estimated Location") | ||
| if self.name and estimated_string in self.name: | ||
| # remove string starting with "(Estimated Location" | ||
| self.name = re.sub( | ||
| rf"\s\({estimated_string}.*", "", self.name, flags=re.IGNORECASE | ||
| ) | ||
| else: | ||
| self.is_estimated = self._initial_is_estimated | ||
| return super().save(*args, **kwargs) |
There was a problem hiding this comment.
The _set_estimated parameter is used to bypass the automatic clearing of the is_estimated flag when coordinates are modified. While this pattern works, it's not a standard Django pattern. Consider using a context manager or a dedicated method like save_as_estimated() to make the intent clearer and avoid potential misuse of this private parameter in the public save method signature.
|
|
||
| class TestEstimatedLocationMixin(CreateWHOISMixin): | ||
| def setUp(self): | ||
| super(CreateWHOISMixin, self).setUp() |
There was a problem hiding this comment.
This line calls super(CreateWHOISMixin, self).setUp() which skips the immediate parent class in the MRO. This should be super().setUp() or if you specifically need to skip the immediate parent, it should be documented why. The current implementation may cause issues with multiple inheritance and is generally considered an anti-pattern in Python.
| _WHOIS_GEOIP_CLIENT = ( | ||
| "openwisp_controller.config.whois.service.geoip2_webservice.Client" | ||
| ) |
There was a problem hiding this comment.
The variable name _WHOIS_GEOIP_CLIENT is defined but the actual string path doesn't match the common Python import pattern. Consider using a more descriptive name that clarifies this is a mock path, such as _MOCKED_GEOIP_CLIENT_PATH.
| operations = [ | ||
| migrations.AddField( | ||
| model_name="organizationconfigsettings", | ||
| name="estimated_location_enabled", |
There was a problem hiding this comment.
The field name estimated_location_enabled uses "estimated_location" terminology, but earlier in the migration filename and other parts of the codebase, it's referred to as "approximate_location". This inconsistency between "approximate" and "estimated" terminology should be unified. Based on the broader context of the PR, "estimated" appears to be the correct term.
| if (typeof gettext === "undefined") { | ||
| var gettext = function (word) { |
There was a problem hiding this comment.
Variable 'gettext' is used before its declaration.
| if (typeof gettext === "undefined") { | |
| var gettext = function (word) { | |
| if (typeof window !== "undefined" && typeof window.gettext === "undefined") { | |
| window.gettext = function (word) { |
| """ | ||
|
|
||
| def to_representation(self, obj): | ||
| data = super(EstimatedLocationMixin, self).to_representation(obj) |
There was a problem hiding this comment.
First argument to super() should be EstimatedLocationGeoJsonSerializer.
| if config_app_settings.WHOIS_CONFIGURED: | ||
| self.title = _("geographic position") | ||
|
|
||
| def lookups(self, request, model_admin): |
There was a problem hiding this comment.
DeviceLocationFilter.lookups returns tuple of size 2 and tuple of size 4.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
After deploying this, the last_ip is always empty.
pip freeze | grep controller
openwisp-controller @ https://github.com/openwisp/openwisp-controller/tarball/gsoc25-whois#sha256=542d4b7387d8968d0e93b331fcf1c669f60de4ffcbbf8d3046d2ceed87260937
The last_ip info is not being collected at all.
>>> Device.objects.exclude(last_ip=None).count()
0
Checking the logs I don't see the fetch_whois_details task being executed at all:
tail log/celery*.log -f -n 1000 | grep fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
. openwisp_controller.config.whois.tasks.fetch_whois_details
What else can I do to debug?
| # Initial value for last_ip is required in WHOIS | ||
| # to remove WHOIS info related to that ip address. | ||
| if app_settings.WHOIS_CONFIGURED: | ||
| self._changed_checked_fields.append("last_ip") | ||
|
|
There was a problem hiding this comment.
This is also something to be wary of.
The clear_last_ip command previously used queryset update(), which skipped model save() and signals and left stale cached instances. As a result, DeviceChecksumView could not re-set last_ip when the IP address remained unchanged. This change saves devices individually to ensure proper cache invalidation.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @openwisp_controller/config/management/commands/clear_last_ip.py:
- Line 22: The unused variadic parameter in the handle method should be prefixed
to signal it's intentionally unused: change the signature from def handle(self,
*args, **options): to def handle(self, *_args, **options): so callers are
unaffected but linters/readers know the parameter is unused; update only the
handle function signature (no other code changes needed).
- Line 38: The query uses Device.objects.filter(...).only("last_ip") which
defers other fields and causes N+1 queries when Device.save() accesses related
fields (e.g., organization.config_settings.shared_secret); remove
.only("last_ip") and instead eager-load the relations needed by Device.save() by
replacing that call with something like
Device.objects.filter(_is_deactivated=False).select_related("organization__config_settings")
so the organization and its config_settings are fetched in the same query before
iterating and calling device.save().
- Around line 40-46: Change the management command help text in clear_last_ip.py
(the Command class help string) to state that it clears last_ip only for active
devices that do NOT already have a WHOISInfo record, and add a short inline
comment above the devices = devices.exclude(...
last_ip__in=Subquery(...WHOISInfo...)) line explaining that devices with
existing WHOISInfo are intentionally preserved so they won't re-query WHOIS;
also update the whois.rst documentation to replace the phrase "all active
devices across organizations" with the precise behavior (clears last_ip for
active devices lacking WHOISInfo across organizations) and include the rationale
that preserving existing WHOISInfo avoids unnecessary re-lookup.
🧹 Nitpick comments (7)
openwisp_controller/config/whois/tests/tests.py (6)
31-31: Consider moving queryset initialization into test methods.The module-level
notification_qs = Notification.objects.all()creates a queryset at import time, which could lead to stale results across tests. Django querysets are lazy, but defining them at module level can cause confusion.🔎 Recommended approach
Move the queryset definition into the test methods where it's used, or define it as a property/method in the test class:
-notification_qs = Notification.objects.all()Then in test methods where needed:
notification_qs = Notification.objects.all()Or as a test class method:
def get_notification_qs(self): return Notification.objects.all()
39-54: Consider extracting dispatch_uid constants to avoid duplication.The
_disconnect_signalsmethod duplicates dispatch_uid strings that are already defined inhandlers.py(lines 14, 19, 24). If these strings change, both locations need updating.🔎 Proposed refactor
Extract these constants into a shared location (e.g., a constants module or the handlers module):
# In handlers.py or a new constants.py DEVICE_DELETE_WHOIS_INFO_DISPATCH_UID = "device.delete_whois_info" INVALIDATE_ORG_CONFIG_CACHE_SAVE_DISPATCH_UID = "invalidate_org_config_cache_on_org_config_save" INVALIDATE_ORG_CONFIG_CACHE_DELETE_DISPATCH_UID = "invalidate_org_config_cache_on_org_config_delete"Then reference them in both locations:
dispatch_uid=DEVICE_DELETE_WHOIS_INFO_DISPATCH_UID
158-165: Simplify error message assertion.The error handling pattern using
try/exceptwithself.fail()is unnecessarily complex. Django'sassertRaisesMessageor direct assertion on the exception is clearer.🔎 Simpler approach
with self.subTest("Test WHOIS not configured does not allow enabling WHOIS"): with mock.patch.object( app_settings, "WHOIS_CONFIGURED", False ), self.assertRaises(ValidationError) as context_manager: org_settings_obj.full_clean() - try: - self.assertEqual( - context_manager.exception.message_dict["whois_enabled"][0], - "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set " - + "before enabling WHOIS feature.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["whois_enabled"][0], + "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set " + "before enabling WHOIS feature.", + )Apply the same pattern to lines 325-331, 338-346, and 362-368.
175-175: Replacegetattrwith direct attribute access.Using
getattrwith a constant attribute name is unnecessary and flagged by Ruff (B009). Direct attribute access is clearer and no less safe.🔎 Proposed fix
with self.subTest("Test setting WHOIS enabled to True"): org_settings_obj.whois_enabled = True org_settings_obj.save(update_fields=["whois_enabled"]) org_settings_obj.refresh_from_db(fields=["whois_enabled"]) - self.assertEqual(getattr(org_settings_obj, "whois_enabled"), True) + self.assertEqual(org_settings_obj.whois_enabled, True) with self.subTest("Test setting WHOIS enabled to False"): org_settings_obj.whois_enabled = False org_settings_obj.save(update_fields=["whois_enabled"]) org_settings_obj.refresh_from_db(fields=["whois_enabled"]) - self.assertEqual(getattr(org_settings_obj, "whois_enabled"), False) + self.assertEqual(org_settings_obj.whois_enabled, False) with self.subTest( "Test setting WHOIS enabled to None fallbacks to global setting" ): # reload app_settings to ensure latest settings are applied importlib.reload(app_settings) org_settings_obj.whois_enabled = None org_settings_obj.save(update_fields=["whois_enabled"]) org_settings_obj.refresh_from_db(fields=["whois_enabled"]) self.assertEqual( - getattr(org_settings_obj, "whois_enabled"), + org_settings_obj.whois_enabled, app_settings.WHOIS_ENABLED, )Based on learnings, this aligns with static analysis recommendations.
Also applies to: 181-181, 192-192
374-379: Inconsistent mock path usage.Class-level constants are defined for mock paths (lines 374-379), but line 500 uses a different constant
_WHOIS_GEOIP_CLIENTinstead of the module-level path string. This inconsistency could cause confusion.🔎 Proposed fix
Use the module-level constant consistently:
_WHOIS_GEOIP_CLIENT = ( "openwisp_controller.config.whois.service.geoip2_webservice.Client" ) _WHOIS_TASKS_INFO_LOGGER = "openwisp_controller.config.whois.tasks.logger.info" _WHOIS_TASKS_WARN_LOGGER = "openwisp_controller.config.whois.tasks.logger.warning" _WHOIS_TASKS_ERR_LOGGER = "openwisp_controller.config.whois.tasks.logger.error" # ... @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) @mock.patch(_WHOIS_TASKS_INFO_LOGGER) - @mock.patch(_WHOIS_GEOIP_CLIENT) + @mock.patch("openwisp_controller.config.whois.service.geoip2_webservice.Client") def test_whois_creation(self, mock_client, mock_info):Wait, actually line 500 correctly uses
_WHOIS_GEOIP_CLIENTbut it's missing theself.prefix. Let me check - no, it's used as a string in the decorator, so it should reference the class constant:- @mock.patch(_WHOIS_GEOIP_CLIENT) + @mock.patch(TestWHOISTransaction._WHOIS_GEOIP_CLIENT)Or define it at module level like the other mock paths in lines 498-499.
Also applies to: 497-500
585-595: Clarify comment about WHOIS deletion logic.The comment at line 594 states "WHOIS related to the device's last_ip should be deleted" but the test expects
count=1, meaning the WHOIS record should NOT be deleted. The test logic is correct (WHOIS should be preserved when other devices use the same IP), but the comment is misleading.🔎 Proposed fix
with self.subTest( "Test WHOIS not deleted when device is deleted and" " other active devices are linked to the last_ip" ): ip_address = device.last_ip device.delete(check_deactivated=False) self.assertEqual(mock_info.call_count, 0) mock_info.reset_mock() - # WHOIS related to the device's last_ip should be deleted + # WHOIS related to the device's last_ip should NOT be deleted + # because another active device still uses this IP self.assertEqual(WHOISInfo.objects.filter(ip_address=ip_address).count(), 1)openwisp_controller/config/management/commands/clear_last_ip.py (1)
47-55: Consider adding error handling for individual save operations.If a single
device.save()fails, the entire command will abort, potentially leaving the operation incomplete. Consider wrapping the save operation in a try/except block to handle failures gracefully and continue processing remaining devices.🔎 Proposed enhancement
updated_devices = 0 + failed_devices = 0 for device in devices.iterator(): - device.last_ip = None - device.save() - updated_devices += 1 + try: + device.last_ip = None + device.save() + updated_devices += 1 + except Exception as e: + failed_devices += 1 + self.stderr.write( + f"Failed to clear last_ip for device {device.pk}: {e}" + ) if updated_devices: self.stdout.write( f"Cleared last IP addresses for {updated_devices} active device(s)." ) + if failed_devices: + self.stderr.write( + f"Failed to clear last_ip for {failed_devices} device(s)." + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/tests/tests.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_controller/config/whois/tests/tests.py (7)
openwisp_controller/tests/utils.py (2)
TestAdminMixin(9-18)_login(17-18)openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (3)
CreateWHOISMixin(14-41)_create_whois_info(15-35)setUp(37-41)openwisp_controller/geo/base/models.py (2)
device(133-134)save(60-87)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(233-242)openwisp_controller/config/base/multitenancy.py (1)
save(90-101)openwisp_controller/geo/estimated_location/tests/utils.py (1)
setUp(9-15)
openwisp_controller/config/management/commands/clear_last_ip.py (1)
openwisp_controller/config/base/device.py (1)
save(277-297)
🪛 Ruff (0.14.10)
openwisp_controller/config/whois/tests/tests.py
175-175: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
181-181: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
192-192: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
openwisp_controller/config/management/commands/clear_last_ip.py
22-22: Unused method argument: args
(ARG002)
36-36: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.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.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (9)
openwisp_controller/config/whois/tests/tests.py (4)
315-369: LGTM! Comprehensive field validation tests.The validation tests cover all critical field constraints including length limits, format validation, and range checks. The coordinate validation test cases are particularly thorough.
Note: Consider applying the simplified error handling pattern mentioned in the previous comment to lines 325-331, 338-346, and 362-368.
497-617: LGTM! Comprehensive WHOIS lifecycle tests.The
test_whois_creationmethod thoroughly covers WHOIS record creation, update, and deletion scenarios including:
- Creation when device is created
- Update when device IP changes
- Proper cleanup based on whether other devices share the IP
- Deactivated device handling
The helper function
_verify_whois_detailsprovides good verification encapsulation.
670-709: Well-structured error handling tests.The
test_whois_task_failure_notificationmethod uses a clean helper function pattern to test multiple exception types with consistent assertions. This ensures proper logging and notification behavior across all geoip2 error scenarios.
711-782: LGTM! Comprehensive Selenium UI tests.The
test_whois_device_adminmethod thoroughly validates WHOIS information visibility in the Django admin under various configurations:
- WHOIS configured vs. not configured
- WHOIS enabled vs. disabled at org level
- WHOIS info exists vs. does not exist
The
_assert_no_js_errorshelper properly filters browser console logs and handles browser-specific differences.openwisp_controller/config/management/commands/clear_last_ip.py (5)
1-3: LGTM!All imports are necessary and correctly used throughout the command.
6-10: LGTM!The command class structure and help text are well-formed and descriptive.
12-20: LGTM!The argument handling for interactive/non-interactive mode follows Django conventions correctly.
26-36: LGTM!The interactive confirmation logic is clear and correctly handles user input. The error message is concise and appropriate for this context.
56-61: LGTM!The output messages are clear and appropriately handle both success cases (with device count) and no-op cases.
| ) | ||
| return super().add_arguments(parser) | ||
|
|
||
| def handle(self, *args, **options): |
There was a problem hiding this comment.
Remove or prefix unused parameter.
The *args parameter is never used in the method body.
🔎 Suggested fix
- def handle(self, *args, **options):
+ def handle(self, **options):📝 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.
| def handle(self, *args, **options): | |
| def handle(self, **options): |
🧰 Tools
🪛 Ruff (0.14.10)
22-22: Unused method argument: args
(ARG002)
🤖 Prompt for AI Agents
In @openwisp_controller/config/management/commands/clear_last_ip.py around line
22, The unused variadic parameter in the handle method should be prefixed to
signal it's intentionally unused: change the signature from def handle(self,
*args, **options): to def handle(self, *_args, **options): so callers are
unaffected but linters/readers know the parameter is unused; update only the
handle function signature (no other code changes needed).
| if input("".join(message)).lower() != "yes": | ||
| raise CommandError("Operation cancelled by user.") | ||
|
|
||
| devices = Device.objects.filter(_is_deactivated=False).only("last_ip") |
There was a problem hiding this comment.
Remove .only("last_ip") to avoid N+1 query problem.
Using .only("last_ip") causes Django to defer loading other fields. However, when device.save() is called at line 54, the Device.save() method accesses deferred fields like self.organization.config_settings.shared_secret (see lines 276-296 in device.py). This triggers additional database queries for each device in the loop, creating an N+1 query problem.
🔎 Proposed fix
- devices = Device.objects.filter(_is_deactivated=False).only("last_ip")
+ devices = Device.objects.filter(_is_deactivated=False).select_related(
+ "organization__config_settings"
+ )This optimizes the query by eager-loading the related organization and config_settings that Device.save() accesses.
📝 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.
| devices = Device.objects.filter(_is_deactivated=False).only("last_ip") | |
| devices = Device.objects.filter(_is_deactivated=False).select_related( | |
| "organization__config_settings" | |
| ) |
🤖 Prompt for AI Agents
In @openwisp_controller/config/management/commands/clear_last_ip.py around line
38, The query uses Device.objects.filter(...).only("last_ip") which defers other
fields and causes N+1 queries when Device.save() accesses related fields (e.g.,
organization.config_settings.shared_secret); remove .only("last_ip") and instead
eager-load the relations needed by Device.save() by replacing that call with
something like
Device.objects.filter(_is_deactivated=False).select_related("organization__config_settings")
so the organization and its config_settings are fetched in the same query before
iterating and calling device.save().
| devices = devices.exclude(last_ip=None).exclude( | ||
| last_ip__in=Subquery( | ||
| WHOISInfo.objects.filter(ip_address=OuterRef("last_ip")).values( | ||
| "ip_address" | ||
| ) | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full clear_last_ip.py file
cat openwisp_controller/config/management/commands/clear_last_ip.pyRepository: openwisp/openwisp-controller
Length of output: 2423
🏁 Script executed:
# Search for tests related to clear_last_ip
find . -type f -name "*.py" -path "*/test*" | xargs grep -l "clear_last_ip" 2>/dev/nullRepository: openwisp/openwisp-controller
Length of output: 121
🏁 Script executed:
# Search for documentation about this command
find . -type f \( -name "*.md" -o -name "*.rst" -o -name "*.txt" \) | xargs grep -l "clear_last_ip" 2>/dev/nullRepository: openwisp/openwisp-controller
Length of output: 93
🏁 Script executed:
# Look for WHOISInfo model definition
find . -type f -name "*.py" | xargs grep -l "class WHOISInfo" 2>/dev/nullRepository: openwisp/openwisp-controller
Length of output: 152
🏁 Script executed:
# Search for any comments or documentation about the relationship between Device and WHOISInfo
rg "WHOISInfo" -B2 -A2 --type=py | head -100Repository: openwisp/openwisp-controller
Length of output: 7203
🏁 Script executed:
# Read the WHOIS documentation
cat ./docs/user/whois.rstRepository: openwisp/openwisp-controller
Length of output: 4233
🏁 Script executed:
# Read the WHOISInfo model
cat ./openwisp_controller/config/base/whois.pyRepository: openwisp/openwisp-controller
Length of output: 5737
🏁 Script executed:
# Find tests for clear_last_ip command
find . -type f -name "*.py" -path "*/test*" -exec grep -l "clear_last_ip" {} \;Repository: openwisp/openwisp-controller
Length of output: 121
🏁 Script executed:
# Search for any discussions or comments about the purpose of clearing IPs without WHOIS info
rg "clear_last_ip|last_ip.*WHOIS" -B5 -A5 --type=pyRepository: openwisp/openwisp-controller
Length of output: 3772
🏁 Script executed:
# Check if there are any git commits or discussions explaining this design
rg "clear_last_ip" --type=py -B3 -A3 | grep -i "reason\|purpose\|exclude\|filter" | head -20Repository: openwisp/openwisp-controller
Length of output: 54
🏁 Script executed:
# Check the full test context to understand the expected behavior better
cat openwisp_controller/config/whois/tests/tests.py | grep -A30 "test_last_ip_management_command"Repository: openwisp/openwisp-controller
Length of output: 2364
Clarify the command's help text and documentation—it does not actually clear all active devices.
The command excludes devices that already have WHOISInfo records and only clears IPs for devices without WHOIS data. However:
- The help text states "Clears the last IP address (if set) for every active device across all organizations"—implying ALL active devices are cleared, which is false.
- The documentation in
whois.rstsays the same thing: "clear the last IP address of all active devices across organizations"—also misleading. - The filtering logic (line 40-46) that excludes devices with WHOISInfo is intentional but undocumented—there's no explanation in the help text or code comments about why.
The underlying design appears sound (preserve established WHOIS data while forcing devices without lookups to re-register and trigger fresh WHOIS queries), but this intent is hidden. Update the help text to be specific about the WHOIS filtering and clarify the command's actual behavior in the documentation.
🤖 Prompt for AI Agents
In @openwisp_controller/config/management/commands/clear_last_ip.py around lines
40-46, Change the management command help text in clear_last_ip.py (the Command
class help string) to state that it clears last_ip only for active devices that
do NOT already have a WHOISInfo record, and add a short inline comment above the
devices = devices.exclude(... last_ip__in=Subquery(...WHOISInfo...)) line
explaining that devices with existing WHOISInfo are intentionally preserved so
they won't re-query WHOIS; also update the whois.rst documentation to replace
the phrase "all active devices across organizations" with the precise behavior
(clears last_ip for active devices lacking WHOISInfo across organizations) and
include the rationale that preserving existing WHOISInfo avoids unnecessary
re-lookup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
openwisp_controller/config/whois/tests/tests.py (4)
157-164: Remove redundant try/except around assertion.The
try/exceptblock catchingAssertionErrorand callingself.fail()is redundant. TheassertEqualalready raisesAssertionErrorwith the appropriate message when the assertion fails, and Django's test runner will capture and report it correctly.🔎 Proposed fix
- try: - self.assertEqual( - context_manager.exception.message_dict["whois_enabled"][0], - "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set " - + "before enabling WHOIS feature.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["whois_enabled"][0], + "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set " + + "before enabling WHOIS feature.", + )
324-330: Remove redundant try/except blocks around assertions.Similar to the previous comment, these
try/exceptblocks are redundant. Django's test framework already captures and reportsAssertionErrorappropriately.🔎 Proposed fix for lines 324-330
- try: - self.assertEqual( - context_manager.exception.message_dict["ip_address"][0], - "WHOIS information cannot be created for private IP addresses.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["ip_address"][0], + "WHOIS information cannot be created for private IP addresses.", + )Apply similar changes to lines 337-345 and 361-367.
Also applies to: 337-345, 361-367
174-174: Prefer direct attribute access overgetattrwith constant.Using
getattrwith a constant attribute name is unnecessary and less clear than direct attribute access.As per static analysis hints.
🔎 Proposed fix
- self.assertEqual(getattr(org_settings_obj, "whois_enabled"), True) + self.assertEqual(org_settings_obj.whois_enabled, True)Apply the same change to lines 180 and 191.
Also applies to: 180-180, 191-191
667-669: Clarify misleading comment.The comment states "we need to allow the task to propagate exceptions" but the setting
CELERY_TASK_EAGER_PROPAGATES=Falseactually prevents exception propagation. This is the correct behavior for testing theon_failurecallback, but the comment is misleading.🔎 Proposed fix
- # we need to allow the task to propagate exceptions to ensure + # we need to prevent the task from propagating exceptions to ensure # `on_failure` method is called and notifications are executed @override_settings(CELERY_TASK_EAGER_PROPAGATES=False)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/whois/tests/tests.py
🧰 Additional context used
🪛 Ruff (0.14.10)
openwisp_controller/config/whois/tests/tests.py
174-174: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
180-180: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
191-191: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.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.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (1)
openwisp_controller/config/whois/tests/tests.py (1)
33-781: Comprehensive test coverage looks good.The test suite provides thorough coverage of the WHOIS feature including:
- Configuration and signal handling
- Model validation
- API endpoints and responses
- Background task execution and error handling
- Selenium-based UI verification
The test structure is well-organized with appropriate use of mocking, subtests, and transaction testing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @openwisp_controller/config/whois/tests/tests.py:
- Around line 427-434: The test updates device2.last_ip but never saves it, so
the signal handler won't run and mocked_task.assert_not_called() is a
meaningless check; fix by calling device2.save() after setting device2.last_ip
(mirroring the device1 case) so the save triggers the signal and the assertion
validates behavior of mocked_task (refer to device1.save(), device2.save(), and
mocked_task.assert_not_called()).
🧹 Nitpick comments (3)
openwisp_controller/config/whois/tests/tests.py (3)
70-80: Consider usingself.assertTrue/self.assertFalsefor consistency.Using bare
assertstatements works but provides less informative failure messages compared to unittest assertion methods. Consider usingself.assertTrue(not any(...))orself.assertFalse(any(...))for consistency with the rest of the test suite and better failure diagnostics.
170-193: Replacegetattrwith direct attribute access.Using
getattrwith a constant attribute name is unnecessary and less readable than direct attribute access.🔧 Suggested fix
with self.subTest("Test setting WHOIS enabled to True"): org_settings_obj.whois_enabled = True org_settings_obj.save(update_fields=["whois_enabled"]) org_settings_obj.refresh_from_db(fields=["whois_enabled"]) - self.assertEqual(getattr(org_settings_obj, "whois_enabled"), True) + self.assertTrue(org_settings_obj.whois_enabled) with self.subTest("Test setting WHOIS enabled to False"): org_settings_obj.whois_enabled = False org_settings_obj.save(update_fields=["whois_enabled"]) org_settings_obj.refresh_from_db(fields=["whois_enabled"]) - self.assertEqual(getattr(org_settings_obj, "whois_enabled"), False) + self.assertFalse(org_settings_obj.whois_enabled) with self.subTest( "Test setting WHOIS enabled to None fallbacks to global setting" ): # reload app_settings to ensure latest settings are applied importlib.reload(app_settings) org_settings_obj.whois_enabled = None org_settings_obj.save(update_fields=["whois_enabled"]) org_settings_obj.refresh_from_db(fields=["whois_enabled"]) self.assertEqual( - getattr(org_settings_obj, "whois_enabled"), + org_settings_obj.whois_enabled, app_settings.WHOIS_ENABLED, )
312-320: Consider simplifying assertion pattern.The
try/except AssertionErrorpattern withself.fail()is verbose. You can pass a message directly to the assertion method.🔧 Suggested simplification
with self.assertRaises(ValidationError) as context_manager: self._create_whois_info(ip_address="127.0.0.1") - try: - self.assertEqual( - context_manager.exception.message_dict["ip_address"][0], - "WHOIS information cannot be created for private IP addresses.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["ip_address"][0], + "WHOIS information cannot be created for private IP addresses.", + "ValidationError message not equal to expected message.", + )The same pattern is used at lines 325-335 and 348-357 and could be similarly simplified.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/whois/tests/tests.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_controller/config/whois/tests/tests.py (1)
openwisp_controller/config/whois/tests/utils.py (5)
WHOISTransactionMixin(44-155)_create_whois_info(15-35)setUp(37-41)_task_called(60-155)_mocked_client_response(46-58)
🪛 Ruff (0.14.10)
openwisp_controller/config/whois/tests/tests.py
174-174: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
180-180: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
191-191: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.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~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (4)
openwisp_controller/config/whois/tests/tests.py (4)
1-30: LGTM!Imports are well-organized. The module-level
notification_qsis acceptable since Django QuerySets are lazy and will execute fresh queries when accessed.
195-269: LGTM!Comprehensive API test coverage for WHOIS information in device endpoints, properly testing both configured and unconfigured states.
271-301: LGTM!Good test coverage for the management command, including cache invalidation verification through the DeviceChecksumView.
700-771: LGTM!Comprehensive Selenium tests for WHOIS admin UI visibility across different configuration states. The browser log filtering appropriately handles known non-critical messages.
| with self.subTest("Test task calls when last_ip is changed and is public"): | ||
| device1.last_ip = "172.217.22.12" | ||
| device1.save() | ||
| mocked_task.assert_called() | ||
| mocked_task.reset_mock() | ||
| device2.last_ip = "172.217.22.13" | ||
| mocked_task.assert_not_called() | ||
| mocked_task.reset_mock() |
There was a problem hiding this comment.
Missing device2.save() call appears to be a test logic error.
In the subTest for "Test task calls when last_ip is changed and is public", device1.save() is called at line 429 after updating last_ip, but device2.save() is missing at line 433. Without saving, the signal handler won't fire, making the assert_not_called() check meaningless since the task would never be triggered by a mere attribute assignment.
🐛 Proposed fix
with self.subTest("Test task calls when last_ip is changed and is public"):
device1.last_ip = "172.217.22.12"
device1.save()
mocked_task.assert_called()
mocked_task.reset_mock()
device2.last_ip = "172.217.22.13"
+ device2.save()
mocked_task.assert_not_called()
mocked_task.reset_mock()📝 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.
| with self.subTest("Test task calls when last_ip is changed and is public"): | |
| device1.last_ip = "172.217.22.12" | |
| device1.save() | |
| mocked_task.assert_called() | |
| mocked_task.reset_mock() | |
| device2.last_ip = "172.217.22.13" | |
| mocked_task.assert_not_called() | |
| mocked_task.reset_mock() | |
| with self.subTest("Test task calls when last_ip is changed and is public"): | |
| device1.last_ip = "172.217.22.12" | |
| device1.save() | |
| mocked_task.assert_called() | |
| mocked_task.reset_mock() | |
| device2.last_ip = "172.217.22.13" | |
| device2.save() | |
| mocked_task.assert_not_called() | |
| mocked_task.reset_mock() |
🤖 Prompt for AI Agents
In @openwisp_controller/config/whois/tests/tests.py around lines 427 - 434, The
test updates device2.last_ip but never saves it, so the signal handler won't run
and mocked_task.assert_not_called() is a meaningless check; fix by calling
device2.save() after setting device2.last_ip (mirroring the device1 case) so the
save triggers the signal and the assertion validates behavior of mocked_task
(refer to device1.save(), device2.save(), and mocked_task.assert_not_called()).
Checklist
Reference to Existing Issue
Completion of GSoC Project : WHOIS Information and IP Address-Based Geolocation
Description of Changes
This feature fetches all WHOIS related information and location details using device's last reported IP address. The fetched information is available at device's admin and related apis.
All the changes done are documented and have related tests.
More info: https://openwisp.org/blog/gsoc-2025-whois-info-and-estimated-geographic-locations/