[feature] Allow showing moving device in dashboard map #664

[dee077]

Adds the ability to animate a node's position by listening to the websock endpoint

Fixes #664

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 #664.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

Walkthrough

Adds real-time device location updates to the map UI by introducing listenForLocationUpdates(map) which opens a ReconnectingWebSocket to ws/loci/location/, listens for location messages, moves the corresponding node marker in real time, and updates/repositions any open Leaflet popup after map.render(). Also fixes series selection predicate for echarts series. Supporting changes update tests (URL trailing slash expectations), test server settings and URLs for selenium tests, and replace an external openwisp-controller dependency reference.

Sequence Diagram(s)

sequenceDiagram
    participant Browser as Client (Browser)
    participant WS as ReconnectingWebSocket<br/>ws/loci/location/
    participant Map as Map Component
    participant Popup as Leaflet Popup

    Browser->>WS: Open connection (after map.render)
    activate WS
    WS-->>Browser: Connection established

    loop on location update
        WS->>Browser: Send location update (device_id, lat, lon)
        Browser->>Map: Find marker by device_id
        Map->>Map: Move marker to (lat, lon) (with transition)
        alt popup for device is open
            Browser->>Popup: Hide current popup
            Browser->>Popup: Reposition/show popup at new coords
        end
    end

    Browser->>WS: Close connection
    deactivate WS

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main feature: enabling dynamic display of moving devices on the dashboard map, which matches the core change across multiple files.
Description check	✅ Passed	The PR description covers the main objective and references the issue, but lacks details on implementation approach, WebSocket usage, and testing status despite unchecked test boxes.
Linked Issues check	✅ Passed	The PR implements WebSocket subscription for real-time location updates and marker repositioning (#664), matching the core requirements of subscribing to the location stream and animating device movement dynamically.
Out of Scope Changes check	✅ Passed	All changes directly support the feature: device-map.js adds WebSocket logic for real-time updates, test files adapt to ChannelsLiveServerTestCase infrastructure, and dependencies/settings/URLs support WebSocket testing infrastructure.

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

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/664-moving-devices

---

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]

@coderabbitai review

[nemesifier]

The selenium tests are failing due to Alert Text: Something went wrong while loading the charts, did you notice that @dee077?

[dee077]

Need help!

While changing from StaticLiveServerTestCase to ChannelsLiveServerTestCase I have faced these issues.
Here we are setting OPENWISP_MONITORING_API_BASEURL = "http://testserver"

openwisp-monitoring/tests/openwisp2/settings.py

Lines 221 to 227 in a9f02e4

	if TESTING:
	OPENWISP_MONITORING_MAC_VENDOR_DETECTION = False
	OPENWISP_MONITORING_API_URLCONF = "openwisp_monitoring.urls"
	OPENWISP_MONITORING_API_BASEURL = "http://testserver"
	# for testing AUTO_IPERF3
	OPENWISP_MONITORING_AUTO_IPERF3 = True

Now we are making this full url like this and pushed to device_map.html

openwisp-monitoring/openwisp_monitoring/device/apps.py

Lines 383 to 388 in a9f02e4

	if MONITORING_API_BASEURL:
	device_list_url = urljoin(MONITORING_API_BASEURL, str(device_list_url))
	loc_geojson_url = urljoin(MONITORING_API_BASEURL, str(loc_geojson_url))
	indoor_coordinates_list_url = urljoin(
	MONITORING_API_BASEURL, str(indoor_coordinates_list_url)
	)

Now the issue is we made have already made a fix for this which worked for StaticLiveServerTestCase

openwisp-monitoring/openwisp_monitoring/tests/test_selenium.py

Lines 41 to 96 in a9f02e4

	class SeleniumTestMixin(BaseSeleniumTestMixin):
	@classmethod
	def setUpClass(cls):
	"""
	Sets up the necessary configurations for the test environment, ensuring
	that the dashboard templates render correctly during Selenium tests.
	During testing, the `OPENWISP_MONITORING_API_BASEURL` is set to
	`http://testserver`, a dummy value for the test environment. The dashboard
	templates are registered in the `AppConfig.ready` method, and these
	templates depend on specific URLs being correctly configured.
	Since mocking the `OPENWISP_MONITORING_API_BASEURL` does not update the
	URLs in the already registered dashboard templates, this method manually
	adjusts the template contexts to ensure they contain the correct URLs.
	"""
	super().setUpClass()
	cls._dashboard_map_context = DASHBOARD_TEMPLATES[0][1].copy()
	cls._dashboard_timeseries_context = DASHBOARD_TEMPLATES[55][1].copy()
	DASHBOARD_TEMPLATES[0][1] = {
	"monitoring_device_list_url": reverse(
	"monitoring:api_location_device_list",
	args=["000"],
	),
	"monitoring_location_geojson_url": reverse(
	"monitoring:api_location_geojson"
	),
	"monitoring_indoor_coordinates_list": reverse(
	"monitoring:api_indoor_coordinates_list", args=["000"]
	),
	"monitoring_labels": {
	"ok": device_app_settings.HEALTH_STATUS_LABELS["ok"],
	"problem": device_app_settings.HEALTH_STATUS_LABELS["problem"],
	"critical": device_app_settings.HEALTH_STATUS_LABELS["critical"],
	"unknown": device_app_settings.HEALTH_STATUS_LABELS["unknown"],
	"deactivated": device_app_settings.HEALTH_STATUS_LABELS["deactivated"],
	},
	}
	DASHBOARD_TEMPLATES[55][1]["api_url"] = reverse(
	"monitoring_general:api_dashboard_timeseries"
	)
	@classmethod
	def tearDownClass(cls):
	super().tearDownClass()
	DASHBOARD_TEMPLATES[0][1] = cls._dashboard_map_context
	DASHBOARD_TEMPLATES[55][1] = cls._dashboard_timeseries_context
	def login(self, username=None, password=None, driver=None):
	super().login(username, password, driver)
	# Workaround for JS logic in chart-utils.js
	# which fails to perform a XHR request
	# during automated tests, it seems that the
	# lack of pause causes the request to fail randomly
	sleep(0.5)

This overriding of DASHBOARD_TEMPLATES make a fix for this issue but on changing it to ChannelsLiveServerTestCase these are never applied because the main change is asgi server and test runner are different processes now, unlike StaticLiveServerTestCase they dont share the same context
Note: This I found this out with ChatGPT and channels docs, not sure if this is the exact case

Things I have tried

• Making OPENWISP_MONITORING_API_BASEURL = None works for selenium tests but other tests fail
• Moving the url join logic to frontend like device_map.html but then not sure this is the right approach as in prod env OPENWISP_MONITORING_API_BASEURL can be different in than window.location

@nemesifier @pandafy can you help me find a fix for this!

[nemesifier]

@dee077 I calculate that removing OPENWISP_MONITORING_API_BASEURL altogether only for selenium tests should work. Try and let me know how it goes.

If it works, we can avoid setting OPENWISP_MONITORING_API_BASEURL when using the selenium test tag.

[nemesifier]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

openwisp_monitoring/device/static/monitoring/js/device-map.js (1)

561-572: Remove duplicate seriesIndex property.

The seriesIndex property is defined twice (lines 564 and 570), which causes the first definition to be ignored. Remove the duplicate on line 570.

🔎 Proposed fix

         const params = {
           componentType: "series",
           componentSubType: series.type,
           seriesIndex: seriesIndex,
           dataIndex: index,
           data: {
             ...series.data[index],
             node: nodeData,
           },
-          seriesIndex: seriesIndex,
           seriesType: series.type,
         };

🧹 Nitpick comments (3)

tests/openwisp2/urls.py (1)

29-36: Clarify why duplicate media serving is necessary.

Media files are already served on line 22 via static(settings.MEDIA_URL, ...). This conditional block adds another media serving route specifically for selenium tests.

Please clarify:

1. Why is the duplicate media serving pattern necessary?
2. Is there a conflict between the static files helper and selenium test requirements?
3. Could this be simplified or documented?

Additionally, parsing sys.argv directly can be fragile if test runners change their argument format. Consider using a settings flag instead if this is a long-term requirement.

openwisp_monitoring/device/static/monitoring/js/device-map.js (1)

603-610: Review popup visibility toggle approach.

The implementation hides and shows the popup during location updates. While this prevents visual glitches, consider if updating the popup content (line 608) is sufficient without hiding it. The hide/show may cause flashing.

Test the behavior with and without the hide/show calls to determine if they're necessary for a smooth user experience.

openwisp_monitoring/tests/test_selenium.py (1)

640-641: Consider removing redundant sleep before wait_for.

The sleep(0.5) on line 641 appears redundant since wait_for on line 642 already handles waiting for the element to be clickable. Consider removing the sleep to improve test speed.

🔎 Proposed refactor

-        self._open_popup("_owGeoMap", location.id)
-        sleep(0.5)
+        self._open_popup("_owGeoMap", location.id)
         self.wait_for(
             "element_to_be_clickable", By.CSS_SELECTOR, ".map-detail .floorplan-btn"
         ).click()

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9f02e4 and 484d84c.

⛔ Files ignored due to path filters (1)

• openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.js is excluded by !**/*.min.js

📒 Files selected for processing (6)

• openwisp_monitoring/device/static/monitoring/js/device-map.js
• openwisp_monitoring/device/tests/test_admin.py
• openwisp_monitoring/tests/test_selenium.py
• requirements.txt
• tests/openwisp2/settings.py
• tests/openwisp2/urls.py

🧰 Additional context used

🪛 Biome (2.1.2)

openwisp_monitoring/device/static/monitoring/js/device-map.js

[error] 564-565: This property is later overwritten by an object member with the same name.

Overwritten with this property.

If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.

(lint/suspicious/noDuplicateObjectKeys)

🪛 Ruff (0.14.10)

openwisp_monitoring/tests/test_selenium.py

740-740: Possible hardcoded password assigned to argument: "password"

(S106)

---

744-744: Possible hardcoded password assigned to argument: "password"

(S106)

---

871-871: Possible hardcoded password assigned to argument: "password"

(S106)

---

875-875: Possible hardcoded password assigned to argument: "password"

(S106)

---

906-906: Possible hardcoded password assigned to argument: "password"

(S106)

---

910-910: Possible hardcoded password assigned to argument: "password"

(S106)

🔇 Additional comments (13)

openwisp_monitoring/device/tests/test_admin.py (2)

133-133: LGTM: Relative path change aligns with test infrastructure updates.

The change from absolute to relative API path correctly addresses the test infrastructure issues mentioned in the PR objectives when using ChannelsLiveServerTestCase.

---

140-141: LGTM: Consistent relative path usage.

The inline JavaScript URL assertions correctly use relative paths, consistent with the test infrastructure changes across the codebase.

tests/openwisp2/settings.py (3)

229-229: LGTM: Relative API paths for tests.

Setting OPENWISP_MONITORING_API_BASEURL to None during testing correctly enables relative path resolution, addressing the test infrastructure issues described in the PR objectives.

---

214-222: No action needed—Redis is already properly configured.

Redis is running and accessible in CI/CD (configured in .github/workflows/ci.yml lines 22-26), Redis is configured in docker-compose.yml for both development and Docker deployments, and developers are already instructed to start Redis via docker compose up -d redis influxdb in the developer installation documentation. The code properly handles both scenarios: selenium tests use RedisChannelLayer (requiring Redis), while non-selenium tests fall back to InMemoryChannelLayer.

---

23-26: Database cleanup is automatically handled by Django's TransactionTestCase.

The test database configuration is correct as-is. Django's TransactionTestCase (used in the test suite) automatically wraps each test in a transaction and rolls back after completion, providing built-in isolation and cleanup. Tests are run sequentially in CI, so parallel execution is not a current concern. No additional cleanup hooks or modifications are needed.

If parallel test execution is introduced in the future (e.g., with pytest-xdist), ensure each parallel worker has its own database or that --create-db and --reuse-db flags are configured appropriately.

openwisp_monitoring/device/static/monitoring/js/device-map.js (1)

597-599: Verify WebSocket endpoint availability with deployment configuration.

The WebSocket endpoint /ws/loci/locations/ is hardcoded in the JavaScript but its routing is delegated to the external openwisp_controller package via get_routes() in Django Channels configuration. Confirm that this endpoint is properly exposed in all deployment environments by verifying:

1. The openwisp_controller package version includes the loci locations WebSocket consumer
2. The endpoint is registered in the Channels routing configuration for each environment
3. ASGI application is correctly configured in production deployments

openwisp_monitoring/tests/test_selenium.py (7)

5-11: LGTM: Import additions support new features.

The added imports appropriately support ChannelsLiveServerTestCase for WebSocket testing, permission management, geometry handling, and organization isolation features.

Also applies to: 28-28, 44-45

---

69-79: LGTM: Correct use of reverse_lazy for class-level URL resolution.

Using reverse_lazy in setUpClass is appropriate since URLs need to be resolved lazily when the class is set up, before URL configuration is fully loaded.

---

317-322: LGTM: Appropriate test class inheritance for WebSocket support.

Switching to ChannelsLiveServerTestCase is necessary for testing WebSocket functionality, and adding TestOrganizationMixin provides organization-related test utilities needed for the new isolation tests.

---

330-335: LGTM: Good practice to mark helper method as private.

Renaming open_popup to _open_popup correctly indicates this is a private helper method.

---

706-706: LGTM: Correct URL encoding for indoor map ID.

Using quote_plus to encode the indoor map ID in the URL fragment is appropriate for handling special characters.

---

717-719: LGTM: More robust wait condition.

Switching to wait_for_visibility provides better synchronization than find_element.

---

738-747: LGTM: Well-structured permission test.

The test correctly verifies that users without devicelocation permissions see the appropriate "No map data to show." message.

[pandafy]

I used ChatGPT to generate a Python script which simulates a device updating it's location. This can aid in testing.

[nemesifier]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[nemesifier]

I tried it on staging and I am very happy of the result.

Here's a recording:

Screencast.from.2026-01-29.10-38-36.mp4

A few things I noticed:

• When I opened the indoor map, it took a while to finish loading. For a brief moment everything appeared gray with no loading indicator, which looks odd. Can we show a loading indicator while the indoor map loads? Same when switching floors.
• The "Open Location on Map" button doesn't clearly communicate its function. Let's use "View on General Map" instead, and add a title attribute: "Opens the general map view focused on this location"
• The "Open Device on Map" button is even less clear. Let's use "View on General Indoor Map" instead, and add a title attribute with a tooltip like: "Opens the general map view and drills down to this device's specific floor plan within the general map."

[dee077]

Screencast.from.2026-01-30.03-37-46.webm

The issue was that creating the loader inside the same div element provided for the netjsongraph instance was a bad idea, as internally it clears all its child elements

Fixing for #704