[chores:bug] Separated openwisp_controller.geo from openwisp_controller.config #1224#1315
[chores:bug] Separated openwisp_controller.geo from openwisp_controller.config #1224#1315
Conversation
📝 WalkthroughWalkthroughThis PR separates estimated-location responsibilities from the config app into the geo app. It adds OrganizationGeoSettings (model, migrations, admin inline, serializer, API view and URL), introduces EstimatedLocationService (caching, task enqueueing, persistence and cache invalidation), moves estimated-location creation/update logic and task orchestration into geo (handlers, tasks, service, utils), emits two new WHOIS signals ( Sequence Diagram(s)sequenceDiagram
participant WHOIS_Task as WHOIS Task
participant EstService as EstimatedLocationService
participant Cache as Cache Layer
participant DB as Database
participant Location as Location Model
WHOIS_Task->>EstService: trigger_estimated_location_task(device_pk, ip)
EstService->>Cache: get(cache_key(org_id))
alt Cache hit
Cache-->>EstService: org_geo_settings
else Cache miss
EstService->>DB: OrganizationGeoSettings.objects.get(organization_id=org_id)
DB-->>EstService: org_geo_settings or DoesNotExist
EstService->>Cache: set(cache_key, org_geo_settings|sentinel, timeout)
end
EstService->>EstService: evaluate estimated_location_enabled
alt Enabled
EstService->>Location: _create_or_update_estimated_location(location_defaults)
Location->>DB: save (transaction)
DB-->>Location: saved instance
EstService-->>WHOIS_Task: send notification (created/updated)
else Disabled
EstService-->>WHOIS_Task: skip / no-op
end
sequenceDiagram
participant WHOIS_Service as WHOIS processing
participant Signals as whois_fetched / whois_lookup_skipped
participant Handlers as geo estimated_location handlers
participant EstService as EstimatedLocationService
participant Celery as Celery (task queue)
WHOIS_Service->>Signals: emit whois_fetched(whois, updated_fields, device)
Signals-->>Handlers: whois_fetched_handler(...)
Handlers->>EstService: check_estimated_location_enabled(org_id)
alt Enabled
Handlers->>EstService: EstimatedLocationService(device).trigger_estimated_location_task(ip)
EstService->>Celery: send_task("whois_estimated_location_task", args)
else Disabled
Handlers-->>WHOIS_Service: do nothing
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/rest-api.rst`:
- Around line 814-823: The docs list only PUT for the OrganizationGeoSettings
endpoint but the view OrganizationGeoSettingsView inherits RetrieveUpdateAPIView
(which allows PATCH), so either update the docs to include PATCH for
/api/v1/controller/organization/{organization_pk}/geo-settings/ OR restrict the
view to remove PATCH by adding http_method_names = ['get', 'put', 'head',
'options'] on OrganizationGeoSettingsView; locate the view (extends
RetrieveUpdateAPIView) to apply the change and ensure the docs and view methods
remain consistent.
In
`@openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py`:
- Around line 6-13: Migration 0064 must depend on the geo data migration and the
geo migration 0006 must copy the field before config removes it: add ("geo",
"0006_create_geo_settings_for_existing_orgs") to the dependencies list in
Migration class in
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py,
and modify geo migration
geo/migrations/0006_create_geo_settings_for_existing_orgs.py to include a
RunPython operation that defines a forward function (e.g.,
copy_estimated_location_enabled) which uses apps.get_model to load
OrganizationConfigSettings and OrganizationGeoSettings, iterates organizations
(or queries existing config records), and copies the estimated_location_enabled
boolean into the newly created OrganizationGeoSettings records (creating or
updating as needed), plus a safe reverse/noop function; use only migration-safe
ORM access (no direct model imports) and include this RunPython in the migration
operations so the config removal does not lose data.
In `@openwisp_controller/geo/admin.py`:
- Around line 212-222: The readonly-fields logic in
GeoSettingsInline.get_readonly_fields is inverted: instead of checking whether
"estimated_location_enabled" is already present, check whether WHOIS is not
configured and then ensure "estimated_location_enabled" is included; update
GeoSettingsInline.get_readonly_fields to test
config_app_settings.WHOIS_CONFIGURED (negated) and, if True, prepend or insert
"estimated_location_enabled" into the fields list (avoiding duplicates) for
OrganizationGeoSettings.
In `@openwisp_controller/geo/estimated_location/handlers.py`:
- Around line 39-56: The whois_info_post_save_handler uses
Device.objects.get(last_ip=instance.ip_address) which can raise
MultipleObjectsReturned; replace that lookup with
Device.objects.filter(last_ip=instance.ip_address).first() and treat a None
result the same way as the previous DoesNotExist case (return early). Update
references in the handler (Device, device, and the subsequent
EstimatedLocationService initialization/usage) so they only run when device is
not None.
In `@openwisp_controller/geo/estimated_location/service.py`:
- Around line 78-97: The update branch for estimated locations sets attrs on
current_location and calls current_location.save(update_fields=update_fields,
_set_estimated=True) without validation; mirror the create path by running
current_location.full_clean() (after setting attributes and before entering
transaction.atomic or before save) to validate the instance and raise/handle
ValidationError as appropriate, so replace the direct save with a validated save
in the block that builds update_fields for current_location and still call
send_whois_task_notification afterwards.
- Around line 31-35: Remove the redundant WHOIS_CONFIGURED check from the
is_estimated_location_enabled property: it should simply return the result of
check_estimated_location_enabled(self.device.organization_id) because
check_estimated_location_enabled already validates
config_app_settings.WHOIS_CONFIGURED; update the is_estimated_location_enabled
property to call and return check_estimated_location_enabled(...) directly and
remove the outer if-block.
In `@openwisp_controller/geo/estimated_location/tests/utils.py`:
- Around line 21-24: The test helper assumes org.geo_settings exists and will
raise RelatedObjectDoesNotExist for pre-existing organizations; update the logic
in _get_org() / the test setup to defensively fetch-or-create the related
OrganizationGeoSettings (use the OrganizationGeoSettings.get_or_create pattern
for the organization) before setting org.geo_settings.estimated_location_enabled
= True and calling save(), so the related object is guaranteed to exist when
toggling estimated_location_enabled.
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 8-17: The migration function create_geo_settings_for_existing_orgs
currently iterates over Organization.objects.all(), which can load all
Organization rows into memory; change the iteration to use
Organization.objects.all().iterator() (or Organization.objects.iterator()) when
looping so each Organization is fetched one at a time, keeping the call to
OrganizationGeoSettings.objects.get_or_create(organization_id=org.pk) the same
and preserving behavior for Organization and OrganizationGeoSettings.
In `@openwisp_controller/geo/settings.py`:
- Around line 1-8: Remove the unused logger definition: delete the top-level
"logger = logging.getLogger(__name__)" and the "import logging" statement since
neither is used in this module; if you prefer to log configuration validation,
instead call logging.getLogger(...) locally where needed (e.g., inside functions
that use get_setting or config_settings) rather than keeping an unused
module-level logger.
In `@openwisp_controller/geo/tests/test_api.py`:
- Around line 1647-1659: The subtest "Cannot update without change permission"
incorrectly uses fake_uuid (a non-existent organization) and the logged-in
admin_user already has change permission, so it will return 404 not 403; either
remove this subtest entirely or relocate and rewrite it to target a real
organization's URL (use the existing url variable or the
test_organization_geo_settings_update test) and ensure the actor lacks change
permission (revoke change permission from admin_user or authenticate a user
without change rights) before calling client.put/client.patch so the response is
403 as intended.
In `@tests/openwisp2/sample_geo/models.py`:
- Around line 34-36: OrganizationGeoSettings currently inherits only from
AbstractOrganizationGeoSettings while other test swappable models include a
DetailsModel mixin to verify extensibility; modify the class declaration to mix
in DetailsModel (e.g., class OrganizationGeoSettings(DetailsModel,
AbstractOrganizationGeoSettings):) keep the nested Meta class as class
Meta(AbstractOrganizationGeoSettings.Meta): abstract = False, and ensure
DetailsModel is imported into this module so migrations/tests can detect the
extra custom field.
- Line 1: Remove the unused import of the swapper module at the top of the test
concrete model file: delete the line importing "swapper" since this concrete
test model does not use swapper.swappable_setting or any swapper symbols; verify
there are no remaining references to "swapper" in functions/classes in this file
(e.g., model class definitions) and run tests/lint to ensure no import is
required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed2367de-a081-4e33-855f-551ab7e0629f
📒 Files selected for processing (32)
docs/developer/extending.rstdocs/user/rest-api.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/utils.pyopenwisp_controller/settings.pytests/openwisp2/sample_geo/models.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (4)
- openwisp_controller/config/tests/test_api.py
- openwisp_controller/config/base/multitenancy.py
- openwisp_controller/config/whois/tasks.py
- openwisp_controller/config/base/whois.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.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.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pytests/openwisp2/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pytests/openwisp2/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pytests/openwisp2/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pytests/openwisp2/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
🧠 Learnings (6)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/settings.pyopenwisp_controller/geo/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/settings.pyopenwisp_controller/geo/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pytests/openwisp2/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
docs/developer/extending.rsttests/openwisp2/settings.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/base/models.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/service.py
🧬 Code graph analysis (20)
openwisp_controller/geo/estimated_location/tests/utils.py (4)
openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(34-36)openwisp_controller/config/whois/tests/utils.py (2)
CreateWHOISMixin(14-40)setUp(36-40)openwisp_controller/geo/base/models.py (1)
save(73-110)
openwisp_controller/geo/estimated_location/mixins.py (2)
openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(12-98)check_estimated_location_enabled(17-29)openwisp_controller/geo/base/models.py (1)
organization_id(160-161)
openwisp_controller/geo/api/serializers.py (4)
openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(34-36)openwisp_controller/geo/admin.py (1)
Meta(35-37)openwisp_controller/geo/api/views.py (3)
Meta(53-55)Meta(70-71)Meta(100-102)
openwisp_controller/geo/api/views.py (4)
openwisp_controller/geo/api/serializers.py (1)
OrganizationGeoSettingsSerializer(26-30)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/mixins.py (1)
ProtectedAPIMixin(34-35)openwisp_controller/geo/base/models.py (1)
organization_id(160-161)
openwisp_controller/geo/estimated_location/tasks.py (2)
openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(12-98)_create_or_update_estimated_location(44-98)openwisp_controller/geo/estimated_location/utils.py (1)
get_location_defaults_from_whois(52-68)
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (2)
openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
Migration(12-62)openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (1)
Migration(20-36)
openwisp_controller/config/settings.py (1)
openwisp_controller/settings.py (1)
get_setting(4-5)
tests/openwisp2/sample_geo/models.py (2)
openwisp_controller/geo/base/models.py (1)
AbstractOrganizationGeoSettings(164-204)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)
openwisp_controller/geo/models.py (1)
openwisp_controller/geo/base/models.py (4)
AbstractOrganizationGeoSettings(164-204)BaseDeviceLocation(127-161)BaseFloorPlan(113-124)BaseLocation(22-110)
openwisp_controller/geo/settings.py (1)
openwisp_controller/settings.py (1)
get_setting(4-5)
openwisp_controller/geo/apps.py (2)
openwisp_controller/geo/estimated_location/handlers.py (2)
register_estimated_location_notification_types(11-36)whois_info_post_save_handler(39-56)openwisp_controller/geo/base/models.py (1)
organization_post_save_receiver(198-204)
openwisp_controller/geo/estimated_location/handlers.py (2)
openwisp_controller/geo/estimated_location/service.py (3)
EstimatedLocationService(12-98)check_estimated_location_enabled(17-29)trigger_estimated_location_task(37-42)openwisp_controller/geo/base/models.py (2)
device(156-157)organization_id(160-161)
openwisp_controller/geo/estimated_location/utils.py (1)
openwisp_controller/config/base/whois.py (2)
_location_name(138-152)formatted_address(120-135)
openwisp_controller/geo/migrations/__init__.py (3)
openwisp_controller/config/migrations/__init__.py (1)
assign_permissions_to_groups(26-60)openwisp_controller/connection/migrations/__init__.py (1)
assign_permissions_to_groups(6-35)openwisp_controller/pki/migrations/__init__.py (1)
assign_permissions_to_groups(6-31)
openwisp_controller/geo/migrations/0005_organizationgeosettings.py (2)
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(6-20)openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (1)
Migration(20-36)
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (3)
openwisp_controller/geo/migrations/__init__.py (1)
assign_geo_settings_permissions_to_groups(36-39)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/geo/base/models.py (1)
organization_id(160-161)
openwisp_controller/geo/admin.py (2)
openwisp_controller/config/admin.py (1)
DeactivatedDeviceReadOnlyMixin(83-104)openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(12-98)check_estimated_location_enabled(17-29)
openwisp_controller/geo/estimated_location/tests/tests.py (3)
openwisp_controller/geo/models.py (2)
OrganizationGeoSettings(29-32)Location(11-14)openwisp_controller/geo/tests/utils.py (1)
TestGeoMixin(9-68)openwisp_controller/geo/base/models.py (2)
save(73-110)device(156-157)
openwisp_controller/geo/base/models.py (1)
openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(12-98)check_estimated_location_enabled(17-29)
openwisp_controller/geo/estimated_location/service.py (3)
openwisp_controller/geo/base/models.py (3)
device(156-157)organization_id(160-161)save(73-110)openwisp_controller/geo/models.py (3)
OrganizationGeoSettings(29-32)Location(11-14)DeviceLocation(23-26)openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)
🔇 Additional comments (53)
openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
19-61: LGTM!The migration correctly creates the
OrganizationGeoSettingsmodel with appropriate field definitions:
- UUID primary key with auto-generation
- Nullable
FallbackBooleanChoiceFieldforestimated_location_enabledwith sensible defaults- OneToOne relationship to organization with proper cascade delete
- Swappable configuration for extensibility
openwisp_controller/geo/utils.py (1)
49-53: LGTM!The new geo-settings endpoint follows the established URL patterns in this file and aligns with the REST API design for organization-scoped resources.
tests/openwisp2/settings.py (1)
283-283: LGTM!The swapper configuration for
OrganizationGeoSettingsis correctly placed with other GEO model settings and follows the established naming convention.docs/developer/extending.rst (2)
353-353: LGTM!The swapper configuration documentation is correctly updated to include the new
GEO_ORGANIZATIONGEOSETTINGS_MODELsetting.
460-464: LGTM!The monkey patching example is appropriately updated to show how to import
GeoSettingsInlinealongside other admin classes from the geo module.openwisp_controller/geo/estimated_location/tasks.py (2)
7-10: LGTM!The imports are correctly updated to use the new
EstimatedLocationServicefrom the geo module and the extractedget_location_defaults_from_whoisutility function, aligning with the PR objective of separating geo logic from config.
85-90: LGTM!The refactoring correctly delegates to
EstimatedLocationServicefor location creation/update, and the log message is appropriately consolidated into a single f-string.openwisp_controller/geo/api/views.py (1)
349-359: Clean organization-scoped settings view wiring.The new view + route export are coherent with the serializer/model wiring and return a clear translated 404 for missing org settings.
Also applies to: 373-373
openwisp_controller/geo/estimated_location/mixins.py (1)
14-37: Service transition in serializer mixins looks correct.Switching to
EstimatedLocationServicekeeps output behavior intact while aligning ownership with the geo module refactor.openwisp_controller/settings.py (1)
4-8: Good consolidation of settings lookup.Centralizing the
OPENWISP_CONTROLLER_*key resolution improves consistency and reduces duplicated setting helpers.openwisp_controller/geo/api/serializers.py (1)
26-30: Serializer addition is aligned with the new endpoint contract.Read-only protection for
organizationandidis correctly set for organization-scoped settings updates.Also applies to: 167-168
openwisp_controller/config/admin.py (1)
1390-1396: Admin inline field cleanup matches the module split.Removing
estimated_location_enabledfrom config inline and retainingwhois_enabledis consistent with the geo-settings separation.openwisp_controller/geo/estimated_location/utils.py (1)
52-68: Utility extraction is clean and purpose-specific.
get_location_defaults_from_whoisis a good separation point for estimated-location defaults and aligns with the refactor intent.openwisp_controller/config/settings.py (1)
6-8: Shared settings accessor usage is a good cleanup.Using the common
get_settinghelper improves consistency across app modules.openwisp_controller/geo/settings.py (1)
10-18: LGTM!The configuration validation logic correctly ensures that WHOIS must be enabled before enabling the estimated location feature. The
ImproperlyConfiguredexception with a clear message is the appropriate Django pattern for configuration errors at startup.openwisp_controller/geo/models.py (2)
3-8: LGTM!The imports are correctly updated to include
AbstractOrganizationGeoSettingsfrom the base models.
29-33: LGTM!The new
OrganizationGeoSettingsmodel follows the established swappable model pattern used by the existingLocation,FloorPlan, andDeviceLocationmodels. The Meta class correctly inherits from the abstract parent and configures swappability.openwisp_controller/geo/apps.py (3)
17-21: LGTM!The imports are correctly updated to include
config_app_settingsand the newwhois_info_post_save_handlerfrom the estimated location handlers module.
29-35: LGTM!The
__setmodels__method is correctly extended to load the additional models needed for signal handling and organization geo settings management.
46-57: LGTM!The
connect_receivers()method properly:
- Uses
dispatch_uidto prevent duplicate signal connections- Conditionally registers the WHOIS handler only when
WHOIS_CONFIGUREDis true- Cleanly separates signal wiring from the main
ready()methodopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (1)
20-36: LGTM!The migration structure is correct with proper dependencies on the model migration and openwisp_users. Using
RunPython.noopfor reverse operations is acceptable for additive data migrations.openwisp_controller/config/whois/service.py (2)
15-16: LGTM!The import cleanup correctly removes dependencies that were related to the estimated location functionality, which has been moved to the geo module.
207-220: LGTM!The
process_ip_data_and_location()method is correctly simplified to only handle WHOIS lookup triggering. The estimated location logic has been properly migrated to the geo module where it's triggered via signal handlers onWHOISInfosave.openwisp_controller/geo/admin.py (3)
16-25: LGTM!The imports are correctly updated to bring in the necessary components for the new geo settings functionality and the refactored estimated location service.
130-134: LGTM!The switch from
WHOISService.check_estimated_location_enabledtoEstimatedLocationService.check_estimated_location_enabledcorrectly aligns with the refactoring that moved estimated location logic to the geo module.
225-228: LGTM!The inline is correctly inserted after
OrganizationLimitsInlinein theOrganizationAdmin.inlineslist.openwisp_controller/geo/migrations/__init__.py (2)
6-25: LGTM!The refactoring to extract
_assign_permissions_to_groupsas a shared helper follows DRY principles and maintains consistency with similar permission assignment patterns in other modules (pki,connection,config).
28-39: LGTM!The delegation pattern cleanly separates the original geo model permissions from the new geo settings permissions while reusing the same underlying logic.
openwisp_controller/geo/base/models.py (5)
60-62: LGTM!The migration from
WHOISService.check_estimated_location_enabled()toEstimatedLocationService.check_estimated_location_enabled()correctly aligns with the PR objective to separate geo-specific logic from the config module.
88-90: LGTM!Consistent with the
clean()method update, thesave()method now uses the newEstimatedLocationServicefor checking estimated location enablement.
164-175: LGTM!Well-designed model with appropriate field choices:
OneToOneFieldensures one settings record per organizationFallbackBooleanChoiceFieldwithgeo_settings.ESTIMATED_LOCATION_ENABLEDas fallback provides sensible defaults- Proper i18n with
_()for user-facing strings
185-195: LGTM!The
clean()validation correctly enforces that WHOIS must be configured before enabling the estimated location feature, matching the behavior previously inOrganizationConfigSettings.
197-204: LGTM!The
organization_post_save_receivercorrectly usesget_or_createfor idempotent settings creation when an organization is created. This pattern safely handles edge cases where the settings might already exist.openwisp_controller/geo/tests/test_api.py (6)
1359-1366: LGTM!Clean helper method for adding model permissions. Uses
ContentTypecorrectly to fetch permissions by codename pattern.
1368-1449: LGTM!Comprehensive test coverage for retrieving organization geo settings:
- Unauthenticated access returns 401
- Missing permissions returns 403
- With view permission returns 200 with expected fields
- Cross-organization access returns 404
- Superuser can access any organization
1450-1542: LGTM!Thorough update operation tests covering:
- PUT and PATCH operations
- Organization field in PUT is correctly ignored
- Cross-organization update attempts return 404
- WHOIS configuration validation returns 400
- Superuser can update any organization
1544-1581: LGTM!Good multi-tenancy tests validating organization-scoped access:
- User cannot access organizations they don't manage
- User can access organizations they do manage
- User managing multiple organizations can access both
1583-1612: LGTM!Proper user access level tests covering:
- Regular organization member without permissions gets 403
- Organization admin with view permission gets 200
- Superuser bypasses permission checks
1511-1521: 🧹 Nitpick | 🔵 TrivialTest assertion may not verify intended behavior.
When
WHOIS_CONFIGUREDis patched toFalse, the PUT request withestimated_location_enabled=Truereturns 400 as expected. However, line 1520-1521 asserts thatorg1_geo_settings.estimated_location_enabledis stillTrueafter the failed request.This assertion is checking that the value wasn't changed by the failed request, but since the previous successful PATCH on line 1477-1484 set it to
True, this just confirms the value didn't change on validation failure - which is correct but could use a clarifying comment.⛔ Skipped due to learnings
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.openwisp_controller/geo/estimated_location/tests/tests.py (11)
40-42: LGTM!Updated test class inheritance to use the new
TestEstimatedLocationMixinandTestGeoMixin, properly reflecting the refactored module structure.
48-63: LGTM!Tests correctly updated to validate the new
OrganizationGeoSettings.estimated_location_enabledfield instead of the oldOrganizationConfigSettingsfield.
76-78: LGTM!Admin form field name correctly updated from
config_settings-0-estimated_location_enabledtogeo_settings-0-estimated_location_enabled.
96-115: LGTM!New dedicated test method for
OrganizationGeoSettingsvalidation provides clear coverage:
- Validates that enabling estimated location requires WHOIS configuration
- Confirms estimated location can be enabled when WHOIS is configured
121-123: LGTM!Test correctly updated to disable estimated location via the new
OrganizationGeoSettingsmodel instead of the old config settings.
155-157: LGTM!Test setup properly uses
org.geo_settingsfor toggling estimated location feature.
234-235: LGTM!Correctly updated to use
geo_settingsfor enabling estimated location in test setup.
238-243: LGTM!Context manager syntax properly updated to use parenthesized form for multiple
mock.patchcalls.
622-629: LGTM!Test correctly toggles estimated location feature via
OrganizationGeoSettings.
801-816: LGTM!Test properly uses
geo_settingsfor toggling estimated location feature during status change tests.
867-868: LGTM!
assertNumQueriescount updated from previous value to 6, reflecting the changed query paths after model refactoring.openwisp_controller/geo/estimated_location/service.py (3)
16-29: LGTM!Well-designed static method with proper fallback logic:
- Returns
Falseearly iforg_idis missing or WHOIS not configured- Falls back to global
ESTIMATED_LOCATION_ENABLEDsetting when per-org settings don't exist- Uses
load_modelto avoid import-time circular dependencies
37-42: LGTM!Task dispatch method correctly uses
settings.CELERY_APP.send_task()with appropriate kwargs for async processing.
44-77: LGTM!Well-structured creation logic:
- Properly handles case when device has no location
- Uses
transaction.atomic()for data integrity- Calls
full_clean()before save for validation- Uses
_set_estimated=Trueto preventis_estimatedfrom being cleared- Sends appropriate notification after successful creation
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ( | ||
| "config", | ||
| "0063_organizationconfigsettings_estimated_location_enabled_and_more", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
# Check if geo migration 0006 actually migrates the estimated_location_enabled value
cat openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyRepository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
# Generated by Django 5.2.12 on 2026-03-23 14:59
from django.db import migrations
from . import assign_geo_settings_permissions_to_groups
def create_geo_settings_for_existing_orgs(apps, schema_editor):
"""
Create OrganizationGeoSettings for all existing organizations
that don't have one yet.
"""
Organization = apps.get_model("openwisp_users", "Organization")
OrganizationGeoSettings = apps.get_model("geo", "OrganizationGeoSettings")
for org in Organization.objects.all():
OrganizationGeoSettings.objects.get_or_create(organization_id=org.pk)
class Migration(migrations.Migration):
dependencies = [
("geo", "0005_organizationgeosettings"),
("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"),
]
operations = [
migrations.RunPython(
assign_geo_settings_permissions_to_groups,
reverse_code=migrations.RunPython.noop,
),
migrations.RunPython(
create_geo_settings_for_existing_orgs,
reverse_code=migrations.RunPython.noop,
),
]
Script executed:
# Search for OrganizationGeoSettings model definition
fd -e py -path "*/geo/*" | xargs rg -l "OrganizationGeoSettings" | head -5Repository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
tests/openwisp2/settings.py
tests/openwisp2/sample_geo/models.py
docs/developer/extending.rst
openwisp_controller/geo/apps.py
openwisp_controller/geo/models.py
stderr:
error: 'h' isn't a valid value for '--type <filetype>'
[possible values: file, directory, symlink, executable, empty, socket, pipe]
For more information try '--help'
Script executed:
# Check geo migration 0005 to see what fields are created for OrganizationGeoSettings
cat openwisp_controller/geo/migrations/0005_organizationgeosettings.pyRepository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
# Generated by Django 5.2.12 on 2026-03-23 14:59
import uuid
import django.db.models.deletion
import swapper
from django.db import migrations, models
import openwisp_utils.fields
class Migration(migrations.Migration):
dependencies = [
("geo", "0004_location_is_estimated"),
("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"),
]
operations = [
migrations.CreateModel(
name="OrganizationGeoSettings",
fields=[
(
"id",
models.UUIDField(
default=uuid.uuid4,
editable=False,
primary_key=True,
serialize=False,
),
),
(
"estimated_location_enabled",
openwisp_utils.fields.FallbackBooleanChoiceField(
blank=True,
default=None,
fallback=False,
help_text="Whether the estimated location feature is enabled",
null=True,
verbose_name="Estimated Location Enabled",
),
),
(
"organization",
models.OneToOneField(
on_delete=django.db.models.deletion.CASCADE,
related_name="geo_settings",
to="openwisp_users.organization",
verbose_name="organization",
),
),
],
options={
"verbose_name": "Geographic settings",
"verbose_name_plural": "Geographic settings",
"abstract": False,
"swappable": swapper.swappable_setting(
"geo", "OrganizationGeoSettings"
),
},
),
]
Script executed:
# Check if config's OrganizationConfigSettings has estimated_location_enabled
rg "class OrganizationConfigSettings" -A 30 openwisp_controller/config/Repository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
openwisp_controller/config/models.py:class OrganizationConfigSettings(AbstractOrganizationConfigSettings):
openwisp_controller/config/models.py- """
openwisp_controller/config/models.py- Configuration management settings
openwisp_controller/config/models.py- specific to each organization
openwisp_controller/config/models.py- """
openwisp_controller/config/models.py-
openwisp_controller/config/models.py- class Meta(AbstractOrganizationConfigSettings.Meta):
openwisp_controller/config/models.py- abstract = False
openwisp_controller/config/models.py- swappable = swapper.swappable_setting("config", "OrganizationConfigSettings")
openwisp_controller/config/models.py-
openwisp_controller/config/models.py-
openwisp_controller/config/models.py-class OrganizationLimits(AbstractOrganizationLimits):
openwisp_controller/config/models.py- """
openwisp_controller/config/models.py- Number of allowed devices specific to each organization
openwisp_controller/config/models.py- """
openwisp_controller/config/models.py-
openwisp_controller/config/models.py- class Meta(AbstractOrganizationLimits.Meta):
openwisp_controller/config/models.py- abstract = False
openwisp_controller/config/models.py- swappable = swapper.swappable_setting("config", "OrganizationLimits")
openwisp_controller/config/models.py-
openwisp_controller/config/models.py-
openwisp_controller/config/models.py-class WHOISInfo(AbstractWHOISInfo):
openwisp_controller/config/models.py- """
openwisp_controller/config/models.py- Stores WHOIS information for devices.
openwisp_controller/config/models.py- """
openwisp_controller/config/models.py-
openwisp_controller/config/models.py- class Meta(AbstractWHOISInfo.Meta):
openwisp_controller/config/models.py- abstract = False
openwisp_controller/config/models.py- swappable = swapper.swappable_setting("config", "WHOISInfo")
Script executed:
# Check config migration 0063 to see when estimated_location_enabled was added
cat openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.pyRepository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
# Generated by Django 5.2.1 on 2025-07-10 18:09
import django.contrib.gis.db.models.fields
from django.db import migrations
import openwisp_utils.fields
class Migration(migrations.Migration):
dependencies = [
("config", "0062_whoisinfo"),
]
operations = [
migrations.AddField(
model_name="organizationconfigsettings",
name="estimated_location_enabled",
field=openwisp_utils.fields.FallbackBooleanChoiceField(
blank=True,
default=None,
fallback=False,
help_text="Whether the estimated location feature is enabled",
null=True,
verbose_name="Estimated Location Enabled",
),
),
migrations.AddField(
model_name="whoisinfo",
name="coordinates",
field=django.contrib.gis.db.models.fields.PointField(
blank=True, help_text="Coordinates", null=True, srid=4326
),
),
]
Script executed:
# Search for AbstractOrganizationConfigSettings to see the field definition
rg "estimated_location_enabled" openwisp_controller/config/ -B 2 -A 2Repository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- (
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- "config",
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py: "0063_organizationconfigsettings_estimated_location_enabled_and_more",
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- ),
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- ]
--
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- migrations.RemoveField(
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- model_name="organizationconfigsettings",
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py: name="estimated_location_enabled",
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- ),
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py- ]
--
openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py- migrations.AddField(
openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py- model_name="organizationconfigsettings",
openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py: name="estimated_location_enabled",
openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py- field=openwisp_utils.fields.FallbackBooleanChoiceField(
openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py- blank=True,
Script executed:
# Check the base model to understand the field
rg "class AbstractOrganizationConfigSettings" -A 50 openwisp_controller/config/Repository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
openwisp_controller/config/base/multitenancy.py:class AbstractOrganizationConfigSettings(UUIDModel):
openwisp_controller/config/base/multitenancy.py- organization = models.OneToOneField(
openwisp_controller/config/base/multitenancy.py- swapper.get_model_name("openwisp_users", "Organization"),
openwisp_controller/config/base/multitenancy.py- verbose_name=_("organization"),
openwisp_controller/config/base/multitenancy.py- related_name="config_settings",
openwisp_controller/config/base/multitenancy.py- on_delete=models.CASCADE,
openwisp_controller/config/base/multitenancy.py- )
openwisp_controller/config/base/multitenancy.py- registration_enabled = models.BooleanField(
openwisp_controller/config/base/multitenancy.py- _("auto-registration enabled"),
openwisp_controller/config/base/multitenancy.py- default=True,
openwisp_controller/config/base/multitenancy.py- help_text=_("Whether automatic registration of devices is enabled or not"),
openwisp_controller/config/base/multitenancy.py- )
openwisp_controller/config/base/multitenancy.py- shared_secret = KeyField(
openwisp_controller/config/base/multitenancy.py- max_length=32,
openwisp_controller/config/base/multitenancy.py- unique=True,
openwisp_controller/config/base/multitenancy.py- db_index=True,
openwisp_controller/config/base/multitenancy.py- verbose_name=_("shared secret"),
openwisp_controller/config/base/multitenancy.py- help_text=_("used for automatic registration of devices"),
openwisp_controller/config/base/multitenancy.py- )
openwisp_controller/config/base/multitenancy.py- whois_enabled = FallbackBooleanChoiceField(
openwisp_controller/config/base/multitenancy.py- help_text=_("Whether the WHOIS lookup feature is enabled"),
openwisp_controller/config/base/multitenancy.py- fallback=app_settings.WHOIS_ENABLED,
openwisp_controller/config/base/multitenancy.py- verbose_name=_("WHOIS Enabled"),
openwisp_controller/config/base/multitenancy.py- )
openwisp_controller/config/base/multitenancy.py- context = JSONField(
openwisp_controller/config/base/multitenancy.py- blank=True,
openwisp_controller/config/base/multitenancy.py- default=dict,
openwisp_controller/config/base/multitenancy.py- load_kwargs={"object_pairs_hook": collections.OrderedDict},
openwisp_controller/config/base/multitenancy.py- dump_kwargs={"indent": 4},
openwisp_controller/config/base/multitenancy.py- help_text=_(
openwisp_controller/config/base/multitenancy.py- "Define reusable configuration variables available "
openwisp_controller/config/base/multitenancy.py- "to all devices in this organization"
openwisp_controller/config/base/multitenancy.py- ),
openwisp_controller/config/base/multitenancy.py- verbose_name=_("Configuration Variables"),
openwisp_controller/config/base/multitenancy.py- )
openwisp_controller/config/base/multitenancy.py-
openwisp_controller/config/base/multitenancy.py- class Meta:
openwisp_controller/config/base/multitenancy.py- verbose_name = _("Configuration management settings")
openwisp_controller/config/base/multitenancy.py- verbose_name_plural = verbose_name
openwisp_controller/config/base/multitenancy.py- abstract = True
openwisp_controller/config/base/multitenancy.py-
openwisp_controller/config/base/multitenancy.py- def __str__(self):
openwisp_controller/config/base/multitenancy.py- return self.organization.name
openwisp_controller/config/base/multitenancy.py-
openwisp_controller/config/base/multitenancy.py- def get_context(self):
openwisp_controller/config/base/multitenancy.py- return deepcopy(self.context)
openwisp_controller/config/base/multitenancy.py-
openwisp_controller/config/base/multitenancy.py- def clean(self):
openwisp_controller/config/base/multitenancy.py- if not app_settings.WHOIS_CONFIGURED and self.whois_enabled:
openwisp_controller/config/base/multitenancy.py- raise ValidationError(
openwisp_controller/config/base/multitenancy.py- {
Script executed:
# Check the complete migration 0064 to see all operations
cat openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyRepository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
# Generated by Django 5.2.12 on 2026-03-23 14:59
from django.db import migrations
class Migration(migrations.Migration):
dependencies = [
(
"config",
"0063_organizationconfigsettings_estimated_location_enabled_and_more",
),
]
operations = [
migrations.RemoveField(
model_name="organizationconfigsettings",
name="estimated_location_enabled",
),
]
Script executed:
# Check if geo 0006 should have data migration logic - look for similar patterns
rg "RunPython" openwisp_controller/geo/migrations/ -A 5Repository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py: migrations.RunPython(
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py- assign_geo_settings_permissions_to_groups,
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py: reverse_code=migrations.RunPython.noop,
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py- ),
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py: migrations.RunPython(
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py- create_geo_settings_for_existing_orgs,
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py: reverse_code=migrations.RunPython.noop,
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py- ),
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py- ]
--
openwisp_controller/geo/migrations/0002_default_groups_permissions.py: migrations.RunPython(
openwisp_controller/geo/migrations/0002_default_groups_permissions.py: assign_permissions_to_groups, reverse_code=migrations.RunPython.noop
openwisp_controller/geo/migrations/0002_default_groups_permissions.py- )
openwisp_controller/geo/migrations/0002_default_groups_permissions.py- ]
Script executed:
# Verify if there's any data migration happening elsewhere
rg "estimated_location_enabled" openwisp_controller/geo/ -B 2 -A 2Repository: openwisp/openwisp-controller
Repository: openwisp/openwisp-controller
Exit code: 0
stdout:
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- self.assertEqual(str(data["organization"]), str(org1.pk))
openwisp_controller/geo/tests/test_api.py- self.assertEqual(
openwisp_controller/geo/tests/test_api.py: data["estimated_location_enabled"],
openwisp_controller/geo/tests/test_api.py: org1_geo_settings.estimated_location_enabled,
openwisp_controller/geo/tests/test_api.py- )
openwisp_controller/geo/tests/test_api.py-
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
openwisp_controller/geo/tests/test_api.py- self.assertEqual(response.status_code, 200)
openwisp_controller/geo/tests/test_api.py- org1_geo_settings.refresh_from_db()
openwisp_controller/geo/tests/test_api.py: self.assertEqual(org1_geo_settings.estimated_location_enabled, False)
openwisp_controller/geo/tests/test_api.py-
openwisp_controller/geo/tests/test_api.py- with self.subTest("PATCH operation"):
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": True},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
openwisp_controller/geo/tests/test_api.py- self.assertEqual(response.status_code, 200)
openwisp_controller/geo/tests/test_api.py- org1_geo_settings.refresh_from_db()
openwisp_controller/geo/tests/test_api.py: self.assertEqual(org1_geo_settings.estimated_location_enabled, True)
openwisp_controller/geo/tests/test_api.py-
openwisp_controller/geo/tests/test_api.py- with self.subTest("PUT with organization field should be ignored"):
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False, "organization": str(org2.pk)},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- org1_geo_settings.refresh_from_db()
openwisp_controller/geo/tests/test_api.py- self.assertEqual(org1_geo_settings.organization, org1)
openwisp_controller/geo/tests/test_api.py: self.assertEqual(org1_geo_settings.estimated_location_enabled, False)
openwisp_controller/geo/tests/test_api.py-
openwisp_controller/geo/tests/test_api.py- with self.subTest("Cannot update geo settings of other organization"):
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- org2_url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- org2_url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": True},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
openwisp_controller/geo/tests/test_api.py- self.assertEqual(response.status_code, 400)
openwisp_controller/geo/tests/test_api.py: self.assertIn("estimated_location_enabled", response.data)
openwisp_controller/geo/tests/test_api.py- org1_geo_settings.refresh_from_db()
openwisp_controller/geo/tests/test_api.py: self.assertEqual(org1_geo_settings.estimated_location_enabled, True)
openwisp_controller/geo/tests/test_api.py-
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": True},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
openwisp_controller/geo/tests/test_api.py- self.assertEqual(response.status_code, 400)
openwisp_controller/geo/tests/test_api.py: self.assertIn("estimated_location_enabled", response.data)
openwisp_controller/geo/tests/test_api.py-
openwisp_controller/geo/tests/test_api.py- with self.subTest("Superuser can update any organization's geo settings"):
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- org2_url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": True},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
openwisp_controller/geo/tests/test_api.py- self.assertEqual(response.status_code, 200)
openwisp_controller/geo/tests/test_api.py- org2_geo_settings.refresh_from_db()
openwisp_controller/geo/tests/test_api.py: self.assertEqual(org2_geo_settings.estimated_location_enabled, True)
openwisp_controller/geo/tests/test_api.py-
openwisp_controller/geo/tests/test_api.py- def test_organization_geo_settings_multi_tenancy(self):
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- org2_url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.put(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/tests/test_api.py- response = self.client.patch(
openwisp_controller/geo/tests/test_api.py- url,
openwisp_controller/geo/tests/test_api.py: {"estimated_location_enabled": False},
openwisp_controller/geo/tests/test_api.py- content_type="application/json",
openwisp_controller/geo/tests/test_api.py- )
--
openwisp_controller/geo/estimated_location/handlers.py- except Device.DoesNotExist:
openwisp_controller/geo/estimated_location/handlers.py- return
openwisp_controller/geo/estimated_location/handlers.py: if not EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/estimated_location/handlers.py- device.organization_id
openwisp_controller/geo/estimated_location/handlers.py- ):
--
openwisp_controller/geo/migrations/0005_organizationgeosettings.py- ),
openwisp_controller/geo/migrations/0005_organizationgeosettings.py- (
openwisp_controller/geo/migrations/0005_organizationgeosettings.py: "estimated_location_enabled",
openwisp_controller/geo/migrations/0005_organizationgeosettings.py- openwisp_utils.fields.FallbackBooleanChoiceField(
openwisp_controller/geo/migrations/0005_organizationgeosettings.py- blank=True,
--
openwisp_controller/geo/estimated_location/service.py-
openwisp_controller/geo/estimated_location/service.py- `@staticmethod`
openwisp_controller/geo/estimated_location/service.py: def check_estimated_location_enabled(org_id):
openwisp_controller/geo/estimated_location/service.py- if not org_id:
openwisp_controller/geo/estimated_location/service.py- return False
--
openwisp_controller/geo/estimated_location/service.py-
openwisp_controller/geo/estimated_location/service.py- return geo_app_settings.ESTIMATED_LOCATION_ENABLED
openwisp_controller/geo/estimated_location/service.py: return org_settings.estimated_location_enabled
openwisp_controller/geo/estimated_location/service.py-
openwisp_controller/geo/estimated_location/service.py- `@property`
openwisp_controller/geo/estimated_location/service.py: def is_estimated_location_enabled(self):
openwisp_controller/geo/estimated_location/service.py- if not config_app_settings.WHOIS_CONFIGURED:
openwisp_controller/geo/estimated_location/service.py- return False
openwisp_controller/geo/estimated_location/service.py: return self.check_estimated_location_enabled(self.device.organization_id)
openwisp_controller/geo/estimated_location/service.py-
openwisp_controller/geo/estimated_location/service.py- def trigger_estimated_location_task(self, ip_address):
--
openwisp_controller/geo/estimated_location/mixins.py- def to_representation(self, obj):
openwisp_controller/geo/estimated_location/mixins.py- data = super().to_representation(obj)
openwisp_controller/geo/estimated_location/mixins.py: if EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/estimated_location/mixins.py- obj.organization_id
openwisp_controller/geo/estimated_location/mixins.py- ):
--
openwisp_controller/geo/estimated_location/mixins.py- def to_representation(self, obj):
openwisp_controller/geo/estimated_location/mixins.py- data = super().to_representation(obj)
openwisp_controller/geo/estimated_location/mixins.py: if EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/estimated_location/mixins.py- obj.organization_id
openwisp_controller/geo/estimated_location/mixins.py- ):
--
openwisp_controller/geo/estimated_location/tests/utils.py- )
openwisp_controller/geo/estimated_location/tests/utils.py- # OrganizationGeoSettings is auto-created by signal,
openwisp_controller/geo/estimated_location/tests/utils.py: # update estimated_location_enabled
openwisp_controller/geo/estimated_location/tests/utils.py: org.geo_settings.estimated_location_enabled = True
openwisp_controller/geo/estimated_location/tests/utils.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/utils.py-
--
openwisp_controller/geo/estimated_location/tests/tests.py- org = self._get_org()
openwisp_controller/geo/estimated_location/tests/tests.py- geo_settings = org.geo_settings
openwisp_controller/geo/estimated_location/tests/tests.py: geo_settings.estimated_location_enabled = True
openwisp_controller/geo/estimated_location/tests/tests.py- with mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", False):
openwisp_controller/geo/estimated_location/tests/tests.py- with self.assertRaises(ValidationError) as context_manager:
openwisp_controller/geo/estimated_location/tests/tests.py- geo_settings.full_clean()
openwisp_controller/geo/estimated_location/tests/tests.py- self.assertIn(
openwisp_controller/geo/estimated_location/tests/tests.py: "estimated_location_enabled",
openwisp_controller/geo/estimated_location/tests/tests.py- context_manager.exception.message_dict,
openwisp_controller/geo/estimated_location/tests/tests.py- )
--
openwisp_controller/geo/estimated_location/tests/tests.py- response = self.client.get(url)
openwisp_controller/geo/estimated_location/tests/tests.py- self.assertContains(
openwisp_controller/geo/estimated_location/tests/tests.py: response, 'name="geo_settings-0-estimated_location_enabled"'
openwisp_controller/geo/estimated_location/tests/tests.py- )
openwisp_controller/geo/estimated_location/tests/tests.py-
--
openwisp_controller/geo/estimated_location/tests/tests.py- response = self.client.get(url)
openwisp_controller/geo/estimated_location/tests/tests.py- self.assertNotContains(
openwisp_controller/geo/estimated_location/tests/tests.py: response, 'name="geo_settings-0-estimated_location_enabled"'
openwisp_controller/geo/estimated_location/tests/tests.py- )
openwisp_controller/geo/estimated_location/tests/tests.py-
--
openwisp_controller/geo/estimated_location/tests/tests.py- with self.subTest("WHOIS must be configured to enable estimated location"):
openwisp_controller/geo/estimated_location/tests/tests.py- geo_settings = org.geo_settings
openwisp_controller/geo/estimated_location/tests/tests.py: geo_settings.estimated_location_enabled = True
openwisp_controller/geo/estimated_location/tests/tests.py- with mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", False):
openwisp_controller/geo/estimated_location/tests/tests.py- with self.assertRaises(ValidationError) as context_manager:
openwisp_controller/geo/estimated_location/tests/tests.py- geo_settings.full_clean()
openwisp_controller/geo/estimated_location/tests/tests.py- self.assertIn(
openwisp_controller/geo/estimated_location/tests/tests.py: "estimated_location_enabled",
openwisp_controller/geo/estimated_location/tests/tests.py- context_manager.exception.message_dict,
openwisp_controller/geo/estimated_location/tests/tests.py- )
--
openwisp_controller/geo/estimated_location/tests/tests.py- with self.subTest("Estimated location can be enabled when WHOIS is configured"):
openwisp_controller/geo/estimated_location/tests/tests.py- geo_settings = org.geo_settings
openwisp_controller/geo/estimated_location/tests/tests.py: geo_settings.estimated_location_enabled = True
openwisp_controller/geo/estimated_location/tests/tests.py- # Should not raise
openwisp_controller/geo/estimated_location/tests/tests.py- geo_settings.full_clean()
--
openwisp_controller/geo/estimated_location/tests/tests.py- def test_estimated_location_field(self):
openwisp_controller/geo/estimated_location/tests/tests.py- org = self._get_org()
openwisp_controller/geo/estimated_location/tests/tests.py: # Disable estimated_location_enabled via OrganizationGeoSettings
openwisp_controller/geo/estimated_location/tests/tests.py: org.geo_settings.estimated_location_enabled = False
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py- with self.assertRaises(ValidationError) as context_manager:
--
openwisp_controller/geo/estimated_location/tests/tests.py- self.assertNotContains(response, "field-is_estimated")
openwisp_controller/geo/estimated_location/tests/tests.py-
openwisp_controller/geo/estimated_location/tests/tests.py: org.geo_settings.estimated_location_enabled = False
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.refresh_from_db()
--
openwisp_controller/geo/estimated_location/tests/tests.py- org.config_settings.whois_enabled = True
openwisp_controller/geo/estimated_location/tests/tests.py- org.config_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py: org.geo_settings.estimated_location_enabled = True
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py-
--
openwisp_controller/geo/estimated_location/tests/tests.py- mocked_set.assert_not_called()
openwisp_controller/geo/estimated_location/tests/tests.py- # The cache `get` is called twice, once for `whois_enabled` and
openwisp_controller/geo/estimated_location/tests/tests.py: # once for `estimated_location_enabled`
openwisp_controller/geo/estimated_location/tests/tests.py- mocked_get.assert_called()
openwisp_controller/geo/estimated_location/tests/tests.py- mocked_estimated_location_task.assert_called()
--
openwisp_controller/geo/estimated_location/tests/tests.py- new_time = timezone.now() - timedelta(days=threshold)
openwisp_controller/geo/estimated_location/tests/tests.py- org = self._get_org()
openwisp_controller/geo/estimated_location/tests/tests.py: org.geo_settings.estimated_location_enabled = False
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py- device = self._create_device(last_ip="172.217.22.10")
--
openwisp_controller/geo/estimated_location/tests/tests.py- # Accessing devicelocation to verify it doesn't exist (raises if not)
openwisp_controller/geo/estimated_location/tests/tests.py- device.devicelocation
openwisp_controller/geo/estimated_location/tests/tests.py: org.geo_settings.estimated_location_enabled = True
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py- whois_obj = device.whois_service.get_device_whois_info()
--
openwisp_controller/geo/estimated_location/tests/tests.py- "Test Estimated Status unchanged if Estimated feature is disabled"
openwisp_controller/geo/estimated_location/tests/tests.py- ):
openwisp_controller/geo/estimated_location/tests/tests.py: org.geo_settings.estimated_location_enabled = False
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.refresh_from_db()
--
openwisp_controller/geo/estimated_location/tests/tests.py- " and desired fields not changed"
openwisp_controller/geo/estimated_location/tests/tests.py- ):
openwisp_controller/geo/estimated_location/tests/tests.py: org.geo_settings.estimated_location_enabled = True
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.save()
openwisp_controller/geo/estimated_location/tests/tests.py- org.geo_settings.refresh_from_db()
--
openwisp_controller/geo/base/models.py- (self._state.adding or estimated_status_changed)
openwisp_controller/geo/base/models.py- and self.is_estimated
openwisp_controller/geo/base/models.py: and not EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/base/models.py- self.organization_id
openwisp_controller/geo/base/models.py- )
--
openwisp_controller/geo/base/models.py- """
openwisp_controller/geo/base/models.py- changed_fields = set()
openwisp_controller/geo/base/models.py: if EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/base/models.py- self.organization_id
openwisp_controller/geo/base/models.py- ):
--
openwisp_controller/geo/base/models.py- on_delete=models.CASCADE,
openwisp_controller/geo/base/models.py- )
openwisp_controller/geo/base/models.py: estimated_location_enabled = FallbackBooleanChoiceField(
openwisp_controller/geo/base/models.py- help_text=_("Whether the estimated location feature is enabled"),
openwisp_controller/geo/base/models.py- fallback=geo_settings.ESTIMATED_LOCATION_ENABLED,
--
openwisp_controller/geo/base/models.py-
openwisp_controller/geo/base/models.py- def clean(self):
openwisp_controller/geo/base/models.py: if not config_settings.WHOIS_CONFIGURED and self.estimated_location_enabled:
openwisp_controller/geo/base/models.py- raise ValidationError(
openwisp_controller/geo/base/models.py- {
openwisp_controller/geo/base/models.py: "estimated_location_enabled": _(
openwisp_controller/geo/base/models.py- "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set "
openwisp_controller/geo/base/models.py- "before enabling Estimated Location feature."
--
openwisp_controller/geo/admin.py- # do not show the is_estimated field on add pages
openwisp_controller/geo/admin.py- # or if the estimated location feature is not enabled for the organization
openwisp_controller/geo/admin.py: if not obj or not EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/admin.py- obj.organization_id
openwisp_controller/geo/admin.py- ):
--
openwisp_controller/geo/admin.py- def get_readonly_fields(self, request, obj=None):
openwisp_controller/geo/admin.py- fields = super().get_readonly_fields(request, obj)
openwisp_controller/geo/admin.py: if obj and EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/admin.py- obj.organization_id
openwisp_controller/geo/admin.py- ):
--
openwisp_controller/geo/admin.py- obj = self.get_object(request, object_id)
openwisp_controller/geo/admin.py- org_id = obj.organization_id if obj else None
openwisp_controller/geo/admin.py: estimated_enabled = EstimatedLocationService.check_estimated_location_enabled(
openwisp_controller/geo/admin.py- org_id
openwisp_controller/geo/admin.py- )
--
openwisp_controller/geo/admin.py- fields = list(super().get_readonly_fields(request, obj))
openwisp_controller/geo/admin.py- if (
openwisp_controller/geo/admin.py: "estimated_location_enabled" in fields
openwisp_controller/geo/admin.py- and not config_app_settings.WHOIS_CONFIGURED
openwisp_controller/geo/admin.py- ):
openwisp_controller/geo/admin.py: fields = ["estimated_location_enabled"] + fields
openwisp_controller/geo/admin.py- return fields
openwisp_controller/geo/admin.py-
Add cross-app migration dependency AND implement data migration logic in geo 0006.
The removal of estimated_location_enabled from config settings must wait for the geo migration to complete, but there's a deeper issue: geo.0006_create_geo_settings_for_existing_orgs creates new OrganizationGeoSettings records without migrating existing estimated_location_enabled values from config settings. This causes data loss if any organizations had estimated_location_enabled=True.
Add a dependency on the geo data migration to ensure proper ordering:
Proposed change
dependencies = [
(
"config",
"0063_organizationconfigsettings_estimated_location_enabled_and_more",
),
+ (
+ "geo",
+ "0006_create_geo_settings_for_existing_orgs",
+ ),
]Additionally, geo.0006_create_geo_settings_for_existing_orgs should include a RunPython operation to copy estimated_location_enabled values from OrganizationConfigSettings to the newly created OrganizationGeoSettings for each organization.
📝 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.
| class Migration(migrations.Migration): | |
| dependencies = [ | |
| ( | |
| "config", | |
| "0063_organizationconfigsettings_estimated_location_enabled_and_more", | |
| ), | |
| ] | |
| class Migration(migrations.Migration): | |
| dependencies = [ | |
| ( | |
| "config", | |
| "0063_organizationconfigsettings_estimated_location_enabled_and_more", | |
| ), | |
| ( | |
| "geo", | |
| "0006_create_geo_settings_for_existing_orgs", | |
| ), | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py`
around lines 6 - 13, Migration 0064 must depend on the geo data migration and
the geo migration 0006 must copy the field before config removes it: add ("geo",
"0006_create_geo_settings_for_existing_orgs") to the dependencies list in
Migration class in
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py,
and modify geo migration
geo/migrations/0006_create_geo_settings_for_existing_orgs.py to include a
RunPython operation that defines a forward function (e.g.,
copy_estimated_location_enabled) which uses apps.get_model to load
OrganizationConfigSettings and OrganizationGeoSettings, iterates organizations
(or queries existing config records), and copies the estimated_location_enabled
boolean into the newly created OrganizationGeoSettings records (creating or
updating as needed), plus a safe reverse/noop function; use only migration-safe
ORM access (no direct model imports) and include this RunPython in the migration
operations so the config removal does not lose data.
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py
Outdated
Show resolved
Hide resolved
Flake8 F401 Unused ImportHello @pandafy, The CI build failed due to a Failure:
Fix: |
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
openwisp_controller/geo/estimated_location/tests/utils.py (1)
16-25:⚠️ Potential issue | 🟡 MinorCreate geo settings defensively in test setup.
_get_org()can still return an organization without a relatedOrganizationGeoSettings, soorg.geo_settingsmay raiseRelatedObjectDoesNotExisthere. Fetch or create the row before togglingestimated_location_enabled.🧪 Proposed fix
org = self._get_org() + geo_settings, _ = OrganizationGeoSettings.objects.get_or_create( + organization=org + ) OrganizationConfigSettings.objects.create( organization=org, whois_enabled=True, ) - # OrganizationGeoSettings is auto-created by signal on Organization - # creation. Access the related object and update - # estimated_location_enabled to enable the feature for tests. - org.geo_settings.estimated_location_enabled = True - org.geo_settings.save() + geo_settings.estimated_location_enabled = True + geo_settings.save(update_fields=["estimated_location_enabled"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/estimated_location/tests/utils.py` around lines 16 - 25, The test setup assumes org.geo_settings exists but may raise RelatedObjectDoesNotExist; change the setup in the utils where _get_org() is used to ensure OrganizationGeoSettings exists before toggling estimated_location_enabled by using a get_or_create (or fetch-or-create) for the OrganizationGeoSettings tied to the org, then set estimated_location_enabled=True and save; keep the OrganizationConfigSettings creation as-is and reference the org via _get_org(), OrganizationConfigSettings, and OrganizationGeoSettings/ org.geo_settings to locate the affected code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/rest-api.rst`:
- Around line 814-834: Split the combined PUT/PATCH section into two
subsections: keep a "Update Organization Geographic Settings" subsection
containing only the PUT method and its curl example, and add a new "Partially
Update Organization Geographic Settings" subsection containing only the PATCH
method and its curl example; retain the endpoint path
/api/v1/controller/organization/{organization_pk}/geo-settings/ and explicitly
state in the PUT subsection that it represents a full replacement while the
PATCH subsection allows partial updates, and ensure each subsection has its own
code-blocks for the HTTP verb and a distinct curl invocation (one using PUT, one
using PATCH) so the docs match the established pattern used elsewhere.
In `@openwisp_controller/config/whois/tasks.py`:
- Around line 89-96: The on_commit lambda currently calls whois_fetched.send
which re-raises receiver exceptions; change it to call whois_fetched.send_robust
with the same kwargs (sender=WHOISInfo, whois=whois_obj,
updated_fields=update_fields, device=device) and iterate over the returned
(receiver, result) tuples to log any exceptions (result instances of Exception)
via the existing logger instead of letting them propagate; update the
transaction.on_commit callback that currently references whois_fetched.send to
use send_robust and explicit exception-logging.
In `@openwisp_controller/geo/admin.py`:
- Around line 212-224: The OrganizationGeoSettings inline currently allows the
singleton row to be deleted via the admin delete checkbox; update the inline to
prevent deletion by adding a class-level can_delete = False (and to be safe
enforce a single instance by setting max_num = 1 or overriding get_max_num to
return 1) on GeoSettingsInline (and apply the same change to the other inline
block around the second occurrence), so admins can only edit fields (like
estimated_location_enabled) and cannot remove the singleton
OrganizationGeoSettings row.
In `@openwisp_controller/geo/apps.py`:
- Around line 66-71: The whois_fetched signal handler (whois_fetched_handler) no
longer covers cases where a device is created or its last_ip changes to an IP
with an already-fresh WHOISInfo row because
WHOISService.process_ip_data_and_location() fallback was removed; add a geo-side
hook that triggers the same estimated-location logic when a Device's last_ip is
created/updated (e.g., connect a receiver to the Device model
post_save/post_update or the serializer/handler that changes last_ip and call
the existing whois_fetched_handler or the internal function that applies
estimated location), or restore the prior fallback call inside
WHOISService.process_ip_data_and_location() to invoke the estimated-location
routine when a fresh WHOISInfo exists; reference whois_fetched,
whois_fetched_handler, and WHOISService.process_ip_data_and_location() to locate
and wire the fix.
In `@openwisp_controller/geo/estimated_location/service.py`:
- Around line 37-44: The code path that handles a missing
OrganizationGeoSettings currently returns early without populating the cache,
causing repeated DB lookups; modify the block that catches
OrganizationGeoSettings.DoesNotExist (where org_settings is fetched) to cache
the no-row fallback (e.g. cache.set(cache_key,
geo_settings.estimated_location_enabled) or a sentinel/False value) before
returning so subsequent calls hit the cache; update the logic around
org_settings, cache_key and OrganizationGeoSettings lookup in service.py to
store the fallback value and then return it.
- Around line 64-68: The send_task call in trigger_estimated_location_task
currently runs inside the WHOIS signal handler and can raise if the broker is
down; wrap current_app.send_task(...) in a try/except that catches broad
exceptions, log the exception at error level (e.g., using
current_app.logger.error or the module logger) with context including
self.device.pk and ip_address, and do not re-raise so the device save proceeds
even if publishing fails.
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 350-355: The test is patching send_whois_task_notification in the
wrong module so the real function runs; update the mocks to patch
openwisp_controller.geo.estimated_location.service.send_whois_task_notification
(the symbol imported and called by
EstimatedLocationService._create_or_update_estimated_location) instead of
config.whois.utils or geo.estimated_location.tasks, and apply the same change to
the other occurrence around lines 749-756 so the service-level notification call
is intercepted and prevents real side effects.
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 13-14: Replace hard-coded apps.get_model("geo",
"OrganizationGeoSettings") (and any apps.get_model usage for the swappable
OrganizationConfigSettings) with the project helper get_swapped_model to resolve
swapped models at runtime: import get_swapped_model from
openwisp_controller.migrations and call get_swapped_model(apps, "geo",
"OrganizationGeoSettings") (and get_swapped_model(apps, "config",
"OrganizationConfigSettings") where applicable) so the migration targets the
installed/swapped models instead of historical defaults; update the copy loop
that uses getattr(..., None) to operate on the swapped models returned by
get_swapped_model.
In `@openwisp_controller/geo/settings.py`:
- Line 8: Remove the redundant inline comment "# Validate that WHOIS is enabled
if estimated location is enabled" in openwisp_controller/geo/settings.py; the
conditional that enforces WHOIS when estimated location is enabled is already
self-descriptive, so delete that comment line to satisfy the guideline against
unnecessary comments.
- Around line 9-13: The startup validation currently checks
config_settings.WHOIS_ENABLED when ESTIMATED_LOCATION_ENABLED is true; change
this to check config_settings.WHOIS_CONFIGURED instead so the validation ensures
WHOIS_GEOIP_ACCOUNT/WHOIS_GEOIP_KEY are present. Update the condition in the
block that references ESTIMATED_LOCATION_ENABLED and the raised
ImproperlyConfigured check from WHOIS_ENABLED to WHOIS_CONFIGURED (and adjust
the exception message to mention WHOIS credentials/WHOIS_CONFIGURED) so the
startup validation matches the rest of the codebase.
---
Duplicate comments:
In `@openwisp_controller/geo/estimated_location/tests/utils.py`:
- Around line 16-25: The test setup assumes org.geo_settings exists but may
raise RelatedObjectDoesNotExist; change the setup in the utils where _get_org()
is used to ensure OrganizationGeoSettings exists before toggling
estimated_location_enabled by using a get_or_create (or fetch-or-create) for the
OrganizationGeoSettings tied to the org, then set
estimated_location_enabled=True and save; keep the OrganizationConfigSettings
creation as-is and reference the org via _get_org(), OrganizationConfigSettings,
and OrganizationGeoSettings/ org.geo_settings to locate the affected code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fa30623-e4df-4400-9300-e23ae417b40b
📒 Files selected for processing (16)
docs/developer/utils.rstdocs/user/rest-api.rstopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/settings.pytests/openwisp2/sample_geo/models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/signals.pyopenwisp_controller/config/whois/tasks.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/signals.pyopenwisp_controller/config/whois/tasks.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/signals.pyopenwisp_controller/config/whois/tasks.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/signals.pyopenwisp_controller/config/whois/tasks.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
🧠 Learnings (7)
📓 Common learnings
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.
📚 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/signals.pyopenwisp_controller/config/whois/tasks.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/signals.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/signals.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/signals.pyopenwisp_controller/config/whois/tasks.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/settings.pyopenwisp_controller/config/whois/service.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/whois/service.py
🧬 Code graph analysis (12)
openwisp_controller/config/whois/tasks.py (3)
openwisp_controller/config/models.py (1)
WHOISInfo(117-124)tests/openwisp2/sample_config/models.py (1)
WHOISInfo(117-123)openwisp_controller/geo/base/models.py (1)
device(156-157)
tests/openwisp2/sample_geo/models.py (4)
openwisp_controller/geo/base/models.py (1)
AbstractOrganizationGeoSettings(164-204)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)tests/openwisp2/sample_connection/models.py (1)
DetailsModel(10-14)tests/openwisp2/sample_pki/models.py (1)
DetailsModel(6-10)
openwisp_controller/geo/estimated_location/tests/utils.py (4)
tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/geo/estimated_location/tests/tests.py (2)
setUp(203-210)setUp(840-843)openwisp_controller/geo/base/models.py (1)
save(73-110)
openwisp_controller/config/whois/tests/tests.py (1)
openwisp_controller/config/whois/tasks.py (1)
fetch_whois_details(62-101)
openwisp_controller/geo/estimated_location/handlers.py (2)
openwisp_controller/geo/estimated_location/service.py (3)
EstimatedLocationService(16-118)check_estimated_location_enabled(21-45)trigger_estimated_location_task(64-68)openwisp_controller/geo/base/models.py (2)
device(156-157)organization_id(160-161)
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (3)
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (1)
Migration(42-61)openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
Migration(12-62)openwisp_controller/config/base/config.py (1)
name(132-139)
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (6)
openwisp_controller/geo/migrations/__init__.py (1)
assign_geo_settings_permissions_to_groups(36-39)tests/openwisp2/sample_users/models.py (1)
Organization(32-36)tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(6-20)openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
Migration(12-62)
openwisp_controller/geo/apps.py (4)
openwisp_controller/geo/estimated_location/handlers.py (2)
register_estimated_location_notification_types(11-36)whois_fetched_handler(39-62)openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(16-118)invalidate_org_settings_cache(53-58)openwisp_controller/geo/base/models.py (1)
organization_post_save_receiver(198-204)openwisp_controller/config/base/whois.py (1)
invalidate_org_settings_cache(111-117)
openwisp_controller/geo/admin.py (5)
openwisp_controller/admin.py (1)
MultitenantAdminMixin(24-29)openwisp_controller/config/admin.py (2)
DeactivatedDeviceReadOnlyMixin(83-104)OrganizationLimitsInline(1370-1374)openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(16-118)check_estimated_location_enabled(21-45)openwisp_controller/geo/models.py (4)
DeviceLocation(23-26)FloorPlan(17-20)Location(11-14)OrganizationGeoSettings(29-32)openwisp_controller/geo/base/models.py (1)
organization_id(160-161)
openwisp_controller/geo/estimated_location/tests/tests.py (5)
openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(11-36)openwisp_controller/geo/estimated_location/service.py (3)
EstimatedLocationService(16-118)get_cache_key(48-50)check_estimated_location_enabled(21-45)openwisp_controller/geo/models.py (2)
OrganizationGeoSettings(29-32)Location(11-14)openwisp_controller/geo/estimated_location/tests/utils.py (1)
TestEstimatedLocationMixin(12-40)openwisp_controller/geo/base/models.py (2)
save(73-110)device(156-157)
openwisp_controller/geo/estimated_location/service.py (6)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)openwisp_controller/geo/base/models.py (3)
device(156-157)organization_id(160-161)save(73-110)openwisp_controller/geo/models.py (3)
OrganizationGeoSettings(29-32)Location(11-14)DeviceLocation(23-26)openwisp_controller/config/whois/service.py (1)
get_cache_key(42-46)openwisp_controller/config/base/whois.py (1)
invalidate_org_settings_cache(111-117)openwisp_controller/config/base/config.py (1)
full_clean(558-562)
openwisp_controller/geo/settings.py (1)
openwisp_controller/settings.py (1)
get_setting(4-5)
| transaction.on_commit( | ||
| lambda: whois_fetched.send( | ||
| sender=WHOISInfo, | ||
| whois=whois_obj, | ||
| updated_fields=update_fields, | ||
| device=device, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the target file
fd -t f "tasks.py" | grep whoisRepository: openwisp/openwisp-controller
Length of output: 113
🏁 Script executed:
# Check the specific lines in question
if [ -f "openwisp_controller/config/whois/tasks.py" ]; then
wc -l openwisp_controller/config/whois/tasks.py
fiRepository: openwisp/openwisp-controller
Length of output: 117
🏁 Script executed:
# Read the relevant section with context
if [ -f "openwisp_controller/config/whois/tasks.py" ]; then
sed -n '75,110p' openwisp_controller/config/whois/tasks.py
fiRepository: openwisp/openwisp-controller
Length of output: 1548
🏁 Script executed:
# Check the imports at the top of the file
if [ -f "openwisp_controller/config/whois/tasks.py" ]; then
head -30 openwisp_controller/config/whois/tasks.py
fiRepository: openwisp/openwisp-controller
Length of output: 976
🏁 Script executed:
# Search for whois_fetched signal definition
rg "whois_fetched\s*=" --type py -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 411
🏁 Script executed:
# Search for receivers registered to whois_fetched
rg "whois_fetched\s*\.\s*(connect|disconnect)" --type py -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 500
🏁 Script executed:
# Find and examine the whois_fetched_handler
rg "whois_fetched_handler" --type py -B 5 -A 20Repository: openwisp/openwisp-controller
Length of output: 5996
🏁 Script executed:
# Also check if there are other receivers connected to whois_fetched
rg "whois_fetched\.connect\(" --type py -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 500
🌐 Web query:
Django Signal send() vs send_robust() exception handling behavior
💡 Result:
-
Signal.send(): does not catch exceptions from receivers. If any receiver raises, the exception propagates and the dispatch loop stops (so later receivers may not run). Returns[(receiver, response), ...]only if no receiver raises. [1], [2] -
Signal.send_robust(): wraps each receiver call intry/except Exception. If a receiver raises a subclass ofException, Django continues to call remaining receivers and returns the exception object in that receiver’s slot:[(receiver, response_or_exception), ...]. In newer Django versions it also logs the error. It does not catchBaseExceptionsubclasses likeKeyboardInterrupt/SystemExit. [1], [2], [3]
Sources:
[1] Django “Signals” docs (exception-handling difference) (docs.djangoproject.com)
[2] Django module source showing send() behavior (propagate/stop) (docs.djangoproject.com)
[3] Django module source showing send_robust() behavior (catch/log/continue) (docs.djangoproject.com)
Citations:
- 1: https://docs.djangoproject.com/en/2.0/topics/signals/?utm_source=openai
- 2: https://docs.djangoproject.com/en/2.1/_modules/django/dispatch/dispatcher/
- 3: https://docs.djangoproject.com/en/3.2/_modules/django/dispatch/dispatcher/
Use robust signal dispatch after commit.
Signal.send() will re-raise receiver exceptions after the WHOIS row has already been committed, so a buggy receiver can mark this task as failed and drive the generic WHOIS error path even though the lookup succeeded. Switch this callback to send_robust() and log any returned exceptions instead of letting them unwind the task.
🔧 Suggested hardening
- transaction.on_commit(
- lambda: whois_fetched.send(
- sender=WHOISInfo,
- whois=whois_obj,
- updated_fields=update_fields,
- device=device,
- )
- )
+ def _emit_whois_fetched():
+ for receiver, response in whois_fetched.send_robust(
+ sender=WHOISInfo,
+ whois=whois_obj,
+ updated_fields=update_fields,
+ device=device,
+ ):
+ if isinstance(response, Exception):
+ logger.error(
+ "whois_fetched receiver %r failed for %s: %s",
+ receiver,
+ new_ip_address,
+ response,
+ )
+
+ transaction.on_commit(_emit_whois_fetched)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/whois/tasks.py` around lines 89 - 96, The
on_commit lambda currently calls whois_fetched.send which re-raises receiver
exceptions; change it to call whois_fetched.send_robust with the same kwargs
(sender=WHOISInfo, whois=whois_obj, updated_fields=update_fields, device=device)
and iterate over the returned (receiver, result) tuples to log any exceptions
(result instances of Exception) via the existing logger instead of letting them
propagate; update the transaction.on_commit callback that currently references
whois_fetched.send to use send_robust and explicit exception-logging.
There was a problem hiding this comment.
@coderabbitai are you hallucinating? Can you re-review this comment and explain the problem properly?
There was a problem hiding this comment.
@pandafy, I re-reviewed the comment carefully. The concern is technically valid, not a hallucination — let me explain it more clearly.
Here is the exact failure path:
- The WHOIS data is successfully written to the DB inside the
with transaction.atomic():block. - After the transaction commits,
transaction.on_commitfires the lambda that callswhois_fetched.send(...). Signal.send()does not catch exceptions from receivers. If any receiver raises, the exception propagates upward into the Celery task execution context (sinceon_commitruns synchronously within the task after the atomic block exits).- This causes
fetch_whois_detailsto be treated as a failed task, which triggersWHOISCeleryRetryTask.on_failure(). on_failure()then sends awhois_device_errornotification — a false positive alert, because the WHOIS fetch itself actually succeeded.
The current receiver (whois_fetched_handler) is simple and unlikely to raise in normal operation. But the concern is about defensive correctness: as more receivers are potentially connected to whois_fetched in the future, any one of them failing would produce a misleading failure notification for a task that completed successfully.
That said, I acknowledge this is a relatively low-risk concern given the current receiver implementation. If you feel this is acceptable for now, we could defer it or track it as a follow-up. Would you like me to open an issue for it?
🧠 Learnings used
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.
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.
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py
Outdated
Show resolved
Hide resolved
Test Failures DetectedHello @pandafy,
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/whois/service.py (1)
215-224:⚠️ Potential issue | 🟠 MajorEmit
whois_lookup_skippedonly after the save transaction commits.The fetch branch right above correctly uses
transaction.on_commit(...)to defer the celery task, but this branch sends the signal synchronously.openwisp_controller/geo/estimated_location/handlers.pyenqueues the estimated-location task immediately from that receiver, creating a race where the handler executes before the transaction fully commits. The pattern already exists inopenwisp_controller/config/whois/tasks.py, wherewhois_fetchedis deferred withtransaction.on_commit()for the same reason.Suggested fix
- elif self.is_whois_enabled and self.is_valid_public_ip_address(new_ip): - # Emit signal when lookup is skipped so receivers can react - whois_lookup_skipped.send(sender=self.__class__, device=self.device) + elif self.is_whois_enabled and self.is_valid_public_ip_address(new_ip): + transaction.on_commit( + lambda: whois_lookup_skipped.send( + sender=self.__class__, + device=self.device, + ) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/service.py` around lines 215 - 224, The whois_lookup_skipped signal is being sent synchronously in the elif branch (conditions: self.is_whois_enabled and self.is_valid_public_ip_address(new_ip)), which can race with DB commit; wrap the signal send in transaction.on_commit so it fires only after commit (mirror the fetch branch). Specifically, replace the direct whois_lookup_skipped.send(sender=self.__class__, device=self.device) call with a transaction.on_commit(lambda: whois_lookup_skipped.send(sender=self.__class__, device=self.device)) so the device and sender are captured and the signal is deferred until after the save transaction completes.
♻️ Duplicate comments (1)
openwisp_controller/geo/tests/test_api.py (1)
1647-1659:⚠️ Potential issue | 🟡 MinorThis subtest never reaches the intended 403 branch.
urlstill points tofake_uuid, so both requests stay on the same 404 path covered above. The test also grantschangepermission on Lines 1620-1622, so it does not represent "without change permission" either. Remove this block or move it to a real-organization URL with a user lackingchange.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/tests/test_api.py` around lines 1647 - 1659, The failing subtest uses the same fake_uuid url and also mistakenly grants change permission earlier, so it never exercises the "403 without change" branch; update the test by either removing this subTest block or changing it to request a real-organization URL (replace the fake_uuid url with the actual org resource URL used elsewhere in the test) and ensure the test user does NOT have the 'change' permission (remove the earlier grant of change permission or explicitly revoke it) before making the PUT/PATCH calls to assert 403; look for the variables/methods named url, the subTest block text "Cannot update without change permission", and the permission-granting lines that give 'change' to adjust accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/geo/settings.py`:
- Around line 13-16: The error text is misleading: the check uses
WHOIS_CONFIGURED but the message points to OPENWISP_CONTROLLER_WHOIS_ENABLED;
update the ImproperlyConfigured message (the raise in the block that validates
OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED) to reference the actual
requirement (WHOIS_CONFIGURED) and/or list the required credentials
(WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY) so users know they must configure
those values rather than merely toggling OPENWISP_CONTROLLER_WHOIS_ENABLED.
In `@openwisp_controller/geo/tests/test_api.py`:
- Around line 1511-1521: The assertion after the failing PUT is wrong: instead
of asserting org1_geo_settings.estimated_location_enabled is True, refresh the
org1_geo_settings (call org1_geo_settings.refresh_from_db()) and assert it
remains the previous value (False) to confirm the failed update left the row
unchanged; update the assertion referencing org1_geo_settings and its
estimated_location_enabled attribute accordingly.
---
Outside diff comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 215-224: The whois_lookup_skipped signal is being sent
synchronously in the elif branch (conditions: self.is_whois_enabled and
self.is_valid_public_ip_address(new_ip)), which can race with DB commit; wrap
the signal send in transaction.on_commit so it fires only after commit (mirror
the fetch branch). Specifically, replace the direct
whois_lookup_skipped.send(sender=self.__class__, device=self.device) call with a
transaction.on_commit(lambda: whois_lookup_skipped.send(sender=self.__class__,
device=self.device)) so the device and sender are captured and the signal is
deferred until after the save transaction completes.
---
Duplicate comments:
In `@openwisp_controller/geo/tests/test_api.py`:
- Around line 1647-1659: The failing subtest uses the same fake_uuid url and
also mistakenly grants change permission earlier, so it never exercises the "403
without change" branch; update the test by either removing this subTest block or
changing it to request a real-organization URL (replace the fake_uuid url with
the actual org resource URL used elsewhere in the test) and ensure the test user
does NOT have the 'change' permission (remove the earlier grant of change
permission or explicitly revoke it) before making the PUT/PATCH calls to assert
403; look for the variables/methods named url, the subTest block text "Cannot
update without change permission", and the permission-granting lines that give
'change' to adjust accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 136103a7-cea4-4cbb-9c8b-b9cc6c4c6878
📒 Files selected for processing (15)
docs/developer/utils.rstdocs/user/rest-api.rstopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_geo/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- 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
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/estimated_location/tests/utils.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/estimated_location/tests/utils.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/estimated_location/tests/utils.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/estimated_location/tests/utils.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧠 Learnings (9)
📓 Common learnings
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.
📚 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/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/estimated_location/tests/utils.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.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:
tests/openwisp2/sample_geo/views.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp} : Avoid unnecessary comments or docstrings for code that is already clear
Applied to files:
openwisp_controller/geo/settings.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
🧬 Code graph analysis (12)
openwisp_controller/geo/estimated_location/tests/utils.py (4)
tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/config/whois/tests/utils.py (2)
CreateWHOISMixin(14-40)setUp(36-40)openwisp_controller/geo/base/models.py (1)
save(73-110)
tests/openwisp2/sample_geo/views.py (1)
openwisp_controller/geo/api/views.py (1)
OrganizationGeoSettingsView(349-358)
openwisp_controller/config/whois/tests/tests.py (5)
openwisp_controller/config/tests/test_selenium.py (1)
SeleniumTestMixin(26-50)openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (1)
_mocked_client_response(45-57)openwisp_controller/config/whois/tasks.py (1)
fetch_whois_details(62-101)openwisp_controller/config/whois/service.py (1)
process_ip_data_and_location(208-224)
openwisp_controller/geo/estimated_location/handlers.py (2)
openwisp_controller/geo/estimated_location/service.py (3)
EstimatedLocationService(16-127)check_estimated_location_enabled(21-47)trigger_estimated_location_task(66-78)openwisp_controller/geo/base/models.py (2)
device(156-157)organization_id(160-161)
openwisp_controller/geo/apps.py (5)
openwisp_controller/geo/estimated_location/handlers.py (3)
register_estimated_location_notification_types(11-36)whois_fetched_handler(39-62)whois_lookup_skipped_handler(65-75)openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(16-127)invalidate_org_settings_cache(55-60)openwisp_controller/config/apps.py (3)
ready(45-54)register_dashboard_charts(342-458)register_menu_groups(163-199)openwisp_controller/geo/base/models.py (1)
organization_post_save_receiver(198-204)openwisp_controller/config/base/whois.py (1)
invalidate_org_settings_cache(111-117)
openwisp_controller/geo/settings.py (1)
openwisp_controller/settings.py (1)
get_setting(4-5)
openwisp_controller/geo/admin.py (4)
openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(16-127)check_estimated_location_enabled(21-47)tests/openwisp2/sample_geo/models.py (4)
DeviceLocation(28-30)FloorPlan(23-25)Location(18-20)OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (4)
DeviceLocation(23-26)FloorPlan(17-20)Location(11-14)OrganizationGeoSettings(29-32)openwisp_controller/geo/base/models.py (1)
organization_id(160-161)
openwisp_controller/config/whois/service.py (2)
openwisp_controller/config/whois/tasks.py (1)
fetch_whois_details(62-101)openwisp_controller/geo/base/models.py (1)
device(156-157)
openwisp_controller/geo/tests/test_api.py (3)
tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)tests/openwisp2/sample_users/models.py (1)
OrganizationUser(39-43)
openwisp_controller/geo/estimated_location/service.py (3)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)openwisp_controller/geo/base/models.py (3)
device(156-157)organization_id(160-161)save(73-110)openwisp_controller/geo/models.py (3)
OrganizationGeoSettings(29-32)Location(11-14)DeviceLocation(23-26)
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (7)
openwisp_controller/geo/migrations/__init__.py (1)
assign_geo_settings_permissions_to_groups(36-39)openwisp_controller/migrations.py (1)
get_swapped_model(13-16)tests/openwisp2/sample_users/models.py (1)
Organization(32-36)tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(6-20)openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
Migration(12-62)
openwisp_controller/geo/estimated_location/tests/tests.py (5)
openwisp_controller/geo/estimated_location/service.py (3)
EstimatedLocationService(16-127)get_cache_key(50-52)check_estimated_location_enabled(21-47)tests/openwisp2/sample_geo/models.py (2)
OrganizationGeoSettings(33-35)Location(18-20)openwisp_controller/geo/models.py (2)
OrganizationGeoSettings(29-32)Location(11-14)openwisp_controller/geo/estimated_location/tests/utils.py (1)
TestEstimatedLocationMixin(12-41)openwisp_controller/geo/base/models.py (2)
save(73-110)device(156-157)
🔇 Additional comments (7)
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (1)
1-63: LGTM!The migration correctly addresses previous review feedback:
- Uses
get_swapped_model()for resolving swappableOrganizationConfigSettingsandOrganizationGeoSettingsmodels- Uses
iterator()for memory-efficient iteration over large datasets- Properly handles the data copy with
getattr()fallback for field safetyopenwisp_controller/geo/estimated_location/tests/utils.py (1)
16-26: LGTM!The test setup correctly:
- Uses
get_or_createpattern to defensively ensureOrganizationGeoSettingsexists (addressing the previous review concern aboutRelatedObjectDoesNotExist)- Separates WHOIS enablement (
OrganizationConfigSettings) from estimated location enablement (OrganizationGeoSettings), aligning with the PR's refactoring goaltests/openwisp2/sample_geo/views.py (1)
28-30: LGTM!The new
OrganizationGeoSettingsViewfollows the established pattern in this file for swappable API views—importing the base, creating an empty subclass for extensibility, and registering theas_view()callable.Also applies to: 69-72, 82-82
docs/developer/utils.rst (1)
382-409: LGTM!The signal documentation follows the established pattern in this file and clearly describes:
- The signal paths
- The arguments provided
- When each signal is emitted
This aligns with the signal definitions in
openwisp_controller/config/signals.py.openwisp_controller/config/signals.py (1)
68-75: LGTM!The new signals follow the established pattern in this module with proper
__doc__strings documenting the provided arguments.docs/user/rest-api.rst (1)
804-850: LGTM!The API documentation for Organization Geographic Settings is well-structured and follows the established patterns in this file. It properly documents:
- GET endpoint for retrieving settings
- PUT endpoint for full updates with a curl example
- PATCH endpoint for partial updates with a curl example (addressing previous review feedback about separating methods)
openwisp_controller/config/whois/tests/tests.py (1)
454-473: LGTM!The new signal emission tests are well-structured:
test_fetch_whois_emits_signalverifieswhois_fetchedis emitted after successful WHOIS lookuptest_whois_lookup_skipped_emits_signalverifieswhois_lookup_skippedis emitted when lookup conditions aren't met (existing WHOISInfo for the IP)Both tests properly use
catch_signaland follow the established testing patterns in this file.
Code Style Failures in CIHello @pandafy, There are several code style and formatting issues detected in the CI run:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 111-115: This subtest relies on WHOIS being enabled but never
ensures it; before calling geo_settings.full_clean() in the "Estimated location
can be enabled when WHOIS is configured" subTest, explicitly enable the WHOIS
flag (e.g., set config_app_settings.WHOIS_CONFIGURED = True or use
override/monkeypatch) so the validation path for
geo_settings.estimated_location_enabled passes; ensure you restore/reset the
flag after the subtest if needed.
- Around line 266-269: The patched cache.get in the second and third subtests is
left as a truthy MagicMock so the cache-hit branch runs; change the mocks for
"django.core.cache.cache.get" to return concrete values (e.g., return_value=None
for explicit misses, or return_value=<actual settings object with
.estimated_location_enabled set appropriately> for cache hits) and keep
cache.set mocked as before so the tests exercise the DB-fallback and
cached-settings paths; locate the patches around the context manager that mocks
"django.core.cache.cache.get" / "django.core.cache.cache.set" and instantiate or
reuse the real settings object used elsewhere in the test to supply to
mocked_get.return_value so .estimated_location_enabled is read from a real
object rather than a MagicMock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a39e01e-7619-4eb5-91d2-ef92b8a78666
📒 Files selected for processing (4)
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/tests/utils.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/tests/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/tests/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/tests/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/tests/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
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.
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/tests/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/tests/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/tests/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp} : Avoid unnecessary comments or docstrings for code that is already clear
Applied to files:
openwisp_controller/geo/settings.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
🧬 Code graph analysis (3)
openwisp_controller/geo/settings.py (1)
openwisp_controller/settings.py (1)
get_setting(4-5)
openwisp_controller/geo/tests/test_api.py (4)
tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)tests/openwisp2/sample_users/models.py (1)
OrganizationUser(39-43)openwisp_controller/geo/api/serializers.py (2)
create(188-207)create(345-356)
openwisp_controller/geo/estimated_location/tests/tests.py (4)
openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(11-36)openwisp_controller/geo/estimated_location/service.py (3)
EstimatedLocationService(16-127)get_cache_key(50-52)check_estimated_location_enabled(21-47)tests/openwisp2/sample_geo/models.py (2)
OrganizationGeoSettings(33-35)Location(18-20)openwisp_controller/geo/models.py (2)
OrganizationGeoSettings(29-32)Location(11-14)
🔇 Additional comments (9)
openwisp_controller/geo/settings.py (1)
8-14: UseWHOIS_CONFIGUREDinstead ofWHOIS_ENABLEDand remove the redundant comment.Two issues persist from prior review feedback:
Line 8: The comment restates what the code already expresses clearly. Remove it per coding guidelines.
Lines 10-13: Checking
WHOIS_ENABLEDis insufficient. If credentials (WHOIS_GEOIP_ACCOUNT/WHOIS_GEOIP_KEY) are missing, this validation passes at startup but the feature fails at runtime. UseWHOIS_CONFIGUREDinstead, which is the established gate throughout the codebase (e.g.,geo/apps.py,geo/base/models.py).🛠️ Proposed fix
ESTIMATED_LOCATION_ENABLED = get_setting("ESTIMATED_LOCATION_ENABLED", False) -# Validate that WHOIS is enabled if estimated location is enabled if ESTIMATED_LOCATION_ENABLED: - if not config_settings.WHOIS_ENABLED: + if not config_settings.WHOIS_CONFIGURED: raise ImproperlyConfigured( - "OPENWISP_CONTROLLER_WHOIS_ENABLED must be set to True before " - "setting OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED to True." + "WHOIS must be configured (OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT " + "and OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY must be set) before " + "enabling OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED." )openwisp_controller/geo/tests/test_api.py (7)
1615-1669: LGTM!The non-existent organization tests properly return 404 for unknown UUIDs. The "Cannot update without change permission" subtest correctly addresses the previous review concern by:
- Revoking the
changepermission- Using a real organization URL (
real_url)- Asserting 403 (Forbidden) as expected
7-7: LGTM!The new imports are necessary for the
OrganizationGeoSettingsAPI tests -ContentTypefor the permission helper andOrganizationGeoSettingsmodel for the endpoint tests.Also applies to: 31-31
1359-1366: LGTM!The permission helper is well-structured and correctly uses Django's ContentType API to dynamically grant model-level permissions for test assertions.
1368-1450: LGTM!Comprehensive test coverage for the retrieve endpoint, properly testing authentication (401), authorization (403), multi-tenancy isolation (404), and successful retrieval scenarios.
1451-1543: LGTM!The update tests are comprehensive and the validation error test correctly asserts that the setting remains
False(the previous state) after the failed PUT request. Good coverage of PUT, PATCH, payload sanitization, multi-tenancy, validation, and superuser scenarios.
1545-1582: LGTM!Good multi-tenancy isolation tests ensuring users can only access organizations they manage.
1584-1613: LGTM!Properly tests the access level hierarchy: regular members are denied, org admins with model permissions succeed, and superusers bypass permission checks.
openwisp_controller/geo/tests/utils.py (1)
22-25: The_get_org()method is available and correctly inherited fromTestLociMixin.The method is provided by the
django_loci.tests.TestLociMixinparent class and is consistently used throughout the codebase in hundreds of test methods across the project. No action needed—the implementation is correct.
Code Style and Test FailuresHello @pandafy, There are multiple issues in your commit:
|
Flake8 and Test FailuresHello @pandafy,
These test failures indicate issues with how the application handles responses, status codes, permissions, or signal emissions in various scenarios. Review the test logic and the corresponding code to identify the root cause of these assertions failing. Specifically, check the expected return values, status codes, and signal calls in the failing tests. |
5f34d78 to
0a08bb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/geo/estimated_location/tests/tests.py (1)
267-270:⚠️ Potential issue | 🟡 MinorReturn real cache values in these two subtests.
mock.patch("django.core.cache.cache.get")without areturn_valuegives you a truthyMagicMock, so both flows take the cache-hit path and read.whois_enabled/.estimated_location_enabledfrom mocks. These assertions can pass without exercising either the DB fallback or the real cached-settings behavior.♻️ Suggested fix
with self.subTest( "Estimated location task called when last_ip is changed and is public" ): with ( - mock.patch("django.core.cache.cache.get") as mocked_get, + mock.patch( + "django.core.cache.cache.get", + side_effect=lambda key, default=None: { + f"organization_config_{org.pk}": org.config_settings, + f"organization_geo_{org.pk}": org.geo_settings, + }.get(key, default), + ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set, ): device.last_ip = "172.217.22.10" device.save() @@ with self.subTest( "Estimated location task called when last_ip has related WhoIsInfo" ): with ( - mock.patch("django.core.cache.cache.get") as mocked_get, + mock.patch( + "django.core.cache.cache.get", + side_effect=lambda key, default=None: { + f"organization_config_{org.pk}": org.config_settings, + f"organization_geo_{org.pk}": org.geo_settings, + }.get(key, default), + ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set, ):Also applies to: 287-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/estimated_location/tests/tests.py` around lines 267 - 270, The patch of django.core.cache.cache.get in the two subtests currently returns a MagicMock (truthy) and forces the cache-hit branch; update the mock.patch calls for "django.core.cache.cache.get" to provide an explicit return_value (or side_effect) representing the real cached value you expect in each subtest (e.g., None to force DB-fallback or a dict/boolean matching the cached-settings shape like {'whois_enabled': True} or False), keep mocking "django.core.cache.cache.set" as needed and assert set/get behavior accordingly so the test actually exercises both cache-miss (DB fallback) and real cached-settings paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 46-49: The migration's dependencies list is missing the config
migration that introduces OrganizationConfigSettings.estimated_location_enabled,
which copy_estimated_location_enabled() reads; update the dependencies variable
to include ("config",
"0063_organizationconfigsettings_estimated_location_enabled_and_more") so the
historical model state during RunPython has the source field available and the
migration runs reliably.
---
Duplicate comments:
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 267-270: The patch of django.core.cache.cache.get in the two
subtests currently returns a MagicMock (truthy) and forces the cache-hit branch;
update the mock.patch calls for "django.core.cache.cache.get" to provide an
explicit return_value (or side_effect) representing the real cached value you
expect in each subtest (e.g., None to force DB-fallback or a dict/boolean
matching the cached-settings shape like {'whois_enabled': True} or False), keep
mocking "django.core.cache.cache.set" as needed and assert set/get behavior
accordingly so the test actually exercises both cache-miss (DB fallback) and
real cached-settings paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93ccf516-7f34-4950-bc2e-23f5c3b9f0a4
📒 Files selected for processing (6)
openwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/tests/mixins.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | 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.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧠 Learnings (7)
📚 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/tests/mixins.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧬 Code graph analysis (5)
openwisp_controller/geo/estimated_location/tests/utils.py (4)
tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/config/whois/tests/utils.py (2)
CreateWHOISMixin(14-40)setUp(36-40)openwisp_controller/geo/base/models.py (1)
save(73-110)
openwisp_controller/geo/apps.py (4)
openwisp_controller/geo/estimated_location/handlers.py (3)
register_estimated_location_notification_types(11-36)whois_fetched_handler(39-62)whois_lookup_skipped_handler(65-75)openwisp_controller/geo/estimated_location/service.py (2)
EstimatedLocationService(14-125)invalidate_org_settings_cache(53-58)openwisp_controller/geo/base/models.py (1)
organization_post_save_receiver(198-204)openwisp_controller/config/base/whois.py (1)
invalidate_org_settings_cache(111-117)
openwisp_controller/geo/estimated_location/service.py (5)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(38-51)openwisp_controller/geo/base/models.py (3)
device(156-157)organization_id(160-161)save(73-110)openwisp_controller/geo/models.py (3)
OrganizationGeoSettings(29-32)Location(11-14)DeviceLocation(23-26)openwisp_controller/config/whois/service.py (1)
get_cache_key(43-47)openwisp_controller/config/base/whois.py (1)
invalidate_org_settings_cache(111-117)
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (6)
openwisp_controller/migrations.py (1)
get_swapped_model(13-16)openwisp_controller/geo/migrations/__init__.py (1)
assign_geo_settings_permissions_to_groups(36-39)tests/openwisp2/sample_geo/models.py (1)
OrganizationGeoSettings(33-35)openwisp_controller/geo/models.py (1)
OrganizationGeoSettings(29-32)openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
Migration(6-20)openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
Migration(12-62)
openwisp_controller/geo/estimated_location/tests/tests.py (6)
openwisp_controller/geo/tests/utils.py (1)
TestGeoMixin(9-68)openwisp_controller/geo/estimated_location/handlers.py (1)
register_estimated_location_notification_types(11-36)openwisp_controller/geo/estimated_location/service.py (3)
EstimatedLocationService(14-125)get_cache_key(48-50)check_estimated_location_enabled(19-45)openwisp_controller/geo/models.py (2)
OrganizationGeoSettings(29-32)Location(11-14)openwisp_controller/geo/estimated_location/tests/utils.py (1)
TestEstimatedLocationMixin(12-43)openwisp_controller/tests/utils.py (1)
TestAdminMixin(9-18)
🔇 Additional comments (1)
openwisp_controller/tests/mixins.py (1)
14-18: LGTM!The new
geo_settingsinline formset parameters follow the established pattern used by other inlines (config_settings,config_limits,notification_settings), and the comment maintains consistency with the existing code style. This correctly supports testing organization edit forms that include the newGeoSettingsInline.
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py
Show resolved
Hide resolved
0a08bb1 to
9c36fab
Compare
Migrations Check FailedHello @pandafy, The CI build failed because of a migrations check failure. This indicates that there are unapplied model changes that need to be migrated. Fix: |
9c36fab to
249f28a
Compare
Test Failures in OpenWISP Controller CIHello @pandafy, There are multiple test failures in the CI run. 1. Test Failures:
Fix: Ensure all datetime objects used in tests and application logic are timezone-aware when timezone support is enabled in Django settings.
Fix: Ensure that the necessary floorplan image files are present in the expected locations within the test setup, or correct the paths in the tests if they are incorrect.
These errors indicate issues with how organizations, UUIDs, and authorization are handled during the template cloning process in the tests. Fix: Review the tests for template cloning to ensure that valid organization IDs and UUIDs are used, and that authorization checks are correctly implemented and passed.
Fix:
2. Potential Infrastructure/Transient Issues:
Fix: This is likely a transient issue or related to the other test failures. Once the primary test failures are resolved, this warning may disappear. If it persists, investigate the coverage configuration. Summary: The primary issues appear to be related to datetime handling, missing test assets (floorplan images), and errors in the template cloning logic, which may be cascading into other tests. Addressing these specific areas should help resolve the majority of the failures. |
Dependency Installation Script ErrorHello @pandafy, The CI failed due to an error in the dependency installation script, specifically an "unexpected EOF while looking for matching Fix: |
a70f49d to
df49464
Compare
Multiple Test Failures DetectedHello @pandafy, There are multiple test failures in the CI logs. 1. Runtime Warnings (Naive Datetimes):
2. Test Failures (Marked with 'E' or 'F'):
3. Floorplan Image Not Found Errors:
4. Slow Tests:
5. Coverage Warning:
To address these issues, please:
|
df49464 to
9f4b5f3
Compare
1d60aa4 to
8d38dee
Compare
8d38dee to
60245d9
Compare
|
@coderabbitai review! |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/whois/service.py (1)
210-224:⚠️ Potential issue | 🟠 MajorKeep the skipped-lookup path on the same post-commit boundary.
The method docstring (line 211) states side effects happen on commit, but line 224 sends
whois_lookup_skippedsynchronously. In this PR that signal drives estimated-location enqueueing (viawhois_lookup_skipped_handler), so a rollback can still publish work for alast_ipchange that never commits. Defer the signal emission withtransaction.on_commit(...)to match the pattern already used for the lookup branch (lines 218–221).♻️ Proposed fix
- elif self.is_whois_enabled and self.is_valid_public_ip_address(new_ip): - # Emit signal when lookup is skipped so receivers can react - whois_lookup_skipped.send(sender=self.__class__, device=self.device) + elif self.is_whois_enabled and self.is_valid_public_ip_address(new_ip): + transaction.on_commit( + lambda: whois_lookup_skipped.send( + sender=self.__class__, device=self.device + ) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/service.py` around lines 210 - 224, The whois_lookup_skipped signal is being sent synchronously instead of on the commit boundary, so wrap the emission in transaction.on_commit to match the fetch_whois_details path: replace the direct call to whois_lookup_skipped.send(sender=self.__class__, device=self.device) with transaction.on_commit(lambda: whois_lookup_skipped.send(sender=self.__class__, device=self.device)) so the skipped-lookup path (in the same method in service.py that uses self.device, new_ip, initial_ip) only fires after the transaction commits (this keeps whois_lookup_skipped_handler behavior consistent with fetch_whois_details.delay).
♻️ Duplicate comments (2)
openwisp_controller/geo/settings.py (1)
8-14:⚠️ Potential issue | 🟠 MajorReplace
WHOIS_ENABLEDwithWHOIS_CONFIGUREDand update the error message.The validation checks
WHOIS_ENABLED, but this only verifies the flag is set toTrue—it doesn't ensure WHOIS credentials (WHOIS_GEOIP_ACCOUNTandWHOIS_GEOIP_KEY) are configured. If a user enables estimated location withWHOIS_ENABLED=Truebut missing credentials, startup validation passes but the feature fails at runtime.The codebase consistently uses
WHOIS_CONFIGUREDas the proper gate (e.g.,geo/apps.py:66,geo/base/models.py:186). Additionally, line 8's comment is redundant as the code is self-explanatory.🛠️ Proposed fix
-# Validate that WHOIS is enabled if estimated location is enabled if ESTIMATED_LOCATION_ENABLED: - if not config_settings.WHOIS_ENABLED: + if not config_settings.WHOIS_CONFIGURED: raise ImproperlyConfigured( - "OPENWISP_CONTROLLER_WHOIS_ENABLED must be set to True before " - "setting OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED to True." + "WHOIS must be configured (OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT " + "and OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY must be set) before " + "enabling OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/settings.py` around lines 8 - 14, Replace the check against config_settings.WHOIS_ENABLED with config_settings.WHOIS_CONFIGURED in the ESTIMATED_LOCATION_ENABLED validation and update the Raised ImproperlyConfigured message to clearly state that WHOIS credentials (WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY) must be set (e.g., "OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT and OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY must be configured when OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED=True"); also remove the now-redundant comment above the block. Ensure you update the conditional that reads ESTIMATED_LOCATION_ENABLED -> if not config_settings.WHOIS_CONFIGURED and the error string mentions the credential names.openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (1)
46-49:⚠️ Potential issue | 🟠 MajorAdd the missing
config.0063dependency.
copy_estimated_location_enabled()readsOrganizationConfigSettings.estimated_location_enabled, but there is still no direct dependency onconfig.0063_organizationconfigsettings_estimated_location_enabled_and_more.config.0064depending on both migrations is not enough here; Django can still schedule this migration beforeconfig.0063, and the currentgetattr(..., None)fallback would silently skip the copy.♻️ Proposed fix
class Migration(migrations.Migration): dependencies = [ + ( + "config", + "0063_organizationconfigsettings_estimated_location_enabled_and_more", + ), ("geo", "0005_organizationgeosettings"), ("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"), ]In Django migrations, if migration B reads a field introduced by app A migration 0063, does B need an explicit dependency on A.0063 even when a later migration in app A depends on both B and A.0063?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py` around lines 46 - 49, The migration's dependencies list is missing the config.0063 migration that introduces OrganizationConfigSettings.estimated_location_enabled, so update the dependencies array in 0006_create_geo_settings_for_existing_orgs.py to include ("config", "0063_organizationconfigsettings_estimated_location_enabled_and_more"); this ensures copy_estimated_location_enabled() can safely read OrganizationConfigSettings.estimated_location_enabled instead of silently falling back via getattr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/tasks.py`:
- Around line 89-101: When the function bails out because the existing WHOIS row
is fresh, ensure you still emit the whois_lookup_skipped signal and register the
old-IP cleanup via transaction.on_commit so the
delete_whois_record(initial_ip_address) runs; update the early-return branch to
call whois_lookup_skipped.send(sender=WHOISInfo, device=device, ...) and
register transaction.on_commit(lambda:
delete_whois_record(ip_address=initial_ip_address)) before returning. Keep the
existing post-commit whois_fetched path unchanged for the non-skip case.
In `@openwisp_controller/geo/admin.py`:
- Around line 229-232: Handle the potential ValueError when inserting
GeoSettingsInline into OrganizationAdmin.inlines by checking for
OrganizationLimitsInline first or using try/except: locate the insertion code
that references OrganizationAdmin.inlines, OrganizationLimitsInline and
GeoSettingsInline and either (a) test if OrganizationLimitsInline is in
OrganizationAdmin.inlines and compute the index before inserting, or (b) wrap
the insert in a try/except ValueError and fall back to appending
GeoSettingsInline to OrganizationAdmin.inlines while logging or silently
continuing as appropriate.
In `@openwisp_controller/geo/estimated_location/service.py`:
- Around line 78-125: Add a guard in _create_or_update_estimated_location to
re-check whether estimated locations are enabled for the device's organization
before performing any DB writes: call the same feature-check used elsewhere
(e.g., estimated_location_enabled or the org settings lookup) using the device's
organization or self.device to decide; if the feature is now disabled, return
the existing current_location immediately (no
create/update/send_whois_task_notification). Place this check at the start of
_create_or_update_estimated_location so the async dequeue path honors current
org settings and avoids persisting estimated locations when disabled.
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 14-18: The migration backfill currently uses default ORM queries;
update both backfill loops to use the active migration DB alias by calling
schema_editor.connection.alias (or the db_alias param) and applying
.using(db_alias) on the queryset and the manager calls: change
Organization.objects.iterator() to
Organization.objects.using(db_alias).iterator() and change
OrganizationGeoSettings.objects.get_or_create(...) to
OrganizationGeoSettings.objects.using(db_alias).get_or_create(...), and apply
the same .using(db_alias) pattern for the second backfill loop (the other
Organization/OrganizationGeoSettings backfill around lines 27-42) so reads and
writes target the migration connection.
---
Outside diff comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 210-224: The whois_lookup_skipped signal is being sent
synchronously instead of on the commit boundary, so wrap the emission in
transaction.on_commit to match the fetch_whois_details path: replace the direct
call to whois_lookup_skipped.send(sender=self.__class__, device=self.device)
with transaction.on_commit(lambda:
whois_lookup_skipped.send(sender=self.__class__, device=self.device)) so the
skipped-lookup path (in the same method in service.py that uses self.device,
new_ip, initial_ip) only fires after the transaction commits (this keeps
whois_lookup_skipped_handler behavior consistent with
fetch_whois_details.delay).
---
Duplicate comments:
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 46-49: The migration's dependencies list is missing the
config.0063 migration that introduces
OrganizationConfigSettings.estimated_location_enabled, so update the
dependencies array in 0006_create_geo_settings_for_existing_orgs.py to include
("config",
"0063_organizationconfigsettings_estimated_location_enabled_and_more"); this
ensures copy_estimated_location_enabled() can safely read
OrganizationConfigSettings.estimated_location_enabled instead of silently
falling back via getattr.
In `@openwisp_controller/geo/settings.py`:
- Around line 8-14: Replace the check against config_settings.WHOIS_ENABLED with
config_settings.WHOIS_CONFIGURED in the ESTIMATED_LOCATION_ENABLED validation
and update the Raised ImproperlyConfigured message to clearly state that WHOIS
credentials (WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY) must be set (e.g.,
"OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT and OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY
must be configured when OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED=True");
also remove the now-redundant comment above the block. Ensure you update the
conditional that reads ESTIMATED_LOCATION_ENABLED -> if not
config_settings.WHOIS_CONFIGURED and the error string mentions the credential
names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23a83637-8bf6-4efb-ba03-8bfaa280ee4d
📒 Files selected for processing (39)
docs/developer/extending.rstdocs/developer/utils.rstdocs/user/rest-api.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/utils.pyopenwisp_controller/settings.pyopenwisp_controller/tests/mixins.pytests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pytests/openwisp2/sample_geo/models.pytests/openwisp2/sample_geo/views.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (4)
- tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py
- openwisp_controller/config/base/multitenancy.py
- openwisp_controller/config/base/whois.py
- openwisp_controller/config/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | 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~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/settings.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/settings.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/settings.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/settings.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧠 Learnings (9)
📓 Common learnings
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.
📚 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/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pydocs/developer/extending.rsttests/openwisp2/sample_geo/models.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pytests/openwisp2/settings.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/settings.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/settings.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/tests/mixins.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/settings.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp} : Avoid unnecessary comments or docstrings for code that is already clear
Applied to files:
openwisp_controller/geo/settings.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
🔇 Additional comments (35)
openwisp_controller/config/signals.py (1)
68-75: Signal contracts are consistent and correctly scoped.
whois_fetchedandwhois_lookup_skippedare defined with payload docs aligned with current emitters/handlers, so this wiring looks solid.openwisp_controller/settings.py (1)
4-8: Good centralization of controller setting lookups.The helper keeps prefix handling consistent and reduces duplication at call sites.
openwisp_controller/config/settings.py (1)
6-8: Import refactor is clean and consistent.Using the shared
get_settingutility here is a good maintainability improvement.tests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.py (1)
20-59: Migration model definition looks correct for geo-settings separation.Field set and relation shape are appropriate for the new organization-level geo settings model.
openwisp_controller/geo/migrations/__init__.py (1)
6-39: Permission-assignment refactor is clear and reusable.The helper + dedicated geo-settings wrapper is a solid decomposition and keeps migration permission logic consistent.
openwisp_controller/tests/mixins.py (1)
14-18: Inline formset params update is appropriate.Adding
geo_settings-*management keys here is the right test-helper adjustment for the new organization geo inline.openwisp_controller/geo/utils.py (1)
49-53: New geo-settings route is well integrated.The endpoint is added cleanly with a clear URL name and organization-scoped path parameter.
tests/openwisp2/settings.py (1)
283-283: Swapper mapping addition is correct.This completes the sample-app model wiring for the new geo organization settings model.
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
7-13: Good cross-app ordering fix.This makes
config.0064wait forgeo.0006, so the legacy field is removed only after geo has created and backfilled the replacement settings.openwisp_controller/geo/estimated_location/tests/utils.py (1)
21-28: Nice fixture hardening.Using
get_or_createhere removes the hidden dependency on organization-creation signals, so these tests keep working with pre-existing organizations too.tests/openwisp2/sample_geo/models.py (1)
33-36: LGTM!The
OrganizationGeoSettingsmodel correctly follows the established testing pattern by including theDetailsModelmixin to verify swappable model extensibility. TheMeta.abstract = Falseproperly makes this a concrete model for the test app.openwisp_controller/geo/api/views.py (1)
349-359: LGTM!The
OrganizationGeoSettingsViewis well-implemented:
- Uses
ProtectedAPIMixinfor proper authentication and organization-scoped permissions.- The
get_object()method correctly retrieves settings byorganization_pkfrom URL kwargs.- The 404 error message is properly marked for translation.
openwisp_controller/config/admin.py (1)
1393-1396: LGTM!The removal of
"estimated_location_enabled"from the config settings inline fields correctly aligns with migration 0064 which removes this field fromOrganizationConfigSettings. The field is now managed via the newOrganizationGeoSettingsmodel in the geo app.openwisp_controller/geo/estimated_location/utils.py (1)
52-68: LGTM!The
get_location_defaults_from_whoishelper is well-structured and correctly extracts location defaults from a WHOISInfo object. The hardcodedtype="outdoor"andis_mobile=Falsevalues are appropriate for estimated locations derived from IP geolocation.docs/user/rest-api.rst (1)
804-850: LGTM!The documentation for the Organization Geographic Settings endpoints is complete and well-structured:
- Properly documents GET, PUT, and PATCH methods in separate sections.
- Includes clear curl examples with authorization headers.
- Follows the established documentation pattern used throughout this file.
openwisp_controller/config/whois/tests/tests.py (1)
454-472: LGTM!The new signal tests are well-structured:
test_fetch_whois_emits_signalproperly verifies thewhois_fetchedsignal is emitted during WHOIS processing.test_whois_lookup_skipped_emits_signalcorrectly tests thewhois_lookup_skippedsignal when lookup conditions aren't met.- Both tests use the
catch_signalcontext manager appropriately.openwisp_controller/geo/api/serializers.py (1)
28-44: LGTM!The
OrganizationGeoSettingsSerializeris well-implemented:
- Correctly marks
idandorganizationas read-only to prevent unauthorized modifications.- The
validate()workaround for openwisp-utils#633 is properly documented with a TODO for future cleanup.- Uses
copy(self.instance)to avoid mutating the original instance during validation.- Properly excludes M2M fields from direct assignment.
openwisp_controller/geo/models.py (1)
3-8: LGTM!The
OrganizationGeoSettingsmodel follows the established swappable model pattern used byLocation,FloorPlan, andDeviceLocation. The import and concrete model definition are correct.Also applies to: 29-32
openwisp_controller/geo/estimated_location/handlers.py (2)
39-62: LGTM!The
whois_fetched_handlerproperly guards against:
- Missing
updated_fields(no WHOIS data changed)- Missing
deviceargument- Estimated location being disabled for the organization
- Existing location when only non-location fields updated
The logic correctly avoids triggering estimated location tasks when unnecessary.
65-75: LGTM!The
whois_lookup_skipped_handlercorrectly triggers estimated location tasks when WHOIS lookup is skipped but estimated location is enabled. Per the signal emission inopenwisp_controller/config/whois/service.py:222-224, this signal is only sent whenis_valid_public_ip_address(new_ip)is true, sodevice.last_ipwill always be a valid public IP at this point.openwisp_controller/geo/apps.py (2)
48-74: LGTM!The
connect_receiversmethod properly wires up all necessary signal handlers with uniquedispatch_uidvalues to prevent duplicate connections:
- Organization post_save → creates OrganizationGeoSettings automatically
- OrganizationGeoSettings post_save/post_delete → invalidates cache
- WHOIS signals → trigger estimated location handlers
36-36:⚠️ Potential issue | 🟡 MinorRemove unused
whois_info_modelassignment.
self.whois_info_modelis loaded on line 36 but is not referenced anywhere in this file. Unlikeorg_geo_settings_modelandorganization_modelwhich are used inconnect_receivers(), this assignment serves no purpose and should be removed.⛔ Skipped due to 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.openwisp_controller/geo/admin.py (2)
130-132: LGTM!The
LocationAdminmethods correctly useEstimatedLocationService.check_estimated_location_enabled()instead of the oldWHOISServicemethod, aligning with the new geo-settings model for feature gating.Also applies to: 138-140, 147-149
212-226: LGTM!The
GeoSettingsInlinecorrectly:
- Sets
can_delete=Falseto prevent deletion of the singleton settings row- Sets
max_num=1to enforce the one-to-one relationship- Makes
estimated_location_enabledreadonly when WHOIS is not configuredopenwisp_controller/geo/tests/test_api.py (4)
1359-1366: LGTM!The
_add_model_permissionhelper cleanly handles permission assignment by resolvingContentTypeand generating proper codenames. This is a good reusable pattern for the geo settings permission tests.
1368-1449: LGTM!Comprehensive test coverage for the
organization_geo_settingsretrieve endpoint:
- Unauthenticated access (401)
- Access without permission (403)
- View permission grants access (200)
- Multi-tenancy isolation (404 for other orgs)
- Change permission required for updates (403)
- Superuser can access any organization
1451-1544: LGTM!The update tests thoroughly validate:
- PUT and PATCH operations work correctly
organizationfield is ignored on updates (cannot reassign)- Multi-tenancy prevents cross-org updates
- WHOIS_CONFIGURED validation returns 400 and doesn't persist invalid changes
- Superuser can update any organization's settings
1546-1670: LGTM!The remaining tests provide good coverage for:
- Multi-tenancy access control across multiple organizations
- User access levels (regular member vs org admin vs superuser)
- Non-existent organization handling (404)
- Permission-based access control for updates (403 without change permission)
openwisp_controller/geo/estimated_location/tests/tests.py (3)
96-116: LGTM!The
test_organization_geo_settings_validationtest correctly validates both scenarios:
- WHOIS not configured → ValidationError when enabling estimated location
- WHOIS configured → validation passes
The class-level
@mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True)ensures the happy path works correctly.
1113-1168: LGTM!The
TestEstimatedLocationCacheclass provides thorough coverage of the caching behavior:
- Cache miss triggers DB lookup and populates cache
- Cache hit returns cached value without DB query
- Save/delete on
OrganizationGeoSettingsinvalidates cache
356-359: LGTM!The
send_whois_task_notificationis now correctly patched in both modules where it's imported and called:
estimated_location.tasks(for task-level calls)estimated_location.service(for service-level calls)This addresses the past review comment about patching the wrong module.
openwisp_controller/geo/base/models.py (2)
60-62: LGTM!The
BaseLocationmethods correctly useEstimatedLocationService.check_estimated_location_enabled()for feature gating. Based on learnings, theelifbranch at line 103-105 intentionally protects theis_estimatedfield from manual changes when the feature is disabled.Also applies to: 88-90
164-203: LGTM!The
AbstractOrganizationGeoSettingsmodel is well-designed:
- OneToOneField with
related_name="geo_settings"enablesorg.geo_settingsaccessFallbackBooleanChoiceFieldprovides global default viageo_settings.ESTIMATED_LOCATION_ENABLEDclean()validation prevents enabling estimated location without WHOIS configuredorganization_post_save_receiverusesget_or_createfor idempotent settings creation- All user-facing strings properly marked for i18n with
_()openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
1-62: LGTM!The migration correctly creates the
OrganizationGeoSettingsmodel:
- UUID primary key follows project conventions
estimated_location_enabledwithnull=True, blank=True, default=Noneallows theFallbackBooleanChoiceFieldto apply its fallback value when unset- OneToOneField to Organization with CASCADE ensures cleanup on org deletion
- Swappable setting enables customization for downstream projects
The
fallback=Falsein the field definition means new organizations will have estimated location disabled by default until explicitly enabled, which is the safe default.openwisp_controller/geo/estimated_location/service.py (1)
64-77: Good failure isolation around task enqueue.Logging and swallowing broker publish errors keeps estimated-location enrichment best-effort instead of breaking the caller path.
| transaction.on_commit( | ||
| lambda: whois_fetched.send( | ||
| sender=WHOISInfo, | ||
| whois=whois_obj, | ||
| updated_fields=update_fields, | ||
| device=device, | ||
| ) | ||
| ) | ||
| if initial_ip_address: | ||
| transaction.on_commit( | ||
| # execute synchronously as we're already in a background task | ||
| lambda: delete_whois_record(ip_address=initial_ip_address) | ||
| ) |
There was a problem hiding this comment.
Don't bypass the skip path when the WHOIS row is already fresh.
Line 79 can return before this new post-commit path runs. In that case the task neither emits whois_lookup_skipped nor registers the delete_whois_record(initial_ip_address) cleanup from this function, so a device can miss the geo update when another worker refreshed the same IP first. Please send the skip signal and keep the old-IP cleanup on that branch too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/whois/tasks.py` around lines 89 - 101, When the
function bails out because the existing WHOIS row is fresh, ensure you still
emit the whois_lookup_skipped signal and register the old-IP cleanup via
transaction.on_commit so the delete_whois_record(initial_ip_address) runs;
update the early-return branch to call
whois_lookup_skipped.send(sender=WHOISInfo, device=device, ...) and register
transaction.on_commit(lambda:
delete_whois_record(ip_address=initial_ip_address)) before returning. Keep the
existing post-commit whois_fetched path unchanged for the non-skip case.
There was a problem hiding this comment.
@coderabbitai are you sure this is correct? The whois_lookup_skipped.send method is already handled in WHOISService.process_ip_data_and_location.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify by checking the actual service code to confirm whether whois_lookup_skipped is emitted before or after the task is dispatched.
🏁 Script executed:
#!/bin/bash
# Check the WHOISService.process_ip_data_and_location method
rg -n "process_ip_data_and_location|whois_lookup_skipped|_need_whois_lookup" openwisp_controller/config/whois/service.py -A 5 -B 2
Length of output: 1621
@pandafy, you're right — I apologize for the incorrect analysis. Looking at process_ip_data_and_location, the two paths are mutually exclusive:
elifbranch →whois_lookup_skippedis emitted when_need_whois_lookupreturnsFalse(lookup not needed at call time).- Task dispatch branch →
fetch_whois_detailsis enqueued when a lookup is needed.
The early return inside the task (when the WHOIS row is found already fresh) is a race condition scenario (e.g., another worker processed the same IP first) — it is a different situation from what whois_lookup_skipped is meant to cover. The signal does not need to be re-emitted from within the task.
My comment about whois_lookup_skipped was incorrect. The only remaining separate question is whether the delete_whois_record(initial_ip_address) cleanup should still run on that early-return path (since the old IP's WHOIS record may remain dangling if another worker processed the new IP first), but that's an independent concern from the signal.
🧠 Learnings used
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.
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.
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py
Outdated
Show resolved
Hide resolved
Test Failure: Query Count MismatchHello @pandafy, (Analysis for commit 60245d9) The test Fix: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/geo/admin.py (1)
229-232: 🧹 Nitpick | 🔵 TrivialConsider defensive handling for inline insertion.
The inline insertion assumes
ConfigSettingsInlineis always present inOrganizationAdmin.inlines. While the import chain fromconfig.adminshould guarantee this, aValueErrorwill be raised if the inline is ever missing due to future refactors.🛡️ Optional: defensive insertion
-OrganizationAdmin.inlines.insert( - OrganizationAdmin.inlines.index(ConfigSettingsInline) + 1, - GeoSettingsInline, -) +try: + _geo_inline_pos = OrganizationAdmin.inlines.index(ConfigSettingsInline) + 1 +except ValueError: + _geo_inline_pos = len(OrganizationAdmin.inlines) +OrganizationAdmin.inlines.insert(_geo_inline_pos, GeoSettingsInline)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/admin.py` around lines 229 - 232, The current insertion into OrganizationAdmin.inlines assumes ConfigSettingsInline is present and can raise ValueError; modify the logic that inserts GeoSettingsInline so it first checks for ConfigSettingsInline in OrganizationAdmin.inlines (using membership or try/except around index lookup) and if found insert GeoSettingsInline after it, otherwise append GeoSettingsInline at the end (or use a safe fallback insertion). Reference OrganizationAdmin.inlines, ConfigSettingsInline and GeoSettingsInline when updating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py`:
- Around line 12-14: Remove or shorten the redundant three-line explanatory
comment above the dependencies tuple in the migration module
0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py;
replace it with a single concise line (or delete it entirely) since the
dependency tuple on the geo app already makes the intent clear—update the
comment that references the geo app/OrganizationConfigSettings→GeoSettings
migration to be one brief line or remove it.
---
Duplicate comments:
In `@openwisp_controller/geo/admin.py`:
- Around line 229-232: The current insertion into OrganizationAdmin.inlines
assumes ConfigSettingsInline is present and can raise ValueError; modify the
logic that inserts GeoSettingsInline so it first checks for ConfigSettingsInline
in OrganizationAdmin.inlines (using membership or try/except around index
lookup) and if found insert GeoSettingsInline after it, otherwise append
GeoSettingsInline at the end (or use a safe fallback insertion). Reference
OrganizationAdmin.inlines, ConfigSettingsInline and GeoSettingsInline when
updating the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: da4ad9fb-25bc-41a5-816f-6f7446a5e410
📒 Files selected for processing (2)
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
🧠 Learnings (6)
📓 Common learnings
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.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/admin.py
🔇 Additional comments (4)
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
7-23: Dependency ordering and field removal look correct.Adding the
geo.0006_create_geo_settings_for_existing_orgsdependency before removingestimated_location_enabledfrom config addresses the migration-order safety concern; this segment is good to merge.openwisp_controller/geo/admin.py (3)
16-31: LGTM!Import changes are well-organized and necessary for the refactored functionality. The import chain from
config.adminensuresConfigSettingsInlineis available for inline positioning.
130-149: LGTM!The transition from
WHOISServicetoEstimatedLocationServiceis correctly implemented. The static method properly handlesNoneorg_idby returningFalse, which aligns with the usage inchange_viewwhereorg_idmay beNone.
212-226: LGTM!The
GeoSettingsInlineproperly implements singleton protection withcan_delete=Falseandmax_num=1. Theget_readonly_fieldsmethod correctly prevents togglingestimated_location_enabledwhen WHOIS is not configured, with appropriate guarding against duplicate field entries.
...fig/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/geo/admin.py`:
- Around line 229-232: Add a focused admin test that imports
openwisp_controller.geo.admin to trigger the inline mutation and then asserts
GeoSettingsInline is inserted immediately after ConfigSettingsInline in
OrganizationAdmin.inlines; in the new test (e.g.,
tests/admin/test_geo_inline_order.py) import OrganizationAdmin,
GeoSettingsInline and ConfigSettingsInline from openwisp_controller.geo.admin,
get their indices via OrganizationAdmin.inlines.index(...), and assert that
index(GeoSettingsInline) == index(ConfigSettingsInline) + 1 to lock the
ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc095378-fd4a-46b9-aed1-23109ee77322
📒 Files selected for processing (1)
openwisp_controller/geo/admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Python==3.10 | django~=4.2.0
- 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.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/admin.py
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/admin.py
🔇 Additional comments (2)
openwisp_controller/geo/admin.py (2)
130-149: Geo-owned enablement checks look consistent.Passing
obj.organization_id/org_idintoEstimatedLocationService.check_estimated_location_enabled()keeps field visibility, readonly handling, and the change-form context aligned with the newOrganizationGeoSettingsowner.
212-226: Cache invalidation is already implemented through Django signal handlers. TheGeoConfig.ready()method connectsEstimatedLocationService.invalidate_org_settings_cacheto bothpost_saveandpost_deletesignals forOrganizationGeoSettings. When the inline saves changes, the post_save signal automatically triggers cache deletion viacache.delete(f"organization_geo_{org_id}"), ensuring the cached settings stay in sync with admin edits.> Likely an incorrect or invalid review comment.
| OrganizationAdmin.inlines.insert( | ||
| max(0, OrganizationAdmin.inlines.index(ConfigSettingsInline) + 1), | ||
| GeoSettingsInline, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a focused admin test for inline ordering.
This import-time mutation of OrganizationAdmin.inlines is easy to regress silently. A small admin test that asserts GeoSettingsInline is inserted immediately after ConfigSettingsInline would lock down the intended placement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/geo/admin.py` around lines 229 - 232, Add a focused admin
test that imports openwisp_controller.geo.admin to trigger the inline mutation
and then asserts GeoSettingsInline is inserted immediately after
ConfigSettingsInline in OrganizationAdmin.inlines; in the new test (e.g.,
tests/admin/test_geo_inline_order.py) import OrganizationAdmin,
GeoSettingsInline and ConfigSettingsInline from openwisp_controller.geo.admin,
get their indices via OrganizationAdmin.inlines.index(...), and assert that
index(GeoSettingsInline) == index(ConfigSettingsInline) + 1 to lock the
ordering.
Test Failure:
|
95a4db6 to
57807fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_controller/config/whois/service.py (1)
213-224:⚠️ Potential issue | 🟠 MajorDon't emit
whois_lookup_skippedon saves where the IP did not change.
initial_ipis captured but ignored here. With a fresh public WHOIS row, any unrelated device save now emitswhois_lookup_skipped, and the geo receiver retriggers estimated-location work even though nothing about IP resolution changed.Suggested fix
if force_lookup or self._need_whois_lookup(new_ip): transaction.on_commit( lambda: fetch_whois_details.delay( device_pk=self.device.pk, initial_ip_address=initial_ip, ) ) - elif self.is_whois_enabled and self.is_valid_public_ip_address(new_ip): + elif ( + new_ip != initial_ip + and self.is_whois_enabled + and self.is_valid_public_ip_address(new_ip) + ): # Emit signal when lookup is skipped so receivers can react whois_lookup_skipped.send(sender=self.__class__, device=self.device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/service.py` around lines 213 - 224, The current logic emits whois_lookup_skipped even when the device IP didn't change; update the branch that sends whois_lookup_skipped to only run when the public IP is valid, WHOIS is enabled, and the IP actually changed by comparing new_ip to initial_ip (use the captured self.device._initial_last_ip). Concretely, modify the elif condition that currently checks is_whois_enabled and is_valid_public_ip_address(new_ip) to also require new_ip != initial_ip before calling whois_lookup_skipped.send; leave the transaction.on_commit path and fetch_whois_details call as-is.openwisp_controller/geo/migrations/__init__.py (1)
6-25:⚠️ Potential issue | 🟡 MinorUse the migration connection when seeding group permissions.
This helper still reads
GroupandPermissionthrough default managers. On a non-default migration alias, the permission grant lands in the wrong database and the migrated DB never gets the geo permissions it needs.Suggested fix
def _assign_permissions_to_groups(apps, schema_editor, operators_and_admins_can_change): + db_alias = schema_editor.connection.alias create_default_permissions(apps, schema_editor) manage_operations = ["add", "change", "delete"] Group = get_swapped_model(apps, "openwisp_users", "Group") try: - admin = Group.objects.get(name="Administrator") - operator = Group.objects.get(name="Operator") + admin = Group.objects.using(db_alias).get(name="Administrator") + operator = Group.objects.using(db_alias).get(name="Operator") # consider failures custom cases # that do not have to be dealt with except Group.DoesNotExist: return for model_name in operators_and_admins_can_change: for operation in manage_operations: - permission = Permission.objects.get( + permission = Permission.objects.using(db_alias).get( codename="{}_{}".format(operation, model_name) ) admin.permissions.add(permission.pk) operator.permissions.add(permission.pk)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/migrations/__init__.py` around lines 6 - 25, The migration reads Group and Permission via default managers causing writes to the wrong DB; modify _assign_permissions_to_groups to use migration-safe models and the migration DB alias: get Permission via apps.get_model("auth", "Permission") (keep using get_swapped_model for Group), then perform all queries/changes with the migration connection alias = schema_editor.connection.alias (e.g., Group.objects.using(alias).get(...), Permission.objects.using(alias).get(...), and admin.permissions.using(alias).add(permission.pk) / operator.permissions.using(alias).add(permission.pk)) so all lookups and permission assignments target the migrating database.
♻️ Duplicate comments (6)
openwisp_controller/config/whois/tasks.py (1)
79-96:⚠️ Potential issue | 🟠 MajorFresh-WHOIS shortcut bypasses the new post-WHOIS flow.
Because the early return on Line 79 never reaches this callback path, the new geo side effects added here are skipped whenever another worker has already refreshed the same IP. That also leaves
initial_ip_addresscleanup undone, so stale WHOIS rows can stick around. Emit the skip-path signal and schedule the old-IP cleanup on that branch too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tasks.py` around lines 79 - 96, The early-return branch (if whois_obj and not whois_service.is_older(...)) currently returns and skips the post-WHOIS flow; instead, emit the same skip-path signal and schedule old-IP cleanup: replace the bare return with a call to whois_fetched.send(sender=WHOISInfo, whois=whois_obj, updated_fields=[], device=device) (or include a small "skipped" marker if your signal consumers expect it) and schedule the initial_ip_address cleanup using the same transaction.on_commit pattern used below (invoke the existing cleanup function on whois_service that removes initial_ip_address inside the commit callback). Ensure you reference whois_obj, whois_service.is_older, whois_fetched, WHOISInfo, device and initial_ip_address so the skip path mirrors the committed path’s side effects.openwisp_controller/geo/settings.py (1)
8-14:⚠️ Potential issue | 🟠 MajorUse
WHOIS_CONFIGUREDinstead ofWHOIS_ENABLEDfor startup validation.The check validates
WHOIS_ENABLED, but if WHOIS credentials (WHOIS_GEOIP_ACCOUNT,WHOIS_GEOIP_KEY) are missing,ESTIMATED_LOCATION_ENABLED=Truewill pass this startup validation but fail at runtime. UseWHOIS_CONFIGUREDinstead, which is the established gate throughout the codebase and requires both credentials to be set.Additionally, the inline comment on line 8 is redundant since the condition is self-descriptive.
Proposed fix
-# Validate that WHOIS is enabled if estimated location is enabled if ESTIMATED_LOCATION_ENABLED: - if not config_settings.WHOIS_ENABLED: + if not config_settings.WHOIS_CONFIGURED: raise ImproperlyConfigured( - "OPENWISP_CONTROLLER_WHOIS_ENABLED must be set to True before " - "setting OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED to True." + "WHOIS must be configured (OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT " + "and OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY must be set) before " + "enabling OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/settings.py` around lines 8 - 14, Replace the startup validation to check config_settings.WHOIS_CONFIGURED instead of config_settings.WHOIS_ENABLED when ESTIMATED_LOCATION_ENABLED is true (so the check ensures both WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY are present); update the ImproperlyConfigured message to reference WHOIS being properly configured (or WHOIS credentials) and remove the redundant inline comment above the condition; target the ESTIMATED_LOCATION_ENABLED conditional and the existing ImproperlyConfigured raise in this block to implement the change.openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (2)
17-18:⚠️ Potential issue | 🟡 MinorRun both backfills on
schema_editor.connection.alias.These loops still read and write through default managers. On a non-default migration alias, Django can read organizations/config rows from one database and populate geo settings in another.
Suggested fix
def create_geo_settings_for_existing_orgs(apps, schema_editor): @@ - Organization = apps.get_model("openwisp_users", "Organization") + db_alias = schema_editor.connection.alias + Organization = apps.get_model("openwisp_users", "Organization") OrganizationGeoSettings = get_swapped_model(apps, "geo", "OrganizationGeoSettings") - for org in Organization.objects.iterator(): - OrganizationGeoSettings.objects.get_or_create(organization_id=org.pk) + for org in Organization.objects.using(db_alias).iterator(): + OrganizationGeoSettings.objects.using(db_alias).get_or_create( + organization_id=org.pk + ) @@ def copy_estimated_location_enabled(apps, schema_editor): @@ + db_alias = schema_editor.connection.alias OrganizationConfigSettings = get_swapped_model( apps, "config", "OrganizationConfigSettings" ) OrganizationGeoSettings = get_swapped_model(apps, "geo", "OrganizationGeoSettings") @@ - for cfg in OrganizationConfigSettings.objects.iterator(): - geo, _ = OrganizationGeoSettings.objects.get_or_create( + for cfg in OrganizationConfigSettings.objects.using(db_alias).iterator(): + geo, _ = OrganizationGeoSettings.objects.using(db_alias).get_or_create( organization_id=cfg.organization_id ) @@ - geo.save(update_fields=["estimated_location_enabled"]) + geo.save(using=db_alias, update_fields=["estimated_location_enabled"])Also applies to: 27-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py` around lines 17 - 18, The migration loops use default DB managers, causing cross-db reads/writes on non-default migration aliases; update both loops that iterate Organization and that create OrganizationGeoSettings to call the managers with the migration connection alias (use Organization.objects.using(schema_editor.connection.alias) and OrganizationGeoSettings.objects.using(schema_editor.connection.alias)) and perform iterator() and get_or_create(...) via that using(...) call so reads and writes happen on the same schema_editor.connection.alias; locate the code around the Organization and OrganizationGeoSettings calls in the migration (functions referencing schema_editor) and change their queryset/manager usage accordingly.
46-49:⚠️ Potential issue | 🟠 MajorDepend on
config.0063before readingestimated_location_enabled.
copy_estimated_location_enabled()readsOrganizationConfigSettings.estimated_location_enabled, but this migration still does not depend on the config migration that adds that column. Django can legally schedule thisRunPythonbefore the source field exists.Suggested fix
dependencies = [ ("geo", "0005_organizationgeosettings"), + ( + "config", + "0063_organizationconfigsettings_estimated_location_enabled_and_more", + ), ("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py` around lines 46 - 49, The migration's RunPython (copy_estimated_location_enabled) reads OrganizationConfigSettings.estimated_location_enabled but lacks a dependency on the migration that creates that field; add a dependency on the config migration that adds estimated_location_enabled (depend on config.0063 as suggested) by inserting the appropriate tuple ("config", "0063") — or the exact app/migration pair that creates the column — into the dependencies list so Django will run the config migration before copy_estimated_location_enabled executes.openwisp_controller/geo/estimated_location/tests/tests.py (1)
267-270:⚠️ Potential issue | 🟡 MinorReturn concrete cache entries in these two subtests.
A bare
cache.getpatch returns a truthyMagicMock, so the WHOIS and geo settings checks treat it as a cache hit and read truthy mock attributes. These assertions can pass without exercising the real settings objects or the miss path.Suggested fix
with self.subTest( "Estimated location task called when last_ip is changed and is public" ): with ( - mock.patch("django.core.cache.cache.get") as mocked_get, + mock.patch( + "django.core.cache.cache.get", + side_effect=lambda key, default=None: { + f"organization_config_{org.pk}": org.config_settings, + f"organization_geo_{org.pk}": org.geo_settings, + }.get(key, default), + ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set, ): @@ with self.subTest( "Estimated location task called when last_ip has related WhoIsInfo" ): with ( - mock.patch("django.core.cache.cache.get") as mocked_get, + mock.patch( + "django.core.cache.cache.get", + side_effect=lambda key, default=None: { + f"organization_config_{org.pk}": org.config_settings, + f"organization_geo_{org.pk}": org.geo_settings, + }.get(key, default), + ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set, ):Also applies to: 287-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/estimated_location/tests/tests.py` around lines 267 - 270, The tests patching django.core.cache.cache.get/cache.set currently leave mocked_get as a truthy MagicMock which incorrectly simulates cache hits; update those subtests (the ones exercising WHOIS and geo settings checks) to return concrete values: for miss-path tests set mocked_get.return_value = None so the code follows the miss branch, and for hit-path tests set mocked_get.return_value to the actual cached object/structure the code expects (e.g., a dict or model-like object used by the WHOIS and geo settings logic) and assert behavior; keep references to the same patched symbols (django.core.cache.cache.get and django.core.cache.cache.set) when modifying the two subtests around the shown block and the similar block at lines ~287-289.openwisp_controller/geo/estimated_location/service.py (1)
88-125:⚠️ Potential issue | 🟠 MajorGuard the dequeue-side write path with the current org setting.
Queued WHOIS jobs can outlive an admin toggle. Without re-checking
is_estimated_location_enabledhere, a task enqueued while the feature was on can still create or update an estimatedLocationafter the feature has been disabled.🛡️ Suggested fix
if not (device_location := getattr(self.device, "devicelocation", None)): device_location = DeviceLocation(content_object=self.device) current_location = device_location.location + if not self.is_estimated_location_enabled: + return current_location if not current_location or ( attached_devices_exists and current_location.is_estimated ):Based on learnings,
BaseLocation.save()intentionally protectsis_estimatedwhen the feature is disabled, so this async path should re-check the flag before persisting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/estimated_location/service.py` around lines 88 - 125, Before performing any DB writes in this async dequeue path, re-check the org-level flag and bail out if disabled: call is_estimated_location_enabled(...) for the device's organization (or the existing helper used elsewhere) immediately before creating a new Location (where current_location is constructed and saved) and immediately before updating/saving an existing estimated location (the branch that calls current_location.save). Do not change validation logic in BaseLocation.save; instead skip the creation/update and avoid calling current_location.save(...) or device_location.save() and do not send send_whois_task_notification if the flag is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/geo/api/views.py`:
- Around line 349-358: The get_object method in OrganizationGeoSettingsView
should perform an explicit object-level permission check before returning the
object: after retrieving the OrganizationGeoSettings instance (in
OrganizationGeoSettingsView.get_object), call
self.check_object_permissions(self.request, obj) and then return obj; keep the
existing DoesNotExist handling that raises Http404. This mirrors the pattern
used in other views (e.g., config/api/views.py and connection/api/views.py) and
ensures consistent, explicit permission validation.
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 14-15: The migration currently uses
apps.get_model("openwisp_users", "Organization") which breaks swappable
installations; replace that call with get_swapped_model(apps, "openwisp_users",
"Organization") so the migration resolves the swapped Organization model
similarly to OrganizationGeoSettings. Update the reference where Organization is
assigned (replace apps.get_model(...) with get_swapped_model(...)) and ensure
the existing import/utility get_swapped_model is used so the backfill iterates
the correct table for swapped Organization models.
In `@openwisp_controller/geo/tests/test_api.py`:
- Around line 1525-1531: The PATCH test currently only asserts a 400 and
response payload but doesn't verify the DB row was not modified; after calling
self.client.patch(...) and checking response.status_code and response.data, call
org1_geo_settings.refresh_from_db() and assert that
org1_geo_settings.estimated_location_enabled (and any other relevant fields
changed by the request) remain equal to their pre-request values to mirror the
PUT branch rollback assertion and ensure no partial write occurred.
---
Outside diff comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 213-224: The current logic emits whois_lookup_skipped even when
the device IP didn't change; update the branch that sends whois_lookup_skipped
to only run when the public IP is valid, WHOIS is enabled, and the IP actually
changed by comparing new_ip to initial_ip (use the captured
self.device._initial_last_ip). Concretely, modify the elif condition that
currently checks is_whois_enabled and is_valid_public_ip_address(new_ip) to also
require new_ip != initial_ip before calling whois_lookup_skipped.send; leave the
transaction.on_commit path and fetch_whois_details call as-is.
In `@openwisp_controller/geo/migrations/__init__.py`:
- Around line 6-25: The migration reads Group and Permission via default
managers causing writes to the wrong DB; modify _assign_permissions_to_groups to
use migration-safe models and the migration DB alias: get Permission via
apps.get_model("auth", "Permission") (keep using get_swapped_model for Group),
then perform all queries/changes with the migration connection alias =
schema_editor.connection.alias (e.g., Group.objects.using(alias).get(...),
Permission.objects.using(alias).get(...), and
admin.permissions.using(alias).add(permission.pk) /
operator.permissions.using(alias).add(permission.pk)) so all lookups and
permission assignments target the migrating database.
---
Duplicate comments:
In `@openwisp_controller/config/whois/tasks.py`:
- Around line 79-96: The early-return branch (if whois_obj and not
whois_service.is_older(...)) currently returns and skips the post-WHOIS flow;
instead, emit the same skip-path signal and schedule old-IP cleanup: replace the
bare return with a call to whois_fetched.send(sender=WHOISInfo, whois=whois_obj,
updated_fields=[], device=device) (or include a small "skipped" marker if your
signal consumers expect it) and schedule the initial_ip_address cleanup using
the same transaction.on_commit pattern used below (invoke the existing cleanup
function on whois_service that removes initial_ip_address inside the commit
callback). Ensure you reference whois_obj, whois_service.is_older,
whois_fetched, WHOISInfo, device and initial_ip_address so the skip path mirrors
the committed path’s side effects.
In `@openwisp_controller/geo/estimated_location/service.py`:
- Around line 88-125: Before performing any DB writes in this async dequeue
path, re-check the org-level flag and bail out if disabled: call
is_estimated_location_enabled(...) for the device's organization (or the
existing helper used elsewhere) immediately before creating a new Location
(where current_location is constructed and saved) and immediately before
updating/saving an existing estimated location (the branch that calls
current_location.save). Do not change validation logic in BaseLocation.save;
instead skip the creation/update and avoid calling current_location.save(...) or
device_location.save() and do not send send_whois_task_notification if the flag
is false.
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 267-270: The tests patching django.core.cache.cache.get/cache.set
currently leave mocked_get as a truthy MagicMock which incorrectly simulates
cache hits; update those subtests (the ones exercising WHOIS and geo settings
checks) to return concrete values: for miss-path tests set
mocked_get.return_value = None so the code follows the miss branch, and for
hit-path tests set mocked_get.return_value to the actual cached object/structure
the code expects (e.g., a dict or model-like object used by the WHOIS and geo
settings logic) and assert behavior; keep references to the same patched symbols
(django.core.cache.cache.get and django.core.cache.cache.set) when modifying the
two subtests around the shown block and the similar block at lines ~287-289.
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 17-18: The migration loops use default DB managers, causing
cross-db reads/writes on non-default migration aliases; update both loops that
iterate Organization and that create OrganizationGeoSettings to call the
managers with the migration connection alias (use
Organization.objects.using(schema_editor.connection.alias) and
OrganizationGeoSettings.objects.using(schema_editor.connection.alias)) and
perform iterator() and get_or_create(...) via that using(...) call so reads and
writes happen on the same schema_editor.connection.alias; locate the code around
the Organization and OrganizationGeoSettings calls in the migration (functions
referencing schema_editor) and change their queryset/manager usage accordingly.
- Around line 46-49: The migration's RunPython (copy_estimated_location_enabled)
reads OrganizationConfigSettings.estimated_location_enabled but lacks a
dependency on the migration that creates that field; add a dependency on the
config migration that adds estimated_location_enabled (depend on config.0063 as
suggested) by inserting the appropriate tuple ("config", "0063") — or the exact
app/migration pair that creates the column — into the dependencies list so
Django will run the config migration before copy_estimated_location_enabled
executes.
In `@openwisp_controller/geo/settings.py`:
- Around line 8-14: Replace the startup validation to check
config_settings.WHOIS_CONFIGURED instead of config_settings.WHOIS_ENABLED when
ESTIMATED_LOCATION_ENABLED is true (so the check ensures both
WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY are present); update the
ImproperlyConfigured message to reference WHOIS being properly configured (or
WHOIS credentials) and remove the redundant inline comment above the condition;
target the ESTIMATED_LOCATION_ENABLED conditional and the existing
ImproperlyConfigured raise in this block to implement the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7798dd6d-b0c7-44c0-8bca-cd234341c244
📒 Files selected for processing (39)
docs/developer/extending.rstdocs/developer/utils.rstdocs/user/rest-api.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/whois.pyopenwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/signals.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/geo/models.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/utils.pyopenwisp_controller/settings.pyopenwisp_controller/tests/mixins.pytests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pytests/openwisp2/sample_geo/models.pytests/openwisp2/sample_geo/views.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (4)
- tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py
- openwisp_controller/config/base/whois.py
- openwisp_controller/config/tests/test_api.py
- openwisp_controller/config/base/multitenancy.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.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.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pytests/openwisp2/settings.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/api/views.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pytests/openwisp2/settings.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/api/views.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pytests/openwisp2/settings.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/api/views.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pytests/openwisp2/settings.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/api/views.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
🧠 Learnings (10)
📓 Common learnings
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.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp} : Avoid unnecessary comments or docstrings for code that is already clear
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/geo/settings.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/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pyopenwisp_controller/geo/estimated_location/tasks.pytests/openwisp2/settings.pyopenwisp_controller/config/settings.pyopenwisp_controller/geo/estimated_location/tests/utils.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/geo/estimated_location/mixins.pyopenwisp_controller/config/signals.pyopenwisp_controller/geo/models.pyopenwisp_controller/settings.pyopenwisp_controller/geo/api/serializers.pyopenwisp_controller/geo/utils.pyopenwisp_controller/geo/estimated_location/handlers.pyopenwisp_controller/geo/estimated_location/utils.pyopenwisp_controller/geo/api/views.pytests/openwisp2/sample_geo/views.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/geo/migrations/__init__.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/tasks.pyopenwisp_controller/tests/mixins.pytests/openwisp2/settings.pytests/openwisp2/sample_geo/models.pyopenwisp_controller/config/signals.pydocs/developer/extending.rstopenwisp_controller/geo/models.pyopenwisp_controller/geo/apps.pyopenwisp_controller/geo/settings.pyopenwisp_controller/geo/base/models.pytests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/admin.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
🔇 Additional comments (29)
openwisp_controller/settings.py (1)
4-5: Good centralization of controller setting reads.Using
get_settinghere keeps access consistent and preserves the previous fallback behavior.Also applies to: 8-8
openwisp_controller/config/signals.py (1)
68-75: Signal additions look consistent and well-scoped.The new signal contracts are clear and align with the related WHOIS/geo integration points.
docs/developer/utils.rst (1)
383-409: Nice documentation update for the new WHOIS signals.The added sections clearly describe path, arguments, and emission conditions.
tests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.py (1)
20-59: Migration shape looks correct for sample geo settings.Model fields and one-to-one organization linkage are correctly defined for the test schema.
openwisp_controller/tests/mixins.py (1)
14-18: Test inline params update is aligned with the new admin inline.Adding the
geo_settings-*management keys keeps formset payloads in sync.openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py (1)
7-22: Migration dependency and removal operation look correct.The dependency ordering and
RemoveFieldare properly set for this cleanup step.openwisp_controller/geo/estimated_location/tests/utils.py (1)
9-9: Good robustness improvement in test setup.Using
OrganizationGeoSettings.objects.get_or_create(...)here safely handles cases where the related row is missing.Also applies to: 16-28
openwisp_controller/geo/utils.py (1)
49-53: Route addition is clean and correctly wired.The new organization geo-settings endpoint is properly registered with its dedicated view and name.
tests/openwisp2/settings.py (1)
283-283: Correct swapper override key.This matches the geo app's
OrganizationGeoSettingsswapper lookup, so the sample app override should resolve cleanly.openwisp_controller/config/admin.py (1)
1389-1395: Good admin split between config and geo settings.Keeping only
whois_enabledhere makesConfigSettingsInlinematch the new ownership boundary, with estimated-location settings now living under geo.docs/user/rest-api.rst (1)
804-849: Endpoint docs now cover the full geo-settings surface.Documenting GET, PUT, and PATCH here keeps the organization geo-settings endpoint discoverable and aligned with the new API surface.
tests/openwisp2/sample_geo/models.py (1)
33-35: Missingswappablesetting in Meta class.The default
OrganizationGeoSettingsmodel inopenwisp_controller/geo/models.pyincludesswappable = swapper.swappable_setting("geo", "OrganizationGeoSettings")in its Meta class. While test models typically don't need this since they are the swapped-in model, other test models in this file (Location,FloorPlan,DeviceLocation) also don't include it, so this appears consistent with the established pattern.openwisp_controller/geo/estimated_location/mixins.py (3)
1-1: LGTM!The import correctly switches to
EstimatedLocationServiceas part of the geo module separation.
14-16: LGTM!The method call correctly uses
EstimatedLocationService.check_estimated_location_enabled(obj.organization_id)which matches the service method signature shown in the context snippets.
31-33: LGTM!Consistent change for the GeoJSON mixin, correctly delegating to the new service.
openwisp_controller/geo/api/views.py (3)
33-33: LGTM!Import added correctly for the new serializer.
40-40: LGTM!Model loaded via swapper correctly.
373-373: LGTM!View callable registered correctly.
openwisp_controller/config/whois/tests/tests.py (2)
24-25: LGTM!Imports added for the new signals and the
catch_signaltest utility.
454-472: LGTM!Good test coverage for the new signal emissions:
test_fetch_whois_emits_signalverifieswhois_fetchedis emitted when WHOIS details are fetchedtest_whois_lookup_skipped_emits_signalverifieswhois_lookup_skippedis emitted when lookup is skipped due to existing WHOISInfoBoth tests properly set up the required mocks and fixtures.
openwisp_controller/geo/models.py (2)
3-8: LGTM!Import updated to include
AbstractOrganizationGeoSettings.
29-32: LGTM!The
OrganizationGeoSettingsmodel follows the established pattern for swappable models in this file, with proper inheritance,Meta.abstract = False, and the swappable setting.openwisp_controller/geo/api/serializers.py (4)
1-5: LGTM!Imports added correctly for
copyandmodels.
25-25: LGTM!Model loaded via swapper correctly.
180-182: LGTM!Error message updated to a clearer, consolidated sentence while maintaining the same validation logic and translatable wrapper.
28-44: No action needed. Theexclude_validationattribute is provided by theValidatedModelSerializerbase class from openwisp-utils, as evidenced by the passing test suite. The serializer'svalidatemethod successfully executes withoutAttributeError, confirming the attribute is available at runtime.> Likely an incorrect or invalid review comment.openwisp_controller/geo/estimated_location/utils.py (1)
52-68: No action needed. Theorganization_idis correctly added by the caller intasks.pyat line 69 when merging the defaults returned byget_location_defaults_from_whoiswith"organization_id": device.organization_id.openwisp_controller/geo/migrations/0005_organizationgeosettings.py (1)
14-17: Consider whetherOrganizationis swappable before applying this suggestion.The foreign key uses
swapper.get_model_name(...), but the dependency remains hard-coded. However, verification shows mixed signals: migration 0006 usesapps.get_model(...)directly for Organization (non-swappable pattern), while the FK field uses swapper. If Organization is indeed swappable in openwisp_users, this should be updated to useswapper.dependency("openwisp_users", "Organization"). Without confirming Organization's swappable status, this review comment cannot be fully validated.openwisp_controller/geo/admin.py (1)
212-232: Nice singleton guard on the new inline.
can_delete = Falseplusmax_num = 1is the right constraint for organization-scoped geo settings, and the WHOIS-dependent readonly handling keeps the admin behavior aligned with the API.
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (1)
53-56:⚠️ Potential issue | 🟠 MajorAdd missing config migration dependency before running the data copy.
copy_estimated_location_enabledreadsOrganizationConfigSettings.estimated_location_enabled, butdependenciesdoes not explicitly require the config migration that adds that field. Please add("config", "0063_organizationconfigsettings_estimated_location_enabled_and_more")to guarantee historical model state at runtime.
Line 53-Line 56.♻️ Proposed fix
class Migration(migrations.Migration): dependencies = [ ("geo", "0005_organizationgeosettings"), + ("config", "0063_organizationconfigsettings_estimated_location_enabled_and_more"), ("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"), ]#!/bin/bash set -euo pipefail # Verify source field is added in config migration 0063 rg -n -C2 'estimated_location_enabled' openwisp_controller/config/migrations/0063_organizationconfigsettings_estimated_location_enabled_and_more.py # Verify current geo 0006 dependencies and RunPython reader rg -n -C3 'dependencies =|copy_estimated_location_enabled|estimated_location_enabled' openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py` around lines 53 - 56, The migration depends on a field added by the config migration but doesn't declare that dependency; update the migrations.dependencies list in 0006_create_geo_settings_for_existing_orgs.py to include ("config", "0063_organizationconfigsettings_estimated_location_enabled_and_more") so the historical model state used by the data-copy routine (see function copy_estimated_location_enabled and the dependencies variable) includes OrganizationConfigSettings.estimated_location_enabled before running the RunPython. Ensure the tuple is added alongside the existing ("geo", "0005_organizationgeosettings") and ("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx") entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/geo/estimated_location/service.py`:
- Around line 70-76: The error log currently records the raw ip_address when
enqueueing fails; remove or redact the IP so logs only include the device
identifier and the exception. Update the logger.error call that references
getattr(self.device, "pk", None), ip_address, exc (the failing enqueue except
block) to omit ip_address or replace it with a redacted placeholder (e.g.,
"<redacted>") and keep the exception and device PK for diagnostics.
---
Duplicate comments:
In
`@openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py`:
- Around line 53-56: The migration depends on a field added by the config
migration but doesn't declare that dependency; update the
migrations.dependencies list in 0006_create_geo_settings_for_existing_orgs.py to
include ("config",
"0063_organizationconfigsettings_estimated_location_enabled_and_more") so the
historical model state used by the data-copy routine (see function
copy_estimated_location_enabled and the dependencies variable) includes
OrganizationConfigSettings.estimated_location_enabled before running the
RunPython. Ensure the tuple is added alongside the existing ("geo",
"0005_organizationgeosettings") and ("openwisp_users",
"0021_rename_user_id_email_openwisp_us_id_06c07a_idx") entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e03db5b-8cbe-4366-baca-9247a02b998f
📒 Files selected for processing (4)
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | 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.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.12 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
🧠 Learnings (9)
📓 Common learnings
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.
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/api/views.pyopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.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/migrations/0006_create_geo_settings_for_existing_orgs.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Regression test must fail if the patch is removed or commented out
Applied to files:
openwisp_controller/geo/tests/test_api.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
🔇 Additional comments (13)
openwisp_controller/geo/tests/test_api.py (7)
1525-1533: LGTM!The PATCH validation test now correctly verifies that a failed update leaves the database row unchanged by calling
refresh_from_db()and asserting the value remainsFalse.
1651-1672: LGTM!The subtest correctly tests permission enforcement by removing the change permission and using the real organization's URL, ensuring the response is 403 (forbidden) rather than 404.
1359-1366: LGTM!Clean helper method for adding model permissions using Django's ContentType framework.
1368-1449: LGTM!Comprehensive test coverage for the retrieve endpoint including:
- Unauthenticated access (401)
- Missing permissions (403)
- Successful retrieval with proper permission (200)
- Multi-tenancy enforcement (404 for other orgs)
- Superuser cross-organization access
1451-1546: LGTM!Well-structured update tests covering PUT/PATCH operations, organization field immutability, cross-organization restrictions, validation when WHOIS is not configured, and superuser privileges.
1548-1585: LGTM!Multi-tenancy tests properly verify that users can only access organizations they manage, and that users with multiple organization memberships can access all their managed organizations.
1587-1616: LGTM!Good coverage of different user access levels: regular members (403), org admins with permissions (200), and superusers without explicit permissions (200).
openwisp_controller/geo/api/views.py (1)
350-358: LGTM!The
OrganizationGeoSettingsViewimplementation is correct:
- Uses
ProtectedAPIMixinfor authentication and multi-tenancy filtering- Correctly retrieves
organization_pkfrom URL kwargs- Properly calls
check_object_permissionsbefore returning the object- Uses
get_object_or_404for clean 404 handlingopenwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py (2)
9-23: Good migration-safe backfill implementation.
create_geo_settings_for_existing_orgscorrectly uses swapped models, migration DB alias, and streaming iteration.
25-50: Copy step is robust and idempotent.The backfill correctly respects the active DB alias and avoids redundant updates with a value-difference check.
openwisp_controller/geo/estimated_location/service.py (3)
35-45: Nice cache fill on the no-row path.Caching a sentinel settings instance here avoids repeating the same lookup for organizations that have not stored geo settings yet.
As per coding guidelines, "Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries."
92-98: Good dequeue-side feature-flag guard.Re-checking the org setting before any write closes the stale-queue path where an already-enqueued task could still persist estimated data after the feature was disabled.
Based on learnings,
BaseLocation.save()intentionally protectsis_estimatedchanges when the estimated location feature is disabled.
102-108: Good to see validation on both write paths.Running
full_clean()before both the create and update saves keeps malformed WHOIS-derived data out of the database.Also applies to: 121-126
| except Exception as exc: # pragma: no cover - defensive logging | ||
| logger.error( | ||
| "Failed to enqueue estimated location task for device %s ip %s: %s", | ||
| getattr(self.device, "pk", None), | ||
| ip_address, | ||
| exc, | ||
| ) |
There was a problem hiding this comment.
Do not log the raw IP address on enqueue failures.
This error path writes ip_address into persistent error logs even though the device id is enough to diagnose a broker outage. Drop or redact the IP here.
🔒 Suggested fix
- except Exception as exc: # pragma: no cover - defensive logging
- logger.error(
- "Failed to enqueue estimated location task for device %s ip %s: %s",
- getattr(self.device, "pk", None),
- ip_address,
- exc,
- )
+ except Exception: # pragma: no cover - defensive logging
+ logger.exception(
+ "Failed to enqueue estimated location task for device %s",
+ getattr(self.device, "pk", None),
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/geo/estimated_location/service.py` around lines 70 - 76,
The error log currently records the raw ip_address when enqueueing fails; remove
or redact the IP so logs only include the device identifier and the exception.
Update the logger.error call that references getattr(self.device, "pk", None),
ip_address, exc (the failing enqueue except block) to omit ip_address or replace
it with a redacted placeholder (e.g., "<redacted>") and keep the exception and
device PK for diagnostics.
Checklist
Reference to Existing Issue
Closes #1224