From b0aa35661bca7b45bef8e76f93aada1fbb3afdea Mon Sep 17 00:00:00 2001
From: Konrad du Plessis <konradzar@gmail.com>
Date: Wed, 22 Apr 2026 14:21:34 +0200
Subject: [PATCH] Fix overtime_needs_pricing flag + add regression tests

The helper used log.overtime (which doesn't exist on WorkLog); the
correct field is overtime_amount. Combined with a defensive
`getattr(..., None) or 0`, the bug made the flag permanently False,
which would have silently hidden the 'Price now' banner in Tasks 3
and 4. Now reads overtime_amount directly (it's non-nullable with a
0.00 default, so no defensive shim is needed).

Adds 4 regression tests:
- test_overtime_needs_pricing_flag: the bug that just got fixed
- test_query_count_is_bounded: N+1 guard (4 queries regardless of worker count)
- test_empty_log_returns_zero_totals: log with no workers attached
- test_log_without_team_has_no_pay_period: log whose team became NULL

Also removes unused `reverse` import from tests.py.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 core/tests.py | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 core/views.py |  4 +++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/core/tests.py b/core/tests.py
index 9d777be..e3429ae 100644
--- a/core/tests.py
+++ b/core/tests.py
@@ -7,7 +7,6 @@ from decimal import Decimal
 
 from django.contrib.auth.models import User
 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
@@ -109,3 +108,51 @@ class WorkLogPayrollContextTests(TestCase):
         self.assertIsNotNone(end)
         self.assertLessEqual(start, self.log.date)
         self.assertGreaterEqual(end, self.log.date)
+
+    def test_overtime_needs_pricing_flag(self):
+        """Flag fires when log has OT hours but no priced_workers yet."""
+        # Start: no OT, no priced workers -> flag False
+        self.assertFalse(_build_work_log_payroll_context(self.log)['overtime_needs_pricing'])
+
+        # Add OT hours but keep priced_workers empty -> flag True
+        self.log.overtime_amount = Decimal('0.50')
+        self.log.priced_workers.clear()
+        self.log.save()
+        self.assertTrue(_build_work_log_payroll_context(self.log)['overtime_needs_pricing'])
+
+        # Price the OT -> flag False again
+        self.log.priced_workers.add(self.worker_a)
+        self.assertFalse(_build_work_log_payroll_context(self.log)['overtime_needs_pricing'])
+
+    def test_query_count_is_bounded(self):
+        """Helper should not issue per-worker queries — guards against N+1."""
+        # 4 queries: payroll_records, priced_workers, workers.all, adjustments
+        # (plus whichever assertNumQueries overhead Django's test client adds).
+        # We assert a tight upper bound; regressions that add per-worker queries
+        # will push this well above the bound and fail the test.
+        with self.assertNumQueries(4):
+            _build_work_log_payroll_context(self.log)
+
+    def test_empty_log_returns_zero_totals(self):
+        """Log with no workers: helper returns empty rows and zero totals."""
+        empty_log = WorkLog.objects.create(
+            date=datetime.date(2026, 4, 11),
+            project=self.project,
+            team=self.team,
+            supervisor=self.admin,
+        )
+        ctx = _build_work_log_payroll_context(empty_log)
+        self.assertEqual(ctx['worker_rows'], [])
+        self.assertEqual(ctx['total_earned'], Decimal('0.00'))
+        self.assertEqual(ctx['total_paid'], Decimal('0.00'))
+        self.assertEqual(ctx['total_outstanding'], Decimal('0.00'))
+        self.assertEqual(ctx['adjustments'], [])
+
+    def test_log_without_team_has_no_pay_period(self):
+        """Log whose team was later soft-deleted to NULL still works."""
+        self.log.team = None
+        self.log.save()
+        ctx = _build_work_log_payroll_context(self.log)
+        self.assertEqual(ctx['pay_period'], (None, None))
+        # The rest of the context should still populate correctly.
+        self.assertEqual(len(ctx['worker_rows']), 3)
diff --git a/core/views.py b/core/views.py
index 49ba980..dd0e8ab 100644
--- a/core/views.py
+++ b/core/views.py
@@ -796,7 +796,9 @@ def _build_work_log_payroll_context(log):
     pay_period = get_pay_period(log.team, reference_date=log.date) if log.team else (None, None)
 
     # Overtime "needs pricing" flag: log has OT hours but no priced_workers yet.
-    log_overtime = getattr(log, 'overtime', None) or 0
+    # log.overtime_amount is a Decimal with default=0.00 — always present on saved
+    # instances, so no defensive getattr needed. Compare via Decimal arithmetic.
+    log_overtime = log.overtime_amount or Decimal('0.00')
     overtime_needs_pricing = log_overtime > 0 and not priced_worker_ids
 
     return {