Membership should be handled by CiviCRM not this extension

[wmortada]

This extension updates the membership directly using the API. I don't believe that it should do this. This extension should just handle the payments, the business logic around membership should be handled by CiviCRM.

I believe that this may be causing a number of issues with memberships:

• #27 Membership since date is getting changed
• start_date and join_date are overwritten on renewal
• end_date is extended twice on renewal (this extension extends the end date while the payment is pending, CiviCRM extends it again when the payment clears)

All of the business logic around membership is triggered when the extension makes API requests to completetransaction and repeattransaction when a webhook is received.

I spent some time looking at this extension during the UK CiviCRM sprint and was hoping to follow this up subsequently but haven't had the time yet unfortunately. If I get time, I would like to follow this up in future but in the meantime I wanted to at least record my understanding of the problem here.

[artfulrobot]

Sounds promising! I copied all that from the Stripe extension (pre Matthew's stewardship of it).

To proceed, it would be fab if you could either write failing unit tests for membership, or write instructions here for the cases that it's failing on, and what the expected results should be. Then I (or someon else) has something to work to.

If anyone can fund the fix (or submit PR), that would be fab.

[wmortada]

Thanks Rich, I hope to look into this in coming weeks. The site I'm working on has a customised version of this extension so I want to check whether the issues that we are seeing are due to this extension or to the customisations that we have.

My thinking at the moment is that we should remove Lines 325-337 of /CRM/GoCardlessUtils.php. This section basically modifies the membership to add start and end dates while the payment is still pending. I understand the rationale behind this but I think this sort of business logic should really be part of CiviCRM core. It isn't something that you would want to vary with each payment processor. For example if someone pays by another direct debit provider or by cheque you would expect the same behaviour.

As to unit tests, I'm not sure. As I see it, the tests would need check to see if CiviCRM core behaves as it should do. Would it make sense to include those tests in this extension? If changes are made to CiviCRM core it may cause these tests to fail (even if the extension hasn't changed). Is that helpful?

When I get a moment I will try to write down how I understand that the CiviCRM membership process works and what the issues are now.

[artfulrobot]

I agree that's how CiviCRM should work, I just don't know if it's how it does work :-O

And if it's how it did work in 4.7 - wouldn't want to break anything for those folks if the behaviour has changed.

But maybe it does work right without that. If you can delete that call and test with your clients who use membership, that would be great. Imagine if that closes three issues at once!

[wmortada]

I shall try it out and report back.

[aydun]

Hi @wmortada @artfulrobot Just wondering whether there has been any further progress on this? I'm putting this on a membership system for the first time and looking at the issues reported. I'd agree it looks like it should not need to directly update memberships.

[wmortada]

Hi @aydun, not made any progress with this yet I'm afraid.

[artfulrobot]

@aydun if you could test with with those lines removed and report back that would be fab.

[aydun]

Take a look at PR #44

[artfulrobot]

Do you think this will solve #27 and #28 also?

[aydun]

Yes, I think it will. There are tests to cover both issues.

[wmortada]

Thanks @aydun. I'll take a look also.

I think it is the right thing to remove that code, but this will change the behaviour of this extension in a way that some users may have come to expect. At the moment it sets start, end and join dates for membership when it is first created as pending. Standard CiviCRM behaviour is to leave those dates blank. Possibly this is something that should be changed in CiviCRM core, but that's not a question to resolve here.

In any case, we should alert users that this behaviour has changed.

[aydun]

As you say, the membership is initially created as Pending. When the first payment is confirmed via the webhook, the dates are adjusted by core so I'm not sure that will make much difference in practice. But yes, we should alert users.

[wmortada]

I think the issue that this code was trying to address was that when the membership is in a pending state it doesn't have a start and end date so you have no idea how long the membership has been pending for. This is less of an issue for card payments as they are typically completed (or cancelled) within a few minutes (though there are cases where they don't complete and are left as pending).

With GoCardless the payment isn't confirmed until 6 working days later so this is more of an issue. This means that it is more likely that there will be memberships in pending status but with no clear indication of when they start or finish. Without these dates it is difficult to tidy your membership records and get rid of pending memberships that shouldn't be there (i.e. because the payment was never completed).

This is particularly an issue if access to the website is restricted by membership status and you want to allow members to access the website immediately (before their payment has cleared). In this case you may want to allow pending members to access the website.

[aydun]

How are you creating those memberships with no dates? The ones I'm seeing are created with start and end dates but Pending - just the same as if the Contribution was Completed. The dates are updated when the Contribution changes from Pending to Completed, but it has dates at the point it is created.

[wmortada]

Ah, okay. I'm working on CiviCRM 4.6 site so it is possible that this has been added in version 4.7/5.0. I was planning to do some more testing in different versions to find out if there were differences but haven't had a chance to yet.

[artfulrobot]

Thanks @wmortada and @aydun, I'll do a new release tomorrow.

[wmortada]

Thanks @artfulrobot