From 43c8d1ddfabab16895a28e442ead720e00d017d3 Mon Sep 17 00:00:00 2001 From: eileen Date: Sun, 5 Jun 2016 00:36:11 -0600 Subject: [PATCH] CRM-18661 Fix & add test for financial rows created by repeattransaction --- CRM/Contribute/BAO/Contribution.php | 60 ++++++------- CRM/Contribute/BAO/ContributionRecur.php | 98 +++++++++++++-------- CRM/Member/BAO/MembershipPayment.php | 8 +- CRM/Price/BAO/LineItem.php | 11 ++- tests/phpunit/CiviTest/CiviUnitTestCase.php | 1 + tests/phpunit/api/v3/ContributionTest.php | 7 +- 6 files changed, 109 insertions(+), 76 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 0508a7116d..3468a8370e 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -2070,10 +2070,12 @@ LEFT JOIN civicrm_contribution contribution ON ( componentPayment.contribution_ * @param CRM_Contribute_BAO_Contribution $contribution * @param array $input * @param array $contributionParams + * @param int $paymentProcessorID * * @return array + * @throws CiviCRM_API3_Exception */ - protected static function repeatTransaction(&$contribution, &$input, $contributionParams) { + protected static function repeatTransaction(&$contribution, &$input, $contributionParams, $paymentProcessorID) { if (!empty($contribution->id)) { return FALSE; } @@ -2082,7 +2084,8 @@ LEFT JOIN civicrm_contribution contribution ON ( componentPayment.contribution_ if (!empty($input['amount'])) { $contribution->total_amount = $contributionParams['total_amount'] = $input['amount']; } - $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution($contributionParams['contribution_recur_id']); + $contributionParams['payment_processor'] = $input['payment_processor'] = $paymentProcessorID; + if (!empty($contributionParams['contribution_recur_id'])) { $recurringContribution = civicrm_api3('ContributionRecur', 'getsingle', array( 'id' => $contributionParams['contribution_recur_id'], @@ -2096,7 +2099,12 @@ LEFT JOIN civicrm_contribution contribution ON ( componentPayment.contribution_ $contributionParams['financial_type_id'] = $recurringContribution['financial_type_id']; } } - $contributionParams['skipLineItem'] = TRUE; + $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution( + $contributionParams['contribution_recur_id'], + array_intersect_key($contributionParams, array('total_amount' => TRUE, 'financial_type_id' => TRUE)) + ); + $input['line_item'] = $contributionParams['line_item'] = $templateContribution['line_item']; + $contributionParams['status_id'] = 'Pending'; if (isset($contributionParams['financial_type_id'])) { // Give precedence to passed in type. @@ -2107,9 +2115,9 @@ LEFT JOIN civicrm_contribution contribution ON ( componentPayment.contribution_ } $contributionParams['contact_id'] = $templateContribution['contact_id']; $contributionParams['source'] = empty($templateContribution['source']) ? ts('Recurring contribution') : $templateContribution['source']; + $createContribution = civicrm_api3('Contribution', 'create', $contributionParams); $contribution->id = $createContribution['id']; - $input['line_item'] = CRM_Contribute_BAO_ContributionRecur::addRecurLineItems($contribution->contribution_recur_id, $contribution); CRM_Contribute_BAO_ContributionRecur::copyCustomValues($contributionParams['contribution_recur_id'], $contribution->id); return TRUE; } @@ -4372,6 +4380,16 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) $recurringContributionID = (empty($recurContrib->id)) ? NULL : $recurContrib->id; $event = CRM_Utils_Array::value('event', $objects); + $paymentProcessorId = ''; + if (isset($objects['paymentProcessor'])) { + if (is_array($objects['paymentProcessor'])) { + $paymentProcessorId = $objects['paymentProcessor']['id']; + } + else { + $paymentProcessorId = $objects['paymentProcessor']->id; + } + } + $completedContributionStatusID = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); $contributionParams = array_merge(array( @@ -4389,7 +4407,7 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) $contributionParams['receive_date'] = $changeDate; } - self::repeatTransaction($contribution, $input, $contributionParams); + self::repeatTransaction($contribution, $input, $contributionParams, $paymentProcessorId); $contributionParams['financial_type_id'] = $contribution->financial_type_id; if (is_numeric($memberships)) { @@ -4515,58 +4533,30 @@ LIMIT 1;"; CRM_Contribute_BAO_ContributionRecur::addrecurSoftCredit($objects['contributionRecur']->id, $contribution->id); } - $paymentProcessorId = ''; - if (isset($objects['paymentProcessor'])) { - if (is_array($objects['paymentProcessor'])) { - $paymentProcessorId = $objects['paymentProcessor']['id']; - } - else { - $paymentProcessorId = $objects['paymentProcessor']->id; - } - } - $contributionStatuses = CRM_Core_PseudoConstant::get('CRM_Contribute_DAO_Contribution', 'contribution_status_id', array( 'labelColumn' => 'name', 'flip' => 1, )); - if ((empty($input['prevContribution']) && $paymentProcessorId) || (!$input['prevContribution']->is_pay_later && $input['prevContribution']->contribution_status_id == $contributionStatuses['Pending'])) { + if (isset($input['prevContribution']) && (!$input['prevContribution']->is_pay_later && $input['prevContribution']->contribution_status_id == $contributionStatuses['Pending'])) { $input['payment_processor'] = $paymentProcessorId; } if (!empty($contribution->_relatedObjects['participant'])) { $input['contribution_mode'] = 'participant'; $input['participant_id'] = $contribution->_relatedObjects['participant']->id; - $input['skipLineItem'] = 1; } elseif (!empty($contribution->_relatedObjects['membership'])) { - $input['skipLineItem'] = TRUE; $input['contribution_mode'] = 'membership'; $contribution->contribution_status_id = $contributionParams['contribution_status_id']; $contribution->trxn_id = CRM_Utils_Array::value('trxn_id', $input); $contribution->receive_date = CRM_Utils_Date::isoToMysql($contribution->receive_date); } - $input['contribution_status_id'] = $contributionParams['contribution_status_id']; - $input['total_amount'] = $input['amount']; - $input['contribution'] = $contribution; - $input['financial_type_id'] = $contribution->financial_type_id; - //@todo writing a unit test I was unable to create a scenario where this line did not fatal on second - // and subsequent payments. In this case the line items are created at - // CRM_Contribute_BAO_ContributionRecur::addRecurLineItems - // and since the contribution is saved prior to this line there is always a contribution-id, - // however there is never a prevContribution (which appears to mean original contribution not previous - // contribution - or preUpdateContributionObject most accurately) - // so, this is always called & only appears to succeed when prevContribution exists - which appears - // to mean "are we updating an exisitng pending contribution" - //I was able to make the unit test complete as fataling here doesn't prevent - // the contribution being created - but activities would not be created or emails sent - - CRM_Contribute_BAO_Contribution::recordFinancialAccounts($input, NULL); CRM_Core_Error::debug_log_message("Contribution record updated successfully"); $transaction->commit(); CRM_Contribute_BAO_ContributionRecur::updateRecurLinkedPledge($contribution->id, $recurringContributionID, - $input['contribution_status_id'], $input['total_amount']); + $contributionParams['contribution_status_id'], $input['amount']); // create an activity record if ($input['component'] == 'contribute') { diff --git a/CRM/Contribute/BAO/ContributionRecur.php b/CRM/Contribute/BAO/ContributionRecur.php index 7e3aa2ae2b..6817975029 100644 --- a/CRM/Contribute/BAO/ContributionRecur.php +++ b/CRM/Contribute/BAO/ContributionRecur.php @@ -450,11 +450,13 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) * Later we might merge in data stored against the contribution recur record rather than just return the contribution. * * @param int $id + * @param array $overrides + * Parameters that should be overriden. Add unit tests if using parameters other than total_amount & financial_type_id. * * @return array * @throws \CiviCRM_API3_Exception */ - public static function getTemplateContribution($id) { + public static function getTemplateContribution($id, $overrides = array()) { $templateContribution = civicrm_api3('Contribution', 'get', array( 'contribution_recur_id' => $id, 'options' => array('limit' => 1, 'sort' => array('id DESC')), @@ -462,7 +464,9 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) 'contribution_test' => '', )); if ($templateContribution['count']) { - return $templateContribution['values'][0]; + $result = array_merge($templateContribution['values'][0], $overrides); + $result['line_item'] = CRM_Contribute_BAO_ContributionRecur::calculateRecurLineItems($id, $result['total_amount'], $result['financial_type_id']); + return $result; } return array(); } @@ -634,46 +638,31 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) * @return array */ public static function addRecurLineItems($recurId, $contribution) { - $lineSets = array(); - - $originalContributionID = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $recurId, 'id', 'contribution_recur_id'); - $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($originalContributionID); - if (count($lineItems) == 1) { - foreach ($lineItems as $index => $lineItem) { - if (isset($contribution->financial_type_id)) { - // CRM-17718 allow for possibility of changed financial type ID having been set prior to calling this. - $lineItems[$index]['financial_type_id'] = $contribution->financial_type_id; - } - if ($lineItem['line_total'] != $contribution->total_amount) { - // We are dealing with a changed amount! Per CRM-16397 we can work out what to do with these - // if there is only one line item, and the UI should prevent this situation for those with more than one. - $lineItems[$index]['line_total'] = $contribution->total_amount; - $lineItems[$index]['unit_price'] = round($contribution->total_amount / $lineItems[$index]['qty'], 2); - } - } - } - if (!empty($lineItems)) { - foreach ($lineItems as $key => $value) { - $priceField = new CRM_Price_DAO_PriceField(); - $priceField->id = $value['price_field_id']; - $priceField->find(TRUE); - $lineSets[$priceField->price_set_id][] = $value; - - if ($value['entity_table'] == 'civicrm_membership') { - try { - civicrm_api3('membership_payment', 'create', array( - 'membership_id' => $value['entity_id'], - 'contribution_id' => $contribution->id, - )); - } - catch (CiviCRM_API3_Exception $e) { - // we are catching & ignoring errors as an extra precaution since lost IPNs may be more serious that lost membership_payment data - // this fn is unit-tested so risk of changes elsewhere breaking it are otherwise mitigated + $foundLineItems = FALSE; + + $lineSets = self::calculateRecurLineItems($recurId, $contribution->total_amount, $contribution->financial_type_id); + foreach ($lineSets as $lineItems) { + if (!empty($lineItems)) { + foreach ($lineItems as $key => $value) { + if ($value['entity_table'] == 'civicrm_membership') { + try { + // @todo this should be done by virtue of editing the line item as this link + // is deprecated. This may be the case but needs testing. + civicrm_api3('membership_payment', 'create', array( + 'membership_id' => $value['entity_id'], + 'contribution_id' => $contribution->id, + )); + } + catch (CiviCRM_API3_Exception $e) { + // we are catching & ignoring errors as an extra precaution since lost IPNs may be more serious that lost membership_payment data + // this fn is unit-tested so risk of changes elsewhere breaking it are otherwise mitigated + } } } + $foundLineItems = TRUE; } } - else { + if (!$foundLineItems) { CRM_Price_BAO_LineItem::processPriceSet($contribution->id, $lineSets, $contribution); } return $lineSets; @@ -901,4 +890,37 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) return FALSE; } + /** + * Calculate line items for the relevant recurring calculation. + * + * @param int $recurId + * @param string $total_amount + * @param int $financial_type_id + * + * @return array + */ + public static function calculateRecurLineItems($recurId, $total_amount, $financial_type_id) { + $originalContributionID = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $recurId, 'id', 'contribution_recur_id'); + $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($originalContributionID); + if (count($lineItems) == 1) { + foreach ($lineItems as $index => $lineItem) { + if ($financial_type_id) { + // CRM-17718 allow for possibility of changed financial type ID having been set prior to calling this. + $lineItem['financial_type_id'] = $financial_type_id; + } + if ($lineItem['line_total'] != $total_amount) { + // We are dealing with a changed amount! Per CRM-16397 we can work out what to do with these + // if there is only one line item, and the UI should prevent this situation for those with more than one. + $lineItem['line_total'] = $total_amount; + $lineItem['unit_price'] = round($total_amount / $lineItem['qty'], 2); + } + $priceField = new CRM_Price_DAO_PriceField(); + $priceField->id = $lineItem['price_field_id']; + $priceField->find(TRUE); + $lineSets[$priceField->price_set_id][] = $lineItem; + } + } + return $lineSets; + } + } diff --git a/CRM/Member/BAO/MembershipPayment.php b/CRM/Member/BAO/MembershipPayment.php index 2018f6a4c7..6f08598fbd 100644 --- a/CRM/Member/BAO/MembershipPayment.php +++ b/CRM/Member/BAO/MembershipPayment.php @@ -56,7 +56,13 @@ class CRM_Member_BAO_MembershipPayment extends CRM_Member_DAO_MembershipPayment CRM_Utils_Hook::pre($hook, 'MembershipPayment', CRM_Utils_Array::value('id', $params), $params); $dao = new CRM_Member_DAO_MembershipPayment(); $dao->copyValues($params); - $dao->id = CRM_Utils_Array::value('id', $params); + // We check for membership_id in case we are being called too early in the process. This is + // cludgey but is part of the deprecation process (ie. we are trying to do everything + // from LineItem::create with a view to eventually removing this fn & the table. + if (!civicrm_api3('Membership', 'getcount', array('id' => $params['membership_id']))) { + return $dao; + } + //Fixed for avoiding duplicate entry error when user goes //back and forward during payment mode is notify if (!$dao->find(TRUE)) { diff --git a/CRM/Price/BAO/LineItem.php b/CRM/Price/BAO/LineItem.php index 00627e6f60..26c6495042 100644 --- a/CRM/Price/BAO/LineItem.php +++ b/CRM/Price/BAO/LineItem.php @@ -83,6 +83,15 @@ class CRM_Price_BAO_LineItem extends CRM_Price_DAO_LineItem { $lineItemBAO->copyValues($params); $return = $lineItemBAO->save(); + if ($lineItemBAO->entity_table == 'civicrm_membership' && $lineItemBAO->contribution_id && $lineItemBAO->entity_id) { + $membershipPaymentParams = array( + 'membership_id' => $lineItemBAO->entity_id, + 'contribution_id' => $lineItemBAO->contribution_id, + ); + if (!civicrm_api3('MembershipPayment', 'getcount', $membershipPaymentParams)) { + civicrm_api3('MembershipPayment', 'create', $membershipPaymentParams); + } + } if ($id) { CRM_Utils_Hook::post('edit', 'LineItem', $id, $lineItemBAO); @@ -449,7 +458,7 @@ AND li.entity_id = {$entityId} $lineItems = CRM_Price_BAO_LineItem::create($line); if (!$update && $contributionDetails) { CRM_Financial_BAO_FinancialItem::add($lineItems, $contributionDetails); - if (isset($line['tax_amount'])) { + if (!empty($line['tax_amount'])) { CRM_Financial_BAO_FinancialItem::add($lineItems, $contributionDetails, TRUE); } } diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 74f3f4902f..d6e83bb63c 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -3064,6 +3064,7 @@ AND ( TABLE_NAME LIKE 'civicrm_value_%' ) 'class_name' => 'Payment_PayPalImpl', 'billing_mode' => 3, 'financial_type_id' => 1, + 'financial_account_id' => 12, ), $params); if (!is_numeric($params['payment_processor_type_id'])) { diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index b683496c72..1c439bab9e 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -1777,7 +1777,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { unset($lineItem1['values'][0]['id'], $lineItem1['values'][0]['entity_id']); unset($lineItem2['values'][0]['id'], $lineItem2['values'][0]['entity_id']); $this->assertEquals($lineItem1['values'][0], $lineItem2['values'][0]); - + $this->_checkFinancialRecords(array('id' => $originalContribution['id'] + 1), 'online'); $this->quickCleanUpFinancialEntities(); } @@ -2885,6 +2885,11 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ); $this->assertDBCompareValues('CRM_Financial_DAO_FinancialItem', $fitemParams, $compareParams); } + // This checks that empty Sales tax rows are not being created. If for any reason it needs to be removed the + // line should be copied into all the functions that call this function & evaluated there + // Be really careful not to remove or bypass this without ensuring stray rows do not re-appear + // when calling completeTransaction or repeatTransaction. + $this->callAPISuccessGetCount('FinancialItem', array('description' => 'Sales Tax', 'amount' => 0), 0); } /** -- 2.25.1