django-loci icon indicating copy to clipboard operation
django-loci copied to clipboard

[fix] Address field real-time update #169

Open dee077 opened this issue 7 months ago • 4 comments

Added address update handling in the websocket message and updated listenForLocationUpdates logic accordingly.

Fixes #169

Checklist

  • [x] I have read the OpenWISP Contributing Guidelines.
  • [x] 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.

Reference to Existing Issue

Closes #169.

Description of Changes

  • Updated the real-time update logic to include address field updates via WebSocket.
  • Modified the listenForLocationUpdates function to handle and broadcast address changes.

Issues

  • Inheriting from ChannelLiveServerTestCase is causing test failures in Django 5.2, which is a new release, and a fix has yet to be released for it.

  • Adding ChannelLiveServerTestCase prevents running tests in parallel with the --parallel flag, as it relies on live WebSocket connections that cannot run concurrently.

Demo

real-time-address.webm

dee077 avatar Apr 27 '25 17:04 dee077

Coverage Status

coverage: 99.819%. remained the same when pulling a1ba01de11b500a0e8b02856caf70279c135e61e on dee077:issues/169-real-time-address-update into 31b23bf700855f9af8de1978a0e60d82889228be on openwisp:master.

coveralls avatar Apr 27 '25 17:04 coveralls

I installed this PR with openwisp-controller and performed the following test:

  1. Opened a location admin in the browser and ensured that the websocket coonnection is working
  2. Updated the geometry field of the location using the REST API with a PATCH request.

Expected outcome The location admin updated the pin on the map according to the received data from websocket. But, the address field was still showing the old value.

This occurs because the JS updates the address field in Django admin if the user changes the pin on the map.

https://github.com/openwisp/django-loci/blob/c6707ef99bf45a9f840030f1b8a096c34614aded/django_loci/static/django-loci/js/loci.js#L508-L529

But, the backend does not perform such operation on save. @nemesifier is this intentional? If so, shall we update the logic in this PR to update the address field using the JS?

@pandafy I think the REST API is a different matter and we could discuss that separately. I am not sure now whether we would want to do that automatically or not

nemesifier avatar May 09 '25 00:05 nemesifier

@pandafy I think the REST API is a different matter and we could discuss that separately. I am not sure now whether we would want to do that automatically or not

Let's say a mobile device updated it's coordinates. The pin on the map will get updated, but the address field will keep showing the old value. Maybe, we can trigger an update to the address field in the admin using the JS logic.

pandafy avatar May 09 '25 09:05 pandafy

@pandafy I think the REST API is a different matter and we could discuss that separately. I am not sure now whether we would want to do that automatically or not

Let's say a mobile device updated it's coordinates. The pin on the map will get updated, but the address field will keep showing the old value. Maybe, we can trigger an update to the address field in the admin using the JS logic.

It's a good point.

A device which moves around and uses the API to update its location should be able to ensure the address field is either updated or cleared out, depending on their implementation.

In this change, we are focusing on the admin to make the user experience consistent when using the web admin, the REST API has a different purpose and I believe it's the responsibility of the API client to decide what to do with the address field (clear it, update it or leave it as is), so I wouldn't involve the REST API in this change, unless I am missing something, if that's the case let me know.

In the video I recorded when creating the issue I flagged the "is mobile" checkbox without thinking about it, but I am more concerned with regular static locations, which usually have the address field filled, while mobile locations usually don't have it because they keep moving around.

~~I think we could tweak the code to update the address field only if it's not empty**, that should be enough to not mess with mobile locations~~

I think the actual changes are fine, what I was asking in the issue is to simply send the updated address field in the broadcasted changes, so the UI can update the address in other windows. No REST API involved.

nemesifier avatar May 09 '25 16:05 nemesifier

Updates:

  • Added selenium tests.
  • Updated ci to run unit tests and selenium tests separately.

dee077 avatar Aug 27 '25 11:08 dee077

Updates

  • Fixed test db logic as suggested
  • Fixed the way of running tests in ci by adding a script runtests, like other modules.

dee077 avatar Oct 05 '25 18:10 dee077