Review admin template structure#475
Conversation
|
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 extracts notification UI into dedicated admin templates (openwisp_notifications/openwisp_notifications/{base_site.html,change_form.html}), adds JS in the change form that exports owNotifyAppLabel/owNotifyModelName/owNotifyObjectId and sets owIsChangeForm, updates the original admin templates to extend the new notification templates, introduces sample app admin templates used by tests, adds admin tests that assert presence of notification assets and placeholders, and adjusts test TEMPLATES loader order and SAMPLE_APP INSTALLED_APPS insertion. Sequence Diagram(s)sequenceDiagram
participant Browser
participant DjangoAdmin
participant notifications_js as notifications.js
participant ReconnectingWS as ReconnectingWebSocket
participant NotificationsAPI
Browser->>DjangoAdmin: GET admin page (base/change_form)
DjangoAdmin->>Browser: render HTML with notifications assets & inline JS
Browser->>notifications_js: load notifications.js
notifications_js->>ReconnectingWS: open ws to /ws/notification/
ReconnectingWS->>NotificationsAPI: websocket messages (subscribe/poll)
NotificationsAPI->>ReconnectingWS: push notification events
ReconnectingWS->>notifications_js: deliver events
notifications_js->>Browser: update notification UI / play sound
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_notifications/tests/test_admin.py`:
- Around line 191-199: The test TestAdmin.test_base_site_template is asserting
hard-coded {% static %} paths which break when ManifestStaticFilesStorage
rewrites filenames; instead resolve the expected URLs via Django's static file
storage and assert those values (e.g., use django.templatetags.static.static or
staticfiles_storage.url to build the expected paths) when checking response
contents in test_base_site_template, or alternatively mark the test with
`@tag`("skip_prod") if you prefer skipping it in prod-like environments; update
the assertions that reference "openwisp-notifications/css/loader.css",
"openwisp-notifications/css/notifications.css",
"openwisp-notifications/js/notifications.js",
"openwisp-notifications/js/vendor/reconnecting-websocket.min.js" to use the
resolved URLs.
In `@tests/openwisp2/sample_notifications/templates/admin/change_form.html`:
- Around line 2-4: The test fixture overrides the script-only block
admin_change_form_document_ready with UI markup; remove the <div
id="sample-notifications-change-form"> from admin_change_form_document_ready and
place that markup into a visual/body block (e.g., admin_change_form or the main
content/body block used by the template) so admin_change_form_document_ready
remains script-only (keeping JS bootstrap like owNotifyAppLabel,
owNotifyModelName, owIsChangeForm in that block).
🪄 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: a3e3b79d-25d6-4557-9b14-c4f90f9570cb
📒 Files selected for processing (4)
openwisp_notifications/tests/test_admin.pytests/openwisp2/sample_notifications/templates/admin/base_site.htmltests/openwisp2/sample_notifications/templates/admin/change_form.htmltests/openwisp2/sample_notifications/tests/test_django.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). (14)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}: General: Flag potential security vulnerabilities in code
General: Flag obvious performance regressions such as heavy loops, repeated I/O, or unoptimized queries
General: Avoid unnecessary comments or docstrings for code that is already clear
General: Code formatting must be compact and readable; do not add excessive blank lines, especially inside function or method bodies
General: Flag unused or redundant code
General: Ensure variables, functions, classes, and files have descriptive and consistent names
General: New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
tests/openwisp2/sample_notifications/tests/test_django.pyopenwisp_notifications/tests/test_admin.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,sh,bash}
📄 CodeRabbit inference engine (Custom checks)
General: 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:
tests/openwisp2/sample_notifications/tests/test_django.pyopenwisp_notifications/tests/test_admin.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,md,rst,txt,adoc}
📄 CodeRabbit inference engine (Custom checks)
General: Flag outdated or incorrect documentation, comments, or docstrings
Files:
tests/openwisp2/sample_notifications/tests/test_django.pyopenwisp_notifications/tests/test_admin.py
**/*.py
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/openwisp2/sample_notifications/tests/test_django.pyopenwisp_notifications/tests/test_admin.py
openwisp_notifications/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mark user-facing strings as translatable with Django i18n helpers, typically
gettext_lazyimported as_
Files:
openwisp_notifications/tests/test_admin.py
openwisp_notifications/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_notifications/tests/**/*.py: Add or update tests for behavior changes
For bug fixes, prefer the red/green workflow: write or update the regression test first, run it against the unfixed code and confirm the failure message is clear, then apply the fix and rerun the targeted test plus the relevant module
Files:
openwisp_notifications/tests/test_admin.py
🧠 Learnings (1)
📚 Learning: 2026-05-07T16:17:11.806Z
Learnt from: pandafy
Repo: openwisp/openwisp-notifications PR: 450
File: openwisp_notifications/tests/test_selenium.py:276-285
Timestamp: 2026-05-07T16:17:11.806Z
Learning: In OpenWISP projects’ test code (e.g., openwisp-notifications), Celery is configured project-wide to run tasks eagerly/synchronously during tests (e.g., `CELERY_TASK_ALWAYS_EAGER=True` or an equivalent setting). Therefore, when reviewing these tests, don’t flag “race condition” or “async Celery” concerns for code that assumes task completion—tasks are guaranteed to finish before execution proceeds to the next line of test code.
Applied to files:
openwisp_notifications/tests/test_admin.py
🪛 HTMLHint (1.9.2)
tests/openwisp2/sample_notifications/templates/admin/base_site.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
tests/openwisp2/sample_notifications/templates/admin/change_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (3)
openwisp_notifications/tests/test_admin.py (1)
201-211: LGTM!tests/openwisp2/sample_notifications/templates/admin/base_site.html (1)
1-5: LGTM!tests/openwisp2/sample_notifications/tests/test_django.py (1)
49-59: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_notifications/templates/admin/openwisp_notifications/base_site.html`:
- 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).
In
`@openwisp_notifications/templates/admin/openwisp_notifications/change_form.html`:
- Around line 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.
🪄 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: 9a0639ce-4503-4d1f-8dc2-e09448498258
📒 Files selected for processing (7)
openwisp_notifications/templates/admin/base_site.htmlopenwisp_notifications/templates/admin/change_form.htmlopenwisp_notifications/templates/admin/openwisp_notifications/base_site.htmlopenwisp_notifications/templates/admin/openwisp_notifications/change_form.htmltests/openwisp2/sample_notifications/templates/admin/base_site.htmltests/openwisp2/sample_notifications/templates/admin/change_form.htmltests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}: General: Flag potential security vulnerabilities in code
General: Flag obvious performance regressions such as heavy loops, repeated I/O, or unoptimized queries
General: Avoid unnecessary comments or docstrings for code that is already clear
General: Code formatting must be compact and readable; do not add excessive blank lines, especially inside function or method bodies
General: Flag unused or redundant code
General: Ensure variables, functions, classes, and files have descriptive and consistent names
General: New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
tests/openwisp2/settings.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,sh,bash}
📄 CodeRabbit inference engine (Custom checks)
General: 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:
tests/openwisp2/settings.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,md,rst,txt,adoc}
📄 CodeRabbit inference engine (Custom checks)
General: Flag outdated or incorrect documentation, comments, or docstrings
Files:
tests/openwisp2/settings.py
**/*.py
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/openwisp2/settings.py
tests/openwisp2/settings.py
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode secrets into committed settings files; use
tests/openwisp2/local_settings.pyonly for local overrides
Files:
tests/openwisp2/settings.py
🪛 HTMLHint (1.9.2)
tests/openwisp2/sample_notifications/templates/admin/change_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
openwisp_notifications/templates/admin/change_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
openwisp_notifications/templates/admin/base_site.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
openwisp_notifications/templates/admin/openwisp_notifications/base_site.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[warning] 23-23: The type attribute must be present on elements.
(button-type-require)
[warning] 58-58: The type attribute must be present on
elements.(button-type-require)
[warning] 61-61: The type attribute must be present on
elements.(button-type-require)
tests/openwisp2/sample_notifications/templates/admin/base_site.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
openwisp_notifications/templates/admin/openwisp_notifications/change_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (1)
tests/openwisp2/sample_notifications/templates/admin/change_form.html (1)
2-4: Keepadmin_change_form_document_readyscript-only.Line 4 still injects visible markup into Django’s document-ready hook. Please move the placeholder into a body/content block so the sample fixture exercises the supported extension point instead.
| {% block user-tools %} | ||
| {{ block.super }} | ||
| {% if request|should_load_notifications_widget %} | ||
| <button class="ow-notifications toggle-btn" id="openwisp_notifications"> |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
not the point of this PR
There was a problem hiding this comment.
@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.
| {% 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 %} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is not the point of this PR.
There was a problem hiding this comment.
@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.
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_notifications/tests/test_admin.py`:
- Around line 327-334: The test currently enforces org-scope filtering inside
the template tag has_notification_setting_permission by asserting False for
target_outside; remove that assertion and instead add an admin access test that
verifies the admin user-change view for target_outside is inaccessible to
requester (expect 403/404 or no access), while keeping the template tag test
focused on permission-only behavior (e.g.,
assertTrue(has_notification_setting_permission(requester)) with no target).
Locate tests referencing has_notification_setting_permission and replace the
target_outside assertion with a request to the admin user-change page for
target_outside, asserting the appropriate access-denied response, leaving the
tag-only permission assertion unchanged.
🪄 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: 4dc02468-6d35-434b-8a63-310d4bcc8a30
📒 Files selected for processing (1)
openwisp_notifications/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). (12)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}: General: Flag potential security vulnerabilities in code
General: Flag obvious performance regressions such as heavy loops, repeated I/O, or unoptimized queries
General: Avoid unnecessary comments or docstrings for code that is already clear
General: Code formatting must be compact and readable; do not add excessive blank lines, especially inside function or method bodies
General: Flag unused or redundant code
General: Ensure variables, functions, classes, and files have descriptive and consistent names
General: New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_notifications/tests/test_admin.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,sh,bash}
📄 CodeRabbit inference engine (Custom checks)
General: 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_notifications/tests/test_admin.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,md,rst,txt,adoc}
📄 CodeRabbit inference engine (Custom checks)
General: Flag outdated or incorrect documentation, comments, or docstrings
Files:
openwisp_notifications/tests/test_admin.py
**/*.py
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
**/*.py: Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
openwisp_notifications/tests/test_admin.py
openwisp_notifications/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mark user-facing strings as translatable with Django i18n helpers, typically
gettext_lazyimported as_
Files:
openwisp_notifications/tests/test_admin.py
🧠 Learnings (8)
📚 Learning: 2026-05-15T17:42:22.451Z
Learnt from: nemesifier
Repo: openwisp/openwisp-notifications PR: 0
File: :0-0
Timestamp: 2026-05-15T17:42:22.451Z
Learning: In the openwisp-notifications repository (Python/Django), Selenium browser tests should only be written for JavaScript-centric functionality. Server-side behavior (such as redirects, access control, and Django messages framework usage) is best tested with Python unit tests and does not require Selenium coverage.
Applied to files:
openwisp_notifications/tests/test_admin.py
📚 Learning: 2026-04-18T15:15:46.991Z
Learnt from: CR
Repo: openwisp/openwisp-notifications PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-04-18T15:15:46.991Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,go,rb,php} : Bug Fixes: The test must be deterministic and not flaky; flag tests that depend on timing, sleeps, specific timezones, system time, randomness without a fixed seed, race conditions, concurrency timing, network access, external services, filesystem state, environment specific configuration, execution order, shared global state, hardcoded ports, or async operations that are not properly awaited
Applied to files:
openwisp_notifications/tests/test_admin.py
📚 Learning: 2026-05-31T08:08:31.780Z
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.
Applied to files:
openwisp_notifications/tests/test_admin.py
📚 Learning: 2026-06-02T20:23:46.321Z
Learnt from: CR
Repo: openwisp/openwisp-notifications PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-02T20:23:46.321Z
Learning: Run focused notification setting tests and check staff, superuser, organization admin, and regular user transitions when changing notification setting creation/deletion
Applied to files:
openwisp_notifications/tests/test_admin.py
📚 Learning: 2026-06-02T20:23:46.321Z
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
Applied to files:
openwisp_notifications/tests/test_admin.py
📚 Learning: 2026-06-02T20:23:46.321Z
Learnt from: CR
Repo: openwisp/openwisp-notifications PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-02T20:23:46.321Z
Learning: Applies to **/models.py : Preserve swappable model support and integration with `openwisp-users` organizations and memberships in Django models
Applied to files:
openwisp_notifications/tests/test_admin.py
📚 Learning: 2026-06-02T20:23:46.321Z
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/**/*.py : Mark user-facing strings as translatable with Django i18n helpers, typically `gettext_lazy` imported as `_`
Applied to files:
openwisp_notifications/tests/test_admin.py
📚 Learning: 2026-06-02T20:23:46.321Z
Learnt from: CR
Repo: openwisp/openwisp-notifications PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-02T20:23:46.321Z
Learning: Be careful with notification preference inheritance, global notification settings, soft-deleted `NotificationSetting` rows, and user/organization permission boundaries
Applied to files:
openwisp_notifications/tests/test_admin.py
🔇 Additional comments (1)
openwisp_notifications/tests/test_admin.py (1)
16-29: LGTM!Also applies to: 172-195, 224-307
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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_notifications/tests/test_admin.py`:
- Around line 327-334: The test currently enforces org-scope filtering inside
the template tag has_notification_setting_permission by asserting False for
target_outside; remove that assertion and instead add an admin access test that
verifies the admin user-change view for target_outside is inaccessible to
requester (expect 403/404 or no access), while keeping the template tag test
focused on permission-only behavior (e.g.,
assertTrue(has_notification_setting_permission(requester)) with no target).
Locate tests referencing has_notification_setting_permission and replace the
target_outside assertion with a request to the admin user-change page for
target_outside, asserting the appropriate access-denied response, leaving the
tag-only permission assertion unchanged.
🪄 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: 4dc02468-6d35-434b-8a63-310d4bcc8a30
📒 Files selected for processing (1)
openwisp_notifications/tests/test_admin.py
📜 Review details
🔇 Additional comments (1)
openwisp_notifications/tests/test_admin.py (1)
16-29: LGTM!Also applies to: 172-195, 224-307
🛑 Comments failed to post (1)
openwisp_notifications/tests/test_admin.py (1)
327-334:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
has_notification_setting_permissionassertion encodes the wrong layer contract.This test expects org-scope filtering inside the template tag (
assertFalsefortarget_outside), but that check belongs to admin/queryset/view access control. Keeping this assertion can lock in behavior the project explicitly avoids and make the test fail for valid implementations. Replace this with an access test on the admin user-change page fortarget_outside(e.g., 403/404/no access), and keep the tag assertion focused on permission-only behavior.Based on learnings: “the
has_notification_setting_permissiontemplate tag ... only needs to check the globalchange_notificationsettingpermission — no additional org-management check is required in the tag itself.”🤖 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/tests/test_admin.py` around lines 327 - 334, The test currently enforces org-scope filtering inside the template tag has_notification_setting_permission by asserting False for target_outside; remove that assertion and instead add an admin access test that verifies the admin user-change view for target_outside is inaccessible to requester (expect 403/404 or no access), while keeping the template tag test focused on permission-only behavior (e.g., assertTrue(has_notification_setting_permission(requester)) with no target). Locate tests referencing has_notification_setting_permission and replace the target_outside assertion with a request to the admin user-change page for target_outside, asserting the appropriate access-denied response, leaving the tag-only permission assertion unchanged.
1 participant
Checklist
Reference to Existing Issue
This addresses #474
Problem: The admin templates admin/base_site.html and admin/change_form.html lived at their original Django paths, making it impossible for other apps to cleanly extend or override them via the standard OpenWISP template loading chain.
Solution: Namespace the templates by moving the real content to admin/openwisp_notifications/base_site.html and admin/openwisp_notifications/change_form.html. The original paths become minimal one-line proxies that extend the namespaced versions, preserving backward compatibility.
This eliminates the ambiguous template resolution that occurred when another app (e.g. sample_notifications) tried to extend the same-named template.
Description of Changes
admin/base_site.html->admin/openwisp_notifications/base_site.htmladmin/change_form.html->admin/openwisp_notifications/change_form.html{% extends %}proxiesDependencyLoaderafterapp_directories.Loader) so the standard loader resolves template names firstsample_notificationsimmediately afteropenwisp_notificationsin INSTALLED_APPS (instead of appending) for correct override resolutiontest_base_site_templateandtest_change_form_templateto verify template rendering, asset inclusion (static()), and the full inheritance chain