Update cancelSubscription form to
a) use properyBag and
b) call doCancelRecur - this separates adds a new function more in line with our other 'do' functions.
This came up after https://github.com/eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor/issues/149 was
logged & I thought it was time to pick this up again.
There is further cleanup to be done & I have deliberately left that out in order to keep this tightly related to the
decisions that need to be made.
1) This creates a new function more in line with our standards it starts with 'do', accepts a propertyBag,
expects a PaymentProcessorException to be thrown if there is a fail.
2) does not require PaymentProcessors to implement cancelSubscription. A common scenario is that a processor
supports recurrings being cancelled but does not need to do anything as updating the contribution
recur record is enough to achieve that. In this case processors should implement supportsCancelRecurring
but they do not need to implement this function. (Currently they are recommended to implement supportsCancelRecurring
in keeping with our standardised 'supports' pattern but the non-standard cancelSubscription is also required.
3) ensures that the only place in the core code that calls this passes the PropertyBag
However, this also introduces the parameter 'subscriptionId' to the PropertyBag. This is one we 'left for it's own discussion'
because - we all hate it.
The options as I see it are
1) subscriptionId - advantage is this is what is already in use & we will need some transitioning if we change it. Disadvantage is that it only really relates to paypal apart from that....
2) processor_id - advantage is this i s in the contribution_recur table, disadvantage is that it's a really silly & confusing name for it
3) recur_profile_id, recur_gateway_id, other we come up with - advantage is we could give it a meaningful name, disadvantage is yet
another terminology in the mix & we might be opening a whole new can of worms.....
At this stage I'm leaning to 1 on the basis that it's a baby step that complies with existing code & doesn't open up a whole
new scope leap. But - open.....