From 06b3315641a08da63f2d67e53e9397df80454e77 Mon Sep 17 00:00:00 2001
From: Konrad du Plessis <konradzar@gmail.com>
Date: Thu, 23 Apr 2026 15:45:41 +0200
Subject: [PATCH] fix(adjustments): pagination URL + filter label accessibility
 + teams.first N+1

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).
---
 core/templates/core/_adjustment_row.html   |  8 +++--
 core/templates/core/payroll_dashboard.html | 38 +++++++++++++++-------
 core/templatetags/format_tags.py           | 24 ++++++++++++++
 3 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/core/templates/core/_adjustment_row.html b/core/templates/core/_adjustment_row.html
index d07d830..95606ec 100644
--- a/core/templates/core/_adjustment_row.html
+++ b/core/templates/core/_adjustment_row.html
@@ -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>
 
diff --git a/core/templates/core/payroll_dashboard.html b/core/templates/core/payroll_dashboard.html
index 2166a19..e08422c 100644
--- a/core/templates/core/payroll_dashboard.html
+++ b/core/templates/core/payroll_dashboard.html
@@ -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>
diff --git a/core/templatetags/format_tags.py b/core/templatetags/format_tags.py
index 7723980..b22bf54 100644
--- a/core/templatetags/format_tags.py
+++ b/core/templatetags/format_tags.py
@@ -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()