Fix issues with retrieving supportsTestMode/supportsLiveMode for payment processors

[mattwire]

Overview

supportsLiveMode should return live processors.
supportsTestMode should return test processors.
We should not be loading processors multiple times in getPaymentProcessors() - improves loading speed a little bit.
Running a backend payment form in test mode (ie URL param mode=test) selected the live processor instead of the test one.

Before

Open backend payment form in test mode (change mode=live to test) (eg. called via:
/civicrm/contact/view/contribution?reset=1&action=add&cid=202&context=contribution&mode=test).
Make sure you have multiple processors available.
Select a different processor which will load it's billingblock via AJAX. The live processor loads instead of the test one.

After

Resolve issues with loading processors in test/live mode. Improve efficiency.

Technical Details

Comments

[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).

[seamuslee001]

Jenkins re test this please

[seamuslee001]

Jenkins re test this please

[seamuslee001]

test fails relate @mattwire

[mattwire]

@seamuslee001 Hey, any ideas why this doesn't fail locally?

[mattwire]

test this please

[eileenmcnaughton]

@mattwire I decided the best way to test this was in a test. However the results from the test indicate that this is broken before and after so I haven't managed to reach comfort with this change

#15499

[mattwire]

@eileenmcnaughton So the issue with the test failing from #15499 testGetProcessors() is because the manual processor does not support test mode. But that actually seems correct to me as every other processor in CiviCRM has a separate processor instance for live and for test mode. We could "fix" it by adding supportsTestMode() = TRUE if we do want to support it but it will still return is_test=0 which is different from every other processor. I'm inclined to just "accept" that it does not support test mode and if it becomes a requirement add a test manual processor instance just like every other processor. Thoughts?

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@mattwire good sleuthing.

I feel like the Manual Processor probably should be returned when we retrieve a list of processors that work in test mode - I'm not sure where you mean it will return is_test=0 - can we make that mode conditional?

I'm also a bit nervous about the change to supportsLiveMode & supportsTestMode - we need to be sure there are no places in core where it loads the live processor & calls supportsTestMode & expects 'TRUE' - I guess we need to go through & test all the forms & just check it all out through a lot of slog

[mattwire]

@eileenmcnaughton There seems to have been a time when a lot of integrations just used the live processor ID and fudged params to use it in test mode. (webform_civicrm is one of these!). It's caused a lot of problems and lot's of custom code in payment extensions to handle this kind of thing which I've been steadily stripping out.
One of the parameters (and DB fields) in civicrm_payment_processor is is_test which is either 0 or 1. It cannot be both! And whenever we use one of the fudges to make it both we kill fairies.

There is an open PR on webform_civicrm to fix this: colemanw/webform_civicrm#266 The "worst case" is that a payment processor stops working in test mode for a particular module or form if we merge this. But without this there's various places where test mode doesn't work (eg. backend contribution forms).

If we return the "live" manual processor then we are introducing an exception to how everything (including the dummy processor) works and we're going to need exceptions coded in everywhere we use it to say "this is a live mode processor but we're using it as if it was in test mode".

[mattwire]

I guess we need to go through & test all the forms & just check it all out through a lot of slog

Most of the forms end up calling getPaymentProcessors so never actually call supportsLive/TestMode directly.
I identified this issue in the first place by change mode=live to mode=test on the backend "submit credit card" forms. And got confused because stripe was still loading the live processor and thus failing.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@mattwire I spun off the bit that changes the IF clause since that if just moving code & it creates quite a bit of noise on this PR. I just tried & failed to replicate the wrong processor id loading in back end forms. Can you give steps to replicate?

[mattwire]

@eileenmcnaughton

1. Open backend payment form in test mode (change mode=live to test) (eg. called via:
   /civicrm/contact/view/contribution?reset=1&action=add&cid=202&context=contribution&mode=test).
2. Make sure you have multiple processors available.
3. Select a different processor which will load it's billingblock via AJAX. The live processor loads instead of the test one.

[eileenmcnaughton]

OK - I'm done now for tonight - but I may get back to this on Friday (my priority after dealing with any regressions is the items marked 'high' on here https://lab.civicrm.org/dev/financial/issues/76 ) now there are instructions to replicate. I've updated the PR template with them. I think it would be worth clearing out the 'noisy' changes in #15568

[mattwire]

@eileenmcnaughton This is stuck on waiting a response from you re the manual payment processor. My view is that we should be treating it the same as any other payment processor - ie. separate processors for live and test, otherwise we'll end up with code special casing things just to support an inconsistency with that processor. In the meantime this is a bit of a problem when debugging processor issues on live sites as I can't use test mode on backend forms.

[eileenmcnaughton]

@mattwire At the moment the manual process has the number zero & no row in the payment processor table. We need to change that & give it rows in the table before we can treat it the same as the others

[mattwire]

@eileenmcnaughton Tests are now passing - I changed the manual processor to specifically support both test/live like before. Ok to merge?

[eileenmcnaughton]

I tested this & the correct processors are loading & prior issue is resolved.