[feature] GSoC25: WHOIS Information and IP Address-Based Geolocation#1175
[feature] GSoC25: WHOIS Information and IP Address-Based Geolocation#1175DragnEmperor wants to merge 59 commits intomasterfrom
Conversation
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.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds WHOIS Lookup and Estimated Location functionality across config and geo apps: new abstract model AbstractWHOISInfo and swappable WHOISInfo model; organization-level config fields (whois_enabled, estimated_location_enabled) with validation; WHOISService (GeoIP2 client, caching, lookup orchestration); Celery tasks to fetch, update, and delete WHOIS records and to manage estimated locations; signal wiring and AppConfig.ready() hooks; admin UI assets/templates and API serializer/filter mixins; management commands, migrations, requirements (geoip2), documentation, and extensive tests and test utilities. Sequence Diagram(s)sequenceDiagram
participant Device
participant WHOISService
participant Database as DB
participant Celery as CeleryTask
participant GeoIP as GeoIP2
Device->>Device: save() (last_ip may change)
Device->>WHOISService: _check_last_ip(creating)
WHOISService->>DB: get WHOISInfo(ip)
alt missing or stale
WHOISService->>Celery: enqueue fetch_whois_details(device_pk, ip)
Celery->>GeoIP: lookup(ip)
GeoIP-->>Celery: whois_details
Celery->>DB: create/update WHOISInfo
Celery->>WHOISService: update cache/state
alt estimated location enabled
Celery->>Celery: enqueue manage_estimated_locations(device_pk, ip)
Celery->>DB: create/update Location (is_estimated)
end
else fresh record
DB-->>WHOISService: WHOISInfo
end
WHOISService-->>Device: expose whois_info for admin/API
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.
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.
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.
docs/user/whois.rst
Outdated
| .. 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 |
docs/user/settings.rst
Outdated
|
|
||
| .. _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.
| 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.
There was a problem hiding this comment.
I think copilot is right in flagging this as well, please double check.
There was a problem hiding this comment.
Will add a comment for the same, this is intentional
...e_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py
Show resolved
Hide resolved
| """ | ||
|
|
||
| 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.
There was a problem hiding this comment.
@DragnEmperor is EstimatedLocationMixin correct?
Can't we rewrite this as:
| data = super(EstimatedLocationMixin, self).to_representation(obj) | |
| data = super().to_representation(obj) |
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?
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.
openwisp_controller/config/management/commands/clear_last_ip.py
Outdated
Show resolved
Hide resolved
openwisp_controller/config/management/commands/clear_last_ip.py
Outdated
Show resolved
Hide resolved
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.
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
940fa3f to
cc65526
Compare
|
@coderabbitai can you create issues for out of diff problems? As they're not related to this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 150
Could you clarify which specific "out of diff problems" you'd like me to create issues for? If you're referring to:
Let me know how you'd like to proceed! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
openwisp_controller/config/settings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/whois/service.py
🧬 Code graph analysis (1)
openwisp_controller/geo/admin.py (2)
openwisp_controller/config/whois/service.py (1)
check_estimated_location_configured(115-121)openwisp_controller/geo/base/models.py (1)
organization_id(153-154)
🔇 Additional comments (10)
openwisp_controller/config/settings.py (1)
71-93: Well-structured settings with proper fail-fast validation.The new WHOIS settings block addresses previous review feedback:
WHOIS_REFRESH_THRESHOLD_DAYSis now validated as a positive integer (Lines 82-87), and theImproperlyConfiguredmessages now include explicit setting names (Lines 76-80, 91-92). The validation ordering (credentials → threshold → feature dependency) is logical and clear.openwisp_controller/config/whois/service.py (2)
244-268: Previous late-binding closure issue properly addressed.Line 252 eagerly captures
initial_ip = self.device._initial_last_ipbefore registering theon_commitcallback. This correctly prevents the previously identified bug where_initial_last_ipcould be reset by_check_last_ip()before the deferred callback executes.
324-354: Notification correctly moved outsidetransaction.atomic()block.Both the create path (Line 334) and update path (Line 350) now send notifications after the atomic block commits, ensuring no notifications are triggered for rolled-back transactions. This addresses the previous review concern about inconsistent notification placement.
openwisp_controller/geo/admin.py (2)
118-128: Correctly addresses prior mutation concern — copies are made.Both
get_list_displayandget_list_filternow operate on copies vialist(), preventing class-level attribute mutation across requests.
137-143:extra_contextmerging correctly addressed.Line 141 uses
extra_context = extra_context or {}before adding keys, preserving any caller-provided context.openwisp_controller/geo/estimated_location/tests/tests.py (5)
1-36: Well-organized test module with appropriate imports and model loading.The test infrastructure is properly set up with
TestCasefor non-transactional tests andTransactionTestCaseforon_commitcallback testing. The_notification_qshelper provides a clean abstraction for notification assertions.
38-106: Configuration settings tests cover key validation scenarios.The
importlib.reloadpattern for testing Django settings is appropriate. The test correctly validates thatImproperlyConfiguredis raised for invalid configurations, org-level validation errors are surfaced, and admin field visibility toggles based onWHOIS_CONFIGURED.
556-585: Baredevice.devicelocationaccess is intentional — confirmed in prior review.Lines 575 and 585 use standalone expressions to trigger
RelatedObjectDoesNotExist/ verify existence. This was discussed in a prior review round and accepted with explanatory comments.
587-632: Excellent idempotency test for shared location stability.This test verifies that identical WHOIS results don't recreate locations when devices reuse the same IP — covering an important edge case for the location-sharing logic. The assertion at Line 632 (
notification_countunchanged) is a good check for spurious side effects.
796-849: API assertion pattern correctly usesresults_by_id— prior concern addressed.Lines 822 and 844 use dictionary lookups by ID rather than relying on response ordering, making these assertions order-independent and non-flaky.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 196-213: In _need_whois_lookup: avoid the unnecessary DB query by
checking the cheap org setting first—first call
is_valid_public_ip_address(new_ip), then check self.is_whois_enabled
(cheap/cache) and return False if disabled, and only then call
self._get_whois_info_from_db(ip_address=new_ip).first() and use
self.is_older(whois_obj.modified) as before; update the control flow so
_get_whois_info_from_db is only invoked when IP is public and WHOIS is enabled.
- Around line 288-307: The _create_or_update_whois function currently always
appends "modified" to update_fields and updates/saves the instance even when no
other attributes changed; change it so "modified" is only appended and
timezone.now() assigned (and save called with update_fields) when at least one
other field was added to update_fields—i.e., after the loop check if
update_fields is non-empty, then add "modified", set whois_instance.modified,
and call whois_instance.save(update_fields=update_fields); ensure you do not
call save or return a non-empty update_fields when nothing actually changed.
- Around line 143-194: process_whois_details currently returns isp, asn, and
cidr possibly as None which will violate WHOISInfo CharField constraints; update
process_whois_details to return empty-string fallbacks for those keys (same
pattern used for "timezone") so "isp", "asn", and "cidr" are "" when missing; in
addition, update _create_or_update_whois so that before saving you call
instance.full_clean() to validate CharField constraints and change the naive
str() change-detection (used when comparing existing values) to compare the
actual stored value (e.g., getattr(instance, field) or normalize both sides to
empty-string) to avoid treating None vs "" as a change and prevent unnecessary
DB writes and IntegrityError on save.
In `@openwisp_controller/geo/admin.py`:
- Around line 186-200: The queryset method's WHOIS-configured branch currently
relies on an implicit fallthrough to handle value == "false", which can confuse
future readers; update the queryset method (function queryset) to make the
"false" case explicit by either adding an elif value == "false": return
queryset.filter(devicelocation__isnull=True) inside the WHOIS_CONFIGURED block
or by adding a brief inline comment above the final return explaining that the
fallthrough intentionally returns devicelocation__isnull=True when value ==
"false" so maintainers understand the behavior.
- Around line 108-116: The get_fields method currently mutates the parent return
value directly; make a defensive copy before mutating by replacing the call to
super().get_fields(request, obj) with list(super().get_fields(request, obj)) so
append/remove won't alter shared state (mirror the pattern used in
get_list_display and get_list_filter); keep the rest of the logic using org_id
and WHOISService.check_estimated_location_configured unchanged.
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 1044-1054: Add an explicit assertion that the response returns all
devices (i.e., the filter was ignored) by checking the pagination/count rather
than only checking presence of each id; after each GET in the subTest (with
WHOIS_CONFIGURED patched False and for estimated_device, outdoor_device,
indoor_device) assert response.data['count'] == 3 or assert
len(response.data.get('results', [])) == 3 to ensure the total number of
returned items is exactly the three expected devices.
Made ordering consistent, clarified floorplan_set is prefetched.
nemesifier
force-pushed
the
gsoc25-whois
branch
from
f59dd79 to
3cdcf5b
Compare
Some tests were breaking, highlighting need for more careful handling of corner cases in LocationAdmin. Renamed check_estimated_location_configured to check_estimated_location_enabled for clarity
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/user/settings.rstopenwisp_controller/config/settings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/templates/admin/geo/location/change_form.html
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/templates/admin/geo/location/change_form.htmlopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tests/tests.pydocs/user/settings.rstopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/whois/service.py
🧬 Code graph analysis (4)
openwisp_controller/geo/api/serializers.py (1)
openwisp_controller/geo/estimated_location/mixins.py (2)
EstimatedLocationGeoJsonMixin(21-33)EstimatedLocationMixin(6-18)
openwisp_controller/geo/estimated_location/mixins.py (2)
openwisp_controller/config/whois/service.py (2)
WHOISService(20-360)check_estimated_location_enabled(115-121)openwisp_controller/geo/base/models.py (1)
organization_id(151-152)
openwisp_controller/geo/api/views.py (1)
openwisp_controller/geo/admin.py (1)
queryset(185-199)
openwisp_controller/config/whois/service.py (3)
openwisp_controller/config/whois/tasks.py (1)
fetch_whois_details(61-107)openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)openwisp_controller/config/models.py (3)
WHOISInfo(117-124)OrganizationConfigSettings(96-104)Config(36-43)
🔇 Additional comments (18)
openwisp_controller/geo/base/models.py (4)
18-28: LGTM on field definitions and type annotation.The
_changed_checked_fieldsandis_estimatedfield definitions are clean and appropriate.
33-43: LGTM on initial value tracking.Proper use of
get_deferred_fields()to handle deferred fields, and dynamic attribute setting via_set_initial_values_for_changed_checked_fieldsis well-structured.
45-64: LGTM on clean() validation.The
DEFERREDguard is correctly applied before comparing_initial_is_estimated, and the feature-enabled check viaWHOISService.check_estimated_location_enabledis appropriate.
80-101: Clarify the estimated location marker format and behavior:The location name format in the codebase does not use tilde markers (
~...~). Names are stored as either"Estimated Location: {ip}"or"{location}: {ip}". Whenis_estimatedis manually cleared insave(), the name field is intentionally left unchanged — only theis_estimatedflag is reset. If marker stripping is intended, it should be explicitly implemented in the save method or clarified whether the current behavior is acceptable.Likely an incorrect or invalid review comment.
openwisp_controller/config/settings.py (1)
71-93: LGTM on WHOIS settings and validations.Settings are well-structured: fail-fast validation using
ImproperlyConfiguredwith descriptive messages that include the setting names. The dependency chain (ESTIMATED_LOCATION_ENABLED → WHOIS_ENABLED → WHOIS_CONFIGURED) is correctly enforced.openwisp_controller/config/whois/service.py (3)
247-271: Late-binding closure fornew_ipin theelifbranch.On line 269, the lambda captures
new_ipby closure. Sincenew_ipis a local variable assigned on line 254, it won't change between registration and commit, so this is safe. However,selfis also captured — if theWHOISServiceinstance or itsdevicereference is mutated before commit, the behavior could differ from the intent. This is unlikely in practice but worth noting.The
force_lookuppath correctly capturesinitial_ipeagerly (line 255, per past review). Both branches are consistent.
314-359: LGTM on estimated location create/update flow.The notification is correctly placed outside the atomic block (line 339), the update path only saves when fields actually changed (line 350), and
_set_estimated=Trueis properly passed to preventsave()from clearingis_estimated.
81-112: Cache invalidation for emptyOrganizationConfigSettings()instances is properly handled.While caching an empty
OrganizationConfigSettings()instance on line 106 could theoretically cache stale defaults if org settings are created later, the signal handlers inopenwisp_controller/config/whois/handlers.pyproperly invalidate the cache on bothpost_saveandpost_deleteofOrganizationConfigSettings. The cache invalidation viaWHOISInfo.invalidate_org_settings_cache()is correctly connected to the model and will clear the cache whenever org settings are created, updated, or deleted.openwisp_controller/geo/api/views.py (2)
55-64: LGTM on dynamic filter injection foris_estimated.Evaluating
WHOIS_CONFIGUREDat runtime via__init__rather than at class definition time is the right call for testability. TheBooleanFilteris appropriate for the boolean field.
328-333: Good: Prefetch eliminates N+1 queries for floorplans inLocationSerializer.to_representation.The
Prefetchwithorder_by("-created")aligns with how the serializer'sto_representation(line 170 in serializers.py) iterates overinstance.floorplan_set.all().openwisp_controller/geo/estimated_location/mixins.py (1)
1-33: LGTM on estimated location serializer mixins.Clean implementation that conditionally includes/excludes
is_estimatedbased on org-level feature flags. Thepop(..., None)pattern is safe, andorganization_idis a direct FK attribute avoiding extra queries.openwisp_controller/geo/api/serializers.py (4)
38-40: LGTM on mixin integration for GeoJSON serializer.MRO is correct — the mixin's
to_representationwill callsuper()intoGeoFeatureModelSerializer, and then conditionally inject/removeis_estimatedin properties.
135-152: LGTM onLocationSerializerchanges.The
EstimatedLocationMixinis correctly positioned first in MRO. Convertingfieldsandread_only_fieldsfrom tuples to lists improves consistency with other serializers in this file.
166-179: LGTM — relies on prefetched floorplans from the view.The comment correctly documents that
floorplan_setordering is handled by thePrefetchinLocationListCreateView. Note thatLocationDetailView(line 344) doesn't use prefetch, so this will issue one extra query for individual location detail requests — which is acceptable for single-object views.
235-237: LGTM onNestedtLocationSerializermixin integration.Same pattern as
GeoJsonLocationSerializer. The typo in the class name ("Nestedt" → "Nested") is pre-existing and outside the scope of this change.openwisp_controller/geo/templates/admin/geo/location/change_form.html (1)
4-11: Theestimated_enabledvariable is properly provided in the admin context. It's injected via theLocationAdmin.change_view()method, where it's derived fromWHOISService.check_estimated_location_enabled(org_id)and explicitly added toextra_contextbefore rendering the change form template.openwisp_controller/geo/admin.py (2)
163-199:DeviceLocationFilterchanges look correct.The dynamic title override in
__init__and the conditionallookups/querysetlogic all work as expected. Thelist_displayandlist_filtermutations addressed in earlier review rounds are no longer present.
108-112:list_display.index("created")assumes the parent class includes this column.The code relies on
AbstractLocationAdmin.list_displayfrom django-loci containing"created". While this pattern works in practice (and is used similarly ingeo/api/filters.pyandsubnet_division/admin.py), it creates an implicit dependency that could silently fail if the parent class changes. A more defensive approach would check for the field's presence first or fall back toappend():♻️ More defensive alternative
def get_list_display(self, request): list_display = list(super().get_list_display(request)) # makes a copy if config_app_settings.WHOIS_CONFIGURED: - list_display.insert(list_display.index("created"), "is_estimated") + if "created" in list_display: + list_display.insert(list_display.index("created"), "is_estimated") + else: + list_display.append("is_estimated") return list_display
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/settings.rst`:
- Around line 833-844: Reorder the settings documentation so all WHOIS_* entries
are grouped together: move the WHOIS_REFRESH_THRESHOLD_DAYS section to appear
immediately after WHOIS_GEOIP_KEY and before ESTIMATED_LOCATION_ENABLED,
matching the code order where the WHOIS_REFRESH_THRESHOLD_DAYS setting appears
before ESTIMATED_LOCATION_ENABLED; update any surrounding anchors or
cross-references if needed to preserve links (referenced symbols:
WHOIS_REFRESH_THRESHOLD_DAYS and ESTIMATED_LOCATION_ENABLED).
- Around line 797-800: Two setting default values are shown as bare None; update
their default formatting to use inline code (double backticks) for consistency:
replace the bare None for the setting with type "str" (the block showing
**type**: ``str`` and **default**: None) with **default**: ``None`` and apply
the same change to OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY so its **default** entry
also shows ``None`` using double backticks.
In `@openwisp_controller/config/whois/service.py`:
- Around line 186-194: process_whois_details currently relies on truthy coercion
for asn and cidr which can drop valid falsy values (e.g., 0) and returns network
objects; change the returned dict so autonomous_system_number is explicitly
handled (assign a local asn = data.traits.autonomous_system_number and set asn =
"" only if asn is None, otherwise str(asn)) and convert network objects for cidr
by doing cidr_val = data.traits.network and setting cidr = "" if cidr_val is
None else str(cidr_val); update the keys "asn" and "cidr" in the returned dict
to use these converted values in process_whois_details.
In `@openwisp_controller/geo/admin.py`:
- Around line 120-128: The removal of "is_estimated" in
AbstractLocationAdmin.get_fields can raise ValueError if the field is already
absent; change the unconditional fields.remove("is_estimated") to a safe
conditional removal: check membership with "if 'is_estimated' in fields" before
removing, using the existing fields list copy returned by super().get_fields;
keep the same guard logic around obj and
WHOISService.check_estimated_location_enabled(obj.organization_id) but only
attempt to remove when the field exists to avoid exceptions.
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 229-231: The test patches django.core.cache.cache.get with a
finite side_effect list which will raise StopIteration on the 4th call; change
the patch on cache.get to use return_value=None instead of side_effect=[None,
None, None] (i.e. patch "django.core.cache.cache.get" with return_value=None and
keep the existing mocked_set patch) so mocked_get always returns None and won't
break if additional cache lookups are added; update any references to mocked_get
accordingly (the mocked_get.assert_called() remains valid).
- Around line 43-105: The test leaves config_app_settings in a
stale/partially-initialized state because importlib.reload(config_app_settings)
is called inside nested override_settings and an assertRaises path; to fix, in
test_estimated_location_configuration_setting add a guaranteed reload of
config_app_settings after the test (e.g., call self.addCleanup(lambda:
importlib.reload(config_app_settings)) at the start of the test) and ensure any
importlib.reload(config_app_settings) that might raise (the one used inside the
ImproperlyConfigured subtest) is wrapped so the module is reloaded to a sane
state (use try/finally or perform the failing reload only inside the
assertRaises but still rely on the addCleanup to restore the module), keeping
references to config_app_settings and OrganizationConfigSettings to locate the
changes.
In `@openwisp_controller/geo/templates/admin/geo/location/change_form.html`:
- Line 2: The template unnecessarily loads the admin_urls tag library; remove
"admin_urls" from the "{% load i18n admin_urls %}" line in the template so it
only loads needed tags (e.g., "{% load i18n %}"), ensuring the "{% load i18n
admin_urls %}" statement is updated to drop admin_urls since no admin_urls
tags/filters are used in this file.
---
Duplicate comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 291-312: In _create_or_update_whois, the update branch currently
sets attributes and calls whois_instance.save(update_fields=update_fields)
without validation; call whois_instance.full_clean() after applying setattr (and
before setting modified/save) to validate changes, then proceed to set
whois_instance.modified and save(update_fields=update_fields); ensure you handle
or propagate ValidationError from full_clean() consistently with the create
path.
openwisp_controller/geo/templates/admin/geo/location/change_form.html
Outdated
Show resolved
Hide resolved
The current design doesn't allow to get whois info efficiently in the API list endpoint, hence we have to remove it.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
docs/user/settings.rstopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/whois/mixins.pyopenwisp_controller/config/whois/serializers.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/templates/admin/geo/location/change_form.html
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/templates/admin/geo/location/change_form.htmldocs/user/settings.rstopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
docs/user/settings.rstopenwisp_controller/config/whois/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/whois/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/whois/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/service.py
🧬 Code graph analysis (4)
openwisp_controller/config/api/serializers.py (1)
openwisp_controller/config/whois/mixins.py (1)
WHOISMixin(5-20)
openwisp_controller/config/whois/mixins.py (4)
openwisp_controller/config/whois/serializers.py (1)
WHOISSerializer(7-16)openwisp_controller/config/whois/utils.py (1)
get_whois_info(54-74)openwisp_controller/config/base/device.py (1)
whois_service(527-532)openwisp_controller/config/whois/service.py (2)
get_device_whois_info(237-245)is_whois_enabled(124-131)
openwisp_controller/geo/estimated_location/tests/tests.py (4)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(10-35)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(92-156)openwisp_controller/geo/estimated_location/tests/utils.py (3)
TestEstimatedLocationMixin(11-34)setUp(12-19)run_task(23-34)
openwisp_controller/config/whois/service.py (3)
openwisp_controller/config/whois/tasks.py (1)
fetch_whois_details(61-107)openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)openwisp_controller/geo/base/models.py (2)
device(147-148)save(66-101)
⏰ 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.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | 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~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (23)
openwisp_controller/geo/templates/admin/geo/location/change_form.html (1)
6-6: The code is correct.estimated_enabledis explicitly injected into the template context in theLocationAdmin.change_view()method (openwisp_controller/geo/admin.py lines 139-141), where it retrieves the value fromWHOISService.check_estimated_location_enabled()and adds it toextra_contextbefore passing it to the parent view.openwisp_controller/geo/estimated_location/tests/tests.py (7)
1-36: LGTM!Imports are well-organized and the swapper-based model loading follows the project's established pattern for Django swappable models.
43-106: Good use ofaddCleanupforconfig_app_settingsreload.The
self.addCleanup(importlib.reload, config_app_settings)at line 45 properly ensures the module is restored to a sane state after the test, regardless of which nestedoverride_settingscontext was active last. This correctly addresses the stale-state concern.One remaining subtlety: after line 57's
importlib.reloadraisesImproperlyConfigured,config_app_settingsmay be left partially initialized for the remainder of the test method (subtests 2–4). This works today because subtest 2 (line 59) doesn't readconfig_app_settings, and subtest 4 (line 92) explicitly reloads it. Subtest 3 (lines 73–86), however, implicitly depends onWHOIS_CONFIGUREDbeingTruewhen the admin view checks it — if the partially-loaded module no longer has that attribute set, the admin assertion could become flaky. If CI stays green, this is fine, but a reload after theassertRaisesblock would make the flow more resilient.
109-174: LGTM!
TestEstimatedLocationFieldcleanly tests the validation, admin visibility toggle foris_estimated, and the WHOIS-disabled scenario. The subtest structure is clear and the assertions are well-targeted.
196-203: LGTM — clean setUp with proper notification type management.Using
contextlib.suppress(ImproperlyConfigured)to safely unregister a possibly-absent notification type before re-registering under the WHOIS mock is a pragmatic pattern for transactional tests.
339-584: Thorough coverage of location creation, update, and sharing scenarios.The
test_estimated_location_creation_and_updatemethod provides excellent coverage of the estimated location lifecycle:
- Creation on new device, update on IP change, behavior with empty address data
- Non-estimated locations correctly left untouched
- Location sharing when devices share the same IP
- Shared location not updated when one device's IP changes (new location created instead)
- Old estimated locations cleaned up when replaced
The
_verify_location_detailshelper avoids repetition across subtests nicely. Mock parameter ordering matches the decorator stacking correctly throughout.
617-662: Good idempotency test for shared locations.
test_unchanged_whois_data_no_location_recreationvalidates that re-assigning the same IP doesn't spuriously recreate locations or generate duplicate notifications — an important correctness property for the WHOIS flow.
826-879: LGTM — order-independent API assertions.The
results_by_iddictionary pattern (lines 852, 874) makes these assertions robust against API ordering changes. The contradictoryassertFalseafterassertNotInfrom earlier reviews has been removed.openwisp_controller/config/whois/serializers.py (1)
1-16: LGTM!Clean serializer setup. The explicit
JSONFielddeclaration foraddressensures proper serialization of the JSON-stored field.openwisp_controller/config/settings.py (1)
71-93: Well-structured settings validation chain.The fail-fast validation with descriptive
ImproperlyConfiguredmessages is good. The dependency ordering (WHOIS credentials → WHOIS enabled → estimated location enabled) prevents misconfiguration at startup.openwisp_controller/config/whois/tests/tests.py (6)
1-47: LGTM — module-level setup.Imports,
MODIFIED_CACHEwithKEY_PREFIXfor test isolation, and the_notification_qs()helper are well-structured.
50-161: LGTM — TestWHOIS class setup and configuration tests.Good use of
addCleanup(importlib.reload, app_settings)to prevent cross-test leakage. Signal disconnect/reconnect testing is thorough.
383-422: LGTM — Model field validation tests.Comprehensive coverage of boundary checks for ISP length, private IP rejection, timezone length, CIDR format, ASN format, and coordinate bounds.
601-718: LGTM — WHOIS creation and lifecycle tests.Thorough coverage of creation on device create, IP change with/without shared IPs, deletion cascading, and deactivated device handling.
844-942: LGTM — Task failure notification and cache tests.Good coverage of all geoip2 exception types with correct notification count expectations (only first error triggers notification, subsequent errors use cache). The
MODIFIED_CACHEwithKEY_PREFIXprevents cross-test cache pollution.
1062-1169: LGTM — Selenium tests with XSS protection.The XSS test using
UnexpectedAlertPresentExceptionis a solid pattern for detecting injected script execution in the admin UI.openwisp_controller/config/whois/service.py (5)
1-27: LGTM — Imports and class initialization.Clean imports, well-structured service class with device binding.
28-67: LGTM — Static utility methods.
get_geoip_client,get_cache_key,is_valid_public_ip_address, and_get_whois_info_from_dbare well-scoped and correctly implemented.
69-112: LGTM —is_olderandget_org_config_settings.Good caching pattern with fallback to empty
OrganizationConfigSettingsinstance, and proper timezone-awareness enforcement.
143-194: LGTM —process_whois_detailswith proper exception handling and fallbacks.Exception re-raising preserves the original cause chain (
from e). Fallbacks for nullable fields (or "", coordinate handling) preventIntegrityErroron non-nullableCharFields.
315-361: LGTM — Estimated location creation/update.Notifications are correctly sent outside the
transaction.atomic()block. The update path only saves when fields actually changed, avoiding unnecessary writes.openwisp_controller/config/whois/mixins.py (1)
5-20: LGTM — WHOISMixin implementation.Clean mixin pattern with proper gating on
WHOIS_CONFIGUREDand per-orgis_whois_enabled. Only applied to the detail serializer (not list), correctly avoiding N+1 concerns.One minor naming note:
serializer_classis conventionally a view-level DRF attribute; renaming towhois_serializer_classwould reduce potential confusion, but this is purely cosmetic since no conflict exists in the current usage.openwisp_controller/config/api/serializers.py (1)
12-12: LGTM — WHOISMixin integration in DeviceDetailSerializer.Correctly applied only to
DeviceDetailSerializer(notDeviceListSerializer), consistent with the N+1 avoidance strategy documented in the PR objectives.Also applies to: 278-278
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/settings.rst`:
- Line 802: Replace the incorrect brand capitalization "Maxmind" with "MaxMind"
in the documentation line containing "Maxmind Account ID required for the
:doc:`WHOIS Lookup feature <whois>`." Update every other occurrence of the same
misspelling in this file (notably the other instance referenced) so the text
reads "MaxMind Account ID required for the :doc:`WHOIS Lookup feature <whois>`."
Ensure only the capitalization change is made and surrounding formatting or
links (the :doc:`whois` reference) are left intact.
In `@openwisp_controller/config/whois/service.py`:
- Around line 247-271: In process_ip_data_and_location ensure new_ip is eagerly
captured for the transaction.on_commit lambda in the elif branch to avoid
late-binding; change the lambda that calls self.trigger_estimated_location_task
so it captures new_ip as a default argument (e.g., lambda ip_address=new_ip:
self.trigger_estimated_location_task(ip_address=ip_address)) so the function
trigger_estimated_location_task receives the intended value even if future
refactors reassign new_ip.
- Around line 291-313: The change-detection using str(...) is fragile for
geometry fields like coordinates; in _create_or_update_whois, special-case the
'coordinates' attribute (and any other geometry fields) and compare them using
direct geometry equality rather than stringification: fetch existing =
getattr(whois_instance, attr), and if attr == "coordinates" use "if existing !=
value:" (or call existing.equals(value) / existing.equals_exact(value,
tolerance) if using GEOS methods) to decide updates; keep the original str-based
comparison for non-geometry fields and preserve the existing update_fields
append/save logic on WHOISInfo.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 284-314: The current test_device_list_api_whois_no_nplus1 creates
devices inside a loop but asserts a fixed self.assertNumQueries(4) each
iteration, which doesn't prove query-count constancy across device counts;
change the test to create devices in stages and compare query counts explicitly:
e.g., create one device (and its WHOISInfo), call
reverse("config_api:device_list") inside self.assertNumQueries(...) to record
the baseline queries, then create two more devices (and WHOISInfo for each) and
call the same endpoint again inside another self.assertNumQueries(...) asserting
the same number as the baseline; keep the checks using WHOISInfo and the
assertNotIn("whois_info", result) but replace the per-iteration assert with a
comparison of the two measured query counts using the same assertNumQueries
value or by capturing counts and asserting equality to prove no N+1.
- Line 308: The subTest description uses a literal "{i}" instead of
interpolating the loop counter; update the with self.subTest("Device List API
with WHOIS info: {i}") call to an f-string (e.g., with self.subTest(f"...{i}"))
so the loop variable i is included in the test case name; locate the with
self.subTest call in tests.py and add the f prefix to the string literal.
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 1079-1090: Rename the loop variable `i` in the test block inside
tests.py (the loop iterating over ["estimated","outdoor","indoor"] within the
subTest labeled "filter_value") to a descriptive name such as `filter_value`;
update all references inside the loop (the inner subTest call and any uses in
the GET params) so they use `filter_value` instead of `i` to improve readability
in the test method.
---
Duplicate comments:
In `@openwisp_controller/geo/templates/admin/geo/location/change_form.html`:
- Line 2: The duplicate review note indicates no action is needed because the
unused `{% load admin_urls %}` has already been removed and only `{% load i18n
%}` remains in the template; confirm that the template's top line is `{% load
i18n %}` (in the change_form.html template) and leave it as-is, removing any
lingering duplicate review comments or notes if present.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/geo/api/serializers.py (1)
38-46:⚠️ Potential issue | 🟡 MinorAdd
is_estimatedtoread_only_fieldsfor consistency
GeoJsonLocationSerializerusesfields = "__all__", which includesis_estimatedwithout marking it read-only. This inconsistently differs fromLocationSerializerandNestedtLocationSerializer, which explicitly excludeis_estimatedfrom theirMeta.fieldslists.While the current view (
GeoJsonLocationList) is read-only (supports only GET), and the model itself provides protection throughsave()andclean()methods, adding explicitread_only_fieldsensures consistency across serializers and guards against future changes.Proposed fix
class GeoJsonLocationSerializer( EstimatedLocationGeoJsonMixin, gis_serializers.GeoFeatureModelSerializer ): device_count = IntegerField() class Meta: model = Location geo_field = "geometry" fields = "__all__" + read_only_fields = ("is_estimated",)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/api/serializers.py` around lines 38 - 46, The GeoJsonLocationSerializer Meta currently uses fields="__all__" which unintentionally exposes the model's is_estimated field for writes; update the GeoJsonLocationSerializer.Meta to add read_only_fields = ("is_estimated",) so is_estimated remains read-only (consistent with LocationSerializer/NestedLocationSerializer) while keeping fields="__all__". Ensure you modify the Meta class inside GeoJsonLocationSerializer.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
docs/user/settings.rstopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyrequirements.txt
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
docs/user/settings.rstopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
docs/user/settings.rstopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/api/serializers.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/api/serializers.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/api/serializers.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/api/serializers.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.py
🧬 Code graph analysis (6)
openwisp_controller/geo/estimated_location/tasks.py (3)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)openwisp_controller/config/base/whois.py (1)
_get_defaults_for_estimated_location(154-165)openwisp_controller/config/whois/service.py (1)
_create_or_update_estimated_location(317-363)
openwisp_controller/config/whois/service.py (4)
openwisp_controller/config/whois/tasks.py (1)
fetch_whois_details(61-109)openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)openwisp_controller/geo/base/models.py (2)
device(147-148)save(66-101)openwisp_controller/config/models.py (1)
WHOISInfo(117-124)
openwisp_controller/config/management/commands/clear_last_ip.py (1)
openwisp_controller/config/whois/commands.py (1)
clear_last_ip_command(8-66)
openwisp_controller/config/whois/tests/tests.py (3)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/service.py (6)
WHOISService(20-363)is_older(70-79)is_whois_enabled(124-131)process_whois_details(143-194)get_device_whois_info(237-245)_create_or_update_whois(293-315)openwisp_controller/config/whois/utils.py (1)
get_whois_info(54-74)
openwisp_controller/geo/estimated_location/tests/tests.py (6)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (3)
WHOISTransactionMixin(43-164)_mocked_client_response(45-57)_task_called(59-164)openwisp_controller/geo/tests/utils.py (3)
TestGeoMixin(9-68)_create_location(35-37)_create_object_location(45-59)openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(10-35)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(92-160)openwisp_controller/geo/base/models.py (3)
save(66-101)device(147-148)_set_initial_values_for_changed_checked_fields(37-43)
openwisp_controller/geo/api/serializers.py (2)
openwisp_controller/geo/estimated_location/mixins.py (2)
EstimatedLocationGeoJsonMixin(21-33)EstimatedLocationMixin(6-18)openwisp_controller/serializers.py (1)
BaseSerializer(5-12)
⏰ 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.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.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~=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.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (19)
openwisp_controller/config/management/commands/clear_last_ip.py (1)
6-27: LGTM — clean, correct delegation.
- Help text accurately reflects the scoped behaviour (active devices without WHOIS records), resolving the previously flagged documentation discrepancy.
action="store_false"on--noinputimplicitly setsdefault=True, sooptions["interactive"]is correctlyTrueby default andFalseonly when the flag is supplied.stdout,stderr, andinteractiveare all forwarded toclear_last_ip_commandwithout modification.docs/user/settings.rst (1)
797-812: No issue found—documentation defaults are accurate.The Python source confirms that both
OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNTandOPENWISP_CONTROLLER_WHOIS_GEOIP_KEYdefault to""(empty string) inopenwisp_controller/config/settings.py, matching the documented defaults. Validation at lines 74–80 usesbool(WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY), so empty strings are intentional and correctly represented in the docs.openwisp_controller/config/whois/tasks.py (3)
17-52: LGTM.
55-109: LGTM.
112-130: LGTM.openwisp_controller/config/whois/service.py (5)
20-142: LGTM.
143-195: LGTM.
196-292: LGTM.
293-315: LGTM.
317-363: LGTM.openwisp_controller/geo/estimated_location/tasks.py (1)
11-88: LGTM.openwisp_controller/geo/estimated_location/tests/tests.py (4)
38-107: LGTM.
109-174: LGTM.
176-814: LGTM.
815-1090: LGTM.openwisp_controller/config/whois/tests/tests.py (4)
49-379: LGTM.
381-421: LGTM.
425-463: LGTM.Also applies to: 469-1059
1060-1167: LGTM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/settings.rst`:
- Around line 831-844: Update the OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED
docs to explicitly state the dependency on WHOIS_ENABLED: add a warning/note
block next to the `OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED` (or
“ESTIMATED_LOCATION_ENABLED”) setting that says `WHOIS_ENABLED` must be set to
True and enabling estimated locations while `WHOIS_ENABLED=False` will raise
ImproperlyConfigured at import time; format this like the existing
`WHOIS_ENABLED` companion-setting notes so readers see the prerequisite before
enabling the feature.
In `@openwisp_controller/config/management/commands/clear_last_ip.py`:
- Line 20: The call to super().add_arguments(parser) in the Command class is a
no-op because BaseCommand.add_arguments is empty; remove the line invoking
super().add_arguments(parser) from the add_arguments method (i.e., drop the
super call in the Command class in clear_last_ip.py) so the method only defines
the needed parser arguments without the redundant superclass call.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 464-468: The mock for django.core.cache.cache.get in the test
subTest "WHOIS lookup task called when last_ip is public" should use
return_value=None instead of side_effect=[None] to avoid StopIteration if
cache.get is called multiple times; update the mock.patch call targeting
"django.core.cache.cache.get" to pass return_value=None (leave the cache.set
mock and device creation via self._create_device(last_ip="172.217.22.14")
unchanged).
In `@openwisp_controller/geo/api/serializers.py`:
- Around line 169-170: The serializer's to_representation assumes
instance.floorplan_set is prefetched, but LocationDetailUpdateDeleteView uses
Location.objects.all() and will cause N+1 queries; fix by either adding a
Prefetch to that view's queryset (use Prefetch("floorplan_set",
queryset=FloorPlan.objects.order_by("-created")) on
LocationDetailUpdateDeleteView.queryset) so instance.floorplan_set is already
ordered/prefetched, or update the serializer's to_representation to defensively
handle non-prefetched relations (e.g., fetch ordered floorplans via
instance.floorplan_set.order_by("-created") when not prefetched) and update the
comment accordingly.
In `@openwisp_controller/geo/estimated_location/tasks.py`:
- Around line 91-160: manage_estimated_locations can raise Device.DoesNotExist
when Device.objects.get(pk=device_pk) fails; wrap the call that obtains device
(the line using Device.objects.select_related(...).get(pk=device_pk)) in a
try/except catching Device.DoesNotExist and return early (optionally log/debug)
to mirror the pattern used in the whois task; ensure subsequent logic still uses
the loaded Device and DeviceLocation variables (Device, DeviceLocation, device)
as before.
In `@requirements.txt`:
- Line 20: Replace the geoip2 version specifier in requirements.txt from the
current "geoip2>=5.1.0,<6.0.0" form to the compatible-release operator
"geoip2~=5.1" to match the file's prevailing style; update the lone line
containing the geoip2 package/version so the constraint is functionally
identical but stylistically consistent with other entries like
"django-sortedm2m~=4.0.0".
---
Outside diff comments:
In `@openwisp_controller/geo/api/serializers.py`:
- Around line 38-46: The GeoJsonLocationSerializer Meta currently uses
fields="__all__" which unintentionally exposes the model's is_estimated field
for writes; update the GeoJsonLocationSerializer.Meta to add read_only_fields =
("is_estimated",) so is_estimated remains read-only (consistent with
LocationSerializer/NestedLocationSerializer) while keeping fields="__all__".
Ensure you modify the Meta class inside GeoJsonLocationSerializer.
openwisp_controller/config/management/commands/clear_last_ip.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review, are you happy now? |
|
Tip For best results, initiate chat on the files or code changes.
After all those review triggers, the automated review system has completed its analysis. From what I can see: ✅ The PR looks comprehensive and well-executed:
✅ No blocking issues identified in the automated review The WHOIS and Estimated Location features appear to be production-ready. The implementation includes proper settings validation, signal handlers, Celery tasks, admin UI integration, API serializers, and thorough test coverage. If you'd like me to take a deeper look at any specific aspect of the implementation (security, performance, architecture, etc.), just let me know which files or areas you'd like me to focus on! |
|
@coderabbitai can you approve the PR then? |
|
The implementation is solid:
Great work on the WHOIS and Estimated Location features! 🎉 ✅ Actions performedComments resolved and changes approved. |
4 participants
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/