Add an "AVS response code" base field to Payment entities

This looks like a great issue to try out with the new issue forks. I kicked one off, https://git.drupalcode.org/issue/commerce-3153381. Info in the sidebar.

I tried to use the new issue fork approach, but when I tried push up my branch, I got:

remote: HTTP Basic: Access denied
fatal: Authentication failed for 'https://git.drupalcode.org/issue/commerce-3153381.git/'

I have the checkmark for, "You have push access", and I entered my Drupal.org username and password.

Here is a patch instead. No tests yet, since I wanted to confirm that I used the correct approach first. I created a field formatter for the avs response code field to display the codes-with-meaning. It can be used to display the avs response codes in custom views.

Writing a to-do here as a reminder: update payment_example gateways to set the avs response code when creating payments.

Just a note regarding AvsResponseCodeHelper:: getAvsResponseCodeMeaning(), this should probably an array instead, keyed by credit card type, and I wonder if we shouldn't make the meaning translatable by using TranslatableMarkup.

->setDescription(t('AVS response code mode.'))

Also wondering if AvsResponseCodeHelper() makes sense vs just adding a method to \Drupal\commerce_payment\CreditCard.

Something like mapAvsResponseCode(), or even just getAvsResponseCodes() which returns the entire array.

+++ b/modules/payment/src/Entity/Payment.php
@@ -383,6 +405,16 @@ class Payment extends ContentEntityBase implements PaymentInterface {
+      ->setDescription(t('AVS response code mode.'))

Any reason for "mode"?

@jsacksick Thanks for the code review. I'll move the "helper" code to CreditCard and make it array-based. Don't know why I didn't think of that in the first place.

And the "mode" was a mistake. Copy-and-paste error from "payment gateway mode".

Also wondering if it wouldn't make sense to add "avs_response_code" last (in baseFieldDefinitions(), instead of next to the "amount" field.

1. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
   @@ -437,6 +437,29 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
   +    if (empty($payment_method = $payment->getPaymentMethod())) {
   

   $payment_method probably deserves its own line. :p

2. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayInterface.php
   @@ -123,6 +123,17 @@ interface PaymentGatewayInterface extends PluginWithFormsInterface, Configurable
   +  public function buildAvsResponseCodeLabel(PaymentInterface $payment);
   

   We should document that this method can return NULL.

Posting a patch with updates based on comments. Will begin work on updating tests to cover new functionality.

Oops, one missing use statement. And two extra use statements.

Added test coverage for the new AVS response code field. Unit, Kernel, and Functional tests are passing locally. FunctionalJavascript tests are being skipped. So it's likely they will need more work.

1. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
   @@ -437,6 +437,37 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
   +    if (empty($payment_method)) {
   

   This and the following "if" could be combined together to be :

   if (!$payment_method || $payment_method->bundle() !== 'credit_card')
   

2. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
   @@ -437,6 +437,37 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
   +    $card_type = $payment_method->card_type->value;
   +    if (empty($avs_code_meanings[$card_type])) {
   

   We probably need to return early if:

   if ($payment_method->get('card_type')->isEmpty())
   

   And

   if (empty($avs_code_meanings[$card_type][$avs_response_code])) {
   

   with the previous "if" could be just changed to a single isset()

+++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
@@ -437,6 +437,37 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
+    $card_type = $payment_method->card_type->value;
+    if (empty($avs_code_meanings[$card_type])) {
...
+    if (empty($avs_code_meanings[$card_type][$avs_response_code])) {
...
+    $code_display = empty($avs_response_code) ? 'NULL' : $avs_response_code;

Can $avs_response_code be empty at this point? After all these checks?? :p

1. +++ b/modules/payment/src/CreditCard.php
   @@ -273,4 +273,70 @@ final class CreditCard {
   +  /**
   +   * Gets all available avs response code meanings.
   +   *
   +   * @return array
   +   *   The avs response code meanings.
   +   */
   +  public static function getAvsResponseCodeMeanings() : array {
   

   We should link to docs where we get this

2. +++ b/modules/payment/src/Entity/Payment.php
   @@ -158,6 +158,28 @@ class Payment extends ContentEntityBase implements PaymentInterface {
   +   * {@inheritdoc}
   +   */
   +  public function getAvsResponseCode() {
   +    return $this->get('avs_response_code')->value;
   +  }
   

   We should check if empty first

3. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
   @@ -437,6 +437,37 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
   +    if ($payment_method->getType()->pluginDefinition['id'] !== 'credit_card') {
   +      return $avs_response_code;
   +    }
   

   Why can't we just check the bundle? why are we fetching the plugin definition?

4. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
   @@ -437,6 +437,37 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
   +    if (empty($avs_code_meanings[$card_type])) {
   +      return $avs_response_code;
   +    }
   +    if (empty($avs_code_meanings[$card_type][$avs_response_code])) {
   +      return $avs_response_code;
   +    }
   

   could be combined, as isset

@mglaman Thanks for the code review. One question. For:

+++ b/modules/payment/src/Entity/Payment.php
@@ -158,6 +158,28 @@ class Payment extends ContentEntityBase implements PaymentInterface {
+ * {@inheritdoc}
+ */
+ public function getAvsResponseCode() {
+ return $this->get('avs_response_code')->value;
+ }
We should check if empty first

Why do we need to check if this is empty here if we don't do the same for payment gateway mode, remove id, or remote state? I don't see how it's any different.

Thanks for the code reviews. Made some minor fixes here.

RE #15 yeah, I forgot. Apparently my brain was in uber strict mode

Looking at this again, and something isn't sitting right.

1. +++ b/modules/payment/src/Plugin/Field/FieldFormatter/AvsResponseCodeFormatter.php
   @@ -0,0 +1,46 @@
   +    foreach ($items as $delta => $item) {
   +      /** @var \Drupal\commerce_payment\Entity\PaymentInterface $payment */
   +      $payment = $item->getEntity();
   +      $elements[$delta] = ['#markup' => $payment->getAvsResponseCodeLabel()];
   +    }
   

   Maybe I'd feel better if we didn't put the label method on the payment. After all, this just invokes the payment gateway. So maybe we should get the label directly from the payment gateway here.

   And instead of having to pass a payment entity to the method, why not just the AVS code?

2. +++ b/modules/payment_example/src/Plugin/Commerce/PaymentGateway/Onsite.php
   @@ -114,6 +114,7 @@ class Onsite extends OnsitePaymentGatewayBase implements OnsiteInterface {
   +    $payment->setAvsResponseCode('A');
        $payment->save();
   
   @@ -251,4 +252,25 @@ class Onsite extends OnsitePaymentGatewayBase implements OnsiteInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function buildAvsResponseCodeLabel(PaymentInterface $payment) {
   +    $avs_response_code = $payment->getAvsResponseCode();
   +    /** @var \Drupal\commerce_payment\Entity\PaymentMethodInterface $payment_method */
   +    $payment_method = $payment->getPaymentMethod();
   +    if (empty($payment_method)) {
   +      return $avs_response_code;
   +    }
   +
   +    $card_type = $payment_method->card_type->value;
   +    if ($card_type == 'dinersclub' || $card_type == 'jcb') {
   +      if ($avs_response_code == 'A') {
   +        return $this->t('A - Approved.');
   +      }
   +      return $avs_response_code;
   +    }
   +    return parent::buildAvsResponseCodeLabel($payment);
   

   I don't know if we should be dynamically generating the label whenever viewing the payment. It almost seems like we may as well also provide a avs_response_label field that gets populated on save, instead of on view.

+++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
@@ -437,6 +437,33 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
+  public function buildAvsResponseCodeLabel(PaymentInterface $payment) {
+    $avs_response_code = $payment->getAvsResponseCode();
+    /** @var \Drupal\commerce_payment\Entity\PaymentMethodInterface $payment_method */
+    $payment_method = $payment->getPaymentMethod();
+    if (!$payment_method || $payment_method->bundle() !== 'credit_card') {
+      return $avs_response_code;
+    }
+    if ($payment_method->get('card_type')->isEmpty()) {
+      return $avs_response_code;
+    }
+
+    $card_type = $payment_method->card_type->value;

+++ b/modules/payment_example/src/Plugin/Commerce/PaymentGateway/Onsite.php
@@ -251,4 +252,25 @@ class Onsite extends OnsitePaymentGatewayBase implements OnsiteInterface {
+    }

Yeah, I think this method should just receive the AVS code and card type. The formatter should provide these.

So according to the Slack discussions, it seems that this requires more work (i.e a new field type that stores both the response and the label).

I stumbled upon this issue while going through the commerce issue queue and been wondering, from the moment I read the issue summary, whether "AVS response code" should really be part of the Payment entity. Especially because of this:

Additionally, some payment gateways won't need AVS at all - e.g. PayPal payment, Affirm financing, bank transfers, etc.

IMHO, it naturally belongs to Payment Method of type credit_card (Drupal\commerce_payment\Plugin\Commerce\PaymentMethodType\CreditCard). I don't see how the AVS response code could change between multiple usages of the same payment method if it is dependent on the billing address and CC itself which both belong to credit_card payment method type.

I don't think this was publicly discussed, or I missed that discussion as part of this issue or Slack, but if it was - please forgive me for repeating the same concerns.

I don't see how the AVS response code could change between multiple usages of the same payment method if it is dependent on the billing address and CC itself which both belong to credit_card payment method type.

You can have the same CC# and change your billing address. Also, what if it's an off-site gateway which returned a credit card also had an AVS code? I guess that isn't very common, as the purpose of an off-site is to handle all of that stuff.

The bigger item is the fact AVS and CVV response codes are always part of the payment transaction data model in other systems (but you could argue so are CC meta, ACH meta, etc.)

You can have the same CC# and change your billing address.

But we actually keep a reference to profile revision, not profile itself, so billing profile should not really change if it's not because of some custom code, no?

Also, what if it's an off-site gateway which returned a credit card also had an AVS code?

If an offsite gateway creates payment method, which is now supported, then it can also keep CC related info, including AVS, on the Payment Method entity.

It just feels organic to me to keep it in a data structure meant for holding CC related data which is now part of Payment Method entity storage,

The bigger item is the fact AVS and CVV response codes are always part of the payment transaction data model in other systems (but you could argue so are CC meta, ACH meta, etc.)

Is there a plan for adding CVV response as well - would that also go to Payment entity given we do not store any CVC/CVV/CV2 related data, so CVV response should remain the same no matter what?

Don't take me wrong. I can see that some merchants would benefit from having that info or even require it. I also understand that this is transaction-level info so it fits better into the Payment model than Payment Method. I just fail to see why rushing AVS Response Code if, from what it seems, Payment storage was built in a way that should be "type" (e.g. CC) agnostic. Adding field(s) that will potentially benefit only a subset of on-site payment gateways sounds like a trade-off I'm not comfortable with. Perhaps our Payment model needs some changes to make it more customizable/pluggable on the payment gateway level? This was actually quite well described in #3118158: [Parent] Store more transaction specific information on Payment entities and #3118412: [meta] Payment methods should be ephemeral

I still need to read through the discussion more carefully but moving things forward a bit with the addition of a standalone label field for the avs response code. Removed the custom field formatter and updated tests.

One option that hasn't been mentioned/discussed would be to define a new "payment type". Payment types are also bundle plugins. Most gateways use the default one atm ("payment_default").

The advantage of this solution is that it'd be up to the gateway plugin to define whether they support setting the AVS response code and eventually the CVV at a later point. It'd also be up to the gateways to write an upgrade path to install the bundle fields after switching to this new payment type.

We could call it "payment_credit_card" or something, but I'm not 100% convinced that is the right solution either. Switching to the new payment type is done from the gateway plugin annotation by specifying the "payment_type" key.

Re: #27 - Adding PaymentType plugin is something I've been thinking about as well. I wasn't aware though, that the Plugin is already defined :) I find it much more accurate and distinct to define AVS/CVV response status fields on a PaymentType level than Payment itself.

I can see how the upgrade path could be somewhat troublesome but focusing on easing that process for payment gateway developers with a set of helper methods + defining payment_credit_cart type with response status helpers from Lisa's work in #25, and, finally, making it an opt-in thing (via payment_type key in plugin gateway annotation) would be a compromise I'm very much fine with.

I don't know what would be the result of such action, but the developer can already freely change payment type between one and another. I imagine the consequences of such action can be dangerous and we should show a site warning if we find entities created with mismatching payment_type.

I have created new branch (https://git.drupalcode.org/issue/commerce-3153381/-/tree/3153381-27) with POC described in #27 and pushed a commit that defines new fields and installs bundle - https://git.drupalcode.org/issue/commerce-3153381/-/commit/9da91c15be510...

I named the new payment type "credit_card" as I find "payment_" prefix redundant if the plugin is only used in the payment context.

Installing the new bundle was fairly easy. Commerce/Drupal did not complain about Payment entities created before switching payment_type from payment_default to credit_card although I think having helper method that will switch payment type for all Payment entities from payment_default (which doesn't have bundle fields) to new credit_card type would be something that could improve DX for payment gateways that want to facilitate new payment type. It might not be needed though since payment_default type is not removed after all.

I agree that the scope of the changes to make commerce 2.x more payment agnostic are some major breaking changes and are probably more appropriate to do in 3.x.

If you're running with the idea that payment is opinionated in 2.x around credit cards does this mean that some of the other payment data points mentioned in the parent issue should take the same approach as it does here?

Moving this issue to "needs review" since the general consensus seems to be that we go with the simpler/faster fix and hold off on larger changes for now.

Just one note regarding OffsiteRedirect:: getAvsResponseCodeLabel() that looks a bit weird to me, we're passing the payment entity there and we check if the "payment_method" field which is never set.

Also the doxygen comment for buildAvsResponseCodeLabel() still says "Builds a label for the AVS response code of the given payment.", although we don't actually pass the payment.

Besides that, I think this looks good.

Thanks for the code review. I just removed the avs-response-code stuff from the Offsite Redirect example gateway altogether since it doesn't really make sense without a payment method.

And I fixed the doxygen comment that still referred to the older version of the method.

Looks good to me.

@mglaman, @rzrama: would you like to give the patch a final review?

Looks good to me.

+++ b/modules/payment/src/Entity/PaymentInterface.php
@@ -97,6 +97,42 @@ interface PaymentInterface extends ContentEntityInterface, EntityWithPaymentGate
+   * @return string|null
+   *   The payment AVS response code label, or NULL if it does not exist.
+   */
+  public function getAvsResponseCodeLabel();

+++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/PaymentGatewayBase.php
@@ -437,6 +437,17 @@ abstract class PaymentGatewayBase extends PluginBase implements PaymentGatewayIn
+    if (!isset($avs_code_meanings[$card_type][$avs_response_code])) {
+      return NULL;
+    }

The only thing would be that this should return an empty string instead. BUT it's a common pattern we already have and throughout Drupal.

🥳 Committed.