⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@Adityashandilya555
Copy link
Contributor

@Adityashandilya555 Adityashandilya555 commented Oct 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.
  • I have updated the documentation.

Reference to Existing Issue

Closes #526.

Description of Changes

  • Added DASHBOARD_BASEURL configuration variable using dashboard domain
  • Changed OPENWISP_MONITORING_API_BASEURL to use DASHBOARD_BASEURL instead of API_BASEURL
  • Chart loading now uses the same domain as the dashboard interface

Screenshot

Screenshot 2025-10-26 at 11 41 37 AM

Please include any relevant screenshots.

Adityashandilya555 and others added 8 commits October 4, 2025 09:15
…p#477

Replaced archived django-celery-email with actively maintained
django-post-office for async email handling.

- Updated requirements.txt: django-celery-email -> django-post-office
- Changed email backend configuration in Dockerfile
- Updated settings.py to use post_office app conditionally

Fixes openwisp#477
…penwisp#477

The archived django-celery-email package has been replaced with
django-celery-email-reboot, a maintained fork that provides Django 4.x/5.x
compatibility. This is a drop-in replacement with identical API.

Changes:
- Updated requirements.txt to use django-celery-email-reboot>=4.1.0,<5.0.0
- Changed EMAIL_BACKEND to djcelery_email.backends.CeleryEmailBackend
- Updated settings.py to check for djcelery_email backend

Testing performed following maintainer's approach:
- Built all Docker images successfully with django-celery-email-reboot 4.1.0
- Tested email flow: docker compose run dashboard python manage.py sendtestemail
- Verified postfix logs showing successful email pipeline:
  Django → Celery → Postfix (queue ID 95C4120370E8)
- Email queued and delivery attempted successfully

The entire email pipeline is working correctly with the new package.

Fixes openwisp#477
This change addresses issue openwisp#526 by making monitoring charts load
from the dashboard domain instead of the API domain. This prevents
SSL certificate issues that can confuse new users when the API
domain doesn't have a trusted certificate.

Changes:
- Added DASHBOARD_BASEURL configuration variable
- Changed OPENWISP_MONITORING_API_BASEURL to use dashboard domain
- Chart loading now uses the same domain as the dashboard interface

Fixes openwisp#526
@pandafy pandafy moved this from To do (general) to Needs review in OpenWISP Contributor's Board Nov 3, 2025
@pandafy pandafy moved this from Needs review to In progress in OpenWISP Contributor's Board Nov 3, 2025
@Adityashandilya555 Adityashandilya555 force-pushed the fix/chart-loading-use-dashboard-domain branch from f063d92 to 8cb14af Compare December 9, 2025 10:43
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.

OPENWISP_NOTIFICATIONS_HOST = API_BASEURL
OPENWISP_CONTROLLER_API_HOST = API_BASEURL
OPENWISP_MONITORING_API_BASEURL = API_BASEURL
OPENWISP_MONITORING_API_BASEURL = DASHBOARD_BASEURL
Copy link
Member

Choose a reason for hiding this comment

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

Would removing OPENWISP_MONITORING_API_BASEURL completely work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removing OPENWISP_MONITORING_API_BASEURL completely should work. I checked the openwisp-monitoring source and the default value is None, which means the charts will use relative URLs and load from the current domain (the dashboard domain).

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that then and simplify this. This way we don't need to define DASHBOARD_BASEURL.
Keep it simple. Please do manual testing to ensure the approach work and record a video showing the result.

@@ -1,2 +1,3 @@
docker>=7.1.0,<7.2.0
openwisp-utils[qa,selenium]~=1.2.0
sphinx<8.0.0
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged. I've removed it while resolving the merge conflict.

@nemesifier nemesifier changed the title Fix/chart loading use dashboard domain [fix] Load charts use dashboard domain Dec 19, 2025
@nemesifier nemesifier changed the title [fix] Load charts use dashboard domain [fix] Load charts using dashboard domain Dec 19, 2025
OPENWISP_NOTIFICATIONS_HOST = API_BASEURL
OPENWISP_CONTROLLER_API_HOST = API_BASEURL
OPENWISP_MONITORING_API_BASEURL = API_BASEURL
OPENWISP_MONITORING_API_BASEURL = DASHBOARD_BASEURL
Copy link
Member

Choose a reason for hiding this comment

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

Let's do that then and simplify this. This way we don't need to define DASHBOARD_BASEURL.
Keep it simple. Please do manual testing to ensure the approach work and record a video showing the result.

@nemesifier
Copy link
Member

@Adityashandilya555 ping

@coderabbitai
Copy link

coderabbitai bot commented Jan 13, 2026

Walkthrough

Removed the OPENWISP_MONITORING_API_BASEURL variable from images/openwisp_dashboard/module_settings.py; no other configuration lines were changed. The file no longer exposes a public setting that assigned the monitoring API base URL to API_BASEURL.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title '[fix] Load charts using dashboard domain' clearly summarizes the main change: using the dashboard domain for chart loading instead of the API domain.
Description check ✅ Passed The PR description covers all required template sections: checklist completion, linked issue reference (#526), and detailed description of changes with a screenshot.
Linked Issues check ✅ Passed The PR successfully addresses issue #526's objective to load charts using the dashboard domain instead of the API domain to avoid SSL certificate failures.
Out of Scope Changes check ✅ Passed The code change (removing OPENWISP_MONITORING_API_BASEURL assignment) is directly in scope with the objective to change chart loading to use the dashboard domain instead of the API domain.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9990783 and 4105107.

📒 Files selected for processing (1)
  • images/openwisp_dashboard/module_settings.py
💤 Files with no reviewable changes (1)
  • images/openwisp_dashboard/module_settings.py
⏰ 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). (1)
  • GitHub Check: CI Build

Comment @coderabbitai help to get the list of available commands and usage tips.

@Adityashandilya555
Copy link
Contributor Author

@Adityashandilya555 ping
@nemesifier my apologies i had a viral fever for 10 days will send you video by todsy

@Adityashandilya555
Copy link
Contributor Author

Change.Device._.OpenWISP.Admin.1.mp4

Removed OPENWISP_MONITORING_API_BASEURL so that monitoring charts
use relative URLs and load from the dashboard domain.

This fixes SSL certificate issues for users without trusted certs
on the API domain.

Closes openwisp#526
@Adityashandilya555 Adityashandilya555 force-pushed the fix/chart-loading-use-dashboard-domain branch from 9990783 to 4105107 Compare January 13, 2026 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

[change] Load charts using dashboard domain

2 participants