chore(absences): 7 polish follow-ups from code review
Small cleanups tracked in docs/plans/parked-work.md: 1. Delete dead AbsenceQuickForm class — Round C replaced the per-row ✗ modal paradigm with the "Submit + Log Absences" button, but the form class never got wired up. No view, URL, template, or test ever referenced it. 2. Single-query team_workers_map via shared _build_team_workers_map helper. Previously fired one SELECT per team because .filter( active=True) on a prefetched M2M bypasses the prefetch cache. Now uses Prefetch(to_attr='active_workers_cached'). Both attendance_log() and absence_log() use the same helper. 3. absence_list permission check now uses _user_can_log_absences instead of duplicating the same `is_admin OR supervised_teams` logic inline. 4. Drop misleading var(--badge-neutral-bg, …) wrapper in custom.css — the variable isn't declared so the fallback always wins. Use the hex directly. 5. conflicting_worklogs() N+1 → single query: was firing one SELECT per (worker, date) pair (25 queries on a 5×5 form). Now 2 queries total via .filter(date__in=…, workers__in=…) + Python-side pair set check. 6. Extract _apply_absence_filters helper — absence_list and absence_export_csv were duplicating the same 7-param filter block (with a TODO comment to factor it out). Now structurally enforced in one place; list view keeps the raw param read-back for template-context dropdown preselection. 7. Replace style="color: var(--badge-bonus-bg)" with class="text-success" on the paid-check icon in site_report_detail.html — same WCAG contrast bug we fixed on the absence templates (background colour used as foreground). All 157 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Konrad du Plessis
parent
70fa085886
commit
d1d3e15444
@ -610,12 +610,9 @@ class SiteReportForm(forms.ModelForm):
|
||||
# ====================================================================
|
||||
# === ABSENCE FORMS ==================================================
|
||||
# ====================================================================
|
||||
# Three forms mirror the SiteReport / WorkerWarning patterns:
|
||||
# Two forms mirror the SiteReport / WorkerWarning patterns:
|
||||
# - AbsenceLogForm: standalone /absences/log/ with date-range support,
|
||||
# team filter, worker checkbox list, conflict detection.
|
||||
# - AbsenceQuickForm: minimal form for the "Mark Absent" modal on
|
||||
# /attendance/log/ — worker + date come from URL/POST, form only
|
||||
# asks for reason / paid / notes.
|
||||
# - AbsenceEditForm: edit one existing absence; can correct
|
||||
# worker/date as well as the other fields.
|
||||
#
|
||||
@ -780,53 +777,46 @@ class AbsenceLogForm(forms.ModelForm):
|
||||
Conflicts are warnings, NOT errors — a worker might genuinely have
|
||||
partial-day work + partial-day absence (e.g. sick leave that started
|
||||
mid-shift). The view shows these on a confirm screen so the admin
|
||||
can review before proceeding."""
|
||||
can review before proceeding.
|
||||
|
||||
PERF: single query for all candidate WorkLogs, then Python-side
|
||||
filter by the actual (worker_id, date) pair set. Previously fired
|
||||
one SELECT per (worker, date) pair (N+1 — 25 queries on a typical
|
||||
5-worker × 5-day submission). Now: 2 queries total (WorkLog + its
|
||||
workers prefetch) regardless of pair count.
|
||||
"""
|
||||
pairs = self.expanded_pairs()
|
||||
if not pairs:
|
||||
return []
|
||||
# Build sets used as the outer filter (broad SQL match) AND the
|
||||
# post-filter pair check (narrow Python match). The outer filter
|
||||
# may match WorkLogs that include OTHER workers on those dates,
|
||||
# so we still verify each (worker_id, date) against pair_set.
|
||||
workers = {w for w, _ in pairs}
|
||||
dates = {d for _, d in pairs}
|
||||
pair_set = {(w.id, d) for w, d in pairs}
|
||||
|
||||
wls = (
|
||||
WorkLog.objects
|
||||
.filter(date__in=dates, workers__in=workers)
|
||||
.select_related('project')
|
||||
.prefetch_related('workers')
|
||||
.distinct()
|
||||
)
|
||||
rows = []
|
||||
for worker, d in self.expanded_pairs():
|
||||
for wl in WorkLog.objects.filter(date=d, workers=worker).select_related('project'):
|
||||
rows.append({
|
||||
'worker_id': worker.id,
|
||||
'worker_name': worker.name,
|
||||
'date': d,
|
||||
'work_log_id': wl.id,
|
||||
'project_name': wl.project.name if wl.project else '—',
|
||||
})
|
||||
for wl in wls:
|
||||
for w in wl.workers.all():
|
||||
if (w.id, wl.date) in pair_set:
|
||||
rows.append({
|
||||
'worker_id': w.id,
|
||||
'worker_name': w.name,
|
||||
'date': wl.date,
|
||||
'work_log_id': wl.id,
|
||||
'project_name': wl.project.name if wl.project else '—',
|
||||
})
|
||||
return rows
|
||||
|
||||
|
||||
class AbsenceQuickForm(forms.ModelForm):
|
||||
"""Minimal form for the ✗ Mark Absent modal on /attendance/log/.
|
||||
|
||||
Worker and date come from URL/POST context (the row the admin clicked
|
||||
on), so this form only asks for the three fields still missing:
|
||||
reason / is_paid / notes."""
|
||||
|
||||
class Meta:
|
||||
model = Absence
|
||||
# `project` is optional — the modal may be opened from a worker row
|
||||
# that already has a current project context (e.g. quick-mark from
|
||||
# /attendance/log/), in which case the view can pre-fill it.
|
||||
fields = ['project', 'reason', 'is_paid', 'notes']
|
||||
widgets = {
|
||||
'project': forms.Select(attrs={'class': 'form-select'}),
|
||||
'reason': forms.Select(attrs={'class': 'form-select'}),
|
||||
'is_paid': forms.CheckboxInput(attrs={'class': 'form-check-input'}),
|
||||
'notes': forms.Textarea(attrs={'rows': 2, 'class': 'form-control'}),
|
||||
}
|
||||
|
||||
def __init__(self, *args, user=None, **kwargs):
|
||||
super().__init__(*args, **kwargs)
|
||||
self.fields['project'].required = False
|
||||
# Supervisor scope: project dropdown only shows their assigned projects.
|
||||
# Admin / staff sees every active project.
|
||||
if user is not None and not (user.is_staff or user.is_superuser):
|
||||
self.fields['project'].queryset = Project.objects.filter(
|
||||
active=True, supervisors=user,
|
||||
)
|
||||
else:
|
||||
self.fields['project'].queryset = Project.objects.filter(active=True)
|
||||
|
||||
|
||||
class AbsenceEditForm(forms.ModelForm):
|
||||
"""Edit one existing Absence. Lets admin correct worker/date as well
|
||||
as the other fields (in case the absence was logged against the wrong
|
||||
|
||||
@ -116,7 +116,7 @@ might want to scan multiple at once.
|
||||
{% for c in checks_display %}
|
||||
<li class="mb-1">
|
||||
{% if c.value %}
|
||||
<i class="fas fa-check-circle me-1" style="color: var(--badge-bonus-bg);"></i>
|
||||
<i class="fas fa-check-circle me-1 text-success"></i>
|
||||
{% else %}
|
||||
<i class="far fa-circle me-1 text-muted"></i>
|
||||
{% endif %}
|
||||
|
||||
@ -1900,9 +1900,8 @@ class AbsenceUserQuerysetTests(TestCase):
|
||||
|
||||
# =============================================================================
|
||||
# === ABSENCE FORM TESTS (Task 3) ===
|
||||
# Tests for the three form classes added in core/forms.py:
|
||||
# Tests for the form classes added in core/forms.py:
|
||||
# - AbsenceLogForm: standalone /absences/log/ with date-range + multi-worker
|
||||
# - AbsenceQuickForm: minimal modal form on /attendance/log/
|
||||
# - AbsenceEditForm: edit one existing absence
|
||||
# =============================================================================
|
||||
|
||||
|
||||
210
core/views.py
210
core/views.py
@ -495,6 +495,35 @@ def index(request):
|
||||
return render(request, 'core/index.html', context)
|
||||
|
||||
|
||||
# === TEAM → ACTIVE WORKERS MAP HELPER ===
|
||||
# Used by both attendance_log() and absence_log() to feed the team-filter
|
||||
# JavaScript ({team_id: [active_worker_id, ...]}) — picking a team in the
|
||||
# dropdown auto-checks (or filters to) that team's active workers.
|
||||
#
|
||||
# PERF: Prefetch with `to_attr=` is critical. The naive version did
|
||||
# `prefetch_related('workers')` then `.filter(active=True)` inside the
|
||||
# loop — but `.filter()` on a prefetched M2M bypasses the prefetch cache
|
||||
# and re-queries the DB, giving an N+1 (one SELECT per team). The
|
||||
# `Prefetch(..., queryset=Worker.objects.filter(active=True),
|
||||
# to_attr='active_workers_cached')` pattern moves the active-filter into
|
||||
# the prefetch query itself — one SELECT total for all teams' workers.
|
||||
def _build_team_workers_map(user):
|
||||
"""Return {team_id: [active_worker_id, ...]} for the team-filter JS.
|
||||
|
||||
Single query via Prefetch(to_attr=…). Admins see every active team;
|
||||
supervisors see only the teams they themselves supervise."""
|
||||
teams_qs = Team.objects.filter(active=True).prefetch_related(
|
||||
Prefetch(
|
||||
'workers',
|
||||
queryset=Worker.objects.filter(active=True),
|
||||
to_attr='active_workers_cached',
|
||||
)
|
||||
)
|
||||
if not is_admin(user):
|
||||
teams_qs = teams_qs.filter(supervisor=user)
|
||||
return {t.id: [w.id for w in t.active_workers_cached] for t in teams_qs}
|
||||
|
||||
|
||||
# === ATTENDANCE LOGGING ===
|
||||
# This is where supervisors log which workers showed up to work each day.
|
||||
# Supports logging a single day or a date range (e.g. a whole week).
|
||||
@ -693,17 +722,9 @@ def attendance_log(request):
|
||||
worker_rates[str(w.id)] = str(w.daily_rate)
|
||||
|
||||
# Build team→workers mapping so the JS can auto-check workers when a
|
||||
# team is selected from the dropdown. Key = team ID, Value = list of worker IDs.
|
||||
team_workers_map = {}
|
||||
teams_qs = Team.objects.filter(active=True).prefetch_related('workers')
|
||||
if not is_admin(user):
|
||||
# Supervisors only see their own teams
|
||||
teams_qs = teams_qs.filter(supervisor=user)
|
||||
for team in teams_qs:
|
||||
active_worker_ids = list(
|
||||
team.workers.filter(active=True).values_list('id', flat=True)
|
||||
)
|
||||
team_workers_map[team.id] = active_worker_ids
|
||||
# team is selected from the dropdown. Key = team ID, Value = list of
|
||||
# active worker IDs. Single query via the shared helper.
|
||||
team_workers_map = _build_team_workers_map(user)
|
||||
|
||||
return render(request, 'core/attendance_log.html', {
|
||||
'form': form,
|
||||
@ -5418,18 +5439,10 @@ def absence_log(request):
|
||||
|
||||
# === TEAM → WORKERS MAP for the in-page team filter (Fix A1, May 2026) ===
|
||||
# Mirrors the pattern in attendance_log(): build a dict of team_id →
|
||||
# [worker_ids] and pass it as JSON so the template's JS can hide
|
||||
# [active_worker_ids] and pass it as JSON so the template's JS can hide
|
||||
# worker rows whose worker_id is not in the selected team's list.
|
||||
# Supervisors only see their own teams; admins see every active team.
|
||||
team_workers_map = {}
|
||||
teams_qs = Team.objects.filter(active=True).prefetch_related('workers')
|
||||
if not is_admin(request.user):
|
||||
teams_qs = teams_qs.filter(supervisor=request.user)
|
||||
for team in teams_qs:
|
||||
active_worker_ids = list(
|
||||
team.workers.filter(active=True).values_list('id', flat=True)
|
||||
)
|
||||
team_workers_map[team.id] = active_worker_ids
|
||||
# Single query via the shared `_build_team_workers_map` helper.
|
||||
team_workers_map = _build_team_workers_map(request.user)
|
||||
|
||||
return render(request, 'core/absences/log.html', {
|
||||
'form': form,
|
||||
@ -5580,6 +5593,57 @@ def _create_absences_atomic(pairs, reason, is_paid, notes, user, worklog_removal
|
||||
_sync_absence_payroll_adjustment(a)
|
||||
|
||||
|
||||
def _apply_absence_filters(qs, request):
|
||||
"""Apply the standard URL-param filters to an Absence queryset.
|
||||
|
||||
URL params: worker, team, project, reason, date_from, date_to, paid.
|
||||
Each filter is best-effort — bad input (non-numeric IDs, malformed
|
||||
date strings, unknown reason keys) is silently ignored rather than
|
||||
500-ing.
|
||||
|
||||
Used by both absence_list and absence_export_csv so filter parity is
|
||||
structurally enforced — adding a new param here updates both views
|
||||
in one place."""
|
||||
worker_id = request.GET.get('worker')
|
||||
team_id = request.GET.get('team')
|
||||
project_id = request.GET.get('project')
|
||||
reasons = request.GET.getlist('reason')
|
||||
date_from = request.GET.get('date_from')
|
||||
date_to = request.GET.get('date_to')
|
||||
paid = request.GET.get('paid')
|
||||
|
||||
if worker_id and worker_id.isdigit():
|
||||
qs = qs.filter(worker_id=worker_id)
|
||||
if team_id and team_id.isdigit():
|
||||
qs = qs.filter(worker__teams__id=team_id).distinct()
|
||||
if project_id and project_id.isdigit():
|
||||
# Direct FK filter (since 0015_absence_project) — matches
|
||||
# absences explicitly linked to this project.
|
||||
qs = qs.filter(project_id=project_id)
|
||||
# Whitelist reason keys against REASON_CHOICES so an attacker can't
|
||||
# sneak an arbitrary string into the SQL filter.
|
||||
valid_reason_keys = dict(Absence.REASON_CHOICES)
|
||||
reasons = [r for r in reasons if r in valid_reason_keys]
|
||||
if reasons:
|
||||
qs = qs.filter(reason__in=reasons)
|
||||
# parse_date() returns None for malformed input — filter is skipped.
|
||||
# Without this guard, Django's date coercion raises ValidationError
|
||||
# and the request 500s on URL fuzzing.
|
||||
if date_from:
|
||||
parsed = parse_date(date_from)
|
||||
if parsed:
|
||||
qs = qs.filter(date__gte=parsed)
|
||||
if date_to:
|
||||
parsed = parse_date(date_to)
|
||||
if parsed:
|
||||
qs = qs.filter(date__lte=parsed)
|
||||
if paid == 'paid':
|
||||
qs = qs.filter(is_paid=True)
|
||||
elif paid == 'unpaid':
|
||||
qs = qs.filter(is_paid=False)
|
||||
return qs
|
||||
|
||||
|
||||
@login_required
|
||||
def absence_list(request):
|
||||
"""Filtered list of absences with pagination + reason badges.
|
||||
@ -5597,8 +5661,9 @@ def absence_list(request):
|
||||
# === ACCESS GATE ===
|
||||
# Admins always pass. Supervisors pass if they supervise at least
|
||||
# one team. Everyone else gets a 403 instead of an empty list, so
|
||||
# it's obvious the page wasn't meant for them.
|
||||
if not (is_admin(user) or user.supervised_teams.exists()):
|
||||
# it's obvious the page wasn't meant for them. DRY: same gate logic
|
||||
# as /absences/log/ — use the shared helper.
|
||||
if not _user_can_log_absences(user):
|
||||
return HttpResponseForbidden('Permission denied.')
|
||||
|
||||
# Base queryset — scoped to what this user is allowed to see, with
|
||||
@ -5610,17 +5675,20 @@ def absence_list(request):
|
||||
)
|
||||
|
||||
# === FILTERS ===
|
||||
# Each filter is best-effort: bad input (non-numeric IDs, bad
|
||||
# date strings, unknown reason keys) is silently ignored rather
|
||||
# than 500-ing. Empty values are skipped.
|
||||
# All filter logic lives in the shared `_apply_absence_filters` helper
|
||||
# (used here AND by absence_export_csv to guarantee filter parity).
|
||||
# We still read the raw param values out here so the template context
|
||||
# below can preselect the matching dropdown/checkbox options.
|
||||
qs = _apply_absence_filters(qs, request)
|
||||
|
||||
# Raw param read-back for template preselection (NOT for filtering —
|
||||
# the helper handles that). Multi-value reason filter (Fix A2, May 2026):
|
||||
# template renders reasons as a checkbox list sharing name="reason",
|
||||
# so the querystring carries ?reason=sick&reason=annual on multi-select.
|
||||
# getlist() pulls them ALL; whitelist against REASON_CHOICES.
|
||||
worker_id = request.GET.get('worker')
|
||||
team_id = request.GET.get('team')
|
||||
project_id = request.GET.get('project')
|
||||
# Multi-value reason filter (Fix A2, May 2026): the template renders
|
||||
# the reasons as a checkbox list, all sharing name="reason", so the
|
||||
# querystring carries ?reason=sick&reason=annual on multi-select.
|
||||
# getlist() pulls them ALL; we then whitelist against REASON_CHOICES
|
||||
# so an attacker can't sneak an arbitrary string into the SQL filter.
|
||||
reasons = request.GET.getlist('reason')
|
||||
valid_reason_keys = dict(Absence.REASON_CHOICES)
|
||||
reasons = [r for r in reasons if r in valid_reason_keys]
|
||||
@ -5628,39 +5696,6 @@ def absence_list(request):
|
||||
date_to = request.GET.get('date_to')
|
||||
paid = request.GET.get('paid')
|
||||
|
||||
if worker_id and worker_id.isdigit():
|
||||
qs = qs.filter(worker_id=worker_id)
|
||||
if team_id and team_id.isdigit():
|
||||
qs = qs.filter(worker__teams__id=team_id).distinct()
|
||||
if project_id and project_id.isdigit():
|
||||
# Direct FK filter — was previously a transitive join via
|
||||
# worker__work_logs__project_id, which was a workaround for not
|
||||
# having Absence.project. Now that the FK exists, filter on it
|
||||
# directly: matches absences explicitly linked to this project.
|
||||
# (An absence whose worker happens to have worked on the project
|
||||
# before but with a NULL absence.project will no longer appear —
|
||||
# which is the correct behaviour: filter by what the absence
|
||||
# says, not by adjacent activity.)
|
||||
qs = qs.filter(project_id=project_id)
|
||||
if reasons:
|
||||
qs = qs.filter(reason__in=reasons)
|
||||
# parse_date() returns None for malformed input (e.g. "not-a-date")
|
||||
# so the filter is simply skipped. Without this guard, Django's
|
||||
# date coercion raises ValidationError (NOT ValueError/TypeError)
|
||||
# and the request 500s — a tiny URL-fuzzing footgun.
|
||||
if date_from:
|
||||
parsed = parse_date(date_from)
|
||||
if parsed:
|
||||
qs = qs.filter(date__gte=parsed)
|
||||
if date_to:
|
||||
parsed = parse_date(date_to)
|
||||
if parsed:
|
||||
qs = qs.filter(date__lte=parsed)
|
||||
if paid == 'paid':
|
||||
qs = qs.filter(is_paid=True)
|
||||
elif paid == 'unpaid':
|
||||
qs = qs.filter(is_paid=False)
|
||||
|
||||
# === PAGINATION ===
|
||||
# 25 per page — keeps the table snappy even with years of history.
|
||||
paginator = Paginator(qs, 25)
|
||||
@ -5802,48 +5837,9 @@ def absence_export_csv(request):
|
||||
|
||||
qs = _absence_user_queryset(request.user).select_related('worker', 'logged_by', 'project')
|
||||
|
||||
# ===========================================================
|
||||
# FILTER BLOCK — DUPLICATED from absence_list above.
|
||||
# Kept verbatim (same params, same order) so the CSV export
|
||||
# honours the list page's filter URL exactly. If a future
|
||||
# change adds a filter to the list view, mirror it here too.
|
||||
# Follow-up TODO: factor into a `_apply_absence_filters(qs, request)`
|
||||
# helper so the two views can't drift apart.
|
||||
# ===========================================================
|
||||
worker_id = request.GET.get('worker')
|
||||
team_id = request.GET.get('team')
|
||||
project_id = request.GET.get('project')
|
||||
# Multi-value reason filter — kept in parity with absence_list (Fix A2).
|
||||
reasons = request.GET.getlist('reason')
|
||||
valid_reason_keys = dict(Absence.REASON_CHOICES)
|
||||
reasons = [r for r in reasons if r in valid_reason_keys]
|
||||
date_from = request.GET.get('date_from')
|
||||
date_to = request.GET.get('date_to')
|
||||
paid = request.GET.get('paid')
|
||||
|
||||
if worker_id and worker_id.isdigit():
|
||||
qs = qs.filter(worker_id=worker_id)
|
||||
if team_id and team_id.isdigit():
|
||||
qs = qs.filter(worker__teams__id=team_id).distinct()
|
||||
if project_id and project_id.isdigit():
|
||||
# Direct FK filter — was previously worker__work_logs__project_id
|
||||
# (a workaround for not having Absence.project). Now uses the
|
||||
# direct FK. Mirrors absence_list above for filter parity.
|
||||
qs = qs.filter(project_id=project_id)
|
||||
if reasons:
|
||||
qs = qs.filter(reason__in=reasons)
|
||||
if date_from:
|
||||
parsed = parse_date(date_from)
|
||||
if parsed:
|
||||
qs = qs.filter(date__gte=parsed)
|
||||
if date_to:
|
||||
parsed = parse_date(date_to)
|
||||
if parsed:
|
||||
qs = qs.filter(date__lte=parsed)
|
||||
if paid == 'paid':
|
||||
qs = qs.filter(is_paid=True)
|
||||
elif paid == 'unpaid':
|
||||
qs = qs.filter(is_paid=False)
|
||||
# Filter parity with absence_list — both views call the same helper
|
||||
# so adding a new filter param updates both in one place.
|
||||
qs = _apply_absence_filters(qs, request)
|
||||
|
||||
resp = HttpResponse(content_type='text/csv')
|
||||
resp['Content-Disposition'] = 'attachment; filename="absences.csv"'
|
||||
|
||||
@ -2195,8 +2195,10 @@ th.sortable.sorted .sort-arrow { opacity: 1; }
|
||||
.badge-absence-sick { background: var(--badge-bonus-bg); color: var(--badge-bonus-fg); }
|
||||
.badge-absence-family { background: var(--badge-bonus-bg); color: var(--badge-bonus-fg); }
|
||||
.badge-absence-annual { background: var(--badge-bonus-bg); color: var(--badge-bonus-fg); }
|
||||
.badge-absence-unpaid { background: var(--badge-neutral-bg, #6c757d); color: #fff; }
|
||||
/* unpaid/other use a neutral grey directly — no theme variation needed, */
|
||||
/* #6c757d has enough contrast on both dark and light backgrounds. */
|
||||
.badge-absence-unpaid { background: #6c757d; color: #fff; }
|
||||
.badge-absence-iod { background: var(--badge-overtime-bg, #ffc107); color: var(--badge-overtime-fg, #000); }
|
||||
.badge-absence-suspension { background: var(--badge-deduction-bg); color: var(--badge-deduction-fg); }
|
||||
.badge-absence-absconded { background: var(--badge-deduction-bg); color: var(--badge-deduction-fg); }
|
||||
.badge-absence-other { background: var(--badge-neutral-bg, #6c757d); color: #fff; }
|
||||
.badge-absence-other { background: #6c757d; color: #fff; }
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user