dev/core#513: Using payment_instrument_id instead of payment_type to determine the payment instrument when using the Transact API

[omarabuhussein]

Before

When using the Contribution.Transact API, CiviCRM will either change the payment instrument to "Credit Card" or "Debit Card" based on the the value of payment_type field.

After

This is wrong and the payment instrument type should be taken form the used payment processor payment_instrument_id field which is what I fixed below.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[JoeMurray]

I just want to caution that we should test this against a variety of payment processors and in various scenarios (eg front and backend payments via payment processor as well as setting payment instrument to credit card when recording a PoS payment, etc) to ensure that the change doesn't cause unexpected issues.

[pradpnayak]

The fix make sense to me. I agree with @JoeMurray we need to some thorough testing on this. It would be good if you could add some api test to support your fix.

[omarabuhussein]

Not adding tests for this was something @eileenmcnaughton specifically asked for here : colemanw/webform_civicrm#180 (comment)

[JoeMurray]

@eileenmcnaughton I'm familiar with tests on APIs implying support. This looks like a potentially good but nonetheless risky change in terms of creating unexpected behaviour. What are your recommendations on adding unit tests here. NB: adding unit tests is separate from requiring manual testing in a diverse set of payment processor contexts.

[eileenmcnaughton]

@JoeMurray so the issue is that this api is not just unsupported but not recommended and known to create poor data outcomes if people then attach the created contribution to a membership. Hence my preference is not to add a unit test on this minor tweak and to take steps to deprecate it (which will remove it from api explorer at the very least)

[eileenmcnaughton]

This is the recommended pattern for this code and is something we have in place in many many many places in the CiviCRM code - assuming @omarabuhussein you have actually tested it yourself I'm happy to treat you as the 'reviewer' in this case as I proposed the actual code change

[JoeMurray]

Let's mark the relevant api as deprecated, and aim to remove in the not too distant future. But I'm uneasy with this being merged without significant testing since it has the smell of something that might break existing implementations.

[eileenmcnaughton]

@JoeMurray the reason I'm confident is that the CRM_Core_Payment object has the called function on it - so it always exists. The data comes from required field in the civicrm_payment_processor table - unless a specific class overrides it and all core places use this method - we just never fixed transact because it's not supported & doesn't work very well anyway

[omarabuhussein]

assuming @omarabuhussein you have actually tested it yourself I'm happy to treat you as the 'reviewer' in this case as I proposed the actual code change

I've tested it in the context of webform (by submitting a webform that uses a live payment processor) and all is fine, but let me know if u think that is not enough @eileenmcnaughton & @JoeMurray

[eileenmcnaughton]

@omarabuhussein ok - I'm merging based on your tests - I'll follow up later with some clearer deprecation of this function. At some point we should bring payment_processor.pay into core & fully remove this function

[omarabuhussein]

thanks @eileenmcnaughton