jenkins-bot has submitted this change and it was merged. Change subject: Quit deleting from pending queue, stop saying limbo ......................................................................
Quit deleting from pending queue, stop saying limbo Get rid of 'limbo' references and old orphan rectifier. Bug: T133433 Change-Id: Ied22c6057496c550f1283489bc13ca4f81ab639a --- M README.txt M adyen_gateway/adyen.adapter.php M amazon_gateway/amazon.adapter.php M astropay_gateway/astropay.adapter.php M extension.json M gateway_common/gateway.adapter.php D globalcollect_gateway/GlobalCollectOrphanRectifier_pooled.php M globalcollect_gateway/globalcollect.adapter.php D globalcollect_gateway/scripts/orphans.php M paypal_gateway/express_checkout/paypal_express.adapter.php M tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php M tests/phpunit/includes/test_gateway/TestingGlobalCollectAdapter.php 12 files changed, 19 insertions(+), 318 deletions(-) Approvals: Awight: Looks good to me, approved jenkins-bot: Verified diff --git a/README.txt b/README.txt index e7b1d48..317bca0 100644 --- a/README.txt +++ b/README.txt @@ -332,16 +332,12 @@ // Incoming donations that we think have been paid for. 'completed' => array(), - // So-called limbo queue for GlobalCollect, where we store donor personal - // information while waiting for the donor to return from iframe or a - // redirect. It's very important that this data is not stored anywhere - // permanent such as logs or the database, until we know this person - // finished making a donation. - // FIXME: Note that this must be an instance of KeyValueStore. - // + // Transactions still needing action before they are settled. + 'pending' => array(), + // Example of a PCI-compliant queue configuration: // - // 'globalcollect-cc-limbo' => array( + // 'pending' => array( // 'type' => 'PHPQueue\Backend\Predis', // # Note that servers cannot be an array, due to some incompatibility // # with aggregate connections. @@ -353,14 +349,10 @@ // // Example of aliasing a queue // - // 'globalcollect-cc-limbo' => array( - // # Point at the main CC limbo queue. - // 'queue' => 'cc-limbo', + // 'pending' => array( + // # Point at a queue named pending-new + // 'queue' => 'pending-new', // ), - - // Transactions still needing action before they are settled. - // FIXME: who reads from this queue? - 'pending' => array(), // Non-critical queues diff --git a/adyen_gateway/adyen.adapter.php b/adyen_gateway/adyen.adapter.php index 7c12752..ae047e3 100644 --- a/adyen_gateway/adyen.adapter.php +++ b/adyen_gateway/adyen.adapter.php @@ -190,7 +190,7 @@ $this->logger->info( "launching external iframe request: " . print_r( $requestParams, true ) ); $this->logPaymentDetails(); - $this->setLimboMessage(); + $this->sendPendingMessage(); break; } } @@ -269,7 +269,6 @@ } } else { - $this->deleteLimboMessage( 'pending' ); $this->finalizeInternalStatus( FinalStatus::FAILED ); $this->logger->info( "Negative response from gateway. Full response: " . print_r( $response, TRUE ) ); } diff --git a/amazon_gateway/amazon.adapter.php b/amazon_gateway/amazon.adapter.php index 3615702..13ccdc1 100644 --- a/amazon_gateway/amazon.adapter.php +++ b/amazon_gateway/amazon.adapter.php @@ -206,7 +206,7 @@ // audit and IPN messages $details = $this->getStompTransaction(); $this->logger->info( 'Got info for Amazon donation: ' . json_encode( $details ) ); - $this->setLimboMessage(); + $this->sendPendingMessage(); } /** @@ -251,7 +251,6 @@ $this->finalizeInternalStatus( $this->capture_status_map[$captureState] ); $this->postProcessDonation(); - $this->deleteLimboMessage( 'pending' ); } /** diff --git a/astropay_gateway/astropay.adapter.php b/astropay_gateway/astropay.adapter.php index 7b971bd..da6b8c8 100644 --- a/astropay_gateway/astropay.adapter.php +++ b/astropay_gateway/astropay.adapter.php @@ -289,7 +289,6 @@ $this->logger->info( "Payment status $status coming back to ResultSwitcher" ); $this->finalizeInternalStatus( $status ); $this->postProcessDonation(); - $this->deleteLimboMessage( 'pending' ); break; case 'NewInvoice': $this->processNewInvoiceResponse( $response ); diff --git a/extension.json b/extension.json index 9ee40cd..494c229 100644 --- a/extension.json +++ b/extension.json @@ -114,7 +114,6 @@ "GlobalCollectAdapter": "globalcollect_gateway/globalcollect.adapter.php", "GlobalCollectOrphanAdapter": "globalcollect_gateway/orphan.adapter.php", "GlobalCollectOrphanRectifier": "globalcollect_gateway/GlobalCollectOrphanRectifier.php", - "GlobalCollectOrphanRectifier_pooled": "globalcollect_gateway/GlobalCollectOrphanRectifier_pooled.php", "IngenicoFinancialNumber": "globalcollect_gateway/IngenicoFinancialNumber.php", "IngenicoLanguage": "globalcollect_gateway/IngenicoLanguage.php", "IngenicoMethodCodec": "globalcollect_gateway/IngenicoMethodCodec.php", diff --git a/gateway_common/gateway.adapter.php b/gateway_common/gateway.adapter.php index 307b0a7..67f016a 100644 --- a/gateway_common/gateway.adapter.php +++ b/gateway_common/gateway.adapter.php @@ -1436,7 +1436,7 @@ $this->logPaymentDetails(); // Feed the message into the pending queue, so the CRM queue consumer // can read it to fill in donor details when it gets a partial message - $this->setLimboMessage(); + $this->sendPendingMessage(); // Avoid 'bad ffname' logspam on return and try again links. $this->session_pushFormName( $this->getData_Unstaged_Escaped( 'ffname' ) ); } @@ -2254,19 +2254,10 @@ DonationQueue::instance()->push( $this->getStompTransaction(), $queue ); } - protected function setLimboMessage( $queue = 'pending' ) { - // FIXME: log the key and raw queue name. - $this->logger->info( "Setting transaction in limbo store [$queue]" ); - DonationQueue::instance()->push( $this->getStompTransaction(), $queue ); - } - - protected function deleteLimboMessage( $queue = 'pending' ) { - $this->logger->info( "Clearing transaction from limbo store [$queue]" ); - try { - DonationQueue::instance()->delete( $this->getCorrelationID(), $queue ); - } catch( BadMethodCallException $ex ) { - $this->logger->warning( "Backend for queue [$queue] does not support deletion. Hope your message had an expiration date!" ); - } + protected function sendPendingMessage() { + $order_id = $this->getData_Unstaged_Escaped( 'order_id' ); + $this->logger->info( "Sending donor details for $order_id to pending queue" ); + DonationQueue::instance()->push( $this->getStompTransaction(), 'pending' ); } /** diff --git a/globalcollect_gateway/GlobalCollectOrphanRectifier_pooled.php b/globalcollect_gateway/GlobalCollectOrphanRectifier_pooled.php deleted file mode 100644 index 87f7430..0000000 --- a/globalcollect_gateway/GlobalCollectOrphanRectifier_pooled.php +++ /dev/null @@ -1,230 +0,0 @@ -<?php - -use Predis\Connection\ConnectionException; - -/** - * Legacy STOMP orphan rectifier - * @deprecated - * - * TODO: Generalize to all gateways, using hooks to implement polymorphism. - */ -class GlobalCollectOrphanRectifier_pooled { - - protected $killfiles = array(); - protected $order_ids = array(); - protected $target_execute_time; - protected $max_per_execute; //only really used if you're going by-file. - protected $adapter; - - public function __construct() { - // Have to turn this off here, until we know it's using the user's ip, and - // not 127.0.0.1 during the batch process. Otherwise, we'll immediately - // lock ourselves out when processing multiple charges. - global $wgDonationInterfaceEnableIPVelocityFilter; - $wgDonationInterfaceEnableIPVelocityFilter = false; - - if ( !$this->getOrphanGlobal( 'enable' ) ){ - $this->info( 'Orphan cron disabled. Have a nice day!' ); - return; - } - - $this->target_execute_time = $this->getOrphanGlobal( 'target_execute_time' ); - $this->max_per_execute = $this->getOrphanGlobal( 'max_per_execute' ); - - // FIXME: Is this just to trigger batch mode? - $data = array( - 'wheeee' => 'yes' - ); - $this->adapter = new GlobalCollectOrphanAdapter(array('external_data' => $data)); - $this->logger = DonationLoggerFactory::getLogger( $this->adapter ); - } - - protected function keepGoing(){ - $elapsed = $this->getProcessElapsed(); - if ( $elapsed < $this->target_execute_time ) { - return true; - } else { - return false; - } - } - - /** - * This will both return the elapsed process time, and echo something for - * the cronspammer. - * @return int elapsed time since start in seconds - */ - protected function getProcessElapsed(){ - $elapsed = time() - $this->start_time; - $this->info( "Elapsed Time: {$elapsed}" ); - return $elapsed; - } - - protected function deleteMessage( $correlation_id, $queue ) { - DonationQueue::instance()->delete( $correlation_id, $queue ); - } - - public function processOrphans() { - // TODO: Make this configurable. - // 20 minutes: this is exactly equal to something on Globalcollect's side. - $time_buffer = 60*20; - - $queue_pool = new CyclicalArray( $this->getOrphanGlobal( 'gc_cc_limbo_queue_pool' ) ); - if ( $queue_pool->isEmpty() ) { - // FIXME: cheesy inline default - $queue_pool = new CyclicalArray( GlobalCollectAdapter::GC_CC_LIMBO_QUEUE ); - } - - $this->info( "Slaying orphans..." ); - $this->start_time = time(); - - //I want to be clear on the problem I hope to prevent with this. Say, - //for instance, we pull a legit orphan, and for whatever reason, can't - //completely rectify it. Then, we go back and pull more... and that - //same one is in the list again. We should stop after one try per - //message per execute. We should also be smart enough to not process - //things we believe we just deleted. - $this->handled_ids = array(); - - while ( $this->keepGoing() && !$queue_pool->isEmpty() ) { - $current_queue = $queue_pool->current(); - try { - $message = DonationQueue::instance()->peek( $current_queue ); - - if ( !$message ) { - $this->info( "Emptied queue [{$current_queue}], removing from pool." ); - $queue_pool->dropCurrent(); - continue; - } - - $correlation_id = 'globalcollect-' . $message['gateway_txn_id']; - if ( array_key_exists( $correlation_id, $this->handled_ids ) ) { - // We already did this one, keep going. It's fine to draw - // again from the same queue. - DonationQueue::instance()->delete( $correlation_id, $current_queue ); - continue; - } - - // Check the timestamp to see if it's old enough, and stop when - // we're below the threshold. Messages are guaranteed to pop in - // chronological order. - $elapsed = $this->start_time - $message['date']; - if ( $elapsed < $time_buffer ) { - $this->info( "Exhausted new messages in [{$current_queue}], removing from pool..." ); - $queue_pool->dropCurrent(); - - continue; - } - - // We got ourselves an orphan! - if ( $this->rectifyOrphan( $message ) ) { - $this->handled_ids[$correlation_id] = 'rectified'; - } else { - $this->handled_ids[$correlation_id] = 'error'; - } - - // Throw out the message either way. - $this->deleteMessage( $correlation_id, $current_queue ); - - // Round-robin the pool before we complete the loop. - $queue_pool->rotate(); - } catch ( ConnectionException $ex ) { - // Drop this server, for the duration of this batch. - $this->error( "Queue server for [$current_queue] is down! Ignoring for this run..." ); - $queue_pool->dropCurrent(); - } - } - - //TODO: Make stats squirt out all over the place. - $rec = 0; - $err = 0; - foreach( $this->handled_ids as $id=>$whathappened ){ - switch ( $whathappened ){ - case 'rectified': - $rec += 1; - break; - case 'error': - $err += 1; - break; - } - } - $final = "\nDone! Final results: \n"; - $final .= " $rec rectified orphans \n"; - $final .= " $err errored out \n"; - if ( isset( $this->adapter->orphanstats ) ){ - foreach ( $this->adapter->orphanstats as $status => $count ) { - $final .= "\n Status $status = $count"; - } - } - $final .= "\n Approximately " . $this->getProcessElapsed() . " seconds to execute.\n"; - $this->info( $final ); - } - - /** - * Uses the Orphan Adapter to rectify (complete the charge for) a single orphan. Returns a boolean letting the caller know if - * the orphan has been fully rectified or not. - * @param array $data Some set of orphan data. - * @param boolean $query_contribution_tracking A flag specifying if we should query the contribution_tracking table or not. - * @return boolean True if the orphan has been rectified, false if not. - */ - protected function rectifyOrphan( $data, $query_contribution_tracking = true ){ - $data['order_id'] = $data['gateway_txn_id']; - - $this->info( "Rectifying orphan: {$data['order_id']}" ); - $rectified = false; - - $normalized = DonationQueue::queueMessageToNormalized( $data ); - $this->adapter->loadDataAndReInit( $normalized, $query_contribution_tracking ); - $results = $this->adapter->do_transaction( 'Confirm_CreditCard' ); - $message = $results->getMessage(); - if ( $results->getCommunicationStatus() ){ - $this->info( $data['contribution_tracking_id'] . ": FINAL: " . $this->adapter->getValidationAction() ); - $rectified = true; - } else { - $this->info( $data['contribution_tracking_id'] . ": ERROR: " . $message ); - if ( strpos( $message, "GET_ORDERSTATUS reports that the payment is already complete." ) === 0 ){ - $rectified = true; - } - - //handles the transactions we've cancelled ourselves... though if they got this far, that's a problem too. - $errors = $results->getErrors(); - if ( !empty( $errors ) && array_key_exists( '1000001', $errors ) ){ - $rectified = true; - } - - //apparently this is well-formed GlobalCollect for "iono". Get rid of it. - if ( strpos( $message, "No processors are available." ) === 0 ){ - $rectified = true; - } - } - - $this->info( $message ); - - return $rectified; - } - - /** - * Gets the global setting for the key passed in. - * @param string $key - * - * FIXME: Reuse GatewayAdapter::getGlobal. - * @return mixed - */ - protected static function getOrphanGlobal( $key ){ - global $wgDonationInterfaceOrphanCron; - if ( array_key_exists( $key, $wgDonationInterfaceOrphanCron ) ){ - return $wgDonationInterfaceOrphanCron[$key]; - } else { - return NULL; - } - } - - protected function info( $msg ) { - $this->logger->info( $msg ); - print( "{$msg}\n" ); - } - - protected function error( $msg ) { - $this->logger->error( $msg ); - error_log( $msg ); - } -} diff --git a/globalcollect_gateway/globalcollect.adapter.php b/globalcollect_gateway/globalcollect.adapter.php index abc5d9d..1550afa 100644 --- a/globalcollect_gateway/globalcollect.adapter.php +++ b/globalcollect_gateway/globalcollect.adapter.php @@ -25,8 +25,6 @@ const GATEWAY_NAME = 'Global Collect'; const IDENTIFIER = 'globalcollect'; const GLOBAL_PREFIX = 'wgGlobalCollectGateway'; - // @deprecated - const GC_CC_LIMBO_QUEUE = 'globalcollect-cc-limbo'; public function getCommunicationType() { return 'xml'; @@ -625,13 +623,7 @@ */ private function transactionConfirm_CreditCard(){ $is_orphan = $this->isBatchProcessor(); - if ( !$is_orphan ) { - // This was a normal front-end donation. - - // @deprecated We should be able to skip any deletion. - $this->logger->info( 'Donor returned, deleting limbo message' ); - $this->deleteLimboMessage( self::GC_CC_LIMBO_QUEUE ); - } else { + if ( $is_orphan ) { // We're in orphan processing mode, so a "pending waiting for donor // input" status means that we'll never complete. Set this range // to map to "failed". @@ -881,10 +873,6 @@ //get the old status from the first txn, and add in the part where we set the payment. $this->transaction_response->setTxnMessage( "Original Response Status (pre-SET_PAYMENT): " . $original_status_code ); } - - // We won't need the limbo message again, either way, so cancel it. - // @deprecated - $this->deleteLimboMessage(); } } } @@ -1659,7 +1647,7 @@ if ( $action != FinalStatus::FAILED ){ // TODO: if method_loses_control rather than hardcode cc. if ( $this->getData_Unstaged_Escaped( 'payment_method' ) === 'cc' ) { - $this->setLimboMessage( self::GC_CC_LIMBO_QUEUE ); + $this->sendPendingMessage(); } } } diff --git a/globalcollect_gateway/scripts/orphans.php b/globalcollect_gateway/scripts/orphans.php deleted file mode 100644 index 1d0ce87..0000000 --- a/globalcollect_gateway/scripts/orphans.php +++ /dev/null @@ -1,21 +0,0 @@ -<?php -//actually, as a maintenance script, this totally is a valid entry point. -// FIXME: Prevent web access even if the security is misconfigured so this is runnable. - -$IP = getenv( 'MW_INSTALL_PATH' ); -if ( $IP === false ) { - $IP = __DIR__ . '/../../../..'; -} - -//If you get errors on this next line, set (and export) your MW_INSTALL_PATH var. -require_once( "$IP/maintenance/Maintenance.php" ); - -class OrphanMaintenance extends Maintenance { - public function execute() { - $rectifier = new GlobalCollectOrphanRectifier_pooled(); - $rectifier->processOrphans(); - } -} - -$maintClass = 'OrphanMaintenance'; -require_once RUN_MAINTENANCE_IF_MAIN; diff --git a/paypal_gateway/express_checkout/paypal_express.adapter.php b/paypal_gateway/express_checkout/paypal_express.adapter.php index 306ebb7..6cc85d7 100644 --- a/paypal_gateway/express_checkout/paypal_express.adapter.php +++ b/paypal_gateway/express_checkout/paypal_express.adapter.php @@ -406,8 +406,6 @@ // response and sort it into complete or pending. $this->finalizeInternalStatus( FinalStatus::COMPLETE ); $this->postProcessDonation(); - // FIXME: deprecated - $this->deleteLimboMessage( 'pending' ); break; case 'SetExpressCheckout': case 'SetExpressCheckout_recurring': @@ -476,8 +474,6 @@ // TODO: Can we do this from do_transaction instead, or at least protect with !recurring... $this->finalizeInternalStatus( $status ); $this->postProcessDonation(); - // FIXME: deprecated - $this->deleteLimboMessage( 'pending' ); break; } diff --git a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php index 70c1ce9..482bb9d 100644 --- a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php +++ b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php @@ -456,10 +456,6 @@ $gateway->setDummyGatewayResponseCode( $code ); $gateway->do_transaction( 'Confirm_CreditCard' ); $this->assertEquals( 1, count( $gateway->curled ), "Gateway kept trying even with response code $code! MasterCard could fine us a thousand bucks for that!" ); - - // Test limbo queue contents. - $this->assertEquals( array( true ), $gateway->limbo_messages, - "Gateway did not delete limbo message for code $code!" ); } /** diff --git a/tests/phpunit/includes/test_gateway/TestingGlobalCollectAdapter.php b/tests/phpunit/includes/test_gateway/TestingGlobalCollectAdapter.php index e940ae9..a1c196e 100644 --- a/tests/phpunit/includes/test_gateway/TestingGlobalCollectAdapter.php +++ b/tests/phpunit/includes/test_gateway/TestingGlobalCollectAdapter.php @@ -9,7 +9,7 @@ public $curled = array ( ); - public $limbo_messages = array(); + public $pending_messages = array(); public $dummyGatewayResponseCode; @@ -48,15 +48,8 @@ } // TODO: Store and test the actual messages. - public function setLimboMessage( $queue = 'pending' ) { - $this->limbo_messages[] = false; - } - - /** - * Stub out the limboStomp fn and record the calls - */ - public function deleteLimboMessage( $queue = 'pending' ) { - $this->limbo_messages[] = true; + public function sendPendingMessage() { + $this->pending_messages[] = false; } /** -- To view, visit https://gerrit.wikimedia.org/r/301630 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ied22c6057496c550f1283489bc13ca4f81ab639a Gerrit-PatchSet: 15 Gerrit-Project: mediawiki/extensions/DonationInterface Gerrit-Branch: master Gerrit-Owner: Ejegg <eeggles...@wikimedia.org> Gerrit-Reviewer: AndyRussG <andrew.green...@gmail.com> Gerrit-Reviewer: Awight <awi...@wikimedia.org> Gerrit-Reviewer: Cdentinger <cdentin...@wikimedia.org> Gerrit-Reviewer: Ssmith <ssm...@wikimedia.org> Gerrit-Reviewer: XenoRyet <dkozlow...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits