[chores:fix] Separated geo.estimated_location from config.whois #1224#1315
Merged
nemesifier merged 16 commits intomasterfrom Apr 7, 2026
Merged
[chores:fix] Separated geo.estimated_location from config.whois #1224#1315nemesifier merged 16 commits intomasterfrom
nemesifier merged 16 commits intomasterfrom
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR moves estimated-location responsibilities from the config app into geo. It adds an OrganizationGeoSettings model, migrations, admin inline, serializer, API view and URL; introduces EstimatedLocationService (caching, task enqueueing, persistence, cache invalidation); relocates estimated-location creation/update logic, tasks, handlers and utils to geo; 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
participant Celery as Celery Queue
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->>Celery: send_task("whois_estimated_location_task", args)
Celery->>Location: execute task -> create/update estimated Location (transactional save)
Location->>DB: save
DB-->>Location: saved instance
EstService-->>WHOIS_Task: send notification (created/updated)
else Disabled
EstService-->>WHOIS_Task: no-op / skip
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 Queue
participant Device as Device Model
WHOIS_Service->>Signals: emit whois_fetched(whois, updated_fields, device)
Signals-->>Handlers: whois_fetched_handler(...)
Handlers->>EstService: check_estimated_location_enabled(device.organization_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
Note over WHOIS_Service,Signals: whois_lookup_skipped triggers similar handler path
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+6
to
+13
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ( | ||
| "config", | ||
| "0063_organizationconfigsettings_estimated_location_enabled_and_more", | ||
| ), | ||
| ] |
Contributor
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.
Suggested change
| 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.
Flake8 F401 Unused ImportHello @pandafy, The CI build failed due to a Failure:
Fix: |
Contributor
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)
Comment on lines
+89
to
+96
| transaction.on_commit( | ||
| lambda: whois_fetched.send( | ||
| sender=WHOISInfo, | ||
| whois=whois_obj, | ||
| updated_fields=update_fields, | ||
| device=device, | ||
| ) | ||
| ) |
Contributor
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.
Member
Author
There was a problem hiding this comment.
@coderabbitai are you hallucinating? Can you re-review this comment and explain the problem properly?
Contributor
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.
Test Failures DetectedHello @pandafy,
|
Contributor
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:
|
Contributor
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. |
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
5f34d78 to
0a08bb1
Compare
Contributor
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.
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
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: |
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
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: |
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
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:
|
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
df49464 to
9f4b5f3
Compare
|
coverage: 98.588% (-0.07%) from 98.66% — issues/1224-separate-geo-logic-from-config into master |
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
1d60aa4 to
8d38dee
Compare
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
2de0490 to
a558103
Compare
CI Failures: Code Style and ImportsHello @pandafy, There are import sorting and formatting issues detected by
Fix: |
CI Failures: Code Style and ImportsHello @pandafy, The CI failed due to an
To fix this, please run the following command in your local environment: openwisp-qa-formatThis command will automatically format your code and sort the imports according to the project's standards. |
pandafy
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
a558103 to
7f5827a
Compare
Backward incompatible change within the master branch (still backward compatible to previous stable releases)
The estimated location feature now has its own MESSAGE_MAP and its own helper for sending notifications, clearly separating geo.estimated_location from config.whois while maintainig a very similar code structure.
nemesifier
requested changes
Apr 6, 2026
| ) | ||
| } | ||
| ) | ||
| return super().clean() |
Member
There was a problem hiding this comment.
This seems unneeded, I am going to remove it.
| return self.device.organization_id | ||
|
|
||
|
|
||
| class AbstractOrganizationGeoSettings(UUIDModel): |
Member
There was a problem hiding this comment.
We should do like we have done in other modules where we don't add a new primary key, relying on the organization one-to-one relation to be the primary key.
Test Failure in
|
nemesifier
force-pushed
the
issues/1224-separate-geo-logic-from-config
branch
from
401d070 to
4e8bf0a
Compare
nemesifier
approved these changes
Apr 6, 2026
pandafy
commented
Apr 7, 2026
Member
Author
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Thank you for resolving the bugs in my implementation @nemesifier! It looks good to merge now.
nemesifier
changed the title
nemesifier
changed the title
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Reference to Existing Issue
Closes #1224