CRM-21311 fix handling of credit cards where Amex has been given a different label #12615

eileenmcnaughton · 2018-08-02T11:22:41Z

Overview

Proposed replacement for #11137

Credit card type is unset on submission causing credit card payment to fail with CVV validation error message. Regression bug introduced in PR #10774

Before

In CiviCRM, change default credit card name for "Amex" to "American Express".
Submit a test credit card transaction using an American Express test credit card number, 378282246310005
Contribution will fail with CVV validation error message.
Existing code does not set the credit card type on submission, causing the CVV not to be validated correctly. American Express uses 4 digit CVV.

After

Credit card type is set correctly, CVV is validated and submission succeeds.

In addition correct label displayed on Confirm and Thank you screens

Technical Details

We have been following a strange pattern where we take the label of a credit card field and munge it into
a css compatible string and then expect that to be a match with an array based on lcasing the
name field for the credit card. This means that if label !== name the house of cards falls.

This is particularly a problem for Amex which needs to be set correctly for the
4 digit cvv not to trigger a validation fail. Notably Amex is probably also the most likely for
people to want to rename it since Amex is an abbreviation.

This PR backs out of all of that and assigns an array of data to js - including the pattern itself
and then updates the js script to refer to the right variable name for each purpose and to stop munging

Don't forget when testing to make sure you don't have cached js!

Comments

@jusfreeman @agilewarealok all this time I haven't been able to bring myself to merge #11137 or to close it as abandoned so tonight I took another look to see if I could address what was really bothering me. The issue with #11137 as I see it is we are still munging strings - https://github.com/civicrm/civicrm-core/pull/11137/files#diff-31a6607a9f03e2336c412d8fad30d711R72 so I went back to look at the approach and concluded that by assigning an array of 'all the things we need' in the first place we could simplify the js & make the insanity stop

CRM-21311: CIVICRM-668 Credit card type is unset on submission causing credit card payment to fail with CVV validation error message

…ion and thank you pages. The Confirm and Thank you pages were not showing the correct label - they were showing the name which we don't expect people to customise

civibot · 2018-08-02T11:22:43Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2018-08-02T11:23:05Z

CRM/Contribute/Form/ContributionBase.php

@@ -610,6 +610,13 @@ public function assignToTemplate() {
            CRM_Utils_System::mungeCreditCard(CRM_Utils_Array::value('credit_card_number', $this->_params))
          );
        }
+        elseif ($paymentField === 'credit_card_type') {


This relates to the confirm & thank you page tpl fix

eileenmcnaughton · 2018-08-02T11:23:17Z

CRM/Core/Payment/Form.php

@@ -279,27 +279,6 @@ public static function validatePaymentInstrument($payment_processor_id, $values,
    $payment->validatePaymentInstrument($values, $errors);
  }

-  /**


begone foul function

Fixed colemanw/webform_civicrm#178 in webform civi which uses this function. There might be more of such extension/modules? Should we be worried?

eileenmcnaughton · 2018-08-02T11:23:45Z

CRM/Financial/Form/Payment.php

+    }
+    $creditCardTypes = [];
+    foreach ($creditCards as $name => $label) {
+      $creditCardTypes[$name] = [


This is the magic - assign the right variables in the first place...

eileenmcnaughton · 2018-08-02T11:24:13Z

templates/CRM/Contribute/Form/Contribution/Confirm.tpl

@@ -247,8 +247,8 @@
                {/if}
            {else}
                <div class="crm-section no-label credit_card_details-section">
-                  <div class="content">{$credit_card_type}</div>
-                  <div class="content">{$credit_card_number}</div>
+                  <div class="content">{$credit_card_type|escape}</div>


just good practice - no reason to think this is insecure

eileenmcnaughton · 2018-08-02T11:25:18Z

templates/CRM/Core/BillingBlock.js

@@ -10,13 +10,13 @@
  function civicrm_billingblock_creditcard_helper() {
    $(function() {
      $.each(CRM.config.creditCardTypes, function(key, val) {
-        var html = '<a href="#" title="' + val + '" class="crm-credit_card_type-icon-' + key + '"><span>' + val + '</span></a>';
+        var html = '<a href="#" data-card_type=" + key + " title="' + val + '" class="crm-credit_card_type-icon-' + val['css_key'] + '"><span>' + val['label'] + '</span></a>';


actually we don't need the data attribute in the end but TBH it would be better next round to use it more & the calculated css class less

eileenmcnaughton · 2018-08-02T11:26:37Z

tests/phpunit/CRM/Financial/BAO/PaymentProcessorTest.php

@@ -62,38 +62,4 @@ public function testGetCreditCards() {
    $this->assertEquals($cards, $expectedCards, 'Verify correct credit card types are returned');
  }

-  public function testCreditCardCSSName() {


function no longer exists to test - it needed to be protected with a test before because it was so obvious a target :-)

@eileenmcnaughton i think we should still replace it with some kind of a test because part of this test wasn't just testing the CSS Names but verifying that when a payment processor only accepted specific card types that only those were sent to the form

Hmm I guess we could extract
so we have this test

/** * Check method create() */ public function testGetCreditCards() { $params = array( 'name' => 'API_Test_PP_Type', 'title' => 'API Test Payment Processor Type', 'class_name' => 'CRM_Core_Payment_APITest', 'billing_mode' => 'form', 'payment_processor_type_id' => 1, 'is_recur' => 0, 'domain_id' => 1, 'accepted_credit_cards' => json_encode(array( 'Visa' => 'Visa', 'Mastercard' => 'Mastercard', 'Amex' => 'Amex', )), ); $paymentProcessor = CRM_Financial_BAO_PaymentProcessor::create($params); $expectedCards = array( 'Visa' => 'Visa', 'Mastercard' => 'Mastercard', 'Amex' => 'Amex', ); $cards = CRM_Financial_BAO_PaymentProcessor::getCreditCards($paymentProcessor->id); $this->assertEquals($cards, $expectedCards, 'Verify correct credit card types are returned'); }

…mised. We have been following a strange pattern where we take the label of a credit card field and munge it into a css compatible string and then expect that to be a match with an array based on lcasing the name field for the credit card. This means that if label !== name the house of cards falls. This is particularly a problem for Amex which needs to be set correctly for the 4 digit cvv not to trigger a validation fail. Notably Amex is probably also the most likely for people to want to rename it since Amex is an abbreviation. This PR backs out of all of that and assigns an array of data to js - including the pattern itself and then updates the js script to refer to the right variable name for each purpose and to stop munging

eileenmcnaughton · 2018-08-03T03:37:12Z

test this please

eileenmcnaughton · 2018-08-03T08:43:12Z

I can't interpret the test result :-(

eileenmcnaughton · 2018-08-03T08:43:17Z

test this please

jusfreeman · 2018-08-07T03:55:19Z

@eileenmcnaughton thanks for addressing this issue.

Just curious, was it not possible to just update and change the existing PR #11137 ?

eileenmcnaughton · 2018-08-07T04:00:38Z

@jusfreeman no -that is under @agilewarealok repo - also - since I changed approach it needs to be possible to re-open a PR from Alok's repo if he considers me to be wrong

jusfreeman · 2018-08-07T05:19:21Z

@eileenmcnaughton I think you mean the agileware repo, as the PR came from agileware:4.7-CRM-21311

eileenmcnaughton · 2018-08-07T05:29:46Z

@jusfreeman I stand corrected

seamuslee001 · 2018-09-17T21:54:41Z

Hi @jusfreeman @eileenmcnaughton where are we going with this?

eileenmcnaughton · 2018-09-17T21:55:23Z

@seamuslee001 I'm hoping someone will review it - I feel confident it is a good fix which is why I haven't closed it

JoeMurray · 2018-09-18T00:16:38Z

@Monish pls try to get to this later in the week.

jusfreeman · 2018-09-18T07:23:56Z

Thanks everyone.

Silly question:
Why are we even validating specific credit card types at all? Do we really need to do this? If a credit card type is not accepted by the payment processor, an error should be returned and the process aborted. So it's the payment processor's job to do the validation.
We can then get rid of the credit card type field too.
Then all we really need to do here is show some icons to guide the user.

eileenmcnaughton · 2018-09-18T07:52:47Z

@jusfreeman js validation is recommended practice because they get a quick response instead of waiting for the payment to be submitted & half-processed & then getting rejected

JoeMurray · 2018-09-18T13:24:52Z

CiviCRM also allows card types to be disabled, which could be used to avoid the higher fees of Amex vs Visa/MC. So Civi could purposefully prevent a valid Amex card from being submitted.

jusfreeman · 2018-09-18T20:44:40Z

CiviCRM also allows card types to be disabled, which could be used to avoid the higher fees of Amex vs Visa/MC

Hi Joe, agree about AMEX and the merchant fees. It's interesting that AMEX is opt-in for EWAY, but enabled by default for PayPal and Stripe.

JoeMurray · 2018-10-09T15:57:07Z

@Edzelopez please aim to review this

eileenmcnaughton · 2018-10-16T07:38:41Z

@Edzelopez were you going to take a look at this?

JoeMurray · 2018-10-16T09:45:00Z

Two comments:

I agree with @seamuslee001 that we should have a test to ensure that only supported cards are accepted on a form configured for a payment processor with limitations on which ones it accepts.
I have this vague disquiet when values are munged before being compared. What if an instance has both Diner's Club and Diner"s Club? Highly unlikely I agree, but when an admin is having trouble they sometimes do weird things. Feel free to disregard if it doesn't make you uneasy.

@Edzelopez will try to test on Friday.

eileenmcnaughton · 2018-10-16T19:39:04Z

Note that this patch is required to fix a regression caused when @seamuslee001 added the ability to support only some card types. I was happy to take on fixing the bug but I'm not wanting to re-work the unit test for the functionality mentioned - perhaps @seamuslee001 can revisit that once this is merged

jitendrapurohit · 2018-10-25T08:37:09Z

I tested this with different card types on front and back end contribution/membership/event forms. I wasn't able to notice any problems related to it. The main issue is fixed and the validation is done properly now. @eileenmcnaughton

eileenmcnaughton · 2018-10-25T09:15:31Z

Thanks @jitendrapurohit - merging

eileenmcnaughton · 2018-10-26T05:45:44Z

@jitendrapurohit I didn't find any other instances googling CRM_Core_Payment_Form::getCreditCardCSSNames - Tim does have a tool 'universe' https://docs.civicrm.org/dev/en/latest/tools/universe/ but "The universe is fairly big. (At time of writing, ~2 GB.) A fast network will help with downloading, and a fast storage will help with searching."

Note that it's not actually supported for extensions to call this function per se - but it's also good to check if we can find any

jitendrapurohit · 2018-10-26T06:07:09Z

Hmm, I just did a quick grep on one of our dev servers having most of the important extension/modules and don't see this function being used except in civi or webform civicrm.

CRM-21311 fix credit card type display on contribution page confirmat…

05d2edf

…ion and thank you pages. The Confirm and Thank you pages were not showing the correct label - they were showing the name which we don't expect people to customise

civicrm-builder added the master label Aug 2, 2018

eileenmcnaughton commented Aug 2, 2018

View reviewed changes

eileenmcnaughton force-pushed the amex branch from 9a98597 to a05d89b Compare August 2, 2018 11:30

eileenmcnaughton force-pushed the amex branch from a05d89b to ce89559 Compare August 2, 2018 11:34

eileenmcnaughton mentioned this pull request Aug 5, 2018

CRM-21311 Credit card type is unset on submission causing credit card payment to fail with CVV validation error message #11137

Closed

jitendrapurohit approved these changes Oct 25, 2018

View reviewed changes

eileenmcnaughton merged commit 4d98825 into civicrm:master Oct 25, 2018

eileenmcnaughton deleted the amex branch October 25, 2018 09:15

jitendrapurohit mentioned this pull request Oct 26, 2018

Fix getCreditCardCSSNames() call which is deleted from core civi colemanw/webform_civicrm#178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21311 fix handling of credit cards where Amex has been given a different label #12615

CRM-21311 fix handling of credit cards where Amex has been given a different label #12615

eileenmcnaughton commented Aug 2, 2018 •

edited by civicrm-builder

Loading

civibot bot commented Aug 2, 2018

eileenmcnaughton Aug 2, 2018

eileenmcnaughton Aug 2, 2018

jitendrapurohit Oct 26, 2018 •

edited

Loading

eileenmcnaughton Aug 2, 2018

eileenmcnaughton Aug 2, 2018

eileenmcnaughton Aug 2, 2018

eileenmcnaughton Aug 2, 2018

seamuslee001 Aug 2, 2018

eileenmcnaughton Aug 2, 2018

eileenmcnaughton commented Aug 3, 2018

eileenmcnaughton commented Aug 3, 2018

eileenmcnaughton commented Aug 3, 2018

jusfreeman commented Aug 7, 2018

eileenmcnaughton commented Aug 7, 2018

jusfreeman commented Aug 7, 2018

eileenmcnaughton commented Aug 7, 2018

seamuslee001 commented Sep 17, 2018

eileenmcnaughton commented Sep 17, 2018

JoeMurray commented Sep 18, 2018

jusfreeman commented Sep 18, 2018

eileenmcnaughton commented Sep 18, 2018

JoeMurray commented Sep 18, 2018

jusfreeman commented Sep 18, 2018

JoeMurray commented Oct 9, 2018

eileenmcnaughton commented Oct 16, 2018

JoeMurray commented Oct 16, 2018

eileenmcnaughton commented Oct 16, 2018

jitendrapurohit commented Oct 25, 2018

eileenmcnaughton commented Oct 25, 2018

eileenmcnaughton commented Oct 26, 2018

jitendrapurohit commented Oct 26, 2018

CRM-21311 fix handling of credit cards where Amex has been given a different label #12615

CRM-21311 fix handling of credit cards where Amex has been given a different label #12615

Conversation

eileenmcnaughton commented Aug 2, 2018 • edited by civicrm-builder Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jitendrapurohit Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Aug 3, 2018

eileenmcnaughton commented Aug 3, 2018

eileenmcnaughton commented Aug 3, 2018

jusfreeman commented Aug 7, 2018

eileenmcnaughton commented Aug 7, 2018

jusfreeman commented Aug 7, 2018

eileenmcnaughton commented Aug 7, 2018

seamuslee001 commented Sep 17, 2018

eileenmcnaughton commented Sep 17, 2018

JoeMurray commented Sep 18, 2018

jusfreeman commented Sep 18, 2018

eileenmcnaughton commented Sep 18, 2018

JoeMurray commented Sep 18, 2018

jusfreeman commented Sep 18, 2018

JoeMurray commented Oct 9, 2018

eileenmcnaughton commented Oct 16, 2018

JoeMurray commented Oct 16, 2018

eileenmcnaughton commented Oct 16, 2018

jitendrapurohit commented Oct 25, 2018

eileenmcnaughton commented Oct 25, 2018

eileenmcnaughton commented Oct 26, 2018

jitendrapurohit commented Oct 26, 2018

eileenmcnaughton commented Aug 2, 2018 •

edited by civicrm-builder

Loading

jitendrapurohit Oct 26, 2018 •

edited

Loading