new api function contribution.completetransaction

[Eileen]

It seems to me there is a need for an api to do 'what the IPNs do'. In this case I am going to be advising CiviCRM of the outcome of the transaction from another application which will have the apikey credential.

I basically want to be able to call

civicrm_api('contribution', 'completetransaction', array('version' => 3, 'id' => 25, 'status_id' => 1));

And have it do the following
1) update the status of the transaction (to either completed or failed)
2) update the status of related entities
3) send out an email as required based on the contribution page / event page settings
4) update recurring contribution id profile - probably some extra data/ thought is needed for this

Traditionally the IPNs have

a) been untestable due to the intermingling of POST/ GET data throughout the functions

b) had validation that effectively plays the role of authentication. ie. because the IPN url is publicly accessable the authentication is that the combination of invoiceid, contribution id , contact id , mode & potentially recurring contribution id is 'known' substitutes for authentication.

c) often hard to code because the code in the various IPN classes is a mixture of generic & specific.

I would think that if authentication has already been done the only required field should be ID - although others could potentially be optionally passed in.

It might be that we also want a 'validate' function e.g

civicrm_api('contribution', 'validate', array('version' => 3, 'contribution_id' => 44, 'invoice_id' => 55);

this could be called by the ipn base function & ensure that such params as are passed in have internal consistency. Potentially it should either take a param 'params_to_validate' or just let the function calling it specify what fields to validate.

[xavier]

It would be really useful indeed. Testing IPN is such a pain!

validation api: is there a validateIPN method in the parent Payment processor class?

[Eileen]

yes, there is a validate fn on the BaseIPN class. It suffers from the same problems as the completion - hard to test etc

[totten]

I agree that this is a good idea -- it's easier to experiment/re-use/script/filter/re-architect the logic if it's part of an "API" or "BAO" than if it's part of an abstract-base-class.

Some questions/thoughts that may or may not go without saying:

 * What permissions would be associated with the API calls? Perhaps "edit contributions"?
 * We shouldn't duplicate the logic from IPN -- we should move/refactor it. (And, of course, leave behind functions for backward-compatibility.)
 * I'm not sure how this addresses the problem of automated testing for concrete IPN implementations, but maybe I'm too inexperienced with Civi's payment-processing code. By their nature, each IPN uses a new wire-protocol to accept a handful of common business operations (completeTransaction, failTransaction, etc). Much of the implementation-work (and development-risk) is about the new wire-protocol. (The common business-operations seem like a smaller part because they're already implemented/tested with CRM_Core_Payment_BaseIPN and CRM_Core_Payment_BaseIPNTest).  The only way to test that we've correctly implemented a wire-protocol is run through a series of fake requests. (This could mean WebTests or tests like http://symfony.com/doc/current/book/testing.html#functional-tests ) But again -- my hands-on experience with Civi payproc is quite limited. Maybe there's an example of how API-ification would help with automated testing?

[Eileen]

1) Yes, edit contributions makes sense, I had been going to just do administer CiviCRM :-)

2) My thinking is that the api would initially wrap code in the BaseIPN class and that code would be tested & hopefully refactored over time.

3) The IPN classes have 2 functions 1) get data out of the IPN response & 2) perform an action on civicrm with it.  #2 shouldn't vary that much between classes & it is #2 that I am primarily concerned with (that is where I have generally hit breakage as #1 only changes very infrequently)

Unfortunately these 2 functions are intermingled in the existing classes that I am familiar with. If you wish to, say, take the set of params that was passed into paypal & run that in code (to troubleshoot or to put in a missed one) you have to do a lot of work to mock a HTTP request.  I did start restructuring some of the IPN. I have refactored copies of the Paypal & Authorize.net IPNs somewhere (e.g. https://github.com/eileenmcnaughton/civicrm-core/commit/0427884c959cce54be93d4b78afe592eb23474cc) because I've needed to troubleshoot - but getting these finished & back into core is sitting behind a bunch of other things on my priority list.

Anyway, your comments about the BaseIPN class being tested threw me - until I looked at the actual tests & realised they were tests I wrote at the SF sprint last year when I refactored some chunks out of the IPN class into a testable state. However, they only cover the validate function, & 2 chunks of the 'completetransaction' function - exclusive of the chunk that ... er.. completes the transaction.

So, what does an api transaction add testability-wise? Well, missing tests! And basically there is a limited set of data required to complete a transaction which doesn't vary much between processors so I feel like once those are all extracted & compiled then finalising them should be able to work off a passed in array to a simple function (with spec function documenting the params).

[SarahG (FountainTribe)]

I have been re-factoring the Authorize.net payment processing code for recurring contributions. ( Authorize.net uses the phrase "Silent Post URL" instead of IPN, but its the same thing).  I have had 2 goals:
1) Easier testing  2) Audit trail for trouble-shooting a production environment.

The approach I have taken is as follows:
1) In the IPN class, I only log the raw message from Authorize.net into a new database table. ( I call this my message table)  I do NOT do anything with the raw message other than this.    The new table has one field for each possible field from Authorize.net. Each field name matches the field name from Authorize.net  ( Some fields are not needed by CiviCRM, and may never be used. But this database table is also my audit trail. )

2) I have a CRON job that queries the table from part one ( part of the SQL is a join on transaction ID against the contribution table, and the where clause only includes message records that are autorecurring and do not yet have a matching contribution.   This CRON job then loops through each record returned and tries to create/update a contribtion record.   

3) The process for trying to process a record from step 2 is: a) Check if there is a "pending/pay later" contribution record associated with this subscription. If there is, then my code updates that record to the status of "completed". (It also updates the transaction ID and the date)   If there is no pending contribution, then I create a brand new contribution record with the latest transaction ID and transaction date. Other details (such as campaign ID, etc) are copied from the first completed contribution associated with the subscription.

This is working well for version 4.2, but I have run into what seems to be a serious bug in the Contribution API preventing this from working in 4.3.4. ( http://issues.civicrm.org/jira/browse/CRM-12983)

This approach has many side benefits, such as having access to the history of messages and I can move this table to other servers for testing purposes. ( For automated testing, a script could populate the message table with a good range of data to test. )

I have also discovered interesting messages from Auth.net, that are useful in production environments. Such as Auth.net sends messages when a transaction is Voided, rejected for fraud, etc.  A client recently insisted that CiviCRM had "lost" 250 contribtions from Authorize.net (The transactions were listed on their paper statement, but were not in CiviCRM as contributions) What had happened is they had used a third-party POS machine directly connected to their Authorize.net account. I found the messages in my message table for those transactions, and it was obvious from the message that those transactions did not originate from CiviCRM.  Since I have a custom search for the message table, I was able to show this to the client.   (With some custom scripting and good deduping, I could potentially process those messages and turn them into contributions if needed)

I was thinking my approach could be reused for other payment processors. For each new payment processor, there would need to be a new "message table" defined. And the IPN code would just need to make sure each IPN message is logged in the message table as it arrives.  Then create a basic CRON/job that reads the message table and calls the new method that I named UpdateRecurringContributionSubscription($crm_recur_id , $trxn_id, $receive_date  ); for each new record.   

[Eileen]

That sounds like quite a big change - but I do agree that troubleshooting missing transactions is currently very difficult & having basic details about all ipns in a db table could be quite useful

[SarahG (FountainTribe)]

I do not think its as big a change as you think.  I think the API method you suggested  "completetransaction"  is the same thing as the method I wrote called " UpdateRecurringContributionSubscription($crm_recur_id , $trxn_id, $receive_date  )" 

Having that method is the key to having something that is easy to test.  (My use of the message table is really to have audit-ability, this part may not be of interest to anyone else)

[Eileen]

I personally WOULD like to see the implementation of the message table as an extension or in core. I think it would be really useful for troubleshooting & auditability

[Eileen]

NB - I have created a 'completetransaction' api - that was fairly simple. The larger part of the job is the tests.

[Eileen]

Based on Xavier's feedback I'm thinking we need to add an extra permission in. Currently 'edit contribution' allows you do any contribution action other than 'get' or 'delete' (ie. create, sendconfirmation, transact are open to anyone with 'edit contribution' or - & this doesn't sound good - anyone with 'access CiviCRM')

So,  given that this is an api intended for external applications 'reporting back' to civi that seems too broad. I guess it could be altered by an extension implementing it though?

[JoeMurray]

Just a high level permissions semantics question:

In the UI, to edit a contribution that paid for a membership or event registration, wouldn't a user need both Edit contribution and Edit membership (or Edit participant as appropriate)? As the edit may cause the membership to enter a different status, this would be my expectation. But I haven't looked closely at this ever.

However these use cases are handled through the browser should also be the way that permissions should be required at the API level.

FWIW, I think having a raw http data table for these IPN callbacks is reasonable. As it is a debug / testing functionality, I would prefer if there was a switch to turn it on or off, defaulting to off. In fact, putting this code into an extension would my preference.

[xavier]

Here the use case is basically an IPN that confirms the payment, so I'd rather grant it less than more permissions

We did try to use the UI permissions and match them at the UI level, but there is a bit of "impedance mismatch" as some permissions are high level conceptual (and they should) and the API is low level CRUD on simple entities. For instance, I don't expect the IPN being able to create a contribution, but only to modify the status of an existing one but there isn't that distinction in the UI, so no permission that are "low level" enough

agree, better logging would be useful, not sure there is a hook that could be used there to write it as an extension.

[andyw]

I don't agree that this is necessarily just debug / testing functionality.

As someone who spends a lot of my day debugging these things, I would say all sites could do with that data being logged really, otherwise you have very little hope of getting to the bottom of problems when they arise. And they will, believe me.

We usually just end up putting our own logging in, then telling the client to "wait for it to happen again", or trying (and failing miserably) to reproduce the problem ourselves.

.. which is by no means ideal

[SarahG (FountainTribe)]

I have posted my code for Authorize.net within the blog post at: http://civicrm.org/blogs/sarahgladstone/rethinking-automated-recurring-contributions-code-attached