Added Cancel Recur Subscription test & setter for supports on Dummy processor

[kartik1000]

Overview

Added Test in follow up to #18196. Have also added a function in CRM_Core_Payment_Dummy to set the return value for support functions.

This also allows the capabilities supported by the Dummy processor to be set from outside the class. This is done by adding a support array where the capabilities are initially set to TRUE but can be altered by using the setSupports Function. This can be highly useful to alter the capabilities of dummy processors from directly outside the class in future tests as well.

Comments

Functions used within the test have been extended from RecurForms.php added in #21588

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

[eileenmcnaughton]

@kartik1000 there is already a test class CRM_Contribute_Form_CancelSubscriptionTest which covers this form (& better matches the form class name) - can you add your test to that rather than add this class?

[mattwire]

@kartik1000 please update per suggestion

[eileenmcnaughton]

I think this approach is generally fine - we have been moving away from the underscore in class property names though - see the property above.

[mattwire]

@kartik1000 Sorry I gave you this example :-) Please change to protected $supports =

[eileenmcnaughton]

I think this makes sense. Minor comments inline

[mattwire]

@kartik1000 The changes for $supports should be made on CRM_Core_Payment_Dummy and not CRM_Core_Payment_Manual because we use Dummy for testing. Manual is used for pay later and also extended by some extensions.

[kartik1000]

@kartik1000 The changes for $supports should be made on CRM_Core_Payment_Dummy and not CRM_Core_Payment_Manual because we use Dummy for testing. Manual is used for pay later and also extended by some extensions.
@mattwire the processor created in addContribution() function is an instance of ‘CRM_Core_Payment_Manual’. I tried making changes in ‘CRM_Core_Payment_Dummy’ but it was not working as we are not creating an instance of it. Since, I am building on Eileen’s test, therefore I decided to continue with the same functions and thus make required changes in ‘CRM_Core_Payment_Manual’.

[eileenmcnaughton]

@kartik1000 I updated the PR template slightly to reflect your setter for supports but I think it maybe needs more details about how it would be used - in the PR template & in code comments. There is no obvious core reason to do this. I'm Ok wtih this as a minor change in core to support an outside-core usage but a future 'cleaner-upper' might need to know the rationale to prevent them from saying 'that's silly why don't we just do x'

[kartik1000]

@kartik1000 I updated the PR template slightly to reflect your setter for supports but I think it maybe needs more details about how it would be used - in the PR template & in code comments. There is no obvious core reason to do this. I'm Ok wtih this as a minor change in core to support an outside-core usage but a future 'cleaner-upper' might need to know the rationale to prevent them from saying 'that's silly why don't we just do x'

@eileenmcnaughton I agree, I will add a description both in template PR and the in code comments (above the support variable preferably).

[kartik1000]

@eileenmcnaughton I have updated the PR description and added the same in in-code comments as well.

[eileenmcnaughton]

@kartik1000 can you go a bit further & explain why someone would wan to do that? Your use case.

[demeritcowboy]

I admit I'm confused by this

allow the capabilities supported by the Dummy processor to be set from outside the class

Wouldn't you just update the processor to support other capabilities?

[mattwire]

Wouldn't you just update the processor to support other capabilities?

@demeritcowboy Normally yes. But dummy is being used here as a special case for testing. By "dynamically" updating the capabilities we can test multiple scenarios that would otherwise be untestable without finding/creating payment processors with specific combinations of "supportsX". And you get scenarios like eg. Stripe doesn't support updateSubscription but will do so if you chose that to test that "a processor does not support updateSubscription" the test would break once you introduce the new feature.

[demeritcowboy]

Ok thanks I see. So then the remaining issues are:

• Should it be Dummy or Manual?
• There were test fails but they've disappeared now. Whatever that was about.

I think using "settings" over the Power of OO can be justified to avoid hundreds of subclasses. Just at the point where some function starts getting nested if supports this then (if not supports that but supports other thing then ...) else if (it's Tuesday...) a subclass should be created.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

I'm actually OK with this being on Dummy or Manual or the parent class (would require a bit more care to ensure no defaults change) - but I don't know how interested @kartik1000 is in doing more on it

[demeritcowboy]

Test fail is:

Manual.php:22 | VariableComment | WrongStyle | Error
You must use "/**" style comments for a member variable comment

[demeritcowboy]

If you're updating this anyway can we add some more explanation, and it's not the Dummy processor. Maybe something like

This support variable is used to allow the capabilities supported by the Manual processor to be set from unit tests so that we don't need to create a lot of new processors to test combinations of features.

[mattwire]

Now cleaned up and tests should pass. Changes should be on Dummy processor because that's what is usually used in tests and it already has other "infrastructure" for testing (eg. setting doPayment result).
#22784 is the reason that @kartik1000 put it on Manual processor because there was a bug in the tests for setting payment processor.