From 11ec1f969018d29ee2cdd3a89200504cb0e9a3f3 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 10 Feb 2020 19:12:36 +1300 Subject: [PATCH] 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..... --- CRM/Contribute/Form/CancelSubscription.php | 12 +++++----- CRM/Core/Payment.php | 28 ++++++++++++++++++++++ Civi/Payment/PropertyBag.php | 1 + 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/CRM/Contribute/Form/CancelSubscription.php b/CRM/Contribute/Form/CancelSubscription.php index 4cbac2cfb6ca..b9af4f6b9d33 100644 --- a/CRM/Contribute/Form/CancelSubscription.php +++ b/CRM/Contribute/Form/CancelSubscription.php @@ -8,6 +8,7 @@ | and copyright information, see https://civicrm.org/licensing | +--------------------------------------------------------------------+ */ +use Civi\Payment\PropertyBag; /** * @@ -187,7 +188,7 @@ public function setDefaultValues() { * Process the form submission. */ public function postProcess() { - $status = $message = NULL; + $message = NULL; $cancelSubscription = TRUE; $params = $this->controller->exportValues($this->_name); @@ -207,17 +208,16 @@ public function postProcess() { // civicrm_contribution_recur.processor_id $cancelParams = ['subscriptionId' => $this->_subscriptionDetails->subscription_id]; try { - $cancelSubscription = $this->_paymentProcessorObj->cancelSubscription($message, $cancelParams); + $propertyBag = new PropertyBag(); + $propertyBag->setRecurProcessorID($this->_subscriptionDetails->subscription_id); + $message = $this->_paymentProcessorObj->doCancelRecurring($propertyBag)['message']; } catch (\Civi\Payment\Exception\PaymentProcessorException $e) { CRM_Core_Error::statusBounce($e->getMessage()); } } - if (is_a($cancelSubscription, 'CRM_Core_Error')) { - CRM_Core_Error::displaySessionError($cancelSubscription); - } - elseif ($cancelSubscription) { + if ($cancelSubscription) { try { civicrm_api3('ContributionRecur', 'cancel', [ 'id' => $this->_subscriptionDetails->recur_id, diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php index 1af72a2301f0..0d5248a62ea4 100644 --- a/CRM/Core/Payment.php +++ b/CRM/Core/Payment.php @@ -1313,6 +1313,34 @@ public function doPayment(&$params, $component = 'contribute') { return $result; } + /** + * Cancel a recurring subscription. + * + * Payment processor classes should override this rather than implementing cancelSubscription. + * + * A PaymentProcessorException should be thrown if the update of the contribution_recur + * record should not proceed (in many cases this function does nothing + * as the payment processor does not need to take any action & this should silently + * proceed. Note the form layer will only call this after calling + * $processor->supports('cancelRecurring'); + * + * @param \Civi\Payment\PropertyBag $propertyBag + * + * @return array + * + * @throws \Civi\Payment\Exception\PaymentProcessorException + */ + public function doCancelRecurring(PropertyBag $propertyBag) { + if (method_exists($this, 'cancelSubscription')) { + $message = NULL; + if ($this->cancelSubscription($message, $propertyBag)) { + return ['message' => $message]; + } + throw new PaymentProcessorException($message); + } + return ['message' => ts('Recurring contribution cancelled')]; + } + /** * Refunds payment * diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php index afdee0abb690..a8a92864a192 100644 --- a/Civi/Payment/PropertyBag.php +++ b/Civi/Payment/PropertyBag.php @@ -65,6 +65,7 @@ class PropertyBag implements \ArrayAccess { 'frequency_interval' => 'recurFrequencyInterval', 'recurFrequencyUnit' => TRUE, 'frequency_unit' => 'recurFrequencyUnit', + 'subscriptionId' => 'recurProcessorID', 'recurProcessorID' => TRUE, 'transactionID' => TRUE, 'transaction_id' => 'transactionID',