r100648 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r100647‎ | r100648 | r100649 >
Date:21:32, 24 October 2011
Author:jpostlethwaite
Status:resolved (Comments)
Tags:
Comment:
Fixed the issues with credit cards for r100643.
Modified paths:
  • /trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect.adapter.php (modified) (history)
  • /trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect_gateway.body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect_gateway.body.php
——@@ -92,26 +92,19 @@
9393 // The form was submitted and the payment method has been set
9494 $this->adapter->log( "Form posted and payment method set." );
9595
96 - /* Commenting out because this is completely breaking Credit Card in GC.
97 - * Under usual circumstances, that would be an automatic revert, but
98 - * there were no small number of clean places to do that.
99 - **/
 96+ /*
 97+ * The $payment_method should default to false.
 98+ *
 99+ * An invalid $payment_method will cause an error.
 100+ */
 101+ $payment_method = ( isset( $data['payment_method'] ) && !empty( $data['payment_method'] ) ) ? $data['payment_method'] : 'cc';
 102+ $payment_submethod = ( isset( $data['payment_submethod'] ) && !empty( $data['payment_submethod'] ) ) ? $data['payment_submethod'] : '';
 103+
 104+ $payment_submethodMeta = $this->adapter->getPaymentSubmethodMeta( $payment_submethod, array( 'log' => true, ) );
100105
101 -// /*
102 -// * The $payment_method should default to false.
103 -// *
104 -// * An invalid $payment_method will cause an error.
105 -// */
106 -// $payment_method = ( isset( $data['payment_method'] ) && !empty( $data['payment_method'] ) ) ? $data['payment_method'] : false;
107 -// $payment_submethod = ( isset( $data['payment_submethod'] ) && !empty( $data['payment_submethod'] ) ) ? $data['payment_submethod'] : false;
108 -//
109 -// $payment_submethodMeta = $this->adapter->getPaymentSubmethodMeta( $payment_submethod, array( 'log' => true, ) );
110 -//
111 -// // Check form for errors
112 -// $form_errors = $this->validateForm( $data, $this->errors, $payment_submethodMeta['validation'] );
 106+ // Check form for errors
 107+ $form_errors = $this->validateForm( $data, $this->errors, $payment_submethodMeta['validation'] );
113108
114 - $form_errors = $this->validateForm( $data, $this->errors, array( 'address', 'amount', 'creditCard', 'email', 'name' ) );
115 -
116109 // If there were errors, redisplay form, otherwise proceed to next step
117110 if ( $form_errors ) {
118111
——@@ -124,10 +117,10 @@
125118
126119 $this->displayResultsForDebug( $result );
127120
128 - //if ( $payment_method == 'credit' ) {
 121+ if ( $payment_method == 'cc' ) {
129122
130 - $this->executeIframeForCreditCard( $result );
131 - //}
 123+ $this->executeIframeForCreditCard( $result );
 124+ }
132125
133126
134127 //TODO: add all the hooks back in.
Index: trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect.adapter.php
——@@ -184,16 +184,26 @@
185185
186186 /**
187187 * Define payment methods
 188+ *
 189+ * The credit card group has a catchall for unspecified payment types.
188190 */
189191 protected function definePaymentMethods() {
190192
191193 $this->payment_methods = array();
192194
 195+ // Credit Cards
 196+ $this->payment_methods['cc'] = array(
 197+ 'label' => 'Credit Cards',
 198+ 'types' => array( '', 'visa', 'mc', 'amex', 'discover', 'maestro', 'solo', 'laser', 'jcb,', 'cb', ),
 199+ );
 200+
 201+ // Real Time Bank Transfers
193202 $this->payment_methods['rtbt'] = array(
194203 'label' => 'Real time bank transfer',
195 - 'types' => array( 'rtbt_ideal', 'rtbt_eps', 'rtbt_sofortuberweisung', 'rtbt_nordea_sweeden', 'rtbt_enets', ),
 204+ 'types' => array( 'rtbt_ideal', 'rtbt_eps', 'rtbt_sofortuberweisung', 'rtbt_nordea_sweeden', 'rtbt_enets', ),
196205 );
197206
 207+ // Bank Transfers
198208 $this->payment_methods['bt'] = array(
199209 'label' => 'Bank transfer',
200210 'types' => array( 'bt', ),
——@@ -211,6 +221,94 @@
212222 $this->payment_submethods = array();
213223
214224 /*
 225+ * Credit Card
 226+ */
 227+
 228+ // None specified - This is a catchall to validate all options for credit cards.
 229+ $this->payment_submethods[''] = array(
 230+ 'paymentproductid' => 0,
 231+ 'label' => 'Any',
 232+ 'group' => 'cc',
 233+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 234+ );
 235+
 236+ /*
 237+ * Credit Card
 238+ */
 239+
 240+ // Visa
 241+ $this->payment_submethods['visa'] = array(
 242+ 'paymentproductid' => 1,
 243+ 'label' => 'Visa',
 244+ 'group' => 'cc',
 245+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 246+ );
 247+
 248+ // MasterCard
 249+ $this->payment_submethods['mc'] = array(
 250+ 'paymentproductid' => 3,
 251+ 'label' => 'MasterCard',
 252+ 'group' => 'cc',
 253+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 254+ );
 255+
 256+ // American Express
 257+ $this->payment_submethods['amex'] = array(
 258+ 'paymentproductid' => 2,
 259+ 'label' => 'American Express',
 260+ 'group' => 'cc',
 261+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 262+ );
 263+
 264+ // Maestro
 265+ $this->payment_submethods['maestro'] = array(
 266+ 'paymentproductid' => 117,
 267+ 'label' => 'Maestro',
 268+ 'group' => 'cc',
 269+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 270+ );
 271+
 272+ // Solo
 273+ $this->payment_submethods['solo'] = array(
 274+ 'paymentproductid' => 118,
 275+ 'label' => 'Solo',
 276+ 'group' => 'cc',
 277+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 278+ );
 279+
 280+ // Laser
 281+ $this->payment_submethods['laser'] = array(
 282+ 'paymentproductid' => 124,
 283+ 'label' => 'Laser',
 284+ 'group' => 'cc',
 285+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 286+ );
 287+
 288+ // JCB
 289+ $this->payment_submethods['jcb'] = array(
 290+ 'paymentproductid' => 125,
 291+ 'label' => 'JCB',
 292+ 'group' => 'cc',
 293+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 294+ );
 295+
 296+ // Discover
 297+ $this->payment_submethods['discover'] = array(
 298+ 'paymentproductid' => 128,
 299+ 'label' => 'Discover',
 300+ 'group' => 'cc',
 301+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 302+ );
 303+
 304+ // CB
 305+ $this->payment_submethods['cb'] = array(
 306+ 'paymentproductid' => 130,
 307+ 'label' => 'CB', // Carte Bancaire OR Carte Bleue
 308+ 'group' => 'cc',
 309+ 'validation' => array( 'address' => true, 'amount' => true, 'creditCard' => true, 'email' => true, 'name' => true, ),
 310+ );
 311+
 312+ /*
215313 * Bank transfers
216314 */
217315
——@@ -325,32 +423,27 @@
326424 */
327425 public function getPaymentSubmethodMeta( $payment_submethod, $options = array() ) {
328426
329 - /* Commenting out because this is completely breaking Credit Card in GC.
330 - * Under usual circumstances, that would be an automatic revert, but
331 - * there were no small number of clean places to do that.
332 - **/
 427+ extract( $options );
333428
334 -// extract( $options );
335 -//
336 -// $log = isset( $log ) ? (boolean) $log : false ;
337 -//
338 -// if ( isset( $this->payment_submethods[ $payment_submethod ] ) ) {
339 -//
340 -// if ( $log ) {
341 -// $this->log( 'Getting payment submethod: ' . ( string ) $payment_submethod );
342 -// }
343 -//
344 -// // Ensure that the validation index is set.
345 -// if ( !isset( $this->payment_submethods[ $payment_submethod ]['validation'] ) ) {
346 -// $this->payment_submethods[ $payment_submethod ]['validation'] = array();
347 -// }
348 -//
349 -// return $this->payment_submethods[ $payment_submethod ];
350 -// }
351 -// else {
352 -// $message = 'The payment submethod [ ' . $payment_submethod . ' ] was not found.';
353 -// throw new Exception( $message );
354 -// }
 429+ $log = isset( $log ) ? (boolean) $log : false ;
 430+
 431+ if ( isset( $this->payment_submethods[ $payment_submethod ] ) ) {
 432+
 433+ if ( $log ) {
 434+ $this->log( 'Getting payment submethod: ' . ( string ) $payment_submethod );
 435+ }
 436+
 437+ // Ensure that the validation index is set.
 438+ if ( !isset( $this->payment_submethods[ $payment_submethod ]['validation'] ) ) {
 439+ $this->payment_submethods[ $payment_submethod ]['validation'] = array();
 440+ }
 441+
 442+ return $this->payment_submethods[ $payment_submethod ];
 443+ }
 444+ else {
 445+ $message = 'The payment submethod [ ' . $payment_submethod . ' ] was not found.';
 446+ throw new Exception( $message );
 447+ }
355448 }
356449
357450 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r100666A bare-minimum fix to get GC credit card forms back off the ground, with a sl...khorn23:59, 24 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r100411Removed BANK_TRANSFER as a transaction. Set it as a transaction type. Added t...jpostlethwaite04:52, 21 October 2011
r100461Changed $transaction_groups to $payment_methods. Changed $transaction_types t...jpostlethwaite21:59, 21 October 2011
r100462Changed $transactionType to $payment_method. Added $result parameter to execu...jpostlethwaite22:00, 21 October 2011
r100643Commenting out parts of r100462, r100461, r100411, in order to get CC in Glob...khorn20:07, 24 October 2011

Comments

#Comment by Khorn (WMF) (talk | contribs)   22:44, 24 October 2011

Credit Card in GlobalCollect is still misbehaving. It should not validate Credit Card info beyond there being a card type, as we haven't collected it through the iframe yet.

#Comment by Jpostlethwaite (talk | contribs)   23:18, 24 October 2011

That makes sense.

So does that mean we will never have to validate a credit card, because they will never be entered on our form?

#Comment by Jpostlethwaite (talk | contribs)   23:20, 24 October 2011

I was testing by putting in the full credit card info on our end, which is why it was working for me.

#Comment by Jpostlethwaite (talk | contribs)   23:22, 24 October 2011

What options should we require to validate for credit cards?

- address - amount - email - name

#Comment by Khorn (WMF) (talk | contribs)   16:21, 25 October 2011

The iframe on the second page, which is hosted by GlobalCollect, collects card number, expiration date, and cvv. Another way to look at this is: We should only be validating the fields we're sending over to them, as defined in the specific transaction we're doing.

For this reason (ease of lookup), I wonder if we don't want to just add the sections that need to be validated, to the transaction definition itself. That way, it's all in one place, and you don't have to bounce around so much cross-referencing definitions.

#Comment by Jpostlethwaite (talk | contribs)   02:43, 27 October 2011

I am doing this on the javascript:

Validating by payment_method and/or payment_submethod.

That does seem to be a more natural way to do it.

#Comment by Khorn (WMF) (talk | contribs)   16:23, 25 October 2011

Oops: I should be more clear here. We should _not_ be validating on the fields that GC is hosting (card number, expiry, cvv). Everything else is cool to validate. ...if it doesn't care if those sections are omitted from the form entirely.

#Comment by Jpostlethwaite (talk | contribs)   20:39, 5 November 2011

The default credit card form for GlobalCollect is no longer broken.

This is no longer an issue.

Status & tagging log