Skip to content

[fix] OpenWisp DependencyLoader should be after django template loaders #490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

[fix] OpenWisp DependencyLoader should be after django template loaders #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54e03b9
[fix] OpenWisp DependencyLoader should be after django template loaders
Mar 3, 2026
098ad5c
[change] Add login and confirmation templates in sample_app
Mar 3, 2026
41bb95c
Merge branch 'master' into fix_template_loaders_order_in_tests
asmodehn Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 1 addition & 45 deletions openwisp_users/templates/admin/action_confirmation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,45 +1 @@
{% extends 'admin/base_site.html' %}
{% load i18n l10n admin_urls static %}

{% block extrahead %}
{{ block.super }}
{{ media }}
<script src='{% static 'admin/js/cancel.js' %}'></script>
{% endblock %}

{% block bodyclass %}{{ block.super }} app-{{ opts.app_label }} model-{{ opts.model_name }} delete-confirmation
delete-selected-confirmation{% endblock %}

{% block breadcrumbs %}
<div class='breadcrumbs'>
<a href='{% url 'admin:index' %}'>{% trans 'Home' %}</a>
&rsaquo; <a href='{% url 'admin:app_list' app_label=opts.app_label %}'>{{ opts.app_config.verbose_name }}</a>
&rsaquo; <a href='{% url opts|admin_urlname:'changelist' %}'>{{ opts.verbose_name_plural|capfirst }}</a>
&rsaquo; {% trans 'Modify active status' %}
</div>
{% endblock %}

{% block content %}
{% if action == 'make_inactive' %}
<p>{% blocktrans %}Are you sure you want to make the selected users inactive?{% endblocktrans %}</p>
{% else %}
<p>{% blocktrans %}Are you sure you want to make the selected users active?{% endblocktrans %}</p>
{% endif %}
<h2>{% trans "Summary" %}</h2>
<ul>
{% 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 %}
<div class='submit-row'>
<input type='hidden' name='action' value='{{ action }}'/>
<input type='submit' name='confirmation' value='{% trans "Confirm" %}'/>
<a href='#' onclick='window.history.back(); return false;'
class='button cancel-link'>{% trans 'No, take me back' %}</a>
</div>
</form>
{% endblock %}
{% extends 'admin/openwisp_users/action_confirmation.html' %}
52 changes: 1 addition & 51 deletions openwisp_users/templates/admin/login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,51 +1 @@
{% extends "admin/login.html" %}
{% load i18n %}
{% block content %}
{% 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 %}

{% if form.non_field_errors %}
{% for error in form.non_field_errors %}
<p class="errornote">
{{ error }}
</p>
{% endfor %}
{% endif %}

<div id="content-main">

{% if user.is_authenticated %}
<p class="errornote">
{% 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 %}
</p>
{% endif %}

<form action="{{ app_path }}" method="post" id="login-form">{% csrf_token %}
<div class="form-row">
{{ form.username.errors }}
<label class="required" for="id_username">
{% trans 'Email, phone number or username' %}:
</label>
{{ form.username }}
</div>
<div class="form-row">
{{ form.password.errors }}
{{ form.password.label_tag }} {{ form.password }}
<input type="hidden" name="next" value="{{ next }}" />
</div>
<div class="form-row">
<a href="{% url 'account_reset_password' %}">Forgot Password?</a>
</div>
<div class="submit-row">
<input type="submit" value="Log in" />
</div>
</form>

</div>
{% endblock %}
{% extends 'admin/openwisp_users/login.html' %}
45 changes: 45 additions & 0 deletions openwisp_users/templates/admin/openwisp_users/action_confirmation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{% extends 'admin/base_site.html' %}
{% load i18n l10n admin_urls static %}

{% block extrahead %}
{{ block.super }}
{{ media }}
<script src='{% static 'admin/js/cancel.js' %}'></script>
{% endblock %}

{% block bodyclass %}{{ block.super }} app-{{ opts.app_label }} model-{{ opts.model_name }} delete-confirmation
delete-selected-confirmation{% endblock %}
Comment on lines +10 to +11
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

🧹 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.


{% block breadcrumbs %}
<div class='breadcrumbs'>
<a href='{% url 'admin:index' %}'>{% trans 'Home' %}</a>
&rsaquo; <a href='{% url 'admin:app_list' app_label=opts.app_label %}'>{{ opts.app_config.verbose_name }}</a>
&rsaquo; <a href='{% url opts|admin_urlname:'changelist' %}'>{{ opts.verbose_name_plural|capfirst }}</a>
&rsaquo; {% trans 'Modify active status' %}
</div>
{% endblock %}

{% block content %}
{% if action == 'make_inactive' %}
<p>{% blocktrans %}Are you sure you want to make the selected users inactive?{% endblocktrans %}</p>
{% else %}
<p>{% blocktrans %}Are you sure you want to make the selected users active?{% endblocktrans %}</p>
{% endif %}
<h2>{% trans "Summary" %}</h2>
<ul>
{% 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 %}
Comment on lines +30 to +37
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

🧹 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.

<div class='submit-row'>
<input type='hidden' name='action' value='{{ action }}'/>
<input type='submit' name='confirmation' value='{% trans "Confirm" %}'/>
<a href='#' onclick='window.history.back(); return false;'
class='button cancel-link'>{% trans 'No, take me back' %}</a>
</div>
</form>
{% endblock %}
52 changes: 52 additions & 0 deletions openwisp_users/templates/admin/openwisp_users/login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
{# Extends Django's built-in admin login template #}
{% extends "admin/login.html" %}
{% load i18n %}
{% block content %}
{% 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 %}
Comment on lines +5 to +9
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

find . -path "*/openwisp_users/templates/admin/openwisp_users/login.html" -type f

Repository: 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 -5

Repository: 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 (not with) to enable pluralization, and put your plural branch inside the same block using {% plural %}. Django uses the count value 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 + count in 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. blocktranslate pluralization is internally converted to an ngettext() call, so the ngettext placeholder 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/blocktranslate handle 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 context keyword (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 2

Repository: openwisp/openwisp-users

Length of output: 712

🏁 Script executed:

# Check if blocktrans is used elsewhere in templates
rg "blocktrans" openwisp_users/templates/ -l

Repository: 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.


{% if form.non_field_errors %}
{% for error in form.non_field_errors %}
<p class="errornote">
{{ error }}
</p>
{% endfor %}
{% endif %}

<div id="content-main">

{% if user.is_authenticated %}
<p class="errornote">
{% 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 %}
Comment on lines +23 to +26
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
{% 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.

</p>
{% endif %}

<form action="{{ app_path }}" method="post" id="login-form">{% csrf_token %}
<div class="form-row">
{{ form.username.errors }}
<label class="required" for="id_username">
{% trans 'Email, phone number or username' %}:
</label>
{{ form.username }}
</div>
<div class="form-row">
{{ form.password.errors }}
{{ form.password.label_tag }} {{ form.password }}
<input type="hidden" name="next" value="{{ next }}" />
</div>
<div class="form-row">
<a href="{% url 'account_reset_password' %}">Forgot Password?</a>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

</div>
<div class="submit-row">
<input type="submit" value="Log in" />
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
<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.

</div>
</form>

</div>
{% endblock %}
1 change: 1 addition & 0 deletions tests/openwisp2/sample_users/templates/admin/action_confirmation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% extends 'admin/openwisp_users/action_confirmation.html' %}
1 change: 1 addition & 0 deletions tests/openwisp2/sample_users/templates/admin/login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% extends 'admin/openwisp_users/login.html' %}
5 changes: 1 addition & 4 deletions tests/openwisp2/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,11 @@
TEMPLATES = [
{
"BACKEND": "django.template.backends.django.DjangoTemplates",
"DIRS": [
os.path.join(os.path.dirname(BASE_DIR), "openwisp_users", "templates")
],
"OPTIONS": {
"loaders": [
"django.template.loaders.filesystem.Loader",
"openwisp_utils.loaders.DependencyLoader",
"django.template.loaders.app_directories.Loader",
"openwisp_utils.loaders.DependencyLoader",
],
"context_processors": [
"django.template.context_processors.debug",
Expand Down
Loading