Payment processors doDirectPayment and is_recur

[adixon]

For payment processors that use doDirectPayment with is_recur (form type), the civicrm_contribution and civicrm_contribution_recur records get written with status = 2 instead of 1, i.e. pending.

That makes sense in some models, but not for my new extension that I'm writing, where I can confirm that the first payment goes through right away.

Is there are param that I should be returning to do this?

[xavier]

we had the same problem and hacked our way through with a hook.

In general, the payment processor framework makes a lot of assumptions that might not apply all the time indeed. Would you have the time to improve it?

ie, having methods in the payment processor that are called to set/alter things like the status or contribution type instead of being hard coded?

[adixon]

xavier: great, thanks. Can you elaborate on which hooks worked and/or point me to your code?

in terms of the bigger picture, I've never been convinced by the current framework, so adding more methods isn't the direction I'd like to see.

[Eileen]

That doesn't feel right to me - I think core should be patched so that the payment processor can return the status of the initial contribution

[xavier]

xavier: great, thanks. Can you elaborate on which hooks worked and/or point me to your code?

in terms of the bigger picture, I've never been convinced by the current framework, so adding more methods isn't the direction I'd like to see.

that's the sepa one, I used the pre and post hooks on contribution and contributionRecur to alter the needed. It would be easier to test and more robust to let the payment processor do it directly.

What would be wrong with that approach of having methods in the PP called by the core to alter that?

given the number of exiting PP, I don't see any other direction than adding features on the existing framework and refactor, but we need to keep "binary/api" compatibility so an existing PP would still work.

X+

[adixon]

Okay, so here's the bigger picture: the fact that there's no way to set the status to 1 for recurring payments without this back door is really a symptom of how the payment processor subsystem was created. i.e. a few payment processors were created for the most popular ones, and there was an attempt to generalize some code into the base payment processor object and implement the details of each payment processor by subclassing it. Presumably in the case of recurring payments, all the initial examples used some kind of call back (IPN?) to verify the recurring payments, so that was not considered to be something that needed to be overrideable in the doDirectPayment method, etc.

When I look at the diversity of ways that payment gateways work these days, I think that this initial attempt (it was 7 years ago and hasn't really been updated since) needs a rethink. In particular, it seems a bit too monolithic, and too separate from the recording of payments. I'd like to see a more flexible framework that enforces the recording of the payments to be connected with the payment processor. Something that would allow an extension to more simply build new payment pages that can more easily reflect what the payment gateway can do, rather than trying to overgeneralize a basic payment page logic that tries to be everything. Right now, building a new payment page is almost as scary as tinkering with the default one.

[Eileen]

Allan,

I do agree - although the path forwards is not terribly clear. We have made slow steps towards getting some of the logic out of the form layer - but I have seen quite a few commits go through that put logic into it so am optimistic but not certain we are going forwards on this.

I looked recently at adding functionality to both contribution & event form & came to the conclusion that we probably need to give them a shared parent form class (because no-one wants every function shared by front end forms to be in CRM_Core_Form) & extract out a whole lot of shared functionality into that class.

The payment processors are another erg - I agree that they are set up to reflect the paypal model rather than a generic model. But I don't see how to address it (athough I do see extracting out shared functionality as a pre-requisite).

FYI, in 4.4 I have refactored the paypal IPN so that at least the IPN requests can be re-run from code. Building on this, in our repo I have edited ipn.php so that all incoming requests are logged into a table & we have an api action to re-submit them (https://github.com/fuzionnz/civicrm/commit/b852843adeacedf7b488adb4056b9f4eafbc569c).

[xavier]

Okay, so here's the bigger picture: the fact that there's no way to set the status to 1 for recurring payments without this back door is really a symptom of how the payment processor subsystem was created. i.e. a few payment processors were created for the most popular ones, and there was an attempt to generalize some code into the base payment processor object and implement the details of each payment processor by subclassing it.

Agree and agreed it isn't finished and doesn't work well for recurring payments. (for the record, I think "generic" is mostly well done by refactorisation once you got enough use cases, not by overthinking beforehand based on assumptions over one use case: it tends to create a rube goldberg or abstract pattern decorator factory pattern observer pattern)

Anyway, what's the way forward? wouldn't a setStatus () {return 1;} in the parent class PP that can be overrided by each PP if they want something different be a solution?

X+

[Eileen]

I would probably pass the contribution params by reference & allow status (& anything else) to be altered

[adixon]

Getting back to my original quest from which I'd hijacked my own thread ...

The way to fix the contribution_status_id relatively easily in the existing world is to just use the hook_civicrm_pre, which is designed to let you tamper with records just before they get written.

So you just have to implement hook_civicrm_pre for op = 'create', and objectName = 'Contribution' or 'ContributionRecur'.

The only slightly tricky bit is that the params variable for the initial contribution doesn't actually contain the payment processor id, so you have to dig back by looking it up using the payment page id, which you do have.  And if you do try this at home also note that a call to this api:

Code: [Select]

$result = civicrm_api('ContributionPage', 'getsingle', $params);
will return the payment processor id with key 'payment_processor', and not 'payment_processor_id' as you would expect.

In case that code gets messy and you just want to understand how to do this for your own code, this commit
https://github.com/adixon/ca.civicrm.iats/commit/bcc66d0af74d9c23b35db99ddb88a7856b1e2d27
should be the useful bit you need (it has a bit of extra noise in there, but look at everything after the iats_civicrm_pre function.)

And now, re-hijacking my thread: I think a real simple solution to avoid even this amount of extra code would be to just allow doDirectPayment to set the status value. I remembered that was the first thing I tried, and if the code in the processCreditCard just checked before setting the status, I think that would be the easiest fix. But I hesitate to provide a patch because figuring out the code flow is a bit of nightmare and I suspect it's more complicated than I know ...