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

Extract cancelParticipant and cancelMembership functions in baseIPN #15134

Merged

Conversation

mattwire
Copy link
Contributor

Overview

This extracts the "business logic" ie. cancelParticipant() and cancelMembership() in baseIPN so we have a single functions that handles these functions instead of being duplicated across two functions cancelled() and failed().
Partial from #14397

Before

Code duplicated in two places.

After

Code only in one place.

Technical Details

This is a straight extraction, no functional changes should have been made.

Comments

@eileenmcnaughton @sluc23

@civibot
Copy link

civibot bot commented Aug 25, 2019

(Standard links)

@civibot civibot bot added the master label Aug 25, 2019
@mattwire mattwire force-pushed the 927_extractparticipant_membership branch 2 times, most recently from 067219e to 20ea135 Compare August 25, 2019 10:32
@eileenmcnaughton
Copy link
Contributor

@mattwire this seems basically OK to me - I'm just wondering why you made the 2 new functions static (I like that they are private).

I'm Ok with this but we should add tests if we make any further changes / cleanup here. I'd also like to deprecate the failed & cancelled functions - we might be stuck with some behaviours for paypal but we should establish our preferred behaviour as an api & encourage other processors to use that.

@mattwire mattwire force-pushed the 927_extractparticipant_membership branch from 20ea135 to 56c7d91 Compare August 25, 2019 14:22
@mattwire
Copy link
Contributor Author

@eileenmcnaughton They don't need to be static - thanks for spotting - I've changed.

I don't think I'll be making further changes to the baseIPN class and agree that most stuff should be deprecated. Once this is merged the cancelParticipant() and cancelMembership() functions provide a single point that could be overridden in some way to change that logic per dev/core/issues#927

@eileenmcnaughton
Copy link
Contributor

unrelated fail

@eileenmcnaughton eileenmcnaughton merged commit e590450 into civicrm:master Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants