fix: restore coupon usage when invoices are cancelled

[my-dev-jour]

Summary

• add a Sales Invoice on_cancel hook to restore POS coupon usage after successful cancellation
• keep credit JE cleanup on before_cancel and realtime updates on on_cancel
• add regression tests for coupon rollback hook behavior

Testing

• python3 -m py_compile pos_next/api/sales_invoice_hooks.py pos_next/hooks.py pos_next/api/test_sales_invoice_hooks.py
• full Frappe unit tests were not run in this workspace because the frappe module is not installed here

Closes #195

[engahmed1190]

Code review

Found 1 issue:

1. rollback_coupon_usage has no is_return guard -- cancelling a return (credit note) invoice will decrement the coupon usage counter even though no usage was ever incremented for the return. Return invoices inherit coupon_code from the original invoice via ERPNext's make_sales_return. This can drive the used counter below its correct value, allowing single-use coupons to be reused. Add an early return for doc.get("is_return").

POSNext/pos_next/api/sales_invoice_hooks.py

Lines 158 to 167 in 1e9b0c3

	def rollback_coupon_usage(doc):
	"""Restore coupon usage for cancelled invoices that consumed a POS coupon."""
	coupon_code = doc.get("coupon_code")
	if not coupon_code or not frappe.db.table_exists("POS Coupon"):
	return
	try:
	from pos_next.pos_next.doctype.pos_coupon.pos_coupon import decrement_coupon_usage
	decrement_coupon_usage(coupon_code)

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[my-dev-jour]

Addressed. I added an early return for doc.get("is_return") in rollback_coupon_usage() so return/credit-note cancellations do not decrement coupon usage. I also added regression coverage for both the return-invoice path and the missing-coupon-table path.

[engahmed1190]

Code Review — REQUEST CHANGES ❌

Summary

Adds on_cancel hook for Sales Invoice to restore POS coupon usage via decrement_coupon_usage. Good edge case handling for return invoices and missing coupon table.

Test Results

4/5 tests pass, 1 fails. test_rollback_coupon_usage_skips_when_coupon_table_missing fails because the frappe.db.table_exists mock doesn't properly intercept the Werkzeug LocalProxy when running under bench execute.

Correctness

Logic is correct — on_cancel delegates to rollback_coupon_usage, which:

• Skips return invoices (correct — returns copy coupon_code but shouldn't affect usage)
• Skips when coupon table doesn't exist (graceful degradation)
• Wraps decrement_coupon_usage in try/except to not block cancellation

hooks.py change: Correctly converts on_cancel from string to list. Good.

Issue: frappe.db.commit() in decrement_coupon_usage (BLOCKING)

decrement_coupon_usage internally calls frappe.db.commit(). When called from an on_cancel hook, this commits the coupon decrement mid-transaction. If subsequent hooks or Frappe's own cancel logic fails:

• The coupon usage count is already decremented (committed)
• But the invoice cancellation rolls back
• Result: coupon usage is decremented without a corresponding cancellation — data inconsistency

This is the same issue PR #207 fixes for the increment path. The decrement path needs the same treatment.

Required Fix

Remove frappe.db.commit() from decrement_coupon_usage, or create an atomic version analogous to consume_coupon_usage from PR #207. The coupon decrement must participate in the same transaction as the invoice cancellation.

Additional Suggestions (non-blocking)

1. Idempotency: No guard against double-decrement if cancel is retried. decrement_coupon_usage checks coupon.used > 0 before decrementing (floor at zero), which is acceptable but not ideal
2. Missing test: No test verifying behavior when decrement_coupon_usage raises an exception (the try/except path). Assert frappe.log_error is called
3. Locking: No SELECT FOR UPDATE on the decrement path — two simultaneous cancellations using the same coupon could race. Low risk (cancellations are rare) but worth noting for symmetry with #207
4. Fix the test failure: Mock frappe.db at frappe.local.db level or restructure to avoid Werkzeug LocalProxy issues

Verdict

The overall approach is correct. Remove the frappe.db.commit() from decrement_coupon_usage to maintain transactional integrity with the document's cancel operation, then this is ready to merge.