Completing a pending payment for a single contribution/no payment plan membership resulted in renewing the membership twice, or in more accurate terms, extending the membership end date twice.
Before
This issue can be produced by more than one way :
scenario 1
1- Create a new single payment membership with a pending contribution (Not Payment Plan)
2- Let's say membership start date -May 7th, 2020, end Date - May 6th, 2021
3- Record a payment for the pending contribution and use any payment method or use "Submit Credit Card payment"
4- Check the membership end date of the membership . It will be May 6th, 2022 instead of staying on May 6th, 2021.
scenario 2 (it is a little hard to show a gif or pics for this case since the Sagepay account belongs to a client)
1- Create a webform and configure it with single membership and single contribution without payment plan, and also configure it to allow users to pay using a payment processor that uses IPN, in this example we used Sagepay payment processor.
2- After submitting the webform, it will redirect the user to Sagepay website to enter the credit card details and once the user go through that and submit the payment, the created membership will have a start date of May 7th, 2020 (assuming today date is 7th, 2020) and end Date - May 6th, 2022, but the end should have being May 6th, 2021.
After
In both cases above, the membership should not get extended again after completing the payment, here is a gif for the scenario 1 :
Technical Details
The issue happen because in Membershipextras we recalculate the membership status inside MembershipPayment pre create hook, here is how the hook implementation looks like :
public function postProcess() {
if ($this->operation == 'create') {
$this->fixRecurringLineItemMembershipReferences();
$this->recalculateMembershipStatus();
}
}
we recalculate the membership status here (using recalculateMembershipStatus() method) because we try to find out if the membership status should be in-arrears or not. this also have a desired side effect, since we want payment plans with pending contribution(s) to not affect the membership status which is usually not the case on CiviCRM core where a pending contribution means a pending membership, where in Membershipextras and using payment plans we often give the user the benefits of the membership even if the payment is still pending. this happen because we use MembershipStatus.calc API which mainly depends on the membership dates to calculate its status so if the user paid a payment plan with pending contribution(s), CiviCRM will create the membership in the pending status, but after MembershipStatus.calc API get called it reupdate the membership status based on its current dates which is usually the "New" or "Current" status for newly created memberships.
Now the problem with the "New" or "Current" status for memberships with pending contribution(s), is that CiviCRM will treat it differently from "Pending" memberships with pending contribution(s) which will result in running the following CiviCRM core code inside the Contribution BAO class :
if ($currentMembership) {
/*
* 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 )
*/
CRM_Member_BAO_Membership::fixMembershipStatusBeforeRenew($currentMembership, $changeDate);
// @todo - we should pass membership_type_id instead of null here but not
// adding as not sure of testing
$dates = CRM_Member_BAO_MembershipType::getRenewalDatesForMembershipType($membershipParams['id'],
$changeDate, NULL, $membershipParams['num_terms']
);
$dates['join_date'] = $currentMembership['join_date'];
}
which what results in extending the membership end date twice.
Now in case of using external payment processor with IPN is quite similar, here is the methods that get called in case of Sagepay PP :
1- after submitting the webform and before it redirects to Sagepay page, a membership with "new" status will be create as well as a pending contribution.
2- once the user get redirected to Sagepay and enter the credit card detail and complete the payments, Sagepay will trigger an IPN request which will call completeTransaction() method.
3- completeTransaction() method will call CRM_Contribute_BAO_Contribution::completeOrder() method.
4- CRM_Contribute_BAO_Contribution::completeOrder will call updateMembershipBasedOnCompletionOfContribution() method.
5- because the membership status is not "pending" as it supposed to be in normal civicrm core, it will result in running what inside the following if statement :
if ($currentMembership) {
/*
* 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 )
*/
CRM_Member_BAO_Membership::fixMembershipStatusBeforeRenew($currentMembership, $changeDate);
// @todo - we should pass membership_type_id instead of null here but not
// adding as not sure of testing
$dates = CRM_Member_BAO_MembershipType::getRenewalDatesForMembershipType($membershipParams['id'],
$changeDate, NULL, $membershipParams['num_terms']
);
$dates['join_date'] = $currentMembership['join_date'];
}
which results in extending the membership again.
Now the reason is this happen only on memberships paid with single contribution/no payment plan, is because for payment plans we have code in many places that controls the end date of the membership by either extending it or preventing it from getting extended.
in this PR, we prevent the MembershipPayment pre edit hook from getting called for non payment plan payments :
if ($this->operation == 'create' && !empty($this->relatedRecurContributionId)) {
are no longer getting called for non payment plan payments, which is safe for three reasons :
1- $this->fixRecurringLineItemMembershipReferences() : this method only operate on payment plans from the beginning, so it make no sense to call for non payment plans payments.
2- $this->recalculateMembershipStatus(); : this method was mainly added to help checking and altering the membership status to in-arrears or not in case there are pending payments in the past, but in-arrears related statuses also only meant (and make sense) to be used on payment plans memberships, so it also make no sense to call for non payment plan payments.
3- not calling $this->recalculateMembershipStatus(); for non payment plans payments is what fixes the issue here, since the membership in this case will be created in pending status instead of "New" or "Current", but unlike payment plan payments, it is fine if the membership status is not "New" or "Current" for non payment plan payments since it is how CiviCRM core work when it comes to that. so we are more reverting it to how it originally work.
Overview
Completing a pending payment for a single contribution/no payment plan membership resulted in renewing the membership twice, or in more accurate terms, extending the membership end date twice.
Before
This issue can be produced by more than one way :
scenario 1 1- Create a new single payment membership with a pending contribution (Not Payment Plan) 2- Let's say membership start date -May 7th, 2020, end Date - May 6th, 2021 3- Record a payment for the pending contribution and use any payment method or use "Submit Credit Card payment" 4- Check the membership end date of the membership . It will be May 6th, 2022 instead of staying on May 6th, 2021.
scenario 2 (it is a little hard to show a gif or pics for this case since the Sagepay account belongs to a client) 1- Create a webform and configure it with single membership and single contribution without payment plan, and also configure it to allow users to pay using a payment processor that uses IPN, in this example we used Sagepay payment processor. 2- After submitting the webform, it will redirect the user to Sagepay website to enter the credit card details and once the user go through that and submit the payment, the created membership will have a start date of May 7th, 2020 (assuming today date is 7th, 2020) and end Date - May 6th, 2022, but the end should have being May 6th, 2021.
After
In both cases above, the membership should not get extended again after completing the payment, here is a gif for the scenario 1 :
Technical Details
The issue happen because in Membershipextras we recalculate the membership status inside MembershipPayment pre create hook, here is how the hook implementation looks like :
we recalculate the membership status here (using recalculateMembershipStatus() method) because we try to find out if the membership status should be in-arrears or not. this also have a desired side effect, since we want payment plans with pending contribution(s) to not affect the membership status which is usually not the case on CiviCRM core where a pending contribution means a pending membership, where in Membershipextras and using payment plans we often give the user the benefits of the membership even if the payment is still pending. this happen because we use MembershipStatus.calc API which mainly depends on the membership dates to calculate its status so if the user paid a payment plan with pending contribution(s), CiviCRM will create the membership in the pending status, but after MembershipStatus.calc API get called it reupdate the membership status based on its current dates which is usually the "New" or "Current" status for newly created memberships.
Now the problem with the "New" or "Current" status for memberships with pending contribution(s), is that CiviCRM will treat it differently from "Pending" memberships with pending contribution(s) which will result in running the following CiviCRM core code inside the Contribution BAO class :
which what results in extending the membership end date twice.
Now in case of using external payment processor with IPN is quite similar, here is the methods that get called in case of Sagepay PP :
1- after submitting the webform and before it redirects to Sagepay page, a membership with "new" status will be create as well as a pending contribution. 2- once the user get redirected to Sagepay and enter the credit card detail and complete the payments, Sagepay will trigger an IPN request which will call completeTransaction() method. 3- completeTransaction() method will call CRM_Contribute_BAO_Contribution::completeOrder() method. 4- CRM_Contribute_BAO_Contribution::completeOrder will call updateMembershipBasedOnCompletionOfContribution() method. 5- because the membership status is not "pending" as it supposed to be in normal civicrm core, it will result in running what inside the following if statement :
which results in extending the membership again.
Now the reason is this happen only on memberships paid with single contribution/no payment plan, is because for payment plans we have code in many places that controls the end date of the membership by either extending it or preventing it from getting extended.
in this PR, we prevent the MembershipPayment pre edit hook from getting called for non payment plan payments :
which means that :
are no longer getting called for non payment plan payments, which is safe for three reasons :
1- $this->fixRecurringLineItemMembershipReferences() : this method only operate on payment plans from the beginning, so it make no sense to call for non payment plans payments.
2- $this->recalculateMembershipStatus(); : this method was mainly added to help checking and altering the membership status to in-arrears or not in case there are pending payments in the past, but in-arrears related statuses also only meant (and make sense) to be used on payment plans memberships, so it also make no sense to call for non payment plan payments.
3- not calling $this->recalculateMembershipStatus(); for non payment plans payments is what fixes the issue here, since the membership in this case will be created in pending status instead of "New" or "Current", but unlike payment plan payments, it is fine if the membership status is not "New" or "Current" for non payment plan payments since it is how CiviCRM core work when it comes to that. so we are more reverting it to how it originally work.