Clearing existing CC failure log if any when cc is charged successfully

[kumar-interserver]

No description provided.

[claude]

🔴 The new $returnURL === false gate on the retry branch (line 588) has an unintended side effect: webpage-initiated charges (e.g. charge_card_invoice.php:40 passes returnURL=true) now skip the retry branch on decline and fall through to the pre-existing else (lines ~603-609), which silently calls App::accounts()->update($custid, ['payment_method' => 'paypal', 'cc_auto' => '0']) — so a single webpage decline now (a) skips trying the backup CC even when a working one is on file, (b) doesn't log a carddecline row (the insert lives inside the retry branch at line ~600), and (c) silently flips the customer's stored payment_method to paypal and disables cc_auto. Suggest moving the force-paypal fallback behind its own billingd-only check, or otherwise decoupling it from the retry gate (e.g. gate on $returnURL === false or a dedicated billingd flag for the paypal switch too, not just for retry).

Extended reasoning...

The change

Line 588 adds $returnURL === false as a new AND-term in the retry-branch condition. The stated intent (inline comment) is "is to work only when billingd calls" — and indeed charge_card()'s default is $returnURL = false, which billingd-style callers leave untouched.

The problem

The retry branch and the "force customer to paypal" branch are the two arms of the same if/else. They were already paired before this PR:

if (count($ccs) > 1 && RETRY_CC == 1 && (... ot_cc logic ...)) {
    // log carddecline + retry with backup CC
    $retval = retry_charge_card(...);
} else {
    // force account onto paypal
    App::accounts()->update($custid, ['payment_method' => 'paypal', 'cc_auto' => '0']);
}

Adding $returnURL === false to the if shrinks the set of calls that enter the retry arm but expands the set that enters the else arm, since it is the complement. Any webpage-initiated call (where returnURL is true or a string) that previously retried with a backup CC now silently force-switches the account to paypal and disables cc_auto. It also skips the carddecline row insert that lives inside the retry branch (line ~600), which means billingd later has no record of the decline.

Real caller verified

src/charge_card_invoice.php:40 calls charge_card($custid, $amount, $invoice_id, $module, true) — a webpage-initiated invoice-payment flow passing returnURL = true. Before this PR, a decline here with count($ccs) > 1 and RETRY_CC = 1 entered the retry branch. After this PR, the same call falls into the else branch and silently mutates stored account settings.

Addressing the refutation

One verifier argued this is pre-existing because "at the PR base (commit 64eec63) the paypal switch ran unconditionally after every CC failure." That is inspecting the wrong commit. PR #18 begins at 09c2137; its base is a597ae9 (the commit immediately preceding it). Commits 5c5fad5 ("var cc_id undefined error and other updates") and a597ae9 landed before this PR and already moved the paypal switch into the else arm of the retry conditional. git diff a597ae9 HEAD -- src/cc.inc.php confirms the base for this PR has the paypal switch in the else branch, not unconditional. So the "webpage caller gets force-paypal" outcome is genuinely a new behavior introduced by this PR, not a pre-existing state.

The second half of the refutation — that losing the retry on webpage declines is intentional per the inline comment — is half true. The inline comment documents intent to gate the retry; the force-paypal side effect on webpage callers is not acknowledged anywhere and is almost certainly unintentional.

Step-by-step proof

Scenario: customer 100 has primary CC 4111...1111 and backup CC 5500...0004. RETRY_CC = 1. User clicks "Charge CC" on an unpaid invoice, which hits charge_card_invoice.php:40 → charge_card(100, 50.00, 999, $module, true). Primary CC declines at Authorize.net.

1. Control enters the default: (decline) case of the switch.
2. Retry condition evaluates: count($ccs)=2 > 1 ✓, RETRY_CC == 1 ✓, $returnURL === false → false (it's true), so the whole AND is false.
3. Execution falls to the else arm → App::accounts()->update(100, ['payment_method' => 'paypal', 'cc_auto' => '0']).
4. No carddecline row is written (that insert is inside the retry arm at line ~600).
5. Result: customer 100's stored payment method is silently changed to paypal, CC auto-billing is disabled, the working backup CC is never tried, and billingd's next pass has no history of this decline.

Before this PR the same scenario entered the retry arm, attempted the backup CC, and logged the decline — none of those outcomes were changed-to-paypal.

Suggested fix

Either gate the paypal fallback on the same billingd-only check:

} elseif ($returnURL === false) {
    App::accounts()->update($custid, ['payment_method' => 'paypal', 'cc_auto' => '0']);
}

or keep retry webpage-accessible and drop the new gate. The key point is the force-paypal side effect should not fire on webpage-initiated charges just because retry was suppressed.