Awight has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/341727 )
Change subject: [WIP] Deprecate "manual" errors
......................................................................
[WIP] Deprecate "manual" errors
The original intent may have been a firewall between the validation errors
which can only be set by the adapter, and errors which can be set by other
classes.
I'm simplifying down to a single list of errors. If any errors are present,
you will not advance to the next step in your donation.
Change-Id: I279052521a20e28d0935ce100c31087a2519e365
---
M amazon_gateway/amazon.api.php
M gateway_common/DonationData.php
M gateway_common/GatewayPage.php
M gateway_common/donation.api.php
M gateway_common/gateway.adapter.php
M gateway_forms/Mustache.php
M globalcollect_gateway/orphan.adapter.php
M tests/phpunit/Adapter/AstroPay/AstroPayTest.php
M tests/phpunit/GatewayPageTest.php
M tests/phpunit/GatewayValidationTest.php
M tests/phpunit/includes/test_gateway/TestingGenericAdapter.php
11 files changed, 82 insertions(+), 97 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/DonationInterface
refs/changes/27/341727/1
diff --git a/amazon_gateway/amazon.api.php b/amazon_gateway/amazon.api.php
index 07ad771..c82a956 100644
--- a/amazon_gateway/amazon.api.php
+++ b/amazon_gateway/amazon.api.php
@@ -25,11 +25,11 @@
$adapter = new AmazonAdapter( $adapterParams );
- if ( $adapter->getAllErrors() ) {
+ if ( $adapter->getErrors() ) {
$output->addValue(
null,
'errors',
- $adapter->getAllErrors()
+ $adapter->getErrors()
);
} else if ( $token && $adapter->checkTokens() ) {
if ( $recurring ) {
diff --git a/gateway_common/DonationData.php b/gateway_common/DonationData.php
index 07c6d43..4983ab1 100644
--- a/gateway_common/DonationData.php
+++ b/gateway_common/DonationData.php
@@ -1088,7 +1088,7 @@
$this->getVal( 'language' ),
array( $this->gateway->getGlobal(
'FallbackCurrency' ) )
);
- $this->gateway->addManualError( $error );
+ $this->gateway->mergeError( $error );
}
}
}
diff --git a/gateway_common/GatewayPage.php b/gateway_common/GatewayPage.php
index 8d32960..f4ce1c7 100644
--- a/gateway_common/GatewayPage.php
+++ b/gateway_common/GatewayPage.php
@@ -163,14 +163,13 @@
* a piece of mostly UI, this function needs to be moved inside the
gateway
* adapter class.
*
- * @return boolean Returns false on an error-free validation, otherwise
true.
- * FIXME: that return value seems backwards to me.
+ * @return boolean Returns true on an error-free validation, otherwise
false.
*/
public function validateForm() {
- $validated_ok = $this->adapter->revalidate();
+ $validated_ok = $this->adapter->validate();
- return !$validated_ok;
+ return $validated_ok;
}
/**
@@ -348,14 +347,16 @@
if ( $this->isProcessImmediate() ) {
// Check form for errors
// FIXME: Should this be rolled into
adapter.doPayment?
- $form_errors = $this->validateForm() ||
$this->adapter->getManualErrors();
+ $validated_ok = $this->validateForm();
- // If there were errors, redisplay form,
otherwise proceed to next step
- if ( $form_errors ) {
- $this->displayForm();
- } else {
- // Attempt to process the payment, and
render the response.
+ // Proceed to the next step, unless there were
errors.
+ if ( $validated_ok ) {
+ // Attempt to process the payment, then
render the response.
$this->processPayment();
+ } else {
+ // Redisplay form to give the donor
notification and a
+ // chance correct their errors.
+ $this->displayForm();
}
} else {
$this->adapter->session_addDonorData();
@@ -363,7 +364,7 @@
}
} else { //token mismatch
$error['general']['token-mismatch'] = $this->msg(
'donate_interface-token-mismatch' );
- $this->adapter->addManualError( $error );
+ $this->adapter->mergeError( $error );
$this->displayForm();
}
}
@@ -510,7 +511,7 @@
else {
$error['general'][ $code ] = $message;
}
- $this->adapter->addManualError( $error );
+ $this->adapter->mergeError( $error );
}
$this->displayForm();
} else {
diff --git a/gateway_common/donation.api.php b/gateway_common/donation.api.php
index d560699..ab9bc1f 100644
--- a/gateway_common/donation.api.php
+++ b/gateway_common/donation.api.php
@@ -27,9 +27,10 @@
return; // already failed with a dieUsage call
}
- $gatewayObj->revalidate();
- if ( $gatewayObj->getAllErrors() ) {
- $outputResult['errors'] = $gatewayObj->getAllErrors();
+ $validated_ok = $gatewayObj->validate();
+ if ( !$validated_ok ) {
+ $outputResult['errors'] = $gatewayObj->getErrors();
+ // FIXME: What is this junk? Smaller API, like
getResult()->addErrors
$this->getResult()->setIndexedTagName(
$outputResult['errors'], 'error' );
$this->getResult()->addValue( null, 'result',
$outputResult );
return;
diff --git a/gateway_common/gateway.adapter.php
b/gateway_common/gateway.adapter.php
index fe1cc9a..b6f87ca 100644
--- a/gateway_common/gateway.adapter.php
+++ b/gateway_common/gateway.adapter.php
@@ -53,7 +53,7 @@
protected $dataConstraints = array();
/**
- * $error_map maps gateway errors to client errors
+ * $error_map Reference map from gateway error to client error.
*
* The key of each error should map to a i18n message key or a callable
* By convention, the following three keys have these meanings:
@@ -248,9 +248,10 @@
$this->session_resetOnSwitch(); // Need to do this before
creating DonationData
// FIXME: this should not have side effects like setting
order_id_meta['final']
+ // TODO: On second thought, neither set data nor validate here.
$this->dataObj = new DonationData( $this,
$options['external_data'] );
- $this->setValidationErrors(
$this->getOriginalValidationErrors() );
+ $this->errors = $this->dataObj->getValidationErrors();
$this->unstaged_data = $this->dataObj->getDataEscaped();
$this->staged_data = $this->unstaged_data;
@@ -276,7 +277,7 @@
$error = array( 'general' => array( 'internal-0001' =>
$this->getErrorMapByCodeAndTranslate(
'internal-0001' )
) );
- $this->addManualError( $error );
+ $this->mergeError( $error );
}
}
@@ -910,11 +911,12 @@
$return = new PaymentTransactionResponse();
$return->setCommunicationStatus( false );
$return->setMessage( 'Failed data validation' );
- foreach ( $this->getAllErrors() as $code => $error ) {
+ foreach ( $this->errors as $code => $error ) {
+ // TODO: Error should already be in a native
format.
$return->addError( $code, array( 'message' =>
$error, 'logLevel' => LogLevel::INFO, 'debugInfo' => '' ) );
}
// TODO: should we set $this->transaction_response ?
- $this->logger->info( "Failed Validation. Aborting
$transaction " . print_r( $this->getValidationErrors(), true ) );
+ $this->logger->info( "Failed Validation. Aborting
$transaction " . print_r( $this->errors, true ) );
return $return;
}
@@ -1481,6 +1483,8 @@
/**
* Parse the response to get the errors in a format we can log and
otherwise deal with.
* @return array a key/value array of codes (if they exist) and
messages.
+ * TODO: Move to a parsing class, where these are part of an interface
+ * rather than empty although non-abstract.
*/
protected function parseResponseErrors( $response ) {
return array();
@@ -2148,46 +2152,24 @@
return get_called_class();
}
- //only the gateway should be setting validation errors. Everybody else
should set manual errors.
- protected function setValidationErrors( $errors ) {
- $this->validation_errors = $errors;
+ /**
+ * Return list of all errors that prevent this transaction from
continuing.
+ */
+ public function getErrors() {
+ return $this->errors;
}
- public function getValidationErrors() {
- if ( !empty( $this->validation_errors ) ) {
- return $this->validation_errors;
- } else {
- return false;
- }
- }
-
- public function addManualError( $errors, $reset = false ) {
- if ( $reset ){
- $this->manual_errors = array();
- return;
- }
- $this->manual_errors = array_merge( $this->manual_errors,
$errors );
- }
-
- public function getManualErrors() {
- if ( !empty( $this->manual_errors ) ) {
- return $this->manual_errors;
- } else {
- return false;
- }
- }
-
- public function getAllErrors(){
- $validation = $this->getValidationErrors();
- $manual = $this->getManualErrors();
- $return = array();
- if ( is_array( $validation ) ){
- $return = array_merge( $return, $validation );
- }
- if ( is_array( $manual ) ){
- $return = array_merge( $return, $manual );
- }
- return $return;
+ /**
+ * Add errors those already stored.
+ *
+ * @param array $errors Map from field or field group key to error
string.
+ * Merged by key rather than as a list, hence the awkward plural.
+ *
+ * TODO: Encapsulate errors as objects. Provide both merge list and
append item to list methods?
+ * FIXME: Nasty that array_merge overwrites rather than appending. Fix
while encapsulating the list.
+ */
+ public function mergeError( $errors ) {
+ $this->errors = array_merge( $this->errors, $errors );
}
/**
@@ -2373,10 +2355,6 @@
return $this->batch;
}
- public function getOriginalValidationErrors() {
- return $this->dataObj->getValidationErrors();
- }
-
/**
* Build list of required fields
* TODO: Determine if this ever needs to be overridden per gateway, or
if
@@ -2465,19 +2443,23 @@
*
* TODO: Maybe validate on $unstaged_data directly?
*/
- public function revalidate() {
+ public function validate() {
$check_not_empty = $this->getRequiredFields();
+ // This is where the gateway adds its own validations.
+ // TODO: Something with transformers?
- $validation_errors = $this->dataObj->getValidationErrors( true,
$check_not_empty );
- $this->setValidationErrors( $validation_errors );
+ // FIXME: We're relying on side effects. Call an obvious
action like dataObj->validate() instead.
+ $this->dataObj->getValidationErrors( true, $check_not_empty );
+
return $this->validatedOK();
}
+ /**
+ * @return boolean True if submitted data is valid and sufficient to
proceed to the next step.
+ * TODO: Were we also trying to indicate whether the validation step
has succeeded here, by distinguishing array() != false?
+ */
public function validatedOK(){
- if ( $this->getValidationErrors() === false ){
- return true;
- }
- return false;
+ return !$this->errors;
}
/**
diff --git a/gateway_forms/Mustache.php b/gateway_forms/Mustache.php
index 7101c55..6629553 100644
--- a/gateway_forms/Mustache.php
+++ b/gateway_forms/Mustache.php
@@ -250,7 +250,7 @@
* @return array
*/
protected function getErrors() {
- $errors = $this->gateway->getAllErrors();
+ $errors = $this->gateway->getErrors();
$return = array( 'errors' => array(
'general' => array(),
'field' => array(),
diff --git a/globalcollect_gateway/orphan.adapter.php
b/globalcollect_gateway/orphan.adapter.php
index eb85ba8..c1edf13 100644
--- a/globalcollect_gateway/orphan.adapter.php
+++ b/globalcollect_gateway/orphan.adapter.php
@@ -81,7 +81,7 @@
//have to do this again here.
$this->reAddHardData();
- $this->revalidate();
+ $this->validate();
}
public function addRequestData( $dataArray ) {
diff --git a/tests/phpunit/Adapter/AstroPay/AstroPayTest.php
b/tests/phpunit/Adapter/AstroPay/AstroPayTest.php
index 7af0d9c..90a7dbc 100644
--- a/tests/phpunit/Adapter/AstroPay/AstroPayTest.php
+++ b/tests/phpunit/Adapter/AstroPay/AstroPayTest.php
@@ -521,7 +521,7 @@
$init['currency_code'] = 'CLP';
$gateway = $this->getFreshGatewayObject( $init );
- $errors = $gateway->getValidationErrors();
+ $errors = $gateway->getErrors();
$this->assertNotEmpty( $errors );
$this->assertTrue(
diff --git a/tests/phpunit/GatewayPageTest.php
b/tests/phpunit/GatewayPageTest.php
index 27edc9c..48df283 100644
--- a/tests/phpunit/GatewayPageTest.php
+++ b/tests/phpunit/GatewayPageTest.php
@@ -81,9 +81,9 @@
$this->assertTrue( $this->adapter->validatedOK() );
- $manualErrors = $this->adapter->getManualErrors();
+ $errors = $this->adapter->getErrors();
$msg = $this->page->msg(
'donate_interface-fallback-currency-notice', 'USD' )->text();
- $this->assertEquals( $msg, $manualErrors['general'] );
+ $this->assertEquals( $msg, $errors['general'] );
$this->assertEquals( 100,
$this->adapter->getData_Unstaged_Escaped( 'amount' ) );
$this->assertEquals( 'USD',
$this->adapter->getData_Unstaged_Escaped( 'currency_code' ) );
}
@@ -97,9 +97,9 @@
$this->page->validateForm();
- $manualErrors = $this->adapter->getManualErrors();
+ $errors = $this->adapter->getErrors();
$msg = $this->page->msg(
'donate_interface-fallback-currency-notice', 'OMR' )->text();
- $this->assertEquals( $msg, $manualErrors['general'] );
+ $this->assertEquals( $msg, $errors['general'] );
$this->assertEquals( 38,
$this->adapter->getData_Unstaged_Escaped( 'amount' ) );
$this->assertEquals( 'OMR',
$this->adapter->getData_Unstaged_Escaped( 'currency_code' ) );
}
@@ -112,8 +112,8 @@
$this->assertTrue( $this->adapter->validatedOK() );
- $manualErrors = $this->adapter->getManualErrors();
- $this->assertEquals( null, $manualErrors['general'] );
+ $errors = $this->adapter->getErrors();
+ $this->assertEquals( null, $errors['general'] );
$this->assertEquals( 100,
$this->adapter->getData_Unstaged_Escaped( 'amount' ) );
$this->assertEquals( 'USD',
$this->adapter->getData_Unstaged_Escaped( 'currency_code' ) );
}
@@ -124,9 +124,9 @@
$this->page->validateForm();
- $manualErrors = $this->adapter->getManualErrors();
+ $errors = $this->adapter->getErrors();
$msg = $this->page->msg(
'donate_interface-fallback-currency-notice', 'USD' )->text();
- $this->assertEquals( $msg, $manualErrors['general'] );
+ $this->assertEquals( $msg, $errors['general'] );
$this->assertEquals( 100,
$this->adapter->getData_Unstaged_Escaped( 'amount' ) );
$this->assertEquals( 'USD',
$this->adapter->getData_Unstaged_Escaped( 'currency_code' ) );
}
@@ -137,8 +137,8 @@
$this->page->validateForm();
- $manualErrors = $this->adapter->getManualErrors();
- $this->assertEquals( null, $manualErrors['general'] );
+ $errors = $this->adapter->getErrors();
+ $this->assertEquals( null, $errors['general'] );
$this->assertEquals( 200,
$this->adapter->getData_Unstaged_Escaped( 'amount' ) );
$this->assertEquals( 'BBD',
$this->adapter->getData_Unstaged_Escaped( 'currency_code' ) );
}
diff --git a/tests/phpunit/GatewayValidationTest.php
b/tests/phpunit/GatewayValidationTest.php
index 77d9444..e009026 100644
--- a/tests/phpunit/GatewayValidationTest.php
+++ b/tests/phpunit/GatewayValidationTest.php
@@ -75,7 +75,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'amount', $errors );
}
@@ -90,7 +90,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'amount', $errors );
}
@@ -105,7 +105,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'currency_code', $errors );
}
@@ -126,7 +126,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'country', $errors );
}
@@ -141,7 +141,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'email', $errors );
}
@@ -156,7 +156,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'fname', $errors );
}
@@ -169,7 +169,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'currency_code', $errors );
}
@@ -191,7 +191,7 @@
$this->assertFalse( $this->adapter->validatedOK() );
- $errors = $this->adapter->getValidationErrors();
+ $errors = $this->adapter->getErrors();
$this->assertArrayHasKey( 'general', $errors );
}
}
diff --git a/tests/phpunit/includes/test_gateway/TestingGenericAdapter.php
b/tests/phpunit/includes/test_gateway/TestingGenericAdapter.php
index e010e82..c00a5b5 100644
--- a/tests/phpunit/includes/test_gateway/TestingGenericAdapter.php
+++ b/tests/phpunit/includes/test_gateway/TestingGenericAdapter.php
@@ -22,7 +22,8 @@
class TestingGenericAdapter extends GatewayAdapter {
/**
- * A list of fake errors that is returned each time revalidate() is
called.
+ * A list of fake errors that is returned each time validate() is
called.
+ * FIXME: s/re/validate
*/
public $errorsForRevalidate = array();
@@ -37,16 +38,16 @@
return 'xml';
}
- public function revalidate($check_not_empty = array()) {
+ public function validate($check_not_empty = array()) {
if ( !empty( $this->errorsForRevalidate ) ) {
$fakeErrors =
$this->errorsForRevalidate[$this->revalidateCount];
if ( $fakeErrors !== null ) {
$this->revalidateCount++;
- $this->setValidationErrors( $fakeErrors );
+ $this->mergeError( $fakeErrors );
return empty( $fakeErrors );
}
}
- return parent::revalidate($check_not_empty);
+ return parent::validate($check_not_empty);
}
public function normalizeOrderID( $override = null, $dataObj = null ) {
--
To view, visit https://gerrit.wikimedia.org/r/341727
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I279052521a20e28d0935ce100c31087a2519e365
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/DonationInterface
Gerrit-Branch: master
Gerrit-Owner: Awight <awi...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
