Cancel first contribution associated to membership, cancels the membership

changed the description

added ~81 ~88 needs:concept approval triaged labels

Thanks @sluc23 for opening the discussion on this topic. I know that @KarinG has a nice workflow on setting the membership (status, end date etc...) when a contribution is recorded. I agree with @sluc23 that the membership process sometimes feels a bit sluggy with contributions.

@KarinG what do you think?

mentioned in issue #928 (closed)

I've just created and linked #928 (closed) which links to some work I was doing in this area. Your issue isn't directly referenced there I don't think but it's all the same code area and the same type of issues :-)

@jaapjansma @sluc23 - when we run into issues where CiviCRM Membership out of the box does not quite work for an org - we write an extension that essentially examines Contributions (post hook; scheduled job) and contains org specific logic (based on contribution status, amount, financial type) -> it then calculates the membership (extend existing one or create new one, set status, calculate, retype, new expiry date). Key advantage it's your rules and the Contributions will always be matching the Membership (and vice versa). They are disconnected so you can't accidentally delete one or the other. So we're populating CiviCRM Membership - but these orgs never Create New Membership or sell Memberships. They sell Contributions/Recurring Contributions/Credit Card/ACHEFT/upload offline ones and some of them magically Create/Extend/Update a Membership.

Conclusion: for your scenario someone else will say it should cancel the Membership and works as is - [for one of our clients a failed ACHEFT/due to NSF indeed does not create a new Membership according to their rules]. CiviCRM can't contain all possible rules that may or may not apply - Membership is complicated and every org has very different rules (I've got one where Membership expiry date is +1y post last successful contribution of certain min $amount and certain financial type). Best to handle that ourselves.

thanks @jaapjansma, @mattwire @KarinG for your feedback.
I agree that Membership/Contribution area is complex and the alternatives of how different ORGs use them can be huge.

Just to keep the scope of this issue as minimum, would you support a new setting in CiviMember to enable/disable the cancellation of memberships when the first contribution is cancelled? We can have this setting by default as YES, to keep current behavior of CiviCRM

I don't want to change current behavior.. just to be able to disable it from UI. So far, our solution the past years has been to patch this part of the core as I described in the SE post above

Pinging @jamienovick1 - Compucorp has been busy with memberships recently ...

There is an alterCalculatedMembershipStatus hook, it seems like it would make sense to trigger that when CRM_Contribute_BAO_Contribution::transitionComponents() performs the status change, no?

With that in place, an extension could easily change that behaviour without having to resort to weird post hook workarounds where the already-changed membership status is reverted to what it was before.

Just for reference, the hook is documented: https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_alterCalculatedMembershipStatus/

This is an area where different workflows for different orgs make sense. FYI our general approach these days is to move to extensions to add new or override existing core workflows. Adding more core settings to enable/disable functionality is something @eileen tries to prevent from being merged.

@sluc23 The problem with adding settings is that we'd end up needing lot's of settings as there are many different requirements. The problem with the existing rules is that they are undocumented, sometimes broken, do not work for many organisations. My intention with #928 (closed) was to try and fix what was broken, document what it does currently, simplify what core does to the minimum and make it easy to override via extensions / hooks.

I would certainly support adding support for hook_civicrm_alterCalculatedMembershipStatus() (or an alternative hook) if there is a sensible place for this in core. As currently it's not possible (I don't think) to customise the rules around membership renewals which are linked to recurring contributions - and this is a common area for bugs to occur. Ideally we want to say: "Use core implementation for calculating membership renewal" OR "Use our extension to calculate membership renewal".

Hey all,

In the membership extras extension: https://civicrm.org/blog/jamienovick/membership-extras%E2%80%A6-leaping-civicrm%E2%80%99s-membership-module-in-2019 I believe we have done some work on this so that memberships are not cancelled when a contribution is cancelled as we have this exact scenario with instalments and renewals for offline and direct debit processes.

I'll get one of the devs who worked on it to give some more details about the solution.

I think there is a secondary issue for direct debit with contribution status's where there isn't really a good "status" to put a contribution in when it fails so that it can be "retried". We avoided adding a new contribution status as I wasn't sure of the durability of such a solution, and instead put the contribution to cancelled status, which doesn't feel quite right to me - but I believe the cancelled status allows you to move it back to completed which felt not quite right to me, but seems to work ok (I wasn't so close to the details there). Probably could be done in a better way.

Best

J

@Miya27 - ^ see above - not sure if you can add a few details about how we solved this for membership extras?

@jamienovick1, I don't think we've tackled that specific scenario to prevent a membership being cancelled when its first contribution is cancelled, but we do have options in place that can help manage different use cases. For example, once the contribution is cancelled (and thus the membership), you can activate the membership's period manually, if it is so required and create a new contribution to record the payment if need be.

Another option would be to create a new membership. In this case, we have logic in place so that if a contact already has a membership of that type, that membership is used and a new period is added, instead of creating a completely new membership. This results in a cleaner membership history for the contact, as instead of having multiple records for the same membership type, there would only be one record, and within it, several periods, active if paid for, inactive if not.

thanks all for your feedback. Looks like in this case that having this default behavior of forcing membership cancellation on first contribution cancel will be hard to change.

Generally speaking I agree with the leap on extension approach now a days, put new features on extensions, minimum changes in core, but in this case this new feature is hard to imagine in an extension. It is basically try to avoid/rollback what CiviCRM is doing by default.. adding hooks to re-activate a membership that we don't want to be automatic cancelled, or create a duplicated membership with some non direct logic to have the member active.. just seems odd, like an unnecessary overhead processes.. and it's the type of scenarios where the balance of patching the core vs developing a new extension, goes in favor of the patch in my pov

@sluc23 Will you be attending the sprint in Amsterdam? We could work through solutions there?

I think we can agree that a patch to core would be accepted if we can find a way that allows the existing functionality to continue working by default. So a hook that intercepts core at the right moment (before the membership is cancelled/updated/whatever) and allows an extension to replace what core does.

yes I'll be in Amsterdam.. definitely a good topic to work together

@eileen could you comment on what refactoring might be necessary prior to allowing a hook to be added as outlined aboveby @mattwire

Folks the trouble is that we neither want to further complicate this code nor via a hook prevent it frim being refactored. I think so long as the hook has an interface that is high enough level that it could allow needed refactoring it could make sense to put it in. But Eileen usually has more detailed and nuanced views and is harder line.

SO my general practice whenever I look at code - either because I'm digging into solving a problem of my own or trying to input on a review / someone else's - is to do a minor refactor at that point. I generally find that this is pretty quick and easy & if I am careful to do a minimal amount of change (extract one function cleanly, cleanup one variable etc) then they get reviewed fairly quickly because our review process is 'cleanup-biased'. Following this approach I generally find that by the time I sit down to really work on the problem it's a lot easier - sometimes to the extent that the problem got fix along the way. I guess this is what I mean when I tell people it's usually easier to 'chip away at things'

In this case I have now put up a PR to extract the cancel function - it has some comments on it but basically I think the use of an https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_apiWrappers/ hook on the cleaned up function might be simple and work well (& I'd assume someone would add unit tests to core using the hook in that way to achieve the desired result).

There are some further comments on the PR - I would ALSO take very much the same approach for 'cancel' aiming to wind up with a Contribution.cancel call along with a Contribution.fail call & to have any places in the code that are using transitioncomponents to do this call those functions directly.

@eileen which PRs are you referring in your last comment?

mentioned in commit 7c919b71

mentioned in commit 1ee67fa2

How odd - I could have sworn I added a link https://github.com/civicrm/civicrm-core/pull/14197

I've been listening/inquiring/commenting a bit on this issue at the sprint and read this branch/draft based around Civi::service('civicrm_rules_financial')->supportsEditPaymentLinkedToPaymentProcessor(). I don't have detailed understanding of the subsystem/use-cases, but there was some verbal discussion about taking this as a pattern to extend/emulate, so I wanted to feedback a bit more on general architectural grounds.

In verbal conversation, it sounded like there was an earlier notion to expose a new setting (e.g. Civi::settings()->get('edit_payment_linked_to_payment_processor_foo_bar_whiz_bang')). This was abandoned because folks wanted to avoid new settings on some general principle. I can relate to that, but it seems a bit misapplied. To my mind, the goal of "avoid new settings" is really one variation of the general imperative to "minimize complexity."

It sounded like the goal is to allow more variations in behavior: one deployment might have one behavior grounded in one opinion/circumstance while a second deployment has second behavior grounded in a different option/circumstance. Any approach which addresses does that will increase the complexity. This represents essential complexity.

There are various ways that one might direct this new complexity, e.g. with a

PHP define() or global variable
Civi setting
Hook
Service
Database column or table
File or file-scanner

Each has some trade-offs. IMHO, there are basically two reasons to choose one or another:

One option is intrinsically closest. For example, if the toggle is fundamentally a boolean value that doesn't change much, then a setting is probably closest.
Predictions about the future. Maybe one option isn't best today - but you've got a good reason to guess something different will be better tomorrow. For example, if you predict that there will be multiple deeply-entwined functions or variables (which are usually changed in batch), then a service is probably better.

There were a few claims that came up in discussion which I think were misplaced:

"A setting would add more complexity (than a service)." => Disagree. One must accept the essential complexity. Pick the model which most closely matches the problem. (If the closest model is a system-wide boolean, then use a setting. If the closest model is a set of mutually coupled functions, then use a service.)
"It's harder to test a setting (than a service)." => Disagree. Civi::service('civicrm_rules_financial')->supportsEditPaymentLinkedToPaymentProcessor() is no easier to unit-test than Civi::settings()->get('edit_payment_linked_to_payment_processor_foo_bar_whiz_bang'). Either way, a substantive test would need to do some mocking and check that the contract is met for each variation of the option.
"A setting implies that it's a supported contract and will increase the maintenance." => Disagree. All the techniques should equally imply support+maintenance. There are plenty of defines/hooks/settings/services/file-names for which a change would be a problem downstream. If it's important that we retain some discretion to break/change this thing (e.g. because it's too hard to validate the design upfront), then the right move is to clearly flag the specific thing (setting/hook/whatever) as provisional/experimental/unstable. The support/maintenance issue is more about communications than about the specific technical artifacts.

@totten did you add this to the right ticket?

FWIW on the actual topic I think if it's about changing a form behaviour we should make it doable via a form hook - e.g validate, although it feels more like a permission

Also flagging https://github.com/civicrm/civicrm-core/pull/14137 as another example of logic hardcoded into core:

currently when you record a refund payment (of any amount) with a participant id ALL related participant records are set to 'Registered' - this seems kinda odd but we could probably transfer into the payment.create BAO & retain for now.

changed the description

Some related cleanup here: https://github.com/civicrm/civicrm-core/pull/14397

mentioned in commit 95a34163

Just flagging that the code that was added for https://issues.civicrm.org/jira/browse/CRM-18177 seemed to only patch it in the CRM_Contribute_BAO_Contribution::transitionComponents function but not in the IPN end so we probably need to determine if we are happy with that logic and add to the IPN or remove from transitionComponents, I would be in favour of standardising and incorporating CRM-18177's logic into the IPN ends

And ideally we refactor so that both call the same function, rather than duplicating the logic.

mentioned in issue #161 (closed)

Just revisiting this - I don't think anyone has made a case that they WANT memberships to be cancelled when the contribution is cancelled, even pending ones. We already removed that behaviour from at least one place in the code https://issues.civicrm.org/jira/browse/CRM-18177

So it seems to me that NOT cancelling is

a) more consistent with our general assumptions about the relationship between orders & payments and

b) preferred by the people in this thread.

HOWEVER, we are worried that we don't speak for everyone and someone out there might like the existing behaviour.

If we had built it to not cancel in the first place then a pre hook to take extra actions on cancel would have felt like a natural fit for an extension. However, we put the not-always-good behaviour into core & turning it OFF in an extension is hard.

My guess is the core behaviour is actually the edge case rather than the norm & we should flip it ie

switch core to not alter memberships on cancel normally
add an optional core extension with a pre hook to do membership cancels on related contribution cancels (if in pending mode) - allow people to install on upgrade.

If we did this would could simplify the core code considerably as well as make it more consistent.

It's also worth noting that where the cancel is done by a backoffice user they generally know the outcome they are after - we could consider adding a 'cancel contribution form' as a contribution action (just for pending contributions in the first instance) that displays what entities are linked and has a checkbox as to whether to cancel them or not.

Thanks for returning to this @eileen. I agree with your thoughts and feel it is more important to choose one of the options you outline than to get caught up on which is best and lose focus and momentum again. So my vote is to go ahead with 1 and 2 but mainly to move ahead.

Ah, this is where I saw the note about a core extension for membership stuff. I'm in favour of that but I would like to see a more generic "membership" extension that eventually handles all the core logic and could more easily be overridden/replaced depending on the organisation need. By all means start with a single function (ie. the membership cancel) but let's build it in a way that allows us to move more membership logic there over time.

For the actual cancel action I envisage a settings UI to manage the actual actions. Eg. When cancelling a related contribution:

Set Membership status to: XX (dropdown).
Update membership end date to cancellation date: Yes/No.

This would both make it clear what will happen to a membership and allow it to be configurable for organisations that require it to do something different. An organisation could for example choose to implement a custom membership status and automatically update the membership to that custom status without having to write any code, they'd just need to change the settings to match.

I also think that a setting would be preferable and how Matt suggests it could work above makes a lot of sense to me. The only iteration I would consider is to check the behavior for "failed" contributions also.

It is possible that a contact could attempt to pay for a pending contribution for a membership online via a contribution page and have the transaction fail. In that case we again wouldn't want the membership to be cancelled (or the contribution to be marked as failed in fact - the "invoice" still stands). I'm not sure what the behavior is currently for that but just a thought.

Just to mention I think Tim has also commented #927 (comment 17490) that he is broadly in agreement with settings where they make logical sense to reduce code complexity?

Thanks for looking into this Eileen, I think it would allow us to remove this patch from Membership Extras:

https://github.com/compucorp/uk.co.compucorp.membershipextras#patch-1-prevent-cancelled-installments-from-cancelling-membership

and was the reason for: #161 (closed)

cc: @omar_compucorp @Miya27 @ana_compucorp @erawat

Best

Regarding a setting & code complexity - currently the code complexity comes from us always doing this cancel and then trying to sometimes not to. Reversing this to being don't do it unless it's a good thing cleans up the code complexity as a pre-hook is a pretty clean way to do that. I don't see a setting reducing code complexity.

@mattwire @jamienovick1 so my underlying assumption is that the edge case is where people do want to cancel the membership when the contribution is cancelled and so we should move that to a (core) extension or let people use civirules which seems to be a logical way to meet the extended functionality @mattwire suggested above - and the 'normal' case would be that cancelling a contribution would leave the membership unaltered - this is certainly how our new philosophy about the meaning of contributions would suggest it should be,.

Do you find that the existing behaviour IS popular with a number of your clients?

@mattwire I've opened #2116 (closed) as an issue for extensionising civimember in general - the scope of this issue is really whether we can simplify the handling of cancels such that the code is nicer and it does not impose an unpopular work flow on everyone (but we still find a way to offer it).

Depending on the agreed solution I'm prepared to work on that this month.

Just thinking more about this - to achieve the goal of cleaning up the code we would need to move ALL the cancel actions to an extension. This PR is pseudocode - not real code but it points out where we would remove code in order to move it to the extension post hook https://github.com/civicrm/civicrm-core/pull/18741/

Obviously it would take a lot of work to make that a reality but it would get us to the point where

our ability to fix the code is no longer blocked by the state of the cancel / failed code
all code paths that cancel / fail contributions do the same thing
extension writers have to contort themselves to opt out
we still have a way for sites to continue with the existing code but we can start to think about ways to nudge them onto better code - which might be civirules or membership extras for support for a range of workflows.

I realise there is a pretty good chance this is on the cusp of landing back in the too hard basket so I wanted to at least record what a specced out version of this approach is - mostly defined by tests that would test cancels & fails from

Paypal IPN
Contribute form
Order.cancel
?? is there another path?

Along with documentation updates, upgrade messaging

@eileen That sounds sensible to me. I want to be sure that eventually we don't end up with loads of extensions just containing little bits of logic as that will become unmanageable. But once we've extracted to an extension it will be much easier to refactor/merge/split extensions so I don't see that as a blocker.

All this "business logic" has sat in the "too hard" basket for a while but it really does need to be extracted as it will really make CiviCRM much more reliable and easier to understand/use/modify for different requirements.

I like the idea for now of extracting all the cancel/fail actions into the core extension. If you are happy to work on it I'm happy to support and I'm sure that others will be too eg. @sluc23 ?

@mattwire OK - I guess the "loads of extensions just containing little bits of logic as that will become unmanageable" is really a line we are going to need to figure out over time. I don't know where it lies - drupal obviously has extensions for pretty much every field type but Wordpress not so much.

This is the shell tidied up - https://github.com/civicrm/civicrm-core/pull/18784 but it does nothing as yet

mentioned in commit c00edb21

mentioned in commit fd377e02

mentioned in commit 47de3d1b

mentioned in commit cfa6478a

mentioned in commit eb1e4326

mentioned in commit 39610e8b

mentioned in commit 1ba7b504

mentioned in commit 3abfea54

mentioned in commit 6ff69e9c

Cancel first contribution associated to membership, cancels the membership

Child items ...

Activity