[fix] OpenWisp DependencyLoader should be after django template loaders#490
[fix] OpenWisp DependencyLoader should be after django template loaders#490asmodehn wants to merge 3 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughReorders template loaders in tests settings so Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
| {% 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
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_users/templates/admin/openwisp_users/login.html (1)
5-8:⚠️ Potential issue | 🟠 MajorUse proper Django i18n tags;
{% block trimmed %}is incorrect here.This template still contains hardcoded UI strings and uses
{% block trimmed %}, which defines a template block instead of a translatable trimmed message. Please switch these toblocktrans/transforms.🌐 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 %} @@ {% if user.is_authenticated %} <p class="errornote"> -{% block trimmed %} +{% blocktrans trimmed with username=username %} 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 %} @@ - <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' %}" />#!/bin/bash # Read-only verification for unresolved i18n/template-tag issues in this template. set -euo pipefail TARGET="openwisp_users/templates/admin/openwisp_users/login.html" nl -ba "$TARGET" | sed -n '1,120p' echo rg -n "{% block trimmed %}|Please correct the error below|Please correct the errors below|Forgot Password\?|value=\"Log in\"" "$TARGET"Also applies to: 23-27, 44-47
🤖 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 5 - 8, The template uses a non-i18n `{% block trimmed %}` and hardcoded strings inside the form error block and other UI bits (e.g., the conditional messages "Please correct the error below"/"Please correct the errors below", "Forgot Password?", and the login button's value="Log in"); replace the `{% block trimmed %}` usage with proper Django i18n tags and wrap all user-facing strings in `{% trans %}` or `{% blocktrans %}` as appropriate (use `{% blocktrans %}` for conditional/templated text like the pluralized error message and `{% trans %}` for simple labels), updating the error block around `form.errors` and the "Forgot Password?" and login button value to use those i18n tags so strings are translatable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_users/templates/admin/openwisp_users/login.html`:
- Around line 5-8: The template uses a non-i18n `{% block trimmed %}` and
hardcoded strings inside the form error block and other UI bits (e.g., the
conditional messages "Please correct the error below"/"Please correct the errors
below", "Forgot Password?", and the login button's value="Log in"); replace the
`{% block trimmed %}` usage with proper Django i18n tags and wrap all
user-facing strings in `{% trans %}` or `{% blocktrans %}` as appropriate (use
`{% blocktrans %}` for conditional/templated text like the pluralized error
message and `{% trans %}` for simple labels), updating the error block around
`form.errors` and the "Forgot Password?" and login button value to use those
i18n tags so strings are translatable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2de3186b-edc1-424f-8830-478526c1da25
📒 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.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework (django.utils.translation.gettext, gettext_lazy, or ugettext aliases)
Files:
tests/openwisp2/settings.py
🧠 Learnings (1)
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework (django.utils.translation.gettext, gettext_lazy, or ugettext aliases)
Applied to files:
openwisp_users/templates/admin/openwisp_users/login.html
🪛 HTMLHint (1.9.2)
tests/openwisp2/sample_users/templates/admin/login.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
tests/openwisp2/sample_users/templates/admin/action_confirmation.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
openwisp_users/templates/admin/action_confirmation.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
openwisp_users/templates/admin/login.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
openwisp_users/templates/admin/openwisp_users/login.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[warning] 47-47: No matching [ label ] tag found.
(input-requires-label)
openwisp_users/templates/admin/openwisp_users/action_confirmation.html
[error] 7-7: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 7-7: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 7-7: Tag must be paired, no start tag: [ </script> ]
(tag-pair)
[error] 15-15: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 15-15: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 15-15: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 16-16: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 16-16: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 16-16: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 17-17: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 17-17: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 17-17: Tag must be paired, no start tag: [ ]
(tag-pair)
[warning] 40-40: No matching [ label ] tag found.
(input-requires-label)
🔇 Additional comments (6)
tests/openwisp2/settings.py (1)
93-97: Loader precedence update looks correct.Placing
openwisp_utils.loaders.DependencyLoaderafter Django’s default loaders is the right direction for predictable template inheritance during tests.openwisp_users/templates/admin/openwisp_users/action_confirmation.html (1)
1-45: Centralized confirmation template implementation is solid.Template inheritance, i18n tags, and confirmation form wiring are all consistent with the intended admin action flow.
openwisp_users/templates/admin/action_confirmation.html (1)
1-1: Good delegation to the centralized template.This keeps the override surface minimal and avoids template drift.
tests/openwisp2/sample_users/templates/admin/action_confirmation.html (1)
1-1: Test template override strategy is clean.Using inheritance here is the right way to validate extension behavior without duplicating markup.
tests/openwisp2/sample_users/templates/admin/login.html (1)
1-1: Delegation is correct for the sample app login template.This is a good minimal override pattern for test coverage of template extension.
openwisp_users/templates/admin/login.html (1)
1-1: Wrapper template change is good.This keeps
admin/logincustomization centralized in one maintainable location.
|
coverage: 97.894%. remained the same — asmodehn:fix_template_loaders_order_in_tests into openwisp:master |
|
@nemesifier, @pandafy Please have a look at this, and related issues, when you can. This prevents finding the wrong base template when django's app loader look for it from an extension, and this way, we can keep the loader order logical, and consistent on all modules, once similar changes are made in other repos. PS: I ignored all coderabbit comments, since I just moved the file without changes. |
2 participants
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.