jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/345790 )
Change subject: Cache directory lookup 404s
......................................................................
Cache directory lookup 404s
These are legitimate responses in REST-ese. Don't failmail when
someone tries to pay with iDEAL in an unsupported currency.
We should still route them elsewhere in the frontend, tho!
Bug: T161072
Change-Id: I28a1bc71ffc7a8982b686028ab53a558032de3c7
---
M Core/Http/HttpStatusValidator.php
M PaymentProviders/Ingenico/Api.php
M PaymentProviders/Ingenico/ApiException.php
M PaymentProviders/Ingenico/BankPaymentProvider.php
A PaymentProviders/Ingenico/RestResponseValidator.php
A PaymentProviders/Ingenico/Tests/Data/emptyDirectory.response
M PaymentProviders/Ingenico/Tests/phpunit/BankPaymentProviderTest.php
M SmashPig.yaml
8 files changed, 90 insertions(+), 13 deletions(-)
Approvals:
jenkins-bot: Verified
Awight: Looks good to me, approved
diff --git a/Core/Http/HttpStatusValidator.php
b/Core/Http/HttpStatusValidator.php
index a36230b..68c6859 100644
--- a/Core/Http/HttpStatusValidator.php
+++ b/Core/Http/HttpStatusValidator.php
@@ -12,16 +12,13 @@
public function shouldRetry( $parsedResponse ) {
$statusCode = $parsedResponse['status'];
+ if ( array_search( $statusCode, $this->getSuccessCodes() ) !==
false ) {
+ Logger::debug( "Successful request" );
+ return false;
+ }
$body = $parsedResponse['body'];
switch ( $statusCode ) {
- case 200: // Everything is AWESOME
- case 201: // Also fine, and we created a thing
- $continue = false;
-
- Logger::debug( "Successful request" );
- break;
-
case 400: // Oh noes! Bad request.. BAD CODE, BAD BAD
CODE!
$continue = false;
Logger::error( "Request returned (400) BAD
REQUEST: $body" );
@@ -39,4 +36,11 @@
}
return $continue;
}
+
+ protected function getSuccessCodes() {
+ return array(
+ 200, // Everything is AWESOME
+ 201 // Also fine, and we created a thing
+ );
+ }
}
diff --git a/PaymentProviders/Ingenico/Api.php
b/PaymentProviders/Ingenico/Api.php
index 5da77bb..3a7f0cc 100644
--- a/PaymentProviders/Ingenico/Api.php
+++ b/PaymentProviders/Ingenico/Api.php
@@ -84,7 +84,9 @@
$messages[] = "Error code {$error['code']}:
{$error['message']}.";
}
$concatenated = implode( ' ', $messages );
- throw new ApiException( $concatenated );
+ $ex = new ApiException( $concatenated );
+ $ex->setRawErrors( $decoded['errors'] );
+ throw $ex;
}
}
}
diff --git a/PaymentProviders/Ingenico/ApiException.php
b/PaymentProviders/Ingenico/ApiException.php
index 44af1fc..3f3ed73 100644
--- a/PaymentProviders/Ingenico/ApiException.php
+++ b/PaymentProviders/Ingenico/ApiException.php
@@ -6,4 +6,13 @@
class ApiException extends SmashPigException {
+ protected $rawErrors;
+
+ public function setRawErrors( $errors ) {
+ $this->rawErrors = $errors;
+ }
+
+ public function getRawErrors() {
+ return $this->rawErrors;
+ }
}
diff --git a/PaymentProviders/Ingenico/BankPaymentProvider.php
b/PaymentProviders/Ingenico/BankPaymentProvider.php
index 87c034d..0f5b237 100644
--- a/PaymentProviders/Ingenico/BankPaymentProvider.php
+++ b/PaymentProviders/Ingenico/BankPaymentProvider.php
@@ -2,8 +2,9 @@
namespace SmashPig\PaymentProviders\Ingenico;
-use SmashPig\Core\Context;
use Psr\Cache\CacheItemPoolInterface;
+use SmashPig\Core\Context;
+use Symfony\Component\HttpFoundation\Response;
/**
* Handle bank payments via Ingenico
@@ -50,12 +51,21 @@
'currencyCode' => $currency
);
$path = "products/$productId/directory";
- $response = $this->api->makeApiCall( $path, 'GET',
$query );
-
$banks = array();
- foreach ( $response['entries'] as $entry ) {
- $banks[$entry['issuerId']] =
$entry['issuerName'];
+ try {
+ $response = $this->api->makeApiCall( $path,
'GET', $query );
+
+ foreach ( $response['entries'] as $entry ) {
+ $banks[$entry['issuerId']] =
$entry['issuerName'];
+ }
+ } catch ( ApiException $ex ) {
+ $errors = $ex->getRawErrors();
+ if ( empty( $errors ) ||
$errors[0]['httpStatusCode'] !== Response::HTTP_NOT_FOUND ) {
+ throw $ex;
+ }
+ // If there is a single 404 error, that means
there is no directory info for
+ // the country/currency/product. That's
legitimate, so cache the empty array
}
$duration = $this->cacheParameters['duration'];
$cacheItem->set( array(
diff --git a/PaymentProviders/Ingenico/RestResponseValidator.php
b/PaymentProviders/Ingenico/RestResponseValidator.php
new file mode 100644
index 0000000..0a74d33
--- /dev/null
+++ b/PaymentProviders/Ingenico/RestResponseValidator.php
@@ -0,0 +1,12 @@
+<?php
+namespace SmashPig\PaymentProviders\Ingenico;
+
+use SmashPig\Core\Http\HttpStatusValidator;
+
+class RestResponseValidator extends HttpStatusValidator {
+ protected function getSuccessCodes() {
+ $codes = parent::getSuccessCodes();
+ $codes[] = '404'; // also a valid response in REST-ese
+ return $codes;
+ }
+}
diff --git a/PaymentProviders/Ingenico/Tests/Data/emptyDirectory.response
b/PaymentProviders/Ingenico/Tests/Data/emptyDirectory.response
new file mode 100644
index 0000000..984a385
--- /dev/null
+++ b/PaymentProviders/Ingenico/Tests/Data/emptyDirectory.response
@@ -0,0 +1,16 @@
+HTTP/1.1 404 Not Found
+Date: Mon, 27 Mar 2017 12:08:10 GMT
+Server: Apache/2.4.23 (Unix) OpenSSL/1.0.1t
+X-Powered-By: Servlet/3.0 JSP/2.2
+Transfer-Encoding: chunked
+Content-Type: application/json
+
+{
+ "errorId" : "2036059b-1138-42b0-8734-21d669a79d44",
+ "errors" : [ {
+ "code" : "210390",
+ "message" : "NO DIRECTORY FOUND",
+ "httpStatusCode" : 404
+ } ]
+}
+
diff --git
a/PaymentProviders/Ingenico/Tests/phpunit/BankPaymentProviderTest.php
b/PaymentProviders/Ingenico/Tests/phpunit/BankPaymentProviderTest.php
index d9a2e3e..6a47874 100644
--- a/PaymentProviders/Ingenico/Tests/phpunit/BankPaymentProviderTest.php
+++ b/PaymentProviders/Ingenico/Tests/phpunit/BankPaymentProviderTest.php
@@ -69,6 +69,21 @@
$this->assertEquals( $results, $cachedResults );
}
+ /**
+ * When the lookup returns 404 we should cache the emptiness
+ */
+ public function testCacheEmptyBankList() {
+ $this->setUpResponse( 'emptyDirectory', 404 );
+ $this->curlWrapper->expects( $this->once() )
+ ->method( 'execute' );
+ $results = $this->provider->getBankList( 'NL', 'COP' );
+ $this->assertEquals( array(), $results );
+ $cached = $this->cache->getItem( 'BLAH_BLAH_NL_COP_809' );
+ $this->assertTrue( $cached->isHit() );
+ $again = $this->provider->getBankList( 'NL', 'COP' );
+ $this->assertEquals( $results, $again );
+ }
+
public function testBustedCacheExpiration() {
$cacheItem = new HashCacheItem(
'BLAH_BLAH_NL_EUR_809',
@@ -91,6 +106,12 @@
);
}
+ /**
+ * @param string $filename Name of a file in ../Data representing a
+ * response (headers, blank line, body), which must use dos-style
+ * \r\n line endings.
+ * @param integer $statusCode
+ */
protected function setUpResponse( $filename, $statusCode ) {
$contents = file_get_contents( __DIR__ .
"/../Data/$filename.response" );
$parsed = CurlWrapper::parseResponse(
diff --git a/SmashPig.yaml b/SmashPig.yaml
index 19e2f1b..ac7159b 100644
--- a/SmashPig.yaml
+++ b/SmashPig.yaml
@@ -477,6 +477,9 @@
key: SMASHPIG_IDEAL_BANK_STATUS
availability-url:
https://availability.ideal.nl/api/api/GetIssuers
+ curl:
+ validator:
+ class: SmashPig\PaymentProviders\Ingenico\RestResponseValidator
# deprecated, delete when projects using SmashPig rename adaptors
globalcollect:
--
To view, visit https://gerrit.wikimedia.org/r/345790
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I28a1bc71ffc7a8982b686028ab53a558032de3c7
Gerrit-PatchSet: 3
Gerrit-Project: wikimedia/fundraising/SmashPig
Gerrit-Branch: master
Gerrit-Owner: Ejegg <[email protected]>
Gerrit-Reviewer: Awight <[email protected]>
Gerrit-Reviewer: Cdentinger <[email protected]>
Gerrit-Reviewer: Ejegg <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
