Don't freeze fields for auto-renew memberships

changed the description

added ~81 needs:concept approval needs:docs need update triaged labels

I do like your proposal of making membership editable when they are linked to a recurring contribution. I cannot think of any good reason why not. However I do think it is good to have some sort of discussion on it, just to make sure nothing breaks.

I have added the tag needs documentation so that the user/admin manual also gets updated.

Thanks @jaapjansma. There's been a fair bit of discussion about this already at the sprint and in GitLab/GitHub, but good to have some more input. As you say we don't want to break anything!

See:

A fix for the end_date has now been merged into core: https://github.com/civicrm/civicrm-core/pull/15540

@mattwire @eileen what should I do to take this forward?

Hi @wmortada - basically we just need good answers / agreement for each of the 3 issues and an understanding of their impact. Ideally try and get some membership organisations to comment.

The ability to edit the membership type is an issue for our organisation because sometimes people sign up for the wrong membership type and we need to change this. For non-recurring payments this is no problem, we can just change it manually and if necessary take additional payment or make a refund. But for recurring payments we are unable to do this because the field is frozen. We are then stuck and have no way of changing the membership type in CiviCRM through the UI.

There was an idea (probably just because the organisation that originally implemented did it that way) that the recur amount followed the membership amount and could not be edited. However, in many circumstances this really doesn't make sense - you are making a recurring payment in relation to a membership. That membership has a default price but there is no reason why the organisation staff can't choose to modify the price for it once setup. There could be an issue when switching the frequency as you'd end up with a monthly recur linked to an annual membership and after 12 months your membership would be extended by 12 months. But it's already possible to do this so opening this up won't make it any worse. I'm a +1 on allowing this to be changed.

The ability to edit the membership organisation isn't an issue for us as we only have one membership organisation. However, other organisations may want to be able to do this for the same reason as above.

I think there used to be some "one membership type" per organisation type restrictions but these don't make sense or apply anymore. If we unfreeze membership type above then I think we should do the same for organisation.

The ability to edit the membership status also isn't an issue for us, but I don't really understand the logic of freezing it for auto-renew memberships and so I'm also including it here for consistency.

The membership status is calculated automatically by a scheduled job, so unless overriding I'm not sure that it would be a good idea to allow it to be changed here as it would just get changed back next time cron ran.

Thanks for the responses @mattwire.

The membership status is calculated automatically by a scheduled job, so unless overriding I'm not sure that it would be a good idea to allow it to be changed here as it would just get changed back next time cron ran.

I think there may be circumstances in which an organisation needs to override the membership status (possibly for a limited period of time). For example they may want to temporarily disable a membership if there is a dispute. Obviously the organisation will need to think about why they are doing it and the consequences of making this change.

The argument about it being changed back on the next cron run also applies to memberships that aren't auto-renew. I don't see why auto-renew memberships should be treated differently. For consistency I think it would be better to remove all of the logic that freezes these fields.

My basic argument is that whether these fields should be frozen or not is down to the business logic of the individual organisation. I don't think it should be imposed by core CiviCRM.

I think there may be circumstances in which an organisation needs to override the membership status (possibly for a limited period of time). For example they may want to temporarily disable a membership if there is a dispute. Obviously the organisation will need to think about why they are doing it and the consequences of making this change.

@wmortada Have you seen the membership status override function in core? I'm not sure if it was in the version you are on but it's in recent versions and does what you are requesting?

@mattwire yes, I'm aware of that. It's a great feature but unfortunately it isn't available if the membership is set to auto-renew because the membership status field is frozen. Hence my suggestion not to freeze theses fields.

changed the description

@wmortada Great! So I misunderstood. In which case I'm in favour of unfreezing the membership status override for auto renew memberships too!

I think historically there were no payment processors that supported edits on recurring payments. My concern is that changes be backward compatible, so if new behaviour is desired, then do something like have a property that defaults to unavailable for payment processors but can be enabled when code is there to handle it. Also, my sense is that most of the code related to the changes above is dependent not on a specific payment processor, but the capabilities of a payment processor. In other words, please make the code generic, and not packaged with a payment processor. @mattwire do you have thoughts on where it should be packaged?

@JoeMurray We are actually talking about removing a couple of lines of code that prevents you from changing some parameters, rather than adding any code. I'm happy with that and I think in the unlikely event that a specific payment processor required those restrictions it could implement them directly in the processor.

I've recently discovered this question from @michaelmcandrew on StackExchange about the membership status field:

Can I status override an auto renewing membership?

This question provides more evidence that freezing these fields is causing issues for organisations using CiviCRM.

The nub of the issue is why are we treating auto-renew memberships differently from non auto-renew memberships?

Given that the SE question has been around since January and June of this year, and no one has been able to come up with a good reason for this functionality being in place, I would vote for approving this in concept. It feels like one of those situations where we switching from meeting 10% of people's needs to 90% in a way that will not cause issues for the 10% (since it still requires a user to actually make a change to the membership record.

I put out another mention here: https://chat.civicrm.org/civicrm/pl/f7gzqqd84jbh8yuora19k6ye5h. I would be happy to put out a broadcast message on civicrm-dev if people want to get more eyeballs on it, but that might be overkill at this point.

The issue isn't the amount of code but not breaking things. It's not a pattern we have favoured to require changes in a number of extensions in order to prevent problems. What would happen if people tried to change the membership type to one with a different amount and an auto renewing membership payment could not have its amount changed because that has never been implemented?

That sounds like a legitimate argument for freezing membership type (and membership organisation) - thanks @JoeMurray

I don't know enough about the internals of how the membership and the recurring contribution are linked to comment further, but I presume the key question to answer is: 'Does the membership update care that the amount was different? Does it do any checking to ensure that the payment is >= the minimum fee, or does it just renew it by 1 membership period by virtue of the fact that the recurring contribution is related to the membership?

@JoeMurray - do you have any similar reasons to prevent the user from overriding the membership status?

Membership status on an autorenewing membership should generally be driven by whether payments are succeeding or not. Sure, I suppose one could set membership status override to true then set the status to something even if there is no connection back to the payment processor. But then you might end up continuing to collect payments for a cancelled membership. This is complex, unfortunately tightly coupled code between membership status and payments...it would need a thorough review of all use cases before I'd be comfortable concluding changes wouldn't lead to serious grief for some installations.

Membership status on an autorenewing membership should generally be driven by whether payments are succeeding or not.

Agreed (this is the same for non auto-renewing memberships)

one could set membership status override to true then set the status to something even if there is no connection back to the payment processor. But then you might end up continuing to collect payments for a cancelled membership

Yes - but I think that is the point of status override. It is a chance for the user to say that there is a reason to deviate from what would normally happen based on payments.

tightly coupled code between membership status and payments

Could you elaborate on the tightly coupledness? Is there anything specific to auto-renew membership payments that leads you think think that it should be considered differently to auto-renewing memberships?

I wonder if it makes sense to split membership type and membership status into separate issues as has been done for membership end date.

I've done a bit more digging and have managed to identify when this behaviour was first introduced. It looks like it dates back to a commit from Deepak Srivastava on 22 December 2010.

https://github.com/civicrm/civicrm-svn/commit/44322fdf62e414673acc4922dba60105f7143c1f#diff-fa0276456bc05f4629f775e6846f62edR518

This dates from a time when SVN was used for version control so the details are a bit limited but it appears to relate to CiviCRM 3.3 and the Authorize.net payment processor.

Perhaps @deepak.srivastava might remember more about the rationale behind this?

@mattwire it looks like you are the current maintainer of the Authorize.net extension. Any thoughts on whether this functionality is still needed for that extension to work correctly?

I am reluctant to introduce changes that might lead people to think that they had stopped payments when they hadn't, or to create problems that will not be presented to users promptly. At a minimum a warning would be good.

I would be prefer to see a warning if one could override status to cancelled of an auto-recurring contribution if its payment processor did not support that.

Changing status of a membership associated with a series of payments is a more complicated thing for administrators to get their head around than just changing the status of the payment series. It is not obvious to me that people will all expect the system to either do something to the payment or not; my sense is that expectatiins will be divided.

I've been looking in to this a bit more. It looks like payment processors can set a flag to say that they support cancelling a subscription. If so the user is given a link to cancel the subscription.

if ($isUpdateToExistingRecurringMembership && CRM_Member_BAO_Membership::isCancelSubscriptionSupported($this->_id)) {
      $this->assign('cancelAutoRenew',
        CRM_Utils_System::url('civicrm/contribute/unsubscribe', "reset=1&mid={$this->_id}")
      );
    }

Lines 650-654 of CRM/Member/Form/Membership.php

I suggest that for payment processors that support this option we leave the functionality as it is but we unfreeze the fields for those that don't. This would work out as follows:

If the payment processor supports cancelling a subscription:

the fields are frozen
the user can cancel the subscription from within CiviCRM

If the payment processor doesn't support cancelling a subscription:

the fields are not frozen so the user is able to make changes
their is a warning message to inform users that they will have to manually make changes to the subscription at the payment processor

How does that sound?

I've submitted a PR: https://github.com/civicrm/civicrm-core/pull/16217

Needs a bit of work to display the membership type fields but let me know if you have any comments.

I've closed that PR and I'm going to try a different approach to achieving the same end. Hopefully get round to this in the next week.

I've submitted a new PR: https://github.com/civicrm/civicrm-core/pull/16322

This takes a slightly different approach and disables the membership type field using JavaScript rather than freezing the field in QuickForm. Doing it this way means that the field can easily be enable again using JavaScript.

@JoeMurray @mattwire @michaelmcandrew would be great to get your thoughts on my PR (above).

@wmortada - having another look at this now.

I'm not sure that deciding whether or not to freeze fields should depend on whether the membership processor supports cancellation.

There are separate threads of argument for whether it makes sense to unfreeze the different fields (membership type, and membership status). I feel like it would be easier to come to consensus on either one separately.

From what you said initially, you are interested in unfreezing the membership type so maybe we should concentrate on that. You don't know why the membership status is frozen, but maybe you wouldn't care it if remained frozen.

So maybe we should concentrate on membership type?

I'm still not sure of the answer to the question 'Does CiviCRM do any checking to ensure that the recurring payment associated with a membership is appropriate before renewing it but if I had to guess, based on when I last looked at the code, I think it renews it by 1 membership period by virtue of the fact that the recurring contribution is related to the membership.

So if we unfreeze the membership type column, we are putting the onus on the admin to ensure that the recurring payment is indeed valid for the membership payment. Whether or not you think admins should have that power is a bit of a philosophical discussion. Personally, I think they should have that power, as long as the understand the consequences. Maybe we add a warning in the UI? I can't see a way that this would lead to 'serious grief' since any changes would have to be the result of intervention on the part of the admin.

I like the idea of getting this into core, but if we can't come to an agreement on the above, then an extension that unfreezes those fields would be simple to implement and maintain.

My recollection is that at the time we were doing this stuff none of the payment processor integrations that supported recurring donations, which are commonly used for memberships, supported changing the amount or stopping an existing series of recurring donations (for example, in order to start a new one of a different amount). They tended to (maybe all of them) give responsibility to the payment processor for initiating subsequent payments. We didn't want the user interface in CiviCRM to give the impression that things would work when they wouldn't without manual intervention at the payment processor.

We now have some payment processor integrations I believe that support calls to change the amount on existing recurring payments. Some do it by talking to the payment processor and telling them to handle it; others do it because they have the CiviCRM side change the amount it asks for using Card on File during subsequent requests for payments on the series.

I would be comfortable with allowing membership type to be changed in backend if ((price of old and new membership types is the same) OR (price of old and new membership types is different AND ((membership is not recurring) OR (membership is recurring AND payment processor supports changing the amount of a recurring membership)))).

I'm not in agreement with Michael on one point. I still think it is unacceptable to give the silent impression that things are working fine when the system knows that a problem will occur unless the user does something outside the system. At a minimum, there should be a warning popup that explains what is required each time an unsupported action is requested that requires action elsewhere, and asks the user to confirm. It's not enough to set once and assume that all future users will know about this. I think core could check capabilities of payment processor and put up a warning if they are not there.

Hey Joe,

Thanks for the input

I still think it is unacceptable to give the silent impression that things are working fine when the system knows that a problem will occur unless the user does something outside the system.

I agree that this wouldn't be ideal (I did suggest adding a 'I hope you know what you are doing' warning in the UI).

I would be comfortable with allowing membership type to be changed in backend if ((price of old and new membership types is the same) OR (price of old and new membership types is different AND ((membership is not recurring) OR (membership is recurring AND payment processor supports changing the amount of a recurring membership)))).

This sounds sensible.

Thanks for your input @michaelmcandrew and @JoeMurray. That's much appreciated.

I'm not sure that deciding whether or not to freeze fields should depend on whether the membership processor supports cancellation.

My thinking here was to retain the current behaviour for payment processors that support cancellation. My concern is with the payment processors that don't support cancellation.

If the payment processor supports cancellation the user is given the option to cancel the subscription which will then unfreeze the fields. At least the user is able to do something.

If the payment processor doesn't support cancellation then there is nothing that the user can do in CiviCRM to unfreeze the fields. Even if they make the required changes at the payment processor they are still unable to correct the CiviCRM record.

If you think it would be better to unfreeze fields regardless then I'd be more than happy to change that. I was just trying to reduce the scope of the change.

So maybe we should concentrate on membership type?

~~Happy to focus on membership organisation/type. This is what is causing us an issue at the moment.~~ (Being able to change the membership status is also an issue for us.)

In our use case we have two membership types that are for the same amount so changing between these two doesn't require any change at the payment processor. We just need to be able to change the membership type in CiviCRM.

We have another membership type for a higher amount. In that case we would need to increase/reduce the subscription amount at the payment processor as well as changing the membership type in CiviCRM. We may also need to take a one-off payment for the difference between the fees between the two membership types.

I still think it is unacceptable to give the silent impression that things are working fine when the system knows that a problem will occur unless the user does something outside the system.

I have added help text with a suitable warning. Do you think this needs to be more overt or that the text needs to be changed?

I would be comfortable with allowing membership type to be changed in backend if ((price of old and new membership types is the same) OR (price of old and new membership types is different AND ((membership is not recurring) OR (membership is recurring AND payment processor supports changing the amount of a recurring membership)))).

I agree that this sounds sensible but I would probably put the logic the other way round to say that we would stop users from changing the membership type by freezing this field if ((membership is recurring) AND (price of old and new membership types is different) AND NOT (payment processor supports changing the amount of a recurring membership)).

The problem is that I'm not sure if it is possible for CiviCRM to tell whether payment processor supports changing the amount of a recurring membership. Do payment processor extensions provide this information to CiviCRM?

Also I wanted to clarify if you mean payment processor supports changing the amount or CiviCRM payment processor extension supports changing the amount.

In the case I'm interested in: payment processor supports changing the amount = TRUE but CiviCRM payment processor extension supports changing the amount = FALSE.

Actually being able to change the membership status is an issue for us so I'd like to keep the ability to change this.

A lot of clients I've worked with don't necessarily fix the membership fee based on the membership type. Different people may pay different amounts for the same membership type.

Internally CiviCRM does not have a fixed membership fee. It does have:

A membership type minimum fee - this is basically the default and in some situations forms will not allow to go below this minimum.
A price-set fee - which can specify a membership type.
An "amount" on a recurring contribution.
A set of line items on a contribution.

That already gives us 4 places that a membership fee can be set / modified.

To write an extension that freezes the membership type / org / status (as they are currently) would be:

hook_civicrm_buildForm($formName, $form) {
  if ($formName === 'CRM_Member_Form_Membership') {
    $element = $form->getElement('membership_type_id');
    $element->freeze();
    $element = $form->getElement('is_override');
    $element->freeze();
  }
}

So I would say keep the code simple, get rid of the conditional $canCancelAutoRenew and just make it always override-able with a suitable warning on the form.

As an aside, with lot's of users of Stripe / Smartdebit I tell them to install uk.co.mjwconsult.variablerecurpayments which includes an option/form to unlink/link a membership to a recurring. So they follow the workflow:

Unlink recur from membership.
Make changes to membership.
Link recur to membership.

This is obviously prone to error but allows them to make whatever changes they want. Cancel is problematic because you then need to either get the client to signup again if you've cancelled it with the provider or hack the database to uncancel it again.

The core product should make sense in terms of behaviour for those who do not have programming chops or programming budget. I'll try to come back with more specific suggestion in a few days.

Thanks for the feedback @mattwire @JoeMurray @michaelmcandrew.

I've submitted a new PR which I hope incorporates your suggestions https://github.com/civicrm/civicrm-core/pull/16609

Let me know your thoughts.

Hello, I am a membership administrator for a small charity and want to add my insight as to how we manage membership and why it would be extremely useful to be able to amend the 'membershp type' field for auto-renew memberships.

We have multiple membership types, some of which members upgrade to to gain a different set of member benefits. In this scenario it would be easy to change the recurring contributions they are making by simply altering their subscription for them, but due to the fact the membership type is frozen this would mean they would be paying the correct rate but for the wrong membership type. In this scenario we would actually have to cancel the whole membership and re-start it again with the correct type. This is a bad customer experience for our members and more work for our admin team. It would be a lot quicker if we were able to update the membership type and their payments; job done. I am really keen, for this reason, for this specific field to be unfrozen.

I also agree that it would be useful to be able to amend the membership status. I can think of one example where this would be useful but I am sure there will be many others; for example lets suppose that one member rings to cancel their membership. On this same day there is also a bulk email or postal mailing going out to members that afternoon. It would be better to be able to cancel the membership there and then so that they do not get the mailing, rather than mark their mailing preferences as 'do not mail'. This is because if they at some stage renew (which we hope they will) they would still have those communication settings in place but would have no way of knowing why they were not getting any membership communications, which will be a bad experience for the member and the administrator. This is quite a specific situation but you can understand how there could be many more like this would would mean having control over auto-renew memberships in the same way that we can control regular memberships helps the administrator.

Hope this is useful info!

Thanks for adding your perspective Nicola!

Following @mattwire's suggestion, I'm creating an extension to recreate the existing functionality for those organisations that need it. This is still a work in progress but you can see it here: https://github.com/wmortada/freeze-membership-fields

This appears to be working but I would appreciate it if others could test it and review the code. Thanks!

I'm in the process of moving the extension to GitLab. (Probably should have done that initially but wasn't clear on the process.)

Here's the GitLab link: https://lab.civicrm.org/extensions/freeze-membership-fields

mentioned in commit 0a70c96f

PR #16609 has now been merged into core which fixes this issue. This should be included in CiviCRM 5.25. Hurray!

Thanks to everyone that has contributed to resolving this.

What should we do to notify users of the change and make them aware of the extension if they want to keep the functionality as it is?

I was doing some more testing and spotted an error which would break the 'Cancel auto-renew' link for payment processors that support this option. I've created a new PR to fix this:

https://github.com/civicrm/civicrm-core/pull/16881

This has now been merged into core.

mentioned in commit a62826e4

@wmortada - great! thanks for shepherding this along - a nice improvement.

What should we do to notify users of the change and make them aware of the extension if they want to keep the functionality as it is?

It should go into the release notes at least. Not sure whether is makes sense to add a message to the upgrader (not sure what the policy is on that)

I looked at https://docs.civicrm.org/dev/en/latest/core/contributing/ and https://docs.civicrm.org/dev/en/latest/core/release-process/ I don't think that any of this process is documented. @andrewhunt is probably a good person to correct me on that and/or provide further guidance. Or maybe someone else from the release team?

You could potentially put something in the pre- or post-upgrade messages. Considering the degree of changes that don't get covered in those places, I think that would probably be overkill. Basically, if every change in the UI behavior got a pre- or post-upgrade message, those messages would be unusable.

The key thing is that @alifrumin and/or I be easily able to know what to tell readers of the release notes. In a long ticket like this, it's sometimes hard to distinguish the initial proposal from what ultimately happened. For us, and for our readers who follow the link to read the details, you can't assume we have followed anything that went on. Conversations sometimes split between GitLab and a PR (sometimes a PR that gets closed), and sometimes key things are resolved on email or -most conversations, or in person at a sprint.

I'd recommend writing a summary of the outcome and put it under a new header in the issue description. If you can't do that (like if it's someone else's issue), put it in a comment at the end. This is also a good opportunity to decide whether to close the issue.

Thanks @andrewhunt. I've added a 'Summary of changes' at the bottom of the description. I hope this makes it clear but let me know if you need anything else. I'll close this issue now.

Thanks @andrewhunt! I've created an issue here to get this documented in the dev guide: https://github.com/civicrm/civicrm-dev-docs/issues/787

changed the description

closed

Don't freeze fields for auto-renew memberships

Issue

Proposal

Summary of changes

Child items 0

Activity