r101960 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r101959‎ | r101960 | r101961 >
Date:05:41, 4 November 2011
Author:jpostlethwaite
Status:ok (Comments)
Tags:fundraising 
Comment:
Removing usage of $result and $data from the view. Enabling bank transfer, real time bank transfer and direct debit. See r101956.
Modified paths:
  • /trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect_gateway.body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect_gateway.body.php
——@@ -34,9 +34,7 @@
3535 * Show the special page
3636 *
3737 * @todo
38 - * - Add transaction type handler
39 - * - What should a failure on transaction_type issues do? log & message client
40 - * - Set up BANK_TRANSFER: Story #308
 38+ * - Finish error handling
4139 *
4240 * @param $par Mixed: parameter passed to the page or null
4341 */
——@@ -85,18 +83,20 @@
8684
8785 // dispatch forms/handling
8886 if ( $this->adapter->checkTokens() ) {
89 -
90 - //TODO: Get rid of $data out here completely, by putting this logic inside the adapter somewhere.
91 - //All we seem to be doing with it now, is internal adapter logic outside of the adapter.
92 - $data = $this->adapter->getDisplayData();
 87+
 88+ if ( $this->adapter->posted ) {
9389
94 - if ( $this->adapter->posted ) {
9590 // The form was submitted and the payment method has been set
9691 /*
9792 * The $payment_method should default to false.
9893 *
9994 * An invalid $payment_method will cause an error.
10095 */
 96+
 97+ //TODO: Get rid of $data out here completely, by putting this logic inside the adapter somewhere.
 98+ //All we seem to be doing with it now, is internal adapter logic outside of the adapter.
 99+ $data = $this->adapter->getDisplayData();
 100+
101101 $payment_method = ( isset( $data['payment_method'] ) && !empty( $data['payment_method'] ) ) ? $data['payment_method'] : 'cc';
102102 $payment_submethod = ( isset( $data['payment_submethod'] ) && !empty( $data['payment_submethod'] ) ) ? $data['payment_submethod'] : '';
103103
——@@ -113,14 +113,45 @@
114114 // allow any external validators to have their way with the data
115115 // Execute the proper transaction code:
116116
117 - $result = $this->adapter->do_transaction( 'INSERT_ORDERWITHPAYMENT' );
118 -
119 - $this->displayResultsForDebug( $result );
 117+ if ( $payment_method == 'cc' ) {
120118
121 - if ( $payment_method == 'cc' ) {
122 - $this->executeIframeForCreditCard( $result );
 119+ $this->adapter->do_transaction( 'INSERT_ORDERWITHPAYMENT' );
 120+
 121+ // Display an iframe for credit cards
 122+ if ( $this->executeIframeForCreditCard() ) {
 123+
 124+ // Nothing left to process
 125+ return;
 126+ }
123127 }
 128+ elseif ( $payment_method == 'rtbt' ) {
124129
 130+ $this->adapter->do_transaction( 'INSERT_ORDERWITHPAYMENT' );
 131+
 132+ $formAction = $this->adapter->getTransactionDataFormAction();
 133+
 134+ // Redirect to the bank
 135+ if ( !empty( $formAction ) ) {
 136+ return $wgOut->redirect( $formAction );
 137+ }
 138+
 139+ }
 140+ elseif ( $payment_method == 'dd' ) {
 141+
 142+ $this->adapter->do_transaction( 'DO_BANKVALIDATION' );
 143+
 144+ if ( $this->adapter->getTransactionStatus() ) {
 145+
 146+ $this->adapter->do_transaction( 'INSERT_ORDERWITHPAYMENT' );
 147+ }
 148+
 149+ }
 150+ else {
 151+ $this->adapter->do_transaction( 'INSERT_ORDERWITHPAYMENT' );
 152+ }
 153+
 154+ return $this->resultHandler();
 155+
125156 }
126157 } else {
127158 // Display form
——@@ -128,8 +159,8 @@
129160 // See GlobalCollectAdapter::stage_returnto()
130161 $oid = $wgRequest->getText( 'order_id' );
131162 if ( $oid ) {
132 - $result = $this->adapter->do_transaction( 'GET_ORDERSTATUS' );
133 - $this->displayResultsForDebug( $result );
 163+ $this->adapter->do_transaction( 'GET_ORDERSTATUS' );
 164+ $this->displayResultsForDebug();
134165 }
135166
136167 // If the result of the previous transaction was failure, set the retry message.
——@@ -151,33 +182,34 @@
152183 /**
153184 * Execute iframe for credit card
154185 *
155 - * @param array $result The result array from the gateway adapter
156 - *
157 - * @todo
158 - * - this needs to be moved out of @see GlobalCollectGateway and into the adapter.
 186+ * @return boolean Returns true if formaction exists for iframe.
159187 */
160 - public function executeIframeForCreditCard( $result ) {
 188+ protected function executeIframeForCreditCard() {
161189
162190 global $wgOut;
163191
164 - if ( !empty( $result['data'] ) ) {
 192+ $formAction = $this->adapter->getTransactionDataFormAction();
 193+
 194+ if ( $formAction ) {
165195
166 - if ( array_key_exists( 'FORMACTION', $result['data'] ) ) {
167 - $paymentFrame = Xml::openElement( 'iframe', array(
168 - 'id' => 'globalcollectframe',
169 - 'name' => 'globalcollectframe',
170 - 'width' => '680',
171 - 'height' => '300',
172 - 'frameborder' => '0',
173 - 'style' => 'display:block;',
174 - 'src' => $result['data']['FORMACTION']
175 - )
176 - );
177 - $paymentFrame .= Xml::closeElement( 'iframe' );
 196+ $paymentFrame = Xml::openElement( 'iframe', array(
 197+ 'id' => 'globalcollectframe',
 198+ 'name' => 'globalcollectframe',
 199+ 'width' => '680',
 200+ 'height' => '300',
 201+ 'frameborder' => '0',
 202+ 'style' => 'display:block;',
 203+ 'src' => $formAction,
 204+ )
 205+ );
 206+ $paymentFrame .= Xml::closeElement( 'iframe' );
178207
179 - $wgOut->addHTML( $paymentFrame );
180 - }
 208+ $wgOut->addHTML( $paymentFrame );
 209+
 210+ return true;
181211 }
 212+
 213+ return false;
182214 }
183215
184216 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r102236MFT r90286, r100671, r100837, r100950, r101060, r101063, r101064, r101073, r1......khorn03:06, 7 November 2011
r102237MFT r90286, r100671, r100837, r100950, r101060, r101063, r101064, r101073, r1......khorn03:07, 7 November 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r101956Added $error_map, getErrorMap(), getErrorMapByCodeAndTranslate(), getTransact...jpostlethwaite05:18, 4 November 2011

Comments

#Comment by Jpostlethwaite (talk | contribs)   17:17, 4 November 2011

The webitects form may be broken due to an API key and not because of these commits.

#Comment by Khorn (WMF) (talk | contribs)   21:34, 5 November 2011

Moving the $data back inside the controller, is extremely helpful. One question, though: Why did you remove $result? This is exactly the sort of place you'd want to use the return values of do_transaction, unless you've already moved all the logic for handling everything about that result to the inside of the adapter, and for some things, that might be more trouble than it's worth.

#Comment by Khorn (WMF) (talk | contribs)   23:17, 5 November 2011 
+					elseif ( $payment_method == 'dd' ) {
+
+						$this->adapter->do_transaction( 'DO_BANKVALIDATION' );
+
+						if ( $this->adapter->getTransactionStatus() ) {
+
+							$this->adapter->do_transaction( 'INSERT_ORDERWITHPAYMENT' );
+						}

getTransactionStatus only tells us if we have gotten a rational response form the remote server: It does not parse the result at all. Currently, this will submit the direct debit request if the remote server exists and your DO_BANKVALIDATION request is not completely malformed.

According to the documentation, you need to be pulling a node called "CHECKRESULT", parsing the value there, and acting on that result.

#Comment by Khorn (WMF) (talk | contribs)   04:45, 6 November 2011

Additionally, in the cases where two calls to do_transaction are necessary, there needs to be some level of error handling that takes into account the cases where GlobalCollect is down or broken. If GC is not responding properly, we should log it as a failure to even make the communication, and break out of the normal workflow.

#Comment by Awjrichards (talk | contribs)   21:32, 6 November 2011

Marking OK as this is a non-blocking issue. Jeremy, please note this and we'll get this sorted post launch.

Status & tagging log