WIP CRM-21682 automatic membership renewal cleanup #11556
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3930,6 +3930,7 @@ public static function recordAdditionalPayment($contributionId, $trxnsData, $pay | |
| } | ||
| } | ||
|
|
||
| // update membership details | ||
| self::updateMembershipBasedOnCompletionOfContribution( | ||
| $contributionDAO, | ||
| $contributionId, | ||
|
|
@@ -5406,99 +5407,86 @@ protected static function isPaymentInstrumentChange(&$params, $pendingStatuses) | |
| * @param int $primaryContributionID | ||
| * @param string $changeDate | ||
| * | ||
| */ | ||
| protected static function updateMembershipBasedOnCompletionOfContribution($contribution, $primaryContributionID, $changeDate) { | ||
| // Load memberships from contribution info. There may be multiple memberships (or none) | ||
| $membershipFields = array('id', 'contact_id', 'is_test', 'membership_type_id', 'contribution_recur_id', 'status_id.is_current_member', | ||
| 'start_date', 'join_date', 'end_date', 'status_id'); | ||
| $memberships = array(); | ||
| 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, | ||
|
There was a problem hiding this comment. 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. |
||
| 'return' => $membershipFields, | ||
| 'options' => ['limit' => 0], | ||
| )); | ||
| } | ||
| elseif (!empty($primaryContributionID)) { | ||
| // Load membership associated with original contribution | ||
| $membershipPaymentResult = civicrm_api3('MembershipPayment', 'get', array( | ||
| 'contribution_id' => $primaryContributionID, | ||
| 'options' => ['limit' => 0], | ||
| )); | ||
| if (!empty($membershipPaymentResult['count'])) { | ||
| foreach ($membershipPaymentResult['values'] as $payment) { | ||
| $membershipIDs[] = $payment['membership_id']; | ||
| } | ||
| $membershipResult = civicrm_api3('Membership', 'get', array( | ||
|
There was a problem hiding this comment. hmmm, why not just get the memberships using $primaryContributionID directly instead of fetching the payments IDs from the payments first ? There was a problem hiding this comment. @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? |
||
| 'id' => array('IN' => $membershipIDs), | ||
| 'return' => $membershipFields, | ||
| 'options' => ['limit' => 0], | ||
| )); | ||
| } | ||
| } | ||
| if (isset($membershipResult) && !empty($membershipResult['count'])) { | ||
| $memberships = $membershipResult['values']; | ||
| } | ||
|
|
||
| // Loop through all found memberships and update status/renew | ||
| foreach ($memberships as $membershipId => $membership) { | ||
| if ($membership) { | ||
| if ((!empty($contribution->contribution_recur_id)) | ||
| && (!CRM_Member_BAO_Membership::isRecurFrequencyEqualToMembershipType($membership['membership_type_id'], $membership['contribution_recur_id']))) { | ||
| Civi::log()->warning('You have enabled auto-renew on membership (id=' . $membership['id'] . ') but the frequencies do not match! The membership will not be auto-renewed.'); | ||
| continue; | ||
| } | ||
|
|
||
| $membershipParams = array( | ||
| 'id' => $membership['id'], | ||
|
There was a problem hiding this comment. add space before line |
||
| 'contact_id' => $membership['contact_id'], | ||
| 'is_test' => $membership['is_test'], | ||
| 'membership_type_id' => $membership['membership_type_id'], | ||
| 'membership_activity_status' => 'Completed', | ||
| ); | ||
|
|
||
|
There was a problem hiding this comment. hmmm, but what if the membership type changed ? , your new code removed this query that handle that but did not offer a replacement There was a problem hiding this comment. @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. |
||
| $membershipParams['num_terms'] = $contribution->getNumTermsByContributionAndMembershipType( | ||
| $membershipParams['membership_type_id'], | ||
| $primaryContributionID | ||
| ); | ||
|
|
||
| // Does the contact have a "current" membership (ie. not expired/pending etc). | ||
| if (!empty($membership['status_id.is_current_member'])) { | ||
| /* | ||
| * Fixed FOR CRM-4433 | ||
| * In BAO/Membership.php(renewMembership function), we skip the extend membership date and status | ||
| * when Contribution mode is notify and membership is for renewal ) | ||
| * FIXME: Is this still required? | ||
| */ | ||
| CRM_Member_BAO_Membership::fixMembershipStatusBeforeRenew($membership, $changeDate); | ||
| } | ||
|
|
||
| // Tell the Membership BAO to calculate membership status. | ||
| $membershipParams['skipStatusCal'] = 0; | ||
| $membershipParams['exclude_is_admin'] = TRUE; | ||
| $membershipParams['is_override'] = FALSE; | ||
| $membershipParams['status_override_end_date'] = 'null'; | ||
| } | ||
|
|
||
| //CRM-17723 - reset static $relatedContactIds array() | ||
| // @todo move it to Civi Statics. | ||
| $var = TRUE; | ||
| CRM_Member_BAO_Membership::createRelatedMemberships($var, $var, TRUE); | ||
| civicrm_api3('Membership', 'create', $membershipParams); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this part a change in behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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).