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

[fix] Fixed monitoring charts not loading from device admin #329 #349

Merged
merged 5 commits into from
Aug 28, 2024

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Aug 27, 2024

Upgraded OpenWISP modules

Fixes #329


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.
  • I have updated the documentation.

Blockers

@@ -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
Copy link
Member Author

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.

@@ -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
Copy link
Member Author

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()
Copy link
Member

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

Copy link
Member Author

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.

@pandafy
Copy link
Member Author

pandafy commented Aug 28, 2024

@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.

@nemesifier nemesifier merged commit 8c545b2 into master Aug 28, 2024
3 checks passed
@nemesifier nemesifier deleted the issues/329-monitoring-chart branch August 28, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Monitoring charts do not load from device admin
2 participants