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

jusfreeman · 2017-10-14T06:09:32Z

Overview

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.

Technical Details

Regression bug introduced in PR #10774 - See https://github.com/civicrm/civicrm-core/pull/10774/files#diff-31a6607a9f03e2336c412d8fad30d711R72

This function has a comment that it was created to be used only for CSS names. Since PR #10774 it is being used to return a json string. Recommend refactor function name and remove comment - since it is not just being used for CSS anymore.

See CRM/Core/Payment/Form.php - public static function getCreditCardCSSNames

https://github.com/civicrm/civicrm-core/pull/10774/files#diff-31f2b294fac11671bf5ae72d8e6d7172R115

Comments

Who uses AMEX anyway?

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

civicrm-builder · 2017-10-14T06:09:33Z

Can one of the admins verify this patch?

jusfreeman · 2017-10-14T06:10:28Z

@monishdeb another one to review buddy!

monishdeb · 2017-10-14T08:49:06Z

@jusfreeman I have reviewed your changes and it fixes the issues you mentioned above but causes another regression. Let me explain with screencast, where after submission, card type ID is not saved.

After applying the patch do a live contribution with Visa (4111111111111111 / 123)

Without patch do a live contribution with Visa (4111111111111111 / 123)

As you can see in comparison to the earlier transaction, the card type and card number (last 4 digit) is recorded now.

monishdeb · 2017-10-14T08:53:14Z

This patch is also affecting backoffice contribution as CC(Credit Card) type id is not recorded. Do a backoffice contribution using CC as payment instrument and use Visa/1111 as CC type and last 4 digit card number. You will find that CC type ID is not saved (you need to look at civicrm_financial_trxn)

eileenmcnaughton · 2017-10-16T02:00:42Z

I think this HAS TO BE fixed in 4.7.27 - even if at expense of recording credit card type - we can't be not accepting cards.

I'm confused as to why there is a problem with this fix. It seems the array on the form looks like

<select name="credit_card_type" id="credit_card_type" class="crm-form-select" style="display: none;">
	<option value="">- select -</option>
	<option value="Visa">Visa</option>
	<option value="MasterCard">MasterCard</option>
	<option value="Amex">American Express</option>
	<option value="Discover">Discover</option>
</select>

Later on we see

  public static function formatCreditCardDetails(&$params) {
    if (in_array('credit_card_type', array_keys($params))) {
      $params['card_type_id'] = CRM_Core_PseudoConstant::getKey('CRM_Core_BAO_FinancialTrxn', 'card_type_id', $params['credit_card_type']);
    }
    if (!empty($params['credit_card_number']) && empty($params['pan_truncation'])) {
      $params['pan_truncation'] = substr($params['credit_card_number'], -4);
    }
  }

So we are taking the name field from the option value & using it with 'getKey' - so as long as we are always working with the KEY/ machine_name and never with the label we should be OK. So the js fix changes us from using label to name - which seems a correct part of the fix.

I can't see why the php function change is needed at this stage. We should totally NOT allow anything in the machine_name field that machines don't like

@seamuslee001 ^^

seamuslee001 · 2017-10-16T02:07:03Z

I think that sounds right but not 100% sure, I will try and test this when i am a bit less jetlagged

jusfreeman · 2017-10-17T03:18:47Z

Jenkins please test

eileenmcnaughton · 2017-10-18T20:17:29Z

So my concern here is whether this now means the format that will be passed to the processors will change slightly (e.g from visa to Visa). Paypal code DOES pass this through to Paypal. Others might too.

I think it makes sense to update the name field in the DB to lower case so that will stay consistent?

UPDATE civicrm_option_value v
INNER JOIN civicrm_option_group g
ON v.option_group_id = g.id AND g.name= 'accept_creditcard'
SET v.name = lower(v.name) ;

eileenmcnaughton · 2017-10-18T20:18:34Z

(the code changes do seem like a sensible magicobotomy)

jusfreeman · 2017-10-18T22:14:31Z

I think it makes sense to update the name field in the DB to lower case so that will stay consistent?

@eileenmcnaughton Simple solutions are best and yes, I think that's fine to do that too. Still the credit card type key should be lowercased in code before passing to the payment processor - if that's what they accept (or have been given in the past).

It's worth keeping in mind that the problem being solved here really was that the key and the label for the credit card type were assumed to be the same value. And so were used interchangeably throughout the code. Now that users can easily change the label for any credit card type, this breaks credit card type being set and on-line payments is then non-functional.

I noticed that CiviCRM actually ships with pear libraries which have built-in credit card number validation and setting of the credit card type. So all this jiggery pokery could be made simpler by just doing server side validation and setting of the credit card type. And to be honest, I think that would be a more robust solution going forward - the current state is very fragile. Why even ask the user what type of credit card it is, when this can be computed?

I can see there is still a problem on the Contribution confirmation page where the credit card type key is displayed, instead of the label. That should also be changed and all other payment confirmation pages (Event, Contribution, Membership etc.) changed to display the credit card type label.

For changing the confirmation pages, I am not really sure what to do here and would appreciate advise or someone else helping out.

Files:
templates/CRM/Contribute/Form/Contribution/Confirm.tpl
CRM/Contribute/Form/Contribution/Confirm.php

eileenmcnaughton · 2017-10-18T22:39:38Z

"Why even ask the user what type of credit card it is, when this can be computed?" - good question @KarinG @colemanw @JoeMurray - do any of you know?

KarinG · 2017-10-18T22:42:30Z

No - I don’t - I’m pretty sure I asked the same question;

KarinG · 2017-10-18T22:55:48Z

So what 4.7.x version has this bug? I have a 4.7 site that’s on 4.7.22 - that is happily accepting AMEX (I just checked);

KarinG · 2017-10-18T23:04:11Z

It’s more a lookup rather than a compute: https://en.m.wikipedia.org/wiki/Payment_card_number#Issuer_identification_number_.28IIN.29

eileenmcnaughton · 2017-10-19T02:48:52Z

@KarinG the bug only manefests if you change the label on the option value from Amex to American Express

KarinG · 2017-10-19T03:16:37Z

Ah I see - thank you; yeah we don’t do that; so that explains why we’ve not seen it;

eileenmcnaughton · 2017-10-19T03:20:13Z

right - it's kinda edge.

monishdeb · 2017-10-19T03:26:12Z

Jenkin test this please

jusfreeman · 2017-10-19T03:54:57Z

right - it's kinda edge.

@eileenmcnaughton as you know, we live on the edge!

eileenmcnaughton · 2017-10-19T04:03:56Z

You rebel you!

I think the best path to getting this resolved is to update the name field in the DB to be lower case (via upgrade script) and keep the main js change in this PR but cull out a couple of the changes that are about handling the casing change. We might make the option values 'is_reserved' = TRUE too, since the name values are not actually permitted to be amended.

I think there is a case for not having card type but I think it should be resolved as a follow up since it probably involves email discussion and requires at least 6 people to attempt to hijack the change with their pet project before we can be sure everyone is happy with the basic principle of removing the field :-)

jusfreeman · 2017-10-27T23:07:34Z

Jenkin test this please

jusfreeman · 2017-10-27T23:08:40Z

I think the best path to getting this resolved is to update the name field in the DB to be lower case (via upgrade script)

@eileenmcnaughton can you provide some guidance on doing this change?

eileenmcnaughton · 2017-11-14T08:40:32Z

it can be done in CRM_Upgrade_Incremental_php_FourSeven

… credit card payment to fail with CVV validation error message

…ardCSSNames and replace with CRM_Contribute_PseudoConstant::creditCard for consistency - remove reliance on lowercase credit card names

… remove JS validation of card types which cannot be defined, remove unused call to CRM.config.creditCardTypes

agilewarealok · 2017-12-06T09:35:20Z

@eileenmcnaughton We've updated the PR by adding Update query in CRM_Upgrade_Incremental_php_FourSeven under upgrade_4_7_27

colemanw · 2017-12-06T16:16:22Z

That upgrade function looks correct, except it will need to be moved to function upgrade_4_7_30 because of our release schedule (4.7.28 is going out today. 4.7.27 is already out and 4.7.29-rc is about to be cut). If we consider this critical enough and the PR is complete we could potentially merge it into the 4.7.29-rc, but currently it looks like there are still some outstanding review comments from @eileenmcnaughton about culling case handling changes.

jusfreeman · 2017-12-06T22:56:28Z

For the "culling case handling changes" - all the conversion to lowercase in JS are required for CSS declarations. I recommend leaving as is.

@eileenmcnaughton would mind reviewing this again, thanks.

eileenmcnaughton · 2018-01-25T01:53:53Z

templates/CRM/Core/BillingBlock.js

-      $.each(CRM.config.creditCardTypes, function(key, val) {
-        var html = '<a href="#" title="' + val + '" class="crm-credit_card_type-icon-' + key + '"><span>' + val + '</span></a>';
+      $.each(CRM.config.creditCardTypes, function(card_type_key, val) {
+        card_type_css = card_type_key.toLowerCase();


this line should not be required now - we have a list of names that are already lower case

@eileenmcnaughton We've just tested against master and logging CRM.config.creditCardTypes return following JSON string.

{Visa: "Visa", MasterCard: "MasterCard", Amex: "Amex", Discover: "Discover"}

So the first character of each keys are still in uppercase.

eileenmcnaughton · 2018-01-25T01:55:24Z

CRM/Financial/Form/Payment.php

@@ -117,7 +117,7 @@ public function setDefaultValues() {
   */
  public static function addCreditCardJs($paymentProcessorID = NULL, $region = 'billing-block') {
    $creditCards = CRM_Financial_BAO_PaymentProcessor::getCreditCards($paymentProcessorID);
-    $creditCardTypes = CRM_Core_Payment_Form::getCreditCardCSSNames($creditCards);
+    $creditCardTypes = CRM_Contribute_PseudoConstant::creditCard($creditCards);


So we want to be assigning an array of name=>label yeah - ie

[
'visa' =.> 'Visa',
'mastercard' => 'MasterCard'
/....
]

eileenmcnaughton · 2018-01-25T01:56:17Z

templates/CRM/Core/BillingBlock.js

@@ -27,13 +28,14 @@
      // set the card type value as default if any found
      var cardtype = $('#credit_card_type').val();
      if (cardtype) {
-        $.each(CRM.config.creditCardTypes, function(key, value) {
+        $.each(CRM.config.creditCardTypes, function(card_type_key, value) {
+          card_type_css = card_type_key.toLowerCase();


ditto here - we should be already in lowercase

eileenmcnaughton · 2018-01-25T01:56:59Z

templates/CRM/Core/BillingBlock.js

    var card_types = {
-      'mastercard': '(5[1-5][0-9]{2}|2[3-6][0-9]{2}|22[3-9][0-9]|222[1-9]|27[0-1][0-9]|2720)[0-9]{12}',


not sure we should be changing this - we should be running off the lower case 'name' field?

eileenmcnaughton · 2018-01-25T01:57:42Z

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

@@ -48,21 +48,21 @@ public function testGetCreditCards() {
      'domain_id' => 1,
      'accepted_credit_cards' => json_encode(array(
        'Visa' => 'Visa',
-        'Mastercard' => 'Mastercard',
-        'Amex' => 'Amex',
+        'MasterCard' => 'Mastercard',


aren't we doing 'mastercard' => 'Mastercard' now?

jusfreeman · 2018-02-19T03:43:36Z

@eileenmcnaughton happy for you to edit this PR so we can get it merged.

eileenmcnaughton · 2018-08-05T21:27:42Z

Closing in favour of #12615 (which requires review)

civicrm-builder added the master label Oct 14, 2017

agileware-justin added 4 commits December 5, 2017 12:10

CRM-21311 CIVICRM-668 Credit card type is unset on submission causing…

111ab28

… credit card payment to fail with CVV validation error message

CRM-21311 CIVICRM-668 Remove use of CRM_Core_Payment_Form::getCreditC…

3caec18

…ardCSSNames and replace with CRM_Contribute_PseudoConstant::creditCard for consistency - remove reliance on lowercase credit card names

CRM-21311 CIVICRM-668 Refactor BillingBlock.js

d1f2a22

CRM-21311 CIVICRM-668 Restore incorrect capitalisation of Mastercard,…

fb3e8df

… remove JS validation of card types which cannot be defined, remove unused call to CRM.config.creditCardTypes

agilewarealok force-pushed the 4.7-CRM-21311 branch 2 times, most recently from 5c57d5b to f2c8031 Compare December 5, 2017 06:48

CRM-21311: Updating civicrm_option_value name to lowercase.

436d557

agilewarealok force-pushed the 4.7-CRM-21311 branch from f2c8031 to 436d557 Compare December 5, 2017 06:59

eileenmcnaughton reviewed Jan 25, 2018

View reviewed changes

eileenmcnaughton mentioned this pull request Aug 2, 2018

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

Merged

eileenmcnaughton closed this Aug 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jusfreeman commented Oct 14, 2017 •

edited by civicrm-builder

Loading

civicrm-builder commented Oct 14, 2017

jusfreeman commented Oct 14, 2017

monishdeb commented Oct 14, 2017 •

edited

Loading

monishdeb commented Oct 14, 2017 •

edited

Loading

eileenmcnaughton commented Oct 16, 2017

seamuslee001 commented Oct 16, 2017

jusfreeman commented Oct 17, 2017

eileenmcnaughton commented Oct 18, 2017

eileenmcnaughton commented Oct 18, 2017

jusfreeman commented Oct 18, 2017

eileenmcnaughton commented Oct 18, 2017

KarinG commented Oct 18, 2017

KarinG commented Oct 18, 2017

KarinG commented Oct 18, 2017

eileenmcnaughton commented Oct 19, 2017

KarinG commented Oct 19, 2017

eileenmcnaughton commented Oct 19, 2017

monishdeb commented Oct 19, 2017

jusfreeman commented Oct 19, 2017

eileenmcnaughton commented Oct 19, 2017

jusfreeman commented Oct 27, 2017

jusfreeman commented Oct 27, 2017

eileenmcnaughton commented Nov 14, 2017

agilewarealok commented Dec 6, 2017

colemanw commented Dec 6, 2017

jusfreeman commented Dec 6, 2017

eileenmcnaughton Jan 25, 2018

agilewarealok Apr 6, 2018

eileenmcnaughton Jan 25, 2018

eileenmcnaughton Jan 25, 2018

eileenmcnaughton Jan 25, 2018

eileenmcnaughton Jan 25, 2018

jusfreeman commented Feb 19, 2018

eileenmcnaughton commented Aug 5, 2018

		var card_types = {
		'mastercard': '(5[1-5][0-9]{2}\|2[3-6][0-9]{2}\|22[3-9][0-9]\|222[1-9]\|27[0-1][0-9]\|2720)[0-9]{12}',

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

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

Conversation

jusfreeman commented Oct 14, 2017 • edited by civicrm-builder Loading

Overview

Before

After

Technical Details

Comments

civicrm-builder commented Oct 14, 2017

jusfreeman commented Oct 14, 2017

monishdeb commented Oct 14, 2017 • edited Loading

monishdeb commented Oct 14, 2017 • edited Loading

eileenmcnaughton commented Oct 16, 2017

seamuslee001 commented Oct 16, 2017

jusfreeman commented Oct 17, 2017

eileenmcnaughton commented Oct 18, 2017

eileenmcnaughton commented Oct 18, 2017

jusfreeman commented Oct 18, 2017

eileenmcnaughton commented Oct 18, 2017

KarinG commented Oct 18, 2017

KarinG commented Oct 18, 2017

KarinG commented Oct 18, 2017

eileenmcnaughton commented Oct 19, 2017

KarinG commented Oct 19, 2017

eileenmcnaughton commented Oct 19, 2017

monishdeb commented Oct 19, 2017

jusfreeman commented Oct 19, 2017

eileenmcnaughton commented Oct 19, 2017

jusfreeman commented Oct 27, 2017

jusfreeman commented Oct 27, 2017

eileenmcnaughton commented Nov 14, 2017

agilewarealok commented Dec 6, 2017

colemanw commented Dec 6, 2017

jusfreeman commented Dec 6, 2017

eileenmcnaughton Jan 25, 2018

Choose a reason for hiding this comment

agilewarealok Apr 6, 2018

Choose a reason for hiding this comment

eileenmcnaughton Jan 25, 2018

Choose a reason for hiding this comment

eileenmcnaughton Jan 25, 2018

Choose a reason for hiding this comment

eileenmcnaughton Jan 25, 2018

Choose a reason for hiding this comment

eileenmcnaughton Jan 25, 2018

Choose a reason for hiding this comment

jusfreeman commented Feb 19, 2018

eileenmcnaughton commented Aug 5, 2018

jusfreeman commented Oct 14, 2017 •

edited by civicrm-builder

Loading

monishdeb commented Oct 14, 2017 •

edited

Loading

monishdeb commented Oct 14, 2017 •

edited

Loading