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.

Sign up for GitHub

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

Jump to bottom

Partial refactor of completeMembershipFromContribution #12271

Merged
merged 1 commit into from
Jun 8, 2018
Merged

Partial refactor of completeMembershipFromContribution #12271

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 23 additions & 29 deletions CRM/Contribute/BAO/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2441,7 +2441,7 @@ public function loadRelatedObjects(&$input, &$ids, $loadAll = FALSE) {
}
}

$this->loadRelatedMembershipObjects($ids);
$ids = $this->loadRelatedMembershipObjects($ids);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just ditching pass-by-param here


if ($this->_component != 'contribute') {
// we are in event mode
Expand Down Expand Up @@ -3846,8 +3846,7 @@ public static function recordAdditionalPayment($contributionId, $trxnsData, $pay
}

// load related memberships on basis of $contributionDAO object
$membershipIDs = array();
$contributionDAO->loadRelatedMembershipObjects($membershipIDs);
$contributionDAO->loadRelatedMembershipObjects();

// build params for recording financial trxn entry
$params['contribution'] = $contributionDAO;
Expand Down Expand Up @@ -3929,15 +3928,11 @@ public static function recordAdditionalPayment($contributionId, $trxnsData, $pay
}
}

// update membership details
if (!empty($contributionDAO->_relatedObjects['membership'])) {
self::updateMembershipBasedOnCompletionOfContribution(
$contributionDAO,
$contributionDAO->_relatedObjects['membership'],
$contributionId,
$trxnsData['trxn_date']
);
}
self::updateMembershipBasedOnCompletionOfContribution(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty much per Matt's - load relatedObjects in updateMembershipBasedOnCompletionOfContribution & do early return in there too

$contributionDAO,
$contributionId,
$trxnsData['trxn_date']
);

// update financial item statuses
$baseTrxnId = CRM_Core_BAO_FinancialTrxn::getFinancialTrxnId($contributionId);
Expand Down Expand Up @@ -4550,7 +4545,6 @@ public static function completeOrder(&$input, &$ids, $objects, $transaction, $re
}

$participant = CRM_Utils_Array::value('participant', $objects);
$memberships = CRM_Utils_Array::value('membership', $objects);
$recurContrib = CRM_Utils_Array::value('contributionRecur', $objects);
$recurringContributionID = (empty($recurContrib->id)) ? NULL : $recurContrib->id;
$event = CRM_Utils_Array::value('event', $objects);
Expand Down Expand Up @@ -4598,10 +4592,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)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

memberships is defined as

$memberships = CRM_Utils_Array::value('membership', $objects);

so that line is meaningless

Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

& actually - $memberships not used anyway

$memberships = array($objects['membership']);
}

$values = array();
if (isset($input['is_email_receipt'])) {
$values['is_email_receipt'] = $input['is_email_receipt'];
Expand Down Expand Up @@ -4629,15 +4619,12 @@ public static function completeOrder(&$input, &$ids, $objects, $transaction, $re
$values['is_email_receipt'] = $recurContrib->is_email_receipt;
}

if (!empty($memberships)) {
self::updateMembershipBasedOnCompletionOfContribution(
$contribution,
$memberships,
$primaryContributionID,
$changeDate,
CRM_Core_PseudoConstant::getLabel('CRM_Contribute_BAO_Contribution', 'contribution_status_id', CRM_Utils_Array::value('contribution_status_id', $input))
);
}
self::updateMembershipBasedOnCompletionOfContribution(
$contribution,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no memberships param

$primaryContributionID,
$changeDate,
CRM_Core_PseudoConstant::getLabel('CRM_Contribute_BAO_Contribution', 'contribution_status_id', CRM_Utils_Array::value('contribution_status_id', $input))
);
}
else {
if (empty($input['IAmAHorribleNastyBeyondExcusableHackInTheCRMEventFORMTaskClassThatNeedsToBERemoved'])) {
Expand Down Expand Up @@ -4820,9 +4807,11 @@ public static function createCreditNoteId() {
*
* @param array $ids
*
* @return array $ids
*
* @throws Exception
*/
public function loadRelatedMembershipObjects(&$ids) {
public function loadRelatedMembershipObjects($ids = []) {
$query = "
SELECT membership_id
FROM civicrm_membership_payment
Expand Down Expand Up @@ -4853,6 +4842,7 @@ public function loadRelatedMembershipObjects(&$ids) {
}
}
}
return $ids;
}

/**
Expand Down Expand Up @@ -5397,15 +5387,19 @@ protected static function isPaymentInstrumentChange(&$params, $pendingStatuses)
* load them in this function. Code clean up would compensate for any minor performance implication.
*
* @param \CRM_Contribute_BAO_Contribution $contribution
* @param array $memberships
* @param int $primaryContributionID
* @param string $changeDate
* @param string $contributionStatus
* This shouldn't be required but historical function overload by repeattransaction probably requires it.
*
* @todo investigate completely bypassing this function if $contributionStatus != Completed.
*/
protected static function updateMembershipBasedOnCompletionOfContribution($contribution, $memberships, $primaryContributionID, $changeDate, $contributionStatus = 'Completed') {
protected static function updateMembershipBasedOnCompletionOfContribution($contribution, $primaryContributionID, $changeDate, $contributionStatus = 'Completed') {
$contribution->loadRelatedMembershipObjects();
if (empty($contribution->_relatedObjects['membership'])) {
return;
}
$memberships = $contribution->_relatedObjects['membership'];
foreach ($memberships as $membershipTypeIdKey => $membership) {
if ($membership) {
$membershipParams = array(
Expand Down