Skip to content

Review admin template structure #475

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.

Sign up for GitHub

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

Open
asmodehn wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Review admin template structure #475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b7a6e5
[tests] Confirm template cannot be extended with current loaders order
Jun 2, 2026
9b1e592
[change] Reorder templates loaders in test
Jun 2, 2026
414522f
[change] Namespace admin templates and fix sample app overrides
Jun 2, 2026
a028204
[change] Fix test_admin to resolve static path via static()
Jun 4, 2026
57eacf6
[change] Merge remote-tracking branch 'upstream/master' into review_t…
Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 1 addition & 90 deletions openwisp_notifications/templates/admin/base_site.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,90 +1 @@
{% extends "admin/base_site.html" %}
{% load i18n static notification_tags %}

{% block extrastyle %}
{{ block.super }}
{% if request|should_load_notifications_widget %}
<link rel="stylesheet" type="text/css" href="{% static 'openwisp-notifications/css/loader.css' %}" />
<link rel="stylesheet" type="text/css" href="{% static 'openwisp-notifications/css/notifications.css' %}" />
{% endif %}
{% endblock extrastyle %}

{% block extrahead %}
{{ block.super }}
{% if request|should_load_notifications_widget and not media.js and 'jquery' not in block.super %}
<script src="{% static 'admin/js/vendor/jquery/jquery.min.js' %}"></script>
<script src="{% static 'admin/js/jquery.init.js' %}"></script>
{% endif %}
{% endblock %}

{% block user-tools %}
{{ block.super }}
{% if request|should_load_notifications_widget %}
<button class="ow-notifications toggle-btn" id="openwisp_notifications">
<span id="ow-notification-btn"></span>
{% unread_notifications %}
</button>
{% csrf_token %}
<div class="ow-notification-toast-wrapper"></div>
<div class="ow-notification-dropdown ow-hide">
<div class="filters">
<span class="toggle-btn" id="ow-mark-all-read" tabindex="0" role="button">
{% trans 'Mark all as read' %}
</span>
<a class="toggle-btn" href="{% url 'notifications:notification_preference' %}">
{% trans 'Notification Preferences' %}
</a>
</div>
<div class="ow-notification-wrapper ow-round-bottom-border">
<div id="ow-notifications-loader" class="ow-hide"><div class="loader"></div></div>
</div>
<div class="ow-no-notifications ow-round-bottom-border ow-hide">
<p>{% trans 'No new notification.' %}</p>
</div>
</div>
{% endif %}
{% endblock %}

{% block footer %}
{{ block.super }}
{% if request|should_load_notifications_widget %}
<div class="ow-overlay ow-overlay-notification ow-overlay-inner ow-hide">
<div class="ow-dialog-notification">
<span class="ow-dialog-close ow-dialog-close-x">&times;</span>
<div class="ow-notification-level-wrapper ow-dialog-notification-level-wrapper"></div>
<h2 class="ow-message-title"></h2>
<div class="ow-message-description"></div>
<div class="ow-dialog-buttons">
<button class="ow-message-target-redirect ow-hide button default success-btn">
{% trans 'Open' %}
</button>
<button class="ow-dialog-close button default">
{% trans 'Close' %}
</button>
</div>
</div>
</div>
<script type="text/javascript" src="{% static 'openwisp-notifications/js/vendor/reconnecting-websocket.min.js' %}"></script>
<script type="text/javascript">
{% if OPENWISP_NOTIFICATIONS_HOST %}
const notificationApiHost = new URL('{{ OPENWISP_NOTIFICATIONS_HOST }}');
{% else %}
const notificationApiHost = window.location;
{% endif %}
const webSocketProtocol = notificationApiHost.protocol === 'http:' ? 'ws' : 'wss';
const notificationSound = new Audio('{{ OPENWISP_NOTIFICATIONS_SOUND | default:"" }}');
// Create websocket connection
const notificationSocket = new ReconnectingWebSocket(
`${webSocketProtocol}://${notificationApiHost.host}/ws/notification/`,
null, {
debug: false,
// The library re-connects if it fails to establish a connection in "timeoutInterval".
// On slow internet connections, the default value of "timeoutInterval" will
// keep terminating and re-establishing the connection.
timeoutInterval: 7000,
}
);
</script>
<script type="text/javascript" src="{% static 'openwisp-notifications/js/notifications.js' %}"></script>
{% endif %}
{% endblock footer %}
{% extends 'admin/openwisp_notifications/base_site.html' %}
36 changes: 1 addition & 35 deletions openwisp_notifications/templates/admin/change_form.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1 @@
{% extends 'admin/change_form.html' %}

{% comment %}
This template is required to supply information regarding the displayed object
from Django's template context to JavaScript for proper functioning of object notification
widget. Following data is supplied:
1. object's app_label
2. object's model_name
3. object's primary key
{% endcomment %}

{% block admin_change_form_document_ready %}
{{ block.super }}
{% if change %}
<script>
const owNotifyAppLabel = '{{ opts.app_label }}',
owNotifyModelName = '{{ opts.model_name }}',
uuidRegex = /\/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i,
owIsChangeForm = true;
let objectId;
try{
objectId = uuidRegex.exec(window.location.pathname)[0];
}
catch(error){
try{
objectId = /\/(\d+)\//.exec(window.location.pathname)[0];
}
catch(error){
throw error;
}
}
const owNotifyObjectId = objectId.replace(/\//g,'');
</script>
{% endif %}
{% endblock %}
{% extends 'admin/openwisp_notifications/change_form.html' %}
90 changes: 90 additions & 0 deletions openwisp_notifications/templates/admin/openwisp_notifications/base_site.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
{% extends "admin/base_site.html" %}
{% load i18n static notification_tags %}

{% block extrastyle %}
{{ block.super }}
{% if request|should_load_notifications_widget %}
<link rel="stylesheet" type="text/css" href="{% static 'openwisp-notifications/css/loader.css' %}" />
<link rel="stylesheet" type="text/css" href="{% static 'openwisp-notifications/css/notifications.css' %}" />
{% endif %}
{% endblock extrastyle %}

{% block extrahead %}
{{ block.super }}
{% if request|should_load_notifications_widget and not media.js and 'jquery' not in block.super %}
<script src="{% static 'admin/js/vendor/jquery/jquery.min.js' %}"></script>
<script src="{% static 'admin/js/jquery.init.js' %}"></script>
{% endif %}
{% endblock %}

{% block user-tools %}
{{ block.super }}
{% if request|should_load_notifications_widget %}
<button class="ow-notifications toggle-btn" id="openwisp_notifications">

@coderabbitai coderabbitai Bot Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Set an explicit type="button" on these controls.

These buttons are not submit actions, so relying on the HTML default can trigger accidental form submission if this block is rendered inside or near a form later.

Suggested fix 
-    <button class="ow-notifications toggle-btn" id="openwisp_notifications">
+    <button type="button" class="ow-notifications toggle-btn" id="openwisp_notifications">
...
-              <button class="ow-message-target-redirect ow-hide button default success-btn">
+              <button type="button" class="ow-message-target-redirect ow-hide button default success-btn">
...
-              <button class="ow-dialog-close button default">
+              <button type="button" class="ow-dialog-close button default">

Also applies to: 58-61

🧰 Tools
🪛 HTMLHint (1.9.2)

[warning] 23-23: The type attribute must be present on elements.

(button-type-require)

🤖 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_notifications/templates/admin/openwisp_notifications/base_site.html`
at line 23, The buttons with class "ow-notifications toggle-btn" (e.g., the
element with id "openwisp_notifications") and the other notification control
buttons in the same template should explicitly include type="button" to prevent
accidental form submission; edit
openwisp_notifications/templates/admin/openwisp_notifications/base_site.html and
add type="button" to the button tags referenced by class "ow-notifications
toggle-btn" and any other similar notification buttons in that file (lines
around the toggle and the controls at 58-61).

@asmodehn asmodehn Jun 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not the point of this PR

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@asmodehn, understood — I'll drop the suggestion for this PR. Would you like me to open a follow-up issue to track adding explicit type="button" attributes to the notification button elements in base_site.html?

(ᵕ˃ᴗ˂ᵕ)

🧠 Learnings used 
Learnt from: Dhanus3133
Repo: openwisp/openwisp-notifications PR: 469
File: openwisp_notifications/templatetags/notification_tags.py:46-52
Timestamp: 2026-05-31T08:08:31.780Z
Learning: In openwisp-notifications, the Django admin's queryset/row-level access control already restricts a staff user to viewing only user records that belong to organizations they manage. This means the `has_notification_setting_permission` template tag (in `openwisp_notifications/templatetags/notification_tags.py`) only needs to check the global `change_notificationsetting` permission — no additional org-management check is required in the tag itself. By the time a staff user can see the "Notification Preferences" button on a target user's admin change page, the admin layer has already verified org-membership, which is the same condition enforced by `NotificationPreferenceView._has_other_user_permission()` in `openwisp_notifications/views.py`. So there is no risk of the button leading to a 403.

<span id="ow-notification-btn"></span>
{% unread_notifications %}
</button>
{% csrf_token %}
<div class="ow-notification-toast-wrapper"></div>
<div class="ow-notification-dropdown ow-hide">
<div class="filters">
<span class="toggle-btn" id="ow-mark-all-read" tabindex="0" role="button">
{% trans 'Mark all as read' %}
</span>
<a class="toggle-btn" href="{% url 'notifications:notification_preference' %}">
{% trans 'Notification Preferences' %}
</a>
</div>
<div class="ow-notification-wrapper ow-round-bottom-border">
<div id="ow-notifications-loader" class="ow-hide"><div class="loader"></div></div>
</div>
<div class="ow-no-notifications ow-round-bottom-border ow-hide">
<p>{% trans 'No new notification.' %}</p>
</div>
</div>
{% endif %}
{% endblock %}

{% block footer %}
{{ block.super }}
{% if request|should_load_notifications_widget %}
<div class="ow-overlay ow-overlay-notification ow-overlay-inner ow-hide">
<div class="ow-dialog-notification">
<span class="ow-dialog-close ow-dialog-close-x">&times;</span>
<div class="ow-notification-level-wrapper ow-dialog-notification-level-wrapper"></div>
<h2 class="ow-message-title"></h2>
<div class="ow-message-description"></div>
<div class="ow-dialog-buttons">
<button class="ow-message-target-redirect ow-hide button default success-btn">
{% trans 'Open' %}
</button>
<button class="ow-dialog-close button default">
{% trans 'Close' %}
</button>
</div>
</div>
</div>
<script type="text/javascript" src="{% static 'openwisp-notifications/js/vendor/reconnecting-websocket.min.js' %}"></script>
<script type="text/javascript">
{% if OPENWISP_NOTIFICATIONS_HOST %}
const notificationApiHost = new URL('{{ OPENWISP_NOTIFICATIONS_HOST }}');
{% else %}
const notificationApiHost = window.location;
{% endif %}
const webSocketProtocol = notificationApiHost.protocol === 'http:' ? 'ws' : 'wss';
const notificationSound = new Audio('{{ OPENWISP_NOTIFICATIONS_SOUND | default:"" }}');
// Create websocket connection
const notificationSocket = new ReconnectingWebSocket(
`${webSocketProtocol}://${notificationApiHost.host}/ws/notification/`,
null, {
debug: false,
// The library re-connects if it fails to establish a connection in "timeoutInterval".
// On slow internet connections, the default value of "timeoutInterval" will
// keep terminating and re-establishing the connection.
timeoutInterval: 7000,
}
);
</script>
<script type="text/javascript" src="{% static 'openwisp-notifications/js/notifications.js' %}"></script>
{% endif %}
{% endblock footer %}
35 changes: 35 additions & 0 deletions openwisp_notifications/templates/admin/openwisp_notifications/change_form.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{% extends 'admin/change_form.html' %}

{% comment %}
This template is required to supply information regarding the displayed object
from Django's template context to JavaScript for proper functioning of object notification
widget. Following data is supplied:
1. object's app_label
2. object's model_name
3. object's primary key
{% endcomment %}
Comment on lines +3 to +10

@coderabbitai coderabbitai Bot Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stop deriving owNotifyObjectId from window.location.pathname.

This hard-codes UUID and integer admin URLs, then throws on anything else. A model with a string PK or a custom admin path will break this bootstrap before notifications.js runs. The comment above is also stale: it says the primary key comes from template context, but the implementation parses the browser URL instead.

Use Django’s server-side object id in the template and drop the regex/try/catch path parsing entirely. If this Django version does not expose object_id in the default change-form context, pass it explicitly from the admin view instead of inferring it from the URL.

Suggested direction 
 {% block admin_change_form_document_ready %}
   {{ block.super }}
   {% if change %}
     <script>
-      const owNotifyAppLabel = '{{ opts.app_label }}',
-            owNotifyModelName = '{{ opts.model_name }}',
-            uuidRegex = /\/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i,
-            owIsChangeForm = true;
-      let objectId;
-      try{
-        objectId = uuidRegex.exec(window.location.pathname)[0];
-      }
-      catch(error){
-        try{
-          objectId = /\/(\d+)\//.exec(window.location.pathname)[0];
-        }
-        catch(error){
-          throw error;
-        }
-      }
-      const owNotifyObjectId = objectId.replace(/\//g,'');
+      const owNotifyAppLabel = '{{ opts.app_label|escapejs }}';
+      const owNotifyModelName = '{{ opts.model_name|escapejs }}';
+      const owNotifyObjectId = '{{ object_id|escapejs }}';
+      const owIsChangeForm = true;
     </script>
   {% endif %}
 {% endblock %}

As per coding guidelines, "Flag outdated or incorrect documentation, comments, or docstrings" and "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".

In Django admin's `change_form.html` template context, is `object_id` available by default, and what is the recommended way to expose the current object's primary key in a custom admin `change_form.html` override?

Also applies to: 16-32

🤖 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_notifications/templates/admin/openwisp_notifications/change_form.html`
around lines 3 - 10, The template currently derives owNotifyObjectId by parsing
window.location.pathname; replace that client-side URL parsing with the
server-side object PK from the Django change-form context (use object_id if
present or original.pk/original.pk|default:object.pk as appropriate) and remove
the regex/try/catch path logic; if your Django version or view override does not
expose object_id/original, update the admin view to pass the object's PK into
the template context, then have the template assign owNotifyObjectId directly
and let notifications.js consume it.

@asmodehn asmodehn Jun 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not the point of this PR.

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@asmodehn, understood — this PR focuses on template structure/namespacing, not on fixing the object-ID extraction logic.

Would you like me to open a follow-up issue to track the concern about replacing the client-side window.location.pathname regex parsing with a server-side object_id from the Django change-form context? That way it won't be lost.

(ᵔ ᴥ ᵔ)

🧠 Learnings used 
Learnt from: CR
Repo: openwisp/openwisp-notifications PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-02T20:23:46.321Z
Learning: Applies to openwisp_notifications/{serializers,views,handlers,tasks}.py : Preserve validation and permission checks when changing serializers, views, handlers, or tasks that include notification payloads with related objects and URLs

Learnt from: Dhanus3133
Repo: openwisp/openwisp-notifications PR: 469
File: openwisp_notifications/templatetags/notification_tags.py:46-52
Timestamp: 2026-05-31T08:08:31.780Z
Learning: In openwisp-notifications, the Django admin's queryset/row-level access control already restricts a staff user to viewing only user records that belong to organizations they manage. This means the `has_notification_setting_permission` template tag (in `openwisp_notifications/templatetags/notification_tags.py`) only needs to check the global `change_notificationsetting` permission — no additional org-management check is required in the tag itself. By the time a staff user can see the "Notification Preferences" button on a target user's admin change page, the admin layer has already verified org-membership, which is the same condition enforced by `NotificationPreferenceView._has_other_user_permission()` in `openwisp_notifications/views.py`. So there is no risk of the button leading to a 403.


{% block admin_change_form_document_ready %}
{{ block.super }}
{% if change %}
<script>
const owNotifyAppLabel = '{{ opts.app_label }}',
owNotifyModelName = '{{ opts.model_name }}',
uuidRegex = /\/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i,
owIsChangeForm = true;
let objectId;
try{
objectId = uuidRegex.exec(window.location.pathname)[0];
}
catch(error){
try{
objectId = /\/(\d+)\//.exec(window.location.pathname)[0];
}
catch(error){
throw error;
}
}
const owNotifyObjectId = objectId.replace(/\//g,'');
</script>
{% endif %}
{% endblock %}
28 changes: 28 additions & 0 deletions openwisp_notifications/tests/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.contrib.auth import get_user_model
from django.core.cache import cache
from django.forms.widgets import MediaOrderConflictWarning
from django.templatetags.static import static
from django.test import TestCase, override_settings, tag
from django.urls import reverse

Expand Down Expand Up @@ -193,6 +194,33 @@ def test_notification_preferences_button_staff_user(self):
response = self.client.get(user_admin_page)
self.assertNotContains(response, expected_html, html=True)

def test_base_site_template(self):
response = self.client.get(self._url)
self.assertContains(response, static("openwisp-notifications/css/loader.css"))
self.assertContains(
response, static("openwisp-notifications/css/notifications.css")
)
self.assertContains(
response, static("openwisp-notifications/js/notifications.js")
)
self.assertContains(
response,
static("openwisp-notifications/js/vendor/reconnecting-websocket.min.js"),
)
self.assertContains(response, 'id="openwisp_notifications"')
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def test_change_form_template(self):
org = self._get_org()
url = reverse(
f"admin:{self.users_app_label}_organization_change", args=(org.pk,)
)
response = self.client.get(url)
self.assertContains(response, "owIsChangeForm = true")
self.assertContains(
response, f"const owNotifyAppLabel = '{self.users_app_label}'"
)
self.assertContains(response, "owNotifyModelName = 'organization'")

def test_notification_preferences_button_with_permission(self):
perm = get_notification_setting_permission("change")
org = self._get_org()
Expand Down
5 changes: 5 additions & 0 deletions tests/openwisp2/sample_notifications/templates/admin/base_site.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{% extends 'admin/openwisp_notifications/base_site.html' %}
{% block user-tools %}
{{ block.super }}
<div id="sample-notifications-base-site"></div>
{% endblock %}
5 changes: 5 additions & 0 deletions tests/openwisp2/sample_notifications/templates/admin/change_form.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{% extends 'admin/openwisp_notifications/change_form.html' %}
{% block admin_change_form_document_ready %}
{{ block.super }}
<div id="sample-notifications-change-form"></div>
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{% endblock %}
13 changes: 13 additions & 0 deletions tests/openwisp2/sample_notifications/tests/test_django.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from django.apps.registry import apps
from django.contrib.contenttypes.models import ContentType
from django.core.cache import cache
from django.urls import reverse

from openwisp_notifications.swapper import load_model, swapper_load_model
from openwisp_notifications.tests.test_admin import TestAdmin as BaseTestAdmin
Expand Down Expand Up @@ -45,6 +46,18 @@
class TestAdmin(BaseTestAdmin):
app_label = "sample_notifications"

def test_base_site_template(self):
response = self.client.get(self._url)
self.assertContains(response, 'id="sample-notifications-base-site"')

def test_change_form_template(self):
org = self._get_org()
url = reverse(
f"admin:{self.users_app_label}_organization_change", args=(org.pk,)
)
response = self.client.get(url)
self.assertContains(response, 'id="sample-notifications-change-form"')


class TestAdminMedia(BaseTestAdminMedia):
pass
Expand Down
5 changes: 3 additions & 2 deletions tests/openwisp2/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@
"OPTIONS": {
"loaders": [
"django.template.loaders.filesystem.Loader",
"openwisp_utils.loaders.DependencyLoader",
"django.template.loaders.app_directories.Loader",
"openwisp_utils.loaders.DependencyLoader",
],
"context_processors": [
"django.template.context_processors.debug",
Expand Down Expand Up @@ -203,9 +203,10 @@
pass

if os.environ.get("SAMPLE_APP", False):
notifications_index = INSTALLED_APPS.index("openwisp_notifications")
INSTALLED_APPS.insert(notifications_index, "openwisp2.sample_notifications")
INSTALLED_APPS.remove("openwisp_notifications")
EXTENDED_APPS = ["openwisp_notifications"]
INSTALLED_APPS.append("openwisp2.sample_notifications")
OPENWISP_NOTIFICATIONS_NOTIFICATION_MODEL = "sample_notifications.Notification"
OPENWISP_NOTIFICATIONS_NOTIFICATIONSETTING_MODEL = (
"sample_notifications.NotificationSetting"
Expand Down
Loading