Partial refactor of completeMembershipFromContribution #12271
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eileenmcnaughton
commented
Jun 6, 2018
| @@ -2441,7 +2441,7 @@ public function loadRelatedObjects(&$input, &$ids, $loadAll = FALSE) { | |||
| } | |||
| } | |||
|
|
|||
| $this->loadRelatedMembershipObjects($ids); | |||
| $ids = $this->loadRelatedMembershipObjects($ids); | |||
There was a problem hiding this comment.
just ditching pass-by-param here
eileenmcnaughton
commented
Jun 6, 2018
| $trxnsData['trxn_date'] | ||
| ); | ||
| } | ||
| self::updateMembershipBasedOnCompletionOfContribution( |
There was a problem hiding this comment.
pretty much per Matt's - load relatedObjects in updateMembershipBasedOnCompletionOfContribution & do early return in there too
eileenmcnaughton
commented
Jun 6, 2018
| @@ -4598,10 +4594,6 @@ public static function completeOrder(&$input, &$ids, $objects, $transaction, $re | |||
| self::repeatTransaction($contribution, $input, $contributionParams, $paymentProcessorId); | |||
| $contributionParams['financial_type_id'] = $contribution->financial_type_id; | |||
|
|
|||
| if (is_numeric($memberships)) { | |||
There was a problem hiding this comment.
memberships is defined as
$memberships = CRM_Utils_Array::value('membership', $objects);
so that line is meaningless
There was a problem hiding this comment.
& actually - $memberships not used anyway
eileenmcnaughton
commented
Jun 6, 2018
| ); | ||
| } | ||
| self::updateMembershipBasedOnCompletionOfContribution( | ||
| $contribution, |
There was a problem hiding this comment.
no memberships param
eileenmcnaughton
force-pushed
the
membership
branch
from
0350a5e
to
46400cc
Compare
eileenmcnaughton
force-pushed
the
membership
branch
from
46400cc
to
bbb16fe
Compare
eileenmcnaughton
force-pushed
the
membership
branch
from
bbb16fe
to
e6c7e48
Compare
omarabuhussein
approved these changes
Jun 8, 2018
|
this is good to merge from my side @eileenmcnaughton |
|
OK - let's do this & then next round can have some tests! Merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This is a partial of #11556 - it contains part of that code tidy up
Before
Code less maintainable
After
Code more maintainable
Technical Details
@omarabuhussein I took a look at #11556 as we were discussing and this is about the size chunk I feel comfortable reviewing / merging .... and which I feel comfortable makes sense after reviewing the code myself
It moves the loading of related membership objects to the function that @mattwire has reworked - without actually changing it to an array as Matt does or changing the processing. I took a look at the paths by which updateMembershipBasedOnCompletionOfContribution is reached and I feel pretty comfortable it is equivalent to the result of $contributionDAO->loadRelatedMembershipObjects(); in all cases - that is the main change in here. I'm not too worried about the performance impact of potentially loading that twice. I don't think it's much in the scheme of things. If you feel comfortable with this portion we can merge it & get @mattwire to rebase the larger PR
Comments