Support fixed day of month

[mattwire]

This adds support for specifying a fixed day for the direct debit per #93 in the case of Monthly/Yearly direct debits.

It does not support specifying the month - that could be added in the future.

[artfulrobot]

@mattwire I've taken an initial look, but not tested. This is a massive PR! And surely most of it should be in core? I'm a bit worried about maintainability.

Do we need a whole Api entity and action just to provide a static list of days of the month?! Like it's these four lines:

$dates = [['key' => 0, 'label' => 'any day'];
$d = new Datetime('1 Jan 2020');
while ($d->format('j') < 29) { $dates[] = ['key' => $d->format('j'), 'label' => $d->format('jS')]; $d->modify('+1 day'); }
$dates[] = ['key' => -1, 'label' => 'last day'];

Or, even just hard code it (not going to change!) / generate with JS?

I may have misunderstood.

Though I could add to it e.g. to replace/wrap CRM_GoCardlessUtils::getSettings() and save having to maintain defaults in two places (as per note).

Another thought: it annoys many clients that the first GC DD comes in 6+ days after it's set up. Allowing people to choose a date means that if that's too soon then it's a long wait. e.g. set up a DD on the 1st and choose the 1st for a membership. Now you have to wait a month to get your membership. I realise people can choose not to use this feature, but I bet it confuses people about things like membership starts.

Hmmm! Sounded so simple from the title!

[mattwire]

Some of this PR came across from Stripe/Smartdebit which both allow specifying a specific "start_date". Technically gocardless does too but the API also supports specifying a "day of month" which seems simpler. It would be nice to have some of this framework in core but I don't see that happening any time soon..

There are some complexities in this PR which could be simplified.

As you've mentioned there is an entire API4 entity just to get the list of possible days for the settings page - angularjs is not my strongest area so could not see another way, but I guess we could just hardcode directly on the js side and simplify things a bit. We still need the PHP functions to format the options that are displayed to the user when the contribution page is loaded though.

On the js side I copied a couple of bits over from CRM.payment (mjwshared) library and added the trigger for ajaxcomplete - did you test the gocardless forceRecurring on contribution pages when there are multiple processors and gocardless is not default?

[artfulrobot]

I can help with angular/js.

Day of month and start date two separate features. I've used fixed start date before with upgrades from another DD bureau. I agree that day of month is easier to implement and probably the more generally useful case.

I did test forcerecurring with multiple processors and I'm pretty sure without setting default but if have to re check. Apart from Selenium (which I find extortionately expensive to maintain) I don't know if a way automate tests for the different form possibilities.

[artfulrobot]

How's this?

const m={1:'st', 2:'nd', 3:'rd', 21:'st', 22:'nd', 23:'rd'};
const opts = [{key: '0', value: 'any day (earliest possible)'}];
for (var i=1;i<29;i++) opts.push({key: i.toString(), value: m[i] || (i + 'th')});
opts.push({key: '-1', value: 'last day of month'});

[mattwire]

Hi @artfulrobot I updated and simplified with your suggestion.

[artfulrobot]

@mattwire thanks I'll try to check it out soon

[artfulrobot]

Thanks @mattwire and sorry it's taken me so long to review this.

• Request: reviewing the diff to the js file was hard because there's many minor formatting changes, e.g. your IDE seems to prefer function () instead of function(). I don't know why these seemed to upset diff's algorithms, but they did. If there's a way to minimise those then that might make it easier. If not, no worries.
• The JS doesn't run unless the user has opted to force recurring when GC is selected. See https://github.com/artfulrobot/uk.artfulrobot.civicrm.gocardless/blob/master/CRM/GoCardlessUtils.php#L477
• Strange placement of the drop-down select for the day of month, is that deliberate?
  
• I get test failures running your branch. There are no failures when I run the master branch.

..FFFFF.................F...........                              36 / 36 (100%)
                                                                                                         
Time: 33.05 seconds, Memory: 64.50MB                                                                     
                                                    
There were 6 failures:                                                                                                                                                                                             
                                                                                                         
1) GoCardlessTest::testTransferCheckoutCompletesWithoutInstallments                                      
Failed asserting that '4' matches expected 5.                                                            
                                                                                                                                                                                                                   
/buildkit/build/dmaster/web/sites/default/files/civicrm/ext/uk.artfulrobot.civicrm.gocardless/tests/phpunit/GoCardlessTest.php:287
/buildkit/.local/bin/phpunit6:570                                                                        
                                                                                                                                                                                                                   
2) GoCardlessTest::testTransferCheckoutCompletesWithoutInstallmentsNewMembership
Failed asserting that '4' matches expected 5.                                                            
                                                                                                                                                                                                                   
/buildkit/build/dmaster/web/sites/default/files/civicrm/ext/uk.artfulrobot.civicrm.gocardless/tests/phpunit/GoCardlessTest.php:388
/buildkit/.local/bin/phpunit6:570                                                                        
                                                                                                         
3) GoCardlessTest::testTransferCheckoutCompletesWithoutInstallmentsExistingCurrentMembership                                                                                                                       
Failed asserting that '6' matches expected 2.                                                            
                                                                                                         
/buildkit/build/dmaster/web/sites/default/files/civicrm/ext/uk.artfulrobot.civicrm.gocardless/tests/phpunit/GoCardlessTest.php:492
/buildkit/.local/bin/phpunit6:570
                                                                                                                                                                                                                   
4) GoCardlessTest::testTransferCheckoutCompletesWithoutInstallmentsExistingGraceMembership
Failed asserting that '6' matches expected 3.                                                                                                                                                                      
                                                    
/buildkit/build/dmaster/web/sites/default/files/civicrm/ext/uk.artfulrobot.civicrm.gocardless/tests/phpunit/GoCardlessTest.php:586
/buildkit/.local/bin/phpunit6:570
                                                                                                                                                                                                                   
5) GoCardlessTest::testTransferCheckoutCompletesWithInstallments
Some predictions failed:                                                                                                                                                                                           
Double\GoCardlessPro\Services\SubscriptionsService\P17:
  No calls have been made that match:
      Double\GoCardlessPro\Services\SubscriptionsService\P17->create(exact(["params" => ["amount" => 100, "currency" => "GBP", "interval" => 1, "name" => "test contribution", "interval_unit" => "monthly", "links
" => ["mandate" => "MANDATEID"], "metadata" => ["civicrm" => "{"contactID":34,"contributionRecurID":30}"], "count" => 7]]))
    but expected at least one.
                                                    
/buildkit/.local/bin/phpunit6:570

6) GoCardlessTest::testIssue59
Failed asserting that '4' matches expected 5.

/buildkit/build/dmaster/web/sites/default/files/civicrm/ext/uk.artfulrobot.civicrm.gocardless/tests/phpunit/GoCardlessTest.php:1891
/buildkit/.local/bin/phpunit6:570

FAILURES!
Tests: 36, Assertions: 139, Failures: 6.

[mattwire]

Thanks @artfulrobot I've updated based on suggestions. Will respond to comments / check test failures :-)

[artfulrobot]

@mattwire great. If we can get tests passing I can get this into 1.10.2

[artfulrobot]

@mattwire dunno what changed but tests passing for me now.

I see:

This only works if "Force Recurring" is enabled and "Monthly" or "Yearly" is selected.

Fine by me.

Only outstanding issue is placement of the date. Do you think the bottom of .crm-section.is_recur-section might be a more appropriate place?

FYI: I want to release 1.10.2 very soon as I have some people waiting on some of it, be good to include this. Might even make it 1.11.0 instead since this is a new feature ;-) Also, I think I'm going to shift over to lab after this release as all bug issues are now closed.

[mattwire]

@artfulrobot You were right. On Stripe it is where you thought the placement should be - I've just pushed an update that fixes that here too.