Retry CC feature

[kumar-interserver]

Retry CC feature when primary CC fails payment.

[claude]

🔴 RETRY_CC at src/cc.inc.php:573 is referenced bare (RETRY_CC == 1) without a defined() guard, but the constant is declared nowhere in the plugin (repo-wide grep for RETRY_CC returns only this line and the two retry_cc request-param uses). On a host that hasn't externally defined RETRY_CC, PHP 8.0+ throws a fatal Error and the entire decline branch 500s before the retry logic, the paypal fallback, or the decline email run; on PHP 7.x it emits E_WARNING and 'RETRY_CC' == 1 evaluates to false, silently disabling the feature this PR advertises. Fix: guard with defined('RETRY_CC') && RETRY_CC == 1 and ideally register the setting in src/Plugin.php alongside the other constants (e.g. AUTHORIZENET_LOGIN).

Extended reasoning...

What the bug is

The PR introduces a new config-driven feature toggle via RETRY_CC at src/cc.inc.php:573:

if (
    count($ccs) > 1 &&
    RETRY_CC == 1 &&
    (!isset(App::variables()->request['ot_cc']) || isset(App::variables()->request['retry_cc']))
) { ... }

RETRY_CC is referenced as a bare constant with no defined() guard. A full-repo grep for RETRY_CC returns exactly one match — this line. No define('RETRY_CC', ...) exists in the plugin, there is no ->add_*_setting(...) registration for it in src/Plugin.php (unlike AUTHORIZENET_LOGIN, MAXMIND_*_DISABLE_CC, etc.), and no documentation mentions that a host must define it.

Why existing code doesn't prevent this

Elsewhere in the same file, other host-supplied constants like AUTHORIZENET_LOGIN are used bare — that is a pre-existing anti-pattern — but for the DOMAIN constant at line 525, this very file guards with defined(DOMAIN) (itself buggy since it's missing the quotes, but the intent is clearly to guard). The PR doesn't even attempt a guard for the new toggle, and because RETRY_CC is brand new it is guaranteed to be missing on every deploy until ops adds it.

Impact by PHP version

• PHP 8.0+: Referencing an undefined constant throws Error: Undefined constant "RETRY_CC". This kills the entire default (decline) branch of charge_card() before the retry logic, the PayPal fallback (App::accounts()->update(['payment_method' => 'paypal', 'cc_auto' => '0'])), and the prior decline-email send all run. Every card decline becomes an uncaught fatal — a 500 on the payment pipeline. composer.json technically lists php >=5.0, but require-dev phpunit/phpunit ^9.6 requires PHP 7.3+ and the codebase uses modern syntax, so an 8+ production runtime is highly plausible.
• PHP 7.x: Emits E_WARNING: Use of undefined constant RETRY_CC and falls back to the string literal 'RETRY_CC'. The loose comparison 'RETRY_CC' == 1 coerces the string to 0 (no leading digits) and evaluates to false. The retry feature is silently disabled on every host that hasn't added the constant — defeating the headline purpose of this PR with no error visible to operators.

Step-by-step proof (PHP 8.1, host has not defined RETRY_CC)

1. Customer has two cards on file. charge_card($custid, ...) is invoked.
2. Primary card fails Authorize.Net validation; response['code'] != '1', so we enter the default switch branch at line 517.
3. The decline-path Smarty email template is populated and fetched (lines 518-565).
4. Execution reaches line 571: if (count($ccs) > 1 && RETRY_CC == 1 && ...).
5. PHP evaluates RETRY_CC. The constant is undefined → Error: Undefined constant "RETRY_CC" is thrown.
6. Execution never reaches the retry_charge_card() call, the multiMail() call was already removed by this PR (it used to sit at line 567 pre-change), and the else branch that sets payment_method => paypal / cc_auto => 0 never runs.
7. The fatal propagates up and the outer caller (invoice cron, pay_balance handler, etc.) gets a 500. The customer's invoice is not marked declined, no email is sent, the account is not downgraded to PayPal.

Step-by-step proof (PHP 7.4, host has not defined RETRY_CC)

1. Same setup, same decline.
2. At line 571: RETRY_CC is undefined → E_WARNING, value is string 'RETRY_CC'.
3. 'RETRY_CC' == 1 evaluates to false (string → int → 0).
4. The if is false, so we go to the else branch and run App::accounts()->update([...paypal, cc_auto=0]).
5. The entire retry-on-backup-card feature this PR advertises never runs; operators see no error in normal logs, and every multi-card customer with a decline is silently downgraded to PayPal without getting a backup-card retry.

How to fix

Minimum: wrap the reference with a defined check (note the string form — defined(DOMAIN) elsewhere in this file is itself broken):

if (
    count($ccs) > 1 &&
    defined('RETRY_CC') && RETRY_CC == 1 &&
    (!isset(App::variables()->request['ot_cc']) || isset(App::variables()->request['retry_cc']))
) { ... }

Better: also register the new setting in src/Plugin.php alongside the existing constants (AUTHORIZENET_LOGIN, MAXMIND_*) so hosts have a single place to configure it, and document the default. That makes the feature actually enable-able by ops without a code change.

[claude]

🔴 This PR deletes make_cc_decline() and email_cc_decline() from src/cc.inc.php but leaves three stale references: src/Plugin.php:67-68 still registers them as lazy-loadable requirements, tests/CcFunctionsTest.php:255-256 asserts their function make_cc_decline(/function email_cc_decline( strings are present (so testAllExpectedFunctionsDeclared() now fails deterministically), and CLAUDE.md:33 still documents them as core CC functions. Fix by either dropping the Plugin.php registrations + test entries + doc mentions in this PR, or retaining the two functions as thin back-compat stubs.

Extended reasoning...

What the bug is

The PR removes both make_cc_decline($custid, $invoice_id) and email_cc_decline($custid, $invoice_id) from src/cc.inc.php (the 56-line deletion around the old line 244 range) but does not update any of the call-sites or references that name them. Three concrete breakages remain in the tree:

1. src/Plugin.php:67-68 — the plugin's getRequirements() still contains:

   $loader->add_requirement('make_cc_decline', '/../vendor/detain/myadmin-authorizenet-payments/src/cc.inc.php');
   $loader->add_page_requirement('email_cc_decline', '/../vendor/detain/myadmin-authorizenet-payments/src/cc.inc.php');

   These entries wire the names into the lazy-loader. Any caller that resolves function_requirements('make_cc_decline') or function_requirements('email_cc_decline') will successfully include cc.inc.php and then fatal with Call to undefined function the moment it tries to invoke the function.

2. tests/CcFunctionsTest.php:245-271 — testAllExpectedFunctionsDeclared() iterates the $expectedFunctions array (which still lists 'make_cc_decline' and 'email_cc_decline' at lines 255-256) and asserts "function {$func}(" appears in the contents of cc.inc.php. Both assertions will now fail because the strings function make_cc_decline( and function email_cc_decline( no longer appear anywhere in the file.

3. CLAUDE.md:33 — the architecture section still lists make_cc_decline() and email_cc_decline() in the bullet enumerating core CC functions, so the docs lie about what the module exports.

Step-by-step proof

Test failure (deterministic on this branch):

1. Run vendor/bin/phpunit tests/CcFunctionsTest.php.
2. testAllExpectedFunctionsDeclared() reads cc.inc.php into $content.
3. Loop reaches $func = 'make_cc_decline'; assertStringContainsString('function make_cc_decline(', $content) fails because the function body was deleted in this PR.
4. Test exits with failure before ever checking email_cc_decline, which would also fail.

Latent runtime fatal:

1. Anywhere in the wider myadmin codebase that calls function_requirements('email_cc_decline') (a common idiom for this plugin) will consult the loader registered at Plugin.php:68.
2. The loader includes cc.inc.php successfully.
3. The caller then invokes email_cc_decline($custid, $invoice_id), triggering PHP Fatal error: Uncaught Error: Call to undefined function email_cc_decline().

Why existing code doesn't prevent it

The PR's scope was only src/cc.inc.php (confirmed by changed-files count="1" in the PR metadata). Neither Plugin.php, the test, nor CLAUDE.md was updated to match the deletion, and nothing else defines these functions elsewhere in the repo.

Impact

• Test suite is red on this branch (testAllExpectedFunctionsDeclared fails on two assertions).
• Any external caller using the existing lazy-loader contract for these names will crash at runtime instead of getting a clean "function not found" at load time.
• Public documentation (CLAUDE.md) diverges from reality.

How to fix

Pick one of:

A. Clean removal — in this PR, also:

• Delete Plugin.php lines 67-68.
• Remove 'make_cc_decline' and 'email_cc_decline' from $expectedFunctions in tests/CcFunctionsTest.php:255-256.
• Remove the two function names from the Core CC Functions bullet at CLAUDE.md:33.

B. Back-compat stubs — restore thin wrappers in cc.inc.php (e.g. function email_cc_decline($custid, $invoice_id) { /* no-op or forward to new flow */ }) so existing loader entries and callers continue to work.

[claude]

🔴 During the retry-card recursion, both the add_output() alert banner at line 507 and the multiMail(payment_failed.tpl) at line 568 fire on every level of retry_charge_card() recursion. A customer with N cards that all decline receives N 'Credit Card Payment Declined' emails and, in customer-facing flows that reach line 571 without a preset ot_cc (e.g. pay_balance / charge_card_invoice with no explicit card choice), sees N stacked 'Your credit card has declined' banners in the rendered page. Fix: gate both side effects on !isset(App::variables()->request['retry_cc']) so they fire only at the outermost entry, and emit once at the top after the retry chain terminates unsuccessfully.

Extended reasoning...

What the bug is

The default (decline) branch of charge_card() runs two user-visible side effects unconditionally on every entry:

• Line 507 — add_output('<div class="container alert alert-danger">...Your credit card has declined...</div>') appends a full alert-danger banner to the output buffer.
• Line 568 — (new \\MyAdmin\\Mail())->multiMail($subject, $email, get_invoice_email($data), 'client/payment_failed.tpl') dispatches the 'Credit Card Payment Declined' email.

Both sit above the retry block (lines 571-594). The retry block calls retry_charge_card() at line 587, which itself re-enters charge_card(). On that recursive re-entry the decline-branch side effects fire again for every card in the retry chain that also declines.

Why the guard doesn't stop recursion

The retry gate at line 574 is (!isset(App::variables()->request['ot_cc']) || isset(App::variables()->request['retry_cc'])). retry_charge_card() sets both ot_cc (to the next card id) and retry_cc = 1 before recursing (lines 765-766). So on the inner call isset(ot_cc) is true (making !isset false) but isset(retry_cc) is also true — the OR short-circuits to true. The inner decline path enters the retry block again, re-runs add_output() + multiMail(), and recurses further. Termination is provided only by get_next_cc() returning false once the carddecline log has eaten every card.

Step-by-step proof (N=3 all-decline, RETRY_CC=1, auto-billing / pay_balance without explicit ot_cc)

1. Outer charge_card() charges primary — declines → default: branch.
2. Line 507 fires banner Minor upgrade to enable and disable CC #1 into the output buffer.
3. Line 568 dispatches email Minor upgrade to enable and disable CC #1 ('Credit Card Payment Declined').
4. Retry gate: count($ccs)>1 ✓, RETRY_CC==1 ✓, !isset(ot_cc) ✓ (outer had no ot_cc). Inserts primary into carddecline history, calls retry_charge_card().
5. retry_charge_card → get_next_cc returns backup1's id. Sets ot_cc=backup1, retry_cc=1. Recursively calls charge_card.
6. Inner charge_card charges backup1 — declines → default: branch.
7. Line 507 fires banner Payment handle param added #2 (appended to the SAME output buffer as Minor upgrade to enable and disable CC #1).
8. Line 568 dispatches email Payment handle param added #2.
9. Retry gate on inner: isset(ot_cc) true → !isset false; isset(retry_cc) true → OR = true. Enters retry block again. Inserts backup1 into carddecline history, calls retry_charge_card again.
10. get_next_cc skips primary (in carddecline log) and backup1 (just added), returns backup2's id. Sets ot_cc=backup2. Recursive charge_card.
11. backup2 declines → banner Increased CC limit to 4 from 2 #3, email Increased CC limit to 4 from 2 #3.
12. Next get_next_cc finds nothing → returns false; retry_charge_card returns false; stack unwinds.

Net result: customer's inbox has 3 decline emails for one failed invoice, and any customer-facing page that rendered the output buffer (pay_balance, charge_card_invoice — the former triggered from a user session where no specific ot_cc was chosen) shows 3 stacked 'Your credit card has declined' alerts.

Addressing the refutations

• 'Email duplication is a duplicate of bug_001 / an earlier inline comment.' The earlier comments on this PR describe different email defects: (a) email fires before the retry runs (so it's sent even when retry succeeds), and (b) the $cc_id-undefined bug (already fixed in the current code — line 765 now uses $next_cc). This bug is specifically about duplication across recursion levels: with N failing cards you get N emails, not one. It is distinct and observable even with the prior fixes in place.
• 'add_output stacking only matters for cron, where no human renders the output.' Not so: when pay_balance (or any customer-facing charge) is invoked without an explicit ot_cc preset by the caller, the outer call enters the retry block (!isset(ot_cc) is true), and each recursive decline appends another banner to the buffer that is rendered on the customer's page. The 'skip retry when ot_cc is set' guard only protects the explicit-card path; it does not protect the default-card path in a customer-facing flow. So the stacking is real and user-visible.
• The merged bug intentionally does not claim the carddecline-history insert is buggy — the !in_array($cc, $dec_ccs) guard at line 584 correctly records each distinct declining card exactly once (and get_next_cc depends on it). That concern is explicitly excluded here.

How to fix

Gate both user-visible side effects on the current call not being a retry recursion, and emit them once at the outermost level when the chain terminates unsuccessfully:

$isRetryCall = isset(App::variables()->request['retry_cc']);
if (!$isRetryCall) {
    add_output('<div class="container alert alert-danger">...</div>');
}
// ... build smarty email body ...
$email = $smarty->fetch('email/client/payment_failed.tpl');

if (count($ccs) > 1 && defined('RETRY_CC') && RETRY_CC == 1 && (!isset(App::variables()->request['ot_cc']) || $isRetryCall)) {
    // ... history insert ...
    $retval = retry_charge_card(...);
    if (!$retval && !$isRetryCall) {
        // Retry chain fully failed — send the single top-level decline email now
        (new \\MyAdmin\\Mail())->multiMail($subject, $email, get_invoice_email($data), 'client/payment_failed.tpl');
    }
} else {
    if (!$isRetryCall) {
        (new \\MyAdmin\\Mail())->multiMail($subject, $email, get_invoice_email($data), 'client/payment_failed.tpl');
    }
    App::accounts()->update($custid, ['payment_method' => 'paypal', 'cc_auto' => '0']);
}

A cleaner long-term fix passes 'is-retry' as an explicit argument to charge_card() rather than piggy-backing on the request store.

[detain]

looking over changes