Membership tests and fix

[aydun]

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]

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

[artfulrobot]

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]

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]

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]

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.