Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAE-522: Prevent extending membership end date #383

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ahed-compucorp
Copy link
Contributor

@ahed-compucorp ahed-compucorp commented Jun 2, 2021

Overview

Usually recording the payment for the membership's contribution means :

  • Activating the membership by changing its status from pending to active.
  • Renewing the membership by extending its end_date.

We have a use case when the admin adds a new line items to a completed contribution and that will change its status to "Partially paid", but recording the payment for that contribution will trigger the renewal membership processing if the contribution was related to a membership.

Before

Recording a payment for a partially paid contribution will renew its related membership.

Peek 2021-06-02 11-33

After

Recording a payment for a partially paid contribution will not renew its related membership.

Peek 2021-06-02 11-31

Technical Details

The method updateMembershipBasedOnCompletionOfContribution at Contribution.php#L4793 does not handle the case when recording another payment for the membership's contribution.

At Compucorp, we use the "biz.jmaconsulting.lineitemedit" extension to add and edit the line items.

Adding a line item changes the contribution status to "Partially Paid" and recording a payment for an active membership's contribution will renew it by default and extend its end_date, but this is undesirable behaviour and this PR will prevent it.

At first, I tried to check the MembershipPayment but that wasn't right because the CiviCRM will create it during the membership creation or renewing and not during the payment recording. Then I checked if the membership was active and not is_pay_later with checking if the contribution is Partially paid. Lastly, I tried to check if the contribution's paid amount is larger than the membership fee , but I couldn't think of a use case that will need it so I removed it from this PR.

Comments

It was a bit tricky to decide where to send this PR, let me see what you think about it.

Usually recording the payment for the membership's contribution means :
- Activating the membership by changing its status from pending to
active.
- Renewing the membership by extending its end_date.

At Compucorp, we use the "biz.jmaconsulting.lineitemedit" extension to
add and edit the line items.

Adding a line item changes the contribution status to "Partially Paid"
and recording a payment for an active membership's contribution will
renew it by default and extend its end_date, but this is undesirable
behaviour and this commit will prevent it.
* Prevents editing the active Membership if its contribution has another
* payment.
*
* Usually recording the payment for the membership's contribution means :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case only for non payment-plans payments , it worth noting that here.

*/
public function preventEditingTheActiveMembershipIfItsContributionHasAnotherPayment() {
// Check if the membership is active and is not pay later.
$membershipCount = civicrm_api3('Membership', 'getcount', [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but I think using "get" is more efficient.

'sequential' => 1,
'active_only' => 1,
'id' => $this->id,
'is_pay_later' => 0,
Copy link
Member

@omarabuhussein omarabuhussein Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Civi does not keep this field updated, so if you create a membership with :

1- Pending contribution (is_pay_later will be 1)
2- Complete the contribution (is_pay_later will remain 1)

so if someone then added a new line item to the completed contribution then it won't be picked here, since is_pay_later is still = 1 since Civi does not update it.

but to be honest, I don't think it is needed, so If the membership is :

1- active
2- and the contribution is currently partially paid and we are trying to record another payment.

then just skip updating its end date, since CiviCRM only extends the date if the payment is completed. But this might cause issues if we are using the line item editor with Payment plans, so I just asked Sheela on the ticket if we are using it with payment plans.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a condition to skip payment plans.

Copy link
Member

@omarabuhussein omarabuhussein Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I am rethinking this, and even the current approach might be an issue for manual renewal, here is why:

1- Create a Membership with a completed contribution (the membership status will be active).
2- Renew the membership manually but with a Pending contribution (the membership will remain active, and the end date will not get extended yet since the contribution is pending).
3- Partially pay the contribution (the membership is still active, and the end date still the same since Civi only extends the end date if the contribution got completed).
4- Pay the remaining amount of the contribution, now this step on CiviCRM core will complete the contribution, then extend the membership end date, but with this new logic, it will see an active membership with a partially paid contribution getting completed, and thus it will prevent the end date from getting extended, which is wrong.

at this point, I am not sure how can we make this work, will need more thinking.

@ahed-compucorp ahed-compucorp force-pushed the MAE-522-prevent-extending-membership-end-date branch from 45b47da to aa1869a Compare June 10, 2021 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants