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

hook to amend params or override completeRedirectFlowWithGoCardless() #108

Closed deepak-srivastava closed 3 years ago

deepak-srivastava commented 3 years ago

We working on a requirement to modify subscriptions in a way that for yearly memberships create direct debit with payment taken now and then next member payment taken Jan following year.

The solution we implementing is not very generic at this point, that we can submit to this extension, and providing a hook helps to keep the work clean and separate in another extension.

The pull request adds a hook around completeRedirectFlowWithGoCardless() which helps modify params or override the method, allowing further to modify the subscription schedule for specific cases.

artfulrobot commented 3 years ago

This looks great. Some comments / suggestions

  1. Feel free to drop $deets in favour of $details - I'm terrible for including colloquialisms in code :-)
  2. I wonder whether we could do without callMail and instead go on: if ! $result then completeRedirectFlowWithGoCardless? Or do you thin the explicit parameter is important for readability?
  3. Q: is it worth adding a test for?
  4. Is CRM_GoCardless_Hook a standard pattern for keeping all hooks in one place? I'm not opposed and I can see it makes sense and I like the docblock, so just a question.

I'm thinking of releasing v10.0.2 soon, do you think we should include this?

deepak-srivastava commented 3 years ago

1 and 2 are done.

  1. It doesn't change the functionality, so don't think it's worth. Plus I'm not sure of a quick way to test hook implementation, nor i know if any hook implementation test examples from core civi. In any case I would need some time to come back on it.
  2. Think so, I've noticed similar standard patterns in other extensions. And pattern is clear enough to suggest it's for gocardless related hooks. If you have any other ideas for the pattern, let me know.

Thanks

artfulrobot commented 3 years ago

@deepak-srivastava Thanks, this looks good to merge. I'll get it in 1.10.2

See https://docs.civicrm.org/dev/en/latest/testing/phpunit/#hookinterface

I've added a test at https://github.com/artfulrobot/uk.artfulrobot.civicrm.gocardless/pull/109

deepak-srivastava commented 3 years ago

@artfulrobot #109 looks good to me. Thanks.

artfulrobot commented 3 years ago

Great, thanks very much for your contribution, I'll merge it!