robbrandt commented 3 years ago

We recently upgraded to the iATS extension v1.74 in order to deal with #326 "cryptogram" error. Previously we were on a slightly older version maybe 1.72?

We have run into a problem where $0 transactions result in an error:

Error: There is a problem with the payment gateway. If the problem continues please contact support.

$0 transactions happen when a 100% discount code is used using CiviDiscount. A "gift" membership, in our case.

We didn't have a problem with these as far as I know prior to 1.74.

Suggestions?

KarinG commented 3 years ago

Hi Rob - you can't submit $0 to a Payment Processor. In fact for iATS minimum is $1 as $0.10 would cost more in transaction fees than the $0.10 would bring in. I would recommend you create a separate Contribution page to handle Gift Memberships [and don't hook up a Payment Processor on that page].

robbrandt commented 3 years ago

Thanks. Obviously sending a $0 transaction to a payment processor is pointless, but the scenario needs to be handled appropriately by the extension. It worked prior to 1.74. And it worked with FirstData. Seems like there should be logic in the extension that if the amount is zero, the function should just return true (or whatever value makes sense).

It's not a simple enough scenario to use a different form. CiviCRM actually handles it nicely. If a discount code results in a transaction amount of zero, the payment section collapses/disappears and the form just gets submitted. The iATS extension needs to do this as well.

I'll send you a URL you can try via Mattermost so you can see how it should work.

adixon commented 3 years ago

As a general statement, it's clear that a 0$ transaction shouldn't be sent to a payment processor.

My assumption is that civicrm code shouldn't be invoking the payment processor extension code if the amount is $0, and I believe that has been the case in the past.

But the first pay code does work differently, and it's possible that the mix of discount codes w/ first pay is generating a $0 request to the payment processor code and perhaps the extension should be handling those explicitly.

On the other hand, we do have lots of clients using first pay and this is the first report, so it might be core code related as well.

robbrandt commented 3 years ago

I'll tell you everything I know.

We were long time users of FirstData payment processors using the core settings for them. But it's unmaintained and has gotten flaky so we switched to iATS in early December. We were using 1.72 of the extension. I can confirm that our gift memberships worked fine.

Peggah at iATS confirms that we are using First Pay.

We are still using CiviCRM 5.13, as we have been stuck on php 5.6 for a while. As of mid-March we have solved the php 5.6 problem and can switch to php 7.x and the latest CiviCRM whenever there's a quiet moment, but that moment hasn't come yet. So CiviCRM 5.13/php 5.6 has been a constant throughout our experience.

I upgraded to v1.74 of the extension on March 31 to deal with the cryptogram error, as I mentioned earlier. Soon after, we started getting this error message. I can confirm that 1) we have no reports of that error prior to the upgrade, 2) we started getting reports of errors after the upgrade, 3) searching our contribution records, there are no instances of successful "free memberships" since March 31, 4) I have run real transactions simulating the gift process, and adding a $1 donation to the gift process results in a successful transaction, and 5) adding nothing to the free membership ($0) results in the failure.

The only thing that's changed has been upgrading to v 1.74 from 1.72.

adixon commented 3 years ago

Thanks for the extra details. There was a 'rebasing' of the extension in that upgrade, deprecating some old methods. I suspect that might be the issue considering you're running such an old civi version. Here's a summary:

https://github.com/iATSPayments/com.iatspayments.civicrm/compare/1.7.2...master

Strategically, I'd suggest you prioritize upgrading your civi version before anything else, that's a security concern that you don't want to expose any longer than you have to.

I do recall in an older version of civicrm that it used to generate 0$ membership contributions and no longer does.

robbrandt commented 3 years ago

Thanks. Could I ask you to humor me, and test this? Create a contribution page with a membership section, using a priceset, and a membership that costs $0 and see if you can complete the "transaction". I did just that, and it still failed. So it doesn't appear, at least, to have anything to do with the discount code.

My calendar is full so doing the upgrade now, with all the requisite testing of our extensions, would be a problem. And I am not even sure it will solve the problem.

tommybobo commented 3 years ago

I am also encountering this issue. I have isolated the bug to this point in the git history - https://github.com/iATSPayments/com.iatspayments.civicrm/commit/89a435c7a15e860a2aeaa4c6767bbd4eb1d83a08

It looks like the switch from doDirectPayment to doPayment is the source of the issue. I am not sure if it is Core or the extension.

I am seeing this on a Wordpress 5.7.1 with PHP 7.4 and CiviCRM 5.36.1- no plugins or additional extensions are turn on

robbrandt commented 3 years ago

Since we are using totally different versions of CiviCRM it seems very unlikely to be a core issue. The only commonality is a new version of the iATS extension.

KarinG commented 3 years ago

The reason we had to switch from using doDirectPayment to using doPayment is because doDirectPayment is deprecated in CiviCRM Core ->

https://github.com/civicrm/civicrm-core/blob/5be8da6dba41d70cd8f58a0ee59a734b9a58d8b5/CRM/Core/Payment.php#L1304-L1305

tommybobo commented 3 years ago

Yeah - doPayment has a check for $0 contributions but it isn't catching in our cases. https://lab.civicrm.org/dev/core/-/blob/master/CRM/Core/Payment.php#L1374

KarinG commented 3 years ago

I suspect there may be some missing mapping between $propertyBag->getAmount and other variables used re: amounts.

adixon commented 3 years ago

@tommybobo - thanks for figuring that out!

tommybobo commented 3 years ago

@KarinG Sorry for the confusion the $propertybag isn't in use until 5.38. I tested the alpha with no change in result. I am wondering if the Payment is being sent to iATS before the check is even done. -- I am not seeing the error for no billing address when I submit a zero dollar transaction with only an email.

yurg commented 3 years ago

I've opened an issue regarding the same problem here https://github.com/iATSPayments/com.iatspayments.civicrm/issues/356 - it was just closed w/o any real resolution.

KarinG commented 3 years ago

$propertybag was added in 5.20 - but its use has been slowly and strategically increased over time. I'm going to check in with Matt Wire on this.

KarinG commented 3 years ago

@tommybobo - ah I see what you mean now -> this casting '$propertyBag = \Civi\Payment\PropertyBag::cast($params);' was only introduced very recently and not in a release. I've asked Matt to see if/how he handles $0 with Stripe Extension.

mattwire commented 3 years ago

It is not to do with propertyBag but because the switch to doPayment overrides the original method on CRM_Core_Payment and we have to re-implement the 0 amount ourselves.

KarinG commented 3 years ago

Awesome - thanks Matt! 🎉

@tommybobo -> can you please test Matt's PR #363 (Matt says he has not tested it yet) @adixon -> Matt also told me that this is exactly how he recently addressed this for Stripe Extension.

tommybobo commented 3 years ago

@KarinG

I am getting this error.

$Fatal Error Details = array:3 [ "message" => "Property 'amount' has not been set." "code" => null "exception" => BadMethodCallException {#3229

message: "Property 'amount' has not been set."

#code: 0
#file: "/home/redacted/wp-content/plugins/civicrm/civicrm/Civi/Payment/PropertyBag.php"
#line: 263
trace: {
  /home/redacted/wp-content/plugins/civicrm/civicrm/Civi/Payment/PropertyBag.php:263 {
    ›   }
    ›   throw new \BadMethodCallException("Property '$prop' has not been set.");
    › }
  }
  /home/redacted/wp-content/plugins/civicrm/civicrm/Civi/Payment/PropertyBag.php:402 { …}
  /home/redacted/wp-content/uploads/civicrm/ext/com.iatspayments.civicrm/CRM/Core/Payment/iATSTrait.php:32 { …}
  /home/redacted/wp-content/uploads/civicrm/ext/com.iatspayments.civicrmCRM/Core/Payment/iATSService.php:176 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Contribute/Form/Contribution/Confirm.php:1788 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Contribute/Form/Contribution/Confirm.php:1532 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Contribute/Form/Contribution/Confirm.php:2589 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Contribute/Form/Contribution/Confirm.php:2434 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Contribute/Form/Contribution/Confirm.php:858 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Core/Form.php:526 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Core/StateMachine.php:144 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Core/QuickForm/Action/Next.php:43 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/packages/HTML/QuickForm/Controller.php:203 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/packages/HTML/QuickForm/Page.php:103 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Core/Controller.php:347 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Core/Invoke.php:313 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Core/Invoke.php:69 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm/CRM/Core/Invoke.php:36 { …}
  /home/redacted/wp-content/plugins/civicrm/civicrm.php:1172 { …}
  /home/redacted/wp-content/plugins/civicrm/includes/civicrm.basepage.php:354 { …}
  /home/redacted/wp-includes/class-wp-hook.php:292 { …}
  /home/redacted/wp-includes/class-wp-hook.php:316 { …}
  /home/redacted/wp-includes/plugin.php:551 { …}
  /home/redacted/wp-includes/class-wp.php:763 { …}
  /home/redacted/wp-includes/functions.php:1291 { …}
  /home/redacted/wp-blog-header.php:16 { …}
  /home/redacted/index.php:17 { …}
}

} ]

adixon commented 3 years ago

Posted to civicrm issue queue here: https://lab.civicrm.org/dev/core/-/issues/2589

yurg commented 3 years ago

Would be any further movement performed here in regards of https://lab.civicrm.org/dev/core/-/issues/2589#note_58998 please?

adixon commented 3 years ago

I've created a PR here: https://github.com/iATSPayments/com.iatspayments.civicrm/pull/364

Testing welcomed.

robbrandt commented 3 years ago

I've created a PR here:

364

Testing welcomed.

This works for me, even in CiviCRM 5.13. I tested both $0 and $ > 0 transactions.

Thank you for coming through with a fix for this problem.

KarinG commented 3 years ago

@tommybobo -> can you please test Alan's alternate PR -> #364 - It's super simple and very effective. If you can confirm that it works we'll merge it in.

tommybobo commented 3 years ago

@KarinG Its working for me. Thank you for getting this fixed.

iATSPayments / com.iatspayments.civicrm

$0 transactions: Error: There is a problem with the payment gateway. If the problem continues please contact support. #362

message: "Property 'amount' has not been set."

364