[change] Reuse django-leaflet for dashboard map #709 - #794
[change] Reuse django-leaflet for dashboard map #709#794kunalverma2512 wants to merge 11 commits into
Conversation
Removed bundled netjsongraph files and updated device map templates to load unbundled ECharts and django-leaflet directly, preventing redundant Leaflet loading. Fixes openwisp#709
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates the admin map page to load Leaflet via django-leaflet and switches MapPageAdmin.Media to use ECharts plus the netjsongraph.echarts bundle and lib/netjsongraph CSS. Admin tests were updated to expect the new JS files. Separately, setup.py now appends a CI-specific openwisp-utils Git dependency into install_requires. Sequence Diagram(s)sequenceDiagram
participant AdminTemplate as map_page.html
participant DjangoLeaflet as django-leaflet
participant AdminMedia as MapPageAdmin.Media
AdminTemplate->>DjangoLeaflet: render `{% leaflet_js %}` + `{% leaflet_css %}`
AdminTemplate->>AdminMedia: render `{{ media }}` (echarts.min.js + netjsongraph.echarts.min.js + lib/netjsongraph css)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_monitoring/device/admin.py`:
- Around line 591-599: The Media asset paths in the device admin (Media class in
openwisp_monitoring/device/admin.py) were changed to lib/netjsongraph/... but
apps.py (AppConfig in openwisp_monitoring/device/apps.py) and tests still
reference monitoring/js/lib/netjsongraph.* and the new files
(lib/netjsongraph/js/netjsongraph.echarts.min.js and
lib/netjsongraph/css/netjsongraph*.css) do not exist; fix this by making asset
references consistent across the codebase: either (A) restore the old paths in
the Media class to monitoring/js/lib/netjsongraph.* and
monitoring/css/netjsongraph.css everywhere (admin.py, apps.py, tests) or (B) add
the missing static files under static/lib/netjsongraph/js and
static/lib/netjsongraph/css and update apps.py and all tests to reference
lib/netjsongraph/...; ensure the chosen approach updates admin.py Media, the
AppConfig asset references, and any unit tests so all three components point to
the same existing asset filenames and that staticfiles collection will include
them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e21d724-ac4e-4965-80d4-fa93e12d8175
⛔ Files ignored due to path filters (1)
openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (3)
openwisp_monitoring/device/admin.pyopenwisp_monitoring/device/static/monitoring/css/netjsongraph.cssopenwisp_monitoring/device/templates/admin/map/map_page.html
💤 Files with no reviewable changes (1)
- openwisp_monitoring/device/static/monitoring/css/netjsongraph.css
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/device/admin.py
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/device/admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/device/admin.py
🔇 Additional comments (2)
openwisp_monitoring/device/templates/admin/map/map_page.html (1)
17-19: LGTM!openwisp_monitoring/device/admin.py (1)
592-592: ⚡ Quick winNo action required—fullscreen plugin compatibility verified.
The
leaflet.fullscreen.min.jsplugin is confirmed compatible with Leaflet loaded from django-leaflet. The plugin uses standard Leaflet APIs (L.Control, L.Map.include, L.DomUtil, L.DomEvent) that remain stable across versions, and existing tests (test_floorplan_fullscreen, test_admin.py) actively verify fullscreen functionality works correctly.
| "lib/netjsongraph/js/netjsongraph.echarts.min.js", | ||
| "monitoring/js/lib/leaflet.fullscreen.min.js", | ||
| ] | ||
| css = { | ||
| "all": [ | ||
| "monitoring/css/device-map.css", | ||
| "leaflet/leaflet.css", | ||
| "monitoring/css/leaflet.fullscreen.css", | ||
| "monitoring/css/netjsongraph.css", | ||
| "lib/netjsongraph/css/netjsongraph.css", | ||
| "lib/netjsongraph/css/netjsongraph-theme.css", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining references to old netjsongraph bundle paths
# Check for old JS path references
echo "=== Checking for old netjsongraph.min.js references ==="
rg -n "netjsongraph\.min\.js" --type py --type html --type js -g '!*.min.js'
# Check for old CSS path references
echo -e "\n=== Checking for old monitoring/css/netjsongraph.css references ==="
rg -n "monitoring/css/netjsongraph\.css" --type py --type html
# Verify new paths exist or are documented
echo -e "\n=== Checking for new lib/netjsongraph path references ==="
fd -t f "netjsongraph.echarts.min.js"
fd -t f -p "lib/netjsongraph/css/netjsongraph.css"Repository: openwisp/openwisp-monitoring
Length of output: 1070
🏁 Script executed:
# Check the context around apps.py lines 378-381 to understand the scope
sed -n '375,385p' openwisp_monitoring/device/apps.py
# Check if admin.py is the only file with new paths, or if there are other updated files
echo "=== Searching for lib/netjsongraph references in .py files ==="
rg -n "lib/netjsongraph" --type py
# Verify the exact state in admin.py around the modified lines
echo -e "\n=== Admin.py context around lines 591-599 ==="
sed -n '589,601p' openwisp_monitoring/device/admin.pyRepository: openwisp/openwisp-monitoring
Length of output: 1941
Critical: Asset migration is incomplete and will cause runtime failures.
The admin.py Media class has been updated to reference assets from lib/netjsongraph/ (lines 591, 598-599), but:
- Other files still use old paths: apps.py (lines 378, 381) and test files continue to reference
monitoring/js/lib/netjsongraph.min.jsandmonitoring/css/netjsongraph.css - New assets do not exist: The files
lib/netjsongraph/js/netjsongraph.echarts.min.jsandlib/netjsongraph/css/netjsongraph*.cssare not present in the repository - Conflicting references: Two different Media configurations in the same codebase now use incompatible asset paths
Either migrate all asset references across apps.py, test files, and admin.py consistently, or verify that openwisp-utils has been integrated and the new assets are available. This PR cannot merge in its current state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_monitoring/device/admin.py` around lines 591 - 599, The Media asset
paths in the device admin (Media class in openwisp_monitoring/device/admin.py)
were changed to lib/netjsongraph/... but apps.py (AppConfig in
openwisp_monitoring/device/apps.py) and tests still reference
monitoring/js/lib/netjsongraph.* and the new files
(lib/netjsongraph/js/netjsongraph.echarts.min.js and
lib/netjsongraph/css/netjsongraph*.css) do not exist; fix this by making asset
references consistent across the codebase: either (A) restore the old paths in
the Media class to monitoring/js/lib/netjsongraph.* and
monitoring/css/netjsongraph.css everywhere (admin.py, apps.py, tests) or (B) add
the missing static files under static/lib/netjsongraph/js and
static/lib/netjsongraph/css and update apps.py and all tests to reference
lib/netjsongraph/...; ensure the chosen approach updates admin.py Media, the
AppConfig asset references, and any unit tests so all three components point to
the same existing asset filenames and that staticfiles collection will include
them.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
Hi @nemesifier, I am completely done with the changes for this issue! Just a heads-up that the CI tests are currently failing with The script loading order is correctly set up to load Please let me know what the next steps are for further working on this project, and if any more guidance will help me get things finalized as nicely as possible! |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The failing selenium test indicates there's a problem.
- Added missing echarts.min.js to MapPageAdmin to fix NetJSONGraph initialization. - Temporarily pointed setup.py to custom openwisp-utils branch for CI testing. - Resolved flake8 trailing whitespace and black line-length limits in setup.py. Fixes openwisp#709
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@setup.py`:
- Around line 22-26: The temporary git dependency is being appended inside the
requirements parsing loop (the requirements.append(...) call), causing
duplication and always including the fork; move that requirements.append call
out of the loop so it runs once after the requirements list is built, and wrap
it with an environment check (e.g., if os.getenv("ADD_OPENWISP_UTILS_FORK") or
similar) so the git dependency is only added when the env var is set; also add
an import for os if not present and ensure the logic references the same
requirements list variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1eefea9c-c4cf-4615-8c0a-0c78397e0860
📒 Files selected for processing (2)
openwisp_monitoring/device/admin.pysetup.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
setup.pyopenwisp_monitoring/device/admin.py
🧠 Learnings (3)
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
setup.pyopenwisp_monitoring/device/admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
setup.pyopenwisp_monitoring/device/admin.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/admin.py
🔇 Additional comments (1)
openwisp_monitoring/device/admin.py (1)
591-593: LGTM!
Moved the temporary openwisp-utils CI requirement outside the requirements.txt parsing loop to prevent duplicate appending and pip ResolutionImpossible errors. Fixes openwisp#709
There was a problem hiding this comment.
♻️ Duplicate comments (1)
setup.py (1)
22-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the temporary git dependency to CI only.
On Line 22–26, this temporary fork is still added unconditionally, so all installs (not just CI) can pull a feature-branch dependency. Please guard it behind an environment variable before appending.
#!/bin/bash # Verify whether setup.py gates the temporary openwisp-utils git dependency by env var. # Expected: an `os.environ.get(...)` / `os.getenv(...)` check controls this append. nl -ba setup.py | sed -n '1,80p' rg -n 'openwisp-utils @ git\+https://github.com/kunalverma2512/openwisp-utils.git@feature/517-add-netjsongraph-static|os\.environ|getenv' setup.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py` around lines 22 - 26, The temporary git dependency appended via requirements.append("openwisp-utils @ git+https://github.com/kunalverma2512/...@feature/517-add-netjsongraph-static") must be gated to CI only; modify the block so it only runs when an environment variable (e.g., CI or INCLUDE_TEMP_GIT_DEP) is set by checking os.environ.get(...) or os.getenv(...), and only then call requirements.append for that openwisp-utils entry; update or add a short comment indicating this is CI-only and ensure you reference the existing requirements.append call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@setup.py`:
- Around line 22-26: The temporary git dependency appended via
requirements.append("openwisp-utils @
git+https://github.com/kunalverma2512/...@feature/517-add-netjsongraph-static")
must be gated to CI only; modify the block so it only runs when an environment
variable (e.g., CI or INCLUDE_TEMP_GIT_DEP) is set by checking
os.environ.get(...) or os.getenv(...), and only then call requirements.append
for that openwisp-utils entry; update or add a short comment indicating this is
CI-only and ensure you reference the existing requirements.append call when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a5cce28-7b94-4885-a923-bd064b292bb7
📒 Files selected for processing (1)
setup.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
setup.py
🧠 Learnings (2)
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
setup.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
setup.py
Removed the temporary openwisp-utils git requirement from setup.py as it conflicts with upstream dependencies like openwisp-controller during pip resolution. The missing echarts.min.js file added to MapPageAdmin previously should be sufficient to pass initialization. Fixes openwisp#709
|
Hi @nemesifier, you were completely right about the Selenium tests indicating a problem. Thank you for pointing that out! I dug into the logs and realized exactly what went wrong. Here is a summary of what I just updated:
Current Status: Please let me know if this looks good to you now, or if there is anything else I should adjust! |
You can use docker to build a test environment via the given |
|
@kunalverma2512 I ran your code locally and it seems like you have a few issues, these issues are also there in the CI These test cases are making your test suit fail, most importantly the 1st one. I haven't dig into this PR/issue much but you either
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_monitoring/device/admin.py`:
- Around line 591-592: The MapPageAdmin.Media asset paths changed to use the
netjsongraph distribution from openwisp-utils, but tests and production
requirements weren't updated: add openwisp-utils to requirements.txt with the
minimum version that contains the netjsongraph assets (use the version from the
openwisp-utils PR that introduced lib/netjsongraph), and update the
test_mappage_admin_media_files() expectations in test_admin.py (references:
MapPageAdmin.Media and test_mappage_admin_media_files) to remove the old
monitoring/js/lib/netjsongraph.min.js entry and assert the new paths (e.g.
"lib/netjsongraph/js/echarts.min.js" and
"lib/netjsongraph/js/netjsongraph.echarts.min.js") are present. Ensure the
requirements change is included in production requirements and run the test to
confirm the new asset list matches MapPageAdmin.Media.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 346ee5b9-6e11-42ce-88f1-890abb07a51b
⛔ Files ignored due to path filters (1)
openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (4)
openwisp_monitoring/device/admin.pyopenwisp_monitoring/device/static/monitoring/css/netjsongraph.cssopenwisp_monitoring/device/templates/admin/map/map_page.htmlsetup.py
💤 Files with no reviewable changes (1)
- openwisp_monitoring/device/static/monitoring/css/netjsongraph.css
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
setup.pyopenwisp_monitoring/device/admin.py
🧠 Learnings (3)
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
setup.pyopenwisp_monitoring/device/admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
setup.pyopenwisp_monitoring/device/admin.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/admin.py
🔇 Additional comments (2)
setup.py (1)
21-21: No actionable change here (formatting-only newline).openwisp_monitoring/device/templates/admin/map/map_page.html (1)
17-18: LGTM!
Updated the static_files lists in test_mappage_admin_media_files and test_dashboard to assert the presence of the new unbundled echarts and netjsongraph files loaded directly from openwisp-utils, replacing the outdated local asset paths. Fixes openwisp#709
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_monitoring/device/tests/test_admin.py (1)
902-914:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate CSS path to match admin.py configuration in both test methods.
The test assertions on lines 905 and 1259 use
monitoring/css/netjsongraph.css, but the admin configuration (admin.py line 599) serveslib/netjsongraph/css/netjsongraph.css. Update both occurrences tolib/netjsongraph/css/netjsongraph.cssto match the actual served assets and maintain test consistency.The JS paths have been correctly updated to the unbundled versions (
lib/netjsongraph/js/echarts.min.jsandlib/netjsongraph/js/netjsongraph.echarts.min.js), but the CSS path was not aligned with the corresponding admin configuration change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_monitoring/device/tests/test_admin.py` around lines 902 - 914, The test asserts are checking for the wrong CSS asset path; update the occurrences of "monitoring/css/netjsongraph.css" in the test(s) to "lib/netjsongraph/css/netjsongraph.css" so the static_files list and the self.assertContains checks match the admin.py asset configuration (update both places where that CSS path appears in this test module).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openwisp_monitoring/device/tests/test_admin.py`:
- Around line 902-914: The test asserts are checking for the wrong CSS asset
path; update the occurrences of "monitoring/css/netjsongraph.css" in the test(s)
to "lib/netjsongraph/css/netjsongraph.css" so the static_files list and the
self.assertContains checks match the admin.py asset configuration (update both
places where that CSS path appears in this test module).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8823da24-4612-4b5c-93c4-5d14fa0500c9
📒 Files selected for processing (1)
openwisp_monitoring/device/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/device/tests/test_admin.py
🧠 Learnings (5)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
🔇 Additional comments (1)
openwisp_monitoring/device/tests/test_admin.py (1)
1248-1266: ⚡ Quick winConsistent with dashboard test—same CSS verification needed.
The media file expectations match the changes in
test_dashboard(lines 907-908), which is correct. However, the same CSS path question applies: line 1260 still expectsmonitoring/css/netjsongraph.cssrather thanlib/netjsongraph/css/netjsongraph.css. Please verify this is intentional or update both test methods together if the CSS path should also move to lib/netjsongraph.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Hi! Thanks for taking the time to run this locally and share the logs. Here is a breakdown of what's happening with those errors: Error 1 (test_mappage_admin_media_files): Error 2 (Selenium Status 255): Error 3 (Metric Configuration already registered): I'll push the fix for Error 1 shortly! Let me know if you'd like me to look into anything else. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I think this would also close #710, wouldn't it? Please confirm, if so, update the PR description to reflect this.
Then, 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 which shows the browser network tab open showing the separate JS files for netjsongraph` and leaflet being loaded as expected AND the geographic map UI working.
Once everything is done correctly the CI will pass. Manual testing on your end is needed to accomplish this.
| continue | ||
| # add line to requirements | ||
| requirements.append(line) | ||
|
|
Restored setup.py to remove unrelated formatting changes from a previous reverted commit. 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#709 Fixes openwisp#710
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
Moved Leaflet core assets from the map_page.html template directly into the MapPageAdmin Media class to enforce the correct strictly-ordered loading sequence. This guarantees that leaflet.js loads before the fullscreen plugin and NetJSONGraph, resolving the Javascript ReferenceError that crashed the map during automated browser tests. Fixes openwisp#709
Updated the static_files lists in test_mappage_admin_media_files and test_dashboard to match the new unbundled asset paths. Removed the obsolete local monitoring/css/netjsongraph.css reference and added the new lib/netjsongraph/ CSS and JS paths provided by openwisp-utils. Fixes openwisp#709
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Updated the static file paths in apps.py for the dashboard map to use the new unbundled NetJSONGraph assets from openwisp-utils. Also explicitly added leaflet.js to the dashboard javascript configuration since it is no longer bundled within netjsongraph. This resolves the 404 errors and Javascript crashes on the dashboard page during Selenium tests. Fixes openwisp#709
Updated the static file test assertions in test_apps.py and test_admin.py to accurately reflect the newly unbundled NetJSONGraph assets and the explicit leaflet.js core requirement. Removed obsolete local paths. Fixes openwisp#709
Test Failures in
|
bf43932 to
9862322
Compare
|
@nemesifier Yes, I confirm this PR also solves #710, and I have updated the description to reflect that. |
|
Hi @nemesifier, I have completed the suggested changes and attached the requested screenshots and also updated the PR description to include Closes #710 . Please let me know if everything looks good! |
3 participants
Removed bundled netjsongraph files and updated device map templates to load unbundled ECharts and django-leaflet directly, preventing redundant Leaflet loading.
Fixes #709
Checklist
Reference to Existing Issue
Closes #709.
Closes #710.
Description of Changes
To achieve the "Ideal Situation" described in #709 and avoid loading Leaflet twice, this PR makes the following changes:
netjsongraph.min.jsand its associated CSS from themonitoring/static/folder.MapPageAdminindevice/admin.pyto point to the new, unbundled ECharts assets fromopenwisp-utils.{% leaflet_js %}and{% leaflet_css %}intomap_page.htmlso the dashboard map correctly reuses the Leaflet assets directly fromdjango-leaflet.This PR depends on the shared static files introduced in my
openwisp-utilsPR .Screenshot