Allow webform_civicrm to work with any payment processor

[mattwire]

Now we are using validatePaymentInstrument we don't need to hardcode any of the credit card validation in the webform_civicrm as it's all handled through that function. This PR adds the necessary changes to allow webform_civicrm to use payment processors that do not have credit card fields (tested with smartdebit).

WIP pending further testing - I think this is good to merge, but waiting for it to be tested by a couple of clients first!

[colemanw]

@mattwire cool, thanks for this. But will this break Civi 4.6 compatibility with this module? If so, we'll need to switch between old and new handling depending on Civi version in use.

[mattwire]

@colemanw I don't think there is anything in there that will break 4.6 support but I'm unable to test for a few days yet as I don't have access to a 4.6 dev environment.

[mattwire]

Ref #106 #90
Notes for myself: #106 is redundant with this PR. #90 is required by this PR and needs including.

[mattwire]

@colemanw Now verified on 4.6. A minor change was required (5d4fa90) to retain compatibility with 4.6.

With a little more testing this should be good to merge. @KarinG Could you confirm if iats is working properly for you with this PR (it works for me) as this includes the previous changes on #106 and #90.

[aydun]

@mattwire Just to note that I have successfully tested this on 4.7 using Stripe. I have not attempted to test the new features, but it does not appear to break anything in my limited testing of existing features.

[KarinG]

Hey @mattwire - so some non-cc payment processors were already working fine w/ webform_civicrm & 4.6 or 4.7 -> notably iATS ACHEFT (Direct Debit/e-Cheque) - I will want to make sure that this does not break that; will try get to that soon.

[davejenx]

Tested successfully on 4.7.27 using org.civicrm.smartdebit .

[KarinG]

Thank you @davejenx - I want to make sure this does not break an already (pre-PR) working DirectDebit payment integration; will try find time soon.

[KarinG]

Hey @mattwire - sorry for the delay - key thoughts:

1. on the Title of your PR -> webform_civicrm to date - is already working with non-Credit Card payment processors; we run ACHEFT (which is really CiviCRM core's Direct Debit with some relabeling of field labels to support Canadian and US terminology) through webform_civicrm; So would it be more accurate to say what we're trying to do here is support JS Payment Processors (like Stripe)? Which we are interested in supporting but I really need the before and after on this. As the title alone does not.

2. You mentioned (at the top of this PR) that:

Now we are using validatePaymentInstrument we don't need to hardcode any of the credit card validation in the webform_civicrm as it's all handled through that function.

We had to - however - revert the PR in which you introduced validatePaymentInstrument #107 as it produced a critical regression in credit card processing - for more than one payment processor;

I see you're re-introducing validatePaymentInstrument in this PR; Did you test this PR with e.g. iATS TEST88 credit card and/or Authorize.net fake visa (the funny one with 15 digits);

[mattwire]

Hi @KarinG thankyou for your feedback.

on the Title of your PR -> webform_civicrm to date - is already working with non-Credit Card payment processors; we run ACHEFT (which is really CiviCRM core's Direct Debit with some relabeling of field labels to support Canadian and US terminology) through webform_civicrm; So would it be more accurate to say what we're trying to do here is support JS Payment Processors (like Stripe)? Which we are interested in supporting but I really need the before and after on this. As the title alone does not.

Right. So I've changed the title from "non-creditcard" to "any" as that is more accurate.
Before
webform_civicrm works with a set of payment processors that depend on hardcoded validation coded into the webform_civicrm module.

After
webform_civicrm works with any payment processor that is implemented in a standard civicrm way. Hard-coded validation in webform_civicrm for specific processors is removed. There is a possibility that an older payment processor may break if it doesn't implement the relevant functions but it is likely that they will also break with civicrm core if that is the case.
This PR has been tested with:

• Smartdebit.
• Stripe (note that stripe is unreliable with the existing hardcoded customisation, but does work properly with this PR).
• Sagepay (circle and omnipay versions).

It should be tested with:

• IATS
• AuthorizeNet
• Other major payment processors?

Background
Before this PR webform_civicrm hardcodes a fair amount of handling for specific payment processors and then passes a modified set of fields across to the civicrm method validateCreditCard which should not be called directly - validatePaymentInstrument should be called instead which will call the appropriate method and can be overridden/re-implemented by payment processors as required.
Note that validatePaymentInstrument has the ability to do far more validation, or none at all (eg for Stripe which is already validated using JS).

You mentioned (at the top of this PR) that: Now we are using validatePaymentInstrument we don't need to hardcode any of the credit card validation in the webform_civicrm as it's all handled through that function.
We had to - however - revert the PR in which you introduced validatePaymentInstrument #107 as it produced a critical regression in credit card processing - for more than one payment processor;

The reason behind this was actually because we left in the hard-coded customisations which modified fields before passing them to CiviCRM. So now the hard-coded customisations have gone with this PR the failure with validatePaymentInstrument is gone too.

I see you're re-introducing validatePaymentInstrument in this PR; Did you test this PR with e.g. iATS TEST88 credit card and/or Authorize.net fake visa (the funny one with 15 digits);

No I haven't. I will try and find some time but I'm not really very familiar with those two processors as I've never used them. Do let me know if you get chance to test with one/both of them.

[KarinG]

Hey @mattwire - it's really the older/core payment processors - for which the validation in core was hard-coded - that need to be tested, i.e. Authorize.net and PayPal; and 4.6 - webform_civicrm module still supports many 4.6 sites;

So - how about?
[KarinG] - iATSPayments on 4.7
[KarinG] - Authorize.net on 4.6
[Matt] - PayPal on 4.6 + 4.7

[KarinG]

iATSPayments on 4.7 - all good - tested with TEST88 / Credit Card and TEST88 / ACHEFT - custom fields that we inject via extension (chq img; field labels + validation depending on CAD vs USD) - works well:

[KarinG]

Authorize.net on 4.6 - unfortunately this PR breaks credit card transactions with Authorize.net [iATS Payments - works on 4.6]

1: 7 Credit card expiration date is invalid.

[KarinG]

And I suspect you'll find the same with PayPal...

[mattwire]

@KarinG Just to confirm, did you test authorizenet on 4.7 or just 4.6? And is it only broken on 4.6?

[KarinG]

I ran that on a 4.6/production clone (I don't have any authorize.net clients on 4.7)

[mattwire]

@KarinG I just pushed a possible fix for authorizeNet on 4.6 based on @eileenmcnaughton comments. I haven't tested it yet though as I still need to setup a 4.6 environment.

[KarinG]

That makes sense - that should work; I can pull that in and double check that;

[KarinG]

What about PayPal - can you do a PayPal test on 4.7?

[eileenmcnaughton]

@mattwire fix above looks right -

The unfortunate thing is we have never documented what parameters the payment processors should expect to get in the $params array and I think since 'year' & 'month' have always been passed through there is reasonable to see those as part of the contract.

But I think over time we should migrate to using getters on CRM_Core_Payment & try to deprecate the concept of processing the params array in the processors - so we would have

$this->getAmount()
$this->getAmountInCents();
$this->getExpiryYear{);
$this->getBillingFirstName();

etc

[mattwire]

I've now tested Paypal pro on 4.7 (Note that I can't complete because I don't have a validated paypal pro test account but running via the phpstorm xdebugger I get past the validation fine then it fails with "security key is not valid").
I pushed another commit to add ip_address, country and state_province to the list of params passed by webform as paypal pro expects those to be there and PHP notices if not.

[eileenmcnaughton]

Yeah - I would be wary of anything in this commit that causes something previously passed to be no- longer passed - that also seems like a tangental change to the main goal of using validatePaymentInstrument etc

[mattwire]

@eileenmcnaughton @KarinG I've just dropped the final commit from here (which added paypal pro parameter fixes for php notices) as I now realise this has nothing to do with my webform changes here and is a general core issue which is now included in this Work In Progress PR against core here: civicrm/civicrm-core@d46f1d6

[KarinG]

Hey @mattwire - so we need to park this until that core issue gets in - or we break Paypal/Webform integration - is that right?

[mattwire]

Hey @mattwire - so we need to park this until that core issue gets in - or we break Paypal/Webform integration - is that right?

@KarinG No, the paypal pro notices are nothing to do with my changes to webform, I thought they were until I ran across this on stackexchange https://civicrm.stackexchange.com/questions/24316/paypal-pro-billing-address-state-province-country-not-being-passed
So there is an issue in core, but it should not hold up this PR as it affects native Civi forms as well.

[KarinG]

Hey Matt - we still need some all the way through testing w/ PayPal on this PR though;

[petednz]

On the same form that we have stripe and gocardless we also have Paypal. I successfully completed a PayPal payment with outcome in civi being that the payment was completed.

[colemanw]

@petednz so that was testing this PR specifically? And the functionality was not working before?

[petednz]

I can't give that answer - @jitendrapurohit may be able to test to confirm. afaik paypal had not been retested after the upgrade - we found stripe + gocardless were not working post upgrade - this patch fixed for those two - i was then checking to see this fix had not broken paypal - all i can say at this point.

[jitendrapurohit]

@colemanw Stripe payment processor was failing before this PR and got fixed after applying this changes. GoCardless additionally needed #152 to function correctly.

[KarinG]

@petednz @jitendrapurohit - thank you for testing this on PayPal.

@colemanw - these Edits are required for Stripe to work with webform_civicrm; they are fine with payment processor extensions - but risky to core-baked-old payment processors like authnet (that failed and Matt fixed it); only outstanding test was a PayPal one - and Fuzion just confirmed that for us.

So merging this.