Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
detain merged 5 commits into from
Apr 27, 2026
Merged

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

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
09c2137
Clearing existing CC failure log if any when cc is charged successfully
kumar-interserver Apr 24, 2026
170d646
Missed closing bracket - fixes
kumar-interserver Apr 24, 2026
68e46b3
Few more minor fixes
kumar-interserver Apr 24, 2026
132b928
Few more minor fixes
kumar-interserver Apr 24, 2026
696e867
git add . git diff ssh my@192.64.80.218 ssh my@mynew.interserver.net …
kumar-interserver Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/cc.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,21 @@ function charge_card($custid, $amount = false, $invoice = false, $module = 'defa
}
//Prepay Invoices updates ends
}
//Clear out this CC Decline history
$db->query("SELECT * FROM user_log WHERE history_owner = {$custid} AND history_type = 'carddecline'", __LINE__, __FILE__);
if ($db->num_rows() > 0) {
$history_id = null;
while ($db->next_record(MYSQL_ASSOC)) {
if ($cc == $GLOBALS['tf']->decrypt($db->Record['history_new_value'])) {
$history_id = $db->Record['history_id'];
break;
}
}
if ($history_id && intval($history_id) > 0) {
Comment thread
claude[bot] marked this conversation as resolved.
$history_id = intval($history_id);
$db->query("DELETE FROM user_log WHERE history_id = $history_id", __LINE__, __FILE__);
}
}
Comment thread
kumar-interserver marked this conversation as resolved.
break;
default:
myadmin_log('billing', 'notice', 'FAILURE (custid:'.$custid.',exp:'.$cc_exp.',cc:'.mask_cc($cc, true).',amount:'.$amount.', code:'.$response['code'].') raw: '.$cc_response, __LINE__, __FILE__);
Expand Down Expand Up @@ -571,6 +586,7 @@ function charge_card($custid, $amount = false, $invoice = false, $module = 'defa
if (
count($ccs) > 1 && //more than 1 cc present then proceed
RETRY_CC == 1 && //When CC Retry is enabled from config
$returnURL === false && //is to work only when billingd calls

@claude claude Bot Apr 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

(!isset(App::variables()->request['ot_cc']) || isset(App::variables()->request['retry_cc']))
) {
$cc_encrypted = $GLOBALS['tf']->encrypt(trim(str_replace([' ', '_', '-'], ['', '', ''], $cc)));
Expand Down
Loading