Skip to content

Conversation

@DragnEmperor
Copy link
Member

@DragnEmperor DragnEmperor commented Apr 26, 2025

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.

Reference to Existing Issue

Closes #560.

Description of Changes

For readonly mode the field name used to track geometry field is different leading to its json value not being parsed, hence leading to unsaved_changes alert. A small change of addition of the said field in 'jsonValues' of 'unsaved_changes.js' Fixed it.
Also increased the timeout for waiting of success message present for selenium test of device admin to tackle TimeoutException.

@DragnEmperor DragnEmperor changed the title [chores] Fixed unsaved changes alert for readonly map #560 [fix] Fixed unsaved changes alert for readonly map #560 Apr 26, 2025
For readonly mode the field name used to track geometry field
is different leading to its json value not being parsed, hence
leading to unsaved_changes alert. A small change of addition
of the said field in 'jsonValues' of 'unsaved_changes.js' Fixed
it.
Also increased the timeout for waiting of success message present
for selenium test of device admin to tackle TimeoutException.

Closes openwisp#560

Signed-off-by: DragnEmperor <[email protected]>
Aryamanz29
Aryamanz29 previously approved these changes Apr 28, 2025
Copy link
Member

@devkapilbansal devkapilbansal left a comment

Choose a reason for hiding this comment

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

Hi @DragnEmperor can we have a test for this to ensure this feature might not break in future?

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.

@pandafy did we already have an unsaved changes alert test somewhere or not yet?

@DragnEmperor
Copy link
Member Author

DragnEmperor commented Apr 30, 2025

@pandafy did we already have an unsaved changes alert test somewhere or not yet?

@nemesifier I checked the tests and we do have test case for unsaved changes alert. But for readonly user the field names are a bit different which lead to the alert being triggered.
I am adding the same for read only user

@pandafy
Copy link
Member

pandafy commented Apr 30, 2025

@pandafy did we already have an unsaved changes alert test somewhere or not yet?

@nemesifier are you referencing to this test?

class TestDeviceAdminUnsavedChanges(
SeleniumTestMixin,
CreateConfigTemplateMixin,
StaticLiveServerTestCase,
):
browser = 'chrome'
def test_unsaved_changes(self):
"""
Execute this test using Chrome instead of Firefox.
Firefox automatically accepts the beforeunload alert, which makes it
impossible to test the unsaved changes alert.
"""
self.login()
device = self._create_config(organization=self._get_org()).device
path = reverse('admin:config_device_change', args=[device.id])
with self.subTest('Alert should not be displayed without any change'):
self.open(path)
self.hide_loading_overlay()
try:
WebDriverWait(self.web_driver, 1).until(EC.alert_is_present())
except TimeoutException:
pass
else:
self.fail('Unsaved changes alert displayed without any change')
with self.subTest('Alert should be displayed after making changes'):
# The WebDriver automatically accepts the
# beforeunload confirmation dialog. To verify the message,
# we log it to the console and check its content.
#
# our own JS code sets e.returnValue when triggered
# so we just need to ensure it's set as expected
self.web_driver.execute_script(
'django.jQuery(window).on("beforeunload", function(e) {'
' console.warn(e.returnValue); });'
)
# simulate hand gestures
self.find_element(by=By.TAG_NAME, value='body').click()
self.find_element(by=By.NAME, value='name').click()
# set name
self.find_element(by=By.NAME, value='name').send_keys('new.device.name')
# simulate hand gestures
self.find_element(by=By.TAG_NAME, value='body').click()
self.web_driver.refresh()
for entry in self.get_browser_logs():
if (
entry['level'] == 'WARNING'
and "You haven\'t saved your changes yet!" in entry['message']
):
break
else:
self.fail('Unsaved changes code was not executed.')

@nemesifier
Copy link
Member

@pandafy did we already have an unsaved changes alert test somewhere or not yet?

@nemesifier are you referencing to this test?

class TestDeviceAdminUnsavedChanges(
SeleniumTestMixin,
CreateConfigTemplateMixin,
StaticLiveServerTestCase,
):
browser = 'chrome'
def test_unsaved_changes(self):
"""
Execute this test using Chrome instead of Firefox.
Firefox automatically accepts the beforeunload alert, which makes it
impossible to test the unsaved changes alert.
"""
self.login()
device = self._create_config(organization=self._get_org()).device
path = reverse('admin:config_device_change', args=[device.id])
with self.subTest('Alert should not be displayed without any change'):
self.open(path)
self.hide_loading_overlay()
try:
WebDriverWait(self.web_driver, 1).until(EC.alert_is_present())
except TimeoutException:
pass
else:
self.fail('Unsaved changes alert displayed without any change')
with self.subTest('Alert should be displayed after making changes'):
# The WebDriver automatically accepts the
# beforeunload confirmation dialog. To verify the message,
# we log it to the console and check its content.
#
# our own JS code sets e.returnValue when triggered
# so we just need to ensure it's set as expected
self.web_driver.execute_script(
'django.jQuery(window).on("beforeunload", function(e) {'
' console.warn(e.returnValue); });'
)
# simulate hand gestures
self.find_element(by=By.TAG_NAME, value='body').click()
self.find_element(by=By.NAME, value='name').click()
# set name
self.find_element(by=By.NAME, value='name').send_keys('new.device.name')
# simulate hand gestures
self.find_element(by=By.TAG_NAME, value='body').click()
self.web_driver.refresh()
for entry in self.get_browser_logs():
if (
entry['level'] == 'WARNING'
and "You haven\'t saved your changes yet!" in entry['message']
):
break
else:
self.fail('Unsaved changes code was not executed.')

@pandafy yes thanks! @DragnEmperor can you add a similar test in the geo app which replicates the bug (test should fail) when the fix is missing please?

@DragnEmperor
Copy link
Member Author

@pandafy did we already have an unsaved changes alert test somewhere or not yet?

@nemesifier are you referencing to this test?

class TestDeviceAdminUnsavedChanges(
SeleniumTestMixin,
CreateConfigTemplateMixin,
StaticLiveServerTestCase,
):
browser = 'chrome'
def test_unsaved_changes(self):
"""
Execute this test using Chrome instead of Firefox.
Firefox automatically accepts the beforeunload alert, which makes it
impossible to test the unsaved changes alert.
"""
self.login()
device = self._create_config(organization=self._get_org()).device
path = reverse('admin:config_device_change', args=[device.id])
with self.subTest('Alert should not be displayed without any change'):
self.open(path)
self.hide_loading_overlay()
try:
WebDriverWait(self.web_driver, 1).until(EC.alert_is_present())
except TimeoutException:
pass
else:
self.fail('Unsaved changes alert displayed without any change')
with self.subTest('Alert should be displayed after making changes'):
# The WebDriver automatically accepts the
# beforeunload confirmation dialog. To verify the message,
# we log it to the console and check its content.
#
# our own JS code sets e.returnValue when triggered
# so we just need to ensure it's set as expected
self.web_driver.execute_script(
'django.jQuery(window).on("beforeunload", function(e) {'
' console.warn(e.returnValue); });'
)
# simulate hand gestures
self.find_element(by=By.TAG_NAME, value='body').click()
self.find_element(by=By.NAME, value='name').click()
# set name
self.find_element(by=By.NAME, value='name').send_keys('new.device.name')
# simulate hand gestures
self.find_element(by=By.TAG_NAME, value='body').click()
self.web_driver.refresh()
for entry in self.get_browser_logs():
if (
entry['level'] == 'WARNING'
and "You haven\'t saved your changes yet!" in entry['message']
):
break
else:
self.fail('Unsaved changes code was not executed.')

@pandafy yes thanks! @DragnEmperor can you add a similar test in the geo app which replicates the bug (test should fail) when the fix is missing please?

Sure!

@coveralls
Copy link

coveralls commented Apr 30, 2025

Coverage Status

coverage: 98.855%. remained the same
when pulling f2ce2fe on DragnEmperor:issues/560-readonly-map
into 751e9b2 on openwisp:master.

Added a selenium test case for readonly user to
check if unsaved changes alert is triggered on
device admin page.

Signed-off-by: DragnEmperor <[email protected]>
@nemesifier nemesifier added the bug label May 2, 2025
@nemesifier nemesifier merged commit e1d4e69 into openwisp:master May 2, 2025
12 checks passed
@nemesifier nemesifier moved this from To do (general) to Done in OpenWISP Contributor's Board Dec 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[bug] Map tab doesn't work in readonly mode

6 participants