django-loci
django-loci copied to clipboard
[fix] Address field real-time update #169
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
listenForLocationUpdatesfunction to handle and broadcast address changes.
Issues
-
Inheriting from
ChannelLiveServerTestCaseis causing test failures in Django 5.2, which is a new release, and a fix has yet to be released for it. -
Adding
ChannelLiveServerTestCaseprevents running tests in parallel with the --parallel flag, as it relies on live WebSocket connections that cannot run concurrently.
Demo
coverage: 99.819%. remained the same when pulling a1ba01de11b500a0e8b02856caf70279c135e61e on dee077:issues/169-real-time-address-update into 31b23bf700855f9af8de1978a0e60d82889228be on openwisp:master.
I installed this PR with openwisp-controller and performed the following test:
- Opened a location admin in the browser and ensured that the websocket coonnection is working
- Updated the
geometryfield 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
addressfield 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
@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 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.
Updates:
- Added selenium tests.
- Updated ci to run unit tests and selenium tests separately.
Updates
- Fixed test db logic as suggested
- Fixed the way of running tests in ci by adding a script
runtests, like other modules.