Clean up UpdateBilling/UpdateSubscription/ContributionRecur to use getters #21538

mattwire · 2021-09-19T10:19:20Z

Overview

Clean up UpdateSubscription/UpdateBilling forms to use existing "getters".

Before

Code accessing properties directly.
UpdateBilling missing recur ID.
UpdateBilling/UpdateSubscription has subscriptionId

After

Code accessing via "get" functions.
UpdateBilling has recur ID.
UpdateBilling/UpdateSubscription has recurProcessorID which is a copy of subscriptionId.

Technical Details

Comments

civibot · 2021-09-19T10:19:22Z

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

mattwire · 2021-09-19T10:20:20Z

CRM/Contribute/Form/UpdateBilling.php

@@ -203,27 +203,28 @@ public function postProcess() {
    $processorParams['month'] = CRM_Core_Payment_Form::getCreditCardExpirationMonth($processorParams);
    $processorParams['year'] = CRM_Core_Payment_Form::getCreditCardExpirationYear($processorParams);
    $processorParams['subscriptionId'] = $this->getSubscriptionDetails()->processor_id;
-    $processorParams['amount'] = $this->_subscriptionDetails->amount;
+    $processorParams['amount'] = $this->getSubscriptionDetails()->amount;
+    $processorParams['id'] = $this->getContributionRecurID();


UpdateSubscription didn't used to pass the ID of the contribution recur but was fixed some time ago. UpdateBilling still hasn't been fixed and it's crazy not to pass the ID when we have it!

@mattwire if we are adding this we should use the parameter name in the documentation - ie
$processorParams['contributionRecurID']

I'm OK with you passing additional names as well - but we should make sure we pass out the parameters we have contracted to in all cases

https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#recurring-contribution-parameters

@eileenmcnaughton Good idea. I've updated both UpdateBilling and UpdateSubscription to pass both id and contributionRecurID so we are consistent.

@mattwire I see this class as more correct because it builds up a separate 'processorParams' - the other class might be storing id in params for 'other reasons'?

mattwire · 2021-09-19T10:24:48Z

CRM/Contribute/Form/UpdateSubscription.php

@@ -209,7 +209,8 @@ public function postProcess() {
    $params['id'] = $this->getContributionRecurID();
    $message = '';

-    $params['subscriptionId'] = $this->getSubscriptionDetails()->processor_id;
+    $params['recurProcessorID'] = $params['subscriptionId'] = $this->getSubscriptionDetails()->processor_id;


This adds recurProcessorID which matches what is defined as the "correct" key in PropertyBag. It's more accurate than "subscriptionId" which doesn't exist in the database by that name and good to define it now so that payment processors can choose to use that key.

mattwire · 2021-09-19T10:24:58Z

CRM/Contribute/Form/UpdateBilling.php

@@ -202,28 +202,29 @@ public function postProcess() {
    $processorParams['country'] = CRM_Core_PseudoConstant::country($params["billing_country_id-{$this->_bltID}"], FALSE);
    $processorParams['month'] = CRM_Core_Payment_Form::getCreditCardExpirationMonth($processorParams);
    $processorParams['year'] = CRM_Core_Payment_Form::getCreditCardExpirationYear($processorParams);
-    $processorParams['subscriptionId'] = $this->getSubscriptionDetails()->processor_id;
-    $processorParams['amount'] = $this->_subscriptionDetails->amount;
+    $processorParams['recurProcessorID'] = $processorParams['subscriptionId'] = $this->getSubscriptionDetails()->processor_id;


This adds recurProcessorID which matches what is defined as the "correct" key in PropertyBag. It's more accurate than "subscriptionId" which doesn't exist in the database by that name and good to define it now so that payment processors can choose to use that key.

eileenmcnaughton · 2021-09-22T23:55:18Z

CRM/Contribute/Form/UpdateBilling.php

@@ -47,14 +47,14 @@ public function preProcess() {
    if ($this->_mid) {
      $this->_paymentProcessor = CRM_Financial_BAO_PaymentProcessor::getProcessorForEntity($this->_mid, 'membership', 'info');
      $this->_paymentProcessor['object'] = CRM_Financial_BAO_PaymentProcessor::getProcessorForEntity($this->_mid, 'membership', 'obj');
-      $this->_subscriptionDetails = CRM_Contribute_BAO_ContributionRecur::getSubscriptionDetails($this->_mid, 'membership');
+      $this->setSubscriptionDetails();


I don't think this works - setSubscriptionDetails was already called when it called parent::preProcess - so either that worked & this is unnecessary or it didn't work & this won't help

That code kinda does my head in but I think the idea should be

-either we have the contribution_recur_id or we need to load it from the contribution or membership.

either we have the membership id or we need to load it from the contribution or recurring contribution

shit just got circular.

@eileenmcnaughton Well spotted! I just swapped the line for the "equivalent" but have now looked in more detail and you are right. It's already called in preProcess() and we already have retrieved all the IDs by that point. Also the getSubscriptionDetails() function is only using the different entity IDs to work out the recur ID - so the same data is returned no matter what related entity is passed in.

So I've now just removed this extra call to get/set the subscription details.

eileenmcnaughton · 2021-09-22T23:59:01Z

@mattwire the bulk of this is fine. The 2 lines I commented on are the only 2 I have concerns with. 1 has an easy fix - the other you might prefer to let out of this PR as it gets confusing

As an aside - I'd really prefer that we switched subscriptionDetails to an array - then it could be a v4 api result object. Out of scope

mattwire · 2021-09-23T09:28:30Z

@eileenmcnaughton Thanks for reviewing. I've updated based on that.

As an aside - I'd really prefer that we switched subscriptionDetails to an array - then it could be a v4 api result object. Out of scope

Hopefully that gets easier once we've converted everything to work via the getSubscriptionDetails() getter instead of accessing class properties directly.

eileenmcnaughton · 2021-09-23T09:33:46Z

CRM/Contribute/Form/UpdateBilling.php

@@ -47,14 +47,13 @@ public function preProcess() {
    if ($this->_mid) {
      $this->_paymentProcessor = CRM_Financial_BAO_PaymentProcessor::getProcessorForEntity($this->_mid, 'membership', 'info');
      $this->_paymentProcessor['object'] = CRM_Financial_BAO_PaymentProcessor::getProcessorForEntity($this->_mid, 'membership', 'obj');
-      $this->_subscriptionDetails = CRM_Contribute_BAO_ContributionRecur::getSubscriptionDetails($this->_mid, 'membership');


@mattwire are you sure this isn't altering what is stored in $this->_subscriptionDetails - my brain failed before making sense of that....

Yes. parent::preProcess()gets/sets recur ID, contribution ID and membership ID if available. Then callsCRM_Contribute_BAO_ContributionRecur::getSubscriptionDetails()` which retrieves the recur information.

If we pass in only the membership ID there is no way it could change.

If we pass in the recur ID there is no way it could change.

If we pass in only contribution ID we don't hit the if ($this->_mid)` so it doesn't change.

If we pass in contribution ID and membership ID then we're using the civicrm_membership_payment table and there's a possibility of returing a different recur. But that's a completely unsupported configuration because we'd have to have two recurring contributions making payments on the same membership and that has never been supported in CiviCRM (we do partially support the opposite which is two memberships on the same recurring contribution but that scenario is not changed here).

OK - my brain won't process that but I feel confident you have worked through it

eileenmcnaughton · 2021-09-23T09:36:02Z

CRM/Contribute/Form/UpdateBilling.php

-        'groupName' => $this->_subscriptionDetails->membership_id ? 'msg_tpl_workflow_membership' : 'msg_tpl_workflow_contribution',
-        'valueName' => $this->_subscriptionDetails->membership_id ? 'membership_autorenew_billing' : 'contribution_recurring_billing',
-        'contactId' => $this->_subscriptionDetails->contact_id,
+        'groupName' => $this->getSubscriptionDetails()->membership_id ? 'msg_tpl_workflow_membership' : 'msg_tpl_workflow_contribution',


FYI - I did a bunch of work on making groupName obsoleter at the start of the year & I'm hoping to actively drop it in the near future (I'll put it in a dev digest so just mentioning it)

I'm aware and looked briefly but then decided it was out of scope for this PR because it would require testing the email/messagetemplate flow as well and that seemed to get complicated

eileenmcnaughton · 2021-09-23T09:38:27Z

CRM/Contribute/Form/UpdateBilling.php

-    $processorParams['amount'] = $this->_subscriptionDetails->amount;
+    $processorParams['recurProcessorID'] = $processorParams['subscriptionId'] = $this->getSubscriptionDetails()->processor_id;
+    $processorParams['amount'] = $this->getSubscriptionDetails()->amount;
+    $processorParams['contributionRecurID'] = $processorParams['id'] = $this->getContributionRecurID();


Do we need both? If not future us might be trying to chase our tail making sense of whether id is required so I'd only add id if there is a reason since it's a duplicate

No. I just added it for consistency. I've written helpers to convert params to propertyBag in a version-safe/compatible way in the mjwshared library - passing contributionRecurID actually simplifies that helper because we just check if it has that property and if not fallback to retrieving from subscriptionId. Updated

…tters

mattwire · 2021-09-23T11:00:59Z

CRM/Contribute/Form/UpdateBilling.php

@@ -198,32 +197,33 @@ public function postProcess() {
      list($key) = explode('-', $key);
      $processorParams[$key] = $val;
    }
-    $processorParams['state_province'] = CRM_Core_PseudoConstant::stateProvince($params["billing_state_province_id-{$this->_bltID}"], FALSE);
-    $processorParams['country'] = CRM_Core_PseudoConstant::country($params["billing_country_id-{$this->_bltID}"], FALSE);
+    $processorParams['billingStateProvince'] = $processorParams['state_province'] = CRM_Core_PseudoConstant::stateProvince($params["billing_state_province_id-{$this->_bltID}"], FALSE);


See #21583. billingStateProvince got missed originally from propertyBag but should be there.

eileenmcnaughton · 2021-09-23T11:09:27Z

OK - I think this is mergeable

civibot bot added the master label Sep 19, 2021

mattwire commented Sep 19, 2021

View reviewed changes

mattwire force-pushed the subscriptioncleanup branch from e1a002a to 17a278b Compare September 19, 2021 10:22

mattwire commented Sep 19, 2021

View reviewed changes

eileenmcnaughton reviewed Sep 22, 2021

View reviewed changes

mattwire force-pushed the subscriptioncleanup branch 2 times, most recently from c9eb68a to f37a123 Compare September 23, 2021 09:25

eileenmcnaughton reviewed Sep 23, 2021

View reviewed changes

Clean up UpdateBilling/UpdateSubscription/ContributionRecur to use ge…

cc929f4

…tters

mattwire force-pushed the subscriptioncleanup branch from f37a123 to cc929f4 Compare September 23, 2021 10:52

Add standardized params for billingStateProvince and billingCountry

7a75d70

mattwire commented Sep 23, 2021

View reviewed changes

eileenmcnaughton added the merge on pass label Sep 23, 2021

seamuslee001 merged commit cd57c44 into civicrm:master Sep 23, 2021

mattwire deleted the subscriptioncleanup branch September 23, 2021 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up UpdateBilling/UpdateSubscription/ContributionRecur to use getters #21538

Clean up UpdateBilling/UpdateSubscription/ContributionRecur to use getters #21538

mattwire commented Sep 19, 2021 •

edited

Loading

civibot bot commented Sep 19, 2021

mattwire Sep 19, 2021

eileenmcnaughton Sep 22, 2021

mattwire Sep 23, 2021

eileenmcnaughton Sep 23, 2021

mattwire Sep 19, 2021

mattwire Sep 19, 2021

eileenmcnaughton Sep 22, 2021

mattwire Sep 23, 2021

eileenmcnaughton commented Sep 22, 2021

mattwire commented Sep 23, 2021

eileenmcnaughton Sep 23, 2021

mattwire Sep 23, 2021

eileenmcnaughton Sep 23, 2021

eileenmcnaughton Sep 23, 2021

mattwire Sep 23, 2021

eileenmcnaughton Sep 23, 2021

mattwire Sep 23, 2021

mattwire Sep 23, 2021

eileenmcnaughton commented Sep 23, 2021

Clean up UpdateBilling/UpdateSubscription/ContributionRecur to use getters #21538

Clean up UpdateBilling/UpdateSubscription/ContributionRecur to use getters #21538

Conversation

mattwire commented Sep 19, 2021 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Sep 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 22, 2021

mattwire commented Sep 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 23, 2021

mattwire commented Sep 19, 2021 •

edited

Loading