jenkins-bot has submitted this change and it was merged.
Change subject: Be less magical about unstaging order status things
......................................................................
Be less magical about unstaging order status things
Following the paypal_express model, whitelist fields we expect to get back from
each API call, and add those to our normalized response data.
Bug: T141487
Change-Id: I8e6d044f17cd3d875a3da93adf6303abbcd3c3b5
---
M gateway_common/gateway.adapter.php
M globalcollect_gateway/config/var_map.yaml
M globalcollect_gateway/globalcollect.adapter.php
M tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanAdapterTest.php
M tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanRectifierTest.php
M tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php
M
tests/phpunit/includes/Responses/globalcollect/DO_FINISHPAYMENT_200.testresponse
A
tests/phpunit/includes/Responses/globalcollect/GET_ORDERSTATUS_600_badCvv.testresponse
M tests/phpunit/includes/Responses/globalcollect/SET_PAYMENT_200.testresponse
9 files changed, 124 insertions(+), 134 deletions(-)
Approvals:
Ejegg: Looks good to me, approved
jenkins-bot: Verified
diff --git a/gateway_common/gateway.adapter.php
b/gateway_common/gateway.adapter.php
index 4273bc2..227962f 100644
--- a/gateway_common/gateway.adapter.php
+++ b/gateway_common/gateway.adapter.php
@@ -2603,6 +2603,7 @@
$msg = self::getGatewayName() . ': Email Domain map: '
. print_r( $emailDomainMap, true );
+ // TODO: Remove this weaksalsa debug message...
$this->logger->debug( $msg );
// Lookup a score if it is defined
diff --git a/globalcollect_gateway/config/var_map.yaml
b/globalcollect_gateway/config/var_map.yaml
index 87362a6..37b0b8f 100644
--- a/globalcollect_gateway/config/var_map.yaml
+++ b/globalcollect_gateway/config/var_map.yaml
@@ -9,6 +9,7 @@
ATTEMPTID: attempt_id
AUTHORISATIONID: authorization_id
AMOUNT: amount
+AVSRESULT: avs_result
BANKACCOUNTNUMBER: bank_account_number
BANKAGENZIA: bank_agenzia # dd:IT
BANKCHECKDIGIT: bank_check_digit
@@ -27,6 +28,7 @@
CREDITCARDNUMBER: card_num
CURRENCYCODE: currency_code
CVV: cvv
+CVVRESULT: cvv_result
DATECOLLECT: date_collect
DESCRIPTOR: descriptor # eWallets
DIRECTDEBITTEXT: direct_debit_text
diff --git a/globalcollect_gateway/globalcollect.adapter.php
b/globalcollect_gateway/globalcollect.adapter.php
index 6bc534b..d7be717 100644
--- a/globalcollect_gateway/globalcollect.adapter.php
+++ b/globalcollect_gateway/globalcollect.adapter.php
@@ -367,6 +367,14 @@
'ACTION' => 'GET_ORDERSTATUS',
'VERSION' => '2.0'
),
+ 'response' => array(
+ 'EFFORTID',
+ 'ATTEMPTID',
+ 'CURRENCYCODE',
+ 'AMOUNT',
+ 'AVSRESULT',
+ 'CVVRESULT',
+ ),
);
$this->transactions['CANCEL_PAYMENT'] = array(
@@ -616,43 +624,17 @@
* @return PaymentTransactionResponse
*/
private function transactionConfirm_CreditCard(){
- // Pulling vars straight from the querystring
- $pull_vars = array(
- 'CVVRESULT' => 'cvv_result',
- 'AVSRESULT' => 'avs_result',
- );
- $qsResults = array();
- if ( !$this->isBatchProcessor() ) {
- // FIXME: Refactor as normal unstaging.
- foreach ( $pull_vars as $theirkey => $ourkey) {
- $tmp = WmfFramework::getRequestValue(
$theirkey, null );
- if ( !is_null( $tmp ) ) {
- $qsResults[$ourkey] = $tmp;
- }
- }
- }
+ $is_orphan = $this->isBatchProcessor();
+ if ( !$is_orphan ) {
+ // This was a normal front-end donation.
- $is_orphan = false;
- if ( count( $qsResults ) ){
- // Nothing unusual here. Oh, except we are reading
query parameters from
- // what we hope is a redirect back from the processor,
caused by an earlier
- // transaction.
- $this->addResponseData( $qsResults );
- $logmsg = 'CVV Result from querystring: ' .
$this->getData_Unstaged_Escaped( 'cvv_result' );
- $logmsg .= ', AVS Result from querystring: ' .
$this->getData_Unstaged_Escaped( 'avs_result' );
- $this->logger->info( $logmsg );
-
- // If we have a querystring, this means we're
processing a live donor
- // coming back from GlobalCollect, and the transaction
is not orphaned
// @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 {
- // We're in orphan processing mode, so instead of
waiting around for
- // the user to add more data and complete pending
transactions,
- // interpret this pending code range as "failed".
-
- $is_orphan = true;
+ // 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".
$this->addCodeRange( 'GET_ORDERSTATUS', 'STATUSID',
FinalStatus::FAILED, 0, 70 );
}
@@ -667,52 +649,17 @@
$status_response = null;
for ( $loops = 0; $loops < $loopcount && !$cancelflag &&
!$problemflag; ++$loops ){
- $gotCVV = false;
$status_result = $this->do_transaction(
'GET_ORDERSTATUS' );
$validationAction = $this->getValidationAction();
- // FIXME: Refactor as normal unstaging.
- $xmlResults = array(
- 'cvv_result' => '',
- 'avs_result' => ''
- );
- $status_response = $status_result->getData();
- if ( !empty( $status_response ) ) {
- foreach ( $pull_vars as $theirkey => $ourkey) {
- if ( !array_key_exists( $theirkey,
$status_response ) ) {
- continue;
- }
- $gotCVV = true;
- $xmlResults[$ourkey] =
$status_response[$theirkey];
- if ( array_key_exists( $ourkey,
$qsResults ) && $qsResults[$ourkey] != $xmlResults[$ourkey] ) {
- $problemflag = true;
- $problemmessage = "$theirkey
value '$qsResults[$ourkey]' from querystring does not match value
'$xmlResults[$ourkey]' from GET_ORDERSTATUS XML";
- }
- }
- // Make sure we're recording the right amounts,
in case donor has
- // opened another window and messed with their
session values
- // since our original INSERT_ORDERWITHPAYMENT.
The donor is
- // being charged the amount they intend to
give, so this isn't
- // a reason to fail the transaction.
- // Since we're adding these via
addResponseData, amount will be
- // divided by 100 in unstaging.
- // FIXME: need a general solution - anything
with a resultswitcher
- // is vulnerable to this kind of thing.
- $xmlResults['amount'] =
$status_response['AMOUNT'];
- $xmlResults['currency_code'] =
$status_response['CURRENCYCODE'];
- }
- $this->addResponseData( $xmlResults );
- $logmsg = 'CVV Result from XML: ' .
$this->getData_Unstaged_Escaped( 'cvv_result' );
- $logmsg .= ', AVS Result from XML: ' .
$this->getData_Unstaged_Escaped( 'avs_result' );
+ $gotCVV = !empty( $this->getData_Unstaged_Escaped(
'cvv_result' ) );
+ // TODO: This logging is redundant with the response
from GET_ORDERSTATUS.
+ $logmsg = 'CVV Result: ' .
$this->getData_Unstaged_Escaped( 'cvv_result' );
+ $logmsg .= ', AVS Result: ' .
$this->getData_Unstaged_Escaped( 'avs_result' );
$this->logger->info( $logmsg );
// FIXME: "isForceCancel"?
if ( $status_result->getForceCancel() ) {
$cancelflag = true; //don't retry or MasterCard
will fine us
- }
-
- if ( $is_orphan && !$cancelflag && !empty(
$status_response ) ) {
- $action = $this->findCodeAction(
'GET_ORDERSTATUS', 'STATUSID', $status_response['STATUSID'] );
- $validationAction =
$this->getValidationAction();
}
//we filtered
@@ -790,8 +737,10 @@
// FIXME: explicit that we want
to fall through?
case FinalStatus::PENDING :
- //if it's really pending at
this point, we need to...
- //...leave it alone. If we're
orphan slaying, this will stay in the queue.
+ // If it's really pending at
this point, we need to
+ // leave it alone.
+ // FIXME: If we're orphan
slaying, this should stay in
+ // the queue, but we currently
delete it.
break 2;
}
}
@@ -800,23 +749,6 @@
//if we got here with no problemflag,
//confirm or cancel the payment based on $cancelflag
if ( !$problemflag ){
- if ( is_array( $status_response ) ) {
- // FIXME: Refactor as normal unstaging.
- //if they're set, get CVVRESULT && AVSRESULT
- $pull_vars['EFFORTID'] = 'effort_id';
- $pull_vars['ATTEMPTID'] = 'attempt_id';
- $addme = array();
- foreach ( $pull_vars as $theirkey => $ourkey) {
- if ( array_key_exists( $theirkey,
$status_response ) ){
- $addme[$ourkey] =
$status_response[$theirkey];
- }
- }
-
- if ( count( $addme ) ){
- $this->addResponseData( $addme );
- }
- }
-
if ( !$cancelflag ) {
$final = $this->do_transaction( 'SET_PAYMENT' );
if ( $final->getCommunicationStatus() === true
) {
@@ -1478,6 +1410,15 @@
$retryVars
);
}
+
+ // Unstage any data that we've whitelisted.
+ // TODO: This should be generalized into the base class.
+ $whitelisted_keys = $this->transaction_option( 'response' );
+ if ( $whitelisted_keys ) {
+ $filtered_data = array_intersect_key( $data,
array_flip( $whitelisted_keys ) );
+ $unstaged = $this->unstageKeys( $filtered_data );
+ $this->addResponseData( $unstaged );
+ }
}
public function stageData() {
diff --git
a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanAdapterTest.php
b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanAdapterTest.php
index dc40bef..3de7a8d 100644
--- a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanAdapterTest.php
+++ b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanAdapterTest.php
@@ -170,24 +170,36 @@
}
/**
- * Don't fraud-fail someone for bad CVV if GET_ORDERSTATUS
- * comes back with STATUSID 25 and no CVVRESULT
- * @group CvvResult
+ * Make sure we're incorporating GET_ORDERSTATUS AVS and CVV responses
into
+ * fraud scores.
*/
- function testConfirmCreditCardStatus25() {
+ function testGetOrderstatusPostProcessFraud() {
+ $this->setMwGlobals( array(
+ 'wgDonationInterfaceEnableCustomFilters' => true,
+ 'wgGlobalCollectGatewayCustomFiltersFunctions' => array(
+ 'getCVVResult' => 10,
+ 'getAVSResult' => 30,
+ ),
+ ) );
$gateway = $this->getFreshGatewayObject( null, array (
'order_id_meta' => array ( 'generate' => FALSE ) ) );
$init = array_merge( $this->getDonorTestData(),
$this->dummy_utm_data );
$init['ffname'] = 'cc-vmad';
$init['order_id'] = '55555';
- $init['email'] = '[email protected]';
+ $init['email'] = '[email protected]';
$init['contribution_tracking_id'] = mt_rand();
+ $init['payment_method'] = 'cc';
$gateway->loadDataAndReInit( $init, $useDB = false );
- $gateway->setDummyGatewayResponseCode( '25' );
+ $gateway->setDummyGatewayResponseCode( '600_badCvv' );
$gateway->do_transaction( 'Confirm_CreditCard' );
$action = $gateway->getValidationAction();
- $this->assertEquals( 'process', $action, 'Gateway should not
fraud fail on STATUSID 25' );
+ $this->assertEquals( 'review', $action,
+ 'Orphan gateway should fraud fail on bad CVV and AVS' );
+
+ $exposed = TestingAccessWrapper::newFromObject( $gateway );
+ $this->assertEquals( 40, $exposed->risk_score,
+ 'Risk score was incremented correctly.' );
}
}
diff --git
a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanRectifierTest.php
b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanRectifierTest.php
index dd3c0f6..51095bb 100644
--- a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanRectifierTest.php
+++ b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectOrphanRectifierTest.php
@@ -77,9 +77,7 @@
$this->pendingDb = PendingDatabase::get();
// Create the schema.
- // FIXME: Reuse something from SmashPig\Tests
- $sql = file_get_contents( __DIR__ .
'/../../../../vendor/wikimedia/smash-pig/Schema/sqlite/001_CreatePendingTable.sql'
);
- $this->pendingDb->getDatabase()->exec( $sql );
+ $this->pendingDb->createTable();
}
/**
diff --git a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php
b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php
index c11b2c6..70c1ce9 100644
--- a/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php
+++ b/tests/phpunit/Adapter/GlobalCollect/GlobalCollectTest.php
@@ -194,26 +194,36 @@
}
/**
- * If CVVRESULT is unrecognized, fraud-fail and warn
- * @group CvvResult
+ * Make sure we're incorporating GET_ORDERSTATUS AVS and CVV responses
into
+ * fraud scores.
*/
- function testConfirmCreditCardBadCVVResult() {
+ function testGetOrderstatusPostProcessFraud() {
+ $this->setMwGlobals( array(
+ 'wgDonationInterfaceEnableCustomFilters' => true,
+ 'wgGlobalCollectGatewayCustomFiltersFunctions' => array(
+ 'getCVVResult' => 10,
+ 'getAVSResult' => 30,
+ ),
+ ) );
+
$init = $this->getDonorTestData();
+ $init['ffname'] = 'cc-vmad';
+ $init['order_id'] = '55555';
+ $init['email'] = '[email protected]';
+ $init['contribution_tracking_id'] = mt_rand();
$init['payment_method'] = 'cc';
- $init['payment_submethod'] = 'visa';
- $init['email'] = '[email protected]';
-
- $this->setUpRequest( array( 'CVVRESULT' => ' ' ) );
- DonationInterface_FraudFiltersTest::setupFraudMaps( $this );
-
$gateway = $this->getFreshGatewayObject( $init );
- $gateway->setDummyGatewayResponseCode( '800' );
+
+ $gateway->setDummyGatewayResponseCode( '600_badCvv' );
$gateway->do_transaction( 'Confirm_CreditCard' );
- $result = $gateway->getCvvResult();
- $this->assertEquals( false, $result, 'Gateway should fraud fail
if CVVRESULT is not mapped' );
- $matches = $this->getLogMatches( LogLevel::WARNING,
"/Unrecognized cvv_result ' '$/" );
- $this->assertNotEmpty( $matches, 'Did not log expected warning
on unmapped CVVRESULT' );
+ $action = $gateway->getValidationAction();
+ $this->assertEquals( 'review', $action,
+ 'Orphan gateway should fraud fail on bad CVV and AVS' );
+
+ $exposed = TestingAccessWrapper::newFromObject( $gateway );
+ $this->assertEquals( 40, $exposed->risk_score,
+ 'Risk score was incremented correctly.' );
}
/**
@@ -233,24 +243,6 @@
$gateway->do_transaction( 'Confirm_CreditCard' );
$this->assertEquals( 'N',
$gateway->getData_Unstaged_Escaped('cvv_result'), 'CVV Result not taken from
XML response' );
- }
-
- /**
- * If querystring and XML have different CVVRESULT, that's awfully fishy
- */
- function testConfirmCreditCardFailsOnCvvResultConflict() {
- $init = $this->getDonorTestData();
- $init['payment_method'] = 'cc';
- $init['payment_submethod'] = 'visa';
- $init['email'] = '[email protected]';
-
- $this->setUpRequest( array( 'CVVRESULT' => 'M' ) );
-
- $gateway = $this->getFreshGatewayObject( $init );
-
- $result = $gateway->do_transaction( 'Confirm_CreditCard' );
- // FIXME: this is not a communication failure, it's a fraud
failure
- $this->assertFalse( $result->getCommunicationStatus(), 'Credit
card should fail if querystring and XML have different CVVRESULT' );
}
/**
@@ -350,6 +342,8 @@
'SWIFTCODE' => 'swift_code',
'TRANSACTIONTYPE' => 'transaction_type',
'FISCALNUMBER' => 'fiscal_number',
+ 'AVSRESULT' => 'avs_result',
+ 'CVVRESULT' => 'cvv_result',
);
$exposed = TestingAccessWrapper::newFromObject( $gateway );
diff --git
a/tests/phpunit/includes/Responses/globalcollect/DO_FINISHPAYMENT_200.testresponse
b/tests/phpunit/includes/Responses/globalcollect/DO_FINISHPAYMENT_200.testresponse
index 6efeaf0..4da431e 100644
---
a/tests/phpunit/includes/Responses/globalcollect/DO_FINISHPAYMENT_200.testresponse
+++
b/tests/phpunit/includes/Responses/globalcollect/DO_FINISHPAYMENT_200.testresponse
@@ -22,7 +22,7 @@
<RESPONSEDATETIME>20140327165513</RESPONSEDATETIME>
</META>
<ROW>
- <AMOUNT>10101</AMOUNT>
+ <AMOUNT>2345</AMOUNT>
<CURRENCYCODE>EUR</CURRENCYCODE>
<STATUSID>800</STATUSID>
<STATUSDATE>20140704021814</STATUSDATE>
diff --git
a/tests/phpunit/includes/Responses/globalcollect/GET_ORDERSTATUS_600_badCvv.testresponse
b/tests/phpunit/includes/Responses/globalcollect/GET_ORDERSTATUS_600_badCvv.testresponse
new file mode 100644
index 0000000..7bc87f6
--- /dev/null
+++
b/tests/phpunit/includes/Responses/globalcollect/GET_ORDERSTATUS_600_badCvv.testresponse
@@ -0,0 +1,42 @@
+<XML>
+ <REQUEST>
+ <ACTION>GET_ORDERSTATUS</ACTION>
+ <META>
+ <MERCHANTID>1</MERCHANTID>
+ <IPADDRESS>123.123.123.123</IPADDRESS>
+ <VERSION>2.0</VERSION>
+ <REQUESTIPADDRESS>123.123.123.123</REQUESTIPADDRESS>
+ </META>
+ <PARAMS>
+ <ORDER>
+ <ORDERID>9998890004</ORDERID>
+ </ORDER>
+ </PARAMS>
+ <RESPONSE>
+ <RESULT>OK</RESULT>
+ <META>
+ <REQUESTID>245</REQUESTID>
+
<RESPONSEDATETIME>20100419133351</RESPONSEDATETIME>
+ </META>
+ <STATUS>
+ <PAYMENTMETHODID>1</PAYMENTMETHODID>
+ <STATUSID>600</STATUSID>
+ <CURRENCYCODE>EUR</CURRENCYCODE>
+ <FRAUDRESULT>N</FRAUDRESULT>
+ <EFFORTID>1</EFFORTID>
+
<CREDITCARDNUMBER>************7977</CREDITCARDNUMBER>
+ <AUTHORISATIONCODE>654321</AUTHORISATIONCODE>
+
<PAYMENTREFERENCE>900100000010</PAYMENTREFERENCE>
+ <ATTEMPTID>2</ATTEMPTID>
+ <MERCHANTID>1</MERCHANTID>
+ <AMOUNT>2345</AMOUNT>
+ <STATUSDATE>20100419132926</STATUSDATE>
+ <PAYMENTPRODUCTID>1</PAYMENTPRODUCTID>
+ <CVVRESULT>N</CVVRESULT>
+ <AVSRESULT>E</AVSRESULT>
+ <ORDERID>9998890004</ORDERID>
+ <EXPIRYDATE>1210</EXPIRYDATE>
+ </STATUS>
+ </RESPONSE>
+ </REQUEST>
+</XML>
diff --git
a/tests/phpunit/includes/Responses/globalcollect/SET_PAYMENT_200.testresponse
b/tests/phpunit/includes/Responses/globalcollect/SET_PAYMENT_200.testresponse
index a88c1d1..b721820 100644
---
a/tests/phpunit/includes/Responses/globalcollect/SET_PAYMENT_200.testresponse
+++
b/tests/phpunit/includes/Responses/globalcollect/SET_PAYMENT_200.testresponse
@@ -12,7 +12,7 @@
<ORDERID>626113410</ORDERID>
<EFFORTID>1</EFFORTID>
<PAYMENTPRODUCTID>1</PAYMENTPRODUCTID>
- <AMOUNT>10101</AMOUNT>
+ <AMOUNT>2345</AMOUNT>
<CURRENCYCODE>EUR</CURRENCYCODE>
<MERCHANTREFERENCE>626113410</MERCHANTREFERENCE>
</PAYMENT>
--
To view, visit https://gerrit.wikimedia.org/r/306852
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8e6d044f17cd3d875a3da93adf6303abbcd3c3b5
Gerrit-PatchSet: 18
Gerrit-Project: mediawiki/extensions/DonationInterface
Gerrit-Branch: master
Gerrit-Owner: Awight <[email protected]>
Gerrit-Reviewer: AndyRussG <[email protected]>
Gerrit-Reviewer: Awight <[email protected]>
Gerrit-Reviewer: Cdentinger <[email protected]>
Gerrit-Reviewer: Ejegg <[email protected]>
Gerrit-Reviewer: Ssmith <[email protected]>
Gerrit-Reviewer: XenoRyet <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
