Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure contributionRecurID is set on processor #15673

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 31, 2019

Overview

Ensure contributionRecurID is available to payment processors that need it in cancelSubscription

Discussion starts here https://chat.civicrm.org/civicrm/pl/9m1goccmtpdy58cb7jyx791t7e

Before

Payment processor cannot access contributionRecurID

After

Payment processor can call $this->getContributionRecurID()

Technical Details

Payment processors can use this call
https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:ppal_get?expand=1#diff-96a0ea19e8ac102ef100c35b3b3d1c31R505

and then the getters will work regardless of whether the param was passed in or set

Comments

Am giving this MOP as discussed & agreed on chat with @artfulrobot

@civibot
Copy link

civibot bot commented Oct 31, 2019

(Standard links)

@civibot civibot bot added the master label Oct 31, 2019
Copy link
Contributor

@artfulrobot artfulrobot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea as we have decided to eventually get rid of cancelParams.

@eileenmcnaughton
Copy link
Contributor Author

@artfulrobot it's probably useful to add the line

$this->inputParams = $params;

liberally in you goCardless as then you can use the getters regardless of how they were set

@artfulrobot
Copy link
Contributor

Assuming this makes it into 5.20, I'm using this in my GoCardless Payment Processor:

  /**                                                                                                                                                                                       
     * Does this processor support cancelling recurring contributions through code.                                                                                                           
     *                                                                                                                                                                                        
     * GoCardless does, but only if CiviCRM version 5.20+                                                                                                                                     
     * because it depends on                                                                                                                                                                  
     * https://github.com/civicrm/civicrm-core/pull/15673                                                                                                                                     
     *                                                                                                                                                                                        
     * @return bool                                                                                                                                                                           
     */                                                                                                                                                                                       
    protected function supportsCancelRecurring() {                                                                                                                                            
      list($a, $b) = explode('.', CRM_Utils_System::version());                                                                                                                               
      return  ($a >= 5 && $b >= 20);                                                                                                                                                       
    }

@mattwire
Copy link
Contributor

@artfulrobot You should be able to use PHP version_compare - various examples in core

@mattwire mattwire merged commit dd57e69 into civicrm:master Oct 31, 2019
@artfulrobot
Copy link
Contributor

Ha! I had not realised version_compare was a php native function! I knew I'd seen something like that and was grepping away for 'function.*version'!

@eileenmcnaughton eileenmcnaughton deleted the set branch October 31, 2019 20:40
@eileenmcnaughton
Copy link
Contributor Author

@artfulrobot the alternative is to increment the goCardless version - ie. you have a working goCardless version that people can use.

Now you are adding a new feature - if people want it they can upgrade & use it - so you add a new release in info.xml & set the version to 5.20 and people on less than 5.20 can only use the old version without the new feature. People with it can get the new version.

I switched to this approach after nearly running extended reports into the ground trying to support multiple versions. I came pretty close to abandoning it but up doing a big overhaul and changing my support strategy. Now every time I work on it I increment the supports version to what I'm working on. If people want the latest then it's up to them to figure out how to make that work.Perhaps they can run the latest version on an olde Civi - but that's nothing to do with me. The version I released for their Civi version will work as well as it always has on their version

@artfulrobot
Copy link
Contributor

@eileenmcnaughton sure, I'm +1 for requiring a fairly up to date CiviCRM version. But TBH this line is for me and my clients - I'm doing the work now while they're on 5.18 or so, but want it to magically turn on when they get to 5.20. I'll need the (other) new features to work for them before they get to 5.20

@mattwire
Copy link
Contributor

mattwire commented Nov 1, 2019

It's also much easier for a client to say "ok, we'll wait six months to do that report", than to say "ok, we'll wait six months to take another payment" (because the payment processor updated their API and you need to have the latest extension in civi to support it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants