-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
[fix] Fixed monitoring charts not loading from device admin #329 #349
Conversation
Upgraded OpenWISP modules Fixes #329
images/openwisp_base/Dockerfile
Outdated
@@ -27,16 +27,16 @@ ENV PYTHONPATH=/home/openwisp/.local/lib/python3.10/site-packages | |||
|
|||
RUN pip install --no-cache-dir --user --upgrade pip~=24.1.2 setuptools~=70.3.0 wheel~=0.43.0 | |||
# TODO: Remove when next version of openwisp-monitoring is released | |||
ARG OPENWISP_MONITORING_SOURCE=https://github.com/openwisp/openwisp-monitoring/tarball/fb2814b6dd8213ddb45a8497cf41dfb10eee04a2 | |||
ARG OPENWISP_MONITORING_SOURCE=https://github.com/openwisp/openwisp-monitoring/tarball/fix-device-chart-api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Update this before merging.
images/openwisp_base/Dockerfile
Outdated
@@ -59,13 +59,13 @@ ARG OPENWISP_NOTIFICATION_SOURCE=default | |||
RUN if [ "$OPENWISP_NOTIFICATION_SOURCE" != "default" ] ; then \ | |||
pip install --no-cache-dir --user --upgrade ${OPENWISP_NOTIFICATION_SOURCE}; \ | |||
fi | |||
ARG OPENWISP_USERS_SOURCE=https://github.com/openwisp/openwisp-users/tarball/1130d5a4cbde0a09f57df638bba45bd53ea27192 | |||
ARG OPENWISP_USERS_SOURCE=https://github.com/openwisp/openwisp-users/tarball/fix-django-allauth-dep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Update this before merging.
self._wait_for_element() | ||
self.get_resource('test-device', '/admin/config/device/') | ||
self._wait_for_element() | ||
self.base_driver.find_element(By.CSS_SELECTOR, 'ul.tabs li.charts').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding only this test on the latest master and it fails here, I was expecting it to fail in the try/except block below, but that's not the case. Is this intended or not?
Failure output:
ERROR: test_device_monitoring_charts (__main__.TestServices)
----------------------------------------------------------------------
Traceback (most recent call last):
File "tests/runtests.py", line 198, in test_device_monitoring_charts
self.base_driver.find_element(By.CSS_SELECTOR, 'ul.tabs li.charts').click()
File "/lib/python3.8/site-packages/selenium/webdriver/remote/webdriver.py", line 748, in find_element
return self.execute(Command.FIND_ELEMENT, {"using": by, "value": value})["value"]
File "/lib/python3.8/site-packages/selenium/webdriver/remote/webdriver.py", line 354, in execute
self.error_handler.check_response(response)
File "/lib/python3.8/site-packages/selenium/webdriver/remote/errorhandler.py", line 229, in check_response
raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.NoSuchElementException: Message: no such element: Unable to locate element: {"method":"css selector","selector":"ul.tabs li.charts"}
(Session info: chrome-headless-shell=127.0.6533.99); For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#no-such-element-exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to replicate this error locally. Did you also copy the changes in data.py and utis.py?
Anyway, I have improved the test to make it more robust.
@nemesifier and I concluded to use dashboard service (dashboard.openwisp.org) instead of the API service (api.openwisp.org) for making API request from the Django admin. This solution resolves the issue where session cookies are not sent with cross-origin requests, as the request will now be made to the same origin. After reviewing OpenWISP's code, I observed that all modules are properly configured to make API requests from the Django admin to the API service, and they function correctly. The noted inconsistencies in my initial investigation were indeed inconsistencies. The notification widget was making requests to the dashboard service due to a missing configuration. The same issue applied to the command widget. The dashboard monitoring charts did not originally account for using the API service for API requests, which is why they were making requests to the dashboard service. In openwisp/openwisp-monitoring#609, I have adjusted the logic to support this functionality. |
Upgraded OpenWISP modules
Fixes #329
Checklist
Blockers