Skip to content
This repository has been archived by the owner on Jun 25, 2021. It is now read-only.

Membership should be handled by CiviCRM not this extension #42

Closed
wmortada opened this issue Nov 1, 2018 · 17 comments
Closed

Membership should be handled by CiviCRM not this extension #42

wmortada opened this issue Nov 1, 2018 · 17 comments
Labels
help wanted Membership Issues to do with membership

Comments

@wmortada
Copy link
Contributor

wmortada commented Nov 1, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

wmortada commented Nov 2, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

wmortada commented Nov 2, 2018

I shall try it out and report back.

@artfulrobot artfulrobot added the Membership Issues to do with membership label Nov 8, 2018
@aydun
Copy link
Contributor

aydun commented Nov 13, 2018

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
Copy link
Contributor Author

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

@artfulrobot
Copy link
Owner

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

@aydun
Copy link
Contributor

aydun commented Nov 13, 2018

Take a look at PR #44

@artfulrobot
Copy link
Owner

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

@aydun
Copy link
Contributor

aydun commented Nov 14, 2018

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

@wmortada
Copy link
Contributor Author

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
Copy link
Contributor

aydun commented Nov 14, 2018

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
Copy link
Contributor Author

wmortada commented Nov 14, 2018

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
Copy link
Contributor

aydun commented Nov 14, 2018

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
Copy link
Contributor Author

wmortada commented Nov 15, 2018

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
Copy link
Owner

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

@wmortada
Copy link
Contributor Author

Thanks @artfulrobot

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Membership Issues to do with membership
Projects
None yet
Development

No branches or pull requests

3 participants