[geo] keep floorplan org in sync with location

[xoaryaa]

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Closes #385

Description of Changes

• fixed an inconsistency where FloorPlan.organization could remain stale after changing the related Location.organization.
• implemented synchronization on location organization updates by bulk-updating related floorplans to keep organization data consistent.
• added a regression test to ensure floorplan organization updates when the location organization changes.

Screenshot

no UI changes

[coderabbitai]

Walkthrough

This PR fixes a bug where changing a location's organization does not synchronize the organization assignment to related floorplan objects. The solution adds a save() method to BaseLocation that detects organization changes and updates all associated floorplans accordingly. Additionally, a clean() method is added to BaseFloorPlan to validate that the floorplan's organization stays synchronized with its related location's organization. A test verifies this synchronization behavior when location organization is modified.

Sequence Diagram

sequenceDiagram
    participant Admin as Admin/User
    participant Location as Location Model
    participant Floorplan as Floorplan Model
    participant DB as Database

    Admin->>Location: Change organization_id
    Location->>Location: save()
    Note over Location: Detect org_id changed?
    alt Organization Changed
        Location->>DB: Fetch related floorplans
        DB-->>Location: Floorplans data
        Location->>Floorplan: Update organization_id on all<br/>related floorplans
        Floorplan->>DB: Save changes
    else Organization Unchanged
        Location->>DB: Save location normally
    end
    
    Admin->>Floorplan: Access/Validate floorplan
    Floorplan->>Floorplan: clean()
    Floorplan->>Location: Verify org via location
    Location-->>Floorplan: organization_id
    Floorplan->>Floorplan: Sync org context
    Floorplan->>DB: Validated and synced

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: keeping floorplan organization in sync with location organization, which is the primary objective.
Description check	✅ Passed	The description follows the template with completed checklist items, issue reference, and clear change description; documentation section is marked incomplete but acknowledged.
Linked Issues check	✅ Passed	The code changes directly address issue #385 by implementing synchronization to keep FloorPlan.organization consistent with Location.organization when the location's organization changes.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the organization sync inconsistency between FloorPlan and Location; no unrelated or out-of-scope modifications are present.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

🧹 Recent nitpick comments

openwisp_controller/geo/base/models.py (2)

1-9: Import ordering: Django imports should be grouped together.

The django.apps import at line 8 should be grouped with other Django imports at the top (lines 1-6), not placed after the swapper import.

♻️ Suggested import reordering

 from django.contrib.gis.db import models
+from django.apps import apps
 from django_loci.base.models import (
     AbstractFloorPlan,
     AbstractLocation,
     AbstractObjectLocation,
 )
 from swapper import get_model_name
-from django.apps import apps
 from openwisp_users.mixins import OrgMixin, ValidateOrgMixin

---

16-34: Consider using __init__ to track organization changes and avoid the DB query on every save.

The current implementation queries the database to detect organization changes. For better performance, track the original organization_id in __init__:

♻️ Optimization: Track original value in __init__

def __init__(self, *args, **kwargs):
    super().__init__(*args, **kwargs)
    self._original_organization_id = self.organization_id

def save(self, *args, **kwargs):
    org_changed = self.pk and self._original_organization_id != self.organization_id
    super().save(*args, **kwargs)
    if org_changed:
        app_label, model_name = get_model_name("geo", "FloorPlan").split(".")
        FloorPlan = apps.get_model(app_label, model_name)
        FloorPlan.objects.filter(location=self).update(
            organization_id=self.organization_id
        )
    self._original_organization_id = self.organization_id

openwisp_controller/geo/tests/test_models.py (1)

32-47: Good regression test for issue #385.

The test correctly verifies the core behavior: when a location's organization changes, associated floorplans are updated. Using refresh_from_db() is the right approach since the save() method uses .update() which bypasses Django model instances.

Consider adding an additional assertion to verify that floorplans belonging to other locations are not inadvertently affected:

💡 Optional: verify isolation

         floorplan.refresh_from_db()
         self.assertEqual(floorplan.organization_id, org_b.id)
+
+        # Verify floorplans on other locations are unaffected
+        other_floorplan = self._create_floorplan()
+        self.assertNotEqual(other_floorplan.organization_id, org_b.id)

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c5c4a5 and 4f0dca8.

📒 Files selected for processing (2)

• openwisp_controller/geo/base/models.py
• openwisp_controller/geo/tests/test_models.py

🧰 Additional context used

🧠 Learnings (4)

📚 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/tests/test_models.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/tests/test_models.py
• openwisp_controller/geo/base/models.py

📚 Learning: 2026-01-15T15:07:17.354Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

• openwisp_controller/geo/tests/test_models.py
• openwisp_controller/geo/base/models.py

📚 Learning: 2026-01-15T14:06:53.460Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.

Applied to files:

• openwisp_controller/geo/base/models.py

🧬 Code graph analysis (1)

openwisp_controller/geo/tests/test_models.py (3)

openwisp_controller/geo/tests/utils.py (1)

• _create_floorplan (39-43)

tests/openwisp2/sample_users/models.py (1)

• Organization (32-36)

openwisp_controller/geo/base/models.py (2)

• organization_id (84-85)
• save (16-34)

⏰ 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.1.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.12 | 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.10 | django~=4.2.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=4.2.0

🔇 Additional comments (3)

openwisp_controller/geo/base/models.py (1)

43-48: LGTM!

The clean() method properly synchronizes the floorplan's organization with its location's organization during validation. The hasattr check correctly handles cases where the location FK hasn't been set yet, and _validate_org_relation ensures consistency.

openwisp_controller/geo/tests/test_models.py (2)

1-4: LGTM!

The import changes are appropriate - swapper is needed for loading the Organization model in the new test, and the inheritance from loci_test_models.BaseTestModels maintains the test framework structure.

---

15-19: LGTM!

The class signature correctly inherits from loci_test_models.BaseTestModels with TestGeoMixin taking precedence in the MRO, which allows project-specific test utilities to override base behavior as needed.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nemesifier]

Thanks @xoaryaa, this is a reasonable fix for #385 and doing it in Location.save() (rather than only in the admin) is the right call, since it keeps floorplans consistent for API and programmatic changes too. Tests are included, good.

A few notes:

1. Use the project's model loader. You added from django.apps import apps and do app_label, model_name = get_model_name(...).split("."); apps.get_model(...). The codebase convention is swapper.load_model("geo", "FloorPlan"), which is cleaner and handles swapping consistently. Please switch to that. Also fix the import ordering.
2. Extra query per save. When self.pk is set you do a .values_list(...) query on every save to detect an org change. That is acceptable, but you could avoid it by tracking the original org on load, or only running it when organization_id is actually in the changed fields. Not blocking.
3. .update() bypasses validation/signals on FloorPlan. That is fine here because you are propagating a known-consistent org value, just be aware no FloorPlan save signals fire. If anything downstream depends on FloorPlan post_save for org changes, it will not run. Worth a moment's thought.
4. Confirm the test asserts the floorplan org actually changes after the location org changes.

Solid fix. Address point 1 and confirm the test, and this is good to merge.