Improve create payment processor documentation

[artfulrobot]

Before

Really hard to write a payment processor; results in wild variations between processors; results in difficult to maintain code and inconsistencies.

After

Include documentation on the responsibilities and roles of the various payment processor classes; what data and logic they should handle and what they should not.

PR WIP

What do you think about this? @eileenmcnaughton @mattwire

I'm desperately trying to do things right while keeping things working on the GoCardless extension, but every example I find to reference seems riddled with inconsistencies, which I suspect is to do with things being written at different points in time; guesswork; bodges/hacks for particular awkward third parties; changes in core (like Order API)... The existing documentation has loads of problems IMO, so rather than just sigh I thought I'd have a stab at setting stuff straight.

But this may not be the best way to do it; I don't know, so don't hold back if you think there's a better way to improve things.

[mattwire]

@artfulrobot You should find quite a bit of useful commenting in the latest stripe / mjwshared extensions. In general I've tried to "do things the right way" in the stripe extension and handle all the required hacks / bodges in the mjwshared extension. I've tried to comment with reasons why in the code

[artfulrobot]

@eileenmcnaughton @mattwire (please cc others, I'm not sure who works on contribs)

Have a look at the Creating a Payment Processor page (just down to where it says "stop reading now"!)

I've:

• set out some advice to try to keep CiviCRM's logic and UX out of processors - wording here will need a tweak after more discussion
• outlined how extensions should use property bag.
• suggested an implementation of property bag for core, including methods to create the property bag, and take heavy lifting off of processors (example) and allow extensions to add custom properties - see latest commit at Add Payment PropertyBag for payment processor method arguments civicrm-core#15697

There's lots left to iron out but it feels like inching forward nevertheless.

I'm looking at doPayment, but I'd like to get on to the other methods too (which I think will mostly be simpler).

And I'd like to start thinknig about the outputs from these functions - they should be standardised too. They could possibly start returning PropertyBags themselves, or possibly not - main thing is that we want them to know exactly what's expected.

[artfulrobot]

(at risk of mucking up this thread...)

@artfulrobot You should find quite a bit of useful commenting in the latest stripe / mjwshared extensions. In general I've tried to "do things the right way" in the stripe extension and handle all the required hacks / bodges in the mjwshared extension. I've tried to comment with reasons why in the code

Thanks @mattwire indeed your code is beautifully commented 💯 and it's a good idea separating out into the traits 💡

I notice howevs, that the code does:

api3('Contribution', 'completetransaction', ['status' => 'Completed', 'id'=>123...])
// and
api3('Contribution', 'repeattransaction', ['status' => 'Completed', 'original_contribution_id'=>123...])

instead of using Payment.create in the first instance, and instead of using Order.create, or at least creating the contribution as 'pending' with repeattransaction and then using Payment.create.

And when do we start using template_contribution_id instead of original_contribution_id?

[JoeMurray]

@eileenmcnaughton once you're back in February I think this is a place where you should weigh in.

[artfulrobot]

This is something that I've suggested we have a voice discussion about.

[homotechsual]

I don't think proposals belong in docs content.

[homotechsual]

@artfulrobot what's the status/likely movement on this?

[artfulrobot]

@artfulrobot what's the status/likely movement on this?

At the time I believed I could get to a point of clarity, a point that made sense, and that documenting this would be the first step to this clarity crystalising for other payment processors.

I needed a chat with others to iron out some of the things that documenting this threw up, but that did not happen. As other contribution work has pushed forward, I had to deal with fixing my payment extensions; the net effect has been that it's pushed my knowledge backwards. Then coronavirus lock down hit, and now there's no time for anything!

I had hoped to join the frighteningly few people who really understand and get the payments system, and I had hoped to be able to maintain that understanding and contribute in an ongoing way, and by documenting I had hoped to further enlarge that circle. I've not been able to acheive that, which is an experience to learn from at some point.

So I've left a mess. I think the best thing to do is probably for me to rework this PR leaving notes about the Payment PropertyBag, but with a warning or something saying: this will be how it's done one day in the future, but for now you have to carry on (reimplementing version-specific should-be-core functionality in your extension). Then as and when I'm able to catch up on the payments stuff I can see if there's interest in moving forward with this with the hope of making implementing payment processors something it's possible to document clearly.

One learning is that documentation is a bit like tests. We are all (ish) sold on the importance of tests - it shows how each atomic function is designed to work; they can document edge cases; they can prove whether something works and when it gets broke. Documentation is also like this - if you can't explain how it's supposed to work, it's probably because something doesn't work.

[artfulrobot]

OK, I've:

• rebased
• hacked out all the proposal stuff (sorry @MikeyMJCO!)
• added warnings for where stuff might not be 100% accurate.
• tried to make it better than before while not being held up by perfection.

[homotechsual]

@artfulrobot thanks for this - I'm going to merge this as-is and we can improve incrementally from here :-)