search

artfulrobot / uk.artfulrobot.civicrm.gocardless

A CiviCRM extension providing GoCardless integration to handle UK Direct Debits.
GNU Affero General Public License v3.0
5 stars 18 forks source link

Membership tests and fix #44

Closed aydun closed 5 years ago

aydun commented 5 years ago

I've added some tests for memberships in the first commit which shows a problem. The second commit fixes that.

For background, see analysis by @wmortada in #42

artfulrobot commented 5 years ago

Looks great, will have proper look on Thursday. Thanks so much.

artfulrobot commented 5 years ago

Hi @aydun

Thanks for all the test code, the extension really needed a membership expert to write that stuff as I'm not familiar with the intricacies of it.

I confirm all the tests are passing for me. One question about the tests: you've added membership code to the existing testTransferCheckoutCompletesWithoutInstallments test, instead of having a separate test. At first I thought "Ah, but we still need to test the non-membership case", so I looked into it.

The code I was remembering is at https://github.com/artfulrobot/uk.artfulrobot.civicrm.gocardless/blob/master/CRM/GoCardlessUtils.php#L225

Which has branches for membership. However, if I understand correctly, I don't think the membership elseif() of that code will ever be run, since there will always be a ContributionRecur record (since we don't do one-offs - yet).

So actually, that code (my code there) is pretty useless (until the extension does do one offs) and therfore it makes no difference adding to the existing test; so I think it's fine. Do you agree?

Also, I'm considering publishing this as 'stable' now these membership things have been fixed because it's been in use for years and the membership was the bit I was worried about. Thoughts on that anyone?

aydun commented 5 years ago

I wouldn't call myself a membership expert, but I know a bit about it!

As you say, I added membership tests to testTransferCheckoutCompletesWithoutInstallments. I started with a separate test and then found I was repeating most of what was there and just adding tests. I don't think it alters the validity of the existing tests. However, on reflection maybe we should keep the original as well so that we are also explicitly testing a non-membership scenario.

As regards the code you highlighted ... I couldn't see how that membership elseif() would get used since we're only dealing with recurring situations, but I assumed it was there for a reason! I think we can just remove the whole elseif() block at https://github.com/artfulrobot/uk.artfulrobot.civicrm.gocardless/blob/master/CRM/GoCardlessUtils.php#L237-L244

So, I'd suggest we remove that code and reshuffle the tests so that we explicitly test membership and non-membership scenarios. If you agree I'll push another commit. A 'stable' release would be good - perhaps we can get @wmortada and @upperholme to confirm their issues are resolved first?

artfulrobot commented 5 years ago

I'd be tempted to comment the code out with a note, rather than remove it, just in case issue #12 ever gets implemented. V happy for you to do that.

Without that membership-specific thing in place I'm not so bothered about having a separate test for non memberships, since the membership one is identical but plus membership. So the membership test would still fail if the normal case was failling. And if there is a difference in future we can split the tests up then? Over to you.

Many thanks, and great work @wmortada for grok'ing the code to realise that one API call was causing all the problems!

aydun commented 5 years ago

Ok, I've reinstated the original test so we explicitly test both membership and non-membership. I've squashed the commits back to 2 so I think this ready to merge.