CRM-19792, fixed payment processor params to include email

[pradpnayak]

---

• CRM-19792: Authorize.net membership renewals with credit card not processed though event regs are
  https://issues.civicrm.org/jira/browse/CRM-19792

[omarabuhussein]

from coding perspective the PR looks good to me , but I am not sure how to manually test this since it will require Authorize.net test account . So if there are steps to follow in order to test this I would be happy to follow , otherwise it is approved from my side.

@pradpnayak

[pradpnayak]

Here is the test account for authorize.net
https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/CiviTest/CiviUnitTestCase.php#L1306

You can create a new one at https://developer.authorize.net/hello_world/sandbox/ and configure in CiviCRM using https://wiki.civicrm.org/confluence/display/CRMDOC/Authorize.net+Configuration

I am not sure if i am allowed to paste here my authorize.net account. :( I can inbox you if you want.

[eileenmcnaughton]

I think there are Authorize.net creds in the test suite

[omarabuhussein]

thanks guys , usally payments prcessors are require some extra stuff like public IP address .. etc .. but it appear that authorize.net is not enforcing that and any one can setup an account with it . so I just created the account and just want to test this on civi.

[omarabuhussein]

@pradpnayak
just tried with and without your fix by setting :

• authorize.net as the default payment processor
• requesting and paying for membership via test membership create/renewal page
• then changing the membership status to grace as well as its start and end date to be in the past \
• then renew the membership via test drive page again

but contribution status for the membership was fine (turned to completed ) and the membership get renewed .. so I am not sure how it possible to replicate the issue ( how did you replicate it so I can follow the same steps ? )

[eileenmcnaughton]

I suspect it might be an account setting in A.net - ie. if A.net is configured to send out an email it will

[pradpnayak]

One of JMA's client had a problem. Initially i couldn't replicate this error on test environment, but we replicated this live instance. The transaction submitted to Authorize.net was failing because the email was no longer being submitted to the payment processor and it required it. Some googling suggests that there may be a setting in Authorize.net account that determines if this is a required field.

[omarabuhussein]

I just set the email field as required from authorize.net and tested again with and without this patch and both way it is still work fine .. just to make what I did clear here are the steps :

1- set authorized.net as my default payment processor and fill up my test account details there.
2- accessing test "Member Signup and Renewal" as an anonymous user and applying for membership using example@test.com as an email.
3- confirming that test membership and contribution are created ( both get created and contribution status is completed)
4- changing the membership start and end data to be in the past and changing its status to be Expired.
5- accessing test "Member Signup and Renewal" as an anonymous user again and using the same email example@test.com I completed the payment process.
6- confirming that membership is renewed ( yes without problem, the status become New)
7- confirming a new contribution created for membership renewal and with status = completed ( yes without problem )

let me know if there something else I can do or if there is another way to test this .

[pradpnayak]

As i said earlier in my comment, i could not replicate this on test environment but I could on live authorize.net. But when we have simple contribution without membership, email is send in payment processor params, but not incase of membership. Therefore i added those 2 lines to add email fields or any other required fields to payment processor params.

@JoeMurray your thoughts on this? Since you were also able to replicate issue on live credit card processing.

[JoeMurray]

We were not able to access the client's Auth.net account interface, but Hershel, client and us were able to see problem without this and see it fixed with this. There are numerous reports of this error with Auth.net for other software using Auth.net visible via googling. There are ways clients can configure Auth.net to require it according to Google. There are no problems providing it as an extra param sent when not required. Let's merge this @eileenmcnaughton is my recommendation.

[eileenmcnaughton]

I've been kind of ignoring this one a bit because I feel a bit uncomfortable with it. My issue is not so much whether it fixes a bug, but whether it introduces more fragility. I believe that this function is called in other places along the path of the form processing and this is adding a couple more. My suspicion is that it should be called once fairly early on in the flow and we should ensure that the parameters are passed along.

From a test pov we can check in the alterPaymentProcessorParams hook what variables get passed to it & we could add that check to various tests to ensure they are consistent.

[eileenmcnaughton]

OK, I'm going to merge this. I agree it is safe & fixes the issue and it would be nice to get it sorted. I did take a look for a better solution and while I had some thoughts I think fundamentally we need to start to talk about what is making those forms so dubious, and basically it is the fact that a whole lot of arrays are being passed around without clear separation of concerns, and even the whole form object is being passed around. Debugging is made harder by the fact that they are often passed as reference & randomly altered. My general thoughts about taking steps towards making this code less toxic are

• never pass a value by reference when you can help it
• try to only pass values by reference to really small functions
• try to extract out a chunk of the code into a smaller function any time you touch one of these big nasty functions
• unit tests, if you fix something & don't add a unit test (e.g here) you should view it as 'temporarily fixed, may break again' Unit tests should test the form submission process where possible.
• prefer to pass lots of single variables over arrays or objects. This is counter intuitive but when working towards refactoring & clarifying what is actually being passed it is useful

[omarabuhussein]

prefer to pass lots of single variables over arrays or objects. This is counter intuitive but when working towards refactoring & clarifying what is actually being passed it is useful

1. passing a lot single variables : there is a lot of researches on the best number of parameters to pass to a method , but according to code complete book , number of parameters should not be more than 7 otherwise it will be hard to track the method , and we could argue about that , I personally think that the number of parameters should not be more than 5 . if the method require more than that then it need serious refactoring .

2- This is counter intuitive : I believe passing arrays is counter intuitive and will require the developer to know about the implementation details of the method which is not right . but for objects case , I think it depends .. as long as proper interfaces are written and get injected into the class then that's great .

try to only pass values by reference to really small functions

I think passing values by reference should be avoided as much as possible even for small functions (methods)

[eileenmcnaughton]

@omarabuhussein my comments are not necessary about best-practice per se. I would advocate different things if writing new code, but when trying to unravel toxic & unpredictable code I think it helps. This is about the process of how to chip away at refactoring bad code.

We have a lot of code where a bunch or arrays are passed around, or objects that do not have proper interfaces / patterns/expectations. In this case I think it's worth seeing what is used from those & if 2 properties are used & the rest ignored then pass in those 2 properties rather than the array. My thinking is largely motivated by the fact that if you pass an array in then later someone will hack in a bug fix that adds another way of using it. So, when trying to unravel code, or working on toxic code I advocate many specific parameters being passed in.

[omarabuhussein]

make sense