Replacing 'Related Payment Plan Periods' custom group with 'Is active' custom field, that is part of a new custom group called 'Payment plan extra attributes', the reason is to reduce complexity especially when importing data from other systems using the Importer extension.
Before
We had a custom group called 'Related Payment Plan Periods' that has two custom fields called "previous period" and "next period", these custom fields served 3 purposes :
1- Keeping a historical record of which payment plan followed which, in case of multiple instalments renewals.
2- And most importantly, Without it, we won't be able to know which payment plan to renew, for example, if we have a membership with 3 terms (means 3 payment plans), and let's suppose that the first payment plan (term) ended by the end of 2018, the 2nd term ended by 2019 and the 3rd by 2020, when we auto-renew the autorenewal job will see that the 2020 term (the 3rd one) does not have a "next period" (set to NULL) and thus it will pick it for renewal. Now while this could have been solved by always getting the last payment plan for a given contact, it won't work if there is more than one active payment plan for that contact for two or more different memberships.
3- We only allow people to change payment plan instalments using the manage instalments screen, and we only allow it for the last term, and we use these fields to look for the one with no "next period" (the last one) and thus we only show manage instalments screen link for it.
Now while working on the importer extension, we discovered that it is hard to fill the "previous period" and "next period" custom fields (that are part of 'Related Payment Plan Periods'), since most systems do not have a similar structure for payment plans. And as a solution to this, we found that it might be easier to replace these two custom fields with a new one called "is active" that is only marked as true for the most recent payment plan that is related to the membership.
The contact with this approach can still have more than one active payment plans for different memberships same as with the current approach, but we are just simplifying things for the importer.
Here is a diagram that shows the current model for a membership with 3 terms :
as you can see, it represents a doubly linked list where each payment plan for the membership have a link for the one that comes before and after it.
After
With the new approach and as described above, the first time you create a multi instalment payment plan membership, it will be set as Active, and when you renew it, the old one will be deactivated and the new one will be set as Active and that goes on...
At the time of the creation
1st renewal
2nd renewal
..etc
Also, as part of this change, I removed the upgrader that used to make old payment plans compatible with Membershipextras upon its installation (by creating subscription line items and some other related data), The reason is such data is complex and the upgrader makes many assumptions where rather such kind of data should be created as part of the data import process.
I update the autorenewal jobs to only auto-renew plans that are active and ensure that the newly created plan is will be active, and I also update the code that controls the link to the "manage instalments" screen to use the new custom field.
3- Also as part of the same commit, I changed this class CRM/MembershipExtras/Hook/PostProcess/RecurringContributionLineItemCreator.php by removing the "$hasPreviousPeriod " condition which was using the "previous period" custom field, the reason is that I couldn't find any real value or purpose for having it, given that it's only called from certain contexts that none of them requires such check.
7- In this commit, I changed the name of the new custom group from 'Is payment plan active?' to 'Payment plan extra attributes' to make it more usable in the future in case we need to add more fields to manage payment plans.
hence that all of this works for multiple instalments, for a single instalment there is only one payment plan that will always be active (unless the payment plan is cancelled).
Note about tests
There was some non-related failure in tests (that affected all new PRs) which I fixed here temporarily here :
Overview
Replacing 'Related Payment Plan Periods' custom group with 'Is active' custom field, that is part of a new custom group called 'Payment plan extra attributes', the reason is to reduce complexity especially when importing data from other systems using the Importer extension.
Before
We had a custom group called 'Related Payment Plan Periods' that has two custom fields called "previous period" and "next period", these custom fields served 3 purposes :
1- Keeping a historical record of which payment plan followed which, in case of multiple instalments renewals. 2- And most importantly, Without it, we won't be able to know which payment plan to renew, for example, if we have a membership with 3 terms (means 3 payment plans), and let's suppose that the first payment plan (term) ended by the end of 2018, the 2nd term ended by 2019 and the 3rd by 2020, when we auto-renew the autorenewal job will see that the 2020 term (the 3rd one) does not have a "next period" (set to NULL) and thus it will pick it for renewal. Now while this could have been solved by always getting the last payment plan for a given contact, it won't work if there is more than one active payment plan for that contact for two or more different memberships. 3- We only allow people to change payment plan instalments using the manage instalments screen, and we only allow it for the last term, and we use these fields to look for the one with no "next period" (the last one) and thus we only show manage instalments screen link for it.
Now while working on the importer extension, we discovered that it is hard to fill the "previous period" and "next period" custom fields (that are part of 'Related Payment Plan Periods'), since most systems do not have a similar structure for payment plans. And as a solution to this, we found that it might be easier to replace these two custom fields with a new one called "is active" that is only marked as true for the most recent payment plan that is related to the membership.
The contact with this approach can still have more than one active payment plans for different memberships same as with the current approach, but we are just simplifying things for the importer.
Here is a diagram that shows the current model for a membership with 3 terms :
as you can see, it represents a doubly linked list where each payment plan for the membership have a link for the one that comes before and after it.
After
With the new approach and as described above, the first time you create a multi instalment payment plan membership, it will be set as Active, and when you renew it, the old one will be deactivated and the new one will be set as Active and that goes on...
At the time of the creation
1st renewal
2nd renewal
..etc
Also, as part of this change, I removed the upgrader that used to make old payment plans compatible with Membershipextras upon its installation (by creating subscription line items and some other related data), The reason is such data is complex and the upgrader makes many assumptions where rather such kind of data should be created as part of the data import process.
Technical Details
1- Here https://github.com/compucorp/uk.co.compucorp.membershipextras/pull/355/commits/97af40856aff0f017c66f71712fa7712fc078866 I migrate the data from the old custom group to the new one, I assume that any payment with next_period set to Null is the last payment plan in the renewal sequence, and thus I only set them to be active.
2- Here https://github.com/compucorp/uk.co.compucorp.membershipextras/pull/355/commits/866c29805b6c4216407fbfce5d3474131a8b4092
I update the autorenewal jobs to only auto-renew plans that are active and ensure that the newly created plan is will be active, and I also update the code that controls the link to the "manage instalments" screen to use the new custom field.
3- Also as part of the same commit, I changed this class CRM/MembershipExtras/Hook/PostProcess/RecurringContributionLineItemCreator.php by removing the "$hasPreviousPeriod " condition which was using the "previous period" custom field, the reason is that I couldn't find any real value or purpose for having it, given that it's only called from certain contexts that none of them requires such check.
4- Here https://github.com/compucorp/uk.co.compucorp.membershipextras/pull/355/commits/0bdb5b1326b84a3ec5331a91c7aeecb70a1b5bbb I removed the code that makes payment plans compatible with membership extras (see why above).
5- Here https://github.com/compucorp/uk.co.compucorp.membershipextras/pull/355/commits/fa03fe3a993a295d7a12c74a3bc99861275c89df I make sure that the payment plan active field is set correctly when doing manual renewal.
6- Here https://github.com/compucorp/uk.co.compucorp.membershipextras/pull/355/commits/94d3bf061de59a6d084964bdd87f6518044dc38a I am adding a webform API that will be called from webform_civicrm_membership_extras in this PR https://github.com/compucorp/webform_civicrm_membership_extras/pull/31 to make sure the active field is set correctly on webform submissions.
7- In this commit, I changed the name of the new custom group from 'Is payment plan active?' to 'Payment plan extra attributes' to make it more usable in the future in case we need to add more fields to manage payment plans.
hence that all of this works for multiple instalments, for a single instalment there is only one payment plan that will always be active (unless the payment plan is cancelled).
Note about tests
There was some non-related failure in tests (that affected all new PRs) which I fixed here temporarily here :
https://github.com/compucorp/uk.co.compucorp.membershipextras/pull/355/commits/5f5eb4026ec75dfa81d89de6df86072027adeba2
The explanation for this fix is in the commit message.