Compare commits

...

3 Commits

Author SHA1 Message Date
Flatlogic Bot
e0d2c74360 Regenerate staticfiles/css/custom.css after bugfix deploy
Restores the .work-log-row hover rule into the collected CSS.
Replaces the Flatlogic-auto-noise commit (683e2b0) which had the misleading message 'Ver 30.04 Fix reports and add Supervisor' but only contained this same collectstatic output.
2026-04-22 18:13:05 +00:00
Konrad du Plessis
0ceceebba4 Fix: supervisor picker hid regular active users (only admins showed)
Reported: when creating a new team or project from the friendly UI
(/teams/new/ or /projects/new/), the Supervisor dropdown only lists
is_staff / is_superuser accounts. Users who should be eligible to
supervise (e.g. eendman, supervisor_smoke) are invisible in the
picker even though they are active.

Root cause:
core.forms._supervisor_user_queryset filtered to
  is_active=True AND (is_staff OR is_superuser OR groups__name='Work Logger')
That was strictly more restrictive than the app's own permission
helper is_supervisor(user) in views.py, which grants supervisor
powers to ANYONE assigned to a team/project (via the team.supervisor
FK or project.supervisors M2M), regardless of group membership.
On Konrad's dev DB that excluded 2 of 6 active users from the picker
(one in a custom group, one in no group) even though both were valid
supervisor candidates by the permission model.

Fix:
Queryset now returns every active user. The act of assigning a user
to a team/project is what confers supervisor-ness downstream, so
the picker no longer needs a pre-registered allow-list. Inactive
users (is_active=False) remain excluded — the one hard guardrail.

Docstring rewritten to explain the new behavior and why. Stale comment
in TeamForm.__init__ updated to match (the old comment still described
the pre-fix Work-Logger-group requirement).

Tests: 4 new regression tests in SupervisorPickerQuerysetTests:
  - regular active user is selectable (the core bug)
  - user in an unrelated group is selectable
  - inactive user is still excluded (guardrail)
  - admin is still selectable (no regression for prior use case)
All 28 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-22 19:52:29 +02:00
Konrad du Plessis
f1e246ce24 Fix: filtered payroll report inflates worker totals by N^2
Reported: when the generate-report page is filtered by BOTH project and
team, every amount in the "Worker Breakdown" and "Payments by Date"
tables blew up by ~100x. Example: Billy Baloyi R 5,400 (correct)
became R 604,800 (wrong, 112x) after selecting Wilkot + Civils One.

Root cause:
_build_report_context chained `records.filter(work_logs__project_id=X)
.distinct().filter(work_logs__team_id=Y).distinct()`. In Django's ORM
each chained M2M filter creates a SEPARATE JOIN alias on
core_payrollrecord_work_logs, so the SQL produces the cartesian product
of (matching-logs-for-project) x (matching-logs-for-team) rows per
PayrollRecord. A downstream `.values().annotate(Sum('amount_paid'))`
then summed across those duplicated rows - inflating every total by
N * M where N and M are the log counts per record.

Why total_paid_out looked correct: `.aggregate(Sum(...))` wraps the
query in a subquery when distinct() is in play, so it dedupes before
summing. `.values().annotate(Sum(...))` uses GROUP BY on the raw
joined rows and doesn't get that help.

Fix:
Replace chained M2M filters with id__in subquery filters:
  records.filter(id__in=PayrollRecord.objects.filter(
      work_logs__project_id=X).values('id'))
This keeps the outer queryset JOIN-free, so values().annotate(Sum())
aggregates over distinct records. Same pattern applied to the
adjustments team-filter (worker__teams M2M) for the adjustment
summary.

Tests: 5 new regression tests in ReportContextFilterInflationTests
covering project-only, team-only, both-filters, total_paid_out
invariant, and the adjustment summary path. All 24 tests pass
(19 existing + 5 new).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-22 19:51:07 +02:00
4 changed files with 223 additions and 21 deletions

View File

@ -372,18 +372,23 @@ WorkerWarningFormSet = inlineformset_factory(
def _supervisor_user_queryset(): def _supervisor_user_queryset():
"""Users eligible to supervise a team or project. """Users eligible to supervise a team or project.
Matches the app's role model (see `is_supervisor` in views.py): Returns ALL active users. Any active user can be picked as a supervisor
anyone who is a Django admin (is_staff/is_superuser) OR is a member the picker doesn't need to pre-filter by group or staff flags because
of the "Work Logger" group. Active accounts only no deactivated the app's `is_supervisor(user)` helper (in views.py) already grants
users in the picker. supervisor permissions to anyone assigned to a team/project FK/M2M,
regardless of their group membership.
Previously this filter required `is_staff`/`is_superuser` OR membership
in a "Work Logger" group, which was strictly more restrictive than the
permission model and hid field supervisors from the picker. Fix
(2026-04-23): trust the admin making the assignment; any active user
can be chosen, and the act of assignment is what confers supervisor
powers downstream.
Inactive users (`is_active=False`) are still excluded deactivated
accounts should never appear in dropdowns.
""" """
from django.db.models import Q return User.objects.filter(is_active=True).order_by('username')
return (
User.objects.filter(is_active=True)
.filter(Q(is_staff=True) | Q(is_superuser=True) | Q(groups__name='Work Logger'))
.distinct()
.order_by('username')
)
class TeamForm(forms.ModelForm): class TeamForm(forms.ModelForm):
@ -408,12 +413,11 @@ class TeamForm(forms.ModelForm):
def __init__(self, *args, **kwargs): def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs) super().__init__(*args, **kwargs)
# Supervisor dropdown — show anyone who is either admin (is_staff/ # Supervisor dropdown — show any active user. The app's is_supervisor
# is_superuser) OR a member of the "Work Logger" group. This matches # helper (views.py) grants supervisor powers to whoever is assigned
# the app's role model: team supervisors are typically Work Loggers, # here, regardless of group or staff flags, so the picker doesn't
# not admins, so filtering by is_staff alone hides the people who # need to pre-filter by role. Only is_active=True users appear, so
# actually supervise teams day-to-day. We use `active=True` to drop # deactivated accounts are hidden from the dropdown.
# deactivated accounts from the picker.
self.fields['supervisor'].queryset = _supervisor_user_queryset() self.fields['supervisor'].queryset = _supervisor_user_queryset()
self.fields['supervisor'].required = False self.fields['supervisor'].required = False
# Include inactive workers too — matches admin parity. The template # Include inactive workers too — matches admin parity. The template

View File

@ -10,7 +10,7 @@ from django.test import TestCase
from django.urls import reverse from django.urls import reverse
from core.models import Project, Team, Worker, WorkLog, PayrollRecord, PayrollAdjustment from core.models import Project, Team, Worker, WorkLog, PayrollRecord, PayrollAdjustment
from core.views import _build_work_log_payroll_context from core.views import _build_work_log_payroll_context, _build_report_context
class WorkLogPayrollContextTests(TestCase): class WorkLogPayrollContextTests(TestCase):
@ -312,3 +312,167 @@ class WorkLogPayrollDetailTests(TestCase):
url = reverse('work_log_payroll_detail', args=[self.log.id]) url = reverse('work_log_payroll_detail', args=[self.log.id])
resp = self.client.get(url) resp = self.client.get(url)
self.assertEqual(resp.status_code, 403) self.assertEqual(resp.status_code, 403)
# =============================================================================
# === TESTS FOR PAYROLL REPORT FILTER INFLATION ===
# Regression tests for the M2M double-JOIN bug in _build_report_context.
#
# THE BUG (fixed 2026-04-23):
# Filtering a report by project AND team via chained `.filter(work_logs__field=X)`
# calls produced TWO separate JOIN aliases on the core_payrollrecord_work_logs
# M2M table. Any downstream `.values().annotate(Sum(...))` then aggregated
# across the cartesian product of matching rows, inflating every per-worker
# total_paid and every payments_by_date amount by N² (where N = number of
# matching work logs per record). Total_paid_out itself was correct because
# `.aggregate(Sum(...))` wraps distinct() in a subquery, but the per-row
# values/annotate pattern doesn't get that help.
#
# These tests lock down the fix: with filters applied, the worker-level and
# date-level totals must equal the real payment amount, not a multiplied one.
# =============================================================================
class ReportContextFilterInflationTests(TestCase):
"""Report aggregates must not inflate when project + team filters combine."""
def setUp(self):
self.admin = User.objects.create_user(username='admin-r', is_staff=True)
self.project = Project.objects.create(name='Solar Farm Gamma')
self.team = Team.objects.create(name='Team Gamma', supervisor=self.admin)
self.worker = Worker.objects.create(
name='Test Worker', id_number='TW1', monthly_salary=Decimal('4000')
)
# Worker must be in the team's M2M for the adjustment-summary test
# to find them via worker__teams__id=team_id. In real data this is
# the standard setup — workers belong to a team.
self.team.workers.add(self.worker)
# Three work logs in the range, all in the same project + team.
# This is the minimum setup that reproduces the N² inflation: with
# one payroll record linked to 3 logs, the double-JOIN produces 9 rows.
self.logs = []
for day in (5, 10, 15):
log = WorkLog.objects.create(
date=datetime.date(2026, 3, day),
project=self.project,
team=self.team,
supervisor=self.admin,
)
log.workers.add(self.worker)
self.logs.append(log)
# One payment covering all 3 logs.
self.record = PayrollRecord.objects.create(
worker=self.worker,
amount_paid=Decimal('600.00'),
date=datetime.date(2026, 3, 20),
)
self.record.work_logs.add(*self.logs)
def _ctx(self, project_id=None, team_id=None):
return _build_report_context(
datetime.date(2026, 3, 1),
datetime.date(2026, 3, 31),
project_id=project_id,
team_id=team_id,
)
def test_worker_breakdown_not_inflated_with_project_filter_only(self):
ctx = self._ctx(project_id=self.project.id)
self.assertEqual(len(ctx['worker_breakdown']), 1)
# Pre-fix: this was 600 × 3 = 1800 (one JOIN, 3-way inflation).
self.assertEqual(ctx['worker_breakdown'][0]['total_paid'], Decimal('600.00'))
def test_worker_breakdown_not_inflated_with_both_filters(self):
ctx = self._ctx(project_id=self.project.id, team_id=self.team.id)
self.assertEqual(len(ctx['worker_breakdown']), 1)
# Pre-fix: this was 600 × 3 × 3 = 5400 (two JOINs, 9-way inflation).
self.assertEqual(ctx['worker_breakdown'][0]['total_paid'], Decimal('600.00'))
def test_payments_by_date_not_inflated_with_both_filters(self):
ctx = self._ctx(project_id=self.project.id, team_id=self.team.id)
payments = list(ctx['payments_by_date'])
self.assertEqual(len(payments), 1)
self.assertEqual(payments[0]['total'], Decimal('600.00'))
def test_total_paid_out_stays_correct_with_both_filters(self):
"""Regression guard: total_paid_out was ALREADY correct pre-fix
because .aggregate() handles distinct() via a subquery. Lock it in
so a future refactor doesn't accidentally reintroduce inflation here."""
ctx = self._ctx(project_id=self.project.id, team_id=self.team.id)
self.assertEqual(ctx['total_paid_out'], Decimal('600.00'))
def test_adjustment_summary_not_inflated_with_team_filter(self):
"""Adjustments filtered by team go through worker__teams (M2M) — same
bug class. values().annotate(Sum()) would inflate if the worker is in
multiple teams or if the JOIN is chained with other M2M filters."""
PayrollAdjustment.objects.create(
worker=self.worker,
project=self.project,
type='Bonus',
amount=Decimal('100.00'),
date=datetime.date(2026, 3, 10),
description='Test bonus',
)
ctx = self._ctx(project_id=self.project.id, team_id=self.team.id)
totals = {item['type']: item['total'] for item in ctx['adjustment_totals']}
self.assertEqual(totals.get('Bonus'), Decimal('100.00'))
# =============================================================================
# === TESTS FOR SUPERVISOR PICKER QUERYSET ===
# Regression tests for the TeamForm/ProjectForm supervisor dropdown.
#
# THE BUG (fixed 2026-04-23):
# The picker required either admin flags (is_staff/is_superuser) or "Work Logger"
# group membership. Any active user NOT pre-enrolled in that specific group
# was invisible, even though the app's downstream `is_supervisor()` helper
# grants supervisor powers to anyone assigned to a team/project FK/M2M —
# regardless of group. The picker was strictly more restrictive than the
# permission model, hiding field supervisors from the person trying to assign
# them. Fix: allow any active user; let the act of assignment confer
# supervisor-ness (matching how `is_supervisor` works).
# =============================================================================
class SupervisorPickerQuerysetTests(TestCase):
"""Supervisor picker must surface all active users, not just admins."""
def test_regular_active_user_is_selectable(self):
"""A plain active user (no is_staff, no is_superuser, no Work Logger
group) must appear. Pre-fix: excluded by the Q filter."""
from core.forms import _supervisor_user_queryset
regular = User.objects.create_user(
username='field_supervisor', password='pass',
is_staff=False, is_superuser=False,
)
self.assertIn(regular, _supervisor_user_queryset())
def test_user_in_unrelated_group_is_still_selectable(self):
"""A user in some random group (not 'Work Logger') is still an active
user and must show up. Pre-fix: excluded because the Q filter specifically
checked groups__name='Work Logger'."""
from django.contrib.auth.models import Group
from core.forms import _supervisor_user_queryset
user = User.objects.create_user(username='misc_group_user', password='pass')
group, _ = Group.objects.get_or_create(name='Some Other Group')
user.groups.add(group)
self.assertIn(user, _supervisor_user_queryset())
def test_inactive_user_still_excluded(self):
"""Inactive users must NEVER appear in the picker — even if they used
to be admins or Work Loggers. Active-only is the one hard guardrail."""
from core.forms import _supervisor_user_queryset
inactive = User.objects.create_user(
username='former_employee', password='pass',
is_active=False, is_staff=True,
)
self.assertNotIn(inactive, _supervisor_user_queryset())
def test_admin_still_selectable(self):
"""Defense-in-depth: admins continue to appear (no regression for the
existing use case)."""
from core.forms import _supervisor_user_queryset
admin = User.objects.create_user(
username='an_admin', password='pass', is_staff=True
)
self.assertIn(admin, _supervisor_user_queryset())

View File

@ -1878,11 +1878,30 @@ def _build_report_context(start_date, end_date, project_id=None, team_id=None):
date_filter = Q(date__gte=start_date, date__lte=end_date) date_filter = Q(date__gte=start_date, date__lte=end_date)
# --- PayrollRecords in range --- # --- PayrollRecords in range ---
#
# IMPORTANT — avoid M2M double-JOIN inflation:
# Chaining `.filter(work_logs__project_id=X).distinct().filter(work_logs__team_id=Y)`
# creates TWO separate JOIN aliases on core_payrollrecord_work_logs. Any
# later `.values().annotate(Sum())` then aggregates across the cartesian
# product of matching rows, inflating per-worker and per-date totals by
# N × M (where N and M are the counts of matching logs per record).
# `.aggregate(Sum())` is safe because Django wraps it in a subquery when
# distinct() is in play, but `.values().annotate(Sum())` isn't — so we
# use id__in subqueries to keep the outer queryset JOIN-free.
# See ReportContextFilterInflationTests for regression coverage.
records = PayrollRecord.objects.filter(date_filter) records = PayrollRecord.objects.filter(date_filter)
if project_id: if project_id:
records = records.filter(work_logs__project_id=project_id).distinct() records = records.filter(
id__in=PayrollRecord.objects.filter(
work_logs__project_id=project_id
).values('id')
)
if team_id: if team_id:
records = records.filter(work_logs__team_id=team_id).distinct() records = records.filter(
id__in=PayrollRecord.objects.filter(
work_logs__team_id=team_id
).values('id')
)
# --- Total Paid Out (sum of all payments made) --- # --- Total Paid Out (sum of all payments made) ---
total_paid_out = records.aggregate(total=Sum('amount_paid'))['total'] or Decimal('0.00') total_paid_out = records.aggregate(total=Sum('amount_paid'))['total'] or Decimal('0.00')
@ -1895,11 +1914,16 @@ def _build_report_context(start_date, end_date, project_id=None, team_id=None):
) )
# --- Adjustments in range --- # --- Adjustments in range ---
# project_id filters via an FK column (no JOIN inflation risk), but
# team_id goes through worker__teams M2M — apply the same subquery
# pattern as above to keep adj_by_type's values().annotate(Sum()) safe.
adjustments = PayrollAdjustment.objects.filter(date_filter) adjustments = PayrollAdjustment.objects.filter(date_filter)
if project_id: if project_id:
adjustments = adjustments.filter(project_id=project_id) adjustments = adjustments.filter(project_id=project_id)
if team_id: if team_id:
adjustments = adjustments.filter(worker__teams__id=team_id).distinct() adjustments = adjustments.filter(
worker__in=Worker.objects.filter(teams__id=team_id).values('id')
)
# --- Work Logs in range (for calculating actual labour cost) --- # --- Work Logs in range (for calculating actual labour cost) ---
work_logs_qs = WorkLog.objects.filter(date__gte=start_date, date__lte=end_date) work_logs_qs = WorkLog.objects.filter(date__gte=start_date, date__lte=end_date)

View File

@ -1481,3 +1481,13 @@ body, .card, .modal-content, .form-control, .form-select,
border-color var(--transition-normal), border-color var(--transition-normal),
box-shadow var(--transition-normal); box-shadow var(--transition-normal);
} }
/* === Work log payroll: clickable row hover === */
/* Applied only by base.html / templates that add class="work-log-row" */
/* (admin-only; supervisors never get the class so hover doesn't apply). */
.work-log-row {
transition: background-color 120ms ease-in-out;
}
.work-log-row:hover td {
background: var(--bg-card-hover);
}