From 171742907488e854a1a28a3e3930fa52a584f993 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 19:15:55 +1200 Subject: [PATCH 1/6] fix: leave reconciliation no longer needs the impossible pay-run delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prod (KAN-326): posting payroll crashed with the xero-python error "Invalid value for pay_run_status (Deleted)" every run once a week's leave stopped matching. Three defects, all fixed here: - The differ keyed leave on a uniform hours-per-day, so mixed-hours leave (e.g. 4.5/8/8/8) could never match and was deleted-and-recreated every run. Verified live: Xero only preserves span + total units (one lumped period per pay week), so the key is now (type, span, total) and the builder emits one request per contiguous run of days, summing same-day entries. - delete_same_week_draft_pay_run built PayRun(pay_run_status="Deleted"), which the SDK rejects client-side — and the NZ Payroll API has no pay-run delete/update endpoint at all (GET-only; UI-only deletion), so the self-heal could never work. Removed it and its helpers outright. - When a stale leave overlaps a desired request of the same type it is now updated in place — verified live that Xero permits leave updates during a draft pay run, unlike deletes. Only counterpart-less leave is deleted; if Xero blocks that, DraftPayRunBlocksLeaveChange tells the operator which draft pay run to remove in the Xero UI, with the staff member's name attached by the orchestrator. create_employee_leave now takes the verified payload shape (single pay-week period with total units — per-day units are silently discarded by Xero). Tests rebuilt around real xero_python models so client-side SDK validation is actually exercised; the old suite mocked out the very function that crashed. scripts/verify_kan326_leave_contracts.py captured the live API contracts and stays until the demo tenant is cleaned up. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- apps/workflow/api/xero/__init__.py | 10 +- apps/workflow/api/xero/payroll.py | 887 ++++++++---------- .../workflow/tests/test_xero_payroll_leave.py | 388 +++++--- mypy-baseline.txt | 9 - scripts/verify_kan326_leave_contracts.py | 395 ++++++++ 5 files changed, 1096 insertions(+), 593 deletions(-) create mode 100644 scripts/verify_kan326_leave_contracts.py diff --git a/apps/workflow/api/xero/__init__.py b/apps/workflow/api/xero/__init__.py index b2f17c197..337ffbfdf 100644 --- a/apps/workflow/api/xero/__init__.py +++ b/apps/workflow/api/xero/__init__.py @@ -24,12 +24,12 @@ ) from .client import RateLimitedRESTClient, quota_floor_breached from .payroll import ( - DraftPayRunBlocksLeaveDeletion, + DraftPayRunBlocksLeaveChange, + LeaveRequestSpec, coerce_xero_date, create_employee_leave, create_pay_run, create_payroll_employee, - delete_same_week_draft_pay_run, ensure_earnings_rate_cache, ensure_leave_type_cache, ensure_pay_run_for_week, @@ -54,6 +54,7 @@ reconcile_leave_for_staff_week, reconcile_leave_for_week_for_staff, sync_xero_pay_items, + update_employee_leave_in_place, update_employee_name, validate_pay_items_for_week, ) @@ -144,7 +145,8 @@ pass __all__ = [ - "DraftPayRunBlocksLeaveDeletion", + "DraftPayRunBlocksLeaveChange", + "LeaveRequestSpec", "NoActiveXeroApp", "RateLimitedRESTClient", "XeroSyncEvent", @@ -162,7 +164,6 @@ "create_project", "create_time_entries", "deep_sync_xero_data", - "delete_same_week_draft_pay_run", "ensure_earnings_rate_cache", "ensure_leave_type_cache", "ensure_pay_run_for_week", @@ -250,6 +251,7 @@ "transform_purchase_order", "transform_quote", "transform_stock", + "update_employee_leave_in_place", "update_employee_name", "update_expense_entries", "update_project", diff --git a/apps/workflow/api/xero/payroll.py b/apps/workflow/api/xero/payroll.py index 0c6b3afbf..e4e7eaca4 100644 --- a/apps/workflow/api/xero/payroll.py +++ b/apps/workflow/api/xero/payroll.py @@ -3,9 +3,10 @@ import logging import time from collections import defaultdict +from collections.abc import Sequence from datetime import date, datetime, timedelta from decimal import Decimal -from typing import Any, Dict, List, Optional, Tuple +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, TypedDict from uuid import UUID from xero_python.payrollnz import PayrollNzApi @@ -13,11 +14,13 @@ Address, BankAccount, Employee, + EmployeeLeave, EmployeeLeaveSetup, EmployeeTax, EmployeeWorkingPattern, EmployeeWorkingPatternWithWorkingWeeksRequest, Employment, + LeavePeriod, PaymentMethod, PayRun, SalaryAndWage, @@ -27,6 +30,9 @@ WorkingWeek, ) +if TYPE_CHECKING: + from apps.job.models.costing import CostLine + from apps.workflow.api.xero.auth import api_client, get_tenant_id from apps.workflow.api.xero.transforms import transform_pay_run from apps.workflow.models import CompanyDefaults, XeroPayItem, XeroPayRun @@ -39,8 +45,13 @@ SLEEP_TIME = 3 -class DraftPayRunBlocksLeaveDeletion(ValueError): - """Xero refuses leave deletion while the employee is in a draft pay run.""" +class DraftPayRunBlocksLeaveChange(ValueError): + """Xero refuses a leave change while the employee is in a draft pay run. + + Draft pay runs cannot be deleted through the Xero NZ Payroll API + (/PayRuns/{PayRunID} is GET-only); only a person can remove them, in the + Xero UI. This error carries those instructions to the operator. + """ # Monkeypatch for Xero Python NZ Payroll API (dev-only, does not affect PROD) @@ -853,119 +864,6 @@ def get_pay_run(pay_run_id: str): raise -def _pay_run_payload_from_object(pay_run: Any, *, status: str) -> PayRun: - return PayRun( - pay_run_id=str(pay_run.pay_run_id), - payroll_calendar_id=str(pay_run.payroll_calendar_id), - period_start_date=coerce_xero_date(pay_run.period_start_date), - period_end_date=coerce_xero_date(pay_run.period_end_date), - payment_date=coerce_xero_date(pay_run.payment_date), - pay_run_status=status, - pay_run_type=getattr(pay_run, "pay_run_type", None), - ) - - -def _update_pay_run(pay_run_id: str, pay_run: PayRun) -> Any: - tenant_id = get_tenant_id() - if not tenant_id: - raise ValueError("No Xero tenant ID configured") - - payroll_api = PayrollNzApi(api_client) - path_params = {"PayRunID": pay_run_id} - header_params = { - "Xero-Tenant-Id": tenant_id, - "Accept": api_client.select_header_accept(["application/json"]), - "Content-Type": api_client.select_header_content_type(["application/json"]), - } - try: - return api_client.call_api( - payroll_api.get_resource_url("/PayRuns/{PayRunID}"), - "PUT", - path_params, - [], - header_params, - body=pay_run, - post_params=[], - files={}, - response_type="PayRunObject", - response_model_finder=payroll_api.get_model_finder(), - auth_settings=["OAuth2"], - _return_http_data_only=True, - _preload_content=True, - _request_timeout=None, - collection_formats={}, - ) - except Exception as exc: - logger.error("Failed to update Xero pay run %s: %s", pay_run_id, exc) - persist_app_error( - exc, - additional_context={ - "operation": "update_pay_run", - "pay_run_id": pay_run_id, - "pay_run_status": pay_run.pay_run_status, - }, - ) - raise - - -def _find_same_week_draft_pay_run(week_start_date: date) -> Any | None: - tenant_id = get_tenant_id() - if not tenant_id: - raise ValueError("No Xero tenant ID configured") - - week_end_date = week_start_date + timedelta(days=6) - local_draft = XeroPayRun.objects.filter( - period_start_date=week_start_date, - period_end_date=week_end_date, - pay_run_status="Draft", - ).first() - if local_draft is not None: - return PayRun( - pay_run_id=str(local_draft.xero_id), - payroll_calendar_id=str(local_draft.payroll_calendar_id), - period_start_date=local_draft.period_start_date, - period_end_date=local_draft.period_end_date, - payment_date=local_draft.payment_date, - pay_run_status=local_draft.pay_run_status, - pay_run_type=local_draft.pay_run_type, - ) - - payroll_api = PayrollNzApi(api_client) - response = payroll_api.get_pay_runs(xero_tenant_id=tenant_id, status="Draft") - for pay_run in getattr(response, "pay_runs", []) or []: - if ( - coerce_xero_date(pay_run.period_start_date) == week_start_date - and coerce_xero_date(pay_run.period_end_date) == week_end_date - and getattr(pay_run, "pay_run_status", None) == "Draft" - ): - return pay_run - return None - - -def delete_same_week_draft_pay_run(week_start_date: date) -> str: - pay_run = _find_same_week_draft_pay_run(week_start_date) - if pay_run is None: - raise ValueError( - "Xero blocked leave deletion because of a draft pay run, but no " - f"same-week draft pay run was found for week {week_start_date}." - ) - - pay_run_id = str(pay_run.pay_run_id) - logger.warning( - "Deleting same-week draft pay run %s so stale Xero leave can be reconciled", - pay_run_id, - ) - response = _update_pay_run( - pay_run_id, - _pay_run_payload_from_object(pay_run, status="Deleted"), - ) - XeroPayRun.objects.filter(xero_id=pay_run_id).update(pay_run_status="Deleted") - time.sleep(SLEEP_TIME) - if response and getattr(response, "pay_run", None): - transform_pay_run(response.pay_run, str(response.pay_run.pay_run_id)) - return pay_run_id - - def ensure_pay_run_for_week(week_start_date: date) -> Dict[str, Any]: """ Ensure a Draft pay run exists in Xero for the given payroll week and return it. @@ -1711,12 +1609,59 @@ def post_timesheet( raise +def _employee_leave_payload( + leave_type_id: str, + start_date: date, + end_date: date, + total_units: Decimal, + week_start_date: date, + week_end_date: date, + description: str, +) -> EmployeeLeave: + """Build the one shape of leave payload the Xero NZ API honours. + + Verified against the live API (KAN-326, 2026-08-02): per-day periods are + accepted but their units are DISCARDED — Xero recomputes them from the + employee's working pattern. A single period spanning the payroll week with + the total units is stored exactly as sent; more than one period per pay + period is rejected on update ("Multiple PayPeriods are not allowed"). + """ + if end_date < start_date: + raise ValueError( + f"end_date {end_date} cannot be before start_date {start_date}" + ) + if not (week_start_date <= start_date and end_date <= week_end_date): + raise ValueError( + f"Leave {start_date}–{end_date} is not within the payroll week " + f"{week_start_date}–{week_end_date}" + ) + if total_units <= 0: + raise ValueError(f"total_units must be positive, got {total_units}") + + return EmployeeLeave( + leave_type_id=leave_type_id, + description=description or f"Leave from {start_date} to {end_date}", + start_date=start_date, + end_date=end_date, + periods=[ + LeavePeriod( + period_start_date=week_start_date, + period_end_date=week_end_date, + number_of_units=float(total_units), + period_status="Approved", + ) + ], + ) + + def create_employee_leave( employee_id: UUID, leave_type_id: str, start_date: date, end_date: date, - hours_per_day: float, + total_units: Decimal, + week_start_date: date, + week_end_date: date, description: str = "", ) -> str: """ @@ -1727,72 +1672,43 @@ def create_employee_leave( leave_type_id: Xero leave type ID (str UUID) start_date: First day of leave end_date: Last day of leave (inclusive) - hours_per_day: Hours per day for this leave + total_units: Total leave hours across the whole span + week_start_date: Monday of the payroll week containing the leave + week_end_date: Sunday of that payroll week description: Optional description of the leave Returns: Leave ID (UUID string) from Xero - - Raises: - Exception: If API call fails or validation fails """ - if not get_tenant_id(): + tenant_id = get_tenant_id() + if not tenant_id: raise ValueError("No Xero tenant ID configured") - if not employee_id: - raise Exception("employee_id is required") - - if not leave_type_id: - raise Exception("leave_type_id is required") - - if not start_date or not end_date: - raise Exception("start_date and end_date are required") - - if end_date < start_date: - raise Exception("end_date cannot be before start_date") - - tenant_id = get_tenant_id() payroll_api = PayrollNzApi(api_client) try: logger.info( f"Creating leave for employee {employee_id}: " - f"{start_date} to {end_date} ({hours_per_day}h/day)" - ) - - # Build leave periods for each day - from xero_python.payrollnz.models import EmployeeLeave, LeavePeriod - - periods = [] - current_date = start_date - while current_date <= end_date: - # Skip weekends (Xero will handle this based on employee's schedule) - periods.append( - LeavePeriod( - period_start_date=current_date, - period_end_date=current_date, - number_of_units=hours_per_day, - period_status="Approved", # Auto-approve for our use case - ) - ) - current_date += timedelta(days=1) - - employee_leave = EmployeeLeave( - leave_type_id=leave_type_id, - description=description or f"Leave from {start_date} to {end_date}", - start_date=start_date, - end_date=end_date, - periods=periods, + f"{start_date} to {end_date} ({total_units}h total)" ) - response = payroll_api.create_employee_leave( xero_tenant_id=tenant_id, employee_id=str(employee_id), - employee_leave=employee_leave, + employee_leave=_employee_leave_payload( + leave_type_id, + start_date, + end_date, + total_units, + week_start_date, + week_end_date, + description, + ), ) if not response or not response.leave: - raise Exception("Failed to create employee leave") + raise ValueError( + f"Xero returned no leave record for employee {employee_id}" + ) leave_id = response.leave.leave_id logger.info(f"Successfully created leave record: {leave_id}") @@ -1815,6 +1731,75 @@ def create_employee_leave( raise +def update_employee_leave_in_place( + employee_id: UUID, + leave_id: str, + leave_type_id: str, + start_date: date, + end_date: date, + total_units: Decimal, + week_start_date: date, + week_end_date: date, + description: str = "", +) -> str: + """ + Update an existing Xero leave record's span and total units in place. + + Xero permits this even while the employee is in a draft pay run (verified + live 2026-08-02), which is what makes update-first reconciliation possible — + deletion is what the draft pay run blocks. + + Returns: + The leave ID (unchanged) for symmetry with create_employee_leave. + """ + tenant_id = get_tenant_id() + if not tenant_id: + raise ValueError("No Xero tenant ID configured") + + payroll_api = PayrollNzApi(api_client) + + try: + logger.info( + f"Updating leave {leave_id} for employee {employee_id}: " + f"{start_date} to {end_date} ({total_units}h total)" + ) + response = payroll_api.update_employee_leave( + xero_tenant_id=tenant_id, + employee_id=str(employee_id), + leave_id=leave_id, + employee_leave=_employee_leave_payload( + leave_type_id, + start_date, + end_date, + total_units, + week_start_date, + week_end_date, + description, + ), + ) + + if not response or not response.leave: + raise ValueError(f"Xero returned no leave record updating leave {leave_id}") + + return str(response.leave.leave_id) + + except Exception as exc: + logger.error( + f"Failed to update leave {leave_id} for employee {employee_id}: {exc}", + exc_info=True, + ) + persist_app_error( + exc, + additional_context={ + "operation": "update_employee_leave_in_place", + "employee_id": str(employee_id), + "leave_id": leave_id, + "leave_type_id": leave_type_id, + }, + ) + raise + + # ============================================================================= # Sync-focused API functions # These return raw Xero objects for use by the sync system @@ -2227,183 +2212,205 @@ def _map_work_entries(entries: List) -> List[Dict[str, Any]]: return timesheet_lines -def _delete_existing_leave_for_week( - employee_id: UUID, - week_start_date: date, - week_end_date: date, -) -> int: - """ - Delete any existing leave records for an employee that fall entirely - within the given week. This prevents duplicates when re-posting payroll. - - Only deletes leave where start_date >= week_start_date AND - end_date <= week_end_date (conservative: won't touch leave spanning - across week boundaries). - - Args: - employee_id: Xero employee ID - week_start_date: Monday of the week - week_end_date: Sunday of the week +def _is_draft_pay_run_leave_block(exc: Exception) -> bool: + """Return whether Xero refused a leave change because of a draft pay run. - Returns: - Number of leave records deleted + Xero surfaces the block only as an error string, no code. Captured live + 2026-08-02 (KAN-326): "Could not delete the leave request. There is a + draft pay run for this employee." """ - tenant_id = get_tenant_id() - if not tenant_id: - raise ValueError("No Xero tenant ID configured") - - payroll_api = PayrollNzApi(api_client) - - logger.info( - f"Checking for existing leave for employee {employee_id} " - f"in week {week_start_date} to {week_end_date}" - ) - - response = payroll_api.get_employee_leaves( - xero_tenant_id=tenant_id, - employee_id=str(employee_id), - ) + message = str(exc).lower() + return "leave request" in message and "draft pay run" in message - if not response or not response.leave: - logger.info("No existing leave found for employee") - return 0 - deleted_count = 0 - for leave in response.leave: - leave_start_date = coerce_xero_date(leave.start_date) - leave_end_date = coerce_xero_date(leave.end_date) - if leave_start_date is None or leave_end_date is None: - raise ValueError( - f"Xero leave {leave.leave_id} has invalid date range: " - f"{leave.start_date!r} to {leave.end_date!r}" - ) +class LeaveRequestSpec(TypedDict): + """One leave request Docketworks wants to exist in Xero.""" - # Only delete leave fully within the week - if leave_start_date >= week_start_date and leave_end_date <= week_end_date: - logger.info( - f"Deleting existing leave {leave.leave_id} " - f"({leave_start_date} to {leave_end_date})" - ) - try: - payroll_api.delete_employee_leave( - xero_tenant_id=tenant_id, - employee_id=str(employee_id), - leave_id=str(leave.leave_id), - ) - except Exception as exc: - if _is_draft_pay_run_leave_delete_block(exc): - raise DraftPayRunBlocksLeaveDeletion( - "Xero has an existing leave request " - f"{leave.leave_id} for employee {employee_id} " - f"({leave_start_date} to {leave_end_date}), but Xero " - "will not let Docketworks delete it while that employee " - "is in a draft pay run." - ) from exc - raise - deleted_count += 1 - time.sleep(SLEEP_TIME) + leave_type_id: str + start_date: date + end_date: date + total_units: Decimal + description: str - logger.info(f"Deleted {deleted_count} existing leave records") - return deleted_count +LeaveKey = tuple[str, date, date, Decimal] -def _is_draft_pay_run_leave_delete_block(exc: Exception) -> bool: - """Return whether Xero blocked leave deletion because of a draft pay run.""" - message = str(exc).lower() - return "delete the leave request" in message and "draft pay run" in message +def _leave_total_units(leave: EmployeeLeave) -> Decimal: + """Total leave hours across all of a Xero leave record's periods. -def _leave_units_per_day(leave: Any) -> Decimal | None: - periods = getattr(leave, "periods", None) or [] - units = [] + Xero collapses leave into one period per pay period and keeps only the + total (per-day breakdowns sent to the API are discarded — verified live + 2026-08-02, KAN-326), so the total is the only unit figure that + round-trips and the only one leave can be matched on. + """ + periods = leave.periods or [] + if not periods: + raise ValueError(f"Xero leave {leave.leave_id} has no periods") + total = Decimal("0") for period in periods: - value = getattr(period, "number_of_units", None) - if value is None: - value = getattr(period, "number_of_units_taken", None) - if value is not None: - units.append(Decimal(str(value)).quantize(Decimal("0.001"))) - if not units: - return None - first = units[0] - if any(unit != first for unit in units): - return None - return first + units = period.number_of_units + if units is None: + units = period.number_of_units_taken + if units is None: + raise ValueError( + f"Xero leave {leave.leave_id} period " + f"{period.period_start_date} has no units" + ) + total += Decimal(str(units)) + return total.quantize(Decimal("0.001")) def _leave_request_key( leave_type_id: str, start_date: date, end_date: date, - hours_per_day: Decimal, -) -> tuple[str, date, date, Decimal]: + total_units: Decimal, +) -> LeaveKey: return ( str(leave_type_id), start_date, end_date, - Decimal(str(hours_per_day)).quantize(Decimal("0.001")), + Decimal(str(total_units)).quantize(Decimal("0.001")), ) -def _build_leave_requests(entries: List) -> List[Dict[str, Any]]: - grouped = defaultdict(list) - pay_items = {} +def _build_leave_requests(entries: Sequence["CostLine"]) -> List[LeaveRequestSpec]: + """Derive the desired Xero leave requests from leave-flagged CostLines. + + One request per contiguous run of dates per leave type; hours on the same + day are summed. Mixed hours across days (e.g. 8/8/8/4.5) stay one request + carrying the total, matching the only representation Xero preserves. + """ + day_totals: defaultdict[str, defaultdict[date, Decimal]] = defaultdict( + lambda: defaultdict(Decimal) + ) + descriptions: Dict[str, str] = {} for entry in entries: pay_item = entry.xero_pay_item if pay_item is None: raise ValueError(f"CostLine {entry.id} has no xero_pay_item set") - grouped[pay_item.xero_id].append(entry) - pay_items[pay_item.xero_id] = pay_item - - requests = [] - for leave_type_id, type_entries in grouped.items(): - pay_item = pay_items[leave_type_id] - type_entries.sort(key=lambda e: e.accounting_date) - if not type_entries: - continue - - current_start = type_entries[0].accounting_date - current_end = type_entries[0].accounting_date - current_hours = Decimal(str(type_entries[0].quantity)).quantize( - Decimal("0.001") - ) - - for entry in type_entries[1:]: - entry_hours = Decimal(str(entry.quantity)).quantize(Decimal("0.001")) - expected_next = current_end + timedelta(days=1) - if entry.accounting_date == expected_next and entry_hours == current_hours: - current_end = entry.accounting_date + leave_type_id = str(pay_item.xero_id) + day_totals[leave_type_id][entry.accounting_date] += Decimal(str(entry.quantity)) + descriptions[leave_type_id] = pay_item.name + + specs: List[LeaveRequestSpec] = [] + for leave_type_id, by_date in day_totals.items(): + days = sorted(by_date) + spans: List[Tuple[date, date]] = [] + span_start = span_end = days[0] + for day in days[1:]: + if day == span_end + timedelta(days=1): + span_end = day else: - requests.append( - { - "leave_type_id": pay_item.xero_id, - "start_date": current_start, - "end_date": current_end, - "hours_per_day": current_hours, - "description": pay_item.name, - } + spans.append((span_start, span_end)) + span_start = span_end = day + spans.append((span_start, span_end)) + + for start_day, end_day in spans: + total = sum( + ( + units + for day, units in by_date.items() + if start_day <= day <= end_day + ), + Decimal("0"), + ) + specs.append( + LeaveRequestSpec( + leave_type_id=leave_type_id, + start_date=start_day, + end_date=end_day, + total_units=total.quantize(Decimal("0.001")), + description=descriptions[leave_type_id], ) - current_start = entry.accounting_date - current_end = entry.accounting_date - current_hours = entry_hours + ) + return specs - requests.append( - { - "leave_type_id": pay_item.xero_id, - "start_date": current_start, - "end_date": current_end, - "hours_per_day": current_hours, - "description": pay_item.name, - } - ) - return requests + +def _draft_pay_run_summary() -> str: + """Name every draft pay run the operator may need to delete in Xero. + + Xero blocks leave changes for an employee in ANY draft pay run, not just + the week being posted, so enumerate all mirrored drafts. + """ + drafts = list( + XeroPayRun.objects.filter(pay_run_status="Draft").order_by("period_start_date") + ) + if not drafts: + return ( + "the draft pay run (it has not yet synced to Docketworks — " + "look for it under Payroll → Pay runs)" + ) + return " and ".join( + f"the draft pay run for {draft.period_start_date} to " + f"{draft.period_end_date}" + for draft in drafts + ) + + +def _draft_block_message( + action: str, + leave_id: str, + leave_start_date: date, + leave_end_date: date, +) -> str: + return ( + f"Xero is blocking a payroll leave change: leave request {leave_id} " + f"({leave_start_date} to {leave_end_date}) needs to be {action} to " + "match the timesheet, but Xero locks leave while the employee is in a " + "draft pay run, and draft pay runs cannot be deleted through Xero's " + "API. In Xero go to Payroll → Pay runs, delete " + f"{_draft_pay_run_summary()}, then post to Xero again." + ) + + +def _take_overlapping_spec( + desired_by_key: Dict[LeaveKey, LeaveRequestSpec], + leave_type_id: str, + leave_start_date: date, + leave_end_date: date, +) -> Optional[LeaveRequestSpec]: + """Pop the unmatched desired request that best overlaps a stale leave. + + Same leave type and at least one shared day required; largest overlap + wins, earliest start breaks ties (deterministic). + """ + best_key: Optional[LeaveKey] = None + best_rank: Optional[Tuple[int, date]] = None + for key, spec in desired_by_key.items(): + if spec["leave_type_id"] != leave_type_id: + continue + overlap = ( + min(spec["end_date"], leave_end_date) + - max(spec["start_date"], leave_start_date) + ).days + 1 + if overlap <= 0: + continue + rank = (-overlap, spec["start_date"]) + if best_rank is None or rank < best_rank: + best_key = key + best_rank = rank + if best_key is None: + return None + return desired_by_key.pop(best_key) def reconcile_leave_for_staff_week( employee_id: UUID, - entries: List, + entries: Sequence["CostLine"], week_start_date: date, week_end_date: date, ) -> List[str]: + """Make the employee's Xero leave for the week match the timesheet. + + Key-matched leave is kept untouched. Stale leave with an overlapping + desired request of the same type is updated in place — Xero permits leave + updates even while the employee is in a draft pay run (verified live + 2026-08-02, KAN-326), unlike deletion. Only leave with no desired + counterpart is deleted; if Xero blocks that, DraftPayRunBlocksLeaveChange + tells the operator exactly which draft pay run to remove in the Xero UI. + """ tenant_id = get_tenant_id() if not tenant_id: raise ValueError("No Xero tenant ID configured") @@ -2413,18 +2420,19 @@ def reconcile_leave_for_staff_week( xero_tenant_id=tenant_id, employee_id=str(employee_id), ) - existing_leaves = getattr(response, "leave", None) or [] - desired_by_key = { + existing_leaves = response.leave or [] + desired_by_key: Dict[LeaveKey, LeaveRequestSpec] = { _leave_request_key( - request["leave_type_id"], - request["start_date"], - request["end_date"], - request["hours_per_day"], - ): request - for request in _build_leave_requests(entries) + spec["leave_type_id"], + spec["start_date"], + spec["end_date"], + spec["total_units"], + ): spec + for spec in _build_leave_requests(entries) } - kept_leave_ids = [] + kept_leave_ids: List[str] = [] + stale_leaves: List[Tuple[EmployeeLeave, date, date]] = [] for leave in existing_leaves: leave_start_date = coerce_xero_date(leave.start_date) leave_end_date = coerce_xero_date(leave.end_date) @@ -2436,20 +2444,55 @@ def reconcile_leave_for_staff_week( if not ( leave_start_date >= week_start_date and leave_end_date <= week_end_date ): - continue + continue # never touch leave spanning week boundaries - hours_per_day = _leave_units_per_day(leave) - existing_key = None - if hours_per_day is not None: - existing_key = _leave_request_key( - leave.leave_type_id, - leave_start_date, - leave_end_date, - hours_per_day, - ) + existing_key = _leave_request_key( + leave.leave_type_id, + leave_start_date, + leave_end_date, + _leave_total_units(leave), + ) if existing_key in desired_by_key: kept_leave_ids.append(str(leave.leave_id)) del desired_by_key[existing_key] + else: + stale_leaves.append((leave, leave_start_date, leave_end_date)) + + updated_leave_ids: List[str] = [] + for leave, leave_start_date, leave_end_date in stale_leaves: + replacement = _take_overlapping_spec( + desired_by_key, + str(leave.leave_type_id), + leave_start_date, + leave_end_date, + ) + if replacement is not None: + try: + updated_leave_ids.append( + update_employee_leave_in_place( + employee_id=employee_id, + leave_id=str(leave.leave_id), + leave_type_id=replacement["leave_type_id"], + start_date=replacement["start_date"], + end_date=replacement["end_date"], + total_units=replacement["total_units"], + week_start_date=week_start_date, + week_end_date=week_end_date, + description=replacement["description"], + ) + ) + except Exception as exc: + if _is_draft_pay_run_leave_block(exc): + raise DraftPayRunBlocksLeaveChange( + _draft_block_message( + "replaced", + str(leave.leave_id), + leave_start_date, + leave_end_date, + ) + ) from exc + raise + time.sleep(SLEEP_TIME) continue logger.info( @@ -2463,65 +2506,64 @@ def reconcile_leave_for_staff_week( leave_id=str(leave.leave_id), ) except Exception as exc: - if _is_draft_pay_run_leave_delete_block(exc): - raise DraftPayRunBlocksLeaveDeletion( - "Xero has an obsolete leave request " - f"{leave.leave_id} for employee {employee_id} " - f"({leave_start_date} to {leave_end_date}), but Xero " - "will not let Docketworks delete it while that employee " - "is in a draft pay run." + if _is_draft_pay_run_leave_block(exc): + raise DraftPayRunBlocksLeaveChange( + _draft_block_message( + "removed", + str(leave.leave_id), + leave_start_date, + leave_end_date, + ) ) from exc raise time.sleep(SLEEP_TIME) - created_leave_ids = [] - for request in desired_by_key.values(): - created_leave_ids.append( - create_employee_leave( - employee_id=employee_id, - leave_type_id=request["leave_type_id"], - start_date=request["start_date"], - end_date=request["end_date"], - hours_per_day=float(request["hours_per_day"]), - description=request["description"], - ) + created_leave_ids = [ + create_employee_leave( + employee_id=employee_id, + leave_type_id=spec["leave_type_id"], + start_date=spec["start_date"], + end_date=spec["end_date"], + total_units=spec["total_units"], + week_start_date=week_start_date, + week_end_date=week_end_date, + description=spec["description"], ) - return kept_leave_ids + created_leave_ids + for spec in desired_by_key.values() + ] + return kept_leave_ids + updated_leave_ids + created_leave_ids def reconcile_leave_for_week_for_staff( staff_ids: List[UUID], week_start_date: date, - *, - allow_draft_rebuild: bool = True, -) -> int: +) -> List[str]: """Reconcile Xero leave for selected staff before pay-run creation.""" from apps.accounts.models import Staff from apps.job.models.costing import CostLine week_end_date = week_start_date + timedelta(days=6) - leave_ids = [] + leave_ids: List[str] = [] staff_by_id = Staff.objects.in_bulk(staff_ids) - try: - for staff_id in staff_ids: - staff = staff_by_id.get(staff_id) - if staff is None: - raise ValueError(f"Staff member {staff_id} not found") - if not staff.xero_user_id: - raise ValueError( - f"Staff member {staff.email} does not have a xero_user_id configured" - ) - entries = [ - entry - for entry in CostLine.objects.filter( - cost_set__kind="actual", - kind="time", - staff_id=staff_id, - accounting_date__gte=week_start_date, - accounting_date__lte=week_end_date, - xero_pay_item__uses_leave_api=True, - ).select_related("xero_pay_item") - ] + for staff_id in staff_ids: + staff = staff_by_id.get(staff_id) + if staff is None: + raise ValueError(f"Staff member {staff_id} not found") + if not staff.xero_user_id: + raise ValueError( + f"Staff member {staff.email} does not have a xero_user_id configured" + ) + entries = list( + CostLine.objects.filter( + cost_set__kind="actual", + kind="time", + staff_id=staff_id, + accounting_date__gte=week_start_date, + accounting_date__lte=week_end_date, + xero_pay_item__uses_leave_api=True, + ).select_related("xero_pay_item") + ) + try: leave_ids.extend( reconcile_leave_for_staff_week( UUID(staff.xero_user_id), @@ -2530,115 +2572,12 @@ def reconcile_leave_for_week_for_staff( week_end_date, ) ) - except DraftPayRunBlocksLeaveDeletion: - if not allow_draft_rebuild: - raise - deleted_pay_run_id = delete_same_week_draft_pay_run(week_start_date) - logger.warning( - "Deleted draft pay run %s; retrying stale Xero leave cleanup", - deleted_pay_run_id, - ) - return reconcile_leave_for_week_for_staff( - staff_ids, - week_start_date, - allow_draft_rebuild=False, - ) - return leave_ids - - -def _post_leave_entries( - employee_id: UUID, - entries: List, - week_start_date: date, - week_end_date: date, -) -> List[str]: - """ - Post leave CostLine entries to Xero using the Leave API. - - Deletes any existing leave for the week first (upsert pattern), - then groups consecutive days of the same leave type together. - Uses entry.xero_pay_item to get Xero leave type IDs. - - Args: - employee_id: Xero employee ID - entries: List of leave CostLine entries - week_start_date: Monday of the week - week_end_date: Sunday of the week - - Returns: - List of leave IDs created in Xero - """ - # Delete existing leave for this week to prevent duplicates - _delete_existing_leave_for_week(employee_id, week_start_date, week_end_date) - - # Group entries by XeroPayItem and sort by date - grouped = defaultdict(list) - for entry in entries: - pay_item = entry.xero_pay_item - - if pay_item is None: - raise ValueError(f"CostLine {entry.id} has no xero_pay_item set") - - grouped[pay_item].append(entry) - - # Sort each group by date - for pay_item in grouped: - grouped[pay_item].sort(key=lambda e: e.accounting_date) - - leave_ids = [] - - # Process each leave type - for pay_item, type_entries in grouped.items(): - # Use xero_id directly from XeroPayItem - leave_type_id = pay_item.xero_id - - # Group consecutive days together - if not type_entries: - continue - - current_start = type_entries[0].accounting_date - current_end = type_entries[0].accounting_date - current_hours = float(type_entries[0].quantity) - - for i in range(1, len(type_entries)): - entry = type_entries[i] - expected_next = current_end + timedelta(days=1) - - # Check if consecutive and same hours per day - if ( - entry.accounting_date == expected_next - and abs(float(entry.quantity) - current_hours) < 0.01 - ): - # Extend current range - current_end = entry.accounting_date - else: - # Create leave for current range - leave_id = create_employee_leave( - employee_id=employee_id, - leave_type_id=leave_type_id, - start_date=current_start, - end_date=current_end, - hours_per_day=current_hours, - description=pay_item.name, - ) - leave_ids.append(leave_id) - - # Start new range - current_start = entry.accounting_date - current_end = entry.accounting_date - current_hours = float(entry.quantity) - - # Create leave for final range - leave_id = create_employee_leave( - employee_id=employee_id, - leave_type_id=leave_type_id, - start_date=current_start, - end_date=current_end, - hours_per_day=current_hours, - description=pay_item.name, - ) - leave_ids.append(leave_id) - + except DraftPayRunBlocksLeaveChange as exc: + # The inner function only knows the Xero employee UUID; give the + # operator the staff member's name. + raise DraftPayRunBlocksLeaveChange( + f"{staff.get_display_full_name()}: {exc}" + ) from exc return leave_ids diff --git a/apps/workflow/tests/test_xero_payroll_leave.py b/apps/workflow/tests/test_xero_payroll_leave.py index 9316a2a42..301ba4bd2 100644 --- a/apps/workflow/tests/test_xero_payroll_leave.py +++ b/apps/workflow/tests/test_xero_payroll_leave.py @@ -1,165 +1,341 @@ +"""Leave reconciliation against Xero NZ Payroll (KAN-326). + +These tests build Xero-side echoes and capture outbound payloads with the REAL +xero_python models — only the transport (PayrollNzApi methods, get_tenant_id, +time.sleep) is mocked. The prod incident survived the old suite precisely +because the payload construction was mocked out: a real PayRun model would +have raised on pay_run_status="Deleted" in any test that exercised it. +""" + from datetime import date, datetime from decimal import Decimal from types import SimpleNamespace from unittest.mock import MagicMock, patch from uuid import UUID -from django.test import SimpleTestCase +from django.test import SimpleTestCase, TestCase +from django.utils import timezone +from xero_python.payrollnz.models import EmployeeLeave, LeavePeriod +from apps.job.models.costing import CostLine from apps.workflow.api.xero.payroll import ( - DraftPayRunBlocksLeaveDeletion, - _delete_existing_leave_for_week, + DraftPayRunBlocksLeaveChange, + _build_leave_requests, + create_employee_leave, reconcile_leave_for_staff_week, reconcile_leave_for_week_for_staff, ) +from apps.workflow.models import XeroPayItem, XeroPayRun +EMPLOYEE_ID = UUID("3a2e113b-425e-5e48-b5e5-a596cb4fb2d6") +WEEK_START = date(2026, 7, 27) +WEEK_END = date(2026, 8, 2) +SICK_TYPE = "sick-type-1" -class DeleteExistingLeaveForWeekTests(SimpleTestCase): - @patch("apps.workflow.api.xero.payroll.time.sleep") - @patch("apps.workflow.api.xero.payroll.PayrollNzApi") - @patch("apps.workflow.api.xero.payroll.get_tenant_id", return_value="tenant-1") - def test_accepts_datetime_leave_dates( +# Captured live from the Xero NZ API on 2026-08-02 (KAN-326). +DRAFT_BLOCK_MESSAGE = ( + "Could not delete the leave request. There is a draft pay run " "for this employee." +) + + +def _cost_line(day: date, hours: str) -> CostLine: + return CostLine( + kind="time", + accounting_date=day, + quantity=Decimal(hours), + xero_pay_item=XeroPayItem( + xero_id=SICK_TYPE, name="Sick Leave", uses_leave_api=True + ), + ) + + +def _xero_leave( + leave_id: str, + start: date, + end: date, + total_units: float, + leave_type_id: str = SICK_TYPE, +) -> EmployeeLeave: + """A leave record as Xero actually returns it: one lumped period per pay + week carrying only the total units (per-day breakdowns are not preserved). + """ + return EmployeeLeave( + leave_id=leave_id, + leave_type_id=leave_type_id, + description="Sick Leave", + start_date=datetime(start.year, start.month, start.day), + end_date=datetime(end.year, end.month, end.day), + periods=[ + LeavePeriod( + period_start_date=WEEK_START, + period_end_date=WEEK_END, + number_of_units=total_units, + period_status="Approved", + ) + ], + ) + + +class BuildLeaveRequestsTests(SimpleTestCase): + def test_single_request_for_mixed_hours(self) -> None: + """The prod incident's structural mismatch: a contiguous run with + non-uniform daily hours must stay ONE request carrying the total, not + split into one request per distinct hours value.""" + specs = _build_leave_requests( + [ + _cost_line(date(2026, 7, 28), "4.5"), + _cost_line(date(2026, 7, 29), "8"), + _cost_line(date(2026, 7, 30), "8"), + _cost_line(date(2026, 7, 31), "8"), + ] + ) + + self.assertEqual(len(specs), 1) + self.assertEqual(specs[0]["start_date"], date(2026, 7, 28)) + self.assertEqual(specs[0]["end_date"], date(2026, 7, 31)) + self.assertEqual(specs[0]["total_units"], Decimal("28.500")) + + def test_splits_on_date_gap(self) -> None: + """Non-contiguous leave days become separate requests.""" + specs = _build_leave_requests( + [ + _cost_line(date(2026, 7, 27), "8"), + _cost_line(date(2026, 7, 28), "8"), + _cost_line(date(2026, 7, 30), "8"), + ] + ) + + self.assertEqual( + [(s["start_date"], s["end_date"], s["total_units"]) for s in specs], + [ + (date(2026, 7, 27), date(2026, 7, 28), Decimal("16.000")), + (date(2026, 7, 30), date(2026, 7, 30), Decimal("8.000")), + ], + ) + + def test_sums_same_day_entries(self) -> None: + """Two cost lines on the same day merge into one request instead of + producing overlapping duplicate leave requests in Xero.""" + specs = _build_leave_requests( + [ + _cost_line(date(2026, 7, 28), "4"), + _cost_line(date(2026, 7, 28), "4"), + ] + ) + + self.assertEqual(len(specs), 1) + self.assertEqual(specs[0]["total_units"], Decimal("8.000")) + + +@patch("apps.workflow.api.xero.payroll.time.sleep") +@patch("apps.workflow.api.xero.payroll.PayrollNzApi") +@patch("apps.workflow.api.xero.payroll.get_tenant_id", return_value="tenant-1") +class ReconcileLeaveTests(SimpleTestCase): + def test_keeps_lumped_mixed_hours_leave( self, - mock_get_tenant_id, - mock_payroll_api_cls, - mock_sleep, - ): + mock_get_tenant_id: MagicMock, + mock_payroll_api_cls: MagicMock, + mock_sleep: MagicMock, + ) -> None: + """Regression for the prod incident: a 28.5h lumped Xero leave over a + 4.5/8/8/8 timesheet week must key-match and be left untouched. The old + per-day differ declared it permanently obsolete and deleted it on + every payroll run.""" payroll_api = mock_payroll_api_cls.return_value payroll_api.get_employee_leaves.return_value = SimpleNamespace( - leave=[ - SimpleNamespace( - leave_id="leave-1", - start_date=datetime(2025, 5, 6, 0, 0), - end_date=datetime(2025, 5, 7, 0, 0), - ) - ] + leave=[_xero_leave("leave-1", date(2026, 7, 28), date(2026, 7, 31), 28.5)] ) + entries = [ + _cost_line(date(2026, 7, 28), "4.5"), + _cost_line(date(2026, 7, 29), "8"), + _cost_line(date(2026, 7, 30), "8"), + _cost_line(date(2026, 7, 31), "8"), + ] - deleted = _delete_existing_leave_for_week( - UUID("3a2e113b-425e-5e48-b5e5-a596cb4fb2d6"), - date(2025, 5, 5), - date(2025, 5, 11), + leave_ids = reconcile_leave_for_staff_week( + EMPLOYEE_ID, entries, WEEK_START, WEEK_END ) - self.assertEqual(deleted, 1) - payroll_api.delete_employee_leave.assert_called_once_with( - xero_tenant_id="tenant-1", - employee_id="3a2e113b-425e-5e48-b5e5-a596cb4fb2d6", - leave_id="leave-1", - ) + self.assertEqual(leave_ids, ["leave-1"]) + payroll_api.delete_employee_leave.assert_not_called() + payroll_api.update_employee_leave.assert_not_called() + payroll_api.create_employee_leave.assert_not_called() - @patch("apps.workflow.api.xero.payroll.create_employee_leave") - @patch("apps.workflow.api.xero.payroll.PayrollNzApi") - @patch("apps.workflow.api.xero.payroll.get_tenant_id", return_value="tenant-1") - def test_reconcile_keeps_matching_existing_leave( + def test_updates_changed_leave_in_place( self, - mock_get_tenant_id, - mock_payroll_api_cls, - mock_create_employee_leave, - ): + mock_get_tenant_id: MagicMock, + mock_payroll_api_cls: MagicMock, + mock_sleep: MagicMock, + ) -> None: + """Changed leave of the same type and overlapping span is updated in + place (permitted during a draft pay run), never deleted-and-recreated.""" payroll_api = mock_payroll_api_cls.return_value payroll_api.get_employee_leaves.return_value = SimpleNamespace( - leave=[ - SimpleNamespace( - leave_id="leave-1", - leave_type_id="sick-type-1", - start_date=date(2025, 5, 7), - end_date=date(2025, 5, 7), - periods=[ - SimpleNamespace( - number_of_units=8.0, - number_of_units_taken=None, - ) - ], - ) - ] + leave=[_xero_leave("leave-1", date(2026, 7, 27), date(2026, 7, 30), 32.0)] + ) + payroll_api.update_employee_leave.return_value = SimpleNamespace( + leave=_xero_leave("leave-1", date(2026, 7, 28), date(2026, 7, 31), 28.5) ) entries = [ - SimpleNamespace( - id="costline-1", - accounting_date=date(2025, 5, 7), - quantity=Decimal("8.000"), - xero_pay_item=SimpleNamespace( - xero_id="sick-type-1", - name="Sick Leave", - ), - ) + _cost_line(date(2026, 7, 28), "4.5"), + _cost_line(date(2026, 7, 29), "8"), + _cost_line(date(2026, 7, 30), "8"), + _cost_line(date(2026, 7, 31), "8"), ] leave_ids = reconcile_leave_for_staff_week( - UUID("3a2e113b-425e-5e48-b5e5-a596cb4fb2d6"), - entries, - date(2025, 5, 5), - date(2025, 5, 11), + EMPLOYEE_ID, entries, WEEK_START, WEEK_END ) self.assertEqual(leave_ids, ["leave-1"]) payroll_api.delete_employee_leave.assert_not_called() - mock_create_employee_leave.assert_not_called() + payroll_api.create_employee_leave.assert_not_called() + payroll_api.update_employee_leave.assert_called_once() + call = payroll_api.update_employee_leave.call_args + self.assertEqual(call.kwargs["leave_id"], "leave-1") + payload = call.kwargs["employee_leave"] + self.assertIsInstance(payload, EmployeeLeave) + self.assertEqual(payload.start_date, date(2026, 7, 28)) + self.assertEqual(payload.end_date, date(2026, 7, 31)) + self.assertEqual(len(payload.periods), 1) + self.assertEqual(payload.periods[0].period_start_date, WEEK_START) + self.assertEqual(payload.periods[0].period_end_date, WEEK_END) + self.assertEqual(payload.periods[0].number_of_units, 28.5) + +class ReconcileBlockedChangeTests(TestCase): + @patch("apps.workflow.api.xero.payroll.time.sleep") @patch("apps.workflow.api.xero.payroll.PayrollNzApi") @patch("apps.workflow.api.xero.payroll.get_tenant_id", return_value="tenant-1") - def test_raises_draft_pay_run_leave_delete_block( + def test_blocked_delete_raises_actionable_error( self, - mock_get_tenant_id, - mock_payroll_api_cls, - ): + mock_get_tenant_id: MagicMock, + mock_payroll_api_cls: MagicMock, + mock_sleep: MagicMock, + ) -> None: + """When Xero blocks a required leave deletion, the operator gets told + which draft pay run to delete in the Xero UI — replacing the removed + auto-delete path, which was impossible (the NZ Payroll API has no + pay-run delete endpoint) and crashed every payroll run.""" + XeroPayRun.objects.create( + xero_id=UUID("17d7ca66-ee10-4e8a-918f-f8a5a890d1ac"), + xero_tenant_id="tenant-1", + period_start_date=WEEK_START, + period_end_date=WEEK_END, + payment_date=WEEK_END, + pay_run_status="Draft", + raw_json={}, + xero_last_modified=timezone.now(), + ) payroll_api = mock_payroll_api_cls.return_value payroll_api.get_employee_leaves.return_value = SimpleNamespace( - leave=[ - SimpleNamespace( - leave_id="leave-1", - start_date=date(2025, 5, 7), - end_date=date(2025, 5, 7), - ) - ] + leave=[_xero_leave("leave-1", date(2026, 7, 28), date(2026, 7, 31), 28.5)] + ) + payroll_api.delete_employee_leave.side_effect = Exception(DRAFT_BLOCK_MESSAGE) + + with self.assertRaises(DraftPayRunBlocksLeaveChange) as ctx: + reconcile_leave_for_staff_week(EMPLOYEE_ID, [], WEEK_START, WEEK_END) + + message = str(ctx.exception) + self.assertIn("leave-1", message) + self.assertIn("Payroll → Pay runs", message) + self.assertIn(f"{WEEK_START} to {WEEK_END}", message) + self.assertIsNotNone(ctx.exception.__cause__) + # The old recovery path went on to PUT /PayRuns/{id}; nothing may + # touch pay runs now. + payroll_api.get_pay_runs.assert_not_called() + payroll_api.create_pay_run.assert_not_called() + + @patch("apps.workflow.api.xero.payroll.time.sleep") + @patch("apps.workflow.api.xero.payroll.PayrollNzApi") + @patch("apps.workflow.api.xero.payroll.get_tenant_id", return_value="tenant-1") + def test_unrelated_delete_failure_propagates_untranslated( + self, + mock_get_tenant_id: MagicMock, + mock_payroll_api_cls: MagicMock, + mock_sleep: MagicMock, + ) -> None: + """Only the draft-pay-run block gets the operator guidance; any other + Xero failure must surface as itself.""" + payroll_api = mock_payroll_api_cls.return_value + payroll_api.get_employee_leaves.return_value = SimpleNamespace( + leave=[_xero_leave("leave-1", date(2026, 7, 28), date(2026, 7, 31), 28.5)] ) - payroll_api.delete_employee_leave.side_effect = Exception( - "Could not delete the leave request. There is a draft pay run " - "for this employee." + payroll_api.delete_employee_leave.side_effect = Exception("rate limited") + + with self.assertRaisesRegex(Exception, "rate limited"): + reconcile_leave_for_staff_week(EMPLOYEE_ID, [], WEEK_START, WEEK_END) + + +class CreateEmployeeLeaveTests(SimpleTestCase): + @patch("apps.workflow.api.xero.payroll.PayrollNzApi") + @patch("apps.workflow.api.xero.payroll.get_tenant_id", return_value="tenant-1") + def test_builds_single_lumped_period( + self, + mock_get_tenant_id: MagicMock, + mock_payroll_api_cls: MagicMock, + ) -> None: + """The only payload shape Xero honours (verified live 2026-08-02): + one period spanning the payroll week carrying the total units. Per-day + periods are accepted but their units silently discarded.""" + payroll_api = mock_payroll_api_cls.return_value + payroll_api.create_employee_leave.return_value = SimpleNamespace( + leave=_xero_leave("leave-9", date(2026, 7, 28), date(2026, 7, 31), 28.5) ) - with self.assertRaisesRegex( - DraftPayRunBlocksLeaveDeletion, "existing leave request" - ): - _delete_existing_leave_for_week( - UUID("3a2e113b-425e-5e48-b5e5-a596cb4fb2d6"), - date(2025, 5, 5), - date(2025, 5, 11), - ) + leave_id = create_employee_leave( + employee_id=EMPLOYEE_ID, + leave_type_id=SICK_TYPE, + start_date=date(2026, 7, 28), + end_date=date(2026, 7, 31), + total_units=Decimal("28.500"), + week_start_date=WEEK_START, + week_end_date=WEEK_END, + description="Sick Leave", + ) + + self.assertEqual(leave_id, "leave-9") + payload = payroll_api.create_employee_leave.call_args.kwargs["employee_leave"] + self.assertIsInstance(payload, EmployeeLeave) + self.assertEqual(payload.start_date, date(2026, 7, 28)) + self.assertEqual(payload.end_date, date(2026, 7, 31)) + self.assertEqual(len(payload.periods), 1) + self.assertEqual(payload.periods[0].period_start_date, WEEK_START) + self.assertEqual(payload.periods[0].period_end_date, WEEK_END) + self.assertEqual(payload.periods[0].number_of_units, 28.5) + self.assertEqual(payload.periods[0].period_status, "Approved") + +class OrchestratorTests(SimpleTestCase): @patch("apps.accounts.models.Staff.objects.in_bulk") - @patch("apps.workflow.api.xero.payroll.delete_same_week_draft_pay_run") @patch("apps.workflow.api.xero.payroll.reconcile_leave_for_staff_week") @patch("apps.job.models.costing.CostLine.objects.filter") - def test_deletes_same_week_draft_and_retries_leave_cleanup( + def test_propagates_block_with_staff_name( self, - mock_costline_filter, - mock_reconcile_leave, - mock_delete_draft_pay_run, - mock_staff_in_bulk, - ): + mock_costline_filter: MagicMock, + mock_reconcile_leave: MagicMock, + mock_staff_in_bulk: MagicMock, + ) -> None: + """The per-staff reconciler only knows the Xero employee UUID; the + orchestrator must name the staff member for the operator, chain the + cause, and never retry (the old auto-delete retry is gone).""" staff_id = UUID("1833d340-b4dc-5870-acf9-41791be7fd8d") mock_staff_in_bulk.return_value = { staff_id: SimpleNamespace( email="timothy.harris@example.com", xero_user_id="55de6fd8-a845-4c27-94d8-841ddb815db3", + get_display_full_name=lambda: "Timothy Harris", ) } queryset = MagicMock() queryset.select_related.return_value = [] mock_costline_filter.return_value = queryset - mock_reconcile_leave.side_effect = [ - DraftPayRunBlocksLeaveDeletion("blocked"), - ["leave-1"], - ] - mock_delete_draft_pay_run.return_value = "payrun-1" + mock_reconcile_leave.side_effect = DraftPayRunBlocksLeaveChange("blocked") - leave_ids = reconcile_leave_for_week_for_staff( - [staff_id], - date(2025, 5, 5), - ) + with self.assertRaises(DraftPayRunBlocksLeaveChange) as ctx: + reconcile_leave_for_week_for_staff([staff_id], date(2026, 7, 27)) - self.assertEqual(leave_ids, ["leave-1"]) - mock_delete_draft_pay_run.assert_called_once_with(date(2025, 5, 5)) - self.assertEqual(mock_reconcile_leave.call_count, 2) + self.assertIn("Timothy Harris", str(ctx.exception)) + self.assertIn("blocked", str(ctx.exception)) + self.assertIsInstance(ctx.exception.__cause__, DraftPayRunBlocksLeaveChange) + mock_reconcile_leave.assert_called_once() diff --git a/mypy-baseline.txt b/mypy-baseline.txt index 51f22c9f9..9a0cbf011 100644 --- a/mypy-baseline.txt +++ b/mypy-baseline.txt @@ -1953,7 +1953,6 @@ apps/workflow/api/xero/client.py:0: error: Function is missing a type annotation apps/workflow/api/xero/payroll.py:0: error: Argument "employee_id" to "post_timesheet" has incompatible type "str"; expected "UUID" [arg-type] apps/workflow/api/xero/payroll.py:0: error: Call to untyped function "transform_pay_run" in typed context [no-untyped-call] apps/workflow/api/xero/payroll.py:0: error: Call to untyped function "transform_pay_run" in typed context [no-untyped-call] -apps/workflow/api/xero/payroll.py:0: error: Call to untyped function "transform_pay_run" in typed context [no-untyped-call] apps/workflow/api/xero/payroll.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/workflow/api/xero/payroll.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/workflow/api/xero/payroll.py:0: error: Function is missing a type annotation [no-untyped-def] @@ -1967,13 +1966,9 @@ apps/workflow/api/xero/payroll.py:0: error: Function is missing a type annotatio apps/workflow/api/xero/payroll.py:0: error: Function is missing a type annotation [no-untyped-def] apps/workflow/api/xero/payroll.py:0: error: Function is missing a type annotation for one or more parameters [no-untyped-def] apps/workflow/api/xero/payroll.py:0: error: Incompatible default for parameter "payment_date" (default has type "None", parameter has type "date") [assignment] -apps/workflow/api/xero/payroll.py:0: error: Incompatible return value type (got "list[str]", expected "int") [return-value] apps/workflow/api/xero/payroll.py:0: error: Incompatible types in assignment (expression has type "Any | None", variable has type "date") [assignment] apps/workflow/api/xero/payroll.py:0: error: Missing type arguments for generic type "List" [type-arg] apps/workflow/api/xero/payroll.py:0: error: Missing type arguments for generic type "List" [type-arg] -apps/workflow/api/xero/payroll.py:0: error: Missing type arguments for generic type "List" [type-arg] -apps/workflow/api/xero/payroll.py:0: error: Missing type arguments for generic type "List" [type-arg] -apps/workflow/api/xero/payroll.py:0: error: Missing type arguments for generic type "List" [type-arg] apps/workflow/api/xero/payroll.py:0: error: Missing type arguments for generic type "tuple" [type-arg] apps/workflow/api/xero/payroll.py:0: error: Missing type arguments for generic type "tuple" [type-arg] apps/workflow/api/xero/payroll.py:0: error: Need type annotation for "leave_ids" (hint: "leave_ids: list[] = ...") [var-annotated] @@ -2499,10 +2494,6 @@ apps/workflow/tests/test_xero_document_raw_json.py:0: error: Function is missing apps/workflow/tests/test_xero_document_raw_json.py:0: error: Incompatible types in assignment (expression has type "_FakeProvider", variable has type "AccountingProvider") [assignment] apps/workflow/tests/test_xero_document_raw_json.py:0: error: Incompatible types in assignment (expression has type "_FakeProvider", variable has type "AccountingProvider") [assignment] apps/workflow/tests/test_xero_document_raw_json.py:0: error: Incompatible types in assignment (expression has type "def _attach_workshop_pdf(self, external_id: str) -> None", variable has type "def _attach_workshop_pdf(self, invoice_external_id: str) -> str | None") [assignment] -apps/workflow/tests/test_xero_payroll_leave.py:0: error: Function is missing a type annotation [no-untyped-def] -apps/workflow/tests/test_xero_payroll_leave.py:0: error: Function is missing a type annotation [no-untyped-def] -apps/workflow/tests/test_xero_payroll_leave.py:0: error: Function is missing a type annotation [no-untyped-def] -apps/workflow/tests/test_xero_payroll_leave.py:0: error: Function is missing a type annotation [no-untyped-def] apps/workflow/tests/test_xero_payroll_pay_run_flow.py:0: error: Call to untyped function "_draft" in typed context [no-untyped-call] apps/workflow/tests/test_xero_payroll_pay_run_flow.py:0: error: Call to untyped function "_draft" in typed context [no-untyped-call] apps/workflow/tests/test_xero_payroll_pay_run_flow.py:0: error: Function is missing a type annotation [no-untyped-def] diff --git a/scripts/verify_kan326_leave_contracts.py b/scripts/verify_kan326_leave_contracts.py new file mode 100644 index 000000000..b0302ca0a --- /dev/null +++ b/scripts/verify_kan326_leave_contracts.py @@ -0,0 +1,395 @@ +#!/usr/bin/env python +"""KAN-326 verifier: empirically pin two undocumented Xero NZ Payroll contracts +against the dev demo tenant, BEFORE the leave-reconciliation fix is built. + +Contract 1: is update_employee_leave (PUT) permitted while the employee is in a + draft pay run? (Xero documents the block only for DELETE.) +Contract 2: does create_employee_leave accept explicit non-uniform per-day + periods (8/8/8/4.5), and what shape does Xero echo back? + +Run: .venv/bin/python scripts/verify_kan326_leave_contracts.py +Cleanup: .venv/bin/python scripts/verify_kan326_leave_contracts.py --cleanup + (only AFTER deleting the draft pay run in the Xero UI - see checklist + the main run prints) + +DESTRUCTIVE to the demo tenant: creates a leave record and a draft pay run. +The draft pay run can only be deleted in the Xero UI and blocks all dev +payroll posting on the calendar until it is removed. + +Temporary tooling - delete before the KAN-326 PR merges (outputs recorded in +the PR and Jira). +""" + +import argparse +import os +import sys +import time +from datetime import timedelta + +import django + +os.environ.setdefault("DJANGO_SETTINGS_MODULE", "docketworks.settings") +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +django.setup() + +from xero_python.exceptions import ApiException +from xero_python.payrollnz import PayrollNzApi +from xero_python.payrollnz.models import EmployeeLeave, LeavePeriod + +from apps.accounts.models import Staff +from apps.workflow.api.xero.auth import api_client +from apps.workflow.api.xero.payroll import ( + ensure_pay_run_for_week, + get_tenant_id, + next_postable_payroll_week, +) +from apps.workflow.models import XeroPayItem + +PAUSE = 1.5 # be gentle with the daily API quota + + +def show(label: str, obj: object) -> None: + print(f"\n=== {label} ===") + print(obj) + + +def show_leave(leave: EmployeeLeave) -> None: + print( + f"leave_id={leave.leave_id} type={leave.leave_type_id} " + f"start={leave.start_date} end={leave.end_date} desc={leave.description!r}" + ) + for p in leave.periods or []: + print( + f" period {p.period_start_date} -> {p.period_end_date} " + f"units={p.number_of_units} units_taken={p.number_of_units_taken} " + f"status={p.period_status}" + ) + + +def api_error_body(exc: ApiException) -> str: + return f"status={exc.status} reason={exc.reason} body={exc.body}" + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument("--cleanup", metavar="LEAVE_ID") + parser.add_argument("--employee-name", default="Jack Allen") + parser.add_argument( + "--probe-single-period", + metavar="LEAVE_ID", + help="Follow-up probe: update the given leave with ONE pay-period-wide " + "period carrying custom total units (28.5), while the draft pay run " + "still exists. Tests both update-during-draft and custom-units.", + ) + parser.add_argument( + "--probe-create-single", + action="store_true", + help="Follow-up probe on a DRAFT-FREE week: does create honor custom " + "units in a single lumped period? Can update change the date span? " + "Cleans up after itself (delete works with no draft).", + ) + args = parser.parse_args() + + tenant_id = get_tenant_id() + if not tenant_id: + raise SystemExit("No Xero tenant ID configured") + payroll_api = PayrollNzApi(api_client) + + staff = Staff.objects.filter( + first_name=args.employee_name.split()[0], + last_name=args.employee_name.split()[1], + ).first() + if staff is None or not staff.xero_user_id: + raise SystemExit(f"Staff {args.employee_name!r} with xero_user_id not found") + employee_id = str(staff.xero_user_id) + print(f"Employee: {args.employee_name} ({employee_id})") + + if args.cleanup: + show("CLEANUP: deleting test leave", args.cleanup) + resp = payroll_api.delete_employee_leave( + xero_tenant_id=tenant_id, employee_id=employee_id, leave_id=args.cleanup + ) + print(f"delete response: {resp}") + return + + if args.probe_single_period: + sick_item = XeroPayItem.objects.get(name="Sick Leave", uses_leave_api=True) + week_pair = next_postable_payroll_week() + if week_pair is None: + raise SystemExit("No postable payroll week") + mon, sun = week_pair + single = EmployeeLeave( + leave_type_id=str(sick_item.xero_id), + description="KAN-326 verifier: single-period custom units", + start_date=mon, + end_date=mon + timedelta(days=3), + periods=[ + LeavePeriod( + period_start_date=mon, + period_end_date=sun, + number_of_units=28.5, + period_status="Approved", + ) + ], + ) + try: + resp = payroll_api.update_employee_leave( + xero_tenant_id=tenant_id, + employee_id=employee_id, + leave_id=args.probe_single_period, + employee_leave=single, + ) + show("PROBE: single-period update SUCCEEDED during draft", "") + show_leave(resp.leave) + except ApiException as exc: + show("PROBE: single-period update REJECTED during draft", "") + print(api_error_body(exc)) + time.sleep(PAUSE) + echo = payroll_api.get_employee_leaves( + xero_tenant_id=tenant_id, employee_id=employee_id + ) + ours = [ + lv + for lv in (echo.leave or []) + if str(lv.leave_id) == args.probe_single_period + ] + show("PROBE: echo after single-period update", "") + if ours: + show_leave(ours[0]) + else: + print("leave not found in echo") + return + + if args.probe_create_single: + annual = XeroPayItem.objects.get(name="Annual Leave", uses_leave_api=True) + week_pair = next_postable_payroll_week() + if week_pair is None: + raise SystemExit("No postable payroll week") + # One week AFTER the draft week: no draft pay run covers it. + mon = week_pair[0] + timedelta(days=7) + sun = mon + timedelta(days=6) + lumped = EmployeeLeave( + leave_type_id=str(annual.xero_id), + description="KAN-326 verifier: create single lumped period", + start_date=mon, + end_date=mon + timedelta(days=2), + periods=[ + LeavePeriod( + period_start_date=mon, + period_end_date=sun, + number_of_units=10.5, + period_status="Approved", + ) + ], + ) + try: + resp = payroll_api.create_employee_leave( + xero_tenant_id=tenant_id, + employee_id=employee_id, + employee_leave=lumped, + ) + show("PROBE: create with single lumped period (10.5 units) response", "") + show_leave(resp.leave) + except ApiException as exc: + show("PROBE: create with single lumped period REJECTED", "") + print(api_error_body(exc)) + return + probe_leave_id = str(resp.leave.leave_id) + time.sleep(PAUSE) + + respan = EmployeeLeave( + leave_type_id=str(annual.xero_id), + description="KAN-326 verifier: respan +1 day", + start_date=mon, + end_date=mon + timedelta(days=3), + periods=[ + LeavePeriod( + period_start_date=mon, + period_end_date=sun, + number_of_units=14.0, + period_status="Approved", + ) + ], + ) + try: + resp2 = payroll_api.update_employee_leave( + xero_tenant_id=tenant_id, + employee_id=employee_id, + leave_id=probe_leave_id, + employee_leave=respan, + ) + show("PROBE: update changing date span SUCCEEDED", "") + show_leave(resp2.leave) + except ApiException as exc: + show("PROBE: update changing date span REJECTED", "") + print(api_error_body(exc)) + time.sleep(PAUSE) + + try: + payroll_api.delete_employee_leave( + xero_tenant_id=tenant_id, + employee_id=employee_id, + leave_id=probe_leave_id, + ) + show("PROBE: cleanup delete succeeded (no draft covers that week)", "") + except ApiException as exc: + show( + "PROBE: cleanup delete FAILED - manual cleanup needed for " + f"leave {probe_leave_id}", + "", + ) + print(api_error_body(exc)) + return + + sick = XeroPayItem.objects.get(name="Sick Leave", uses_leave_api=True) + week = next_postable_payroll_week() + if week is None: + raise SystemExit("No postable payroll week (calendar not configured?)") + monday, _sunday = week + print(f"Target week: {monday} (next postable on the configured calendar)") + + # --- Contract 2: create with non-uniform per-day periods ----------------- + days_units = [ + (monday, 8.0), + (monday + timedelta(days=1), 8.0), + (monday + timedelta(days=2), 8.0), + (monday + timedelta(days=3), 4.5), + ] + periods = [ + LeavePeriod( + period_start_date=d, + period_end_date=d, + number_of_units=units, + period_status="Approved", + ) + for d, units in days_units + ] + employee_leave = EmployeeLeave( + leave_type_id=str(sick.xero_id), + description="KAN-326 contract verifier (safe to delete)", + start_date=days_units[0][0], + end_date=days_units[-1][0], + periods=periods, + ) + try: + resp = payroll_api.create_employee_leave( + xero_tenant_id=tenant_id, + employee_id=employee_id, + employee_leave=employee_leave, + ) + except ApiException as exc: + show("CONTRACT 2 FAIL: create rejected non-uniform per-day periods", "") + print(api_error_body(exc)) + raise SystemExit(1) + leave_id = str(resp.leave.leave_id) + show("CONTRACT 2: create accepted; immediate response", "") + show_leave(resp.leave) + time.sleep(PAUSE) + + echo = payroll_api.get_employee_leaves( + xero_tenant_id=tenant_id, employee_id=employee_id + ) + ours = [lv for lv in (echo.leave or []) if str(lv.leave_id) == leave_id] + show("CONTRACT 2: echo from get_employee_leaves", "") + if ours: + show_leave(ours[0]) + else: + print(f"leave {leave_id} NOT found in echo of {len(echo.leave or [])} leaves") + time.sleep(PAUSE) + + # --- Draft pay run covering the employee -------------------------------- + show("Creating draft pay run via ensure_pay_run_for_week", monday) + pay_run = ensure_pay_run_for_week(monday) + pay_run_id = pay_run["pay_run_id"] + print(f"draft pay run: {pay_run_id} status={pay_run['pay_run_status']}") + time.sleep(PAUSE) + + slips = payroll_api.get_pay_slips(xero_tenant_id=tenant_id, pay_run_id=pay_run_id) + in_draft = any(str(s.employee_id) == employee_id for s in (slips.pay_slips or [])) + show( + "Employee included in draft pay run (block precondition)", + f"{in_draft} ({len(slips.pay_slips or [])} slips total)", + ) + if not in_draft: + print( + "WARNING: employee not in draft - block results below are NOT " + "interpretable; fix the calendar assignment and re-run." + ) + time.sleep(PAUSE) + + # --- Contract 1a: update while draft exists ----------------------------- + updated_units = [ + (monday, 8.0), + (monday + timedelta(days=1), 8.0), + (monday + timedelta(days=2), 4.5), + (monday + timedelta(days=3), 4.5), + ] + updated = EmployeeLeave( + leave_type_id=str(sick.xero_id), + description="KAN-326 contract verifier UPDATED (safe to delete)", + start_date=updated_units[0][0], + end_date=updated_units[-1][0], + periods=[ + LeavePeriod( + period_start_date=d, + period_end_date=d, + number_of_units=units, + period_status="Approved", + ) + for d, units in updated_units + ], + ) + try: + upd_resp = payroll_api.update_employee_leave( + xero_tenant_id=tenant_id, + employee_id=employee_id, + leave_id=leave_id, + employee_leave=updated, + ) + show("CONTRACT 1a: update_employee_leave SUCCEEDED during draft", "") + show_leave(upd_resp.leave) + except ApiException as exc: + show("CONTRACT 1a: update_employee_leave REJECTED during draft", "") + print(api_error_body(exc)) + time.sleep(PAUSE) + + # --- Contract 1b: delete while draft exists (expect block) -------------- + try: + payroll_api.delete_employee_leave( + xero_tenant_id=tenant_id, employee_id=employee_id, leave_id=leave_id + ) + show( + "CONTRACT 1b: delete_employee_leave SUCCEEDED during draft " + "(unexpected - no block!)", + "", + ) + print("Test leave already deleted; skip --cleanup for the leave.") + except ApiException as exc: + show("CONTRACT 1b: delete_employee_leave blocked (capture string)", "") + print(api_error_body(exc)) + time.sleep(PAUSE) + + echo2 = payroll_api.get_employee_leaves( + xero_tenant_id=tenant_id, employee_id=employee_id + ) + ours2 = [lv for lv in (echo2.leave or []) if str(lv.leave_id) == leave_id] + show("Final echo of test leave", "") + if ours2: + show_leave(ours2[0]) + else: + print("test leave no longer present in Xero") + + show("CLEANUP CHECKLIST", "") + print( + f"1. In demo-tenant Xero UI: Payroll -> Pay runs -> delete the DRAFT pay " + f"run for week {monday} (id {pay_run_id}). It blocks all dev payroll " + f"posting until removed. API deletion is impossible.\n" + f"2. Run Xero sync (or delete the local XeroPayRun mirror row for " + f"{pay_run_id}) so the mirror matches.\n" + f"3. Re-run this script with --cleanup {leave_id} to delete the test " + f"leave (only works once the draft pay run is gone)." + ) + + +if __name__ == "__main__": + main() From bc27f3e11c5ecaae085d87489b4243b33182f8ae Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 19:25:34 +1200 Subject: [PATCH 2/6] chore: remove the KAN-326 contract verifier now the demo tenant is clean Its captured API contracts are recorded in PR #518 and on the Jira ticket; the test leave and draft pay run it created on the demo tenant have been removed (draft deleted in the Xero UI, leave via --cleanup, stale mirror row purged). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- scripts/verify_kan326_leave_contracts.py | 395 ----------------------- 1 file changed, 395 deletions(-) delete mode 100644 scripts/verify_kan326_leave_contracts.py diff --git a/scripts/verify_kan326_leave_contracts.py b/scripts/verify_kan326_leave_contracts.py deleted file mode 100644 index b0302ca0a..000000000 --- a/scripts/verify_kan326_leave_contracts.py +++ /dev/null @@ -1,395 +0,0 @@ -#!/usr/bin/env python -"""KAN-326 verifier: empirically pin two undocumented Xero NZ Payroll contracts -against the dev demo tenant, BEFORE the leave-reconciliation fix is built. - -Contract 1: is update_employee_leave (PUT) permitted while the employee is in a - draft pay run? (Xero documents the block only for DELETE.) -Contract 2: does create_employee_leave accept explicit non-uniform per-day - periods (8/8/8/4.5), and what shape does Xero echo back? - -Run: .venv/bin/python scripts/verify_kan326_leave_contracts.py -Cleanup: .venv/bin/python scripts/verify_kan326_leave_contracts.py --cleanup - (only AFTER deleting the draft pay run in the Xero UI - see checklist - the main run prints) - -DESTRUCTIVE to the demo tenant: creates a leave record and a draft pay run. -The draft pay run can only be deleted in the Xero UI and blocks all dev -payroll posting on the calendar until it is removed. - -Temporary tooling - delete before the KAN-326 PR merges (outputs recorded in -the PR and Jira). -""" - -import argparse -import os -import sys -import time -from datetime import timedelta - -import django - -os.environ.setdefault("DJANGO_SETTINGS_MODULE", "docketworks.settings") -sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) -django.setup() - -from xero_python.exceptions import ApiException -from xero_python.payrollnz import PayrollNzApi -from xero_python.payrollnz.models import EmployeeLeave, LeavePeriod - -from apps.accounts.models import Staff -from apps.workflow.api.xero.auth import api_client -from apps.workflow.api.xero.payroll import ( - ensure_pay_run_for_week, - get_tenant_id, - next_postable_payroll_week, -) -from apps.workflow.models import XeroPayItem - -PAUSE = 1.5 # be gentle with the daily API quota - - -def show(label: str, obj: object) -> None: - print(f"\n=== {label} ===") - print(obj) - - -def show_leave(leave: EmployeeLeave) -> None: - print( - f"leave_id={leave.leave_id} type={leave.leave_type_id} " - f"start={leave.start_date} end={leave.end_date} desc={leave.description!r}" - ) - for p in leave.periods or []: - print( - f" period {p.period_start_date} -> {p.period_end_date} " - f"units={p.number_of_units} units_taken={p.number_of_units_taken} " - f"status={p.period_status}" - ) - - -def api_error_body(exc: ApiException) -> str: - return f"status={exc.status} reason={exc.reason} body={exc.body}" - - -def main() -> None: - parser = argparse.ArgumentParser() - parser.add_argument("--cleanup", metavar="LEAVE_ID") - parser.add_argument("--employee-name", default="Jack Allen") - parser.add_argument( - "--probe-single-period", - metavar="LEAVE_ID", - help="Follow-up probe: update the given leave with ONE pay-period-wide " - "period carrying custom total units (28.5), while the draft pay run " - "still exists. Tests both update-during-draft and custom-units.", - ) - parser.add_argument( - "--probe-create-single", - action="store_true", - help="Follow-up probe on a DRAFT-FREE week: does create honor custom " - "units in a single lumped period? Can update change the date span? " - "Cleans up after itself (delete works with no draft).", - ) - args = parser.parse_args() - - tenant_id = get_tenant_id() - if not tenant_id: - raise SystemExit("No Xero tenant ID configured") - payroll_api = PayrollNzApi(api_client) - - staff = Staff.objects.filter( - first_name=args.employee_name.split()[0], - last_name=args.employee_name.split()[1], - ).first() - if staff is None or not staff.xero_user_id: - raise SystemExit(f"Staff {args.employee_name!r} with xero_user_id not found") - employee_id = str(staff.xero_user_id) - print(f"Employee: {args.employee_name} ({employee_id})") - - if args.cleanup: - show("CLEANUP: deleting test leave", args.cleanup) - resp = payroll_api.delete_employee_leave( - xero_tenant_id=tenant_id, employee_id=employee_id, leave_id=args.cleanup - ) - print(f"delete response: {resp}") - return - - if args.probe_single_period: - sick_item = XeroPayItem.objects.get(name="Sick Leave", uses_leave_api=True) - week_pair = next_postable_payroll_week() - if week_pair is None: - raise SystemExit("No postable payroll week") - mon, sun = week_pair - single = EmployeeLeave( - leave_type_id=str(sick_item.xero_id), - description="KAN-326 verifier: single-period custom units", - start_date=mon, - end_date=mon + timedelta(days=3), - periods=[ - LeavePeriod( - period_start_date=mon, - period_end_date=sun, - number_of_units=28.5, - period_status="Approved", - ) - ], - ) - try: - resp = payroll_api.update_employee_leave( - xero_tenant_id=tenant_id, - employee_id=employee_id, - leave_id=args.probe_single_period, - employee_leave=single, - ) - show("PROBE: single-period update SUCCEEDED during draft", "") - show_leave(resp.leave) - except ApiException as exc: - show("PROBE: single-period update REJECTED during draft", "") - print(api_error_body(exc)) - time.sleep(PAUSE) - echo = payroll_api.get_employee_leaves( - xero_tenant_id=tenant_id, employee_id=employee_id - ) - ours = [ - lv - for lv in (echo.leave or []) - if str(lv.leave_id) == args.probe_single_period - ] - show("PROBE: echo after single-period update", "") - if ours: - show_leave(ours[0]) - else: - print("leave not found in echo") - return - - if args.probe_create_single: - annual = XeroPayItem.objects.get(name="Annual Leave", uses_leave_api=True) - week_pair = next_postable_payroll_week() - if week_pair is None: - raise SystemExit("No postable payroll week") - # One week AFTER the draft week: no draft pay run covers it. - mon = week_pair[0] + timedelta(days=7) - sun = mon + timedelta(days=6) - lumped = EmployeeLeave( - leave_type_id=str(annual.xero_id), - description="KAN-326 verifier: create single lumped period", - start_date=mon, - end_date=mon + timedelta(days=2), - periods=[ - LeavePeriod( - period_start_date=mon, - period_end_date=sun, - number_of_units=10.5, - period_status="Approved", - ) - ], - ) - try: - resp = payroll_api.create_employee_leave( - xero_tenant_id=tenant_id, - employee_id=employee_id, - employee_leave=lumped, - ) - show("PROBE: create with single lumped period (10.5 units) response", "") - show_leave(resp.leave) - except ApiException as exc: - show("PROBE: create with single lumped period REJECTED", "") - print(api_error_body(exc)) - return - probe_leave_id = str(resp.leave.leave_id) - time.sleep(PAUSE) - - respan = EmployeeLeave( - leave_type_id=str(annual.xero_id), - description="KAN-326 verifier: respan +1 day", - start_date=mon, - end_date=mon + timedelta(days=3), - periods=[ - LeavePeriod( - period_start_date=mon, - period_end_date=sun, - number_of_units=14.0, - period_status="Approved", - ) - ], - ) - try: - resp2 = payroll_api.update_employee_leave( - xero_tenant_id=tenant_id, - employee_id=employee_id, - leave_id=probe_leave_id, - employee_leave=respan, - ) - show("PROBE: update changing date span SUCCEEDED", "") - show_leave(resp2.leave) - except ApiException as exc: - show("PROBE: update changing date span REJECTED", "") - print(api_error_body(exc)) - time.sleep(PAUSE) - - try: - payroll_api.delete_employee_leave( - xero_tenant_id=tenant_id, - employee_id=employee_id, - leave_id=probe_leave_id, - ) - show("PROBE: cleanup delete succeeded (no draft covers that week)", "") - except ApiException as exc: - show( - "PROBE: cleanup delete FAILED - manual cleanup needed for " - f"leave {probe_leave_id}", - "", - ) - print(api_error_body(exc)) - return - - sick = XeroPayItem.objects.get(name="Sick Leave", uses_leave_api=True) - week = next_postable_payroll_week() - if week is None: - raise SystemExit("No postable payroll week (calendar not configured?)") - monday, _sunday = week - print(f"Target week: {monday} (next postable on the configured calendar)") - - # --- Contract 2: create with non-uniform per-day periods ----------------- - days_units = [ - (monday, 8.0), - (monday + timedelta(days=1), 8.0), - (monday + timedelta(days=2), 8.0), - (monday + timedelta(days=3), 4.5), - ] - periods = [ - LeavePeriod( - period_start_date=d, - period_end_date=d, - number_of_units=units, - period_status="Approved", - ) - for d, units in days_units - ] - employee_leave = EmployeeLeave( - leave_type_id=str(sick.xero_id), - description="KAN-326 contract verifier (safe to delete)", - start_date=days_units[0][0], - end_date=days_units[-1][0], - periods=periods, - ) - try: - resp = payroll_api.create_employee_leave( - xero_tenant_id=tenant_id, - employee_id=employee_id, - employee_leave=employee_leave, - ) - except ApiException as exc: - show("CONTRACT 2 FAIL: create rejected non-uniform per-day periods", "") - print(api_error_body(exc)) - raise SystemExit(1) - leave_id = str(resp.leave.leave_id) - show("CONTRACT 2: create accepted; immediate response", "") - show_leave(resp.leave) - time.sleep(PAUSE) - - echo = payroll_api.get_employee_leaves( - xero_tenant_id=tenant_id, employee_id=employee_id - ) - ours = [lv for lv in (echo.leave or []) if str(lv.leave_id) == leave_id] - show("CONTRACT 2: echo from get_employee_leaves", "") - if ours: - show_leave(ours[0]) - else: - print(f"leave {leave_id} NOT found in echo of {len(echo.leave or [])} leaves") - time.sleep(PAUSE) - - # --- Draft pay run covering the employee -------------------------------- - show("Creating draft pay run via ensure_pay_run_for_week", monday) - pay_run = ensure_pay_run_for_week(monday) - pay_run_id = pay_run["pay_run_id"] - print(f"draft pay run: {pay_run_id} status={pay_run['pay_run_status']}") - time.sleep(PAUSE) - - slips = payroll_api.get_pay_slips(xero_tenant_id=tenant_id, pay_run_id=pay_run_id) - in_draft = any(str(s.employee_id) == employee_id for s in (slips.pay_slips or [])) - show( - "Employee included in draft pay run (block precondition)", - f"{in_draft} ({len(slips.pay_slips or [])} slips total)", - ) - if not in_draft: - print( - "WARNING: employee not in draft - block results below are NOT " - "interpretable; fix the calendar assignment and re-run." - ) - time.sleep(PAUSE) - - # --- Contract 1a: update while draft exists ----------------------------- - updated_units = [ - (monday, 8.0), - (monday + timedelta(days=1), 8.0), - (monday + timedelta(days=2), 4.5), - (monday + timedelta(days=3), 4.5), - ] - updated = EmployeeLeave( - leave_type_id=str(sick.xero_id), - description="KAN-326 contract verifier UPDATED (safe to delete)", - start_date=updated_units[0][0], - end_date=updated_units[-1][0], - periods=[ - LeavePeriod( - period_start_date=d, - period_end_date=d, - number_of_units=units, - period_status="Approved", - ) - for d, units in updated_units - ], - ) - try: - upd_resp = payroll_api.update_employee_leave( - xero_tenant_id=tenant_id, - employee_id=employee_id, - leave_id=leave_id, - employee_leave=updated, - ) - show("CONTRACT 1a: update_employee_leave SUCCEEDED during draft", "") - show_leave(upd_resp.leave) - except ApiException as exc: - show("CONTRACT 1a: update_employee_leave REJECTED during draft", "") - print(api_error_body(exc)) - time.sleep(PAUSE) - - # --- Contract 1b: delete while draft exists (expect block) -------------- - try: - payroll_api.delete_employee_leave( - xero_tenant_id=tenant_id, employee_id=employee_id, leave_id=leave_id - ) - show( - "CONTRACT 1b: delete_employee_leave SUCCEEDED during draft " - "(unexpected - no block!)", - "", - ) - print("Test leave already deleted; skip --cleanup for the leave.") - except ApiException as exc: - show("CONTRACT 1b: delete_employee_leave blocked (capture string)", "") - print(api_error_body(exc)) - time.sleep(PAUSE) - - echo2 = payroll_api.get_employee_leaves( - xero_tenant_id=tenant_id, employee_id=employee_id - ) - ours2 = [lv for lv in (echo2.leave or []) if str(lv.leave_id) == leave_id] - show("Final echo of test leave", "") - if ours2: - show_leave(ours2[0]) - else: - print("test leave no longer present in Xero") - - show("CLEANUP CHECKLIST", "") - print( - f"1. In demo-tenant Xero UI: Payroll -> Pay runs -> delete the DRAFT pay " - f"run for week {monday} (id {pay_run_id}). It blocks all dev payroll " - f"posting until removed. API deletion is impossible.\n" - f"2. Run Xero sync (or delete the local XeroPayRun mirror row for " - f"{pay_run_id}) so the mirror matches.\n" - f"3. Re-run this script with --cleanup {leave_id} to delete the test " - f"leave (only works once the draft pay run is gone)." - ) - - -if __name__ == "__main__": - main() From 602f793570a8911cf2d003d458ea1783078dc632 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 19:46:07 +1200 Subject: [PATCH 3/6] fix: manual time-entry commands crashed on the labour_subtype requirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit KAN-230 (db858444) made CostLine.clean() require labour_subtype on time lines but missed the three timesheet backfill commands, all of which hand-roll CostLine creation: create_leave_entries and create_overtime_entries built lines without it (crashing on the first save), and reclassify_overtime_entries dropped it when splitting a line into an OT copy. Worse, create_leave_entries --dry-run reported success for input that would crash, because it returned before any CostLine was even constructed. - Both create commands now build their lines via a module-level builder that sets labour_subtype from the staff default and fails early with a named-staff CommandError when the default is missing. - create_leave_entries --dry-run now performs the real saves inside a transaction and rolls back, so it exercises every model rule, entry_seq assignment, and DB constraint — it can no longer report success for entries a live run would reject. (Validating before save is impossible by design: CostLine.clean() requires entry_seq, which only save() assigns.) - create_overtime_entries validates and builds every CSV row before its existing atomic write; reclassify_overtime_entries copies the source line's labour_subtype. - New tests cover the builders: subtype carried and line saves (the regression that shipped), and the named-staff failure when a staff row has no default subtype. Folded into KAN-326 per user decision - same payroll surface. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- .../commands/create_leave_entries.py | 116 ++++++++------ .../commands/create_overtime_entries.py | 113 +++++++++----- .../commands/reclassify_overtime_entries.py | 1 + .../tests/test_manual_time_entry_commands.py | 145 ++++++++++++++++++ 4 files changed, 292 insertions(+), 83 deletions(-) create mode 100644 apps/timesheet/tests/test_manual_time_entry_commands.py diff --git a/apps/timesheet/management/commands/create_leave_entries.py b/apps/timesheet/management/commands/create_leave_entries.py index fd9e278d6..32f704bc1 100644 --- a/apps/timesheet/management/commands/create_leave_entries.py +++ b/apps/timesheet/management/commands/create_leave_entries.py @@ -13,6 +13,7 @@ from decimal import Decimal from django.core.management.base import BaseCommand, CommandError +from django.db import transaction from apps.accounts.models import Staff from apps.job.models import CostLine, CostSet, Job @@ -105,6 +106,49 @@ } +def build_leave_cost_line( + staff: Staff, + cost_set: CostSet, + job: Job, + leave_type: str, + entry_date: date, + hours: Decimal, +) -> CostLine: + """Build — but not save — one leave CostLine. + + Full model validation runs inside CostLine.save() (which also assigns + entry_seq, so it cannot run earlier); the guard here exists to give the + operator a named-staff error instead of a ValidationError dump. + """ + if staff.default_labour_subtype is None: + raise CommandError( + f"Staff '{staff.get_display_full_name()}' has no " + "default_labour_subtype set" + ) + label = LEAVE_JOB_NAMES[leave_type] + wage = Decimal("0") if leave_type == "unpaid" else staff.base_wage_rate + cost_line = CostLine( + cost_set=cost_set, + kind="time", + desc=f"{label} - {staff.get_display_name()}", + quantity=hours, + unit_cost=wage, + unit_rev=Decimal("0"), + accounting_date=entry_date, + staff=staff, + xero_pay_item=job.default_xero_pay_item, + labour_subtype=staff.default_labour_subtype, + meta={ + "staff_id": str(staff.id), + "date": entry_date.isoformat(), + "is_billable": False, + "created_from_timesheet": True, + "wage_rate_multiplier": 1, + }, + ) + return cost_line + + class Command(BaseCommand): help = "Create missing leave entries to backfill JM from Xero payroll data" @@ -147,7 +191,7 @@ def handle(self, *args, **options): return # --- Validate all entries upfront --- - validated = [] + validated: list[CostLine] = [] for staff_name, entry_date, leave_type, hours in ENTRIES: if leave_type not in leave_cost_sets: raise CommandError( @@ -210,55 +254,41 @@ def handle(self, *args, **options): ) validated.append( - (staff, entry_date, leave_type, hours, cost_set, leave_jobs[leave_type]) + build_leave_cost_line( + staff, + cost_set, + leave_jobs[leave_type], + leave_type, + entry_date, + hours, + ) ) self.stdout.write(f"Validated {len(validated)} entries.") - if dry_run: - self.stdout.write(self.style.WARNING("DRY RUN - no changes made:")) - for staff, entry_date, leave_type, hours, _, _ in validated: - label = LEAVE_JOB_NAMES[leave_type] - wage = Decimal("0") if leave_type == "unpaid" else staff.base_wage_rate - cost = wage * hours + # --- Create entries; a dry run saves them for real (exercising every + # model rule, entry_seq assignment, and DB constraint) then rolls the + # whole transaction back, so it can never report success for entries + # that would fail a live run. --- + with transaction.atomic(): + for cost_line in validated: + cost_line.save() self.stdout.write( - f" {entry_date} ({entry_date.strftime('%a')}) | " - f"{staff.get_display_name()} | {label} | " - f"{hours}h | ${cost}" + self.style.SUCCESS( + f"Created: {cost_line.accounting_date} " + f"({cost_line.accounting_date.strftime('%a')}) | " + f"{cost_line.desc} | {cost_line.quantity}h | " + f"${cost_line.total_cost} | ID: {cost_line.id}" + ) ) - return - - # --- Create entries (all validation passed) --- - for staff, entry_date, leave_type, hours, cost_set, job in validated: - label = LEAVE_JOB_NAMES[leave_type] - - wage = Decimal("0") if leave_type == "unpaid" else staff.base_wage_rate - - cl = CostLine.objects.create( - cost_set=cost_set, - kind="time", - desc=f"{label} - {staff.get_display_name()}", - quantity=hours, - unit_cost=wage, - unit_rev=Decimal("0"), - accounting_date=entry_date, - staff=staff, - xero_pay_item=job.default_xero_pay_item, - meta={ - "staff_id": str(staff.id), - "date": entry_date.isoformat(), - "is_billable": False, - "created_from_timesheet": True, - "wage_rate_multiplier": 1, - }, - ) - self.stdout.write( - self.style.SUCCESS( - f"Created: {entry_date} ({entry_date.strftime('%a')}) | " - f"{staff.get_display_name()} | {label} | {hours}h | " - f"${cl.total_cost} | ID: {cl.id}" + if dry_run: + transaction.set_rollback(True) + self.stdout.write( + self.style.WARNING( + f"DRY RUN - all {len(validated)} entries rolled back." + ) ) - ) + return self.stdout.write( self.style.SUCCESS(f"Done. Created {len(validated)} entries.") diff --git a/apps/timesheet/management/commands/create_overtime_entries.py b/apps/timesheet/management/commands/create_overtime_entries.py index 2c9d26f79..285a05200 100644 --- a/apps/timesheet/management/commands/create_overtime_entries.py +++ b/apps/timesheet/management/commands/create_overtime_entries.py @@ -40,6 +40,50 @@ DESC_PREFIX = "Retrospectively added" + +def build_manual_time_cost_line( + staff: Staff, + cost_set: CostSet, + desc: str, + hours: Decimal, + unit_cost: Decimal, + accounting_date: date, + pay_item: XeroPayItem, + wage_rate_multiplier: float, +) -> CostLine: + """Build — but not save — one manual time CostLine. + + Full model validation runs inside CostLine.save() (which also assigns + entry_seq, so it cannot run earlier); the guard here exists to give the + operator a named-staff error instead of a ValidationError dump. + """ + if staff.default_labour_subtype is None: + raise CommandError( + f"Staff '{staff.get_display_full_name()}' has no " + "default_labour_subtype set" + ) + cost_line = CostLine( + cost_set=cost_set, + kind="time", + desc=desc, + quantity=hours, + unit_cost=unit_cost, + unit_rev=Decimal("0"), + accounting_date=accounting_date, + staff=staff, + xero_pay_item=pay_item, + labour_subtype=staff.default_labour_subtype, + meta={ + "staff_id": str(staff.id), + "date": accounting_date.isoformat(), + "is_billable": False, + "created_from_timesheet": True, + "wage_rate_multiplier": wage_rate_multiplier, + }, + ) + return cost_line + + PREVIEW_CSV_PATH = ( Path(__file__).resolve().parents[4] / "scripts" / "overtime_preview.csv" ) @@ -337,16 +381,37 @@ def _do_apply(self, csv_path: str): f"Row {i}: hours_to_create must be positive, got {hours}" ) + if entry_type == "overtime": + pay_item = ot_pay_item + multiplier = 1.5 + label = "OT" + elif entry_type == "ordinary": + pay_item = ordinary_pay_item + multiplier = 1.0 + label = "ordinary" + else: + leave_type = entry_type.split(":", 1)[1] + pay_item = leave_pay_items[leave_type] + multiplier = float(pay_item.multiplier or 0) + label = leave_type + + cost_line = build_manual_time_cost_line( + staff=staff, + cost_set=cost_set, + desc=f"{DESC_PREFIX} {label} - {staff_name}", + hours=hours, + unit_cost=unit_cost, + accounting_date=accounting_date, + pay_item=pay_item, + wage_rate_multiplier=multiplier, + ) validated.append( { - "staff": staff, + "cost_line": cost_line, "staff_name": staff_name, - "cost_set": cost_set, "job_name": row.get("job_name", "?"), "entry_type": entry_type, - "hours": hours, - "accounting_date": accounting_date, - "unit_cost": unit_cost, + "label": label, "week_start": row["week_start"], } ) @@ -357,48 +422,16 @@ def _do_apply(self, csv_path: str): counts = {"overtime": 0, "ordinary": 0, "leave": 0} with transaction.atomic(): for entry in validated: - staff = entry["staff"] entry_type = entry["entry_type"] - - if entry_type == "overtime": - pay_item = ot_pay_item - multiplier = 1.5 - label = "OT" - elif entry_type == "ordinary": - pay_item = ordinary_pay_item - multiplier = 1.0 - label = "ordinary" - else: - leave_type = entry_type.split(":", 1)[1] - pay_item = leave_pay_items[leave_type] - multiplier = float(pay_item.multiplier or 0) - label = leave_type - - cl = CostLine.objects.create( - cost_set=entry["cost_set"], - kind="time", - desc=f"{DESC_PREFIX} {label} - {entry['staff_name']}", - quantity=entry["hours"], - unit_cost=entry["unit_cost"], - unit_rev=Decimal("0"), - accounting_date=entry["accounting_date"], - staff=staff, - xero_pay_item=pay_item, - meta={ - "staff_id": str(staff.id), - "date": entry["accounting_date"].isoformat(), - "is_billable": False, - "created_from_timesheet": True, - "wage_rate_multiplier": multiplier, - }, - ) + cl = entry["cost_line"] + cl.save() count_key = "leave" if entry_type.startswith("leave:") else entry_type counts[count_key] += 1 self.stdout.write( self.style.SUCCESS( f"Created: {entry['week_start']} | " f"{entry['staff_name']} | " - f"{entry['hours']}h {label} | " + f"{cl.quantity}h {entry['label']} | " f"${cl.total_cost} | " f"job: {entry['job_name']} | ID: {cl.id}" ) diff --git a/apps/timesheet/management/commands/reclassify_overtime_entries.py b/apps/timesheet/management/commands/reclassify_overtime_entries.py index ca68e7731..143812c35 100644 --- a/apps/timesheet/management/commands/reclassify_overtime_entries.py +++ b/apps/timesheet/management/commands/reclassify_overtime_entries.py @@ -443,6 +443,7 @@ def _split_costline( accounting_date=costline.accounting_date, staff=costline.staff, xero_pay_item=ot_pay_item, + labour_subtype=costline.labour_subtype, meta=meta, ) return new_cl diff --git a/apps/timesheet/tests/test_manual_time_entry_commands.py b/apps/timesheet/tests/test_manual_time_entry_commands.py new file mode 100644 index 000000000..6a6fef825 --- /dev/null +++ b/apps/timesheet/tests/test_manual_time_entry_commands.py @@ -0,0 +1,145 @@ +"""Manual time-entry command builders must produce valid time CostLines. + +Regression guard for KAN-326's second defect: the manual backfill commands +(create_leave_entries, create_overtime_entries, reclassify_overtime_entries) +hand-rolled CostLine creation without labour_subtype, which CostLine.clean() +has required for time lines since KAN-230 — so every run crashed on the +first save while --dry-run reported success. The builders under test are +shared by the dry-run and real paths (dry-run now saves inside a rolled-back +transaction), so saving a built line here exercises exactly the validation a +live run hits. +""" + +from datetime import date +from decimal import Decimal + +from django.core.management.base import CommandError +from django.utils import timezone + +from apps.accounts.models import Staff +from apps.company.models import Company +from apps.job.models import CostSet, Job +from apps.testing import BaseTestCase +from apps.timesheet.management.commands.create_leave_entries import ( + build_leave_cost_line, +) +from apps.timesheet.management.commands.create_overtime_entries import ( + build_manual_time_cost_line, +) +from apps.workflow.models import XeroPayItem + + +class ManualTimeEntryBuilderTestCase(BaseTestCase): + def setUp(self) -> None: + self.company = Company.objects.create( + name="Leave Test Company", + email="leave-tests@example.com", + xero_last_modified="2024-01-01T00:00:00Z", + ) + self.staff = Staff.objects.create_user( + email="leave-taker@example.com", + password="testpass", + first_name="Leave", + last_name="Taker", + is_workshop_staff=True, + base_wage_rate=Decimal("30.00"), + ) + self.pay_item = XeroPayItem.objects.create( + xero_id="test-sick-leave-item", + xero_tenant_id="test-tenant", + name="Test Sick Leave", + uses_leave_api=True, + multiplier=Decimal("1.00"), + xero_last_modified=timezone.now(), + ) + self.job = Job( + name="Sick Leave", + company=self.company, + status="special", + default_xero_pay_item=self.pay_item, + ) + self.job.save(staff=Staff.get_automation_user()) + self.cost_set = CostSet.objects.get_or_create( + job=self.job, kind="actual", rev=1, defaults={"summary": {}} + )[0] + + def _clear_default_subtype(self) -> None: + # update() bypasses Staff.save(), which would re-set the default. + Staff.objects.filter(id=self.staff.id).update(default_labour_subtype=None) + self.staff.refresh_from_db() + + +class BuildLeaveCostLineTests(ManualTimeEntryBuilderTestCase): + def test_line_carries_staff_default_subtype_and_saves(self) -> None: + """The exact regression that shipped: leave lines must carry + labour_subtype (from the staff default) and survive the model's + full_clean-on-save, or the command crashes on its first entry.""" + line = build_leave_cost_line( + self.staff, + self.cost_set, + self.job, + "sick", + date(2026, 7, 28), + Decimal("4.500"), + ) + + self.assertIsNotNone(self.staff.default_labour_subtype) + self.assertEqual(line.labour_subtype, self.staff.default_labour_subtype) + line.save() + line.refresh_from_db() + self.assertEqual(line.kind, "time") + self.assertEqual(line.quantity, Decimal("4.500")) + self.assertEqual(line.unit_cost, Decimal("30.00")) + + def test_missing_default_subtype_fails_at_validation_time(self) -> None: + """A staff row without default_labour_subtype must fail while the + command is still validating — i.e. --dry-run catches it — with an + error naming the staff member, not a mid-write crash.""" + self._clear_default_subtype() + + with self.assertRaisesRegex(CommandError, "Leave Taker"): + build_leave_cost_line( + self.staff, + self.cost_set, + self.job, + "sick", + date(2026, 7, 28), + Decimal("8.000"), + ) + + +class BuildManualTimeCostLineTests(ManualTimeEntryBuilderTestCase): + def test_line_carries_staff_default_subtype_and_saves(self) -> None: + """create_overtime_entries shares the same defect class; its builder + must produce lines that pass model validation and save.""" + line = build_manual_time_cost_line( + staff=self.staff, + cost_set=self.cost_set, + desc="Retrospectively added OT - Leave Taker", + hours=Decimal("2.000"), + unit_cost=Decimal("45.00"), + accounting_date=date(2026, 7, 28), + pay_item=self.pay_item, + wage_rate_multiplier=1.5, + ) + + self.assertEqual(line.labour_subtype, self.staff.default_labour_subtype) + self.assertEqual(line.meta["wage_rate_multiplier"], 1.5) + line.save() + line.refresh_from_db() + self.assertEqual(line.quantity, Decimal("2.000")) + + def test_missing_default_subtype_fails_at_validation_time(self) -> None: + self._clear_default_subtype() + + with self.assertRaisesRegex(CommandError, "Leave Taker"): + build_manual_time_cost_line( + staff=self.staff, + cost_set=self.cost_set, + desc="Retrospectively added OT - Leave Taker", + hours=Decimal("2.000"), + unit_cost=Decimal("45.00"), + accounting_date=date(2026, 7, 28), + pay_item=self.pay_item, + wage_rate_multiplier=1.5, + ) From 4c81ddb9c488460cd860247b757d7941e163bd4e Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 19:52:03 +1200 Subject: [PATCH 4/6] types: give CostLine.save its real keyword-only contract Typing the manual-command fix surfaced that CostLine.save/ _save_with_summary_update/_with_sequence_update_fields were untyped, so every typed caller tripped no-untyped-call. Mirror the django-stubs Model.save signature (keyword-only since Django 5) through the override chain, and type create_overtime_entries' validated rows as a TypedDict. Baseline shrinks by 30 lines. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- apps/job/models/costing.py | 45 +++++++++++++++---- .../commands/create_overtime_entries.py | 14 +++++- mypy-baseline.txt | 30 ------------- 3 files changed, 50 insertions(+), 39 deletions(-) diff --git a/apps/job/models/costing.py b/apps/job/models/costing.py index de1218eed..6116b0bb0 100644 --- a/apps/job/models/costing.py +++ b/apps/job/models/costing.py @@ -1,10 +1,12 @@ import logging import uuid +from collections.abc import Iterable from decimal import Decimal from django.core.exceptions import ValidationError from django.db import connection, models, transaction from django.db.models import Q +from django.db.models.base import ModelBase from django.utils import timezone from .costline_validators import ( @@ -386,8 +388,11 @@ def _assign_entry_seq(self) -> None: @staticmethod def _with_sequence_update_fields( - update_fields, *, requires_sequence: bool, staff_newly_set: bool - ): + update_fields: Iterable[str] | None, + *, + requires_sequence: bool, + staff_newly_set: bool, + ) -> set[str] | None: if update_fields is None: return None fields = set(update_fields) @@ -430,7 +435,14 @@ def _update_cost_set_summary(self) -> None: request_job_summary_pdf_refresh() - def save(self, *args, **kwargs): + def save( + self, + *, + force_insert: bool | tuple[ModelBase, ...] = False, + force_update: bool = False, + using: str | None = None, + update_fields: Iterable[str] | None = None, + ) -> None: staff_was_already_set = self.staff_id is not None requires_sequence = self._actual_time_entry_requires_sequence() with transaction.atomic(): @@ -438,15 +450,27 @@ def save(self, *args, **kwargs): staff_newly_set_from_legacy_meta = ( self.staff_id is not None and not staff_was_already_set ) - kwargs["update_fields"] = self._with_sequence_update_fields( - kwargs.get("update_fields"), + update_fields = self._with_sequence_update_fields( + update_fields, requires_sequence=requires_sequence, staff_newly_set=staff_newly_set_from_legacy_meta, ) - self._save_with_summary_update(*args, **kwargs) + self._save_with_summary_update( + force_insert=force_insert, + force_update=force_update, + using=using, + update_fields=update_fields, + ) - def _save_with_summary_update(self, *args, **kwargs): + def _save_with_summary_update( + self, + *, + force_insert: bool | tuple[ModelBase, ...] = False, + force_update: bool = False, + using: str | None = None, + update_fields: Iterable[str] | None = None, + ) -> None: # Fail fast if trying to set revenue on shop jobs job = self.cost_set.job if job.shop_job: @@ -464,7 +488,12 @@ def _save_with_summary_update(self, *args, **kwargs): ) self.full_clean() - super().save(*args, **kwargs) + super().save( + force_insert=force_insert, + force_update=force_update, + using=using, + update_fields=update_fields, + ) self._update_cost_set_summary() def delete(self, *args, **kwargs): diff --git a/apps/timesheet/management/commands/create_overtime_entries.py b/apps/timesheet/management/commands/create_overtime_entries.py index 285a05200..f0fd8beeb 100644 --- a/apps/timesheet/management/commands/create_overtime_entries.py +++ b/apps/timesheet/management/commands/create_overtime_entries.py @@ -21,6 +21,7 @@ from datetime import date, timedelta from decimal import Decimal, InvalidOperation from pathlib import Path +from typing import TypedDict from django.core.management.base import BaseCommand, CommandError from django.db import transaction @@ -41,6 +42,17 @@ DESC_PREFIX = "Retrospectively added" +class _ValidatedEntry(TypedDict): + """One CSV row, validated and built, awaiting the atomic write.""" + + cost_line: CostLine + staff_name: str + job_name: str + entry_type: str + label: str + week_start: str + + def build_manual_time_cost_line( staff: Staff, cost_set: CostSet, @@ -330,7 +342,7 @@ def _do_apply(self, csv_path: str): staff_cache = {} cost_set_cache = {} - validated = [] + validated: list[_ValidatedEntry] = [] for i, row in enumerate(rows, 1): staff_id = row["staff_id"].strip() job_id = row["job_id"].strip() diff --git a/mypy-baseline.txt b/mypy-baseline.txt index 9a0cbf011..5bf2fd9e4 100644 --- a/mypy-baseline.txt +++ b/mypy-baseline.txt @@ -353,9 +353,6 @@ apps/job/mixins.py:0: error: Function is missing a type annotation [no-untyped- apps/job/mixins.py:0: error: Function is missing a type annotation [no-untyped-def] apps/job/mixins.py:0: error: Missing type arguments for generic type "GenericAPIView" [type-arg] apps/job/models/costing.py:0: error: "type[Model]" has no attribute "objects" [attr-defined] -apps/job/models/costing.py:0: error: Call to untyped function "_save_with_summary_update" in typed context [no-untyped-call] -apps/job/models/costing.py:0: error: Function is missing a return type annotation [no-untyped-def] -apps/job/models/costing.py:0: error: Function is missing a type annotation for one or more parameters [no-untyped-def] apps/job/models/job.py:0: error: "Callable[[Any, Any, Any], Any]" has no attribute "__func__" [attr-defined] apps/job/models/job.py:0: error: "Callable[[Any, Any, Any], Any]" has no attribute "__func__" [attr-defined] apps/job/models/job.py:0: error: "Callable[[Any, Any, Any], Any]" has no attribute "__func__" [attr-defined] @@ -733,7 +730,6 @@ apps/job/services/workshop_service.py:0: error: Call to untyped function "_forma apps/job/services/workshop_service.py:0: error: Call to untyped function "_format_time" of "WorkshopTimesheetService" in typed context [no-untyped-call] apps/job/services/workshop_service.py:0: error: Call to untyped function "delete" in typed context [no-untyped-call] apps/job/services/workshop_service.py:0: error: Call to untyped function "get_default_cost_set_summary" in typed context [no-untyped-call] -apps/job/services/workshop_service.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] apps/job/services/workshop_service.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/job/services/workshop_service.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/job/services/workshop_service.py:0: error: Function is missing a return type annotation [no-untyped-def] @@ -765,8 +761,6 @@ apps/job/tests/test_chat_service.py:0: error: Incompatible default for parameter apps/job/tests/test_chat_service.py:0: error: Missing type arguments for generic type "dict" [type-arg] apps/job/tests/test_chat_service.py:0: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True apps/job/tests/test_chat_service.py:0: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase -apps/job/tests/test_costline_schema_validation.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] -apps/job/tests/test_costline_schema_validation.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] apps/job/tests/test_costline_schema_validation.py:0: error: Function is missing a type annotation for one or more parameters [no-untyped-def] apps/job/tests/test_event_deduplication.py:0: error: Call to untyped function "create_safe" of "JobEvent" in typed context [no-untyped-call] apps/job/tests/test_event_deduplication.py:0: error: Call to untyped function "create_safe" of "JobEvent" in typed context [no-untyped-call] @@ -848,7 +842,6 @@ apps/job/views/job_costing_views.py:0: error: Function is missing a type annotat apps/job/views/job_costing_views.py:0: error: Function is missing a type annotation [no-untyped-def] apps/job/views/job_costing_views.py:0: error: Function is missing a type annotation [no-untyped-def] apps/job/views/job_costline_views.py:0: error: Call to untyped function "delete" in typed context [no-untyped-call] -apps/job/views/job_costline_views.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] apps/job/views/job_costline_views.py:0: error: Dict entry 0 has incompatible type "str": "str | _StrPromise"; expected "str": "str" [dict-item] apps/job/views/job_costline_views.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/job/views/job_costline_views.py:0: error: Function is missing a return type annotation [no-untyped-def] @@ -1348,8 +1341,6 @@ apps/purchasing/services/stock_search_service.py:0: error: Function is missing a apps/purchasing/services/stock_search_service.py:0: error: Incompatible return value type (got "ReturnDict[Any, Any]", expected "list[dict[str, Any]]") [return-value] apps/purchasing/services/stock_search_service.py:0: error: Incompatible types in assignment (expression has type "QuerySet[Stock, Stock]", variable has type "list[Stock]") [assignment] apps/purchasing/services/stock_service.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] -apps/purchasing/services/stock_service.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] -apps/purchasing/services/stock_service.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] apps/purchasing/tests/test_serializers.py:0: error: "PurchaseOrder" has no attribute "detail_lines" [attr-defined] apps/purchasing/tests/test_serializers.py:0: error: "PurchaseOrder" has no attribute "detail_lines" [attr-defined] apps/purchasing/tests/test_stock_fts_search.py:0: error: Function is missing a type annotation [no-untyped-def] @@ -1657,34 +1648,13 @@ apps/timesheet/management/commands/create_overtime_entries.py:0: error: Function apps/timesheet/management/commands/create_overtime_entries.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/timesheet/management/commands/create_overtime_entries.py:0: error: Function is missing a type annotation [no-untyped-def] apps/timesheet/management/commands/create_overtime_entries.py:0: error: Function is missing a type annotation [no-untyped-def] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Incompatible type for "accounting_date" of "CostLine" (got "date | Decimal | CostSet | Staff | str | Any", expected "str | date | Combinable") [misc] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Incompatible type for "cost_set" of "CostLine" (got "date | Decimal | CostSet | Staff | str | Any", expected "CostSet | Combinable") [misc] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Incompatible type for "quantity" of "CostLine" (got "date | Decimal | CostSet | Staff | str | Any", expected "str | float | Decimal | Combinable") [misc] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Incompatible type for "unit_cost" of "CostLine" (got "date | Decimal | CostSet | Staff | str | Any", expected "str | float | Decimal | Combinable") [misc] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Incompatible types in assignment (expression has type "date | Decimal | CostSet | Staff | str | Any", variable has type "Staff") [assignment] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Invalid index type "str | Any | date | Decimal | CostSet | Staff" for "dict[str, int]"; expected type "str" [index] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "CostSet" of "date | Decimal | CostSet | Staff | str | Any" has no attribute "isoformat" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "CostSet" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "split" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "CostSet" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "startswith" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "Decimal" of "date | Decimal | CostSet | Staff | str | Any" has no attribute "isoformat" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "Decimal" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "split" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "Decimal" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "startswith" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "Staff" of "date | Decimal | CostSet | Staff | str | Any" has no attribute "isoformat" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "Staff" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "split" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "Staff" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "startswith" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "date" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "split" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "date" of "str | Any | date | Decimal | CostSet | Staff" has no attribute "startswith" [union-attr] -apps/timesheet/management/commands/create_overtime_entries.py:0: error: Item "str" of "date | Decimal | CostSet | Staff | str | Any" has no attribute "isoformat" [union-attr] apps/timesheet/management/commands/create_overtime_entries.py:0: error: Missing type arguments for generic type "dict" [type-arg] apps/timesheet/management/commands/create_special_job.py:0: error: Function is missing a type annotation [no-untyped-def] apps/timesheet/management/commands/create_special_job.py:0: error: Function is missing a type annotation [no-untyped-def] -apps/timesheet/management/commands/reassign_time_entries.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] apps/timesheet/management/commands/reassign_time_entries.py:0: error: Function is missing a type annotation [no-untyped-def] apps/timesheet/management/commands/reassign_time_entries.py:0: error: Function is missing a type annotation [no-untyped-def] apps/timesheet/management/commands/reclassify_overtime_entries.py:0: error: Argument 4 to "_split_costline" of "Command" has incompatible type "Decimal | CostLine | str | Any"; expected "str" [arg-type] apps/timesheet/management/commands/reclassify_overtime_entries.py:0: error: Call to untyped function "_do_preview" in typed context [no-untyped-call] -apps/timesheet/management/commands/reclassify_overtime_entries.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] -apps/timesheet/management/commands/reclassify_overtime_entries.py:0: error: Call to untyped function "save" in typed context [no-untyped-call] apps/timesheet/management/commands/reclassify_overtime_entries.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/timesheet/management/commands/reclassify_overtime_entries.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/timesheet/management/commands/reclassify_overtime_entries.py:0: error: Function is missing a return type annotation [no-untyped-def] From d35512c3ac918425ee8a050da5bd48892a46f6c2 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 20:06:22 +1200 Subject: [PATCH 5/6] fix: address CodeRabbit review - strict paid-units, persist delete failures - _leave_total_units no longer falls back to number_of_units_taken: that is the consumed quantity, not the paid amount the reconciliation key is built on. A period without number_of_units now raises. - The delete_employee_leave handler persists the failure with its business context (operation, employee, leave id) before converting to DraftPayRunBlocksLeaveChange, matching the update path and the converting-handler pattern; persist_app_error idempotency keeps it to one row. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- apps/workflow/api/xero/payroll.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/apps/workflow/api/xero/payroll.py b/apps/workflow/api/xero/payroll.py index e4e7eaca4..3b278329e 100644 --- a/apps/workflow/api/xero/payroll.py +++ b/apps/workflow/api/xero/payroll.py @@ -2251,11 +2251,11 @@ def _leave_total_units(leave: EmployeeLeave) -> Decimal: for period in periods: units = period.number_of_units if units is None: - units = period.number_of_units_taken - if units is None: + # number_of_units_taken is the CONSUMED amount, a different + # quantity — never a substitute for the paid amount we key on. raise ValueError( f"Xero leave {leave.leave_id} period " - f"{period.period_start_date} has no units" + f"{period.period_start_date} has no number_of_units" ) total += Decimal(str(units)) return total.quantize(Decimal("0.001")) @@ -2506,6 +2506,14 @@ def reconcile_leave_for_staff_week( leave_id=str(leave.leave_id), ) except Exception as exc: + persist_app_error( + exc, + additional_context={ + "operation": "delete_employee_leave", + "employee_id": str(employee_id), + "leave_id": str(leave.leave_id), + }, + ) if _is_draft_pay_run_leave_block(exc): raise DraftPayRunBlocksLeaveChange( _draft_block_message( From 22d82266de2f7f2ac5e3672aac715a7cfb422b79 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 20:20:57 +1200 Subject: [PATCH 6/6] fix: scope draft pay-run guidance to the connected tenant Copilot review: _draft_pay_run_summary listed all mirrored drafts, so a stale row from a previously-connected tenant could be named in the operator guidance even though it does not exist in the Xero UI being described. Single-tenant-per-instance makes this mostly theoretical, but current-tenant scoping is the semantically correct query and matches ensure_pay_run_for_week's precedent of scoping mirror reads. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- apps/workflow/api/xero/payroll.py | 7 +++++-- apps/workflow/tests/test_xero_payroll_leave.py | 13 +++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/apps/workflow/api/xero/payroll.py b/apps/workflow/api/xero/payroll.py index 3b278329e..9b74f3b2d 100644 --- a/apps/workflow/api/xero/payroll.py +++ b/apps/workflow/api/xero/payroll.py @@ -2332,10 +2332,13 @@ def _draft_pay_run_summary() -> str: """Name every draft pay run the operator may need to delete in Xero. Xero blocks leave changes for an employee in ANY draft pay run, not just - the week being posted, so enumerate all mirrored drafts. + the week being posted, so enumerate all mirrored drafts — scoped to the + connected tenant, since only its drafts exist in the UI being described. """ drafts = list( - XeroPayRun.objects.filter(pay_run_status="Draft").order_by("period_start_date") + XeroPayRun.objects.filter( + xero_tenant_id=get_tenant_id(), pay_run_status="Draft" + ).order_by("period_start_date") ) if not drafts: return ( diff --git a/apps/workflow/tests/test_xero_payroll_leave.py b/apps/workflow/tests/test_xero_payroll_leave.py index 301ba4bd2..630e348cd 100644 --- a/apps/workflow/tests/test_xero_payroll_leave.py +++ b/apps/workflow/tests/test_xero_payroll_leave.py @@ -227,6 +227,18 @@ def test_blocked_delete_raises_actionable_error( raw_json={}, xero_last_modified=timezone.now(), ) + # A stale draft from a previously-connected tenant must not be named + # in the guidance — it does not exist in the Xero UI being described. + XeroPayRun.objects.create( + xero_id=UUID("99999999-9999-9999-9999-999999999999"), + xero_tenant_id="old-tenant", + period_start_date=date(2025, 1, 6), + period_end_date=date(2025, 1, 12), + payment_date=date(2025, 1, 12), + pay_run_status="Draft", + raw_json={}, + xero_last_modified=timezone.now(), + ) payroll_api = mock_payroll_api_cls.return_value payroll_api.get_employee_leaves.return_value = SimpleNamespace( leave=[_xero_leave("leave-1", date(2026, 7, 28), date(2026, 7, 31), 28.5)] @@ -240,6 +252,7 @@ def test_blocked_delete_raises_actionable_error( self.assertIn("leave-1", message) self.assertIn("Payroll → Pay runs", message) self.assertIn(f"{WEEK_START} to {WEEK_END}", message) + self.assertNotIn("2025-01-06", message) self.assertIsNotNone(ctx.exception.__cause__) # The old recovery path went on to PUT /PayRuns/{id}; nothing may # touch pay runs now.