[fix] OpenWisp DependencyLoader should be after django template loaders#490
[fix] OpenWisp DependencyLoader should be after django template loaders#490asmodehn wants to merge 2 commits intoopenwisp:masterfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR reorders template loaders in the tests settings so DependencyLoader comes after app_directories.Loader and removes a specific DIRS entry. It centralizes admin templates by adding admin/openwisp_users/action_confirmation.html and admin/openwisp_users/login.html, and updates existing openwisp_users and test sample admin templates to extend those new centralized templates. Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_users/templates/admin/login.html`:
- Line 1: The current extends in openwisp_users/templates/admin/login.html
creates a circular inheritance with admin/openwisp_users/login.html because both
resolve to the same local path; fix by breaking the name collision: either
rename one of the templates (e.g., rename local
openwisp_users/templates/admin/login.html to openwisp_login.html and update any
references), or remove the extends from admin/openwisp_users/login.html and make
it extend Django's actual admin template directly (so it no longer points to
admin/login.html in the same directory), or move the parent template out of
openwisp_users/templates/admin/ (or give it a unique name) so that extending
"admin/login.html" refers to Django's contrib admin template rather than the
local file. Ensure any changed template names are updated where referenced.
In `@openwisp_users/templates/admin/openwisp_users/action_confirmation.html`:
- Around line 10-11: The template block bodyclass currently injects semantic
"delete" CSS classes (delete-confirmation, delete-selected-confirmation) even
for activation/deactivation flows; update the block bodyclass in the
openwisp_users/templates/admin/openwisp_users/action_confirmation.html template
to emit descriptive classes (e.g., activate-confirmation,
deactivate-confirmation or activate-selected-confirmation) instead of the
delete-* names, or make the class selection conditional based on the
view/context action flag (e.g., an action or is_deactivation variable) so the
rendered class matches the actual activate/deactivate behavior.
- Around line 30-37: The template calls queryset.all twice which can cause
duplicate DB queries; cache the result into a template variable using a {% with
%} (e.g., assign queryset.all to a local name) and then iterate that cached
variable in both places (the first loop that renders the list of users and the
second loop that emits the hidden inputs for '_selected_action'), updating the
loops that use 'user' and 'obj' to iterate the cached variable instead of
calling queryset.all again.
In `@openwisp_users/templates/admin/openwisp_users/login.html`:
- Line 43: The "Forgot Password?" link text is not wrapped for translation;
update the anchor in the template that uses the URL name account_reset_password
so the link text is wrapped with the Django translation tag (e.g., {% trans
"Forgot Password?" %} or {% blocktrans %}Forgot Password?{% endblocktrans %}) to
ensure i18n consistency with the rest of
openwisp_users/templates/admin/openwisp_users/login.html.
- Line 46: The submit button currently uses a hard-coded value "Log in" in the
input element (input type="submit" value="Log in") — make the label translatable
by replacing the literal with a Django translation tag (e.g., value="{% trans
'Log in' %}" or using blocktrans) and ensure the template loads the i18n library
with {% load i18n %} at the top so the translation tag works.
- Around line 22-25: The template currently declares a block named "trimmed"
instead of using the translation tag with whitespace trimming; replace the
erroneous "{% block trimmed %}...{% endblock %}" with a translation block using
the trimmed option (i.e. use "{% blocktrans trimmed %}" and "{% endblocktrans
%}") around the message so whitespace is trimmed and the string is translatable;
update the block that contains "You are authenticated as {{ username }}, but are
not authorized..." accordingly and remove the unintended "trimmed" block
declaration.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
openwisp_users/templates/admin/action_confirmation.htmlopenwisp_users/templates/admin/login.htmlopenwisp_users/templates/admin/openwisp_users/action_confirmation.htmlopenwisp_users/templates/admin/openwisp_users/login.htmltests/openwisp2/sample_users/templates/admin/action_confirmation.htmltests/openwisp2/sample_users/templates/admin/login.htmltests/openwisp2/settings.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~=5.1.0
- GitHub Check: Python==3.13 | 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.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.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.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (4)
openwisp_users/templates/admin/action_confirmation.html (1)
1-1: LGTM!Clean delegation to the centralized template. This pattern allows extending applications to override this template while inheriting the base functionality from
admin/openwisp_users/action_confirmation.html.tests/openwisp2/settings.py (1)
93-97: LGTM! Loader order correctly places DependencyLoader after Django's standard loaders.The new order (
filesystem.Loader→app_directories.Loader→DependencyLoader) ensures standard Django template resolution takes precedence, which is the correct behavior when extending apps need to override templates from dependencies. This aligns with the PR objective to fix issue#489.tests/openwisp2/sample_users/templates/admin/login.html (1)
1-1: LGTM!Correct test template demonstrating the inheritance pattern for extending applications. This validates that template resolution works correctly with the updated loader order.
tests/openwisp2/sample_users/templates/admin/action_confirmation.html (1)
1-1: LGTM!Correct test template demonstrating the inheritance pattern for the action confirmation page, consistent with the login template approach.
|
|
||
| </div> | ||
| {% endblock %} | ||
| {% extends 'admin/openwisp_users/login.html' %} No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check template inheritance structure
echo "=== Template files in openwisp_users/templates/admin/ ==="
find openwisp_users/templates/admin/ -type f 2>/dev/null | sort
echo ""
echo "=== Checking extends directives in all admin templates ==="
find openwisp_users/templates/admin/ -type f -name "*.html" -exec sh -c 'echo "File: $1"; rg -n "{% extends" "$1" 2>/dev/null || echo "(no extends found)"' _ {} \;
echo ""
echo "=== Full content of login.html ==="
cat -n openwisp_users/templates/admin/login.html 2>/dev/null
echo ""
echo "=== Check if admin/openwisp_users/login.html exists and its content ==="
if [ -f "openwisp_users/templates/admin/openwisp_users/login.html" ]; then
echo "File exists:"
cat -n openwisp_users/templates/admin/openwisp_users/login.html
else
echo "File does not exist at: openwisp_users/templates/admin/openwisp_users/login.html"
fiRepository: openwisp/openwisp-users
Length of output: 2795
Fix circular template inheritance that will cause infinite recursion.
This template extends 'admin/openwisp_users/login.html', which in turn extends "admin/login.html". Since both templates exist in the local openwisp_users/templates/admin/ directory, Django's template loader will resolve "admin/login.html" back to the local openwisp_users/templates/admin/login.html, creating a circular inheritance chain that will fail at runtime.
Restructure the inheritance to avoid the cycle—for example:
- Remove the extends directive from
admin/login.htmland make it extend Django's admin template directly, or - Rename one of the templates to eliminate the naming conflict, or
- Reorganize the inheritance hierarchy so parent and child templates have unambiguous names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_users/templates/admin/login.html` at line 1, The current extends in
openwisp_users/templates/admin/login.html creates a circular inheritance with
admin/openwisp_users/login.html because both resolve to the same local path; fix
by breaking the name collision: either rename one of the templates (e.g., rename
local openwisp_users/templates/admin/login.html to openwisp_login.html and
update any references), or remove the extends from
admin/openwisp_users/login.html and make it extend Django's actual admin
template directly (so it no longer points to admin/login.html in the same
directory), or move the parent template out of openwisp_users/templates/admin/
(or give it a unique name) so that extending "admin/login.html" refers to
Django's contrib admin template rather than the local file. Ensure any changed
template names are updated where referenced.
There was a problem hiding this comment.
Hmm I didn't experience this in my local setup... but what do you mean exactly by "make it extend Django's admin template directly" ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check INSTALLED_APPS order in test settings
echo "=== INSTALLED_APPS in test settings ==="
cat tests/openwisp2/settings.py | grep -A 50 "INSTALLED_APPS" | head -60
echo ""
echo "=== Check if django.contrib.admin appears before openwisp_users ==="
cat tests/openwisp2/settings.py | grep -E "(django\.contrib\.admin|openwisp_users)" | head -20
Length of output: 2480
@asmodehn, good question! Let me clarify the concern and explain what I meant.
The Circular Inheritance Issue
Based on the INSTALLED_APPS order in your test settings, openwisp_users appears before django.contrib.admin:
INSTALLED_APPS = [
# ... other apps ...
"openwisp_users", # position 11
"openwisp_utils.admin_theme",
"rest_framework",
"rest_framework.authtoken",
"django.contrib.admin", # position 15
# ...
]This means Django's template loader will search openwisp_users/templates/ before django/contrib/admin/templates/ when resolving template paths.
Here's the inheritance chain:
openwisp_users/templates/admin/login.htmlextends'admin/openwisp_users/login.html'✓openwisp_users/templates/admin/openwisp_users/login.htmlextends"admin/login.html"- Django resolves
"admin/login.html"→ findsopenwisp_users/templates/admin/login.html(because openwisp_users comes first) → circular reference 🔄
What I Meant by "Extend Django's Admin Template Directly"
I meant that admin/openwisp_users/login.html should extend Django's built-in admin login template from django/contrib/admin/templates/admin/login.html, not the local openwisp_users/templates/admin/login.html.
However, Django's template system doesn't have a built-in way to "skip" templates in the resolution order, so this requires restructuring.
Why You Might Not Have Seen the Error
The circular inheritance would only trigger when you actually render the login page. Did you:
- Access
/admin/login/in your browser? - Run tests that render the admin login template?
If you only ran migrations or other tests that don't render this template, you wouldn't see the recursion error.
Suggested Solution
Rename one of the templates to break the naming conflict:
Option 1: Rename the wrapper template:
# openwisp_users/templates/admin/openwisp_login.html
{% extends 'admin/openwisp_users/login.html' %}Then update any code that references it.
Option 2: Use a different directory structure to make the intent clearer.
Could you verify by accessing the admin login page or running a test that renders it? That should help us confirm whether this is an actual issue or if I'm missing something about your setup.
There was a problem hiding this comment.
I ran the tests (that specifically check that template content) and I also checked in the browser, there was no problem rendering that template. But I should be able to make it more explicit in any case, to avoid any potential confusion...
There was a problem hiding this comment.
@asmodehn, thank you for testing and clarifying! I was mistaken about the circular inheritance concern. You're absolutely right that it works correctly.
Why It Works
Django's template loader is smarter than I initially described. When openwisp_users/templates/admin/openwisp_users/login.html extends "admin/login.html", Django doesn't restart the search from the beginning. Instead, it continues searching through the remaining template locations after the current one, effectively "skipping" the template that's already in the inheritance chain. This is the standard Django mechanism that enables template overriding and extension patterns.
Your inheritance chain:
openwisp_users/templates/admin/login.html→ extends'admin/openwisp_users/login.html'openwisp_users/templates/admin/openwisp_users/login.html→ extends"admin/login.html"→ resolves to Django'sdjango/contrib/admin/templates/admin/login.html(because the loader continues past openwisp_users)
This is the correct pattern for extending Django admin templates from within an app.
Optional Clarifications
If you'd like to make the intent more explicit for future maintainers, you could:
-
Add a comment in
admin/openwisp_users/login.html:{# Extends Django's built-in admin login template #} {% extends "admin/login.html" %}
-
Document the template hierarchy in a docstring or README section explaining the three-layer structure.
But the current code is functionally correct, so these are purely optional documentation improvements. The PR structure looks good!
🐰
| {% block bodyclass %}{{ block.super }} app-{{ opts.app_label }} model-{{ opts.model_name }} delete-confirmation | ||
| delete-selected-confirmation{% endblock %} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Body classes reference "delete" actions but template handles activate/deactivate.
The CSS classes delete-confirmation and delete-selected-confirmation may be intentionally reused for consistent admin styling, but they're semantically misleading for activation status changes. Consider whether custom classes would be more appropriate for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_users/templates/admin/openwisp_users/action_confirmation.html`
around lines 10 - 11, The template block bodyclass currently injects semantic
"delete" CSS classes (delete-confirmation, delete-selected-confirmation) even
for activation/deactivation flows; update the block bodyclass in the
openwisp_users/templates/admin/openwisp_users/action_confirmation.html template
to emit descriptive classes (e.g., activate-confirmation,
deactivate-confirmation or activate-selected-confirmation) instead of the
delete-* names, or make the class selection conditional based on the
view/context action flag (e.g., an action or is_deactivation variable) so the
rendered class matches the actual activate/deactivate behavior.
| {% for user in queryset.all %} | ||
| <li>{% trans "User" %}: {{ user }}</li> | ||
| {% endfor %} | ||
| </ul> | ||
| <form action='' method='post'>{% csrf_token %} | ||
| {% for obj in queryset.all %} | ||
| <input type='hidden' name='_selected_action' value='{{ obj.pk|unlocalize }}'/> | ||
| {% endfor %} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching the queryset to avoid duplicate database queries.
queryset.all is called twice (lines 30 and 35), which may trigger two separate database queries. While Django's QuerySet caching may help in some cases, explicitly iterating twice over queryset.all can cause redundant queries.
♻️ Proposed fix using {% with %} to cache the queryset
+{% with users=queryset.all %}
<ul>
- {% for user in queryset.all %}
+ {% for user in users %}
<li>{% trans "User" %}: {{ user }}</li>
{% endfor %}
</ul>
<form action='' method='post'>{% csrf_token %}
- {% for obj in queryset.all %}
+ {% for obj in users %}
<input type='hidden' name='_selected_action' value='{{ obj.pk|unlocalize }}'/>
{% endfor %}
+{% endwith %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_users/templates/admin/openwisp_users/action_confirmation.html`
around lines 30 - 37, The template calls queryset.all twice which can cause
duplicate DB queries; cache the result into a template variable using a {% with
%} (e.g., assign queryset.all to a local name) and then iterate that cached
variable in both places (the first loop that renders the list of users and the
second loop that emits the hidden inputs for '_selected_action'), updating the
loops that use 'user' and 'obj' to iterate the cached variable instead of
calling queryset.all again.
| {% block trimmed %} | ||
| You are authenticated as {{ username }}, but are not authorized to | ||
| access this page. Would you like to login to a different account? | ||
| {% endblock %} |
There was a problem hiding this comment.
Incorrect use of {% block trimmed %} - this creates a block named "trimmed" instead of trimming whitespace.
The trimmed option belongs on {% blocktrans trimmed %}, not as a separate block tag. This creates an unintended block named "trimmed" and does not trim whitespace from the message.
🐛 Proposed fix
{% if user.is_authenticated %}
<p class="errornote">
-{% block trimmed %}
+{% blocktrans trimmed %}
You are authenticated as {{ username }}, but are not authorized to
access this page. Would you like to login to a different account?
-{% endblock %}
+{% endblocktrans %}
</p>
{% endif %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% block trimmed %} | |
| You are authenticated as {{ username }}, but are not authorized to | |
| access this page. Would you like to login to a different account? | |
| {% endblock %} | |
| {% blocktrans trimmed %} | |
| You are authenticated as {{ username }}, but are not authorized to | |
| access this page. Would you like to login to a different account? | |
| {% endblocktrans %} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_users/templates/admin/openwisp_users/login.html` around lines 22 -
25, The template currently declares a block named "trimmed" instead of using the
translation tag with whitespace trimming; replace the erroneous "{% block
trimmed %}...{% endblock %}" with a translation block using the trimmed option
(i.e. use "{% blocktrans trimmed %}" and "{% endblocktrans %}") around the
message so whitespace is trimmed and the string is translatable; update the
block that contains "You are authenticated as {{ username }}, but are not
authorized..." accordingly and remove the unintended "trimmed" block
declaration.
| <input type="hidden" name="next" value="{{ next }}" /> | ||
| </div> | ||
| <div class="form-row"> | ||
| <a href="{% url 'account_reset_password' %}">Forgot Password?</a> |
There was a problem hiding this comment.
Missing translation wrapper for "Forgot Password?" link text.
For i18n consistency with the rest of the template, this string should be wrapped in {% trans %}.
🌐 Proposed fix
- <a href="{% url 'account_reset_password' %}">Forgot Password?</a>
+ <a href="{% url 'account_reset_password' %}">{% trans "Forgot Password?" %}</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_users/templates/admin/openwisp_users/login.html` at line 43, The
"Forgot Password?" link text is not wrapped for translation; update the anchor
in the template that uses the URL name account_reset_password so the link text
is wrapped with the Django translation tag (e.g., {% trans "Forgot Password?" %}
or {% blocktrans %}Forgot Password?{% endblocktrans %}) to ensure i18n
consistency with the rest of
openwisp_users/templates/admin/openwisp_users/login.html.
| <a href="{% url 'account_reset_password' %}">Forgot Password?</a> | ||
| </div> | ||
| <div class="submit-row"> | ||
| <input type="submit" value="Log in" /> |
There was a problem hiding this comment.
Missing translation wrapper for "Log in" button value.
For i18n consistency, the submit button text should be translatable.
🌐 Proposed fix
- <input type="submit" value="Log in" />
+ <input type="submit" value="{% trans 'Log in' %}" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input type="submit" value="Log in" /> | |
| <input type="submit" value="{% trans 'Log in' %}" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_users/templates/admin/openwisp_users/login.html` at line 46, The
submit button currently uses a hard-coded value "Log in" in the input element
(input type="submit" value="Log in") — make the label translatable by replacing
the literal with a Django translation tag (e.g., value="{% trans 'Log in' %}" or
using blocktrans) and ensure the template loads the i18n library with {% load
i18n %} at the top so the translation tag works.
asmodehn
force-pushed
the
fix_template_loaders_order_in_tests
branch
from
fd35697 to
4b65fbb
Compare
|
Here I use a "template proxy" trick (similar to what can be done with models), to make overriding in sample_app simple enough, so that we don't need to rely on the "change loader order" trick to avoid resolving the wrong template... |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
openwisp_users/templates/admin/openwisp_users/login.html (2)
22-25:⚠️ Potential issue | 🟡 MinorReplace the
trimmedblock withblocktrans trimmed.This currently defines a block named
trimmedinstead of a trimmed translation block.🐛 Proposed fix
-{% block trimmed %} +{% blocktrans trimmed %} You are authenticated as {{ username }}, but are not authorized to access this page. Would you like to login to a different account? -{% endblock %} +{% endblocktrans %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/templates/admin/openwisp_users/login.html` around lines 22 - 25, Replace the plain template block named "trimmed" with a translation block by changing the tag to "blocktrans trimmed" and closing it with "endblocktrans"; locate the block that currently reads {% block trimmed %} ... {% endblock %} around the message that includes the {{ username }} variable and wrap the content in the translation block so the message becomes a trimmed translatable string (keep the {{ username }} placeholder unchanged inside the blocktrans).
43-46:⚠️ Potential issue | 🟡 MinorWrap action labels in translation tags.
Line 43 and Line 46 should be translatable for i18n consistency.
🌐 Proposed fix
- <a href="{% url 'account_reset_password' %}">Forgot Password?</a> + <a href="{% url 'account_reset_password' %}">{% trans "Forgot Password?" %}</a> @@ - <input type="submit" value="Log in" /> + <input type="submit" value="{% trans 'Log in' %}" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/templates/admin/openwisp_users/login.html` around lines 43 - 46, The "Forgot Password?" link text and the submit button label "Log in" are not wrapped for translation; update the template strings in openwisp_users/templates/admin/openwisp_users/login.html by replacing the literal texts "Forgot Password?" and "Log in" with Django i18n tags (e.g., {% trans "Forgot Password?" %} for the anchor and value="{% trans "Log in" %}" for the input, or use {% blocktrans %} as appropriate) so both labels become translatable.openwisp_users/templates/admin/openwisp_users/action_confirmation.html (1)
29-37: 🧹 Nitpick | 🔵 TrivialAvoid iterating
queryset.alltwice.Use a local template variable so both loops consume the same evaluated queryset.
♻️ Proposed refactor
- <ul> - {% for user in queryset.all %} + {% with selected_users=queryset.all %} + <ul> + {% for user in selected_users %} <li>{% trans "User" %}: {{ user }}</li> {% endfor %} </ul> <form action='' method='post'>{% csrf_token %} - {% for obj in queryset.all %} + {% for obj in selected_users %} <input type='hidden' name='_selected_action' value='{{ obj.pk|unlocalize }}'/> {% endfor %} @@ </form> + {% endwith %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/templates/admin/openwisp_users/action_confirmation.html` around lines 29 - 37, The template currently calls queryset.all twice (in the <ul> loop and the hidden inputs loop), which can re-evaluate the queryset; assign queryset.all to a local template variable once (e.g., using {% with items=queryset.all %}) and iterate over that variable in both places so both loops consume the same evaluated queryset (update occurrences of queryset.all in action_confirmation.html to use the chosen local name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_users/templates/admin/openwisp_users/login.html`:
- Around line 4-8: Replace the hardcoded English error heading in the template's
conditional that checks form.errors and not form.non_field_errors with a
translatable plural-aware block using Django's blocktrans count; update the
template section around the form.errors check (the {% if form.errors and not
form.non_field_errors %} ... {% endif %} block) to use blocktrans with a count
variable (e.g., count=form.errors.items|length) and provide singular and plural
messages inside the blocktrans so the message is properly translated and
pluralized.
---
Duplicate comments:
In `@openwisp_users/templates/admin/openwisp_users/action_confirmation.html`:
- Around line 29-37: The template currently calls queryset.all twice (in the
<ul> loop and the hidden inputs loop), which can re-evaluate the queryset;
assign queryset.all to a local template variable once (e.g., using {% with
items=queryset.all %}) and iterate over that variable in both places so both
loops consume the same evaluated queryset (update occurrences of queryset.all in
action_confirmation.html to use the chosen local name).
In `@openwisp_users/templates/admin/openwisp_users/login.html`:
- Around line 22-25: Replace the plain template block named "trimmed" with a
translation block by changing the tag to "blocktrans trimmed" and closing it
with "endblocktrans"; locate the block that currently reads {% block trimmed %}
... {% endblock %} around the message that includes the {{ username }} variable
and wrap the content in the translation block so the message becomes a trimmed
translatable string (keep the {{ username }} placeholder unchanged inside the
blocktrans).
- Around line 43-46: The "Forgot Password?" link text and the submit button
label "Log in" are not wrapped for translation; update the template strings in
openwisp_users/templates/admin/openwisp_users/login.html by replacing the
literal texts "Forgot Password?" and "Log in" with Django i18n tags (e.g., {%
trans "Forgot Password?" %} for the anchor and value="{% trans "Log in" %}" for
the input, or use {% blocktrans %} as appropriate) so both labels become
translatable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
openwisp_users/templates/admin/action_confirmation.htmlopenwisp_users/templates/admin/login.htmlopenwisp_users/templates/admin/openwisp_users/action_confirmation.htmlopenwisp_users/templates/admin/openwisp_users/login.htmltests/openwisp2/sample_users/templates/admin/action_confirmation.htmltests/openwisp2/sample_users/templates/admin/login.htmltests/openwisp2/settings.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.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (5)
openwisp_users/templates/admin/login.html (1)
1-1: Delegation to centralized login template looks good.This keeps the override thin and consistent with the new shared admin template structure.
openwisp_users/templates/admin/action_confirmation.html (1)
1-1: Thin delegation template is good here.This aligns with the centralization approach and avoids duplicate markup.
tests/openwisp2/sample_users/templates/admin/login.html (1)
1-1: Test fixture inheritance update is correct.Good alignment with the shared
admin/openwisp_users/login.htmlbase.tests/openwisp2/sample_users/templates/admin/action_confirmation.html (1)
1-1: Test fixture now correctly delegates to centralized confirmation template.This is consistent with the new template organization.
tests/openwisp2/settings.py (1)
93-97: Template loader order change is correct for the stated objective.Placing
DependencyLoaderafter Django’s filesystem/app loaders preserves expected app template resolution first.
| {% if form.errors and not form.non_field_errors %} | ||
| <p class="errornote"> | ||
| {% if form.errors.items|length == 1 %}Please correct the error below{% else %}Please correct the errors below{% endif %} | ||
| </p> | ||
| {% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/openwisp_users/templates/admin/openwisp_users/login.html" -type fRepository: openwisp/openwisp-users
Length of output: 125
🏁 Script executed:
wc -l openwisp_users/templates/admin/openwisp_users/login.html 2>/dev/null || echo "File not found in standard location"Repository: openwisp/openwisp-users
Length of output: 126
🏁 Script executed:
cat -n openwisp_users/templates/admin/openwisp_users/login.html 2>/dev/null || fd "login.html" | grep openwisp_users | head -5Repository: openwisp/openwisp-users
Length of output: 1788
🌐 Web query:
Django blocktrans count plural translation best practices
💡 Result:
Django blocktranslate / blocktrans with count + {% plural %}: best practices
-
Always use
count(notwith) to enable pluralization, and put your plural branch inside the same block using{% plural %}. Django uses thecountvalue to choose the correct plural form for the active language. [1]{% load i18n %} {% blocktranslate count n=items|length %} There is {{ n }} item. {% plural %} There are {{ n }} items. {% endblocktranslate %}
-
If you need extra variables, combine
with+countin the same tag (as in the docs’ “more complex example”). [1]{% blocktranslate with price=plan.price count years=plan.years %} Costs {{ price }} per year. {% plural %} Costs {{ price }} per {{ years }} years. {% endblocktranslate %}
-
Keep the whole user-facing phrase inside the translation block (don’t stitch together translated fragments). This gives translators freedom to reorder words and handle grammar naturally. [1]
-
Use the same placeholder/variable names in both singular and plural forms.
blocktranslatepluralization is internally converted to anngettext()call, so thengettextplaceholder rules apply; mismatched placeholder names can break message compilation. [1] -
Don’t implement your own “if count == 1” logic to choose singular/plural; plural rules vary widely by language. Let
ngettext/blocktranslatehandle it. [2] -
Avoid trying to bind the same template variable name to different values for singular vs plural branches (e.g., singular model name vs plural model name). Due to gettext/template-context constraints, this can produce incorrect results in some locales; it’s a known limitation discussed in Django’s issue tracker. Prefer wording that doesn’t require swapping the same placeholder between branches (or restructure so different placeholders aren’t needed). [3]
-
No other block tags inside
blocktranslate(beyond its own{% plural %}); preprocess values outside the block (including{% url ... as var %}) and then interpolate the variable. [1] -
Add context when the same English string could translate differently using the
contextkeyword (and in Python,pgettext/npgettext). [2][1]
Sources: Django translation docs (templates + pluralization + ngettext notes) [1][2], Django ticket on blocktrans pluralization limitations [3].
🏁 Script executed:
# Check if there are other similar patterns in the template or codebase
rg "form\.errors" openwisp_users/templates/ -A 2 -B 2Repository: openwisp/openwisp-users
Length of output: 712
🏁 Script executed:
# Check if blocktrans is used elsewhere in templates
rg "blocktrans" openwisp_users/templates/ -lRepository: openwisp/openwisp-users
Length of output: 205
Use blocktrans count for the error message to enable proper pluralization.
Line 6 contains hardcoded strings that are not translatable. The template loads i18n but these messages lack translation tags, which means non-English users will see an English error message. Use blocktrans count with a plural form to handle both singular and plural cases for proper internationalization.
🌐 Proposed fix
{% if form.errors and not form.non_field_errors %}
<p class="errornote">
-{% if form.errors.items|length == 1 %}Please correct the error below{% else %}Please correct the errors below{% endif %}
+{% blocktrans count error_count=form.errors|length %}
+Please correct the error below
+{% plural %}
+Please correct the errors below
+{% endblocktrans %}
</p>
{% endif %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_users/templates/admin/openwisp_users/login.html` around lines 4 - 8,
Replace the hardcoded English error heading in the template's conditional that
checks form.errors and not form.non_field_errors with a translatable
plural-aware block using Django's blocktrans count; update the template section
around the form.errors check (the {% if form.errors and not
form.non_field_errors %} ... {% endif %} block) to use blocktrans with a count
variable (e.g., count=form.errors.items|length) and provide singular and plural
messages inside the blocktrans so the message is properly translated and
pluralized.
4b65fbb to
098ad5c
Compare
1 participant
Checklist
Reference to Existing Issue
Closes #489
Description of Changes
Correct order of template loaders in tests settings : OpenWisp DependencyLoader must come after django template loaders to ensure correct templates are found when extending an app.