Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] Websocket connect error #63 #64

Merged
merged 1 commit into from Jul 21, 2020

Conversation

atb00ker
Copy link
Member

@atb00ker atb00ker commented Jul 7, 2020

Closes #63

@coveralls
Copy link

coveralls commented Jul 7, 2020

Coverage Status

Coverage remained the same at 99.776% when pulling cbb0e67 on atb00ker:issues/63-websocket-error into 08227a0 on openwisp:master.

@atb00ker atb00ker self-assigned this Jul 7, 2020
@atb00ker atb00ker added the bug label Jul 7, 2020
@atb00ker atb00ker added this to In progress in [GSoC20] Merge modules via automation Jul 7, 2020
@atb00ker atb00ker moved this from In progress to Open Pull-Requests in [GSoC20] Merge modules Jul 7, 2020
@atb00ker atb00ker moved this from Open Pull-Requests to Ready for review in [GSoC20] Merge modules Jul 7, 2020
[GSoC20] Merge modules automation moved this from Ready for review to Open Pull-Requests Jul 7, 2020
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reporting this.

The expected behavior (which should happen in the device page), is:

  • if mobile is checked, connect to the websocket server in order to receive the location data
  • if no geographic position is present, show the message in $noLocationDiv (no location data received yet)

The actual behavior with this patch is still not satisfied, because it looks like location.val() is always falsy in the location page, hence it cannot work.

We need to allow make sure location.val() contains the ID of the location also in the location page.
Then we need to make sure the message is correctly displayed.

This change avoids the JS error but kind of hides the fact that the expected behavior is not honored.

Copy link
Member Author

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't notice the no location case error in the previous approach, this should fix it:

@@ -30,7 +30,7 @@ input[type=text]{ width: 320px }
color: #333;
}
.no-location{
padding: 20px 0 0;
padding: 10px 0 10px 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this because in indoor device with no-location message, there is no bottom padding which looks bad.

@@ -147,7 +147,7 @@ django.jQuery(function ($) {
});
// name not relevant in mobile locations
$name.parents('.form-row').hide();
$('.no-location').show();
if (!$geometryTextarea.val()) { $('.no-location').show(); }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If $geometryTextarea.val() has a value, it's not hidden. Hence, map and the message appear together, this condition will help avoid that.

if ($location.val()) {
$locationSelectionRow.hide();
$geoEdit.show();
isNew = false;
pk = $location.val();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing pk value here should do the trick.

@atb00ker atb00ker moved this from Open Pull-Requests to Ready for review in [GSoC20] Merge modules Jul 8, 2020
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, it works! Thanks 👍

@nemesifier nemesifier merged commit 6268d79 into openwisp:master Jul 21, 2020
[GSoC20] Merge modules automation moved this from Ready for review to Done Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[bug] websocket connect error in location change form page
3 participants