fix(adjustments): bulk-delete cascades through Loan + Overtime (critical)
Code-review found a data-integrity bug: the bulk-delete endpoint
bypassed the cascade logic that single-row delete_adjustment does
for 'New Loan', 'Advance Payment', and 'Overtime' types.
Without cascade, bulk-deleting a 'New Loan' adjustment would:
- Delete the PayrollAdjustment row
- LEAVE the linked Loan row orphaned in the DB (still shown in
loan reports, still affecting remaining_balance queries)
- LEAVE any scheduled unpaid Loan Repayment adjustments pointing
at the orphaned Loan (they would silently deduct from the
worker's next pay with no visible parent)
Bulk-deleting an 'Overtime' adjustment would leave the worker
stuck in work_log.priced_workers, making price_overtime() treat
them as already-priced even though the money is gone.
Fix: extracted _delete_adjustment_with_cascade(adj) helper that
captures the exact rules from the existing delete_adjustment view
— returns (ok, reason) so both callers can translate the outcome
into their own response shape. Both views now delegate to it.
bulk_delete_adjustments now loops over the selected rows, calls
the helper per-row, and returns JSON including skipped_reasons
(e.g. {'has_paid_repayments': 1} when a Loan with paid repayments
was refused). Also hardened the id-coercion to int so a garbled
POST payload can't crash the queryset with a ValueError.
Two new tests:
- test_bulk_delete_cascades_new_loan — loan row + unpaid repayment
must also be deleted
- test_bulk_delete_skips_loan_with_paid_repayments — refuses to
delete the loan but still processes other rows in the batch
64/64 tests pass (was 62). No API surface change visible to a user
who only uses the happy path — but the audit trail on Loans is
now safe even under bulk delete.
This commit is contained in:
Konrad du Plessis
parent
5f2e6d8c74
commit
4c3e90f2a7
@ -9,7 +9,7 @@ from django.contrib.auth.models import User
|
||||
from django.test import TestCase
|
||||
from django.urls import reverse
|
||||
|
||||
from core.models import Project, Team, Worker, WorkLog, PayrollRecord, PayrollAdjustment
|
||||
from core.models import Project, Team, Worker, WorkLog, PayrollRecord, PayrollAdjustment, Loan
|
||||
from core.views import _build_work_log_payroll_context, _build_report_context
|
||||
|
||||
|
||||
@ -1133,3 +1133,88 @@ class AdjustmentsTabTests(TestCase):
|
||||
self.assertEqual(resp.status_code, 403)
|
||||
# a1 still present
|
||||
self.assertTrue(PayrollAdjustment.objects.filter(id=self.a1.id).exists())
|
||||
|
||||
def test_bulk_delete_cascades_new_loan(self):
|
||||
"""Bulk-deleting a 'New Loan' adjustment must also delete its
|
||||
linked Loan row AND any still-unpaid Loan Repayment adjustments
|
||||
— same cascade as the single-row delete_adjustment view. Without
|
||||
this, the bulk endpoint would orphan Loan rows and leave pending
|
||||
repayments in place."""
|
||||
# Create a Loan + New Loan adjustment + unpaid repayment
|
||||
loan = Loan.objects.create(
|
||||
worker=self.w1,
|
||||
principal_amount=Decimal('1000'),
|
||||
remaining_balance=Decimal('1000'),
|
||||
date=datetime.date(2026, 4, 1),
|
||||
loan_type='loan',
|
||||
)
|
||||
new_loan_adj = PayrollAdjustment.objects.create(
|
||||
worker=self.w1, project=self.proj, type='New Loan',
|
||||
amount=Decimal('1000'), date=datetime.date(2026, 4, 1),
|
||||
description='Test loan', loan=loan,
|
||||
)
|
||||
unpaid_repayment = PayrollAdjustment.objects.create(
|
||||
worker=self.w1, project=self.proj, type='Loan Repayment',
|
||||
amount=Decimal('500'), date=datetime.date(2026, 5, 1),
|
||||
description='First repayment', loan=loan,
|
||||
)
|
||||
|
||||
self._login_admin()
|
||||
resp = self.client.post(
|
||||
reverse('bulk_delete_adjustments'),
|
||||
{'adjustment_ids': [new_loan_adj.id]},
|
||||
)
|
||||
self.assertEqual(resp.status_code, 200)
|
||||
body = resp.json()
|
||||
self.assertEqual(body['deleted'], 1)
|
||||
|
||||
# New Loan adjustment gone
|
||||
self.assertFalse(PayrollAdjustment.objects.filter(id=new_loan_adj.id).exists())
|
||||
# Linked Loan row gone (cascade)
|
||||
self.assertFalse(Loan.objects.filter(id=loan.id).exists())
|
||||
# Unpaid repayment gone (cascade)
|
||||
self.assertFalse(PayrollAdjustment.objects.filter(id=unpaid_repayment.id).exists())
|
||||
|
||||
def test_bulk_delete_skips_loan_with_paid_repayments(self):
|
||||
"""If a 'New Loan' has any paid repayments, bulk-delete must
|
||||
refuse to delete it (would lose audit trail). Other rows in the
|
||||
batch are unaffected."""
|
||||
loan = Loan.objects.create(
|
||||
worker=self.w1,
|
||||
principal_amount=Decimal('1000'),
|
||||
remaining_balance=Decimal('500'),
|
||||
date=datetime.date(2026, 4, 1),
|
||||
loan_type='loan',
|
||||
)
|
||||
new_loan_adj = PayrollAdjustment.objects.create(
|
||||
worker=self.w1, project=self.proj, type='New Loan',
|
||||
amount=Decimal('1000'), date=datetime.date(2026, 4, 1),
|
||||
description='Test loan', loan=loan,
|
||||
)
|
||||
# One PAID repayment
|
||||
pr = PayrollRecord.objects.create(
|
||||
worker=self.w1, date=datetime.date(2026, 5, 1),
|
||||
amount_paid=Decimal('500'),
|
||||
)
|
||||
PayrollAdjustment.objects.create(
|
||||
worker=self.w1, project=self.proj, type='Loan Repayment',
|
||||
amount=Decimal('500'), date=datetime.date(2026, 5, 1),
|
||||
description='Paid', loan=loan, payroll_record=pr,
|
||||
)
|
||||
|
||||
self._login_admin()
|
||||
# Send the New Loan plus a2 (unpaid Bonus) — expect only a2 to delete
|
||||
resp = self.client.post(
|
||||
reverse('bulk_delete_adjustments'),
|
||||
{'adjustment_ids': [new_loan_adj.id, self.a2.id]},
|
||||
)
|
||||
self.assertEqual(resp.status_code, 200)
|
||||
body = resp.json()
|
||||
self.assertEqual(body['deleted'], 1) # only a2
|
||||
self.assertEqual(body['requested'], 2)
|
||||
self.assertEqual(body['skipped_reasons'], {'has_paid_repayments': 1})
|
||||
|
||||
# New Loan survives, Loan survives, a2 gone
|
||||
self.assertTrue(PayrollAdjustment.objects.filter(id=new_loan_adj.id).exists())
|
||||
self.assertTrue(Loan.objects.filter(id=loan.id).exists())
|
||||
self.assertFalse(PayrollAdjustment.objects.filter(id=self.a2.id).exists())
|
||||
|
||||
138
core/views.py
138
core/views.py
@ -3890,6 +3890,63 @@ def edit_adjustment(request, adj_id):
|
||||
# Removes an unpaid adjustment. Handles cascade logic for Loans and Overtime.
|
||||
# =============================================================================
|
||||
|
||||
# =============================================================================
|
||||
# === ADJUSTMENT CASCADE DELETE HELPER ===
|
||||
# Shared by delete_adjustment (single row) and bulk_delete_adjustments (many
|
||||
# rows) so both paths have identical semantics. "New Loan" and "Advance
|
||||
# Payment" each own a linked Loan row that needs teardown; "Overtime" needs
|
||||
# its worker un-priced from the WorkLog. Without this helper, bulk-delete
|
||||
# would orphan Loan rows and leave priced_workers stale.
|
||||
# =============================================================================
|
||||
|
||||
|
||||
def _delete_adjustment_with_cascade(adj):
|
||||
"""Delete one PayrollAdjustment, cascading through its linked objects.
|
||||
|
||||
Returns a tuple `(ok: bool, reason: str or None)`:
|
||||
- `(True, None)` — row deleted successfully (with any cascades done)
|
||||
- `(False, 'paid')` — adjustment already paid; refuse
|
||||
- `(False, 'has_paid_repayments')` — linked Loan has paid repayments;
|
||||
deleting it would lose the repayment audit trail
|
||||
|
||||
Cascade rules:
|
||||
- New Loan / Advance Payment: delete the linked `Loan` row plus any
|
||||
still-unpaid repayment adjustments. If ANY repayment has already
|
||||
been paid, abort (otherwise we'd lose history of money that
|
||||
already moved).
|
||||
- Overtime: remove the worker from work_log.priced_workers so the
|
||||
overtime can be re-priced cleanly later.
|
||||
- Other types: plain delete, no cascade.
|
||||
"""
|
||||
if adj.payroll_record is not None:
|
||||
return False, 'paid'
|
||||
|
||||
adj_type = adj.type
|
||||
|
||||
if adj_type in ('New Loan', 'Advance Payment') and adj.loan:
|
||||
repayment_type = 'Advance Repayment' if adj_type == 'Advance Payment' else 'Loan Repayment'
|
||||
paid_repayments = PayrollAdjustment.objects.filter(
|
||||
loan=adj.loan,
|
||||
type=repayment_type,
|
||||
payroll_record__isnull=False,
|
||||
)
|
||||
if paid_repayments.exists():
|
||||
return False, 'has_paid_repayments'
|
||||
# Delete all still-unpaid repayments, then the Loan itself
|
||||
PayrollAdjustment.objects.filter(
|
||||
loan=adj.loan,
|
||||
type=repayment_type,
|
||||
payroll_record__isnull=True,
|
||||
).delete()
|
||||
adj.loan.delete()
|
||||
elif adj_type == 'Overtime' and adj.work_log:
|
||||
# "Un-price" the overtime — worker can be re-priced cleanly later
|
||||
adj.work_log.priced_workers.remove(adj.worker)
|
||||
|
||||
adj.delete()
|
||||
return True, None
|
||||
|
||||
|
||||
@login_required
|
||||
def delete_adjustment(request, adj_id):
|
||||
if request.method != 'POST':
|
||||
@ -3898,48 +3955,21 @@ def delete_adjustment(request, adj_id):
|
||||
return HttpResponseForbidden("Not authorized.")
|
||||
|
||||
adj = get_object_or_404(PayrollAdjustment, id=adj_id)
|
||||
|
||||
# Can't delete adjustments that have been paid
|
||||
if adj.payroll_record is not None:
|
||||
messages.error(request, 'Cannot delete a paid adjustment.')
|
||||
return redirect('payroll_dashboard')
|
||||
|
||||
adj_type = adj.type
|
||||
worker_name = adj.worker.name
|
||||
|
||||
# === CASCADE DELETE for New Loan and Advance Payment ===
|
||||
# Both create Loan records that need cleanup when deleted.
|
||||
if adj_type in ('New Loan', 'Advance Payment') and adj.loan:
|
||||
# Determine which repayment type to look for
|
||||
repayment_type = 'Advance Repayment' if adj_type == 'Advance Payment' else 'Loan Repayment'
|
||||
|
||||
# Check if any paid repayments exist for this loan/advance
|
||||
paid_repayments = PayrollAdjustment.objects.filter(
|
||||
loan=adj.loan,
|
||||
type=repayment_type,
|
||||
payroll_record__isnull=False,
|
||||
)
|
||||
if paid_repayments.exists():
|
||||
ok, reason = _delete_adjustment_with_cascade(adj)
|
||||
if not ok:
|
||||
if reason == 'paid':
|
||||
messages.error(request, 'Cannot delete a paid adjustment.')
|
||||
elif reason == 'has_paid_repayments':
|
||||
label = 'advance' if adj_type == 'Advance Payment' else 'loan'
|
||||
messages.error(
|
||||
request,
|
||||
f'Cannot delete {label} for {worker_name} — it has paid repayments.'
|
||||
)
|
||||
return redirect('payroll_dashboard')
|
||||
return redirect('payroll_dashboard')
|
||||
|
||||
# Delete all unpaid repayments for this loan/advance, then the loan itself
|
||||
PayrollAdjustment.objects.filter(
|
||||
loan=adj.loan,
|
||||
type=repayment_type,
|
||||
payroll_record__isnull=True,
|
||||
).delete()
|
||||
adj.loan.delete()
|
||||
|
||||
elif adj_type == 'Overtime' and adj.work_log:
|
||||
# "Un-price" the overtime — remove worker from priced_workers M2M
|
||||
adj.work_log.priced_workers.remove(adj.worker)
|
||||
|
||||
adj.delete()
|
||||
messages.success(request, f'{adj_type} adjustment for {worker_name} deleted.')
|
||||
return redirect('payroll_dashboard')
|
||||
|
||||
@ -3956,31 +3986,49 @@ def delete_adjustment(request, adj_id):
|
||||
@login_required
|
||||
@require_POST
|
||||
def bulk_delete_adjustments(request):
|
||||
"""Delete multiple unpaid PayrollAdjustment rows in one DB call.
|
||||
"""Delete multiple unpaid PayrollAdjustment rows with full cascade.
|
||||
|
||||
Body (form-encoded): `adjustment_ids` — repeated once per ID.
|
||||
Returns JSON: `{"deleted": N, "requested": M}`.
|
||||
Returns JSON: `{"deleted": N, "requested": M, "skipped_reasons": {...}}`.
|
||||
Admin-only; supervisors get 403. POST-only; anything else gets 405
|
||||
from @require_POST.
|
||||
|
||||
Cascade: each row is deleted via `_delete_adjustment_with_cascade`
|
||||
(shared with the single-row `delete_adjustment` view) so bulk and
|
||||
single-row have identical semantics. Rows that fail (already paid,
|
||||
or a linked loan with paid repayments) are counted in `skipped_reasons`
|
||||
but don't block the rest of the batch.
|
||||
"""
|
||||
if not is_admin(request.user):
|
||||
return JsonResponse({'error': 'Admin access required'}, status=403)
|
||||
|
||||
ids = request.POST.getlist('adjustment_ids')
|
||||
# Defensive filter: only DELETE rows that are still unpaid.
|
||||
# If a user's browser had stale state showing a paid row as
|
||||
# unpaid, this quietly skips it instead of destroying payroll
|
||||
# history.
|
||||
to_delete = PayrollAdjustment.objects.filter(
|
||||
# Int-coerce and drop non-digit values (defensive against garbled input —
|
||||
# ids are client-generated so any non-digit would crash the queryset).
|
||||
raw_ids = request.POST.getlist('adjustment_ids')
|
||||
ids = [int(v) for v in raw_ids if v.strip().isdigit()]
|
||||
|
||||
# Fetch each adjustment individually — we need the cascade helper to
|
||||
# operate per-row (it deletes the linked Loan / unprices Overtime).
|
||||
# Pre-filtering for .payroll_record__isnull=True is fine as an upfront
|
||||
# short-circuit but the helper double-checks anyway.
|
||||
adjustments = list(PayrollAdjustment.objects.filter(
|
||||
id__in=ids,
|
||||
payroll_record__isnull=True,
|
||||
)
|
||||
deleted_count = to_delete.count()
|
||||
to_delete.delete()
|
||||
).select_related('loan', 'work_log', 'worker'))
|
||||
|
||||
deleted = 0
|
||||
skipped_reasons = {}
|
||||
for adj in adjustments:
|
||||
ok, reason = _delete_adjustment_with_cascade(adj)
|
||||
if ok:
|
||||
deleted += 1
|
||||
else:
|
||||
skipped_reasons[reason] = skipped_reasons.get(reason, 0) + 1
|
||||
|
||||
return JsonResponse({
|
||||
'deleted': deleted_count,
|
||||
'deleted': deleted,
|
||||
'requested': len(ids),
|
||||
'skipped_reasons': skipped_reasons,
|
||||
})
|
||||
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user