[gsoc 25] WHOIS Information and IP Address-Based Geolocation #1175
Conversation
Closes #1026 Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
- Estimated geographic location logic, closes #1034. - Added notification and admin warning, closes #1035. - Set location estimated flag to ``False`` on manual update, closes #1036. - Added admin and API filters for estimated locations, closes #1028. --------- Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
Added required checks for ensuring WHOIS records older than a certain configurable threshold will be updated along with the coordinates. Closes #1058 Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
pandafy
moved this to In Progress
in [GSoC25] WHOIS Information and IP Address-Based Geolocation
DragnEmperor
moved this from In Progress
to Needs Review
in [GSoC25] WHOIS Information and IP Address-Based Geolocation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a WHOIS IP-geolocation subsystem and an Estimated Location feature: new AbstractWHOISInfo/WHOISInfo models and migrations; WHOISService for GeoIP lookups and caching; Celery tasks and handlers for fetching/updating WHOIS and managing estimated locations; signal wiring and admin UI assets (JS/CSS/templates) to surface WHOIS data; API serializers/mixins exposing whois_info and is_estimated and filters for estimated locations; organization-level config toggles with validation; device hooks to trigger WHOIS on last_ip changes; management command to clear last_ip; geoip2 dependency and extensive tests. Sequence Diagram(s)sequenceDiagram
participant Client
participant DeviceModel as Device
participant WHOISService as WHOISService
participant Celery as CeleryQueue
participant GeoIP as GeoIP2
participant DB
participant GeoSystem as EstimatedLocationSystem
Client->>DeviceModel: save/update last_ip
DeviceModel->>WHOISService: _check_last_ip(creating?)
WHOISService->>WHOISService: check org/global config & freshness
alt needs lookup
WHOISService->>Celery: enqueue fetch_whois_details(device_pk, ip)
Celery->>GeoIP: request WHOIS/GeoIP data
GeoIP-->>Celery: return data or error
Celery->>DB: create/update WHOISInfo
Celery->>WHOISService: notify persistence complete
alt estimated location enabled
WHOISService->>Celery: enqueue manage_estimated_locations(device_pk, ip)
Celery->>GeoSystem: manage_estimated_locations task
GeoSystem->>DB: create/update Location & DeviceLocation
GeoSystem->>DB: attach existing shared location or notify on conflicts
end
else refresh needed
WHOISService->>DB: update existing WHOISInfo (no new lookup)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-01-12T22:27:48.342ZApplied to files:
📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🧬 Code graph analysis (2)openwisp_controller/config/whois/service.py (2)
openwisp_controller/config/management/commands/clear_last_ip.py (3)
🪛 Ruff (0.14.11)openwisp_controller/config/management/commands/clear_last_ip.py36-36: Avoid specifying long messages outside the exception class (TRY003) openwisp_controller/config/base/whois.py101-101: Unused static method argument: (ARG004) 121-121: Unused static method argument: (ARG004) ⏰ 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)
🔇 Additional comments (15)
✏️ 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.
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.
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
940fa3f to
cc65526
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @openwisp_controller/config/base/whois.py:
- Around line 149-159: The _location_name property uses f-strings inside the
translation function _(…), which interpolates values before translation; change
these to use translatable strings with named placeholders and perform formatting
after translation. For the branch that has a location, replace _(f"{location}
~Estimated Location: {self.ip_address}~") with something like _("%(location)s
~Estimated Location: %(ip)s~") % {"location": location, "ip": self.ip_address},
and for the fallback replace _(f"Estimated Location: {self.ip_address}") with
_("Estimated Location: %(ip)s") % {"ip": self.ip_address}; keep references to
formatted_address and ip_address and ensure translation strings include the
placeholders exactly as shown.
- Around line 36-40: The ASN CharField named "asn" currently uses max_length=6
which can truncate 32-bit ASNs; change its max_length to 10 (to cover up to
4,294,967,295), update any associated validators/tests that assume 6 digits, and
add a Django migration so the DB schema is updated; the change targets the "asn"
models.CharField in whois.py.
In @openwisp_controller/geo/base/models.py:
- Around line 72-83: The code must guard against Django's models.DEFERRED
sentinel: before comparing _initial_address or _initial_geometry with current
values in the WHOISService branch (used in the if not _set_estimated and (...)
check), ensure each of _initial_address and _initial_geometry is not
models.DEFERRED; only perform comparisons when the deferred sentinel is absent.
Likewise, when resetting self.is_estimated in the else branch, do not assign
self._initial_is_estimated if it equals models.DEFERRED — only assign when
_initial_is_estimated is not models.DEFERRED (otherwise preserve current
self.is_estimated). Use the models.DEFERRED sentinel checks around the
comparisons and the assignment to prevent corrupting the boolean field; keep the
existing name cleanup (re.sub) behavior unchanged.
In @openwisp_controller/geo/estimated_location/tests/tests.py:
- Around line 845-850: The subTest label is incorrect: update the string passed
to self.subTest in the block that calls self.client.get(path, {"with_geo":
"false"}) so it correctly describes the case being tested (e.g. "Test Without
Geo Filter" or "Test Devices Without Location"); locate the subTest near the
assertions referencing indoor_device, outdoor_device, and estimated_device and
replace "Test Indoor Location Filter" with a descriptive name matching the
with_geo=false scenario.
🧹 Nitpick comments (14)
docs/user/settings.rst (2)
799-799: Minor formatting inconsistency forNonedefaults.Lines 799 and 811 show
**default**: None(unformatted), but similar settings elsewhere in the file (e.g., line 553 forOPENWISP_CONTROLLER_API_HOST) use**default**: \`None``` with backticks for consistency. Consider applying the same formatting style.Also applies to: 811-811
787-790: Clarify the warning about configuration requirements.The warning message "If these three settings are not configured as expected..." is vague. Consider being explicit that when
OPENWISP_CONTROLLER_WHOIS_ENABLEDis set toTrue, bothOPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNTandOPENWISP_CONTROLLER_WHOIS_GEOIP_KEYmust be set as non-empty strings, otherwise anImproperlyConfiguredexception will be raised.📝 Suggested improvement
.. warning:: - If these three settings are not configured as expected, an - ``ImproperlyConfigured`` exception will be raised. + If ``OPENWISP_CONTROLLER_WHOIS_ENABLED`` is set to ``True``, both + ``OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT`` and + ``OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY`` must be set with non-empty + values, otherwise an ``ImproperlyConfigured`` exception will be raised.openwisp_controller/config/whois/tests/tests.py (2)
175-175: Replacegetattrwith direct attribute access.Using
getattrwith a constant attribute name provides no benefit over direct attribute access and reduces readability.♻️ Suggested fix
- self.assertEqual(getattr(org_settings_obj, "whois_enabled"), True) + self.assertEqual(org_settings_obj.whois_enabled, True)Also apply to lines 181 and 192:
- 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, + )
158-165: Consider simplifying the assertion pattern.The try/except wrapping
self.fail()is used in several places. While functional, this could be simplified sinceassertEqualalready provides a clear failure message.♻️ Optional simplification
- 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.", + )The same pattern appears at lines 315-321, 328-336, and 352-358. The try/except adds no value since
assertEqualalready fails with a descriptive message showing expected vs actual values.openwisp_controller/geo/estimated_location/tests/tests.py (5)
62-71: Remove unnecessary try-except block.The
self.assertEqualwill raise anAssertionErrorif the assertion fails. Wrapping it in a try-except only to callself.fail()is redundant and makes the test less readable.♻️ Simplify the assertion
- try: - self.assertEqual( - context_manager.exception.message_dict[ - "estimated_location_enabled" - ][0], - "Estimated Location feature requires " - + "WHOIS Lookup feature to be enabled.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict[ + "estimated_location_enabled" + ][0], + "Estimated Location feature requires " + + "WHOIS Lookup feature to be enabled.", + )
119-125: Remove unnecessary try-except block.Same issue as the previous test—the try-except wrapper around
self.assertEqualadds no value and reduces code clarity.♻️ Simplify the assertion
- try: - self.assertEqual( - context_manager.exception.message_dict["is_estimated"][0], - "Estimated Location feature required to be configured.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["is_estimated"][0], + "Estimated Location feature required to be configured.", + )
173-173: Remove unusednoqadirective.The blanket
noqacomment on Line 173 is unused and should be removed for cleaner code.♻️ Remove the unused directive
- @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" + )Based on learnings, static analysis tools like Ruff can help identify such issues.
287-287: Remove unusednoqadirectives.Lines 287, 290, and 293 contain unused blanket
noqadirectives that should be removed.♻️ Remove the unused directives
@mock.patch( - "openwisp_controller.config.whois.service.send_whois_task_notification" # noqa + "openwisp_controller.config.whois.service.send_whois_task_notification" ) @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" # noqa + "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" ) @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" # noqa + "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" )Based on learnings, static analysis tools like Ruff can help identify such issues.
Also applies to: 290-290, 293-293
599-599: Calling private method in test may indicate a design issue.Line 599 calls
_set_initial_values_for_changed_checked_fields(), which appears to be a private method (indicated by the leading underscore). While this might be necessary to set up test state, it could indicate that the API doesn't provide a proper way to reset or initialize these values.Consider whether a public method should be exposed for this functionality, or if the test could achieve the same result through the public API.
openwisp_controller/config/admin.py (1)
574-588: Good design for test isolation, with an optional style improvement.Converting the Media class attribute to a property allows settings to be overridden in tests, which is excellent for test isolation. The conditional inclusion of WHOIS assets based on
WHOIS_CONFIGUREDis implemented correctly.♻️ Optional: Consider iterable unpacking for slightly cleaner code
Line 579 could use iterable unpacking instead of concatenation for a more modern style:
- 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", ]openwisp_controller/config/base/device.py (1)
122-125: Use unpacking syntax for list concatenation.The list concatenation works correctly but could be more Pythonic and efficient.
♻️ Proposed fix using unpacking syntax
if app_settings.WHOIS_CONFIGURED: # Initial value for last_ip is required in WHOIS # to remove WHOIS info related to that ip address. - self._changed_checked_fields = self._changed_checked_fields + ["last_ip"] + self._changed_checked_fields = [*self._changed_checked_fields, "last_ip"]openwisp_controller/geo/base/models.py (1)
18-23: Consider annotating mutable class attribute withClassVar.The
_changed_checked_fieldslist is a mutable class attribute. While this pattern is common in Django models, annotating it withtyping.ClassVarmakes the intent explicit and satisfies static analysis.♻️ 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/geo/admin.py (1)
105-111: Consider converting fields to a list before modification.The
remove()call on line 110 assumesfieldsis a mutable list. While Django'sget_fields()typically returns a list, explicitly converting to a list ensures safety if a parent class returns a tuple.♻️ Proposed fix
def get_fields(self, request, obj=None): - fields = super().get_fields(request, obj) + fields = list(super().get_fields(request, obj)) org_id = obj.organization_id if obj else None if not WHOISService.check_estimate_location_configured(org_id): if "is_estimated" in fields: fields.remove("is_estimated") return fieldsopenwisp_controller/config/base/whois.py (1)
75-84: Consider using exception chaining for better error context.When re-raising exceptions, using
raise ... from epreserves the original exception chain, which helps with debugging and provides better error context.♻️ Suggested improvement
try: # strict is set to False to allow CIDR without a mask # e.g. 192.168.1.12/24 with strict False normalizes to # 192.168.1.0/24 else it would raise an error. ip_network(self.cidr, strict=False) except ValueError as e: raise ValidationError( {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} - ) + ) from e
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/user/settings.rstdocs/user/whois.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/static/whois/js/whois.jsopenwisp_controller/config/templates/admin/config/change_form.htmlopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openwisp_controller/config/static/whois/js/whois.js
- docs/user/whois.rst
🧰 Additional context used
🧬 Code graph analysis (7)
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/geo/admin.py (1)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-351)check_estimate_location_configured(128-134)
openwisp_controller/geo/base/models.py (2)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-351)check_estimate_location_configured(128-134)openwisp_controller/config/base/device.py (1)
_set_initial_values_for_changed_checked_fields(129-134)
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-41)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(77-134)openwisp_controller/geo/estimated_location/tests/utils.py (2)
TestEstimatedLocationMixin(8-15)setUp(9-15)openwisp_controller/geo/base/models.py (2)
save(59-83)device(129-130)
openwisp_controller/config/whois/tests/tests.py (3)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/base/whois.py (2)
device_whois_info_delete_handler(102-118)invalidate_org_settings_cache(123-129)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(233-242)
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/config/base/whois.py (4)
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/config/base/device.py (2)
Meta(112-119)delete(298-304)tests/openwisp2/sample_config/models.py (1)
Device(26-32)
🪛 Ruff (0.14.10)
openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_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/base/device.py
125-125: Consider [*self._changed_checked_fields, "last_ip"] instead of concatenation
Replace with [*self._changed_checked_fields, "last_ip"]
(RUF005)
openwisp_controller/geo/admin.py
117-117: Consider (*fields, "is_estimated") instead of concatenation
Replace with (*fields, "is_estimated")
(RUF005)
156-156: Unused method argument: request
(ARG002)
156-156: Unused method argument: model_admin
(ARG002)
169-169: Unused method argument: request
(ARG002)
openwisp_controller/geo/base/models.py
18-18: 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/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/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)
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)
⏰ 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). (9)
- 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.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.10 | django~=4.2.0
🔇 Additional comments (31)
openwisp_controller/config/whois/tests/tests.py (8)
1-32: LGTM!The imports are well-organized and appropriate for the test scope. Using
swapper.load_modelis correct for OpenWISP's pluggable model architecture.
196-271: Good API test coverage.The test comprehensively covers all combinations of
WHOIS_CONFIGUREDstate and WHOIS data existence for both list and detail endpoints.
272-303: LGTM!Good integration test that verifies both the management command functionality and cache invalidation behavior.
305-359: LGTM!Comprehensive validation tests covering all model field constraints. The coordinate boundary tests with parameterized cases (lines 342-358) are well structured.
361-408: Good use ofTransactionTestCasefor testing async task behavior.Using
TransactionTestCaseis the correct choice here since the code under test usestransaction.on_commit()for triggering background tasks, which requires actual database commits to function properly.
659-700: Good exception handling test coverage.The helper function
assert_logging_on_exceptioneffectively reduces duplication while testing all relevant GeoIP2 exception types. The test properly verifies notification creation, level, type, and message content.
775-809: Good XSS security test coverage.The test properly injects XSS payloads into WHOIS data fields and verifies that the raw script/HTML content is displayed as text rather than executed. Catching
UnexpectedAlertPresentExceptionis the right approach to detect if any XSS payload triggered.
706-714: LGTM!The browser log filtering helper correctly ignores console API messages and known benign warnings while still catching actual JavaScript errors.
openwisp_controller/geo/estimated_location/tests/tests.py (3)
1-32: LGTM!The imports and module setup are well-organized. All necessary dependencies for comprehensive testing of the estimated location feature are properly imported.
333-334: Consider the fragility of query count assertions.The use of
assertNumQuerieswith specific counts (14, 8, 2, 16) throughout these tests is quite fragile. Minor changes to the ORM queries, signal handlers, or caching behavior can break these assertions. However, if these assertions are intentionally used for performance regression testing, they may be acceptable.Consider whether these specific query counts are essential to the test's purpose or if they could be replaced with more flexible assertions.
Are these precise query counts critical for performance testing, or could the tests be made more resilient by removing or relaxing these assertions?
Also applies to: 355-356, 380-381, 407-408, 436-437, 469-470, 502-503
615-860: Well-structured API and admin filter tests.The filter tests provide comprehensive coverage of the API endpoints and admin interfaces under different WHOIS configuration states. The use of subtests makes the test output clear and maintainable.
openwisp_controller/config/admin.py (5)
34-34: LGTM!The import is clean and follows the project's module organization.
53-53: LGTM!The constant follows the existing pattern (see
prefixon line 52) and is used consistently for WHOIS asset paths.
942-946: LGTM! Efficient context injection.The implementation correctly injects WHOIS details into the context when available, avoiding the need for an additional AJAX request. The walrus operator usage is clean, and the comment clearly explains the benefit. Based on the relevant code snippets,
get_whois_infoincludes proper guards (checkingWHOIS_CONFIGUREDand organization settings) and usesselect_relatedfor efficient querying.
1382-1398: LGTM! Dynamic field configuration implemented correctly.The
ConfigSettingsFormandConfigSettingsInlineare well-designed, with fields dynamically included based on feature flags (REGISTRATION_ENABLED,WHOIS_CONFIGURED). This follows Django best practices for conditional admin form fields.Note: Static analysis warnings about unused
requestandobjparameters inget_fieldsare false positives—these parameters are required by the Django admin API signature.
1401-1403: LGTM! Inline ordering is correct.The wiring correctly inserts
ConfigSettingsInlineat position 0 and adjustslimits_inline_positionto 1 to maintain the proper order of inlines inOrganizationAdmin.openwisp_controller/config/templates/admin/config/change_form.html (2)
15-17: LGTM! Safe JSON embedding using Django best practices.The use of Django's
json_scripttemplate filter is the recommended approach for safely passing data from the backend to JavaScript, preventing XSS vulnerabilities.
25-29: LGTM! Correct JavaScript variable initialization.The parsing logic correctly retrieves and parses the JSON data embedded in the script element, making the WHOIS details available to the frontend JavaScript (whois.js). The pattern is consistent with other variable initializations in this template.
openwisp_controller/config/base/device.py (4)
1-1: LGTM: Required imports for WHOIS integration.The imports are necessary for the WHOIS feature implementation.
Also applies to: 22-22
310-312: LGTM: Defensive callable check.The callable check prevents AttributeError if a field in
_changed_checked_fieldslacks a corresponding change handler method. This is good defensive programming, especially with the addition of "last_ip" which is handled separately.
523-529: LGTM: Appropriate use of cached_property.The
whois_servicecached property provides convenient access to the WHOISService instance. Caching is safe here since the service accesses device state dynamically on each method call.
531-542: Verify the design: update_whois_info() called on every save.The docstring states this method processes details "if last_ip has changed or is being set for the first time," but the implementation also calls
update_whois_info()when the IP hasn't changed (line 541). Since_check_last_ipis invoked on everysave()operation (line 289), this means:
- Every device save (updating name, notes, or any field) triggers
update_whois_info()whenlast_ipis loaded but unchanged- This could cause performance overhead unless
update_whois_info()has efficient guards to skip unnecessary workQuestions:
- Is it intentional to check WHOIS staleness on every save, or should this use lazy evaluation (check only when WHOIS data is accessed)?
- Does
update_whois_info()have appropriate guards to avoid redundant processing?- Should the docstring be updated to reflect that WHOIS info may be refreshed even when IP hasn't changed?
Consider updating the docstring to accurately describe the behavior:
def _check_last_ip(self, creating=False): """ - Process details and location related to last_ip if last_ip has - changed or is being set for the first time. + Process WHOIS details and location related to last_ip when last_ip + changes or is set for the first time. Also checks if existing WHOIS + info needs updating on subsequent saves. """openwisp_controller/geo/base/models.py (3)
1-14: LGTM!The import additions are appropriate for the new functionality. The
remodule for regex,ValidationErrorfor model validation, andWHOISServicefor checking estimation configuration are all necessary for the implementation.
28-38: LGTM!The initialization logic correctly captures initial field values for change tracking. The handling of deferred fields using
get_deferred_fields()is appropriate and consistent with Django patterns. This mirrors the similar implementation inopenwisp_controller/config/base/device.py.
40-57: LGTM!The validation logic correctly prevents setting
is_estimated=Truewhen the estimated location feature is not configured for the organization. The condition properly handles both new records and updates.openwisp_controller/geo/admin.py (5)
15-16: LGTM!The imports are appropriate for the WHOIS configuration checks used throughout the admin classes.
113-118: LGTM!The readonly field logic correctly makes
is_estimatedread-only when the estimation feature is configured. The tuple concatenation works correctly; unpacking syntax(*fields, "is_estimated")is an optional style preference.
120-126: LGTM!The
change_viewoverride correctly passes theestimated_configuredcontext variable to the template for conditional rendering.
151-167: LGTM!The filter correctly adapts its title and lookup options based on WHOIS configuration. The unused
requestandmodel_adminarguments are required by Django'sSimpleListFilterinterface, so the static analysis warnings are false positives.
169-183: LGTM!The queryset logic correctly handles all filter cases:
- Estimated locations filtered by
is_estimated=True- Indoor/outdoor excludes estimated locations (important per the comment)
- "No Location" falls through to the
devicelocation__isnullcheckThe implementation properly ensures estimated locations don't appear in indoor/outdoor results since they default to outdoor type.
openwisp_controller/config/base/whois.py (1)
101-118: No issues found. The method_get_organization__config_settings()is properly defined in the Device model and used correctly here. The double underscore naming is intentional and consistent throughout the codebase.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @openwisp_controller/geo/base/models.py:
- Around line 59-88: The save() method currently only clears is_estimated on
manual edits when WHOISService.check_estimate_location_configured(...) is true
and also contains an elif that reverts is_estimated from _initial_is_estimated
when the feature is not configured; change save() so manual edits always clear
is_estimated unless _set_estimated is True (i.e., move the
address_changed/geometry_changed check out of the configured branch and set
self.is_estimated = False and strip the "~...~" from self.name when a manual
edit is detected), remove the elif that resets self.is_estimated from
_initial_is_estimated so we don’t revert changes when the feature is disabled,
and ensure update_fields handling adds 'is_estimated' and 'name' to
update_fields when those fields are changed (or treat update_fields=None as
full-save behavior) so those mutations aren’t dropped; reference the save
method, _set_estimated flag, WHOISService.check_estimate_location_configured,
_initial_address/_initial_geometry/_initial_is_estimated, and the
name/is_estimated attributes to locate changes.
In @openwisp_controller/geo/estimated_location/tests/tests.py:
- Around line 172-174: Remove the unused blanket "# noqa" comments on the
mock.patch decorator strings that patch
"openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay"
in tests (appears at the decorator around the test near the 172–174 region and
similarly at 287–294); simply delete the "# noqa" suffixes (or replace with a
specific code if a genuine lint suppression is required) so Ruff RUF100 is not
triggered while keeping the mock.patch string unchanged.
- Around line 652-691: Tests are brittle: they assume a fixed ordering of
response.data["results"] and they assert that "is_estimated" is not present and
then immediately index it, causing KeyError/flakiness. Update the list
assertions to locate result objects by comparing each result's "id" to
org1_location.id and org2_location.id (instead of using results[0]/results[1])
and then assert presence/absence or boolean value accordingly; for device
endpoints check response.data["location"]["properties"] using a presence check
before reading the key; for GeoJSON iterate response.data["features"] and for
each feature use existence checks (if "is_estimated" in feature["properties"]
then assert its boolean value; otherwise assert it's absent) rather than
asserting not-in then indexing.
🧹 Nitpick comments (7)
openwisp_controller/config/whois/tests/tests.py (1)
175-175: Consider using direct attribute access.Using
getattrwith a constant attribute name adds unnecessary indirection. Direct attribute access (org_settings_obj.whois_enabled) is clearer and equally safe.♻️ Proposed refactor
- 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"), + org_settings_obj.whois_enabled, app_settings.WHOIS_ENABLED, )Also applies to: 181-181, 192-192
openwisp_controller/config/base/whois.py (1)
82-84: Add exception chaining for better error context.When re-raising an exception within an
exceptclause, useraise ... from eto preserve the original exception chain, which aids in debugging.♻️ Proposed fix
except ValueError as e: raise ValidationError( {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} - ) + ) from eopenwisp_controller/geo/estimated_location/tests/tests.py (3)
35-53:importlib.reload(config_app_settings)inside tests risks leaking mutated module state to other tests.
Consider restoringopenwisp_controller.config.settingsback to baseline after each reload (especially the expected-raise reload).One way to contain the reload side effects
def test_estimated_location_configuration_setting(self): # reload app_settings to apply the overridden settings - importlib.reload(config_app_settings) + importlib.reload(config_app_settings) + self.addCleanup(importlib.reload, config_app_settings) with self.subTest( @@ with override_settings( OPENWISP_CONTROLLER_WHOIS_ENABLED=False, OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED=True, ): with self.assertRaises(ImproperlyConfigured): # reload app_settings to apply the overridden settings importlib.reload(config_app_settings) + # ensure we restore a clean module state after the failed reload + importlib.reload(config_app_settings)Also applies to: 88-93
31-32: Prefer querying notifications locally (and filtering bytype) instead of using module-levelnotification_qs.
This reduces coupling to unrelated notifications created by other code paths within the same test.Proposed tweak
-notification_qs = Notification.objects.all() +def _estimated_location_notifications(): + return Notification.objects.filter(type="estimated_location_info") @@ def _verify_notification(device, messages, notify_level="info"): - self.assertEqual(notification_qs.count(), 1) - notification = notification_qs.first() + qs = _estimated_location_notifications() + self.assertEqual(qs.count(), 1) + notification = qs.first()Also applies to: 519-534
57-72:try/except AssertionErroraroundassertEqualis redundant.
assertEqualalready prints a good diff; this extra wrapper makes failures noisier without adding signal.Also applies to: 117-126
openwisp_controller/geo/base/models.py (2)
18-23: Make_changed_checked_fieldsimmutable and annotate withClassVar(RUF012).
This avoids accidental mutation and satisfies Ruff’s “mutable class attribute” warning.Proposed diff
+from typing import ClassVar + class BaseLocation(OrgMixin, AbstractLocation): - _changed_checked_fields = ["is_estimated", "address", "geometry"] + _changed_checked_fields: ClassVar[tuple[str, ...]] = ( + "is_estimated", + "address", + "geometry", + )
28-39: Initial-value snapshot should be refreshed aftersave()(current snapshot can go stale).
Right now_initial_*values are set only in__init__, so multiplesave()calls on the same instance can keep treating fields as “changed” forever.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0062_whoisinfo.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧰 Additional context used
🧬 Code graph analysis (4)
openwisp_controller/geo/estimated_location/tests/tests.py (4)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(8-41)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(77-134)openwisp_controller/geo/base/models.py (2)
save(59-88)device(134-135)
openwisp_controller/config/whois/tests/tests.py (9)
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/config/base/whois.py (1)
device_whois_info_delete_handler(102-118)openwisp_controller/config/whois/service.py (1)
is_whois_enabled(137-144)openwisp_controller/config/base/config.py (1)
full_clean(567-571)openwisp_controller/geo/estimated_location/tests/utils.py (1)
setUp(9-15)openwisp_controller/geo/tests/test_admin.py (2)
setUp(28-30)setUp(174-175)openwisp_controller/config/tests/test_controller.py (1)
_create_org(44-49)openwisp_controller/geo/api/views.py (1)
update(143-153)
openwisp_controller/config/migrations/0062_whoisinfo.py (2)
openwisp_controller/config/migrations/0061_config_checksum_db.py (1)
Migration(35-56)openwisp_controller/config/base/config.py (1)
name(141-148)
openwisp_controller/config/base/whois.py (2)
openwisp_controller/config/whois/service.py (1)
get_cache_key(60-64)openwisp_controller/config/whois/tasks.py (1)
delete_whois_record(85-94)
🪛 Ruff (0.14.10)
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/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/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/geo/base/models.py
18-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
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)
⏰ 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). (6)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (3)
openwisp_controller/geo/estimated_location/tests/tests.py (1)
34-860: Overall coverage looks strong (config gating + admin + task flows + API exposure + filters).
Nice breadth: you’re exercising both configured/unconfigured behavior and a variety of update/share/conflict scenarios.openwisp_controller/geo/base/models.py (2)
1-5: No circular import risk exists withWHOISServiceimport.The
whois/service.pymodule does not import any geo models—it only imports from Django, third-party libraries, and usesswapper.load_modelfor lazy model loading. The dependency is unidirectional (geo models → whois service), and this pattern is already established in multiple places in the codebase (admin.py,mixins.py, tests) without issues.Likely an incorrect or invalid review comment.
40-58: No fix needed—WHOISService.check_estimate_location_configured()already handlesNonesafely.The method at lines 128–134 of
openwisp_controller/config/whois/service.pyincludes a guard check:if not org_id: return FalseThis returns
Falseimmediately iforganization_idisNoneor falsy, preventing any unexpected exceptions. The proposed diff is redundant.
| 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): | ||
| address_changed = ( | ||
| self._initial_address is not models.DEFERRED | ||
| and self._initial_address != self.address | ||
| ) | ||
| geometry_changed = ( | ||
| self._initial_geometry is not models.DEFERRED | ||
| and self._initial_geometry != self.geometry | ||
| ) | ||
| if not _set_estimated and (address_changed or geometry_changed): | ||
| self.is_estimated = False | ||
| if self.name: | ||
| # remove estimated status between '~' | ||
| self.name = re.sub(r"~[^~]*~", "", self.name) | ||
| elif self._initial_is_estimated is not models.DEFERRED: | ||
| self.is_estimated = self._initial_is_estimated | ||
| return super().save(*args, **kwargs) |
There was a problem hiding this comment.
save() logic: (1) manual edits should clear is_estimated consistently, (2) don’t “revert” is_estimated when not configured, (3) respect update_fields.
As written, manual edits only clear is_estimated when configured (docstring says otherwise), and the elif branch can prevent setting is_estimated=False when the feature isn’t configured. Also, update_fields may drop is_estimated/name changes silently.
Proposed diff
def save(self, *args, _set_estimated=False, **kwargs):
@@
- if WHOISService.check_estimate_location_configured(self.organization_id):
- address_changed = (
- self._initial_address is not models.DEFERRED
- and self._initial_address != self.address
- )
- geometry_changed = (
- self._initial_geometry is not models.DEFERRED
- and self._initial_geometry != self.geometry
- )
- if not _set_estimated and (address_changed or geometry_changed):
- self.is_estimated = False
- if self.name:
- # remove estimated status between '~'
- self.name = re.sub(r"~[^~]*~", "", self.name)
- elif self._initial_is_estimated is not models.DEFERRED:
- self.is_estimated = self._initial_is_estimated
- return super().save(*args, **kwargs)
+ address_changed = (
+ self._initial_address is not models.DEFERRED
+ and self._initial_address != self.address
+ )
+ geometry_changed = (
+ self._initial_geometry is not models.DEFERRED
+ and self._initial_geometry != self.geometry
+ )
+
+ if not _set_estimated and (address_changed or geometry_changed):
+ self.is_estimated = False
+ if self.name:
+ # remove estimated status between '~'
+ self.name = re.sub(r"~[^~]*~", "", self.name).strip()
+
+ update_fields = kwargs.get("update_fields")
+ if update_fields is not None:
+ kwargs["update_fields"] = set(update_fields) | {"is_estimated", "name"}
+
+ result = super().save(*args, **kwargs)
+ self._set_initial_values_for_changed_checked_fields()
+ return resultCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @openwisp_controller/geo/base/models.py around lines 59 - 88, The save()
method currently only clears is_estimated on manual edits when
WHOISService.check_estimate_location_configured(...) is true and also contains
an elif that reverts is_estimated from _initial_is_estimated when the feature is
not configured; change save() so manual edits always clear is_estimated unless
_set_estimated is True (i.e., move the address_changed/geometry_changed check
out of the configured branch and set self.is_estimated = False and strip the
"~...~" from self.name when a manual edit is detected), remove the elif that
resets self.is_estimated from _initial_is_estimated so we don’t revert changes
when the feature is disabled, and ensure update_fields handling adds
'is_estimated' and 'name' to update_fields when those fields are changed (or
treat update_fields=None as full-save behavior) so those mutations aren’t
dropped; reference the save method, _set_estimated flag,
WHOISService.check_estimate_location_configured,
_initial_address/_initial_geometry/_initial_is_estimated, and the
name/is_estimated attributes to locate changes.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai This was done to ensure that only when the feature is configured is_estimated is set to False on manual edits, else, it retains its original value (true or false).
There was a problem hiding this comment.
@DragnEmperor, I understand your intent, but there's a subtle issue with the current approach:
The problem with the elif branch:
When the feature is not configured, the elif branch actively reverts any manual changes to is_estimated:
elif self._initial_is_estimated is not models.DEFERRED:
self.is_estimated = self._initial_is_estimatedThis means if someone does:
location.is_estimated = False # Explicit change
location.save()When the feature is disabled, the elif will revert is_estimated back to its original database value, discarding the user's explicit assignment.
Suggested fix:
Remove the elif branch entirely. When the feature is disabled, Django should save whatever value is currently set on the field (normal Django behavior):
def save(self, *args, _set_estimated=False, **kwargs):
if WHOISService.check_estimate_location_configured(self.organization_id):
address_changed = (
self._initial_address is not models.DEFERRED
and self._initial_address != self.address
)
geometry_changed = (
self._initial_geometry is not models.DEFERRED
and self._initial_geometry != self.geometry
)
if not _set_estimated and (address_changed or geometry_changed):
self.is_estimated = False
if self.name:
self.name = re.sub(r"~[^~]*~", "", self.name)
# Removed elif - let Django save the current field value
return super().save(*args, **kwargs)This way:
- ✅ When feature enabled: manual edits clear
is_estimated(your intent) - ✅ When feature disabled: normal Django behavior applies (no forced reversion)
Also: Consider handling update_fields to ensure is_estimated and name changes aren't silently dropped when update_fields is passed to save().
There was a problem hiding this comment.
@coderabbitai any manual changes by user to the is_estimated field should also be discarded, if the feature is not enabled. This field should be allowed to change only if feature is configured.
update_fields change is a good suggestion
There was a problem hiding this comment.
@DragnEmperor, thank you for clarifying! That makes sense—the elif branch is indeed correct for protecting is_estimated from unauthorized manual changes when the feature is disabled.
Here's a refined version that adds the update_fields handling and ensures initial values are refreshed after save:
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 (only if estimated location
feature is enabled). When the feature is disabled, is_estimated is
protected from manual changes.
*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):
address_changed = (
self._initial_address is not models.DEFERRED
and self._initial_address != self.address
)
geometry_changed = (
self._initial_geometry is not models.DEFERRED
and self._initial_geometry != self.geometry
)
if not _set_estimated and (address_changed or geometry_changed):
self.is_estimated = False
if self.name:
# remove estimated status between '~'
self.name = re.sub(r"~[^~]*~", "", self.name)
# Ensure is_estimated and name are included in update_fields
update_fields = kwargs.get("update_fields")
if update_fields is not None:
kwargs["update_fields"] = set(update_fields) | {"is_estimated", "name"}
elif self._initial_is_estimated is not models.DEFERRED:
# Protect is_estimated from manual changes when feature is disabled
self.is_estimated = self._initial_is_estimated
update_fields = kwargs.get("update_fields")
if update_fields is not None:
kwargs["update_fields"] = set(update_fields) | {"is_estimated"}
result = super().save(*args, **kwargs)
# Refresh initial values after save
self._set_initial_values_for_changed_checked_fields()
return resultKey improvements:
- ✅ Adds
update_fieldshandling in both branches to ensureis_estimatedandnamechanges aren't dropped - ✅ Refreshes initial values after save to properly track future changes
- ✅ Updates docstring to clarify the protection behavior when feature is disabled
✏️ Learnings added
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:44.800Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
50bce2f to
5ea8448
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @openwisp_controller/geo/base/models.py:
- Around line 40-57: The clean() change-detection currently compares
_initial_is_estimated to is_estimated even when _initial_is_estimated may be
models.DEFERRED; update the conditional so it only compares values when the
initial value is not models.DEFERRED (mirror the save() check). Specifically, in
clean() replace the (_initial_is_estimated != self.is_estimated) check with a
guard like (self._initial_is_estimated is not models.DEFERRED and
self._initial_is_estimated != self.is_estimated) so deferred fields don’t
falsely trigger the WHOISService.check_estimate_location_configured validation
for is_estimated.
In @openwisp_controller/geo/estimated_location/tests/tests.py:
- Around line 683-689: The test contains a contradictory assertion: after
confirming for the org2_location branch that "is_estimated" is not in
i["properties"] (self.assertNotIn("is_estimated", i["properties"])), the next
line still accesses i["properties"]["is_estimated"] (self.assertFalse(...))
which will raise KeyError; remove the redundant
self.assertFalse(i["properties"]["is_estimated"]) line in the loop over
response.data["features"] (the block that checks org2_location.id) so the test
only asserts absence of the key for org2_location.
In
@tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:
- Around line 18-66: Update the WHOISInfo test migration to match the main
migration: remove the "details" field from the CreateModel fields list, reorder
the fields so "asn" appears before "isp" (keeping other fields and their
attributes unchanged), and add the model option "swappable":
"CONFIG_WHOISINFO_MODEL" to the options dict for the CreateModel declaration.
🧹 Nitpick comments (7)
openwisp_controller/geo/estimated_location/tests/tests.py (4)
62-71: Simplify assertion - try/except wrapper is unnecessary.The
try/exceptblock wrappingassertEqualis redundant. WhenassertEqualfails, it already raisesAssertionErrorwith a clear message including both expected and actual values. Theself.fail()call adds no diagnostic value.♻️ Suggested simplification
- try: - self.assertEqual( - context_manager.exception.message_dict[ - "estimated_location_enabled" - ][0], - "Estimated Location feature requires " - + "WHOIS Lookup feature to be enabled.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["estimated_location_enabled"][0], + "Estimated Location feature requires " + "WHOIS Lookup feature to be enabled.", + )
119-125: Same unnecessary try/except pattern.Apply the same simplification as noted earlier.
♻️ Suggested simplification
- try: - self.assertEqual( - context_manager.exception.message_dict["is_estimated"][0], - "Estimated Location feature required to be configured.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["is_estimated"][0], + "Estimated Location feature required to be configured.", + )
172-174: Remove unusednoqadirective.Static analysis indicates this
noqadirective is unused. The line no longer triggers a linting violation.♻️ Suggested fix
@mock.patch( - "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" # noqa + "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" )
286-294: Remove unusednoqadirectives.Static analysis indicates these
noqadirectives at lines 287, 290, and 293 are unused.♻️ Suggested fix
@mock.patch( - "openwisp_controller.config.whois.service.send_whois_task_notification" # noqa + "openwisp_controller.config.whois.service.send_whois_task_notification" ) @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" # noqa + "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" ) @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" # noqa + "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" )openwisp_controller/config/whois/tests/tests.py (1)
171-195: Simplify attribute access by removing unnecessarygetattrcalls.Lines 175, 181, and 192 use
getattr(org_settings_obj, "whois_enabled")with a constant attribute name. Direct attribute access is clearer and equally safe.♻️ Proposed refactor
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 static analysis hints.
openwisp_controller/config/base/whois.py (1)
75-84: Preserve exception context withraise ... from e.When catching and re-raising an exception as a different type, use
raise ... from eto preserve the original traceback. This helps with debugging by maintaining the full exception chain.♻️ Proposed refactor
if self.cidr: try: # strict is set to False to allow CIDR without a mask # e.g. 192.168.1.12/24 with strict False normalizes to # 192.168.1.0/24 else it would raise an error. ip_network(self.cidr, strict=False) except ValueError as e: raise ValidationError( {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} - ) + ) from eBased on static analysis hints.
openwisp_controller/geo/base/models.py (1)
18-18: Consider annotating mutable class attribute withClassVar.The static analysis tool correctly identifies that mutable class attributes should be annotated with
typing.ClassVarfor clarity and type safety.♻️ Proposed refactor
+from typing import ClassVar + from django.contrib.gis.db import models from django.core.exceptions import ValidationErrorclass BaseLocation(OrgMixin, AbstractLocation): - _changed_checked_fields = ["is_estimated", "address", "geometry"] + _changed_checked_fields: ClassVar[list[str]] = ["is_estimated", "address", "geometry"]
📜 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/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
🧰 Additional context used
🧬 Code graph analysis (3)
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/whois/service.py (2)
WHOISService(37-351)check_estimate_location_configured(128-134)
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/base/config.py (1)
name(141-148)
🪛 Ruff (0.14.10)
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/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/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/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/base/models.py
18-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
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)
🔇 Additional comments (9)
openwisp_controller/geo/estimated_location/tests/tests.py (4)
1-32: LGTM!The imports are well-organized and appropriate for the test module. The use of
swapper.load_modelfor dynamic model loading aligns with OpenWISP's swappable model pattern.
297-512: Well-structured transaction tests with comprehensive coverage.The
test_estimated_location_creation_and_updatemethod thoroughly covers:
- Location creation on device creation
- Location updates on IP changes
- Non-estimated location preservation
- Location sharing for same IP addresses
- Proper cleanup of orphaned locations
The use of
subTestandassertNumQueriesprovides good isolation and performance validation.
518-612: LGTM!The notification and estimated location status tests provide good coverage for:
- Notification creation for location create/update
- Error notifications for conflicting locations
- Estimated status preservation when feature is disabled
- Estimated status change when geometry is manually updated
725-859: LGTM!The filter tests comprehensively validate:
- API filtering by
is_estimatedwhen WHOIS is configured- Filter behavior when WHOIS is not configured (filter ignored, returns all)
- Admin changelist filters for estimated/outdoor/indoor/no location
- Proper fallback when WHOIS is not configured
openwisp_controller/geo/base/models.py (5)
1-2: LGTM! Imports are properly used.All new imports (
re,ValidationError,gettext_lazy, andWHOISService) are necessary and correctly utilized throughout the new methods.Also applies to: 4-5, 13-13
20-23: LGTM! Field definition is appropriate.The
is_estimatedfield is properly defined with sensible defaults and clear documentation.
28-30: LGTM! Initialization follows proper patterns.The
__init__method correctly initializes the parent class and sets up field change tracking.
32-38: LGTM! Deferred field handling is correct.The method properly distinguishes between deferred and loaded fields, which is essential for accurate change detection.
59-89: LGTM! Save method properly handles complex estimation logic.The
save()method correctly manages several scenarios:
- Properly checks for
DEFERREDbefore comparing field changes (lines 75, 79, 87)- Clears estimated status when users manually edit location data
- Preserves estimated status when the feature is disabled
- Uses the
_set_estimatedparameter to allow the WHOIS system to set estimations without triggering auto-clear logicThe regex pattern on line 86 correctly removes estimated markers from location names.
…n checks Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
5ea8448 to
120556d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
openwisp_controller/geo/base/models.py (1)
18-18: Consider annotating withClassVarfor type clarity.The static analysis tool flags this as a mutable class attribute. While this is a valid pattern for Django models tracking field changes, adding a type annotation would improve clarity and silence the linter.
Optional improvement
+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/geo/estimated_location/tests/tests.py (4)
173-173: Remove unusednoqadirective.The static analysis tool indicates this
noqadirective is no longer necessary. The line length appears to be within acceptable limits.- "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" # noqa + "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay"
286-294: Remove unusednoqadirectives.Static analysis indicates these
noqadirectives are unnecessary.Clean up unused directives
@mock.patch( - "openwisp_controller.config.whois.service.send_whois_task_notification" # noqa + "openwisp_controller.config.whois.service.send_whois_task_notification" ) @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" # noqa + "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" ) @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" # noqa + "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" )
626-632: Simplify_create_device_locationhelper.The intermediate
optionsdict is unnecessary.Simplified implementation
def _create_device_location(self, **kwargs): - options = dict() - options.update(kwargs) - device_location = self.object_location_model(**options) + device_location = self.object_location_model(**kwargs) device_location.full_clean() device_location.save() return device_location
31-31: Module-level queryset could cause test isolation issues.
notification_qsis defined at module level. While this works, if notifications are not properly cleaned between test methods, it could lead to flaky tests. Consider usingsetUp/tearDownor a property.openwisp_controller/config/base/whois.py (2)
81-84: Chain exception for better debugging context.When re-raising a different exception type, chain it with
fromto preserve the original traceback.Proposed fix
except ValueError as e: - raise ValidationError( + raise ValidationError( {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} - ) + ) from e
156-159: Improve translation string formatting.Using
.format()on the result of_()can cause issues with translation tools. The translatable string should contain the full template.Proposed fix
if address: parts = [part.strip() for part in address.split(",")[:2] if part.strip()] location = ", ".join(parts) - return _("{} ~Estimated Location: {}~".format(location, self.ip_address)) - return _("Estimated Location: {}".format(self.ip_address)) + return _( + "%(location)s ~Estimated Location: %(ip)s~" + ) % {"location": location, "ip": self.ip_address} + return _("Estimated Location: %(ip)s") % {"ip": self.ip_address}openwisp_controller/config/whois/tests/tests.py (3)
175-175: Use direct attribute access instead ofgetattr.When the attribute name is a constant string, direct access is clearer and slightly more efficient.
Proposed fix
- self.assertEqual(getattr(org_settings_obj, "whois_enabled"), True) + self.assertEqual(org_settings_obj.whois_enabled, True)
181-181: Use direct attribute access instead ofgetattr.Proposed fix
- self.assertEqual(getattr(org_settings_obj, "whois_enabled"), False) + self.assertEqual(org_settings_obj.whois_enabled, False)
191-194: Use direct attribute access instead ofgetattr.Proposed fix
self.assertEqual( - getattr(org_settings_obj, "whois_enabled"), + org_settings_obj.whois_enabled, app_settings.WHOIS_ENABLED, )
📜 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/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
🧰 Additional context used
🧬 Code graph analysis (6)
openwisp_controller/geo/estimated_location/tests/tests.py (3)
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-41)openwisp_controller/geo/estimated_location/tasks.py (1)
manage_estimated_locations(77-134)
openwisp_controller/config/whois/tests/tests.py (4)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (2)
_create_whois_info(15-35)setUp(37-41)openwisp_controller/config/base/whois.py (1)
device_whois_info_delete_handler(102-118)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(233-242)
openwisp_controller/config/migrations/0062_whoisinfo.py (2)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
Migration(11-79)openwisp_controller/config/migrations/0061_config_checksum_db.py (1)
Migration(35-56)
openwisp_controller/geo/base/models.py (3)
openwisp_controller/config/base/whois.py (1)
clean(66-99)openwisp_controller/config/base/device.py (2)
_set_initial_values_for_changed_checked_fields(129-134)clean(269-274)openwisp_controller/config/base/multitenancy.py (2)
clean(69-88)clean(142-145)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
openwisp_controller/config/migrations/0062_whoisinfo.py (1)
Migration(11-83)
openwisp_controller/config/base/whois.py (1)
openwisp_controller/config/whois/tasks.py (1)
delete_whois_record(85-94)
🪛 Ruff (0.14.10)
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/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/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/geo/base/models.py
18-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
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/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)
🔇 Additional comments (10)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
1-79: LGTM!The migration correctly mirrors the core migration structure, adding the WHOISInfo model and whois_enabled field. Field definitions align with the abstract model in
openwisp_controller/config/base/whois.py.openwisp_controller/geo/base/models.py (3)
28-38: LGTM!The initialization and field tracking logic correctly handles deferred fields, matching the pattern used in
openwisp_controller/config/base/device.py(lines 128-133). Usingget_deferred_fields()is the correct approach for Django's deferred field mechanism.
40-61: LGTM!The validation logic correctly ensures that
is_estimatedcan only be set toTruewhen the Estimated Location feature is properly configured for the organization. The check for_state.addingorestimated_status_changedprevents unnecessary validation errors during normal updates.
63-93: This edge case is theoretical with no practical impact in the current codebase.The concern about
_initial_is_estimatedbeing DEFERRED is valid in principle—if estimation is not configured and the field was deferred during load, the restoration at line 91-92 will skip and the field will keep its in-memory value. However:
- The field is never actually deferred in the codebase (no usage of
.defer('is_estimated')- This behavior is consistent with how other deferred fields are handled (see lines 79, 83)
- Skipping restoration when the value is unknown is defensible design
Consider adding a brief comment explaining this edge case if restoration-on-deferred-load is ever needed, but no changes are required at this time.
openwisp_controller/geo/estimated_location/tests/tests.py (1)
1-858: Comprehensive test coverage for estimated location feature.The tests thoroughly cover configuration settings, field validation, admin UI behavior, task triggering, location creation/update flows, notification handling, API exposure, and admin filters. Good use of subtests for related scenarios and proper mocking of external dependencies.
openwisp_controller/config/base/whois.py (2)
101-118: LGTM!The delete handler correctly checks that WHOIS is enabled for the organization and that no other active devices share the IP before scheduling deletion. Using
transaction.on_commitensures the deletion task only runs after the transaction commits successfully.
66-99: LGTM!Comprehensive validation covering private IP rejection, CIDR format validation with
strict=False(correctly allowing host addresses like192.168.1.12/24), and coordinate bounds checking.openwisp_controller/config/migrations/0062_whoisinfo.py (1)
1-83: LGTM!The migration correctly creates the WHOISInfo model with the expected fields and adds the
whois_enabledfield to OrganizationConfigSettings. The swappable model pattern is correctly configured.openwisp_controller/config/whois/tests/tests.py (2)
775-809: Excellent XSS protection test.This test properly validates that malicious HTML/JavaScript in WHOIS data is escaped and doesn't execute in the browser. The
UnexpectedAlertPresentExceptioncatch correctly detects if XSS payload executed.
1-809: Comprehensive WHOIS test coverage.The tests thoroughly cover signal handling, configuration validation, admin UI visibility, API responses, model field validation, task execution across organizations, WHOIS record lifecycle, threshold-based updates, failure notifications, and Selenium-based UI verification including XSS protection.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
QA checks for the sample app are failing:
ERROR: Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!
Migrations for 'sample_config': tests/openwisp2/sample_config/migrations/0010_whoisinfo_details.py + Add field details to whoisinfo
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:
- Around line 18-67: The test migration's WHOISInfo model contains an extra
"details" field that doesn't exist on the main/swappable WHOISInfo model
(CONFIG_WHOISINFO_MODEL); remove the ("details", ...) entry from the
migrations.CreateModel fields array so the test migration schema exactly matches
the base model schema and avoid swapping/schema mismatches.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
openwisp_controller/config/migrations/0062_whoisinfo.py (1)
Migration(11-83)
🪛 Ruff (0.14.10)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
13-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
17-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). (11)
- GitHub Check: Python==3.11 | django~=4.2.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.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.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
🔇 Additional comments (1)
tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py (1)
68-79: LGTM!The
whois_enabledfield definition correctly matches the main migration and uses the appropriateFallbackBooleanChoiceFieldwith proper defaults and help text.
| migrations.CreateModel( | ||
| name="WHOISInfo", | ||
| fields=[ | ||
| ( | ||
| "created", | ||
| model_utils.fields.AutoCreatedField( | ||
| default=django.utils.timezone.now, | ||
| editable=False, | ||
| verbose_name="created", | ||
| ), | ||
| ), | ||
| ( | ||
| "modified", | ||
| model_utils.fields.AutoLastModifiedField( | ||
| default=django.utils.timezone.now, | ||
| editable=False, | ||
| verbose_name="modified", | ||
| ), | ||
| ), | ||
| ( | ||
| "ip_address", | ||
| models.GenericIPAddressField( | ||
| db_index=True, primary_key=True, serialize=False | ||
| ), | ||
| ), | ||
| ( | ||
| "isp", | ||
| models.CharField( | ||
| blank=True, help_text="Organization for ASN", max_length=100 | ||
| ), | ||
| ), | ||
| ("asn", models.CharField(blank=True, help_text="ASN", max_length=10)), | ||
| ( | ||
| "timezone", | ||
| models.CharField(blank=True, help_text="Time zone", max_length=35), | ||
| ), | ||
| ( | ||
| "address", | ||
| jsonfield.fields.JSONField( | ||
| blank=True, default=dict, help_text="Address" | ||
| ), | ||
| ), | ||
| ("cidr", models.CharField(blank=True, help_text="CIDR", max_length=20)), | ||
| ("details", models.CharField(blank=True, max_length=64, null=True)), | ||
| ], | ||
| options={ | ||
| "abstract": False, | ||
| "swappable": "CONFIG_WHOISINFO_MODEL", | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Critical: Schema mismatch between test and main WHOISInfo models.
The test migration includes a details field (line 61) that is not present in the main migration (openwisp_controller/config/migrations/0062_whoisinfo.py). Since WHOISInfo is a swappable model, the test implementation must exactly mirror the base model's schema to prevent inconsistencies, test failures, and potential runtime errors.
🔧 Proposed fix
Remove the details field to match the base model:
("cidr", models.CharField(blank=True, help_text="CIDR", max_length=20)),
- ("details", models.CharField(blank=True, max_length=64, null=True)),
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| migrations.CreateModel( | |
| name="WHOISInfo", | |
| fields=[ | |
| ( | |
| "created", | |
| model_utils.fields.AutoCreatedField( | |
| default=django.utils.timezone.now, | |
| editable=False, | |
| verbose_name="created", | |
| ), | |
| ), | |
| ( | |
| "modified", | |
| model_utils.fields.AutoLastModifiedField( | |
| default=django.utils.timezone.now, | |
| editable=False, | |
| verbose_name="modified", | |
| ), | |
| ), | |
| ( | |
| "ip_address", | |
| models.GenericIPAddressField( | |
| db_index=True, primary_key=True, serialize=False | |
| ), | |
| ), | |
| ( | |
| "isp", | |
| models.CharField( | |
| blank=True, help_text="Organization for ASN", max_length=100 | |
| ), | |
| ), | |
| ("asn", models.CharField(blank=True, help_text="ASN", max_length=10)), | |
| ( | |
| "timezone", | |
| models.CharField(blank=True, help_text="Time zone", max_length=35), | |
| ), | |
| ( | |
| "address", | |
| jsonfield.fields.JSONField( | |
| blank=True, default=dict, help_text="Address" | |
| ), | |
| ), | |
| ("cidr", models.CharField(blank=True, help_text="CIDR", max_length=20)), | |
| ("details", models.CharField(blank=True, max_length=64, null=True)), | |
| ], | |
| options={ | |
| "abstract": False, | |
| "swappable": "CONFIG_WHOISINFO_MODEL", | |
| }, | |
| ), | |
| migrations.CreateModel( | |
| name="WHOISInfo", | |
| fields=[ | |
| ( | |
| "created", | |
| model_utils.fields.AutoCreatedField( | |
| default=django.utils.timezone.now, | |
| editable=False, | |
| verbose_name="created", | |
| ), | |
| ), | |
| ( | |
| "modified", | |
| model_utils.fields.AutoLastModifiedField( | |
| default=django.utils.timezone.now, | |
| editable=False, | |
| verbose_name="modified", | |
| ), | |
| ), | |
| ( | |
| "ip_address", | |
| models.GenericIPAddressField( | |
| db_index=True, primary_key=True, serialize=False | |
| ), | |
| ), | |
| ( | |
| "isp", | |
| models.CharField( | |
| blank=True, help_text="Organization for ASN", max_length=100 | |
| ), | |
| ), | |
| ("asn", models.CharField(blank=True, help_text="ASN", max_length=10)), | |
| ( | |
| "timezone", | |
| models.CharField(blank=True, help_text="Time zone", max_length=35), | |
| ), | |
| ( | |
| "address", | |
| jsonfield.fields.JSONField( | |
| blank=True, default=dict, help_text="Address" | |
| ), | |
| ), | |
| ("cidr", models.CharField(blank=True, help_text="CIDR", max_length=20)), | |
| ], | |
| options={ | |
| "abstract": False, | |
| "swappable": "CONFIG_WHOISINFO_MODEL", | |
| }, | |
| ), |
🤖 Prompt for AI Agents
In
@tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py
around lines 18 - 67, The test migration's WHOISInfo model contains an extra
"details" field that doesn't exist on the main/swappable WHOISInfo model
(CONFIG_WHOISINFO_MODEL); remove the ("details", ...) entry from the
migrations.CreateModel fields array so the test migration schema exactly matches
the base model schema and avoid swapping/schema mismatches.
There was a problem hiding this comment.
@coderabbitai this is a custom model used for testing, which has an additional "detail" field used in manual and automated tests to verify the model can be extended with custom fields without errors.
There was a problem hiding this comment.
@nemesifier, thank you for the clarification! You're absolutely right—I misunderstood the purpose of the additional details field. This is a valid testing pattern to ensure that swappable models can be extended with custom fields without breaking the application. I appreciate you taking the time to explain this.
✏️ Learnings added
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 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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Great work @DragnEmperor I've done one last line-by-line review, please see my comments below, with those resolved I think we can merge! 🚀 🚀 🚀
| migrations.CreateModel( | ||
| name="WHOISInfo", | ||
| fields=[ | ||
| ( | ||
| "created", | ||
| model_utils.fields.AutoCreatedField( | ||
| default=django.utils.timezone.now, | ||
| editable=False, | ||
| verbose_name="created", | ||
| ), | ||
| ), | ||
| ( | ||
| "modified", | ||
| model_utils.fields.AutoLastModifiedField( | ||
| default=django.utils.timezone.now, | ||
| editable=False, | ||
| verbose_name="modified", | ||
| ), | ||
| ), | ||
| ( | ||
| "ip_address", | ||
| models.GenericIPAddressField( | ||
| db_index=True, primary_key=True, serialize=False | ||
| ), | ||
| ), | ||
| ( | ||
| "isp", | ||
| models.CharField( | ||
| blank=True, help_text="Organization for ASN", max_length=100 | ||
| ), | ||
| ), | ||
| ("asn", models.CharField(blank=True, help_text="ASN", max_length=10)), | ||
| ( | ||
| "timezone", | ||
| models.CharField(blank=True, help_text="Time zone", max_length=35), | ||
| ), | ||
| ( | ||
| "address", | ||
| jsonfield.fields.JSONField( | ||
| blank=True, default=dict, help_text="Address" | ||
| ), | ||
| ), | ||
| ("cidr", models.CharField(blank=True, help_text="CIDR", max_length=20)), | ||
| ("details", models.CharField(blank=True, max_length=64, null=True)), | ||
| ], | ||
| options={ | ||
| "abstract": False, | ||
| "swappable": "CONFIG_WHOISINFO_MODEL", | ||
| }, | ||
| ), |
There was a problem hiding this comment.
@coderabbitai this is a custom model used for testing, which has an additional "detail" field used in manual and automated tests to verify the model can be extended with custom fields without errors.
| """ | ||
|
|
||
| def to_representation(self, obj): | ||
| data = super(EstimatedLocationMixin, self).to_representation(obj) |
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) |
| "verb": _("created"), | ||
| "level": "info", | ||
| "email_subject": _( | ||
| "Estimated Location: Created for device {notification.target}" |
There was a problem hiding this comment.
| "Estimated Location: Created for device {notification.target}" | |
| "Estimated location created for device {notification.target}" |
|
|
||
| class TestEstimatedLocationMixin(CreateWHOISMixin): | ||
| def setUp(self): | ||
| super(CreateWHOISMixin, self).setUp() |
There was a problem hiding this comment.
I think copilot is right in flagging this as well, please double check.
| 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): | ||
| address_changed = ( | ||
| self._initial_address is not models.DEFERRED | ||
| and self._initial_address != self.address | ||
| ) | ||
| geometry_changed = ( | ||
| self._initial_geometry is not models.DEFERRED | ||
| and self._initial_geometry != self.geometry | ||
| ) | ||
| if not _set_estimated and (address_changed or geometry_changed): | ||
| self.is_estimated = False | ||
| if self.name: | ||
| # remove estimated status between '~' | ||
| self.name = re.sub(r"~[^~]*~", "", self.name) | ||
| elif self._initial_is_estimated is not models.DEFERRED: | ||
| self.is_estimated = self._initial_is_estimated | ||
| return super().save(*args, **kwargs) |
| current_location.save( | ||
| update_fields=update_fields, _set_estimated=True | ||
| ) | ||
|
|
| notify_type="estimated_location_updated", | ||
| actor=current_location, | ||
| ) | ||
|
|
| X days (defined by "WHOIS_REFRESH_THRESHOLD_DAYS"). | ||
| - WHOIS is disabled in the organization settings. (query from db) | ||
| """ | ||
|
|
There was a problem hiding this comment.
Can this blank line be removed? I see the other methods don't have a blank line after the docstring.
openwisp_controller/config/management/commands/clear_last_ip.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/management/commands/clear_last_ip.py`:
- Around line 38-42: The .only() call on the Device queryset incorrectly
includes the relation name "organization__config_settings"; remove that
double-underscore entry and only list actual Device model fields (e.g.,
"last_ip" and "key"), keeping the
select_related("organization__config_settings") as-is so Organization and
OrganizationConfigSettings are still joined and loaded.
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 172-175: Remove the unnecessary "# noqa: E501" directives from the
long import strings used in the test decorators so RUF100 is not triggered;
specifically, edit the decorators that patch
"openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay"
and any other `@mock.patch` lines in tests.py (the ones currently ending with "#
noqa: E501") and delete the trailing "# noqa: E501" comments, leaving the patch
decorator strings intact.
♻️ Duplicate comments (2)
openwisp_controller/config/management/commands/clear_last_ip.py (2)
7-10: Help text does not reflect actual filtering behavior.The help text and interactive message state the command clears "every active device" but the implementation excludes devices with existing WHOISInfo records (lines 44-50). This discrepancy was previously flagged.
22-22: Unused*argsparameter.The unused
*argsparameter was previously flagged. Consider prefixing with underscore:def handle(self, *_args, **options):
🧹 Nitpick comments (3)
openwisp_controller/config/whois/service.py (1)
167-179: Improve exception re-raising to preserve traceback context.The exception handling catches GeoIP2 errors and re-raises with custom messages, but the traceback chain is lost. Using
raise ... from e(orfrom Noneif intentionally suppressing) provides better debugging context. Also,raise eon line 179 should be a bareraiseto preserve the original traceback.♻️ 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 + raiseopenwisp_controller/geo/base/models.py (1)
18-18: Consider annotating mutable class attribute withClassVar.The
_changed_checked_fieldslist is a mutable class attribute. While it's not modified at runtime, annotating it withtyping.ClassVarmakes the intent clearer and satisfies the static analysis hint.♻️ 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/whois/tests/tests.py (1)
176-194: Replacegetattrwith direct attribute access for constant attributes.The
getattrcalls with constant string attributes provide no safety benefit over direct attribute access and reduce readability.♻️ 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)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.py
🚧 Files skipped from review as they are similar to previous changes (4)
- openwisp_controller/geo/api/serializers.py
- openwisp_controller/config/whois/tests/utils.py
- openwisp_controller/geo/estimated_location/tests/utils.py
- openwisp_controller/geo/estimated_location/handlers.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-01-15T14:06:44.800Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:44.800Z
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/base/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/tests/tests.py
🧬 Code graph analysis (4)
openwisp_controller/config/management/commands/clear_last_ip.py (2)
openwisp_controller/config/base/device.py (1)
save(276-296)openwisp_controller/config/base/multitenancy.py (1)
save(90-101)
openwisp_controller/geo/base/models.py (2)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-348)check_estimate_location_configured(128-134)openwisp_controller/config/base/device.py (1)
_set_initial_values_for_changed_checked_fields(129-134)
openwisp_controller/geo/estimated_location/mixins.py (1)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-348)check_estimate_location_configured(128-134)
openwisp_controller/config/whois/tests/tests.py (4)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (6)
CreateWHOISMixin(14-41)WHOISTransactionMixin(44-161)_create_whois_info(15-35)setUp(37-41)_task_called(60-161)_mocked_client_response(46-58)openwisp_controller/config/base/whois.py (1)
device_whois_info_delete_handler(102-118)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(232-241)
🪛 Ruff (0.14.11)
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)
openwisp_controller/geo/base/models.py
18-18: 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/geo/estimated_location/tests/tests.py
174-174: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
293-293: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
296-296: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
299-299: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
openwisp_controller/config/whois/tests/tests.py
176-176: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
182-182: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
193-193: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (14)
openwisp_controller/geo/estimated_location/mixins.py (2)
4-16: LGTM!The mixin correctly uses
super().to_representation(obj)(Python 3 style) and conditionally includes/excludes theis_estimatedfield based on the organization's WHOIS configuration. The logic is clean and handles both configured and unconfigured states properly.
19-31: LGTM!The GeoJSON mixin correctly places
is_estimatedin thepropertiesdict following GeoJSON conventions. The conditional logic mirrors the standard mixin appropriately.openwisp_controller/config/whois/service.py (4)
94-125: LGTM!The caching pattern is well-implemented with proper fallback to a default instance when organization settings don't exist. Reusing
Config._CHECKSUM_CACHE_TIMEOUTensures consistency across the codebase.
243-266: LGTM!The use of
transaction.on_commitensures tasks are only enqueued after the database transaction commits successfully, preventing orphaned tasks. The conditional logic correctly handles both WHOIS lookup and estimated location management scenarios.
302-347: LGTM!The method handles the complex scenarios of creating shared locations vs. updating existing estimated locations correctly. The use of
transaction.atomic()for database operations while keeping notifications outside ensures data integrity without coupling transactional and notification concerns.
66-74: LGTM!Good defensive implementation using
ip_addr(ip).is_globalto validate public IP addresses, with proper exception handling for invalid IP formats.openwisp_controller/geo/base/models.py (2)
40-61: LGTM!The
clean()method correctly checks for theDEFERREDsentinel before comparing initial values, preventing false validation errors when the field was deferred during model loading. The validation logic properly ensuresis_estimatedcan only be set toTruewhen the feature is configured.
63-99: LGTM!The
save()method correctly implements:
- Clearing
is_estimatedand the name marker when manual edits occur (feature enabled)- Protecting
is_estimatedfrom manual changes when the feature is disabled (per the retrieved learning)- Properly handling
update_fieldsto ensure mutations aren't silently droppedThe regex
r"~[^~]*~"correctly matches the locale-independent estimated location marker.openwisp_controller/config/whois/tests/tests.py (2)
518-604: LGTM!Comprehensive test coverage for WHOIS creation and deletion scenarios including edge cases like shared IP addresses across multiple devices. The helper function
_verify_whois_detailsreduces code duplication and improves readability.
808-843: LGTM!Excellent XSS protection test that verifies malicious script tags and event handlers are properly escaped in the admin UI. Using
UnexpectedAlertPresentExceptionas the failure indicator is a smart approach for detecting actual XSS execution.openwisp_controller/geo/estimated_location/tests/tests.py (3)
303-375: LGTM!Comprehensive tests for estimated location creation and update flows with proper verification of location name, address, and geometry. The
assertNumQueriesassertions help ensure performance expectations are maintained.
665-701: LGTM!Good use of dictionary comprehension (
results_by_id) for order-independent assertions, addressing the brittleness concern from past reviews. The tests properly verifyis_estimatedfield visibility based on organization configuration.
803-871: LGTM!Comprehensive admin filter tests covering all location type filters and correctly verifying that specialized filters are unavailable when WHOIS is not configured. The test structure with subTests clearly documents each scenario being tested.
openwisp_controller/config/management/commands/clear_last_ip.py (1)
51-59: Good use of iterator and update_fields.The combination of
.iterator()for memory efficiency on large datasets andsave(update_fields=["last_ip"])to minimize database writes is well-implemented. The explanatory comment about why queryset-levelupdate()isn't used is valuable for maintainability.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch( | ||
| "openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay" # noqa: E501 | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if E501 is enabled in Ruff configuration
echo "=== Checking Ruff configuration for E501 ==="
fd -t f \( -name "*.toml" -o -name "*.cfg" -o -name "setup.cfg" \) | head -20Repository: openwisp/openwisp-controller
Length of output: 364
🏁 Script executed:
#!/bin/bash
# Check pyproject.toml for Ruff configuration
if [ -f pyproject.toml ]; then
echo "=== pyproject.toml [tool.ruff] section ==="
grep -A 50 "\[tool\.ruff\]" pyproject.toml
fiRepository: openwisp/openwisp-controller
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Check the specific file and lines mentioned
echo "=== Checking lines 172-175 in the test file ==="
cat -n openwisp_controller/geo/estimated_location/tests/tests.py | sed -n '172,180p'Repository: openwisp/openwisp-controller
Length of output: 563
🏁 Script executed:
#!/bin/bash
# Properly search for configuration files
echo "=== Looking for pyproject.toml ==="
find . -maxdepth 2 -name "pyproject.toml" -type f
echo ""
echo "=== Checking if pyproject.toml exists at root ==="
ls -la pyproject.toml 2>&1 | head -5Repository: openwisp/openwisp-controller
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Extract Ruff configuration section from pyproject.toml
if [ -f pyproject.toml ]; then
echo "=== Full [tool.ruff] section ==="
python3 <<'EOF'
import re
with open('pyproject.toml', 'r') as f:
content = f.read()
# Find [tool.ruff] section
match = re.search(r'\[tool\.ruff\](.*?)(?=\n\[|$)', content, re.DOTALL)
if match:
print(match.group(0)[:2000])
else:
print("No [tool.ruff] section found")
EOF
else
echo "pyproject.toml not found"
fiRepository: openwisp/openwisp-controller
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check the actual line length of line 174
echo "=== Checking line length of line 174 ==="
sed -n '174p' openwisp_controller/geo/estimated_location/tests/tests.py | wc -c
echo "Line 174 content:"
sed -n '174p' openwisp_controller/geo/estimated_location/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 240
🏁 Script executed:
#!/bin/bash
# Search for all Ruff configuration references
echo "=== Searching for E501 in configuration files ==="
rg "E501" --type toml --type cfgRepository: openwisp/openwisp-controller
Length of output: 166
🏁 Script executed:
#!/bin/bash
cat pyproject.tomlRepository: openwisp/openwisp-controller
Length of output: 580
🏁 Script executed:
#!/bin/bash
# Also check setup.cfg and other config files
echo "=== Looking for Ruff config in other files ==="
ls -la | grep -E "(setup.cfg|\.ruff|ruff.toml)"Repository: openwisp/openwisp-controller
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Search for E501 directly in pyproject.toml
echo "=== Searching for E501 ==="
grep -i "E501" pyproject.toml
echo ""
echo "=== Searching for line length related config ==="
grep -i "line.length\|max.line" pyproject.tomlRepository: openwisp/openwisp-controller
Length of output: 165
🏁 Script executed:
#!/bin/bash
cat ruff.tomlRepository: openwisp/openwisp-controller
Length of output: 978
🏁 Script executed:
#!/bin/bash
echo "=== All noqa: E501 directives in the test file ==="
grep -n "noqa: E501" openwisp_controller/geo/estimated_location/tests/tests.pyRepository: openwisp/openwisp-controller
Length of output: 546
Remove unused # noqa: E501 directives.
E501 (line too long) is explicitly disabled in ruff.toml (ignore = [..., "E501", ...]), making these noqa directives unnecessary. Remove them from lines 174, 293, 296, and 299 to avoid RUF100 (unused noqa) violations.
🧰 Tools
🪛 Ruff (0.14.11)
174-174: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In `@openwisp_controller/geo/estimated_location/tests/tests.py` around lines 172 -
175, Remove the unnecessary "# noqa: E501" directives from the long import
strings used in the test decorators so RUF100 is not triggered; specifically,
edit the decorators that patch
"openwisp_controller.geo.estimated_location.tasks.manage_estimated_locations.delay"
and any other `@mock.patch` lines in tests.py (the ones currently ending with "#
noqa: E501") and delete the trailing "# noqa: E501" comments, leaving the patch
decorator strings intact.
There was a problem hiding this comment.
@coderabbitai we are using flake8 for line length checks and not ruff
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai we are using flake8 for line length checks and not ruff
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for flake8 configuration files in the repository
echo "=== Looking for flake8 configuration ==="
fd -t f \( -name ".flake8" -o -name "setup.cfg" -o -name "tox.ini" \) | head -10
echo ""
echo "=== Checking setup.cfg for flake8 config ==="
if [ -f setup.cfg ]; then
grep -A 20 "\[flake8\]" setup.cfg || echo "No [flake8] section found in setup.cfg"
fi
echo ""
echo "=== Checking .flake8 file ==="
if [ -f .flake8 ]; then
cat .flake8
else
echo ".flake8 file not found"
fi
echo ""
echo "=== Checking tox.ini for flake8 config ==="
if [ -f tox.ini ]; then
grep -A 20 "\[flake8\]" tox.ini || echo "No [flake8] section found in tox.ini"
fi
Length of output: 622
@DragnEmperor, thank you for the clarification! You're absolutely right. I verified the configuration and can see that the project uses flake8 for line length enforcement with max-line-length = 88 configured in setup.cfg. The # noqa: E501 directives are valid and necessary for flake8.
I apologize for the incorrect suggestion to remove them. The project uses both ruff (which ignores E501 per ruff.toml) and flake8 (which enforces line length), so those noqa directives should remain.
✏️ Learnings added
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 openwisp/openwisp-controller, the project uses both ruff and flake8 for linting. Flake8 is configured in setup.cfg with max-line-length = 88 and enforces E501 (line too long), while ruff ignores E501 per ruff.toml. Therefore, `# noqa: E501` directives are valid and necessary for flake8, not for ruff.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:44.800Z
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.
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.
fbeb182 to
890daad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/whois/service.py`:
- Around line 317-347: The create path calls send_whois_task_notification inside
the transaction.atomic block, risking notifications for rolled-back work; move
the send_whois_task_notification call for the create branch to immediately after
the with transaction.atomic(): block (same args: device=self.device,
notify_type="estimated_location_created", actor=current_location) so that
current_location and device_location are fully saved/committed before notifying,
mirroring the update path behavior used for send_whois_task_notification with
notify_type="estimated_location_updated".
♻️ Duplicate comments (3)
openwisp_controller/geo/estimated_location/tests/utils.py (1)
8-16: LGTM - the super() pattern is documented.The explicit
super(CreateWHOISMixin, self).setUp()call to skip the parent'ssetUpis a valid approach here. As noted in the comment, this avoids the parent'sOrganizationConfigSettingscreation (which only setswhois_enabled=True) and instead creates it with both flags enabled for estimated location tests. The added comment adequately explains this intent.openwisp_controller/config/management/commands/clear_last_ip.py (2)
22-22: Unused*argsparameter.The
*argsparameter is not used in the method body. While Django'shandle()method signature conventionally includes it, prefixing with underscore signals intentional non-use.♻️ Suggested fix
- def handle(self, *args, **options): + def handle(self, *_args, **options):
7-10: Help text is misleading about the command's actual behavior.The help text states "Clears the last IP address (if set) for every active device across all organizations" but the command actually excludes devices that already have WHOISInfo records (lines 42-48). This discrepancy was noted in a previous review.
Consider updating the help text to accurately describe the filtering:
📝 Suggested fix
help = ( - "Clears the last IP address (if set) for every active device" - " across all organizations." + "Clears the last IP address (if set) for active devices without WHOIS records" + " across all organizations." )
🧹 Nitpick comments (8)
openwisp_controller/geo/estimated_location/tests/tests.py (2)
62-71: Consider simplifying the assertion pattern.The
try/except AssertionErrorwrapper aroundassertEqualis redundant—if the assertion fails, the test will fail anyway with a clear message. The same pattern appears at lines 119-125.♻️ Suggested simplification
- try: - self.assertEqual( - context_manager.exception.message_dict[ - "estimated_location_enabled" - ][0], - "Estimated Location feature requires " - + "WHOIS Lookup feature to be enabled.", - ) - except AssertionError: - self.fail("ValidationError message not equal to expected message.") + self.assertEqual( + context_manager.exception.message_dict["estimated_location_enabled"][0], + "Estimated Location feature requires " + "WHOIS Lookup feature to be enabled.", + )
862-871: Consider adding subtest context for the loop iterations.The loop iterates over filter values but the subtest description doesn't indicate which filter value is being tested when a failure occurs.
♻️ Suggested improvement for better failure messages
with mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", False): with self.subTest( "Test Estimated Location Admin specific filters not available" " when WHOIS not configured" ): for i in ["estimated", "outdoor", "indoor"]: - response = self.client.get(path, {"with_geo": i}) - self.assertContains(response, estimated_device.id) - self.assertContains(response, outdoor_device.id) - self.assertContains(response, indoor_device.id) + with self.subTest(filter_value=i): + response = self.client.get(path, {"with_geo": i}) + self.assertContains(response, estimated_device.id) + self.assertContains(response, outdoor_device.id) + self.assertContains(response, indoor_device.id)openwisp_controller/config/whois/service.py (1)
167-179: Improve exception re-raising for better traceability.The static analysis correctly identifies two issues:
- Line 177: Use
raise ... from eto preserve the exception chain- Line 179: Use bare
raiseinstead ofraise eto preserve the original traceback♻️ 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 + raiseopenwisp_controller/geo/base/models.py (2)
18-18: Consider annotating mutable class attribute withClassVar.The static analysis correctly identifies that
_changed_checked_fieldsis a mutable class attribute. While this doesn't cause runtime issues since the list isn't mutated, adding a type annotation improves code clarity.♻️ Suggested 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"]
63-99: Consider refreshing initial values after save to prevent stale change detection.The
save()method correctly handlesupdate_fieldsand DEFERRED checks. However, after saving, the_initial_*values remain stale. If the same model instance is saved again without re-fetching from the database, the change detection will compare against outdated initial values.A previous review comment suggested calling
_set_initial_values_for_changed_checked_fields()aftersuper().save(). While this may not be an issue in typical usage patterns (model instances are often discarded after save), it could cause subtle bugs in scenarios where the same instance is modified and saved multiple times.♻️ Proposed fix
if update_fields := kwargs.get("update_fields"): kwargs["update_fields"] = set(update_fields) | changed_fields - return super().save(*args, **kwargs) + result = super().save(*args, **kwargs) + self._set_initial_values_for_changed_checked_fields() + return resultopenwisp_controller/config/whois/tests/tests.py (2)
158-165: Redundant try/except around assertion.The try/except block catching
AssertionErrorto callself.fail()is unnecessary sinceassertEqualalready raisesAssertionErrorwith the failure message when it fails. Theself.fail()provides less diagnostic information than the original assertion error.This pattern appears multiple times (lines 158-165, 338-344, 351-359, 375-381).
♻️ Proposed simplification
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.", + )
172-195: Replacegetattrwith direct attribute access.As flagged by static analysis, using
getattr(obj, "attr")with a constant attribute name is unnecessary and less readable than direct attribute accessobj.attr.♻️ 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.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, )openwisp_controller/config/whois/tests/utils.py (1)
132-144: Consider clarifying the subTest description.The subTest name "task not called via DeviceChecksumView for no WHOIS record" is slightly misleading. The actual scenario being tested is that when the device's IP hasn't changed (same
REMOTE_ADDRasdevice.last_ip), no WHOIS task is triggered—this is true regardless of whether a WHOIS record exists.A clearer name might be: "task not called via DeviceChecksumView when IP unchanged".
♻️ Suggested clarification
with self.subTest( - f"{task_name} task not called via DeviceChecksumView for no WHOIS record" + f"{task_name} task not called via DeviceChecksumView when IP unchanged" ):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/geo/api/serializers.py
🧰 Additional context used
🧠 Learnings (3)
📓 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/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T14:06:44.800Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:44.800Z
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/mixins.pyopenwisp_controller/geo/base/models.py
🧬 Code graph analysis (7)
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/tests.py (4)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (4)
WHOISTransactionMixin(44-161)_create_whois_info(15-35)_task_called(60-161)_mocked_client_response(46-58)openwisp_controller/config/base/whois.py (2)
device_whois_info_delete_handler(102-118)invalidate_org_settings_cache(123-129)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(232-241)
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(276-296)openwisp_controller/geo/estimated_location/tests/utils.py (1)
setUp(9-16)
openwisp_controller/geo/estimated_location/tests/tests.py (6)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (3)
setUp(37-41)_mocked_client_response(46-58)_task_called(60-161)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(77-134)openwisp_controller/geo/estimated_location/tests/utils.py (2)
TestEstimatedLocationMixin(8-16)setUp(9-16)openwisp_controller/geo/base/models.py (2)
save(63-99)device(145-146)
openwisp_controller/geo/estimated_location/mixins.py (2)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-348)check_estimate_location_configured(128-134)openwisp_controller/geo/base/models.py (1)
organization_id(149-150)
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/management/commands/clear_last_ip.py (2)
openwisp_controller/config/base/device.py (1)
save(276-296)openwisp_controller/config/base/multitenancy.py (1)
save(90-101)
🪛 Ruff (0.14.11)
openwisp_controller/config/whois/tests/tests.py
176-176: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
182-182: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
193-193: 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/geo/estimated_location/tests/tests.py
174-174: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
293-293: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
296-296: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
299-299: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
openwisp_controller/geo/base/models.py
18-18: 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/management/commands/clear_last_ip.py
22-22: Unused method argument: args
(ARG002)
36-36: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (28)
openwisp_controller/geo/estimated_location/tests/utils.py (1)
1-5: LGTM!The imports and dynamic model loading follow OpenWISP's standard pattern for swappable models.
openwisp_controller/geo/estimated_location/tests/tests.py (9)
1-32: LGTM!Imports and module-level setup are well-organized. Using swapper's
load_modelfor model references is the correct pattern for swappable models.
109-147: LGTM!Test class properly validates the
is_estimatedfield behavior and admin UI visibility under different configuration states. Good use ofrefresh_from_db()to ensure state consistency.
150-171: LGTM!Test class setup properly initializes the test environment, including safe unregistration of notification types using
contextlib.suppress.
172-175: Acknowledged:# noqa: E501directives for flake8.Based on the past review discussion, these directives are retained for flake8 line-length enforcement (which is separate from the Ruff configuration).
177-289: LGTM!Comprehensive test coverage for the estimated location task invocation scenarios:
- Task called on public IP
- Task not called when WHOISInfo already exists (via checksum view)
- Task behavior with threshold-based updates
- Task triggered on address/coordinate changes
Good use of
mock.patchfor cache operations and proper reset of mocks between subtests.
303-518: LGTM!Excellent test coverage for estimated location creation and update flows:
- Location creation on new device
- Location updates when IP changes
- Non-estimated locations preserved
- Location sharing between devices with same IP
- Proper cleanup of old estimated locations
Good use of
assertNumQueriesto verify query performance expectations.
520-578: LGTM!Notification tests properly verify:
- Actor/target relationships
- Notification type and level
- Message content validation
- Error notification for conflicting locations
The
_verify_notificationhelper keeps the assertions DRY and readable.
579-624: LGTM!Tests correctly verify the
is_estimatedstatus removal behavior:
- Status unchanged when feature is disabled (as per the learning about
BaseLocation.save()protecting the field)- Status unchanged when non-geometry fields are modified
- Status correctly cleared when geometry/address fields are manually changed
Good use of
_set_initial_values_for_changed_checked_fields()to reset tracking state.
627-702: LGTM!API tests are well-structured with:
- Order-independent assertions using
results_by_iddict pattern- Proper validation of
is_estimatedfield presence/absence based on org settings- Coverage for locations list, device locations, and GeoJSON endpoints
openwisp_controller/geo/estimated_location/handlers.py (1)
1-39: LGTM!The notification type registration is well-structured with appropriate early exit when WHOIS is not configured. The dynamic model loading via
swapper.load_modelfollows OpenWISP conventions correctly.openwisp_controller/config/whois/service.py (2)
243-266: Well-designed task orchestration.The use of
transaction.on_commitensures tasks are only enqueued after the database transaction succeeds, preventing orphaned tasks. The conditional logic properly handles the case where WHOIS already exists but estimated location management is still needed.
281-300: LGTM!The create/update pattern properly tracks changed fields and validates new instances with
full_clean()before saving.openwisp_controller/geo/base/models.py (2)
28-38: LGTM!The initialization and change tracking logic properly handles deferred fields by checking
get_deferred_fields()and setting the initial value tomodels.DEFERREDwhen appropriate. This prevents incorrect change detection when fields are loaded via.only()or.defer().
40-61: LGTM!The validation logic correctly guards against the
DEFERREDsentinel before comparing values, preventing false validation errors when the field was deferred during model loading.openwisp_controller/geo/estimated_location/mixins.py (1)
1-31: LGTM!Both mixin classes are well-designed with clean conditional logic. The pattern of adding
is_estimatedwhen configured and removing it otherwise ensures the API response doesn't leak feature-flagged fields to clients when the feature is disabled.openwisp_controller/config/management/commands/clear_last_ip.py (1)
38-40: The.only()usage here does not cause an N+1 query problem.In the
clear_last_ip.pycontext,device.save(update_fields=["last_ip"])is called on existing devices that already have keys set. Device.save() only accessesorganization.config_settingson line 279 whenif not self.key:is true, which applies only to new devices being created. For existing devices (as in this command), that code path is skipped. Additionally,_check_changed_fields()only runs for non-new devices and checks fields likeorganization_id,name,group_id, andmanagement_ip—none of which change in this command. Since onlylast_ipis modified,organizationis never accessed and the.only("organization__config_settings")clause has no functional effect. The current query is acceptable for this use case.Likely an incorrect or invalid review comment.
openwisp_controller/config/whois/tests/tests.py (7)
1-31: LGTM! Imports and model setup are well-organized.The imports cover all necessary testing utilities, and the model loading via swapper follows the project's pattern for swappable models.
197-326: Well-structured API and management command tests.The tests comprehensively cover:
- Device List/Detail API with WHOIS info present/absent
- API behavior when WHOIS_CONFIGURED is True/False
- Management command functionality, query count optimization (Line 306), and cache invalidation
328-382: Good model validation coverage.The tests thoroughly verify validators for ISP length, private IP rejection, timezone length, CIDR format, ASN format, and coordinate bounds (longitude: -180 to 180, latitude: -90 to 90).
Note: The same redundant try/except pattern appears here and could be simplified as mentioned earlier.
453-463: LGTM! Previous issue addressed.The missing
device2.save()call from the past review has been correctly added at line 460.
515-636: Comprehensive WHOIS creation and deletion tests.The test covers multiple scenarios well:
- WHOIS creation on device creation
- WHOIS record deletion when IP changes and no other devices share the old IP
- WHOIS record preservation when other devices share the IP
- Proper handling of device deletion with active/deactivated devices sharing IPs
690-731: Good error handling and notification coverage.The test properly verifies logging and notification behavior for all relevant geoip2 exceptions:
OutOfQueriesErrorAddressNotFoundErrorAuthenticationErrorPermissionRequiredErrorThe
CELERY_TASK_EAGER_PROPAGATES=Falsesetting is correctly used to allow theon_failurehandler to execute.
733-843: Thorough Selenium tests with XSS protection verification.The tests cover:
- WHOIS details visibility in device admin under various configuration states
- Proper handling when WHOIS is disabled or no info exists
- XSS protection verification (lines 808-843) - excellent security testing practice
The
_assert_no_js_errorshelper appropriately filters non-relevant browser logs.openwisp_controller/config/whois/tests/utils.py (4)
1-11: LGTM! Clean imports and model loading.Properly imports test utilities and uses swapper for model loading.
14-41: Well-designed test factory mixin.The
_create_whois_infomethod:
- Provides sensible defaults with realistic values (Google's IP range, Mountain View location)
- Calls
full_clean()beforesave()ensuring validation is exercised- Allows flexible override via
**kwargsThe
setUpensuresOrganizationConfigSettingsexists withwhois_enabled=True, providing a consistent test baseline.
44-58: LGTM! Complete mock response structure.The
_mocked_client_responsestatic method properly mocks all attributes from the geoip2 city response including city, country, continent, postal, traits (ASO, ASN, network), and location (timezone, coordinates).
60-161: Comprehensive task verification helper.The
_task_calledmethod thoroughly tests WHOIS task invocation across multiple scenarios:
- Public IP on device creation
- IP change to another public IP
- No change to non-IP fields
- Private IP assignment
- WHOIS disabled at org level
- DeviceChecksumView triggering with WHOIS enabled/disabled
Good use of cache mocking to verify caching behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if not current_location or ( | ||
| attached_devices_exists and current_location.is_estimated | ||
| ): | ||
| with transaction.atomic(): | ||
| current_location = Location(**location_defaults, is_estimated=True) | ||
| current_location.full_clean() | ||
| current_location.save(_set_estimated=True) | ||
| device_location.location = current_location | ||
| device_location.full_clean() | ||
| device_location.save() | ||
| send_whois_task_notification( | ||
| device=self.device, | ||
| notify_type="estimated_location_created", | ||
| actor=current_location, | ||
| ) | ||
| elif current_location.is_estimated: | ||
| update_fields = [] | ||
| for attr, value in location_defaults.items(): | ||
| if getattr(current_location, attr) != value: | ||
| setattr(current_location, attr, value) | ||
| update_fields.append(attr) | ||
| if update_fields: | ||
| with transaction.atomic(): | ||
| current_location.save( | ||
| update_fields=update_fields, _set_estimated=True | ||
| ) | ||
| send_whois_task_notification( | ||
| device=self.device, | ||
| notify_type="estimated_location_updated", | ||
| actor=current_location, | ||
| ) |
There was a problem hiding this comment.
Inconsistent notification placement between create and update paths.
In the create path (lines 320-331), send_whois_task_notification is called inside the transaction.atomic() block. If any subsequent code in the atomic block fails, the notification would have already been triggered for a rolled-back transaction.
In the update path (lines 339-347), the notification is correctly sent outside the atomic block (line 343-347).
Consider moving the create notification outside the atomic block for consistency:
🐛 Proposed fix
if not current_location or (
attached_devices_exists and current_location.is_estimated
):
with transaction.atomic():
current_location = Location(**location_defaults, is_estimated=True)
current_location.full_clean()
current_location.save(_set_estimated=True)
device_location.location = current_location
device_location.full_clean()
device_location.save()
- send_whois_task_notification(
- device=self.device,
- notify_type="estimated_location_created",
- actor=current_location,
- )
+ send_whois_task_notification(
+ device=self.device,
+ notify_type="estimated_location_created",
+ actor=current_location,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not current_location or ( | |
| attached_devices_exists and current_location.is_estimated | |
| ): | |
| with transaction.atomic(): | |
| current_location = Location(**location_defaults, is_estimated=True) | |
| current_location.full_clean() | |
| current_location.save(_set_estimated=True) | |
| device_location.location = current_location | |
| device_location.full_clean() | |
| device_location.save() | |
| send_whois_task_notification( | |
| device=self.device, | |
| notify_type="estimated_location_created", | |
| actor=current_location, | |
| ) | |
| elif current_location.is_estimated: | |
| update_fields = [] | |
| for attr, value in location_defaults.items(): | |
| if getattr(current_location, attr) != value: | |
| setattr(current_location, attr, value) | |
| update_fields.append(attr) | |
| if update_fields: | |
| with transaction.atomic(): | |
| current_location.save( | |
| update_fields=update_fields, _set_estimated=True | |
| ) | |
| send_whois_task_notification( | |
| device=self.device, | |
| notify_type="estimated_location_updated", | |
| actor=current_location, | |
| ) | |
| if not current_location or ( | |
| attached_devices_exists and current_location.is_estimated | |
| ): | |
| with transaction.atomic(): | |
| current_location = Location(**location_defaults, is_estimated=True) | |
| current_location.full_clean() | |
| current_location.save(_set_estimated=True) | |
| device_location.location = current_location | |
| device_location.full_clean() | |
| device_location.save() | |
| send_whois_task_notification( | |
| device=self.device, | |
| notify_type="estimated_location_created", | |
| actor=current_location, | |
| ) | |
| elif current_location.is_estimated: | |
| update_fields = [] | |
| for attr, value in location_defaults.items(): | |
| if getattr(current_location, attr) != value: | |
| setattr(current_location, attr, value) | |
| update_fields.append(attr) | |
| if update_fields: | |
| with transaction.atomic(): | |
| current_location.save( | |
| update_fields=update_fields, _set_estimated=True | |
| ) | |
| send_whois_task_notification( | |
| device=self.device, | |
| notify_type="estimated_location_updated", | |
| actor=current_location, | |
| ) |
🤖 Prompt for AI Agents
In `@openwisp_controller/config/whois/service.py` around lines 317 - 347, The
create path calls send_whois_task_notification inside the transaction.atomic
block, risking notifications for rolled-back work; move the
send_whois_task_notification call for the create branch to immediately after the
with transaction.atomic(): block (same args: device=self.device,
notify_type="estimated_location_created", actor=current_location) so that
current_location and device_location are fully saved/committed before notifying,
mirroring the update path behavior used for send_whois_task_notification with
notify_type="estimated_location_updated".
There was a problem hiding this comment.
@DragnEmperor it's good to have this tool let us know these subtle issues
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
DragnEmperor
force-pushed
the
gsoc25-whois
branch
from
890daad to
f0c2069
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Unfortunately #1196 is hitting us hard now, I shared more info in the chat and in #1196 (comment), please let me know if you need more info.
| raise ValidationError( | ||
| {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} | ||
| ) | ||
|
|
| when device is deleted. | ||
| """ | ||
| Device = load_model("config", "Device") | ||
|
|
| class Command(BaseCommand): | ||
| help = ( | ||
| "Clears the last IP address (if set) for every active device" | ||
| " across all organizations." |
There was a problem hiding this comment.
| " across all organizations." | |
| " which doesn't yet have WHOIS data across all organizations." |
| try: | ||
| self.assertEqual( | ||
| context_manager.exception.message_dict[ | ||
| "estimated_location_enabled" | ||
| ][0], | ||
| "Estimated Location feature requires " | ||
| + "WHOIS Lookup feature to be enabled.", | ||
| ) | ||
| except AssertionError: | ||
| self.fail("ValidationError message not equal to expected message.") |
There was a problem hiding this comment.
What do you think of the AI suggestion here?
- try:
- self.assertEqual(
- context_manager.exception.message_dict[
- "estimated_location_enabled"
- ][0],
- "Estimated Location feature requires "
- + "WHOIS Lookup feature to be enabled.",
- )
- except AssertionError:
- self.fail("ValidationError message not equal to expected message.")
+ self.assertEqual(
+ context_manager.exception.message_dict["estimated_location_enabled"][0],
+ "Estimated Location feature requires "
+ "WHOIS Lookup feature to be enabled.",
+ )Seems simpler, if it does the job we can change this for all these occurrences and simplify the test code.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/management/commands/clear_last_ip.py`:
- Around line 38-42: The .only() call on the Device queryset currently lists the
relation path "organization__config_settings" which defers the FK needed by
select_related and is invalid; update the .only() arguments on the
Device.objects queryset in clear_last_ip.py to include the FK field name
"organization" (instead of "organization__config_settings") alongside "last_ip"
and "key" so the select_related("organization__config_settings") traversal is
not deferred.
♻️ Duplicate comments (1)
openwisp_controller/config/base/whois.py (1)
147-157: i18n issue: String interpolation inside translation function.Using
.format()inside_()causes string interpolation to occur before translation, which breaks proper internationalization. Translators will see the final string with values already substituted, not the template with placeholders. This should use named placeholders with%formatting after the translation call.🌍 Proposed fix for proper i18n support
`@property` def _location_name(self): """ Used to get location name based on the address and IP. """ address = self.formatted_address if address: parts = [part.strip() for part in address.split(",")[:2] if part.strip()] location = ", ".join(parts) - return _("{} ~Estimated Location: {}~".format(location, self.ip_address)) - return _("Estimated Location: {}".format(self.ip_address)) + return _("%(location)s ~Estimated Location: %(ip)s~") % { + "location": location, + "ip": self.ip_address, + } + return _("Estimated Location: %(ip)s") % {"ip": self.ip_address}
🧹 Nitpick comments (4)
openwisp_controller/config/whois/service.py (1)
71-74: Consider catching more specific exceptions.The
is_valid_public_ip_addressmethod catches genericValueError. While this works,ip_address()fromipaddressmodule raisesValueErrorfor invalid input, so this is appropriate. However, adding a brief comment about what triggers the exception could improve clarity.📝 Optional: Add clarifying comment
`@staticmethod` def is_valid_public_ip_address(ip): """ Check if given IP address is a valid public IP address. """ try: return ip and ip_addr(ip).is_global except ValueError: + # ip_address() raises ValueError for malformed IP strings return Falseopenwisp_controller/geo/base/models.py (2)
18-18: Consider addingClassVarannotation.Ruff flags this mutable class attribute. While it works correctly as-is, adding a
ClassVartype annotation makes the intent explicit and silences the linter.📝 Optional 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"]
87-92: Consider adding.strip()after regex substitution.When the
~...~marker is removed from the name, trailing whitespace may remain (e.g.,"Location ~marker~"becomes"Location "). Adding.strip()ensures clean output.🔧 Proposed fix
if not _set_estimated and (address_changed or geometry_changed): self.is_estimated = False if self.name: # remove estimated status between '~' - self.name = re.sub(r"~[^~]*~", "", self.name) + self.name = re.sub(r"~[^~]*~", "", self.name).strip() changed_fields = {"is_estimated", "name"}openwisp_controller/config/base/whois.py (1)
81-84: Add exception chaining for better traceability.When re-raising a
ValidationErrorfrom the caughtValueError, useraise ... from eto preserve the exception chain. This aids debugging by showing the original cause.♻️ Proposed fix
except ValueError as e: - raise ValidationError( + raise ValidationError( {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} - ) + ) from e
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
openwisp_controller/config/base/whois.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/geo/base/models.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/config/whois/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/base/models.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/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/base/models.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/base/models.py
🧬 Code graph analysis (6)
openwisp_controller/geo/estimated_location/tests/tests.py (6)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (4)
WHOISTransactionMixin(44-161)setUp(37-41)_mocked_client_response(46-58)_task_called(60-161)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(77-134)openwisp_controller/geo/estimated_location/tests/utils.py (2)
TestEstimatedLocationMixin(8-16)setUp(9-16)openwisp_controller/geo/base/models.py (3)
save(63-101)device(147-148)_set_initial_values_for_changed_checked_fields(32-38)
openwisp_controller/config/management/commands/clear_last_ip.py (2)
openwisp_controller/config/base/device.py (1)
save(276-296)openwisp_controller/config/base/multitenancy.py (1)
save(90-101)
openwisp_controller/config/whois/tests/tests.py (4)
openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (6)
CreateWHOISMixin(14-41)WHOISTransactionMixin(44-161)_create_whois_info(15-35)setUp(37-41)_task_called(60-161)_mocked_client_response(46-58)openwisp_controller/config/base/whois.py (2)
device_whois_info_delete_handler(101-116)invalidate_org_settings_cache(121-127)openwisp_controller/config/whois/service.py (2)
is_whois_enabled(137-144)get_device_whois_info(232-241)
openwisp_controller/config/base/whois.py (2)
openwisp_controller/config/whois/service.py (1)
get_cache_key(60-64)openwisp_controller/config/whois/tasks.py (1)
delete_whois_record(85-94)
openwisp_controller/config/whois/service.py (4)
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/models.py (3)
WHOISInfo(117-124)OrganizationConfigSettings(96-104)Config(36-43)openwisp_controller/config/base/config.py (1)
full_clean(558-562)
openwisp_controller/geo/base/models.py (3)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-347)check_estimate_location_configured(128-134)openwisp_controller/config/base/whois.py (1)
clean(66-98)openwisp_controller/config/base/device.py (3)
_set_initial_values_for_changed_checked_fields(129-134)clean(269-274)save(276-296)
🪛 Ruff (0.14.11)
openwisp_controller/geo/estimated_location/tests/tests.py
166-166: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
285-285: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
288-288: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
291-291: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
openwisp_controller/config/management/commands/clear_last_ip.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
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)
101-101: Unused static method argument: kwargs
(ARG004)
121-121: Unused static method argument: kwargs
(ARG004)
openwisp_controller/geo/base/models.py
18-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (26)
openwisp_controller/config/management/commands/clear_last_ip.py (2)
51-59: LGTM! Well-documented reasoning for per-instance saves.The comment clearly explains why a queryset-level update is avoided in favor of individual
save()calls—to preserve signal handling and cache invalidation. Usingupdate_fields=["last_ip"]is a good optimization to limit the database write scope.
7-9: Help text has been updated as suggested.The help text now accurately reflects that only devices without WHOIS records are affected. The wording aligns with the past review suggestion from nemesifier.
openwisp_controller/config/whois/tests/utils.py (3)
14-35: LGTM! Well-structured factory method with sensible defaults.The
_create_whois_infomethod provides good default values and correctly callsfull_clean()beforesave()to ensure validation is run. Thekwargsoverride pattern allows test flexibility.
44-58: LGTM! Clean mock response helper.The
_mocked_client_responseprovides a well-structured mock for the GeoIP2 client response with all necessary attributes populated.
60-161: LGTM! Comprehensive test scenarios with clear subTest organization.The
_task_calledhelper thoroughly tests WHOIS task invocation under various conditions (public/private IPs, cache states, WHOIS enabled/disabled, view-triggered flows). The use ofsubTestprovides good isolation and clear failure messages.openwisp_controller/config/whois/tests/tests.py (5)
39-54: LGTM! Proper signal cleanup for isolated tests.The
_disconnect_signalsmethod correctly disconnects all WHOIS-related handlers using the samedispatch_uidvalues used during connection, ensuring proper test isolation.
285-304: Good query count verification.The test verifies that the management command performs an expected number of queries (4 queries: 1 for fetching devices + 3 for individual saves). This helps prevent N+1 regression.
441-451: Past issue addressed:device2.save()is now called.The missing
device2.save()call has been added at line 448, ensuring the signal handler fires and the assertion is meaningful.
796-831: Excellent XSS protection test.The test injects XSS payloads into WHOIS data and verifies they're rendered as escaped text (visible in the DOM) rather than executed. The
UnexpectedAlertPresentExceptioncatch confirms no JavaScript alert was triggered.
686-719: Comprehensive exception handling test coverage.The test verifies logging behavior and notification generation for all relevant GeoIP2 exceptions (
OutOfQueriesError,AddressNotFoundError,AuthenticationError,PermissionRequiredError). The parametric approach viaassert_logging_on_exceptionkeeps the test DRY.openwisp_controller/config/whois/service.py (3)
319-330: LGTM! Notification now outside atomic block.The
send_whois_task_notificationcall for the create path has been moved outside thetransaction.atomic()block (after line 325), matching the update path pattern. This ensures notifications are only sent after the transaction commits successfully.
243-266: LGTM! Good use of transaction.on_commit for task scheduling.Using
transaction.on_commitensures tasks are only scheduled after the database transaction completes, preventing race conditions where tasks might execute before the device data is persisted.
16-17: No circular import risk exists. The import chain is unidirectional:service.py→tasks.py→utils.py→ external modules. Thetasks.pymodule does not import fromservice.py, andutils.pyhas no back-references to eitherservice.pyortasks.py. Module-level imports at lines 16-17 are safe.Likely an incorrect or invalid review comment.
openwisp_controller/geo/base/models.py (3)
28-38: LGTM! Proper deferred field handling in initialization.The
_set_initial_values_for_changed_checked_fieldsmethod correctly checksget_deferred_fields()and assignsmodels.DEFERREDfor deferred fields, preventing incorrect change detection later.
40-61: LGTM! DEFERRED sentinel check addressed.The
clean()method now correctly checksself._initial_is_estimated is not models.DEFERREDbefore comparing values, preventing false validation errors when the field is deferred.
93-101: LGTM! Proper protection ofis_estimatedwhen feature is disabled.Per the design discussion, the
elifbranch correctly reverts manual changes tois_estimatedwhen the estimated location feature is not configured. Theupdate_fieldshandling ensures these changes are persisted when using partial updates. Based on learnings, this is intentional behavior.openwisp_controller/geo/estimated_location/tests/tests.py (7)
1-32: LGTM! Well-organized imports and module setup.The imports are cleanly organized, and the model loading pattern using
swapper.load_modelis consistent with the project's swappable model architecture.
34-101: LGTM! Good configuration validation tests.The test properly validates the
ImproperlyConfiguredexception when estimated location is enabled without WHOIS, and correctly tests admin field visibility under different configurations. The simplified assertion pattern at lines 62-66 is clean.
104-139: LGTM! Thorough field validation and admin visibility tests.The tests correctly verify that the
is_estimatedfield raisesValidationErrorwhen the feature is not configured, and properly tests admin field visibility changes based on organization config settings.
164-167: The# noqa: E501directives are valid.Per project configuration, flake8 enforces E501 (line too long) via setup.cfg while ruff ignores E501 via ruff.toml. These directives are necessary for flake8 compliance. Based on learnings.
512-569: LGTM! Comprehensive notification testing.The
_verify_notificationhelper correctly validates notification attributes including actor, target, type, level, and message content. Good coverage of create, update, and error notification scenarios.
665-693: LGTM! Good fix for order-independent assertions.The
results_by_iddictionary pattern correctly addresses the previously flagged order-dependency issue, making these tests more robust and less flaky.
795-864: LGTM! Thorough admin filter testing.The admin filter tests cover all filter scenarios (estimated, outdoor, indoor, no-location) and properly verify that specialized filters are ignored when WHOIS is not configured. The subtest names correctly describe their test cases.
openwisp_controller/config/base/whois.py (3)
17-61: LGTM! Well-designed abstract model with appropriate field choices.The model fields are well-defined with appropriate max_lengths (ASN correctly uses max_length=10 for 32-bit ASNs), proper indexing on
ip_addressas primary key, and sensible use ofPointFieldwith SRID 4326 for coordinates. The docstring clearly explains the design rationale for using IP address as primary key.
100-127: LGTM! Signal handlers correctly accept**kwargsfor Django compatibility.The
**kwargsparameter in both static methods is required for Django signal handler compatibility, even though not explicitly used. Django'spost_saveandpost_deletesignals pass additional keyword arguments likecreated,using,raw, etc.
159-169: LGTM! Clean defaults for estimated location creation.The method provides appropriate default values for creating estimated locations, with outdoor type and non-mobile status being sensible defaults for IP-based geolocation data.
✏️ 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
5f9e2d2 to
1bdfcb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/base/whois.py`:
- Around line 147-157: The _location_name property currently formats dynamic
values before passing to gettext, preventing translators from seeing
placeholders; change both branches to pass a template string with named
placeholders to _() (e.g., "%(location)s ~Estimated Location: %(ip)s~" and
"Estimated Location: %(ip)s") and then interpolate with a mapping of
{"location": location, "ip": self.ip_address} (use formatted_address and
self.ip_address to build the mapping) so translators receive the template rather
than runtime data.
♻️ Duplicate comments (1)
openwisp_controller/config/management/commands/clear_last_ip.py (1)
38-42: The.only()still includes"organization__config_settings"which should be"organization".Based on the learnings from this PR, when using
select_related("organization__config_settings")with.only(), you need to include the FK field name"organization"to prevent the deferred/traversed conflict error. The current code uses"organization__config_settings"which is a relation path, not a valid field specification for.only().🐛 Proposed fix
devices = ( Device.objects.filter(_is_deactivated=False) .select_related("organization__config_settings") - .only("last_ip", "organization__config_settings", "key") + .only("last_ip", "organization", "key") )
🧹 Nitpick comments (6)
openwisp_controller/config/management/commands/clear_last_ip.py (1)
44-50: Clarify the command's behavior in the help text—it filters out devices with existing WHOIS records.The help text (lines 7-10) now correctly mentions "active devices without WHOIS records", but the filtering logic here (lines 44-50) would benefit from a brief inline comment explaining the rationale: devices with existing WHOISInfo are excluded to preserve their established data and avoid unnecessary re-lookups when IP is cleared.
📝 Suggested comment addition
+ # Exclude devices that already have WHOISInfo records to preserve + # established WHOIS data and avoid unnecessary re-lookups. # Filter out devices that have WHOIS information for their last IP devices = devices.exclude(last_ip=None).exclude(openwisp_controller/config/base/whois.py (1)
81-84: Chain the exception for better debugging context.When re-raising as
ValidationError, chain from the original exception to preserve the stack trace for debugging.🔧 Proposed fix
except ValueError as e: - raise ValidationError( + raise ValidationError( {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}} - ) + ) from eopenwisp_controller/config/whois/service.py (1)
281-299: Consider validating whois_instance before update to avoid partial saves.In
_create_or_update_whois, when updating an existing instance, the code directly saves without callingfull_clean(). While the create path (line 297) callsfull_clean(), the update path skips validation. Ifwhois_detailscontains invalid data, it could corrupt the existing record.🔧 Proposed fix to add validation on update
def _create_or_update_whois(self, whois_details, whois_instance=None): """ Used to update an existing WHOIS instance; else, creates a new one. Returns the updated or created WHOIS instance along with update fields. """ WHOISInfo = load_model("config", "WHOISInfo") update_fields = [] if whois_instance: for attr, value in whois_details.items(): if getattr(whois_instance, attr) != value: update_fields.append(attr) setattr(whois_instance, attr, value) if update_fields: + whois_instance.full_clean() whois_instance.save(update_fields=update_fields) else: whois_instance = WHOISInfo(**whois_details) whois_instance.full_clean() whois_instance.save(force_insert=True) return whois_instance, update_fieldsopenwisp_controller/geo/base/models.py (1)
18-18: Consider annotating mutable class attribute withClassVar.Static analysis suggests annotating
_changed_checked_fieldswithtyping.ClassVarto clarify it's a class-level attribute not meant to be modified per-instance.🔧 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/geo/estimated_location/tests/tests.py (2)
62-66: Simplify the assertion by removing unnecessary try/except.As the reviewer suggested in past comments, this pattern can be simplified. The
assertEqualalready raisesAssertionErrorwith a clear diff when values don't match, making the try/except wrapper redundant.♻️ Proposed simplification
- self.assertEqual( - context_manager.exception.message_dict["estimated_location_enabled"][0], - "Estimated Location feature requires " - + "WHOIS Lookup feature to be enabled.", - ) + self.assertEqual( + context_manager.exception.message_dict["estimated_location_enabled"][0], + "Estimated Location feature requires " + "WHOIS Lookup feature to be enabled.", + )Note: Also removed the unnecessary
+for string concatenation since adjacent string literals are automatically concatenated in Python.
630-637: Consider simplifying dict initialization.The
dict()creation followed byupdate()can be simplified.♻️ Minor simplification
def _create_device_location(self, **kwargs): - options = dict() - options.update(kwargs) + options = dict(**kwargs) device_location = self.object_location_model(**options) device_location.full_clean() device_location.save() device_location.refresh_from_db() return device_locationOr even simpler, pass
kwargsdirectly:def _create_device_location(self, **kwargs): device_location = self.object_location_model(**kwargs) device_location.full_clean() device_location.save() device_location.refresh_from_db() return device_location
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
openwisp_controller/config/base/whois.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/config/whois/tests/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/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/management/commands/clear_last_ip.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/base/models.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/config/management/commands/clear_last_ip.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/base/models.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/base/models.py
🧬 Code graph analysis (4)
openwisp_controller/config/management/commands/clear_last_ip.py (1)
openwisp_controller/config/base/device.py (1)
save(276-296)
openwisp_controller/config/base/whois.py (2)
openwisp_controller/config/whois/service.py (2)
WHOISService(37-347)get_cache_key(60-64)openwisp_controller/config/whois/tasks.py (1)
delete_whois_record(85-94)
openwisp_controller/config/whois/service.py (3)
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/models.py (3)
WHOISInfo(117-124)OrganizationConfigSettings(96-104)Config(36-43)
openwisp_controller/geo/base/models.py (3)
openwisp_controller/config/base/whois.py (1)
clean(66-98)openwisp_controller/config/base/device.py (2)
_set_initial_values_for_changed_checked_fields(129-134)clean(269-274)openwisp_controller/config/base/multitenancy.py (2)
clean(69-88)clean(142-145)
🪛 Ruff (0.14.11)
openwisp_controller/config/management/commands/clear_last_ip.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_controller/geo/estimated_location/tests/tests.py
166-166: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
285-285: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
288-288: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
291-291: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
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)
101-101: Unused static method argument: kwargs
(ARG004)
121-121: Unused static method argument: kwargs
(ARG004)
openwisp_controller/geo/base/models.py
18-18: 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). (11)
- GitHub Check: Python==3.11 | 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.1.0
- GitHub Check: Python==3.13 | 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.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (19)
openwisp_controller/config/management/commands/clear_last_ip.py (1)
51-59: LGTM—individual saves preserve signal handling and cache invalidation.The comment explains the rationale well: using individual
save()calls instead ofqueryset.update()ensures signals fire and caches are properly invalidated. Theupdate_fields=["last_ip"]optimization is appropriate.openwisp_controller/config/base/whois.py (2)
100-116: LGTM—delete handler correctly checks for remaining devices before scheduling deletion.The logic properly:
- Loads Device model dynamically
- Checks if other active devices share the same IP
- Only schedules deletion via
transaction.on_commitwhen safeThe
**kwargsparameter is required by Django's signal handler signature, so the static analysis hint (ARG004) is a false positive.
159-170: LGTM—defaults helper provides clean structure for estimated location creation.The method returns a well-structured dict with sensible defaults for the estimated location feature.
openwisp_controller/config/whois/service.py (3)
189-191: Verify Point coordinate order is (longitude, latitude).The
Pointconstructor uses(x, y)which corresponds to(longitude, latitude). The code correctly passesdata.location.longitudefirst anddata.location.latitudesecond. This matches the GeoIP2 response structure and PostGIS conventions.
316-330: LGTM—notification is now correctly sent outside the atomic block.The previous review flagged that notifications were sent inside
transaction.atomic()for the create path. This has been fixed—send_whois_task_notificationis now called at lines 326-330, which is outside thewith transaction.atomic():block (lines 319-325).
16-17: No circular import risk here - code is safe.The
WHOISServiceimport inutils.pyis inside theget_whois_info()function (line 60), not at module level. Module-level imports inservice.py(from .utils import send_whois_task_notificationandfrom .tasks) are safe because neitherutils.pynortasks.pyimportsservice.pyat module initialization. The deferred import inside the function ensuresservice.pyis fully loaded beforeget_whois_info()is called.openwisp_controller/geo/base/models.py (2)
40-61: LGTM—clean() properly handles deferred fields and validates estimated location configuration.The validation logic correctly:
- Checks if
_initial_is_estimatedis notDEFERREDbefore comparing (line 44)- Only raises validation error when
is_estimatedis being set toTrueon a new instance or changed value- Delegates to
WHOISService.check_estimate_location_configuredfor configuration validation
63-101: LGTM—save() correctly handles estimated location clearing, protection, and update_fields.The implementation:
- Properly guards against
DEFERREDvalues when checking for changes (lines 79-85)- Clears
is_estimatedand strips the~...~marker when address/geometry changes manually (lines 87-92)- Protects
is_estimatedfrom manual changes when feature is disabled (lines 94-96) — this is intentional per the learning- Correctly merges
changed_fieldsintoupdate_fieldswhen present (lines 97-98)- Refreshes initial values after save (line 100)
openwisp_controller/config/whois/tests/tests.py (6)
34-55: LGTM—signal disconnect helper is well-structured.The
_disconnect_signalsmethod properly disconnects all WHOIS-related signal handlers to allow testing in isolation. Usingdispatch_uidensures the correct handlers are targeted.
285-305: LGTM—query count test validates efficient database access.The test correctly expects 4 queries: 1 for fetching devices + 3 for individual saves (devices with
last_ip). The 4th device withoutlast_ipis filtered out and not saved.
356-369: LGTM—coordinate validation tests cover boundary conditions.The test cases properly validate:
- Latitude bounds (±90 degrees)
- Longitude bounds (±180 degrees)
- Both positive and negative out-of-range values
441-451: LGTM—device2.save() is now correctly called.The previous review flagged that
device2.save()was missing. This has been fixed—lines 447-449 now calldevice2.save()anddevice2.refresh_from_db()before assertingmocked_task.assert_not_called().
796-831: Excellent XSS protection test coverage.This test verifies that malicious HTML/script content in WHOIS data is properly escaped when rendered in the admin UI:
- Creates WHOISInfo with XSS payloads in
isp,timezone, andaddress.country- Catches
UnexpectedAlertPresentExceptionto fail if XSS executes- Verifies the raw script/onerror text appears in the rendered output (meaning it was escaped, not executed)
678-719: LGTM—comprehensive error handling and notification tests.The
test_whois_task_failure_notificationmethod:
- Uses
CELERY_TASK_EAGER_PROPAGATES=Falseto allowon_failureto execute- Tests all geoip2 exception types
- Verifies logging call counts and notification creation
- Checks notification attributes (actor, target, level, type, message)
openwisp_controller/geo/estimated_location/tests/tests.py (5)
104-139: LGTM!Test class properly validates the
is_estimatedfield behavior and admin visibility based on organization configuration settings.
142-167: LGTM!The transaction test class is well-structured with appropriate setup, mock paths, and notification type registration. The
# noqa: E501directives are correctly retained for flake8 compliance. Based on learnings, these directives are necessary since flake8 enforces E501 while ruff ignores it.
300-324: LGTM!The
_verify_location_detailshelper function is well-structured and validates all expected location properties including name, address, and geometry against the mocked response.
657-694: LGTM!API tests correctly use dict-based lookups (
results_by_id) to avoid order-dependent assertions, and properly validate the presence/absence ofis_estimatedbased on organization configuration.
795-864: LGTM!The admin filter tests provide comprehensive coverage for all filter options (estimated, outdoor, indoor, no location) and correctly verify that filter-specific behavior is disabled when WHOIS is not configured.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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/