PN17-credit return to wallet and create wallet after insert customer

[MohamedAliSmk]

No description provided.

[engahmed1190]

Code Review — PR #144: Credit Return to Wallet + Auto-Create Wallet on Customer Insert

The PR has three changes: (1) fix payment.account dot-notation, (2) credit return amount to customer wallet, (3) auto-create wallet on customer insert. The intent is good but there are critical logic issues that need fixing before merge.

---

🔴 CRITICAL — Double credit risk: wallet reversal + refund credit both fire on the same return

In submit_invoice (invoices.py), both blocks run sequentially inside the same if is_return check:

if invoice_doc.get("is_return") and invoice_doc.get("return_against"):
    # Block 1: ALWAYS runs — reverses wallet transactions on the ORIGINAL invoice
    reverse_wallet_transactions_for_return(...)

    # Block 2 (NEW): runs when payments list is empty
    add_to_customer_balance = not invoice.get("payments")
    if add_to_customer_balance:
        credit_return_to_wallet(...)

Scenario: Original invoice had a loyalty-to-wallet credit. Customer returns with "Add to Customer Credit Balance." What happens:

1. reverse_wallet_transactions_for_return fires — cancels the loyalty wallet credit (correct, undoes loyalty earned)
2. credit_return_to_wallet fires — creates a new Credit for abs(grand_total) (the refund)

If step 1 fails (it's in a try/except that swallows the error and continues), step 2 still runs. The customer gets the full refund credit without the loyalty reversal — a silent money leak.

Fix: Make credit_return_to_wallet conditional on the success of the reversal, or combine them in a single transaction.

---

🔴 CRITICAL — NameError crash in create_wallet_on_customer_insert

def create_wallet_on_customer_insert(doc, method=None):
    try:
        pos_profiles = get_pos_profiles()
        company = pos_profiles[0].company        # IndexError if empty list
        get_or_create_wallet(doc.name, company)
    except Exception:
        frappe.log_error(
            f"Failed to auto-create wallet for customer {doc.name} in company {company}",
            #                                                              ^^^^^^^^
            # company was NEVER assigned if pos_profiles[0] threw IndexError
        )

If get_pos_profiles() returns an empty list, pos_profiles[0] raises IndexError. The except block references company which was never assigned → secondary NameError. This can either silently swallow both errors or break the Customer after_insert hook, preventing customer creation entirely.

Fix:

def create_wallet_on_customer_insert(doc, method=None):
    company = None
    try:
        pos_profiles = get_pos_profiles()
        if not pos_profiles:
            return
        company = pos_profiles[0].company
        get_or_create_wallet(doc.name, company)
    except Exception:
        frappe.log_error(
            f"Failed to auto-create wallet for customer {doc.name} in company {company}",
            "Wallet Auto-Creation Error"
        )

---

🔴 CRITICAL — Only first POS Profile's company used for wallet creation

company = pos_profiles[0].company

get_pos_profiles() returns all POS Profiles the current user has access to. If there are profiles in multiple companies, only the first one gets a wallet. Worse, this runs as an after_insert hook — the "current user" is whoever creates the customer (could be any cashier). Different cashiers may see different profiles in different order, leading to inconsistent wallet creation across customers.

Fix: Either iterate all unique companies, or accept that this is a single-company feature and document it. At minimum, add a guard for the empty-list case.

---

🔴 HIGH — not invoice.get("payments") is unreliable for detecting "Add to Customer Credit Balance"

add_to_customer_balance = not invoice.get("payments")

This infers intent from the absence of payments. An empty payments array can happen for other reasons:

• Credit sale returns (isOriginalCreditSale) — the frontend also sends empty payments for these, but they should NOT credit to wallet (they reverse the receivable)
• Frontend bugs or network corruption
• Direct API callers

Fix: Send an explicit flag from the frontend: add_to_customer_credit: true. Check that flag instead of inferring from missing payments.

---

🔴 HIGH — Loyalty reversal formula may over-reverse for partial returns

The PR changes the partial return reversal for Loyalty Credit transactions:

# NEW formula:
points_to_reverse = flt(returned_amount / collection_factor)
reverse_amount = flt(points_to_reverse * conversion_factor, 2)

The OLD formula was: reverse_amount = flt(wt.amount * return_ratio, 2) — a proportional share of the original credit.

The new formula computes how many wallet credits the returned amount would earn from scratch, ignoring that:

• Loyalty points are typically earned on net_total or only on eligible items, not grand_total
• If partially returned items are non-loyalty-eligible, the formula still reverses credits for them
• The new formula can produce a reverse_amount larger than the original credit amount (wt.amount)

Example: Original invoice = 1000, loyalty-eligible = 500, earned 50 wallet credit. Customer returns 800 worth of (non-eligible) items. Old: 50 * 0.8 = 40. New: (800 / collection_factor) * conversion_factor could exceed 50.

Fix: Keep the proportional formula as a safe default. If exact recalculation is needed, compute based on the loyalty-eligible amount of the returned items specifically.

---

🟡 MEDIUM — @frappe.whitelist() on credit_return_to_wallet is a security risk

@frappe.whitelist()
def credit_return_to_wallet(return_invoice, amount=None):

This function creates wallet credits with ignore_permissions=True. Exposing it as a whitelisted API endpoint means any logged-in user can call it directly via /api/method/...credit_return_to_wallet with an arbitrary invoice name and amount. This is a privilege escalation / financial fraud vector.

Fix: Remove @frappe.whitelist() — this function is only called server-side from submit_invoice.

---

🟡 MEDIUM — Top-level import creates circular import fragility

from pos_next.pos_next.doctype.wallet.wallet import get_or_create_wallet

This was previously a local import inside credit_loyalty_points_to_wallet (for good reason). Moving it to module level introduces a risk: if wallet.py (doctype) ever imports from wallet_transaction.py, it becomes a circular import failure.

Fix: Keep get_or_create_wallet as a local import inside the functions that use it.

---

🟡 MEDIUM — Mixed indentation (tabs vs spaces)

The WalletTransaction class uses tabs, while standalone functions (credit_return_to_wallet, reverse_wallet_transactions_for_return) use 4-space indentation. This is a pre-existing issue but the PR perpetuates it. Worth standardizing.

---

🟢 MINOR — payment.account fix is cosmetic (but correct)

The change from payment["account"] to payment.account is technically fine — in Frappe, both work for child docs. But dot-notation is the idiomatic pattern and consistent with the submit_invoice path, so this is a good cleanup.

---

🟢 MINOR — Heavy function used to fetch two static fields

lp_details = get_loyalty_program_details_with_points(
    original_doc.customer, loyalty_program=loyalty_program,
    expiry_date=original_doc.posting_date, ...
)
collection_factor = lp_details.collection_factor
conversion_factor = lp_details.conversion_factor

collection_factor and conversion_factor are static properties of the loyalty program. A simple frappe.db.get_value("Loyalty Program", loyalty_program, ["collection_factor", "conversion_factor"]) would be lighter than computing full point balances.

---

🟢 MINOR — Error handling allows inconsistent wallet state

Both wallet operations (reverse_wallet_transactions_for_return and credit_return_to_wallet) are in try/except blocks that log errors but let the return invoice submit succeed. This is an acceptable trade-off, but the user sees an alert and has no clear remediation path. Consider creating a system TODO/task for manual follow-up.

---

Summary

Priority	Count	Issues
🔴 CRITICAL/HIGH	5	Double credit risk, NameError crash, single-company wallet, unreliable signal, over-reversal
🟡 MEDIUM	3	Whitelist security, circular import, indentation
🟢 MINOR	3	Cosmetic fix, heavy function, error handling

The core features (credit-to-wallet on return, auto-create wallet) are useful additions. The main blockers are the double-credit race condition, the crash in create_wallet_on_customer_insert, and the unreliable detection of "add to customer credit balance." These need to be fixed before merge.

[engahmed1190]

Code Review — Request Changes

CRITICAL: Duplicate "Customer" key in hooks.py

This is a showstopper. The diff adds a second "Customer" key to the doc_events dict:

"Customer": {
    "after_insert": [
        "pos_next.api.customers.auto_assign_loyalty_program",
        "pos_next.realtime_events.emit_customer_event"
    ],
    "on_update": "pos_next.realtime_events.emit_customer_event",
    "on_trash": "pos_next.realtime_events.emit_customer_event"
},
"Customer": {   # DUPLICATE KEY — silently overwrites the one above
    "after_insert": "pos_next.api.wallet.create_wallet_on_customer_insert"
},

In Python, duplicate dict keys silently overwrite — the second one wins. This means:

• auto_assign_loyalty_program — GONE
• emit_customer_event on insert/update/trash — ALL GONE
• Only create_wallet_on_customer_insert survives

Fix: Merge into a single "Customer" entry:

"Customer": {
    "after_insert": [
        "pos_next.api.customers.auto_assign_loyalty_program",
        "pos_next.realtime_events.emit_customer_event",
        "pos_next.api.wallet.create_wallet_on_customer_insert"
    ],
    "on_update": "pos_next.realtime_events.emit_customer_event",
    "on_trash": "pos_next.realtime_events.emit_customer_event"
},

---

CRITICAL: Error handling removed from submit_invoice (invoices.py)

The try/except around reverse_wallet_transactions_for_return was removed. Previously, a wallet reversal failure would log the error and show a warning but still allow the return invoice to succeed. Now, any exception in wallet reversal crashes the entire return submission and rolls it back.

A wallet issue should never block a return. The original defensive try/except pattern must be restored.

---

HIGH: No duplicate protection in credit_return_to_wallet

If submit_invoice is called twice (retries, network issues, offline sync), credit_return_to_wallet will create duplicate wallet credits for the same return invoice. There is no check for an existing transaction.

Add before creating the transaction:

existing = frappe.db.exists("Wallet Transaction", {
    "reference_name": return_invoice,
    "transaction_type": "Credit",
    "source_type": "Refund",
    "docstatus": 1
})
if existing:
    return None

---

HIGH: payment.account may fail on plain dict (invoices.py line 664)

The change from payment["account"] to payment.account assumes payment supports attribute access. But payments come from data.get("payments") which is parsed JSON — these may be plain dict objects where .account raises AttributeError. Verify whether these are Frappe _dict or plain dict before making this change.

---

MEDIUM: create_wallet_on_customer_insert design issues (wallet.py)

1. Fragile dependency on POS Opening Shift: Looks up the current user's open POS shift to find the company. If a customer is created from the desk (no open shift), by a different user, or when multiple companies exist — no wallet or wrong wallet.

2. Unbound variable in except block: The company variable is only assigned after the if not pos_profile: return guard. If an exception occurs before that line, the except block references company which doesn't exist yet, causing a NameError that masks the real error.

---

MEDIUM: Error handling removed from partial return debit creation

The original code had try/except around partial return debit wallet transaction creation. The new code removed it. If reverse_wt.submit() fails, the return submission crashes. Restore the try/except.

---

LOW: Unused import in wallet_transaction.py

from erpnext.accounts.doctype.loyalty_program.loyalty_program import get_loyalty_program_details_with_points

This is imported but never used in the code. Remove it.

---

LOW: Naming / readability

• Typo: inoviced_amount_after_return → invoiced_amount_after_return
• is_not_loyalty_eligible is a double-negative — hard to read. Better: loyalty_eligible = invoiced_amount_after_return >= min_spent

---

IMPORTANT: Commit hygiene — squash before merge

This PR has 11 commits for what should be 2-3 logical changes. Commit messages include syntax error fixes, conflict resolutions, and incremental debugging. The git author name is also showing a PAT token instead of a real name.

Please squash into clean, meaningful commits before merging (e.g., one for wallet auto-creation on customer insert, one for credit-return-to-wallet feature, one for the min_spent loyalty fix). This keeps the git history clean and makes future debugging with git bisect / git blame much easier.

---

Summary

Severity	Issue
CRITICAL	Duplicate "Customer" key in hooks.py kills loyalty + realtime events
CRITICAL	Removed try/except in submit_invoice — wallet failure blocks return submission
HIGH	No duplicate protection in credit_return_to_wallet
HIGH	payment.account may fail on plain dict
MEDIUM	create_wallet_on_customer_insert relies on current user's POS shift
MEDIUM	Unbound company variable in except block
MEDIUM	Error handling removed from partial return debit creation
LOW	Unused import, typo, double-negative variable name
IMPORTANT	Squash 11 commits into clean logical commits before merge

[engahmed1190]

HIGH: payment.account may fail on plain dict.

Payments come from data.get("payments") which is parsed JSON. Depending on whether Frappe wraps these as _dict or they remain plain dict, one access style may fail.

Fix — use both styles with a fallback so it works regardless of type:

payment["account"] = account_info.get("account") or getattr(account_info, "account", None)

This way if dict-style returns nothing, attribute access is tried as fallback (and vice versa). Covers both dict and _dict / doc objects safely.

[MohamedAliSmk]

Traceback (most recent call last):
File "apps/pos_next/pos_next/api/invoices.py", line 1233, in submit_invoice
payment["account"] = account_info.get("account") or getattr(account_info, "account", None)
TypeError: 'SalesInvoicePayment' object does not support item assignment

[MohamedAliSmk]

payment is not a dict there — it’s a Frappe child DocType row (SalesInvoicePayment), i.e. a document object.
So this works for dicts:
payment["account"] = ...
but fails for doc objects, giving:
TypeError: 'SalesInvoicePayment' object does not support item assignment
so we should use payment.account

[MohamedAliSmk]

@engahmed1190
we need to discuss this point with Anas
create_wallet_on_customer_insert relies on current user's POS shift
to find best practice in this special cases

[engahmed1190]

Deep Code Review — PR #144 (Line-by-Line, Math Verified, ERPNext Cross-Referenced)

Every finding below has been verified against the actual Frappe framework source, ERPNext loyalty program source, and the POS Next codebase. No assumptions.

---

Previously Raised Issues — Status

Issue from Reviews #1/#2	Status
Duplicate "Customer" key in hooks.py	FIXED
Removed try/except in submit_invoice	FIXED
No duplicate protection in credit_return_to_wallet	FIXED (idempotency guard added)
payment.account dot-notation	CONFIRMED CORRECT — SalesInvoicePayment does not support [] assignment
create_wallet_on_customer_insert NameError/POS shift	FIXED — now uses Global Defaults.default_company
Commit hygiene	NOT ADDRESSED — 14 commits

---

SHOWSTOPPER

1. transaction.submit(ignore_permissions=True) — TypeError at runtime

File: wallet_transaction.py → credit_return_to_wallet

transaction.flags.ignore_permissions = True
transaction.insert(ignore_permissions=True)
transaction.submit(ignore_permissions=True)    # ← CRASH

Verified in frappe/model/document.py:1094-1097:

@frappe.whitelist()
def submit(self):
    return self._submit()

submit() accepts zero keyword arguments. _submit() also takes zero. Neither forwards kwargs to save(). Searched the entire frappe + erpnext codebase: zero instances of .submit(ignore_permissions=True) exist anywhere. The canonical pattern is:

doc.flags.ignore_permissions = True
doc.submit()  # flags is already set, save() checks it internally

This means credit_return_to_wallet will always crash with TypeError: submit() got an unexpected keyword argument 'ignore_permissions'. The feature is completely non-functional.

Fix: Remove the kwarg:

transaction.flags.ignore_permissions = True
transaction.insert(ignore_permissions=True)
transaction.submit()

---

CRITICAL

2. credit_return_to_wallet call is unprotected — TypeError propagates to user

File: invoices.py → submit_invoice, after line 1290

if invoice_doc.get("is_return") and invoice_doc.get("return_against"):
    try:
        reverse_wallet_transactions_for_return(...)
    except Exception:
        ...  # caught, logged

    # OUTSIDE try/except:
    add_to_customer_balance = invoice.get("add_to_customer_balance")
    if add_to_customer_balance:
        credit_return_to_wallet(...)   # ← TypeError from issue #1 propagates here

At this point invoice_doc.submit() has already committed (ERPNext internally commits after GL entries). The invoice exists permanently in the database. But the TypeError propagates to the outer except Exception: raise block, so the client receives an error response for a transaction that actually succeeded.

Worse: If the wallet reversal in step 1 created debit transactions (partial return), those are committed. But the credit from step 2 crashes, leaving the customer with less wallet balance than they should have.

Fix: Wrap in its own try/except:

if add_to_customer_balance:
    try:
        credit_return_to_wallet(...)
    except Exception as e:
        frappe.log_error(
            title="Wallet Credit on Return Error",
            message=f"Return: {invoice_doc.name}, Error: {e}"
        )
        frappe.msgprint(
            _("Return submitted but wallet credit failed. Please contact administrator."),
            alert=True, indicator="orange"
        )

---

3. min_spent is fundamentally misused — wrong cancellation logic

File: wallet_transaction.py → reverse_wallet_transactions_for_return

min_spent = frappe.db.get_value("Loyalty Program Collection", {"parent": loyalty_program}, "min_spent")
invoiced_amount_after_return = flt(original_total) - flt(returned_amount)
if min_spent and flt(invoiced_amount_after_return) < flt(min_spent):
    loyalty_eligible = True  # cancel entire wallet transaction

Verified against ERPNext source (erpnext/accounts/doctype/loyalty_program/loyalty_program.py:95-106):

min_spent is a cumulative lifetime spending tier threshold, NOT a per-invoice minimum. ERPNext compares it against total_spent + current_transaction_amount where total_spent is the sum of ALL purchase_amount from the customer's Loyalty Point Entry records across their entire history. The field label in the doctype JSON is "Minimum Total Spent".

ERPNext never uses min_spent to revoke earned points. It only uses it to select which tier's collection_factor applies.

Example of the bug: Customer has 50,000 lifetime spend. Single invoice for 5,000 gets partially returned to 3,000. min_spent for Silver tier = 10,000. The PR's code sees 3,000 < 10,000 and cancels ALL wallet transactions. But the customer clearly exceeds the tier threshold in lifetime spending — the per-invoice amount is irrelevant.

How ERPNext actually handles return loyalty reversal (sales_invoice.py:507-510):

against_si_doc.delete_loyalty_point_entry()
against_si_doc.make_loyalty_point_entry()  # recalculates with returned amount subtracted

Delete-and-recalculate. No min_spent check whatsoever.

Fix: Remove the entire min_spent block. The proportional reversal (wt.amount * return_ratio) already handles partial returns correctly for wallet currency amounts. If you need to handle the edge case where a return drops eligible_amount / collection_factor below 1 point (making the integer-truncated result 0), mirror ERPNext's recalculate approach instead.

---

4. loyalty_eligible flag applied to ALL wallet transaction types

Even if the min_spent logic were correct, it's applied indiscriminately:

for wt in wallet_transactions:
    if is_full_return or loyalty_eligible:
        wt_doc.cancel()  # cancels ALL types: Credit, Loyalty Credit, etc.

A partial return below min_spent would cancel non-loyalty wallet credits too (manual credits, cashback credits). These should only be proportionally debited, not cancelled.

Fix (if min_spent logic is kept): Filter by transaction type:

if is_full_return or (below_min_threshold and wt.transaction_type == "Loyalty Credit"):

---

MEDIUM

5. validate_amount() blocks legitimate reversal debits

Verified by tracing Frappe's insert() → run_before_save_methods() → validate() chain. flags.ignore_permissions only skips permission checks, NOT the validate() hook. validate() is only skipped by flags.ignore_validate which the PR never sets.

validate_amount() (wallet_transaction.py:32-42):

if self.transaction_type == "Debit":
    balance = get_customer_wallet_balance(self.customer, self.company)
    if flt(self.amount) > flt(balance):
        frappe.throw(_("Insufficient wallet balance..."))

And get_customer_wallet_balance() clamps to zero: return available_balance if available_balance > 0 else 0.0.

Scenario: Original invoice credits 100 to wallet. Customer spends 80 on another purchase. Customer returns the original invoice (50% partial). Reversal tries to debit 50. Balance is 20. The debit is blocked with "Insufficient wallet balance." The exception is caught by the except block and logged, but the reversal is silently lost.

Fix: Add a flag to bypass balance validation for reversals:

reverse_wt.flags.skip_balance_validation = True

Then in validate_amount():

if self.transaction_type == "Debit" and not self.flags.get("skip_balance_validation"):

---

6. No idempotency guard on partial return reversals

credit_return_to_wallet has an idempotency guard, but reverse_wallet_transactions_for_return does not.

• Full returns: Safe — cancelled WTs have docstatus=2, filtered out by docstatus: 1.
• Partial returns: Unsafe — original Credit WTs still have docstatus=1 (they're offset by debits, not cancelled). A retry creates duplicate debit transactions.

Fix: Check for existing reversals:

existing = frappe.db.exists("Wallet Transaction", {
    "reference_name": return_invoice, "transaction_type": "Debit", "docstatus": ["!=", 2]
})
if existing:
    return

---

7. credit_return_to_wallet imports the unsafe get_or_create_wallet

Verified: There are two get_or_create_wallet functions:

Location	Checks auto_create_wallet?	Error handling
pos_next.api.wallet	YES — returns None if disabled	try/except, returns None
pos_next.pos_next.doctype.wallet.wallet	NO — always creates	frappe.throw() (raises)

credit_return_to_wallet imports from the doctype version:

from pos_next.pos_next.doctype.wallet.wallet import get_or_create_wallet

This version will frappe.throw() if no wallet account is configured, which propagates up through the unprotected call in submit_invoice (issue #2).

It also bypasses the auto_create_wallet setting, creating wallets even when the admin has disabled automatic wallet creation.

Fix: Import from the API version: from pos_next.api.wallet import get_or_create_wallet

---

8. create_wallet_on_customer_insert — no try/except blocks Customer creation

Verified: Frappe's after_insert hooks run synchronously inside Document.insert(). There is no savepoint or error isolation per-hook. An uncaught exception rolls back the entire Customer creation.

The function calls get_or_create_wallet (the API version, which has internal try/except for the insert() call). But code paths BEFORE that try/except can still throw (DB errors in get_value for POS Profile, POS Settings lookups).

Fix:

def create_wallet_on_customer_insert(doc, method=None):
    company = frappe.get_cached_value("Global Defaults", "Global Defaults", "default_company")
    if not company:
        return
    try:
        get_or_create_wallet(doc.name, company)
    except Exception:
        frappe.log_error(frappe.get_traceback(), f"Wallet auto-create failed for {doc.name}")

---

9. get_or_create_wallet (API version) creates wallets when NO POS Settings exist

Verified by tracing all three scenarios:

if pos_settings and not cint(pos_settings.get("auto_create_wallet")):
    return None

pos_settings	auto_create_wallet	Guard blocks?	Wallet created?
None (no POS Profile/Settings)	N/A	No (short-circuit)	YES — BUG
dict	0 (disabled)	Yes	No
dict	1 (enabled)	No	Yes

When pos_settings is None, the and short-circuits, the guard doesn't fire, and a wallet is created unconditionally. In environments without any POS infrastructure, every new Customer gets a wallet.

Fix: Invert the guard: if not pos_settings or not cint(pos_settings.get("auto_create_wallet")): return None

Note: This fix would also affect create_manual_wallet_credit() (line 426 of wallet.py) which calls get_or_create_wallet without pos_settings. If the guard is fixed, create_manual_wallet_credit would need to either pass a flag or create the wallet directly for the admin use case.

---

10. Idempotency guard catches Draft transactions — stuck draft blocks future credits

existing_transaction_name = frappe.db.get_value(
    "Wallet Transaction",
    {..., "docstatus": ["!=", 2]},  # matches both Draft(0) AND Submitted(1)
    "name",
)
if existing_transaction_name:
    return frappe.get_doc("Wallet Transaction", existing_transaction_name)

If a previous attempt inserted a Draft but crashed before submit (which WILL happen due to issue #1), the idempotency guard returns that Draft forever. No future attempt creates a proper submitted transaction.

Fix: Handle drafts explicitly:

if existing:
    doc = frappe.get_doc("Wallet Transaction", existing)
    if doc.docstatus == 0:  # stuck draft from a crashed attempt
        doc.flags.ignore_permissions = True
        doc.submit()
    return doc

---

LOW

11. Variable name loyalty_eligible is semantically inverted

Set to True when the customer is no longer eligible. Reads as the opposite. Rename to below_min_threshold or should_cancel_loyalty if kept.

12. round() vs flt(x, 2) — inconsistent rounding

reverse_amount = round(flt(wt.amount) * return_ratio, 2)

round() uses Python's banker's rounding. The rest of the codebase uses flt(x, 2) which uses Decimal rounding. For currency, this rarely matters but is inconsistent. Use flt(wt.amount * return_ratio, 2).

13. Missing newline at end of wallet_transaction.py

Diff shows \ No newline at end of file. Add trailing newline.

---

VERIFIED CORRECT

• payment.account fix — SalesInvoicePayment is a Document object, not a dict. Dot notation is required.
• add_to_customer_balance flag — properly sent from frontend in invoiceData, correctly read via invoice.get("add_to_customer_balance") in backend.
• Hooks merge — single "Customer" entry with all three after_insert hooks.
• Proportional reversal math (wt.amount * return_ratio) — correct for wallet currency amounts. Will not exactly match ERPNext's integer-truncated point recalculation in edge cases, but pragmatically acceptable.

---

Summary

#	Severity	Issue	Verified Against
1	SHOWSTOPPER	submit(ignore_permissions=True) crashes — feature is dead	Frappe source: Document.submit() takes 0 kwargs
2	CRITICAL	No try/except around credit_return_to_wallet in submit_invoice	invoices.py code structure
3	CRITICAL	min_spent misused as per-invoice threshold instead of cumulative tier selector	ERPNext loyalty_program.py:95-106, field label "Minimum Total Spent"
4	CRITICAL	loyalty_eligible cancels ALL wallet types, not just Loyalty Credit	wallet_transaction.py loop logic
5	MEDIUM	validate_amount() blocks reversal debits on insufficient balance	Frappe insert() → validate() chain; ignore_permissions ≠ ignore_validate
6	MEDIUM	No idempotency on partial return reversals — retries create duplicate debits	wallet_transaction.py: no existing-check before debit creation
7	MEDIUM	Imports unsafe get_or_create_wallet that throws + bypasses settings	Two different functions verified in api/wallet.py vs doctype/wallet/wallet.py
8	MEDIUM	create_wallet_on_customer_insert no try/except — can block Customer creation	Frappe after_insert runs synchronously in insert()
9	MEDIUM	Auto-create guard bypassed when no POS Settings exist	Traced all 3 scenarios through the if/and short-circuit
10	MEDIUM	Stuck draft blocks idempotency guard permanently	docstatus != 2 matches both Draft and Submitted
11	LOW	Variable name loyalty_eligible inverted	Code reading
12	LOW	round() vs flt(x,2) inconsistency	Codebase convention
13	LOW	Missing trailing newline	Diff output

Recommended Action

Must fix before merge: Issues 1-4. Issue #1 alone makes the entire credit_return_to_wallet feature non-functional at runtime.

Should fix: Issues 5-10. These cause real production problems in specific but common scenarios (customer spent wallet balance before return, retry on network failure, no POS config).

Nice to have: Issues 11-13.

Squash commits into logical units before merge.

[MohamedAliSmk]

@engahmed1190
min_spent is fundamentally misused — wrong cancellation logic
Is need discuss & investigate.

[MohamedAliSmk]

3. min_spent used as per-invoice threshold instead of tier selector → Not done
4. Cancels all wallet credit types, not just Loyalty Credit → Not done
5. validate_amount() blocks reversal debits on insufficient balance → Not done
6. Unsafe get_or_create_wallet import/function usage → Not done
7. Stuck draft blocks idempotency guard (docstatus != 2) → Not done