Retry CC feature #17
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| use Punic\Currency; | ||
| use Brick\Money\Money; | ||
| use Brick\Math\RoundingMode; | ||
| use MyAdmin\App; | ||
|
|
||
| /** | ||
| * @param $cc | ||
|
|
@@ -243,61 +244,6 @@ | |
| return $value; | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * given the account data array, it parses out and returns an array of ccs | ||
|
|
@@ -569,13 +515,13 @@ | |
| return $retval; | ||
| } | ||
| //$data['status'] = 'pending-fixcc'; | ||
| $subject = $settings['TITLE'].' Credit Card Payment Declined'; | ||
| $smarty = new TFSmarty(); | ||
| $smarty->assign('amount', $amount); | ||
| $smarty->assign('service_name', $settings['TBLNAME']); | ||
| $smarty->assign('company', $settings['TITLE']); | ||
| $smarty->assign('name', $data['name']); | ||
| $smarty->assign('cc_num', mask_cc($cc, true)); | ||
| if (!defined(DOMAIN) || in_array(DOMAIN, ['interserver.net', 'misha.interserver.net', 'mymisha.interserver.net']) || trim(DOMAIN) == '') { | ||
| $smarty->assign('domain', 'my.interserver.net'); | ||
| } else { | ||
|
|
@@ -618,9 +564,34 @@ | |
| $smarty->assign('returnURL', $returnURL); | ||
| } | ||
| $smarty->assign('invoices', $rows); | ||
| $email = $smarty->fetch('email/client/payment_failed.tpl'); | ||
|
claude[bot] marked this conversation as resolved.
|
||
| (new \MyAdmin\Mail())->multiMail($subject, $email, get_invoice_email($data), 'client/payment_failed.tpl'); | ||
|
|
||
| //"ot_cc" is added because it came from pay_balance where they try specific card so no retry for that. | ||
| if ( | ||
| count($ccs) > 1 && //more than 1 cc present then proceed | ||
| RETRY_CC == 1 && //When CC Retry is enabled from config | ||
|
There was a problem hiding this comment. 🔴 RETRY_CC at src/cc.inc.php:573 is referenced bare ( Extended reasoning...What the bug isThe PR introduces a new config-driven feature toggle via if (
count($ccs) > 1 &&
RETRY_CC == 1 &&
(!isset(App::variables()->request['ot_cc']) || isset(App::variables()->request['retry_cc']))
) { ... }
Why existing code doesn't prevent thisElsewhere in the same file, other host-supplied constants like Impact by PHP version
Step-by-step proof (PHP 8.1, host has not defined RETRY_CC)
Step-by-step proof (PHP 7.4, host has not defined RETRY_CC)
How to fixMinimum: wrap the reference with a defined check (note the string form — 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. |
||
| (!isset(App::variables()->request['ot_cc']) || isset(App::variables()->request['retry_cc'])) | ||
| ) { | ||
| $cc_encrypted = $GLOBALS['tf']->encrypt(trim(str_replace([' ', '_', '-'], ['', '', ''], $cc))); | ||
| $dec_ccs = []; | ||
| $db->query("SELECT * FROM user_log WHERE history_owner = {$custid} AND history_type = 'carddecline'", __LINE__, __FILE__); | ||
| if ($db->num_rows() > 0) { | ||
| while ($db->next_record(MYSQL_ASSOC)) { | ||
| $dec_ccs[] = $GLOBALS['tf']->decrypt($db->Record['history_new_value']); | ||
| } | ||
| } | ||
| if (!in_array($cc, $dec_ccs)) { | ||
| $GLOBALS['tf']->history->add('users', 'carddecline', $cc_encrypted, $cc_exp, $custid); | ||
| } | ||
| $retval = retry_charge_card($custid, $amount, $invoice, $module, $returnURL, $useHandlePayment, $queue); | ||
|
Check failure on line 587 in src/cc.inc.php
|
||
|
Comment on lines
567
to
+587
There was a problem hiding this comment. 🔴 During the retry-card recursion, both the Extended reasoning...What the bug isThe default (decline) branch of
Both sit above the retry block (lines 571-594). The retry block calls Why the guard doesn't stop recursionThe retry gate at line 574 is Step-by-step proof (N=3 all-decline, RETRY_CC=1, auto-billing / pay_balance without explicit ot_cc)
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 Addressing the refutations
How to fixGate 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 |
||
| } else { | ||
| $new_data = [ | ||
| 'payment_method' => 'paypal', | ||
| 'cc_auto' => '0' | ||
| ]; | ||
| App::accounts()->update($custid, $new_data); | ||
| } | ||
| //$GLOBALS['tf']->history->add('users', 'carddecline', $data['cc'], $data['cc_exp'], $custid); | ||
| break; | ||
| } | ||
|
|
@@ -752,6 +723,56 @@ | |
| return $retval; | ||
| } | ||
|
|
||
| /** | ||
| * Gets next available verified CC number to charge | ||
| * | ||
| */ | ||
| function get_next_cc($custid) | ||
| { | ||
| function_requirements('parse_ccs'); | ||
| $data = App::accounts()->read($custid); | ||
| $dec_ccs = []; | ||
| $db = get_module_db('default'); | ||
| $db->query("SELECT * FROM user_log WHERE history_owner = {$custid} AND history_type = 'carddecline'", __LINE__, __FILE__); | ||
| if ($db->num_rows() > 0) { | ||
| while ($db->next_record(MYSQL_ASSOC)) { | ||
| $dec_ccs[] = $GLOBALS['tf']->decrypt($db->Record['history_new_value']); | ||
| } | ||
| } | ||
| $ccs = parse_ccs($data); | ||
| foreach ($ccs as $cc_id => $cc_det) { | ||
| $cc_num = $GLOBALS['tf']->decrypt($cc_det['cc']); | ||
| if (!in_array($cc_num, $dec_ccs) && can_use_cc($data, $cc_det, false)) { | ||
| myadmin_log('billing', 'info', "Backup CC - found for customer $custid ".mask_cc($cc_num), __LINE__, __FILE__); | ||
| return $cc_id; | ||
| } | ||
| } | ||
| myadmin_log('billing', 'info', "Backup CC - not found for customer $custid", __LINE__, __FILE__); | ||
| //Here Primary & Backup both CCs failed so updating payment method paypal. | ||
| $new_data = [ | ||
| 'payment_method' => 'paypal', | ||
| 'cc_auto' => '0' | ||
| ]; | ||
| App::accounts()->update($custid, $new_data); | ||
| return false; | ||
| } | ||
|
|
||
| function retry_charge_card($custid, $amount = false, $invoice = false, $module = 'default', $returnURL = false, $useHandlePayment = true, $queue = false) | ||
| { | ||
| myadmin_log('billing', 'info', "Retrying BackupCC - Custid: $custid, Amount: $amount", __LINE__, __FILE__); | ||
| $next_cc = get_next_cc($custid); | ||
| if ($next_cc !== false) { | ||
| App::variables()->request['ot_cc'] = $next_cc; | ||
| App::variables()->request['retry_cc'] = 1; | ||
|
claude[bot] marked this conversation as resolved.
|
||
| $success = charge_card($custid, $amount, $invoice, $module, $returnURL, $useHandlePayment, $queue); | ||
| if ($success) { | ||
| myadmin_log('billing', 'info', "Retrying BackupCC - Success for $custid, Amount: $amount, CC ID - $next_cc", __LINE__, __FILE__); | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
|
claude[bot] marked this conversation as resolved.
|
||
| /** | ||
| * gets the cc bank number / bin for the given encrypted cc | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 This PR deletes
make_cc_decline()andemail_cc_decline()fromsrc/cc.inc.phpbut leaves three stale references:src/Plugin.php:67-68still registers them as lazy-loadable requirements,tests/CcFunctionsTest.php:255-256asserts theirfunction make_cc_decline(/function email_cc_decline(strings are present (sotestAllExpectedFunctionsDeclared()now fails deterministically), andCLAUDE.md:33still 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)andemail_cc_decline($custid, $invoice_id)fromsrc/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:src/Plugin.php:67-68— the plugin'sgetRequirements()still contains:These entries wire the names into the lazy-loader. Any caller that resolves
function_requirements('make_cc_decline')orfunction_requirements('email_cc_decline')will successfully includecc.inc.phpand then fatal withCall to undefined functionthe moment it tries to invoke the function.tests/CcFunctionsTest.php:245-271—testAllExpectedFunctionsDeclared()iterates the$expectedFunctionsarray (which still lists'make_cc_decline'and'email_cc_decline'at lines 255-256) and asserts"function {$func}("appears in the contents ofcc.inc.php. Both assertions will now fail because the stringsfunction make_cc_decline(andfunction email_cc_decline(no longer appear anywhere in the file.CLAUDE.md:33— the architecture section still listsmake_cc_decline()andemail_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):
vendor/bin/phpunit tests/CcFunctionsTest.php.testAllExpectedFunctionsDeclared()readscc.inc.phpinto$content.$func = 'make_cc_decline';assertStringContainsString('function make_cc_decline(', $content)fails because the function body was deleted in this PR.email_cc_decline, which would also fail.Latent runtime fatal:
myadmincodebase that callsfunction_requirements('email_cc_decline')(a common idiom for this plugin) will consult the loader registered atPlugin.php:68.cc.inc.phpsuccessfully.email_cc_decline($custid, $invoice_id), triggeringPHP 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 bychanged-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
testAllExpectedFunctionsDeclaredfails on two assertions).CLAUDE.md) diverges from reality.How to fix
Pick one of:
A. Clean removal — in this PR, also:
Plugin.phplines 67-68.'make_cc_decline'and'email_cc_decline'from$expectedFunctionsintests/CcFunctionsTest.php:255-256.Core CC Functionsbullet atCLAUDE.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.