[change] Load netjsongraph.js from openwisp-utils #272#298
[change] Load netjsongraph.js from openwisp-utils #272#298kunalverma2512 wants to merge 2 commits into
Conversation
Removed redundant netjsongraph static files from the repository to prevent duplication. Updated admin.py and base.html to load the unbundled netjsongraph and echarts libraries directly from openwisp-utils. Fixes openwisp#272
📝 WalkthroughWalkthroughThis PR migrates netjsongraph static assets from locally duplicated paths to centralized Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @nemesifier, the CI checks have failed as expected. As noted in the PR description, the Selenium tests are throwing However, while checking the logs, I also noticed a completely unrelated dependency resolution error that is crashing the test matrix: It looks like the upstream |
There was a problem hiding this comment.
Please force the CI workflow to install your fork (from openwisp/openwisp-utils#675), eg:
# add this to the github actions CI YAML setup step, as last command
pip install --force-reinstall --no-deps https://github.com/kunalverma2512/openwisp-utils/tarball/feature/517-add-netjsongraph-static
Then, update the PR description with a GIF showing the browser network tab open, where the file netjsongraph.echarts.min.js is visibly being loaded (as expected) and the network topology graph UI (which is handled by netjsongraph.js) loads properly.
Once everything is done correctly the CI will pass. Manual testing on your end is needed to accomplish this.
Added the maintainer-requested pip install command to ci.yml to fetch the unbundled netjsongraph assets from the openwisp-utils fork during automated testing. Fixes openwisp#272
|
Hi @nemesifier, I have completed the requested updates by modifying the CI workflow to install my openwisp-utils fork, and all tests are now passing successfully with green checks. Additionally, I have manually tested the UI with the required screenshot demonstrating the topology graph rendering correctly. Please let me know if everything looks good to go! |
Removed redundant netjsongraph static files from the repository to prevent duplication. Updated admin.py and base.html to load the unbundled netjsongraph and echarts libraries directly from openwisp-utils.
Checklist
Reference to Existing Issue
Closes #272
Description of Changes
This PR achieves the goal of avoiding duplicated static files across the OpenWISP ecosystem.
netjsongraphCSS and JS files from the localstatic/directory.BaseAdmininadmin.pyto point to the sharedlib/files provided byopenwisp-utils, and explicitly included the new ECharts dependencies.base.htmlto load the scripts fromopenwisp-utilsas well.Important Note regarding CI: This PR heavily depends on openwisp/openwisp-utils#517. Because the CI environment will pull the current
masterbranch ofopenwisp-utils(which lacks the unbundled ECharts files), the Selenium tests here will inevitably throw 404 errors until that initial PR is merged.Screenshot