Use the payment processor payment method instead of the default configured one

[omarabuhussein]

Before

If you have a payment processor with the payment method configured to be for example Cash or EFT or whatever, then generated contribution after submitting the webform will always use the default payment method instead of the payment processor payment method.

For example we have here a payment processor that is using manual payment class and configured to use "EFT" payment method :

and the site default payment method is set "Check" :

After submitting a webform payment using this payment processor, here is generated contribution :

As u can see, it used the default site payment method instead of the payment processor payment method.

After

I changed the code to use the payment processor payment method instead :

Know issue

What is fixed in the PR above will only work for payment processors that are configured to use "Payment_Manual" class, in which payments will be considered as an offline payment. The reason for live payments, we are using the Contribution API transact action which will change the instrument method based on the payment processor payment_type field.

The code in this line shows that https://github.com/civicrm/civicrm-core/blob/master/api/v3/Contribution.php#L437

And as you can see, if the payment type of the payment processor is a "credit" then the API will change the payment method to "Credit Card", and if the payment type "debit" then it will change the payment method to "Debit Card"

This isssue is fixed in this Core PR : civicrm/civicrm-core#13073

[KarinG]

Thank you @omarabuhussein and @guanhuan for explaining this to me this morning. I now understand why we have not seen this before with 'online' payments.

@mattwire - do you have any payment processors using Payment_Manual class? I like the idea of taking the configured payment method (which affects Financial Accounts) over the default one; it could cause some unexpected issues for people having gotten used to the default behaviour - but that's not a reason not to fix this. Would you agree?

[mattwire]

So the CRM_Core_Payment_Manual class is effectively part of the dummy payment processor. I think that @omarabuhussein / @guanhuan are extending it in their own extension. I don't entirely understand how it relates to the CRM_Core_Payment_Dummy class (@eileenmcnaughton may..).

However, based on the description of the issue, and a quick review of the code it feels like we should not be wrapping changes to the behaviour in webform_civicrm module and should be looking at a "fix" in core if it is not doing the right thing:

The reason for live payments, we are using the Contribution API transact action which will change the instrument method based on the payment processor payment_type field.

In particular, this makes me think the fix should be in core. If we are not using API functions for the Payment_Manual class it is very likely to break in a future CiviCRM release as further changes to payment classes are made (for example, the link to apiv3contributiontransact above points to code that should really be eliminated as "payment_type" has been effectively deprecated for quite a long time (though not fully removed from core because of legacy and usage by built-in payment processors).

[eileenmcnaughton]

So for all intents & purposes the Manual processor is like any other processor. In core it can be used as a pseudoprocessor when the processor id is 0 - aka pay later - but that's irrelevant here.

Basicaly the way to get the payment_instrument_id is to 'ask the processor' -ie something like

$processor = \Civi\Payment\System::singleton()->getById($id);
$payment_instrument_id = $processor->getPaymentInstrumentID();

[eileenmcnaughton]

Also note that the transact api setting the payment instrument id incorrectly as commented by @mattwire - there are no unit tests on the transact api & I mistrust it as it creates the contribution without knowing if it should be linked to a membership & I have no idea how the financial items get fixed up afterwards to be correct.

I have always treated it as unsupported due to the 'api contract' always being that apis without tests are unsupported. Still I guess with a unit test we could at least fix the line that sets the payment_instrument_id - but I wouldn't like to imply any warranty that the financial entries it creates are supported.

The transact api is never used in core code - and I'm pretty sure all core places work using the approach I mentioned

[omarabuhussein]

I thought about creating a core PR to set the payment_instrument correctly for transect API so it does not depend on the payment type anymore but I wasn't sure if it would be correct or not, but knowing that the payment_type field is deprecated make sense so I am more confident now to create a core PR to fix it.

I am also not planning to replace transect API call here since it is used inside the webform module for a long time and replacing it with other API call might require much more changes (most likely) which I think something @KarinG would not prefer, right Karin ?

[KarinG]

Hi Omar - please just fix up transact API in Core. Replacing transact API is on our wish list as we’re working on D8WFC. But it’s not top of the list as Webform CiviCRM is generating good financial data currently because historically it has never just counted on Transact API alone to create them.

[omarabuhussein]

I see, cool then, I will make a core PR soon this week.

[eileenmcnaughton]

@omarabuhussein you could also copy transact api into webform & tweak the name & modify it there - that would mean that we could move towards deprecating it out of core.

[omarabuhussein]

@eileenmcnaughton

If @KarinG is okay with that, then fine to me.

[KarinG]

I think as long as Transact API is in Core - a small fix in Core so that you get the correct payment method returned for your use case - is the best solution.

Please note Webform CiviCRM is not the only module using Transact API. CiviCRM Entity ( @jackrabbithanna @dsnopek ) does too e.g. and there are likely more applications that use it.

[jackrabbithanna]

@eileenmcnaughton if Transact API is going to be deprecated, then you want the Order API to be the standard now?

This will affect CiviCRM Entity Price Set field and cause it to break badly without the Transact API...

If you read through that module, you can see how I work with the Transact API

https://github.com/eileenmcnaughton/civicrm_entity/blob/7.x-2.x/modules/civicrm_entity_price_set_field/includes/civicrm_entity_price_set_field.transaction.inc#L254

If the transaction is successful
https://github.com/eileenmcnaughton/civicrm_entity/blob/7.x-2.x/modules/civicrm_entity_price_set_field/includes/civicrm_entity_price_set_field.event_registration.inc#L738

then I do all the participant creation, participant payment, line items etc.. manually:
Starting here:
https://github.com/eileenmcnaughton/civicrm_entity/blob/7.x-2.x/modules/civicrm_entity_price_set_field/includes/civicrm_entity_price_set_field.event_registration.inc#L827

[eileenmcnaughton]

@jackrabbithanna I'm not suggesting any immediate move to deprecate it -but long term it has never been tested or supported & is not reliable from a financial records point of view so I'd rather reduce reliance on it

[jackrabbithanna]

I arrived at the solution for that module, by painstakingly testing what is actually created by the standard event registration form, and then duplicating that with the API...

What would be helpful is if we all knew the "one" way it should be done, and if that's Order API then that's great...

Transact is good for just making a contribution, nothing else really....

Can you describe what is off with it's handling of financial records....Not saying it's the way forward necessarily for webform_civicrm, but I wonder if Order API does say 8 things.
creates contribution after runnning cc
creates line items
creates participant/membership payment etc...

and out of the things it does, does Transact do at least one or two of those steps correctly?

Transact has also been useful for us, in custom work where we break up orders into different CC transactions depending on custom criteria..

For instance some clients want separate calls for memberships, and contributions depending on custom criteria...

I guess what I'm saying is there's utility in being able to run a transaction via the API without having a full order created.

If we could zero in on exactly how transact is not reliable, and then get some test coverage for it, in conjuction with equivalent tests for Order API, and at least step1-x of order API resulting in the same thing as a transact call, that could be a good thing

Unfortunately I don't know enough about the Order API yet, and there's very little documentation.

Perhaps order API can do what transact does already, that is just a simple transaction with contribution?

[eileenmcnaughton]

@jackrabbithanna TBH I'm not sure if Order api is mature enough - I certainly haven't found time to put the work into it I would like to - hence I'm not strongly pushing a change as this stage. However I think ideally the process would be something like

Order.create
Payment.pay (that is in Omnipay but not core & basically processes a payment without any related entrieds
Contribution.completetransaction

If the flexible form builder takes off that might drive a cleanup in this area as we'll want to use best practice when we incorporate payments into it

(transact is basically a contribution.create call with payment.pay call bundled in)

[jackrabbithanna]

@eileenmcnaughton 10-4 This is a good discussion to have sometime though...making the "one" way to do full orders of any kind, and "full orders" is composed of a consistent set of steps, and perhaps each step could be done independently via API...
and all of it under test coverage/contract...

If there was one API way to do it, it would be super sweet if the Contribution and Event registration stock forms used it too...and the flexible form builder, and webform, and civicrm entity, etc...

Agree this could be an action item in conjunction with flexible form builder

One API to Rule Them All, and in the Consistency Bind Them !!!

[eileenmcnaughton]

@jackrabbithanna I totally agree! I would probably like to add a +1 to every single line of the above comment.

[jackrabbithanna]

So it seems like the Webform initiative is an opportunity to fine tune core's APIs, both transact and Order API, and start getting some test coverage/contracts in place, in preparation for the bigger and better that will be coming in Core...

Maybe the "now" fix is tuning transact, with a mind to replace it with Order as it matures...It may be worth trying out the Order API, and tuning it up and getting test coverage/contracts going, from a long term perspective.

I think it would really be useful to understand what transact and order api both do that is the same, and what the differences are as far as data results in the db.

Webform is the biggest consumer of the API, followed by CiviCRM Entity, and probably some of the Mobile stuff going on...If this could get settled, test coverage, documented etc... It would be a huge plus to the community as a whole.

It makes sense to me that the APIs are kept in Core and maintained and the test contracts there....and Webform etc.. just uses them..so that everyone else can use them too!

Every problem is an opportunity!

[eileenmcnaughton]

:-)

I should amend the above ideal flow to

Order.create
Payment.pay (that is in Omnipay but not core & basically processes a payment without any related entrieds
Payment.create

as that will complete transaction as required

[jackrabbithanna]

@dsnopek
How far off is the Webform code from being able to have a Drupal service that handles transactions?
Encapsulate all the transaction stuff in one service, that could be modified with Service Decorators or some other nifty D8 technique...So maybe using transact++ could be used, but hot swapped for the Order API, or even something custom?

OR

Is it worth putting that kind of a service into CiviCRM Entity and having Webform CiviCRM depend on it?

[jackrabbithanna]

If "transaction processing and post processing" is abstracted into a service, and taken out of the form process code directly, then it could be used a bunch of different ways, by different pieces of functionality, and would lend it self to numerous custom scenarios that developers could use to make cool custom stuff ... :)
If there was ever a case for services, this is one.
You could then have multiple teams working on different versions of a "transaction processing" functionality in tandem...and nothing may need change in Webform's form handling if they swapped out.

[eileenmcnaughton]

@jackrabbithanna I'm loving your enthusiasm - but I have this image of an excited rabbit ricocheting off the walls & I'm just thinking 'I haven't had coffee yet'

[KarinG]

@jackrabbithanna - Matt, David and I started looking at Contributions/Payments in D8WFC this morning. We already have introduced some D8-niceties in D8WFC and have already introduced services (for wf_crm_field_options e.g.) -and will look for more opportunities along the way.

Let's get back to this issue though - does Omar have an ok for a small fix to Transact API in Core?

[mattwire]

Let's get back to this issue though - does Omar have an ok for a small

fix to Transact API in Core? I would say so.. it will need to come with a basic unit test though.

…

On Mon, 5 Nov 2018 at 20:06, Karin Gerritsen ***@***.***> wrote: @jackrabbithanna <https://github.com/jackrabbithanna> - Matt, David and I started looking at Contributions/Payments in D8WFC this morning. We already have introduced some D8-niceties in D8WFC and have already introduced services (for wf_crm_field_options e.g.) -and will look for more opportunities along the way. Let's get back to this issue though - does Omar have an ok for a small fix to Transact API in Core? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB9QQZh6W1JRh7ar3cKwKwRy82T0zsDrks5usJpjgaJpZM4YHL5T> .

[eileenmcnaughton]

Actually @mattwire I'd say leave the test off for now - I really don't want to imply it's supported - I'm happy to merge it if you review it

[eileenmcnaughton]

@jackrabbithanna I just found out Caldera Forms is using Order.create so that revises my opinion about it's maturity - from Andrei

"the Order is created here, the line items are fed from a transient object, and some metadata like trxn_id, fee_amount, etc. is fed from the payment add-ons"

So that is probably a good model to follow - although I would recommend creating the order as pending & adding payments using payment.create as that works with all payment methodologies

[omarabuhussein]

My problem with the 'order' API is its weird syntax https://wiki.civicrm.org/confluence/display/CRM/Order+API and it is lack of clear documentation.

Anyway, I believe then what I need to do now is to create a core PR to fix that line in the Transect API and we are good to go. Cool then, Thanks a lot everyone :) .

[eileenmcnaughton]

@omarabuhussein I sympathise with that on the order api - although I find the progress caldera forms made using it encouraging

[omarabuhussein]

So here we go :

1- I changed this PR to use this :

$processor = \Civi\Payment\System::singleton()->getById($id);
$payment_instrument_id = $processor->getPaymentInstrumentID();

to get the payment instrument id.

2- I created this PR : civicrm/civicrm-core#13073 to fix the Transact API

[jitendrapurohit]

This patch also fixes the display of additional display of financial trxn entries explained in the description of #270.

The patch doesn't apply directly so I tried to manually apply the changes and confirmed that it fixes the issue seen on that PR too. @omarabuhussein Though github doesn't seem to indicate that, can you update this if it has gone stale?

[omarabuhussein]

I rebased my feature branch with with 7.x-5.x @jitendrapurohit , try again now

[eileenmcnaughton]

@colemanw this looks mergeable.

I was just looping back here to add the link to my new doc on how to get off Contribute.transact civicrm/civicrm-dev-docs#705 - but I note the code as stands is correct & I don't want to muddy the waters on that.

The conversation about Order.create being immature is a year old. Since then I became aware that Caldera forms is successfully using it & also Barcelona a couple of people dug into it & found it to be fairly solid

[colemanw]

Agree this looks good. Thanks @omarabuhussein for rebasing.

[KarinG]

@omarabuhussein - can you please review -> https://www.drupal.org/project/webform_civicrm/issues/3108529

[KarinG]

Reason I ask is a site is reporting a (possible) regression related to this PR in that issue.

[omarabuhussein]

@KarinG

will try to find time today or tomorrow morning to look into it

[KarinG]

Thanks Omar 👍

[omarabuhussein]

@KarinG #289

[francescbassas]

This is exactly what happened to me here https://lab.civicrm.org/extensions/redsys/issues/33.
Updating to 7.x-5.0 solves the problem. Great!