WIP CRM-21682 automatic membership renewal cleanup #11556

mattwire · 2018-01-19T09:18:21Z

Overview

WORK IN PROGRESS Parts of this are gradually being extracted into smaller, easier to review PRs. Once this is fully reviewed and merged documentation will also need updating.
Ref: #12315

This is mostly code cleanup and an attempt to understand exactly what happens when a membership is automatically renewed!

Fixes (and this needs adding to Civi docs somewhere as it's currently undocumented and unclear what it does):
Before

Membership will be renewed when contribution is in ANY state except "Pending."
All parameters of recurring contribution are ignored, ie installments, frequency mismatch (eg. monthly recurring, annual membership) so every new contribution will trigger a membership renewal.
When repeattransaction is called ONLY the membership linked to the original contribution will be renewed (if the contribution is in the correct state (ie. Completed - see Before/After)). loadRelatedObjects only loads one membership but the code is expected all memberships to be loaded so does handle multiple memberships if passed in.

After

Membership will be renewed ONLY when contribution is in state "Completed".
Membership will be renewed ONLY if the recurring and membership type frequencies match. Installments parameter has no effect.
When repeattransaction is called: If the contribution has a recurring contribution ID then ALL memberships linked to that recurring contribution will be renewed. If the contribution has no recurring contribution then the membership linked to the original contribution will be renewed (if the contribution is in the correct state (ie. Completed - see Before/After) AND the other conditions are met (ie. state, frequency)).

Documentation (After)

Membership will be renewed ONLY when contribution is in state "Completed" - Only auto-renew membership when contribution status is completed #12315 (5.5.0)
Membership will be renewed ONLY if the recurring and membership type frequencies match (Installments are ignored).
When repeattransaction is called any linked memberships will be renewed if the contribution is in the correct state. If there is a recurring contribution then the frequencies must also match (installments are ignored).
A recurring contribution can be linked to multiple memberships and all memberships will be updated/renewed if all other conditions are met - CRM-21682 Support multiple memberships for recurring #12542 (pending merge)

CRM-21682: Automatic membership renewal fixes

eileenmcnaughton · 2018-01-22T04:36:12Z

The completetransaction process is heavily tested from api_v3_ContributionTest & we have been very stringent about ensuring that any updates are accompanied by tests in there

seamuslee001 · 2018-03-23T09:58:09Z

@mattwire Matt can you rebase this please

guanhuan · 2018-03-26T14:58:48Z

Thanks @mattwire, the business logic looks good.

@omarabuhussein can you help review this PR please?

eileenmcnaughton · 2018-03-26T20:31:12Z

CRM/Contribute/BAO/Contribution.php

+      'start_date', 'join_date', 'end_date', 'status_id');
+    $memberships = array();
+    if (!empty($contribution->contribution_recur_id)) {
+      // Load memberships associated with recurring contribution


is this part a change in behaviour?

@eileenmcnaughton I believe the expected behaviour was that you could have multiple memberships linked to a single recurring contribution and all of the code except this one point supports that. I believe it actually fixes a bug. It relates directly to the notes on the PR:

Before: When repeattransaction is called ONLY the membership linked to the original contribution will be renewed (if the contribution is in the correct state (ie. Completed - see Before/After)). loadRelatedObjects only loads one membership but the code is expected all memberships to be loaded so does handle multiple memberships if passed in.
After: A recurring contribution can be linked to multiple memberships and all memberships will be updated/renewed if all other conditions are met.

Part of the issue with this area of code is that there is no documentation about what it is actually meant to do! I am still getting reports of auto-renewals failing for some clients in obscure circumstances but I really need this patch reviewing/merging before I can get to the bottom of those issues (as the current core code is littered with inconsistencies including comparisons using getLabel instead of getKey/getName).

mattwire · 2018-05-18T21:59:13Z

@omarabuhussein Did you get a chance to look at this?

omarabuhussein · 2018-05-21T10:39:35Z

sorry @mattwire for being late, it is in my todo list but I was busy with many things during the last months and forget about it. Will make my best to review it this week.

omarabuhussein · 2018-05-30T22:01:23Z

sorry I get sick in the previous week and had to take 3 days off and now I am a bit late on many things, I know this get delayed too much from my side and I am really sorry for that but I am putting this now on my first things todo on next Monday morning.

omarabuhussein

@mattwire

I reviewed the PR and left some notes, hence that I reviewed the commits one by one and not by looking at the complete diff for all the commits, so it might be possible to see I commented on something but it was fixed in one of your next commits.

omarabuhussein · 2018-06-05T11:29:15Z

api/v3/Membership.php

    if (empty($params['id'])) {
+      // This is a new membership, calculate the membership dates.


instead of writing an inline comment, you can do :

$isNewMembership = empty($params['id']);

then :

if ($isNewMembership ) {

I think I would prefer to leave the comments here

omarabuhussein · 2018-06-05T11:30:40Z

api/v3/Membership.php

@@ -113,6 +116,7 @@ function civicrm_api3_membership_create($params) {
      );
    }
    else {
+      // This is an existing membership, calculate the membership dates after renewal


if you did what I said in my comment above, then I don't think there will be a need for this inline comment.

I think I would prefer to leave the comments here

omarabuhussein · 2018-06-05T11:32:41Z

api/v3/Membership.php

@@ -101,9 +101,12 @@ function civicrm_api3_membership_create($params) {
  _civicrm_api3_custom_format_params($params, $values, 'Membership');
  $params = array_merge($params, $values);

+  // Calculate membership dates


instead of this inline comment, you can maybe move the dates calculation logic to a new function and pass the parameters by reference

omarabuhussein · 2018-06-05T11:43:01Z

api/v3/Membership.php

-    $action = CRM_Core_Action::UPDATE;
+  else {
+    // edit mode
+    $params['action'] = CRM_Core_Action::UPDATE;


but now the $ids variable is no longer aviable in edit mode, though still you are passing it to :

$membershipBAO = CRM_Member_BAO_Membership::create($params, $ids, TRUE);

also for edit mode you are no longer passing :

$ids['membership']

which I am not sure if it is the right thing to do since there is still some code that use it in the create() method.

omarabuhussein · 2018-06-05T12:35:31Z

CRM/Contribute/BAO/Contribution.php

-        // this picks up membership type changes during renewals
-        // @todo this is almost certainly an obsolete sql call, the pre-change
-        // membership is accessible via $this->_relatedObjects
-        $sql = "


hmmm, but what if the membership type changed ? , your new code removed this query that handle that but did not offer a replacement

@omarabuhussein Can you give an example of when this could happen? I thought from the comments that we can just get the pre-change membership directly.

omarabuhussein · 2018-06-05T14:35:24Z

CRM/Contribute/BAO/Contribution.php

+    if (!empty($contribution->contribution_recur_id)) {
+      // Load memberships associated with recurring contribution
+      $membershipResult = civicrm_api3('Membership', 'get', array(
+        'contribution_recur_id' => $contribution->contribution_recur_id,


it is unlikely to have more than 25 memberships associated with the same contribution, but still would be better to add 'limit' => 0 option here. same for all the API calls below.

omarabuhussein · 2018-06-05T14:42:04Z

CRM/Contribute/BAO/Contribution.php

+        foreach ($membershipPaymentResult['values'] as $payment) {
+          $membershipIDs[] = $payment['membership_id'];
+        }
+        $membershipResult = civicrm_api3('Membership', 'get', array(


hmmm, why not just get the memberships using $primaryContributionID directly instead of fetching the payments IDs from the payments first ?

@omarabuhussein We are working with a single contribution at this point. Membership.get requires a recurring contribution ID which we don't have. Am I missing something?

omarabuhussein · 2018-06-05T14:43:25Z

CRM/Contribute/BAO/Contribution.php

-          'contact_id' => $membership->contact_id,
-          'is_test' => $membership->is_test,
-          'membership_type_id' => $membership->membership_type_id,
+          'id' => $membership['id'],


add space before

$membershipParams = array(..etc

line

omarabuhussein · 2018-06-05T14:56:37Z

@eileenmcnaughton I believe it is important to have a 2nd or maybe even a 3rd eye on this PR in case I missed anything.

@guanhuan Is it possible to have one of our QA team to go through this too?

eileenmcnaughton · 2018-06-05T19:42:44Z

@mattwire @omarabuhussein I feel like a bunch of tests here would help me feel better....

omarabuhussein · 2018-06-06T09:39:44Z

do you mean manual tests or unit tests here @eileenmcnaughton ?

eileenmcnaughton · 2018-06-06T09:53:19Z

@omarabuhussein unit tests.

TBH I'm generally uncomfortable with such a big refactor - I'd prefer to see a single function extraction as one PR & review with a new test & then each change on top would get more tests.

omarabuhussein · 2018-06-06T09:58:19Z

yeah such amount of changes is a bit scary for such legacy tightly coupled code.

eileenmcnaughton · 2018-06-06T10:02:36Z

Right - some of it is just extractions / moving stuff around - which is not too hard to review in isolation - but REALLY hard to review when other changes are combined

eileenmcnaughton · 2018-06-08T10:49:44Z

@mattwire can you rebase this & then remove any changes that don't relate to the one function & focus on how to break down the changes in there to a series of smaller refactors with tests.

I'm keen to work through this but I want to get the risk down!

…ionalPayment Cleanup recordAdditionalPayment towards #11556

…leanup Cleanup of api_v3_membership_create towards #11556

eileenmcnaughton · 2018-06-17T22:29:30Z

@mattwire just a thought on your next refactor round after current ones are merged - it might be worth extracting

    foreach ($memberships as $membershipTypeIdKey => $membership) {
      if ($membership) {
        self::updateCompletedMembership($contribution, $primaryContributionID, $changeDate, $contributionStatus, $membership->id, $membership->contact_id, $membership->is_test, $membership->membership_type_id);
      }
    }

By not passing the $membership you can separate the loading of the membership from the processing of it & focus on the 2 parts of that equation separately

omarabuhussein · 2018-06-18T08:50:45Z

other than @eileenmcnaughton notes above, is this ready to be reviewed again @mattwire ?

eileenmcnaughton · 2018-06-18T09:01:40Z

@omarabuhussein my sense is that @mattwire is going to continue rebasing this & picking things off into smaller commits until it's merged

davejenx · 2018-06-20T14:44:21Z

I agree with the proposed changes to the business logic. The existing behaviour is clearly broken in my view, particularly the (almost) ignoring of contribution status, which we have observed to result in memberships being renewed from failed Direct Debit transactions, using the Smart Debit extension.

eileenmcnaughton · 2018-07-17T07:37:47Z

@mattwire I think we just merged something so this is due a rebase

mattwire · 2018-07-23T20:05:54Z

Rebased and refactored commits. Extracted the next partial into #12542

eileenmcnaughton · 2018-11-15T10:07:53Z

@mattwire this is now the oldest PR & it's WIP - although there is a review-ready sub-PR. I'm going to close this & you can pull the next chunk out when that is reviewed

civicrm-builder added the master label Jan 19, 2018

mattwire changed the title ~~CRM-21682 automatic membership renewal cleanup~~ WIP CRM-21682 automatic membership renewal cleanup Jan 19, 2018

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch from 1bf6c57 to 669f24e Compare January 19, 2018 15:34

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch 3 times, most recently from de40302 to 2184f52 Compare February 2, 2018 09:48

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch from ab0ee09 to dd7bbae Compare February 17, 2018 22:17

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch from dd7bbae to 9bf9237 Compare March 16, 2018 17:15

mattwire mentioned this pull request Mar 16, 2018

CRM-21682 Membership Renewal Tests #11824

Merged

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch from 9bf9237 to 60f19a8 Compare March 16, 2018 17:32

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch 2 times, most recently from 6e44266 to 9ee3ec5 Compare March 23, 2018 10:13

mattwire changed the title ~~WIP CRM-21682 automatic membership renewal cleanup~~ CRM-21682 automatic membership renewal cleanup Mar 23, 2018

mattwire mentioned this pull request Mar 26, 2018

Improving"pay later" with installments prcoessing colemanw/webform_civicrm#111

Closed

eileenmcnaughton reviewed Mar 26, 2018

View reviewed changes

omarabuhussein mentioned this pull request Jun 4, 2018

CRM-21769: Show 'unsupported locale for parsing' warning only when enabling address parsing #11672

Merged

omarabuhussein suggested changes Jun 5, 2018

View reviewed changes

eileenmcnaughton mentioned this pull request Jun 6, 2018

Partial refactor of completeMembershipFromContribution #12271

Merged

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch from 9ee3ec5 to 94c0fb1 Compare June 15, 2018 07:07

mattwire added a commit to mattwire/civicrm-core that referenced this pull request Jun 15, 2018

Cleanup of api_v3_membership_create towards civicrm#11556

fd88af0

This was referenced Jun 15, 2018

Cleanup of api_v3_membership_create towards #11556 #12313

Merged

Cleanup recordAdditionalPayment towards #11556 #12314

Merged

Only auto-renew membership when contribution status is completed #12315

Merged

eileenmcnaughton added a commit that referenced this pull request Jun 15, 2018

Merge pull request #12314 from mattwire/CRM-21682_cleanup_recordAddit…

8bc8a91

…ionalPayment Cleanup recordAdditionalPayment towards #11556

eileenmcnaughton added a commit that referenced this pull request Jun 17, 2018

Merge pull request #12313 from mattwire/CRM-21682_api_v3_membership_c…

c9dbdca

…leanup Cleanup of api_v3_membership_create towards #11556

eileenmcnaughton mentioned this pull request Jun 17, 2018

[NFC] Stop passing no-longer-part-of-signature variable #12329

Merged

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch from 7df185c to fb6a9e1 Compare June 21, 2018 21:44

mattwire changed the title ~~CRM-21682 automatic membership renewal cleanup~~ WIP CRM-21682 automatic membership renewal cleanup Jul 15, 2018

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch 5 times, most recently from 00c6d4b to 0b7ddf7 Compare July 23, 2018 19:49

mattwire and others added 3 commits July 23, 2018 20:53

CRM-21682 Support multiple memberships for recurring

9392f61

CRM-21682 Don't allow auto-renew membership if frequencies do not match

fe39f1a

Drop unnecessary call to getContactMembership

cde3f38

mattwire force-pushed the CRM-21682_automatic_membership_renewal_cleanup branch from 0b7ddf7 to cde3f38 Compare July 23, 2018 19:58

mattwire mentioned this pull request Jul 23, 2018

CRM-21682 Support multiple memberships for recurring #12542

Closed

mattwire mentioned this pull request Jul 30, 2018

dev/core#288 : Use the correct membership date for the notification that appear after completing the membership payment in case pre hook is used #12583

Merged

eileenmcnaughton closed this Nov 15, 2018

		if (empty($params['id'])) {
		// This is a new membership, calculate the membership dates.

WIP CRM-21682 automatic membership renewal cleanup #11556

WIP CRM-21682 automatic membership renewal cleanup #11556

Conversation

mattwire commented Jan 19, 2018 • edited Loading

Overview

Fixes (and this needs adding to Civi docs somewhere as it's currently undocumented and unclear what it does): Before

After

Documentation (After)

eileenmcnaughton commented Jan 22, 2018

seamuslee001 commented Mar 23, 2018

guanhuan commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented May 18, 2018

omarabuhussein commented May 21, 2018

omarabuhussein commented May 30, 2018

omarabuhussein left a 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

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

omarabuhussein commented Jun 5, 2018

eileenmcnaughton commented Jun 5, 2018

omarabuhussein commented Jun 6, 2018

eileenmcnaughton commented Jun 6, 2018

omarabuhussein commented Jun 6, 2018 • edited Loading

eileenmcnaughton commented Jun 6, 2018

eileenmcnaughton commented Jun 8, 2018

eileenmcnaughton commented Jun 17, 2018

omarabuhussein commented Jun 18, 2018 • edited Loading

eileenmcnaughton commented Jun 18, 2018

davejenx commented Jun 20, 2018

eileenmcnaughton commented Jul 17, 2018

mattwire commented Jul 23, 2018

eileenmcnaughton commented Nov 15, 2018

mattwire commented Jan 19, 2018 •

edited

Loading

Fixes (and this needs adding to Civi docs somewhere as it's currently undocumented and unclear what it does):
Before

omarabuhussein commented Jun 6, 2018 •

edited

Loading

omarabuhussein commented Jun 18, 2018 •

edited

Loading