False-positive duplicate detection caused by webform not passing correct parameters to Authorize.net - it needs to pass contributionID
(Joinery internal reference: "Trello#142: Membership Application and Payment Process Not Working")
From what I understand so far, it seems there's a well-thought effort to use propertybag to standardize parameter names in payment processors. Here's one area where this effort seems incomplete and easily improved:
There are a few places in CRM_Core_Payment_AuthorizeNet::doPayment()
where directly accessing $params
could (and does) cause problems; it seems right to switch these to $propertybag
. (BTW I think that usage of propertybag
in the context of alterPaymentProcessorParams hook is still not appropriate, so I'm not suggesting changes to that usage within this method.)
Specific observable problem: (I can create a more detailed and stripped-down recipe if needed, but hoping to avoid the effort.)
- On a site with: D7, civicrm 5.49.5, webform_civicrm.module, and contributiontransactlegacy extension
- We have a webform which accepts membership signups, payable through the core Authorize.net payment processor.
- Payments made through this webform are actually transacted in Authorize.net, but the user is given the error, 'It appears that this transaction is a duplicate. Have you already submitted the form once? If so there may have been a connection problem. Check your email for a receipt from Authorize.net. If you do not receive a receipt within 2 hours you can try your transaction again. If you continue to have problems please contact the site administrator.'
- FWIW I have observed that the below corrective measure fixes this problem for this site.
Why this error happens:
-
CRM_Core_Payment_AuthorizeNet::doPayment()
calls$this->checkDupe($authorizeNetFields['x_invoice_num'], $params['contributionID'] ?? NULL)
(see line 171) to detect a duplicate transaction. - However,
$params['contributionID']
is undefined. (On the other hand,$params['contribution_id']
is defined, as is$propertyBag->getContributionID( )
). - Since
checkDup()
gets a NULL value for$contributionId
, it always thinks the contribution is a duplicate, so we get the above error message.
Corrective measueres
- Seems like the intent of
propertybag
is to clear up exctly this kind of naming inconsistency. I suggest changing Line 171 to use$propertyBag->getContributionID( )
instaed of$params['contributionID']
. -
Line 146, which tries to read
$params['is_recur']
and$params['contributionRecurID']
, is probably also a good candidate for similar cleanup?
Should I go head with a PR or is more discussion needed?