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():
"""Users eligible to supervise a team or project.
Matches the app's role model (see `is_supervisor` in views.py):
anyone who is a Django admin (is_staff/is_superuser) OR is a member
of the "Work Logger" group. Active accounts only no deactivated
users in the picker.
Returns ALL active users. Any active user can be picked as a supervisor
the picker doesn't need to pre-filter by group or staff flags because
the app's `is_supervisor(user)` helper (in views.py) already grants
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)
.filter(Q(is_staff=True) | Q(is_superuser=True) | Q(groups__name='Work Logger'))
.distinct()
.order_by('username')
)
return User.objects.filter(is_active=True).order_by('username')
class TeamForm(forms.ModelForm):
@ -408,12 +413,11 @@ class TeamForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# Supervisor dropdown — show anyone who is either admin (is_staff/
# is_superuser) OR a member of the "Work Logger" group. This matches
# the app's role model: team supervisors are typically Work Loggers,
# not admins, so filtering by is_staff alone hides the people who
# actually supervise teams day-to-day. We use `active=True` to drop
# deactivated accounts from the picker.
# Supervisor dropdown — show any active user. The app's is_supervisor
# helper (views.py) grants supervisor powers to whoever is assigned
# here, regardless of group or staff flags, so the picker doesn't
# need to pre-filter by role. Only is_active=True users appear, so
# deactivated accounts are hidden from the dropdown.
self.fields['supervisor'].queryset = _supervisor_user_queryset()
self.fields['supervisor'].required = False
# 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 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):
@ -312,3 +312,167 @@ class WorkLogPayrollDetailTests(TestCase):
url = reverse('work_log_payroll_detail', args=[self.log.id])
resp = self.client.get(url)
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)
# --- 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)
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:
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 = 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 ---
# 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)
if project_id:
adjustments = adjustments.filter(project_id=project_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_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),
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);
}