admin/38686-vm
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(adjustments): pagination URL + filter label accessibility + teams.first N+1

Browse Source 
Three code-review fixes:

1. Pagination links were building ?status=...&page=2&page=3 on every
   click because the template appended &page= onto an already-
   serialised querystring. Added a reusable url_replace template tag
   that replaces a single key (pre-empts Tasks 5 / 9 which also
   need it) and piped the pagination hrefs through it. Added
   rel=prev/next + aria-label on the <a> tags while we were here.

2. Filter-bar labels had no for= attribute, so screen readers
   announced the native <select> with no name. Added id= on each
   select/input and matching for= on each label. Also gave the
   Select-all checkbox an aria-label (title= alone is not an
   accessible name).

3. Row template's {% with team=adj.worker.teams.first %} issued a
   fresh ORDER BY ... LIMIT 1 query per row despite the view's
   prefetch_related('worker__teams'). Swapped to {% with
   teams=adj.worker.teams.all %} which DOES use the prefetch cache,
   bounding the Team column at 0 extra queries (was ~50 per page).
This commit is contained in:
Konrad du Plessis 2026-04-23 15:45:41 +02:00
parent b450bd3c39
commit 06b3315641
3 changed files with 56 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
core/templates/core/_adjustment_row.html
View File

@ -55,9 +55,13 @@
</td>
{# --- Team (worker's first team, if any — many workers are unteamed) --- #}
{# Uses `teams.all` (NOT `teams.first`) because the view's #}
{# .prefetch_related('worker__teams') populates `_prefetched_objects_cache` #}
{# for `.all()` calls — `.first()` would ignore the cache and fire a #}
{# fresh `ORDER BY ... LIMIT 1` SQL query per row (up to ~50 per page). #}
<td>
{% with team=adj.worker.teams.first %}
{% if team %}{{ team.name }}{% else %}<span class="text-muted">&mdash;</span>{% endif %}
{% with teams=adj.worker.teams.all %}
{% if teams %}{{ teams.0.name }}{% else %}<span class="text-muted">&mdash;</span>{% endif %}
{% endwith %}
</td>

38
core/templates/core/payroll_dashboard.html
View File

@ -563,9 +563,13 @@
<input type="hidden" name="status" value="adjustments">
{# --- Type multi-select (Bonus / Overtime / etc.) --- #}
{# Each label/input pair below has matching for=/id= so screen #}
{# readers announce the field name when focus moves into the #}
{# select. The .adj-multi class still drives Choices.js init — #}
{# adding id= is purely additive. #}
<div class="flex-grow-1" style="min-width: 180px;">
<label class="form-label small mb-1">Type</label>
<select name="type" class="form-select form-select-sm adj-multi" multiple
<label for="adjTypeSelect" class="form-label small mb-1">Type</label>
<select id="adjTypeSelect" name="type" class="form-select form-select-sm adj-multi" multiple
data-placeholder="All types">
{% for t in adj_type_choices %}
<option value="{{ t }}" {% if t in adj_filter_values.type %}selected{% endif %}>{{ t }}</option>
@ -575,7 +579,7 @@
{# --- Workers multi-select (cross-filtered by Teams in Task 7) --- #}
<div class="flex-grow-1" style="min-width: 180px;">
<label class="form-label small mb-1">Workers</label>
<label for="adjWorkerSelect" class="form-label small mb-1">Workers</label>
<select name="worker" id="adjWorkerSelect" class="form-select form-select-sm adj-multi" multiple
data-placeholder="All workers">
{% for w in all_workers_for_filter %}
@ -586,7 +590,7 @@
{# --- Teams multi-select --- #}
<div class="flex-grow-1" style="min-width: 160px;">
<label class="form-label small mb-1">Teams</label>
<label for="adjTeamSelect" class="form-label small mb-1">Teams</label>
<select name="team" id="adjTeamSelect" class="form-select form-select-sm adj-multi" multiple
data-placeholder="All teams">
{% for t in all_teams_for_filter %}
@ -597,8 +601,8 @@
{# --- Status single-select (Unpaid / Paid / All) --- #}
<div style="min-width: 120px;">
<label class="form-label small mb-1">Status</label>
<select name="adj_status" class="form-select form-select-sm">
<label for="adjStatusSelect" class="form-label small mb-1">Status</label>
<select id="adjStatusSelect" name="adj_status" class="form-select form-select-sm">
<option value="" {% if not adj_filter_values.adj_status %}selected{% endif %}>All</option>
<option value="unpaid" {% if adj_filter_values.adj_status == 'unpaid' %}selected{% endif %}>Unpaid</option>
<option value="paid" {% if adj_filter_values.adj_status == 'paid' %}selected{% endif %}>Paid</option>
@ -607,13 +611,13 @@
{# --- Date range --- #}
<div style="min-width: 130px;">
<label class="form-label small mb-1">From</label>
<input type="date" name="adj_date_from" class="form-control form-control-sm"
<label for="adjDateFrom" class="form-label small mb-1">From</label>
<input type="date" id="adjDateFrom" name="adj_date_from" class="form-control form-control-sm"
value="{{ adj_filter_values.adj_date_from }}">
</div>
<div style="min-width: 130px;">
<label class="form-label small mb-1">To</label>
<input type="date" name="adj_date_to" class="form-control form-control-sm"
<label for="adjDateTo" class="form-label small mb-1">To</label>
<input type="date" id="adjDateTo" name="adj_date_to" class="form-control form-control-sm"
value="{{ adj_filter_values.adj_date_to }}">
</div>
@ -651,7 +655,10 @@
<thead>
<tr>
<th style="width: 40px;">
{# aria-label is the accessible name screen readers announce; #}
{# title= is kept as the mouse-hover tooltip for sighted users. #}
<input type="checkbox" class="form-check-input" id="adjSelectAll"
aria-label="Select all unpaid adjustments on this page"
title="Select all unpaid on this page">
</th>
<th class="sortable" data-sort="date">Date <i class="fas fa-sort sort-arrow"></i></th>
@ -676,12 +683,18 @@
</div>
{# --- Pagination --- #}
{# Hrefs go through the `url_replace` template tag (see
core/templatetags/format_tags.py). It rebuilds the current querystring
with ONLY the `page` key swapped out — prevents the `?page=2&page=3`
stacking bug that happened when we naively appended `&page=X` to the
raw `request.GET.urlencode`. #}
{% if adj_page.has_other_pages %}
<nav class="mt-3 d-flex justify-content-center">
<ul class="pagination pagination-sm">
{% if adj_page.has_previous %}
<li class="page-item">
<a class="page-link" href="?{{ request.GET.urlencode }}&page={{ adj_page.previous_page_number }}">Previous</a>
<a class="page-link" rel="prev" aria-label="Previous page"
href="?{% url_replace request 'page' adj_page.previous_page_number %}">Previous</a>
</li>
{% endif %}
<li class="page-item active">
@ -689,7 +702,8 @@
</li>
{% if adj_page.has_next %}
<li class="page-item">
<a class="page-link" href="?{{ request.GET.urlencode }}&page={{ adj_page.next_page_number }}">Next</a>
<a class="page-link" rel="next" aria-label="Next page"
href="?{% url_replace request 'page' adj_page.next_page_number %}">Next</a>
</li>
{% endif %}
</ul>

24
core/templatetags/format_tags.py
View File

@ -58,3 +58,27 @@ def dictlookup(d, key):
return d.get(key)
except (AttributeError, TypeError):
return None
# === url_replace tag ===
# Returns the current request's querystring with one key replaced/removed.
# Used by filter toggles and pagination links so they don't accumulate
# repeated keys (e.g. ?page=2&page=3) on each click. Pass value='' (or None)
# to drop the key entirely; pass a real value to set/replace it.
@register.simple_tag
def url_replace(request, key, value):
"""Clone the current GET querystring, set/remove one key, and return it encoded.
Plain-English: Django templates can't easily rebuild a querystring with
one value swapped out. This tag does it — hand it the request, the key
you want to change, and the new value (or empty string to drop it), and
you get back a ready-to-paste `?...` string. Prevents the `?page=2&page=3`
stacking bug that happens if you just append `&page=X` to the raw
`request.GET.urlencode`.
"""
qd = request.GET.copy()
if value == '' or value is None:
qd.pop(key, None)
else:
qd[key] = str(value)
return qd.urlencode()