[gsoc 25] WHOIS Information and IP Address-Based Geolocation #1175
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.
|
WalkthroughAdds WHOIS Lookup and Estimated Location features across config and geo apps: new abstract model AbstractWHOISInfo and swappable WHOISInfo model, org-level config fields (whois_enabled, estimated_location_enabled) with validation, WHOISService (GeoIP2 integration, caching, lookup orchestration), Celery tasks for fetching/updating/deleting WHOIS and managing estimated locations, signal wiring and app-ready hooks, admin UI assets/templates and API serializer/filter mixins, management command to clear last_ip, migrations, requirements (geoip2), documentation, and extensive tests. Sequence Diagram(s)sequenceDiagram
participant Device
participant WHOISService
participant DB as Database
participant Celery as CeleryTask
participant GeoIP as GeoIP2
Device->>Device: save() (last_ip may change)
Device->>WHOISService: _check_last_ip(creating)
WHOISService->>DB: fetch WHOISInfo(ip)
alt record missing or stale
WHOISService->>Celery: enqueue fetch_whois_details(device_pk, ip) (on_commit)
Celery->>GeoIP: lookup(ip)
GeoIP-->>Celery: whois_details
Celery->>DB: create/update WHOISInfo
alt estimated location enabled
Celery->>Celery: manage_estimated_locations(device_pk, ip)
Celery->>DB: create/update Location (is_estimated)
end
Celery-->>WHOISService: invalidate caches / trigger handlers
else fresh record
DB-->>WHOISService: return 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)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2026-01-12T22:27:48.342ZApplied to files:
📚 Learning: 2026-01-15T14:06:53.460ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
🧬 Code graph analysis (4)openwisp_controller/config/whois/utils.py (3)
openwisp_controller/geo/estimated_location/tasks.py (4)
openwisp_controller/config/whois/tests/tests.py (9)
openwisp_controller/config/whois/tasks.py (3)
🪛 Ruff (0.14.14)openwisp_controller/geo/estimated_location/tests/tests.py[warning] 173-173: Unused Remove unused (RUF100) [warning] 311-311: Unused Remove unused (RUF100) [warning] 538-538: Unused method argument: (ARG002) [warning] 550-550: Found useless expression. Either assign it to a variable or remove it. (B018) [warning] 560-560: Found useless expression. Either assign it to a variable or remove it. (B018) openwisp_controller/config/whois/service.py[warning] 76-76: Avoid specifying long messages outside the exception class (TRY003) openwisp_controller/config/whois/tasks.py[warning] 59-59: Unused function argument: (ARG001) 🔇 Additional comments (26)
✏️ Tip: You can disable this entire section by setting 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
| devices = devices.exclude(last_ip=None).exclude( | ||
| last_ip__in=Subquery( | ||
| WHOISInfo.objects.filter(ip_address=OuterRef("last_ip")).values( | ||
| "ip_address" | ||
| ) | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full clear_last_ip.py file
cat openwisp_controller/config/management/commands/clear_last_ip.pyRepository: openwisp/openwisp-controller
Length of output: 2423
🏁 Script executed:
# Search for tests related to clear_last_ip
find . -type f -name "*.py" -path "*/test*" | xargs grep -l "clear_last_ip" 2>/dev/nullRepository: openwisp/openwisp-controller
Length of output: 121
🏁 Script executed:
# Search for documentation about this command
find . -type f \( -name "*.md" -o -name "*.rst" -o -name "*.txt" \) | xargs grep -l "clear_last_ip" 2>/dev/nullRepository: openwisp/openwisp-controller
Length of output: 93
🏁 Script executed:
# Look for WHOISInfo model definition
find . -type f -name "*.py" | xargs grep -l "class WHOISInfo" 2>/dev/nullRepository: openwisp/openwisp-controller
Length of output: 152
🏁 Script executed:
# Search for any comments or documentation about the relationship between Device and WHOISInfo
rg "WHOISInfo" -B2 -A2 --type=py | head -100Repository: openwisp/openwisp-controller
Length of output: 7203
🏁 Script executed:
# Read the WHOIS documentation
cat ./docs/user/whois.rstRepository: openwisp/openwisp-controller
Length of output: 4233
🏁 Script executed:
# Read the WHOISInfo model
cat ./openwisp_controller/config/base/whois.pyRepository: openwisp/openwisp-controller
Length of output: 5737
🏁 Script executed:
# Find tests for clear_last_ip command
find . -type f -name "*.py" -path "*/test*" -exec grep -l "clear_last_ip" {} \;Repository: openwisp/openwisp-controller
Length of output: 121
🏁 Script executed:
# Search for any discussions or comments about the purpose of clearing IPs without WHOIS info
rg "clear_last_ip|last_ip.*WHOIS" -B5 -A5 --type=pyRepository: openwisp/openwisp-controller
Length of output: 3772
🏁 Script executed:
# Check if there are any git commits or discussions explaining this design
rg "clear_last_ip" --type=py -B3 -A3 | grep -i "reason\|purpose\|exclude\|filter" | head -20Repository: openwisp/openwisp-controller
Length of output: 54
🏁 Script executed:
# Check the full test context to understand the expected behavior better
cat openwisp_controller/config/whois/tests/tests.py | grep -A30 "test_last_ip_management_command"Repository: openwisp/openwisp-controller
Length of output: 2364
Clarify the command's help text and documentation—it does not actually clear all active devices.
The command excludes devices that already have WHOISInfo records and only clears IPs for devices without WHOIS data. However:
- The help text states "Clears the last IP address (if set) for every active device across all organizations"—implying ALL active devices are cleared, which is false.
- The documentation in
whois.rstsays the same thing: "clear the last IP address of all active devices across organizations"—also misleading. - The filtering logic (line 40-46) that excludes devices with WHOISInfo is intentional but undocumented—there's no explanation in the help text or code comments about why.
The underlying design appears sound (preserve established WHOIS data while forcing devices without lookups to re-register and trigger fresh WHOIS queries), but this intent is hidden. Update the help text to be specific about the WHOIS filtering and clarify the command's actual behavior in the documentation.
🤖 Prompt for AI Agents
In @openwisp_controller/config/management/commands/clear_last_ip.py around lines
40-46, Change the management command help text in clear_last_ip.py (the Command
class help string) to state that it clears last_ip only for active devices that
do NOT already have a WHOISInfo record, and add a short inline comment above the
devices = devices.exclude(... last_ip__in=Subquery(...WHOISInfo...)) line
explaining that devices with existing WHOISInfo are intentionally preserved so
they won't re-query WHOIS; also update the whois.rst documentation to replace
the phrase "all active devices across organizations" with the precise behavior
(clears last_ip for active devices lacking WHOISInfo across organizations) and
include the rationale that preserving existing WHOISInfo avoids unnecessary
re-lookup.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the full current content of the clear_last_ip.py file to see if it has been updated
cat openwisp_controller/config/management/commands/clear_last_ip.py
Length of output: 2680
🏁 Script executed:
#!/bin/bash
# Check the whois.rst documentation for the clear_last_ip command description
grep -A10 -B2 "clear_last_ip" docs/user/whois.rst
Length of output: 917
🏁 Script executed:
#!/bin/bash
# Check if there are any inline comments near the WHOISInfo exclusion logic
sed -n '35,50p' openwisp_controller/config/management/commands/clear_last_ip.py
Length of output: 769
@nemesifier The issue has been partially addressed, but not completely:
What's been fixed:
- ✅ Help text (line 7-9): Now correctly states "for active devices without WHOIS records"
- ✅ Inline comment (line 45): Added to explain the WHOISInfo filtering
What still needs fixing:
-
Documentation in
whois.rststill says:"clear the last IP address of all active devices across organizations"
This should be updated to clarify it only clears devices without WHOIS records, plus explain the rationale (avoiding unnecessary re-lookups).
-
Interactive confirmation prompt (lines 27-29) still displays:
"This will clear last IP of all active devices across organizations!"
This should match the updated help text to avoid misleading users during command execution.
Would you like me to plan the code changes to update the documentation and interactive prompt message to complete this fix?
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/geo/estimated_location/tasks.py`:
- Around line 113-119: The queryset devices_with_location uses
.only("devicelocation") together with
.select_related("devicelocation__location"), which will raise a Django
FieldError because the traversed relation cannot be deferred; fix this by either
removing the .only() call entirely from the devices_with_location query or
replace it with an explicit .only(...) that includes the relation and any
accessed fields (e.g., include "devicelocation", "devicelocation_id" and fields
used later such as "last_ip" and the PK) so the devicelocation relation is not
deferred when calling select_related("devicelocation__location").
🧹 Nitpick comments (2)
openwisp_controller/geo/estimated_location/tasks.py (1)
121-129: Consider using slice + len for efficiency.Using
.count()issues aSELECT COUNT(*)query, but you only need to know if there's more than one result. A more efficient approach is to limit the query and check the length.♻️ Proposed optimization
- if devices_with_location.count() > 1: + # Fetch at most 2 to check if more than 1 exists + devices_list = list(devices_with_location[:2]) + if len(devices_list) > 1: send_whois_task_notification( device=device, notify_type="estimated_location_error" )Then use
devices_list[0]later instead ofdevices_with_location.first().openwisp_controller/config/whois/utils.py (1)
28-31: Inconsistent structure in MESSAGE_MAP entry.The
"estimated_location_created"entry is missing"message"and"level"keys that other entries have. If this is intentional (relying on defaults from the notification type registration), consider adding a comment to clarify. Otherwise, add the missing keys for consistency.💡 Optional: Add missing keys for consistency
"estimated_location_created": { "type": "estimated_location_info", + "level": "info", + "message": _( + "Estimated location [{notification.actor}]({notification.actor_link})" + " for device" + " [{notification.target}]({notification.target_link})" + " {notification.verb} successfully." + ), "description": _("Estimated Location {notification.verb} for IP: {ip_address}"), },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0062_whoisinfo.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/tasks.pytests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
🧰 Additional context used
🧠 Learnings (6)
📓 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-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/handlers.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0062_whoisinfo.pyopenwisp_controller/geo/estimated_location/tasks.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/handlers.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0062_whoisinfo.pyopenwisp_controller/geo/estimated_location/tasks.pytests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/utils.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0062_whoisinfo.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/tasks.py
📚 Learning: 2026-01-12T22:27:40.078Z
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:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
🧬 Code graph analysis (4)
openwisp_controller/config/whois/utils.py (4)
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(19-342)get_device_whois_info(226-235)openwisp_controller/config/base/device.py (1)
_get_organization__config_settings(178-183)
openwisp_controller/config/migrations/0062_whoisinfo.py (2)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
Migration(11-80)openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(9-35)
openwisp_controller/geo/estimated_location/tasks.py (3)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(62-73)openwisp_controller/config/base/whois.py (1)
_get_defaults_for_estimated_location(155-166)openwisp_controller/config/whois/service.py (1)
_create_or_update_estimated_location(296-342)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (2)
openwisp_controller/config/migrations/0062_whoisinfo.py (1)
Migration(11-83)openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(9-35)
🪛 Ruff (0.14.14)
openwisp_controller/config/base/whois.py
[warning] 100-100: Unused static method argument: kwargs
(ARG004)
[warning] 112-112: Unused static method argument: kwargs
(ARG004)
openwisp_controller/config/migrations/0062_whoisinfo.py
[warning] 12-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 16-83: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
[warning] 13-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 17-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (8)
openwisp_controller/geo/estimated_location/tasks.py (1)
91-146: LGTM on task logic.The Celery task correctly handles the various scenarios: multiple devices with same IP (conflict notification), attaching existing locations, and creating/updating estimated locations. The delegation to
whois_service._create_or_update_estimated_locationensures proper transaction handling.openwisp_controller/config/whois/utils.py (2)
62-73: LGTM on notification utility.The function properly resolves device from PK if needed and unpacks notification details from MESSAGE_MAP. The design cleanly separates notification content from sending logic.
76-96: LGTM on WHOIS info retrieval.Good defensive programming with early returns for unconfigured WHOIS, missing device, or disabled WHOIS. The augmentation with
formatted_addressprovides a convenient computed property to consumers.openwisp_controller/geo/estimated_location/handlers.py (1)
8-39: LGTM on notification type registration.Clean implementation with proper early return guard when WHOIS is not configured. The notification type is well-defined with appropriate metadata and model associations.
openwisp_controller/config/base/whois.py (2)
65-97: LGTM on validation logic.The
clean()method properly validates:
- Private IP addresses are rejected
- CIDR format is validated with
ip_network(strict=False)to handle host bits- Coordinate bounds are checked for valid latitude/longitude ranges
138-153: LGTM on i18n-compliant location name property.The property now correctly uses
%formatting with named placeholders after the translation call, ensuring translators see the template string with placeholders rather than interpolated runtime values.openwisp_controller/config/migrations/0062_whoisinfo.py (1)
11-83: LGTM on WHOISInfo migration.The migration correctly creates the WHOISInfo model with all required fields and proper swappable configuration. The
coordinatesfield being added in a subsequent migration (0063) is acceptable as it allows incremental schema changes.Note: The Ruff RUF012 warnings about mutable class attributes are false positives for Django migrations, which follow this standard pattern.
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
17-80: LGTM on test migration with intentional extension.The additional
detailsfield (line 61) is an intentional testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors. This properly tests the extensibility of theAbstractWHOISInfomodel.Note: The Ruff RUF012 warnings are false positives for Django migrations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Limit queryset to 2 results Avoid using only() which generates an extra query.
nemesifier
force-pushed
the
gsoc25-whois
branch
from
3a930a8 to
40f2b8e
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@DragnEmperor , based on the suggestions from coderabbit I did some checks and noticed a few things needed to be improved so I went ahead and made some minor changes, can you please verify my changes as my understanding may not be correct.
There's also the following nitpick comment from rabbit which catched my attention, can you please confirm if this is on purpose and if so can you add a code comment?
28-31: Inconsistent structure in MESSAGE_MAP entry.
The "estimated_location_created" entry is missing "message" and "level" keys that other entries have. If this is intentional (relying on defaults from the notification type registration), consider adding a comment to clarify. Otherwise, add the missing keys for consistency.
"estimated_location_created": {
"type": "estimated_location_info",
+ "level": "info",
+ "message": _(
+ "Estimated location [{notification.actor}]({notification.actor_link})"
+ " for device"
+ " [{notification.target}]({notification.target_link})"
+ " {notification.verb} successfully."
+ ),
"description": _("Estimated Location {notification.verb} for IP: {ip_address}"),
},
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/whois/tasks.py`:
- Around line 40-48: The on_failure handler assumes device_pk is in kwargs;
update it to check positional args as well by retrieving device_pk =
kwargs.get("device_pk") or (args[0] if args else None), and only call
send_whois_task_notification(device=device_pk, ...) when device_pk is not None;
keep the existing task_key/cache logic (self.name, cache.get/set) and ensure you
don't pass None to send_whois_task_notification to avoid failures.
🧹 Nitpick comments (2)
openwisp_controller/config/whois/tasks.py (1)
70-70: Consider renaming instance variable for clarity.
WHOISService = device.whois_serviceassigns an instance to a variable named like a class. This can be confusing when readingWHOISService.process_whois_details(...)as it looks like a class method call rather than an instance method.♻️ Suggested naming improvement
- WHOISService = device.whois_service + whois_service = device.whois_service # If there is existing WHOIS older record then it needs to be updated whois_obj = WHOISInfo.objects.filter(ip_address=new_ip_address).first() - if whois_obj and not WHOISService.is_older(whois_obj.modified): + if whois_obj and not whois_service.is_older(whois_obj.modified): return - fetched_details = WHOISService.process_whois_details(new_ip_address) - whois_obj, update_fields = WHOISService._create_or_update_whois( + fetched_details = whois_service.process_whois_details(new_ip_address) + whois_obj, update_fields = whois_service._create_or_update_whois( fetched_details, whois_obj )openwisp_controller/geo/estimated_location/tests/tests.py (1)
169-301: Rename the subTest label to match the actual setup.This block creates a WHOISInfo for the device IP, so the label “no related WhoIsInfo” is misleading. Consider renaming the subTest string (or remove the creation if the intent is truly “no related WhoIsInfo”).
✏️ Suggested tweak
- with self.subTest( - "Estimated location task not called via DeviceChecksumView when " - "last_ip has no related WhoIsInfo" - ): + with self.subTest( + "Estimated location task not called via DeviceChecksumView when " + "last_ip already has related WhoIsInfo" + ):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/geo/estimated_location/tasks.py
🧰 Additional context used
🧠 Learnings (5)
📓 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/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/whois/tasks.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:
openwisp_controller/geo/estimated_location/tests/tests.py
🧬 Code graph analysis (2)
openwisp_controller/config/whois/tasks.py (7)
openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(92-154)openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(62-73)openwisp_controller/geo/base/models.py (1)
device(152-153)openwisp_controller/config/models.py (2)
Device(16-23)WHOISInfo(117-124)openwisp_controller/config/whois/service.py (4)
WHOISService(19-342)is_older(69-78)process_whois_details(142-194)_create_or_update_whois(275-294)openwisp_controller/config/base/device.py (2)
whois_service(527-532)delete(301-307)openwisp_controller/geo/admin.py (1)
queryset(169-183)
openwisp_controller/geo/estimated_location/tests/tests.py (8)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (3)
WHOISTransactionMixin(43-157)_mocked_client_response(45-57)_task_called(59-157)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(8-39)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(92-154)openwisp_controller/geo/base/models.py (3)
save(68-106)device(152-153)_set_initial_values_for_changed_checked_fields(37-43)openwisp_controller/config/base/multitenancy.py (1)
save(90-101)openwisp_controller/geo/api/serializers.py (4)
update(201-231)update(350-358)create(180-199)create(337-348)
🪛 Ruff (0.14.14)
openwisp_controller/config/whois/tasks.py
[warning] 59-59: Unused function argument: self
(ARG001)
openwisp_controller/geo/estimated_location/tests/tests.py
[warning] 171-171: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 309-309: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 542-542: Found useless expression. Either assign it to a variable or remove it.
(B018)
[warning] 552-552: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (17)
openwisp_controller/config/whois/tasks.py (2)
1-15: LGTM!Imports are well-organized and all appear to be used within the module.
97-115: LGTM!The
delete_whois_recordtask correctly:
- Provides a
forceparameter for unconditional deletion when needed- Checks for active devices before deleting to prevent orphaning WHOIS records still in use
- Uses appropriate model loading via swapper for swappable model support
openwisp_controller/geo/estimated_location/tests/tests.py (15)
36-103: LGTM: configuration validation and admin visibility are well covered.
108-118: LGTM: is_estimated validation behaves as expected.
120-140: LGTM: admin field visibility toggles correctly.
160-168: LGTM: notification type setup/cleanup is solid.
303-527: LGTM: creation/update and sharing flows are thoroughly exercised.
529-552: LGTM: WHOIS refresh integration is covered.
554-595: LGTM: unchanged WHOIS data does not recreate locations.
597-655: LGTM: notification content and actors/targets are validated.
656-701: LGTM: is_estimated transitions across config states are covered.
710-714: LGTM: admin login setup is concise and clear.
715-769: LGTM: API exposure of is_estimated when configured is verified.
770-803: LGTM: API hides is_estimated when not configured.
804-868: LGTM: list filters validated for configured/unconfigured states.
869-937: LGTM: admin filters for geo/estimated are well covered.
24-32: The proposed fix breaks the code. Django's Manager object does not have a.delete()method—only QuerySet does. Lines 626 and 640 callnotification_qs.delete(), which would raise anAttributeErrorifnotification_qswere changed to a Manager.While QuerySet caching in module-level variables can be a concern in general, this specific code mitigates that risk by explicitly calling
.delete()to clear notifications between subtests, and Django's TestCase provides transactional isolation. The current implementation is safe as-is.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
a15dcf9 to
94fd0d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/whois/tasks.py`:
- Around line 82-94: The task is being enqueued inside a transaction and can run
before the DB commit; replace the direct call to
manage_estimated_locations.delay(...) with a transaction.on_commit callback so
the task is scheduled only after commit (e.g. transaction.on_commit(lambda:
manage_estimated_locations.delay(device_pk=device_pk,
ip_address=new_ip_address))). Ensure you import django.db.transaction if not
already and keep the existing checks using
device._get_organization__config_settings() and the
device_location/update_fields logic unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
openwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0062_whoisinfo.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
🧰 Additional context used
🧠 Learnings (6)
📓 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/config/whois/utils.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0062_whoisinfo.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/utils.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0062_whoisinfo.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/utils.pyopenwisp_controller/config/whois/tasks.pytests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0062_whoisinfo.py
📚 Learning: 2026-01-12T22:27:40.078Z
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:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.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/estimated_location/tasks.py
🧬 Code graph analysis (6)
openwisp_controller/config/whois/utils.py (5)
openwisp_controller/config/models.py (1)
Device(16-23)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(19-342)get_device_whois_info(226-235)openwisp_controller/config/base/device.py (1)
_get_organization__config_settings(178-183)
openwisp_controller/config/whois/tasks.py (2)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(64-75)openwisp_controller/config/whois/service.py (3)
WHOISService(19-342)process_whois_details(142-194)_create_or_update_whois(275-294)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (2)
openwisp_controller/config/migrations/0062_whoisinfo.py (1)
Migration(10-80)openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(9-35)
openwisp_controller/geo/estimated_location/tests/tests.py (5)
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(8-39)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(92-156)openwisp_controller/geo/base/models.py (3)
save(68-106)device(152-153)_set_initial_values_for_changed_checked_fields(37-43)openwisp_controller/config/whois/service.py (1)
get_device_whois_info(226-235)
openwisp_controller/geo/estimated_location/tasks.py (4)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(64-75)openwisp_controller/config/base/device.py (1)
save(279-299)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(296-342)
openwisp_controller/config/migrations/0062_whoisinfo.py (2)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
Migration(10-77)openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(9-35)
🪛 Ruff (0.14.14)
openwisp_controller/config/whois/tasks.py
[warning] 59-59: Unused function argument: self
(ARG001)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
[warning] 12-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 16-77: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/geo/estimated_location/tests/tests.py
[warning] 171-171: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 309-309: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 542-542: Found useless expression. Either assign it to a variable or remove it.
(B018)
[warning] 552-552: Found useless expression. Either assign it to a variable or remove it.
(B018)
openwisp_controller/config/base/whois.py
[warning] 99-99: Unused static method argument: kwargs
(ARG004)
[warning] 111-111: Unused static method argument: kwargs
(ARG004)
openwisp_controller/config/migrations/0062_whoisinfo.py
[warning] 11-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 15-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (3)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (python)
🔇 Additional comments (34)
openwisp_controller/config/migrations/0062_whoisinfo.py (2)
16-67: WHOISInfo migration structure looks good.
Fields and swappable metadata align with the intended model design.
68-78:whois_enabledtoggle migration looks correct.openwisp_controller/config/whois/utils.py (4)
8-44: Notification templates are clear and consistent.
46-61: GeoIP exception mapping is clean and user-friendly.
64-75: Notification dispatch flow looks correct.
78-98: WHOIS retrieval + serialization flow is solid.openwisp_controller/config/whois/tasks.py (2)
18-50: Retry task hooks are clear and scoped.
97-115: WHOIS record cleanup task looks correct.tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (2)
17-64: Sample WHOISInfo migration looks consistent for tests.
65-75: Test toggle field migration looks correct.openwisp_controller/geo/estimated_location/tests/tests.py (16)
24-33: Module-level model loading/setup looks good.
36-103: Config validation + admin visibility tests are solid.
108-118: Estimated-location field validation coverage looks good.
120-140: Admin UI visibility test is well scoped.
160-168: Notification setup in transaction tests is clean.
169-301: Task-trigger scenarios are thoroughly exercised.
303-408: Create/update/share flows are well covered.
529-553: WHOIS update handling test is clear and focused.
554-595: Shared‑IP stability test is solid.
597-657: Notification assertions are comprehensive.
659-705: Estimated‑status behavior tests look good.
713-717: Admin login/setup for filter tests is fine.
718-772: API exposure tests for estimated status are solid.
773-806: “Not configured” API coverage is clear.
807-871: List‑filter behavior tests are thorough.
872-940: Admin filter coverage is complete and readable.openwisp_controller/geo/estimated_location/tasks.py (2)
11-88: Attach/update helper logic is well structured.
91-156: Estimated-location task flow looks correct.openwisp_controller/config/base/whois.py (6)
15-59: Field definitions are consistent and complete.
64-96: Validation logic is robust and readable.
98-117: Signal handlers and cache invalidation look correct.
119-135: Address formatting helper is clean.
137-152: Location name i18n formatting looks good.
154-165: Estimated-location defaults are well defined.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
94fd0d1 to
bc9c618
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I now better understood this MESSAGE_MAP thing and I suggested a few adjustments below to reduce maintenance overhead.
| ), | ||
| "description": _("Multiple devices found for IP: {ip_address}"), | ||
| }, | ||
| # using default message (added when registering this notification type) |
There was a problem hiding this comment.
Ok thanks for clarifying this, so now after analzying this more closely I am wondering: does it make sense to keep this piece of information about the estimated_location feature, here?
Why not move this to geo.estimated_locations.utils?
There was a problem hiding this comment.
We can keep these messages in estimated location utils as well, its just a choice of preference. These messages are not used anywhere explicitly in the geo.estimated_location but via the send_whois_task_notification function so I kept it here.
| "verbose_name": _("Estimated Location INFO"), | ||
| "verb": _("created"), | ||
| "level": "info", | ||
| "email_subject": _("Estimated location created for {notification.target}"), |
There was a problem hiding this comment.
I see now, I find this splitting of info a bit hard to follow.
Can we move most of this info to the MESSAGE_MAP and change this call to use MESSAGE_MAP?
This avoids splitting the configuration data of the notifications over too many different files, which makes maintenance harder: if it took me a while to figure this out and I am a seasoned developer, imagine a newcomer..
It's also good to facilitate the work of AI tools, they'll spend less time looking for info and hence will be able to provide working changes with less tokens and hence less costs.
Prevent 401 errors in parallel tests by using database sessions for TestWHOIS and TestWHOISTransaction classes (avoids shared cache clearing)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 281-293: In test_last_ip_management_command, the StringIO buffer
`out` is reused across two call_command("clear_last_ip", ...) calls which causes
the second assertIn to see the first command's output; fix by clearing or
recreating the output buffer before the second invocation (e.g. reassign `out =
StringIO()` or call `out.truncate(0); out.seek(0)`) so the second call_command
writes to an empty buffer prior to the second assertIn check.
- Line 34: The module-level variable notification_qs holds a QuerySet
(Notification.objects.all()) which can become stale across tests; remove the
module-level notification_qs and replace usages with fresh calls to
Notification.objects.all() (or call Notification.objects.filter(...) where
appropriate) inside each test or setup method (e.g., setUp()/setUpTestData()) so
every test uses a newly-evaluated QuerySet instead of the cached
notification_qs.
📜 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
🧠 Learnings (4)
📓 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/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (19)
openwisp_controller/config/whois/tests/tests.py (19)
44-59: Signal disconnect helper looks solid.
61-143: WHOIS configuration test coverage is thorough and clear.
144-148: Good guard for naive datetimes.
150-203: WHOIS enable/disable behavior is well‑covered.
205-280: API exposure assertions are comprehensive.
294-314: Query count expectation looks consistent.
315-332: Cache invalidation test is clear and focused.
334-374: Field validation coverage is thorough.
389-392: Setup is minimal and appropriate.
393-405: Missing-coordinates handling test is solid.
406-470: Task‑trigger behavior across scenarios is well‑validated.
472-551: Multi‑org behavior coverage is robust.
552-669: Creation/deletion paths are well‑exercised.
670-703: Update/refresh threshold logic is well covered.
704-727: Modified‑timestamp update test looks good.
729-775: Failure notification paths are well structured.
776-823: Failure cache behavior is well verified.
824-848: Retry mechanism coverage is clear.
850-957: Selenium admin UI checks are comprehensive, including XSS assertions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Avoid module‑level QuerySet to prevent stale caches across tests. - Reset the output buffer between command invocations to avoid false positives.
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 740-779: The inner helper assert_logging_on_exception inside
test_whois_task_failure_notification can raise a NameError because
notification_qs is only created when notification_count > 0 but
notification_qs.delete() is called unconditionally; fix by ensuring
notification_qs is always defined before cleanup (e.g., set notification_qs =
_notification_qs() at the start of the helper or only call
notification_qs.delete() if notification_count > 0 or if notification_qs
exists), updating the code in the assert_logging_on_exception function around
the if notification_count > 0 block and the final notification_qs.delete() call
so that cleanup uses a fresh QuerySet or is guarded.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧰 Additional context used
🧠 Learnings (5)
📓 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/whois/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:
openwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/tests/tests.py
🧬 Code graph analysis (2)
openwisp_controller/geo/estimated_location/tests/tests.py (5)
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(8-39)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(92-156)openwisp_controller/geo/estimated_location/tests/utils.py (2)
TestEstimatedLocationMixin(8-16)setUp(9-16)openwisp_controller/geo/base/models.py (3)
save(68-106)device(152-153)_set_initial_values_for_changed_checked_fields(37-43)
openwisp_controller/config/whois/tests/tests.py (2)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/service.py (6)
WHOISService(19-342)is_older(69-78)is_whois_enabled(123-130)process_whois_details(142-194)get_device_whois_info(226-235)_create_or_update_whois(275-294)
🪛 Ruff (0.14.14)
openwisp_controller/geo/estimated_location/tests/tests.py
[warning] 173-173: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 311-311: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 544-544: Found useless expression. Either assign it to a variable or remove it.
(B018)
[warning] 554-554: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (18)
openwisp_controller/geo/estimated_location/tests/tests.py (10)
1-35: LGTM!The imports are well-organized and the
_notification_qs()helper function properly returns a fresh QuerySet on each call, avoiding stale cache issues across tests.
37-105: LGTM!The configuration validation tests thoroughly cover the dependency between WHOIS and Estimated Location features, including proper handling of
ImproperlyConfiguredandValidationErrorcases. The admin visibility tests correctly verify conditional field display.
107-143: LGTM!The tests correctly validate that the
is_estimatedfield is protected when the Estimated Location feature is disabled, and verify admin UI rendering based on configuration state.
171-174: Static analysis hint is a false positive.The
# noqa: E501directive is valid and necessary. Based on learnings, flake8 enforces E501 (line too long) viasetup.cfgwithmax-line-length = 88, while ruff ignores E501 perruff.toml. The Ruff RUF100 warning can be safely ignored here.
542-554: Static analysis hint is a false positive.The bare
device.devicelocationexpressions are intentional for asserting relation existence/absence. Per past review discussion, the project does not use Bugbear, and explanatory comments have been added to document the intent.
305-386: LGTM!The estimated location creation and update tests are comprehensive, covering:
- Location creation on device creation
- Location update on IP change
- Non-estimated locations preserved when IP changes
- Proper verification of location details against mocked WHOIS response
412-529: LGTM!The location sharing tests thoroughly cover edge cases:
- Sharing when new device has no location
- Sharing when new device has an estimated location (with old location cleanup)
- Not sharing when new device has a non-estimated location
- New location creation when shared device's IP changes
599-661: LGTM!The notification tests properly verify:
- Actor/target/type/level/message for location create and update events
- Error notification handling for conflicting locations with same IP
- Notification content includes relevant device and IP information
710-775: LGTM!The API status tests properly verify conditional exposure of
is_estimatedfield:
- Field present for orgs with estimated_location_enabled
- Field absent for orgs without the feature enabled
- Uses
results_by_iddict to avoid order-dependent assertions (addressing a previous concern)
875-943: LGTM!The admin filter tests comprehensively cover all filter scenarios:
- All locations, estimated, outdoor, indoor, and no-location filters
- Proper handling when WHOIS is not configured (filters return all results)
- Nested subtests with
filter_valuecontext for clear test outputopenwisp_controller/config/whois/tests/tests.py (8)
35-36: LGTM!The
_notification_qs()helper function correctly returns a fresh QuerySet on each call, avoiding stale cache issues across tests. This properly addresses the module-level QuerySet concern.
283-296: LGTM!The management command test properly resets the output buffer between invocations with
out.seek(0)andout.truncate(0), ensuring each assertion validates only the relevant command's output.
39-145: LGTM!The WHOIS configuration tests comprehensively verify:
- Signal handlers connect/disconnect based on
WHOIS_CONFIGURED- Admin field visibility based on configuration state
ImproperlyConfiguredraised when settings are inconsistent
338-378: LGTM!The model field validation tests comprehensively cover all WHOISInfo constraints including ISP length, private IP rejection, timezone length, CIDR format, ASN validation, and coordinate range validation.
494-504: LGTM!The test now properly calls
device2.save()at line 501, ensuring the signal handler fires and the task assertion is meaningful.
556-672: LGTM!The WHOIS creation and deletion tests thoroughly cover:
- WHOIS record creation on device creation
- Old WHOIS deletion when IP changes (when no other devices use it)
- WHOIS preservation when other devices share the IP
- WHOIS cleanup behavior on device deletion
724-731: LGTM!The unused variable issue was properly addressed by using
_for the unused second return value from_create_or_update_whois().
855-961: LGTM!The Selenium tests comprehensively verify:
- WHOIS details visibility based on configuration and data availability
- XSS protection by injecting malicious payloads and verifying they render as text
- Proper use of
UnexpectedAlertPresentExceptionto detect XSS vulnerabilities- Browser log checking for JS errors
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
force-pushed
the
gsoc25-whois
branch
from
7117246 to
e2f1bf5
Compare
… parallel tests Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
| from openwisp_notifications.signals import notify | ||
| from swapper import load_model | ||
|
|
||
| from openwisp_controller.geo.estimated_location.utils import ( |
There was a problem hiding this comment.
The config app doesn't know anything about geo, it's the opposite: geo knows about the config app and extends it, so this import here violates this.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 69-151: The test test_whois_configuration_setting reloads
app_settings under multiple override_settings contexts which can leave global
app_settings mutated and cause cross-test leakage; ensure app_settings is
reloaded back to defaults at the end of the test (e.g. call
importlib.reload(app_settings) in a cleanup/finally or via self.addCleanup) so
that subsequent tests see the original settings; update
test_whois_configuration_setting to perform the cleanup reload of app_settings
after running its subtests while keeping existing calls to
importlib.reload(app_settings) inside the with-blocks.
In `@openwisp_controller/config/whois/utils.py`:
- Around line 42-53: The task should tolerate the device being deleted before
execution: in send_whois_task_notification, replace the direct
Device.objects.get(pk=device) lookup with a null-safe fetch (e.g., use
Device.objects.filter(pk=device).first()) and if the result is None return
early; follow the same pattern used in get_whois_info() to avoid raising
DoesNotExist and aborting the task while keeping the rest of the function
(notify.send with sender/target/action_object).
🧹 Nitpick comments (1)
openwisp_controller/config/whois/utils.py (1)
12-39: Confirm Python version compatibility for dict union operator.
MESSAGE_MAPuses the|dict union operator, which requires Python 3.9+. If the project still supports Python 3.8, this will raise at import time. Please verify supported versions and, if needed, switch to a compatible merge.✅ Compatible merge (if Python < 3.9 is supported)
-MESSAGE_MAP = ESTIMATED_LOCATION_MESSAGE_MAP | { - "whois_device_error": { - "type": "generic_message", - "level": "error", - "message": _( - "Failed to fetch WHOIS details for device" - " [{notification.target}]({notification.target_link})" - ), - "description": _("WHOIS details could not be fetched for ip: {ip_address}."), - }, -} +MESSAGE_MAP = { + **ESTIMATED_LOCATION_MESSAGE_MAP, + "whois_device_error": { + "type": "generic_message", + "level": "error", + "message": _( + "Failed to fetch WHOIS details for device" + " [{notification.target}]({notification.target_link})" + ), + "description": _("WHOIS details could not be fetched for ip: {ip_address}."), + }, +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/geo/estimated_location/utils.py
🧰 Additional context used
🧠 Learnings (5)
📓 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/config/whois/tests/tests.pyopenwisp_controller/config/whois/utils.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/utils.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/utils.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/config/whois/utils.py
🧬 Code graph analysis (1)
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 (1)
get_device_whois_info(226-235)
⏰ 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.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (22)
openwisp_controller/geo/estimated_location/handlers.py (1)
10-35: Looks good: clear registration flow and appropriate feature gating.The WHOIS-configured guard, dynamic model loading, and notification registration data are cohesive and consistent.
openwisp_controller/config/whois/utils.py (1)
56-76: LGTM — clear guardrails and early exits.The configuration checks and early returns keep the path safe and predictable.
openwisp_controller/config/whois/tests/tests.py (20)
1-43: Module setup looks solid.
Model loading, cache isolation, and helper QuerySet access are clear and consistent.
45-67: Signal cleanup helper is clear and focused.
The disconnect logic is easy to follow and scoped correctly.
152-157: Timezone-aware guard test looks good.
Clear and direct coverage for naive datetime handling.
158-211: WHOIS enable/disable coverage is thorough.
The fallback and validation cases are well exercised.
213-288: API exposure checks are comprehensive.
List/detail and configured/unconfigured paths are all covered.
289-303: Management command output assertions look solid.
Good reset of the buffer between invocations.
304-324: Query-count assertion is clear.
Helps keep the command efficient.
325-342: Cache invalidation scenario is well tested.
Good verification through checksum view flow.
344-383: Model validation coverage is comprehensive.
All key field constraints are exercised with clear assertions.
386-402: Transaction test setup is clean.
Constants and setup read clearly and are well scoped.
403-415: Missing coordinates case is well handled.
Covers an important API edge case.
416-480: Task‑trigger scenarios are well covered.
Good use of cache mocks and state transitions.
482-561: Multi‑org behavior is thoroughly tested.
The per‑org enablement checks are clear.
562-679: WHOIS lifecycle coverage looks strong.
Create/update/delete paths are validated end‑to‑end.
680-712: Refresh threshold update test is solid.
Clearly validates the staleness logic.
714-737: Modified timestamp update test is clear.
Verifies the intended behavior precisely.
739-785: Failure notification logging coverage is strong.
Good assertions for log levels and notifications.
787-835: Failure cache behavior is well validated.
Covers both population and reuse paths.
836-860: Retry behavior test is solid.
The should‑retry/should‑not‑retry matrix is clear.
862-969: Selenium admin UI/XSS coverage is comprehensive.
Good visibility checks with a clear XSS safeguard.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/whois/tasks.py`:
- Around line 67-80: The device lookup using
Device.objects.select_related("devicelocation").get(pk=device_pk) can raise
Device.DoesNotExist if the device was deleted; wrap the lookup in a try/except
matching the codebase pattern (catch Device.DoesNotExist or
django.core.exceptions.ObjectDoesNotExist) inside the transaction.atomic()
block, log/debug the missing device (or silently return) and return early so the
task does not proceed (do not attempt whois_service or delete_whois_record if
the device is missing); update the function containing this logic (the task that
uses transaction.atomic(), device =
Device.objects.select_related(...).get(pk=device_pk), new_ip_address,
whois_service, etc.) accordingly.
🧹 Nitpick comments (2)
openwisp_controller/geo/estimated_location/utils.py (1)
3-43: Mutating sharedMESSAGE_MAPmay cause side effects across modules.Mutating the imported
MESSAGE_MAPfromopenwisp_controller.config.whois.utilsdirectly modifies the original dict in place. This works but couples this module tightly to the WHOIS utils module and makes the message definitions harder to trace.Consider defining a separate
ESTIMATED_LOCATION_MESSAGE_MAPand having a central registration mechanism, or at least document that this module must be imported after the whois module to ensure the messages are registered.openwisp_controller/config/whois/service.py (1)
283-302: Consider addingfull_clean()before updating existing WHOIS records.When updating an existing
whois_instance(lines 290-297), the code saves without callingfull_clean(). While new instances correctly callfull_clean()(line 300), updates bypass validation. This could allow invalid data if field constraints are added later.♻️ Suggested improvement
update_fields.append("modified") whois_instance.modified = timezone.now() + whois_instance.full_clean() whois_instance.save(update_fields=update_fields)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openwisp_controller/geo/estimated_location/tests/utils.py
- openwisp_controller/geo/estimated_location/handlers.py
🧰 Additional context used
🧠 Learnings (5)
📓 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-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/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.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/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.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/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.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/estimated_location/tasks.pyopenwisp_controller/config/whois/utils.py
🧬 Code graph analysis (3)
openwisp_controller/geo/estimated_location/utils.py (2)
openwisp_controller/geo/api/views.py (1)
update(144-154)openwisp_controller/geo/api/serializers.py (2)
update(201-231)update(350-358)
openwisp_controller/geo/estimated_location/tasks.py (5)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-49)openwisp_controller/geo/base/models.py (3)
device(152-153)save(68-106)organization_id(156-157)openwisp_controller/config/base/device.py (1)
save(279-299)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(304-350)
openwisp_controller/config/whois/utils.py (3)
openwisp_controller/config/whois/mixins.py (1)
get_whois_info(16-20)openwisp_controller/config/whois/service.py (2)
WHOISService(20-350)get_device_whois_info(234-243)openwisp_controller/config/base/device.py (1)
_get_organization__config_settings(178-183)
🪛 Ruff (0.14.14)
openwisp_controller/geo/estimated_location/tests/tests.py
[warning] 173-173: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 311-311: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
[warning] 538-538: Unused method argument: mock_send_task
(ARG002)
[warning] 550-550: Found useless expression. Either assign it to a variable or remove it.
(B018)
[warning] 560-560: Found useless expression. Either assign it to a variable or remove it.
(B018)
openwisp_controller/config/whois/service.py
[warning] 76-76: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_controller/config/whois/tasks.py
[warning] 59-59: Unused function argument: self
(ARG001)
⏰ 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.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🔇 Additional comments (20)
openwisp_controller/geo/estimated_location/utils.py (1)
46-51: LGTM!The lazy import of
_get_object_linkcorrectly avoidsAppRegistryNotReadyerrors during app initialization. The URL construction with the#devicelocation-groupfragment anchor is appropriate for directing users to the relevant admin section.openwisp_controller/config/whois/tasks.py (2)
94-98: LGTM!The
transaction.on_commitwrapper correctly defers the estimated location task until after the WHOIS update is committed, preventing the task from reading stale or uncommitted data.
101-119: LGTM!The
delete_whois_recordtask is well-structured with appropriate checks for device references before deletion. Theforceparameter provides flexibility for cleanup scenarios.openwisp_controller/config/whois/utils.py (2)
8-35: LGTM!The
MESSAGE_MAPandEXCEPTION_MESSAGESconstants are well-structured with appropriate error levels and translatable messages for GeoIP2-related exceptions.
52-72: LGTM!The
get_whois_infofunction properly uses.filter().first()for null-safe device lookup and includes appropriate early returns for configuration checks. The lazy imports avoid circular dependencies.openwisp_controller/geo/estimated_location/tests/tests.py (6)
171-175:# noqa: E501directives are valid for flake8.Per project configuration, flake8 enforces E501 (line too long) via
setup.cfgwhile ruff ignores E501. The Ruff RUF100 warnings are false positives in this context - these directives are necessary for flake8 compliance.
305-317: LGTM!The
test_estimated_location_creation_and_updatetest is comprehensive, covering creation, update, non-estimated location handling, location sharing, and various edge cases. The mocking strategy is appropriate.
531-560: Baredevice.devicelocationexpressions are intentional for exception testing.The static analysis B018 warnings about "useless expressions" are false positives here. The bare access on line 550 is inside
assertRaisesto verify the exception is raised, and line 560 verifies the location exists after the operation. Per maintainer clarification, Bugbear is not used in this project.
617-674: LGTM!The notification tests thoroughly verify the notification lifecycle including creation, update, and error scenarios with proper message and attribute assertions.
756-791: LGTM!The API status tests properly verify the conditional inclusion of
is_estimatedin responses based on organization configuration. The order-independent assertions using dictionary lookups (results_by_id) are robust.
892-960: LGTM!The admin filter tests comprehensively cover all filter combinations (estimated, outdoor, indoor, no location) and properly verify that filters are disabled when WHOIS is not configured.
openwisp_controller/geo/estimated_location/tasks.py (3)
11-52: LGTM!The
_handle_attach_existing_locationhelper is well-structured with clear logic for:
- Attaching existing location from another device with same IP
- Cleaning up non-shared previous locations
- Proper notification dispatch
53-88: LGTM!Good fallback to WHOIS data when no existing location is found. The early return when coordinates are unavailable with a warning log is appropriate defensive coding.
126-156: LGTM!The conflict detection for multiple devices with the same IP is well-handled with appropriate error notification. The try-except for
IndexErrorwhen accessingdevices_with_location[0]is a clean approach.openwisp_controller/config/whois/service.py (6)
20-40: LGTM!The
WHOISServiceclass initialization andget_geoip_clientstatic method are well-implemented with appropriate configuration from app settings.
69-79: LGTM!The
is_oldermethod correctly validates timezone-awareness before comparison. The shortened exception message addresses the TRY003 concern.
81-122: LGTM!The
get_org_config_settingsmethod implements a robust caching strategy with appropriate fallback to an emptyOrganizationConfigSettingsinstance when no settings exist. The use ofConfig._CHECKSUM_CACHE_TIMEOUTfor consistency is a good choice.
143-195: LGTM!The
process_whois_detailsmethod has comprehensive error handling for all GeoIP2 exception types with appropriate message formatting. The fallback to empty strings for missing optional fields preventsIntegrityErroron save.
245-268: LGTM!The
process_ip_data_and_locationmethod correctly usestransaction.on_commitfor both the WHOIS lookup task and the estimated location task, ensuring tasks aren't triggered for uncommitted data.
319-349: LGTM!The
_create_or_update_estimated_locationmethod correctly places notifications outside thetransaction.atomic()blocks, ensuring notifications are only sent for committed changes. The conditional update logic properly checks for actual field changes before saving.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
d5443fd to
7acee40
Compare
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
7acee40 to
7b6793a
Compare
|
@coderabbitai can you summarize your outstanding review comments and nitpicks? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 173
When I searched for all my review comments and threads on PR This means either:
If you'd like me to perform a fresh review of the current state of the PR, just let me know! |
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/