KAN-326: leave reconciliation without the impossible pay-run delete - #518
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
📝 WalkthroughWalkthroughChangesXero leave reconciliation
Validated manual time-entry commands
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StaffReconciliation
participant XeroAPI
participant LeavePayload
participant LeaveUpdate
StaffReconciliation->>XeroAPI: read existing employee leave
StaffReconciliation->>LeavePayload: aggregate payroll-week leave
LeavePayload-->>StaffReconciliation: total-unit leave requests
StaffReconciliation->>LeaveUpdate: update overlapping leave
LeaveUpdate->>XeroAPI: persist leave change
StaffReconciliation->>XeroAPI: delete unmatched leave
StaffReconciliation->>XeroAPI: create missing leave
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workflow/api/xero/payroll.py (1)
2502-2518: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPersist the delete failure before converting it.
The
exceptblock at line 2508 handles the exception fromdelete_employee_leavebut never callspersist_app_error(exc). The update path is covered becauseupdate_employee_leave_in_placepersists internally. The delete path is not covered, so blocked and unrelated delete failures produce noAppErrorrow.persist_app_erroris idempotent, so calling it here is safe.Based on learnings, `persist_app_error` is idempotent and resolves prior persistence through the `__cause__` chain, so this does not create duplicate rows.🛠️ Proposed fix
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( "removed", str(leave.leave_id), leave_start_date, leave_end_date, ) ) from exc raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/workflow/api/xero/payroll.py` around lines 2502 - 2518, Update the exception handler around delete_employee_leave to call persist_app_error(exc) before converting draft pay-run leave errors into DraftPayRunBlocksLeaveChange or re-raising unrelated failures. Keep the existing exception conversion and propagation behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/workflow/api/xero/payroll.py`:
- Around line 1629-1639: In the validation errors within the leave date checks,
replace the EN DASH characters in the messages with ASCII hyphens or “to” while
preserving the existing error content and validation behavior.
- Around line 2247-2261: Update the leave-period total calculation to use only
period.number_of_units; remove the number_of_units_taken fallback and retain the
existing ValueError when number_of_units is absent. Keep the Decimal
accumulation and quantization behavior unchanged.
In `@scripts/verify_kan326_leave_contracts.py`:
- Around line 274-283: Update the ApiException handler around
payroll_api.create_employee_leave to raise SystemExit with the caught exception
as its explicit cause, preserving the existing error output and exit behavior.
---
Outside diff comments:
In `@apps/workflow/api/xero/payroll.py`:
- Around line 2502-2518: Update the exception handler around
delete_employee_leave to call persist_app_error(exc) before converting draft
pay-run leave errors into DraftPayRunBlocksLeaveChange or re-raising unrelated
failures. Keep the existing exception conversion and propagation behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0845029-eff2-4e03-bf7f-79dfbf360359
📒 Files selected for processing (5)
apps/workflow/api/xero/__init__.pyapps/workflow/api/xero/payroll.pyapps/workflow/tests/test_xero_payroll_leave.pymypy-baseline.txtscripts/verify_kan326_leave_contracts.py
💤 Files with no reviewable changes (1)
- mypy-baseline.txt
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
…ment KAN-230 (db85844) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
…ilures - _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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
Owner
Author
|
Re the outside-diff comment (persist the delete failure before converting): applied in d35512c — the handler now persists with operation/employee/leave context before raising DraftPayRunBlocksLeaveChange, matching the update path. Note the original claim wasn't quite right: the SSE boundary already persisted these via the cause walk, so no rows were being lost — the change adds business context at the right layer per the converting-handler pattern. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes KAN-326 by making Xero NZ Payroll leave reconciliation match the only data shape Xero round-trips (weekly total units), removing the impossible draft pay-run auto-delete path, and hardening manual time-entry backfill commands to always construct valid CostLine time entries (including required labour_subtype), even in --dry-run.
Changes:
- Reworked leave differ/builder to key by
(type, span, total units), update overlapping stale leave in place, and surface actionable “delete draft pay run in Xero UI” guidance when deletions are blocked. - Removed dead / impossible pay-run deletion and related retry recursion; rebuilt tests to exercise real
xero_pythonmodel validation. - Fixed manual timesheet backfill commands by centralizing CostLine-building (sets
labour_subtype, fails early when missing) and making--dry-runexecute real saves inside a rolled-back transaction.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mypy-baseline.txt | Shrinks the mypy baseline to reflect newly-typed/cleaned code paths. |
| apps/workflow/tests/test_xero_payroll_leave.py | Replaces mocked leave/payload tests with real xero_python model-based tests covering lumped totals, update-in-place, and actionable draft-block errors. |
| apps/workflow/api/xero/payroll.py | Implements new leave request keying/building, update-in-place reconciliation, actionable error surfacing, and removes the impossible draft pay-run delete path. |
| apps/workflow/api/xero/init.py | Updates generated re-exports for the renamed exception and new payroll helpers/types. |
| apps/timesheet/tests/test_manual_time_entry_commands.py | Adds regression tests ensuring manual command builders produce savable time CostLines with labour_subtype. |
| apps/timesheet/management/commands/reclassify_overtime_entries.py | Preserves labour_subtype when splitting time lines into overtime copies. |
| apps/timesheet/management/commands/create_overtime_entries.py | Introduces a shared builder for manual time lines (sets subtype, typed validated rows) and saves validated CostLines atomically. |
| apps/timesheet/management/commands/create_leave_entries.py | Uses a shared leave CostLine builder and makes --dry-run perform real saves inside a rolled-back transaction. |
| apps/job/models/costing.py | Tightens typing of CostLine.save() / _save_with_summary_update() to match Django’s keyword-arg save contract and reduce mypy debt. |
Comment on lines
+2337
to
+2339
| drafts = list( | ||
| XeroPayRun.objects.filter(pay_run_status="Draft").order_by("period_start_date") | ||
| ) |
Owner
Author
There was a problem hiding this comment.
Applied in 22d8226, with a caveat on the premise: both dev and prod hold exactly one tenant's rows today (prod is single-tenant by construction), so the scenario needs a tenant re-link that leaves a stale Draft row behind. Taken anyway because current-tenant scoping is the semantically correct query for guidance describing the connected tenant's UI — matching ensure_pay_run_for_week's precedent — and the test now pins that a foreign-tenant draft is never named.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/timesheet/management/commands/create_leave_entries.py (1)
109-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared staff-labour-subtype guard and CostLine construction into one helper. Both builders independently re-implement the same
staff.default_labour_subtype is Noneguard and the same coreCostLine(...)field set (cost_set,kind="time",quantity,unit_cost,unit_rev,accounting_date,staff,xero_pay_item,labour_subtype, and a four-keymetadict). This is the exact validation the PR is fixing; keeping two copies risks one being updated without the other the next time the required field set changes.
apps/timesheet/management/commands/create_leave_entries.py#L109-L151: keepbuild_leave_cost_line's leave-specific parameters (job,leave_type) but delegate the guard clause and the sharedCostLine(...)fields to one common helper.apps/timesheet/management/commands/create_overtime_entries.py#L56-L96: havebuild_manual_time_cost_linecall the same shared helper for the guard clause and sharedCostLine(...)fields, keeping only its owndesc/wage_rate_multiplierhandling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/timesheet/management/commands/create_leave_entries.py` around lines 109 - 151, Extract the shared staff.default_labour_subtype guard and common CostLine construction into one reusable helper. In apps/timesheet/management/commands/create_leave_entries.py lines 109-151, update build_leave_cost_line to retain leave-specific job and leave_type handling while delegating shared fields to that helper; in apps/timesheet/management/commands/create_overtime_entries.py lines 56-96, update build_manual_time_cost_line to use the same helper while retaining its own description and wage_rate_multiplier handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/timesheet/tests/test_manual_time_entry_commands.py`:
- Around line 39-46: Suppress Ruff S106 for the test fixture password by adding
an inline noqa marker to the password assignment in the
Staff.objects.create_user call, or replace the literal with a named test
constant.
---
Nitpick comments:
In `@apps/timesheet/management/commands/create_leave_entries.py`:
- Around line 109-151: Extract the shared staff.default_labour_subtype guard and
common CostLine construction into one reusable helper. In
apps/timesheet/management/commands/create_leave_entries.py lines 109-151, update
build_leave_cost_line to retain leave-specific job and leave_type handling while
delegating shared fields to that helper; in
apps/timesheet/management/commands/create_overtime_entries.py lines 56-96,
update build_manual_time_cost_line to use the same helper while retaining its
own description and wage_rate_multiplier handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 850011fe-2317-4bff-94d8-55ff34b6c31e
📒 Files selected for processing (8)
apps/job/models/costing.pyapps/timesheet/management/commands/create_leave_entries.pyapps/timesheet/management/commands/create_overtime_entries.pyapps/timesheet/management/commands/reclassify_overtime_entries.pyapps/timesheet/tests/test_manual_time_entry_commands.pyapps/workflow/api/xero/payroll.pyapps/workflow/tests/test_xero_payroll_leave.pymypy-baseline.txt
💤 Files with no reviewable changes (1)
- mypy-baseline.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/workflow/tests/test_xero_payroll_leave.py
- apps/workflow/api/xero/payroll.py
Comment on lines
+39
to
+46
| 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"), | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Silence the Ruff hardcoded-password hint on the test fixture.
Static analysis flags password="testpass" as a possible hardcoded password (S106). This is a test-only value, but if S106 is enforced on this path, it will fail CI lint. Add a # noqa: S106 on the line, or move the literal into a named test constant.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 41-41: Possible hardcoded password assigned to argument: "password"
(S106)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/timesheet/tests/test_manual_time_entry_commands.py` around lines 39 -
46, Suppress Ruff S106 for the test fixture password by adding an inline noqa
marker to the password assignment in the Staff.objects.create_user call, or
replace the literal with a named test constant.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prod incident
Posting payroll crashed every run with
Invalid value for pay_run_status (Deleted), must be one of ['Draft', 'Posted', 'None']once one employee's leave stopped matching (KAN-326).Three-link chain, all fixed here:
delete_same_week_draft_pay_runPUTPayRun(status="Deleted"), rejected client-side by the SDK; and/PayRuns/{PayRunID}is GET-only in the NZ Payroll API (per Xero's OpenAPI spec) — draft pay run deletion is Xero-UI-only in every region. Born broken in 72c47f3; its test mocked out the crashing function.The fix
(type, span, total units)— the only representation Xero round-trips. Builder emits one request per contiguous run of days, summing same-day entries.DraftPayRunBlocksLeaveChangesurfaces an actionable Payroll Error naming the staff member and the draft pay run(s) to delete in the Xero UI (enumerating all mirrored drafts — the block is per employee-in-any-draft, not same-week).delete_same_week_draft_pay_run,_update_pay_run,_find_same_week_draft_pay_run,_pay_run_payload_from_object, the retry recursion) and dead code (_post_leave_entries,_delete_existing_leave_for_week) are removed outright (ADR 0017).Verification
scripts/verify_kan326_leave_contracts.pypinned the undocumented API contracts against the dev demo tenant before the fix was built (create-discards-per-day-units, single-period create honoured, update-during-draft works, delete-during-draft blocked with the exact prod string). It stays until the demo tenant is cleaned up (draft pay run 54039178 needs UI deletion first), then gets removed before merge.xero_pythonmodels so client-side SDK validation is actually exercised (the old suite mocked out the very function that crashed): differ/builder edge cases, the prod-shape regression case, update-in-place payload assertions, actionable-error content, orchestrator enrichment without retry.Prod unblock (manual, separate)
Delete draft pay run
17d7ca66(week of 27 Jul) in the prod Xero UI, then re-run payroll posting.🤖 Generated with Claude Code
https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
Summary by CodeRabbit
New Features
Bug Fixes
Addendum: manual time-entry commands crashed on the labour_subtype requirement
Folded into KAN-326 (same payroll surface). KAN-230 (
db858444) madeCostLine.clean()requirelabour_subtypeon time lines but missed all three timesheet backfill commands:create_leave_entriesandcreate_overtime_entrieshand-rolled CostLines without it (crash on the first save), andreclassify_overtime_entriesdropped it when splitting a line into an OT copy.create_leave_entries --dry-runreported success for input that would crash — it returned before any CostLine was constructed.labour_subtypefrom the staff default, failing early with a named-staffCommandErrorwhen unset.create_leave_entries --dry-runperforms the real saves inside a rolled-back transaction — it exercises every model rule,entry_seqassignment, and DB constraint, so it can never again report success for entries a live run would reject. (Pre-save validation is impossible by design:CostLine.clean()requiresentry_seq, which onlysave()assigns.)reclassify_overtime_entriescopies the source line's subtype (all 18k existing time lines carry one — KAN-230 backfilled).CostLine.save()override chain got its real keyword-only typed signature (mirrors django-stubs), and the overtime command's validated rows are a TypedDict. mypy baseline shrinks 39 lines across the PR.