From dc29dfd99e09a8922a8f0d22b12030390992352e Mon Sep 17 00:00:00 2001 From: Matthew Wire Date: Sat, 18 Apr 2020 14:23:15 +0100 Subject: [PATCH 1/3] Retrieve full contribution object once so we don't have to keep checking if bits are set. Fix bug where retrieve fails if you had passed in an unrounded fee_amount which led to addActivity failing with missing contact ID --- CRM/Activity/BAO/Activity.php | 2 +- CRM/Contribute/BAO/Contribution.php | 84 +++++++---------------------- CRM/Core/BAO/FinancialTrxn.php | 3 +- 3 files changed, 22 insertions(+), 67 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 9b8f2292b8a7..0f264c974810 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -1676,7 +1676,7 @@ public static function getContactActivity($contactId) { /** * Add activity for Membership/Event/Contribution. * - * @param object $activity + * @param CRM_Core_DAO $activity * particular component object. * @param string $activityType * For Membership Signup or Renewal. diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 3f2d17d92058..1bcd354d59ec 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -187,15 +187,21 @@ public static function add(&$params, $ids = []) { $contribution->currency = CRM_Core_Config::singleton()->defaultCurrency; } } - - $result = $contribution->save(); + $contribution->save(); + $params['contribution_id'] = $contribution->id; + $contribution = new CRM_Contribute_BAO_Contribution(); + $contribution->id = $params['contribution_id']; + $contribution->find(TRUE); // Add financial_trxn details as part of fix for CRM-4724 + // @todo these params are not part of the contribution object - they should be removed + // Any caller that needs them can get them from the payment linked to the contribution. $contribution->trxn_result_code = $params['trxn_result_code'] ?? NULL; $contribution->payment_processor = $params['payment_processor'] ?? NULL; - //add Account details $params['contribution'] = $contribution; + + // add Account details if (empty($params['is_post_payment_create'])) { // If this is being called from the Payment.create api/ BAO then that Entity // takes responsibility for the financial transactions. In fact calling Payment.create @@ -211,12 +217,10 @@ public static function add(&$params, $ids = []) { CRM_Contribute_BAO_ContributionRecur::updateOnNewPayment( (!empty($params['contribution_recur_id']) ? $params['contribution_recur_id'] : $params['prevContribution']->contribution_recur_id), $contributionStatus, - CRM_Utils_Array::value('receive_date', $params) + $params['receive_date'] ?? NULL ); } - $params['contribution_id'] = $contribution->id; - if (!empty($params['custom']) && is_array($params['custom']) ) { @@ -226,7 +230,7 @@ public static function add(&$params, $ids = []) { CRM_Contact_BAO_GroupContactCache::opportunisticCacheFlush(); CRM_Utils_Hook::post($action, 'Contribution', $contribution->id, $contribution); - return $result; + return $contribution; } /** @@ -493,14 +497,13 @@ public static function create(&$params, $ids = []) { } $params['contribution_id'] = $contribution->id; - $session = CRM_Core_Session::singleton(); if (!empty($params['note'])) { $noteParams = [ 'entity_table' => 'civicrm_contribution', 'note' => $params['note'], 'entity_id' => $contribution->id, - 'contact_id' => $session->get('userID'), + 'contact_id' => CRM_Core_Session::getLoggedInContactID(), ]; if (!$noteParams['contact_id']) { $noteParams['contact_id'] = $params['contact_id']; @@ -508,27 +511,6 @@ public static function create(&$params, $ids = []) { CRM_Core_BAO_Note::add($noteParams); } - // make entry in batch entity batch table - if (!empty($params['batch_id'])) { - // in some update cases we need to get extra fields - ie an update that doesn't pass in all these params - $titleFields = [ - 'contact_id', - 'total_amount', - 'currency', - 'financial_type_id', - ]; - $retrieveRequired = 0; - foreach ($titleFields as $titleField) { - if (!isset($contribution->$titleField)) { - $retrieveRequired = 1; - break; - } - } - if ($retrieveRequired == 1) { - $contribution->find(TRUE); - } - } - CRM_Contribute_BAO_ContributionSoft::processSoftContribution($params, $contribution); $transaction->commit(); @@ -536,22 +518,15 @@ public static function create(&$params, $ids = []) { $activity = civicrm_api3('Activity', 'get', [ 'source_record_id' => $contribution->id, 'options' => ['limit' => 1], - 'sequential' => 1, 'activity_type_id' => 'Contribution', 'return' => ['id', 'campaign'], ]); //CRM-18406: Update activity when edit contribution. - if ($activity['count']) { - // CRM-13237 : if activity record found, update it with campaign id of contribution - // @todo compare campaign ids first. - CRM_Core_DAO::setFieldValue('CRM_Activity_BAO_Activity', $activity['id'], 'campaign_id', $contribution->campaign_id); + if (!empty($activity['id'])) { $contribution->activity_id = $activity['id']; } - if (empty($contribution->contact_id)) { - $contribution->find(TRUE); - } CRM_Activity_BAO_Activity::addActivity($contribution, 'Contribution'); // do not add to recent items for import, CRM-4399 @@ -559,25 +534,8 @@ public static function create(&$params, $ids = []) { $url = CRM_Utils_System::url('civicrm/contact/view/contribution', "action=view&reset=1&id={$contribution->id}&cid={$contribution->contact_id}&context=home" ); - // in some update cases we need to get extra fields - ie an update that doesn't pass in all these params - $titleFields = [ - 'contact_id', - 'total_amount', - 'currency', - 'financial_type_id', - ]; - $retrieveRequired = 0; - foreach ($titleFields as $titleField) { - if (!isset($contribution->$titleField)) { - $retrieveRequired = 1; - break; - } - } - if ($retrieveRequired == 1) { - $contribution->find(TRUE); - } - $financialType = CRM_Contribute_PseudoConstant::financialType($contribution->financial_type_id); - $title = CRM_Contact_BAO_Contact::displayName($contribution->contact_id) . ' - (' . CRM_Utils_Money::format($contribution->total_amount, $contribution->currency) . ' ' . ' - ' . $financialType . ')'; + $financialTypeLabel = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', $contribution->financial_type_id); + $title = CRM_Contact_BAO_Contact::displayName($contribution->contact_id) . ' - (' . CRM_Utils_Money::format($contribution->total_amount, $contribution->currency) . ' ' . ' - ' . $financialTypeLabel . ')'; $recentOther = []; if (CRM_Core_Permission::checkActionPermission('CiviContribute', CRM_Core_Action::UPDATE)) { @@ -1268,7 +1226,7 @@ private static function isContributionUpdateARefund($previousContributionStatusI */ protected static function getContributionTransactionInformation($contributionId, int $financialTypeID) { $rows = []; - $feeFinancialAccount = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($financialTypeID, 'Expense Account is'); + $feeFinancialAccount = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($financialTypeID, 'Expense Account is'); // Need to exclude fee trxn rows so filter out rows where TO FINANCIAL ACCOUNT is expense account $sql = " @@ -3488,11 +3446,10 @@ public static function isSubscriptionCancelled($contributionId) { * * @param array $params * Contribution object, line item array and params for trxn. - * - * * @param array $financialTrxnValues * - * @return null|\CRM_Core_BAO_FinancialTrxn + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ public static function recordFinancialAccounts(&$params, $financialTrxnValues = NULL) { $skipRecords = $update = $return = $isRelatedId = FALSE; @@ -3611,7 +3568,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = $totalAmount = $params['total_amount'] ?? NULL; if (!isset($totalAmount) && !empty($params['prevContribution'])) { - $totalAmount = $params['total_amount'] = $params['prevContribution']->total_amount; + $totalAmount = $params['prevContribution']->total_amount; } //build financial transaction params $trxnParams = [ @@ -3800,7 +3757,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = self::recordAlwaysAccountsReceivable($trxnParams, $params); $trxnParams['pan_truncation'] = $params['pan_truncation'] ?? NULL; $trxnParams['card_type_id'] = $params['card_type_id'] ?? NULL; - $return = $financialTxn = CRM_Core_BAO_FinancialTrxn::create($trxnParams); + $financialTxn = CRM_Core_BAO_FinancialTrxn::create($trxnParams); $params['entity_id'] = $financialTxn->id; if (empty($params['partial_payment_total']) && empty($params['partial_amount_to_pay'])) { self::$_trxnIDs[] = $financialTxn->id; @@ -3837,7 +3794,6 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = } unset($params['line_item']); self::$_trxnIDs = NULL; - return $return; } /** diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php index f3c242da19f3..56a1cf212904 100644 --- a/CRM/Core/BAO/FinancialTrxn.php +++ b/CRM/Core/BAO/FinancialTrxn.php @@ -30,8 +30,7 @@ public function __construct() { /** * Takes an associative array and creates a financial transaction object. * - * @param array $params - * (reference ) an assoc array of name/value pairs. + * @param array $params FinancialTrxn params * * @return CRM_Financial_DAO_FinancialTrxn */ From 328b5efd552adaa775dad66a38fe9b4aa323cd64 Mon Sep 17 00:00:00 2001 From: Matthew Wire Date: Mon, 27 Apr 2020 21:20:16 +0100 Subject: [PATCH 2/3] Fix undefined index on financial_type_id for api_v3_ContributionTest::testSendconfirmationPayLater --- CRM/Contribute/BAO/Contribution.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 1bcd354d59ec..56014c63bddf 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -3546,7 +3546,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = ]; if (in_array($contributionStatus, $pendingStatus)) { $params['to_financial_account_id'] = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship( - $params['financial_type_id'], + $params['contribution']->financial_type_id, 'Accounts Receivable Account is' ); } From e74f01c3c9d6a650cb5fa7a0b4456c979e92eadb Mon Sep 17 00:00:00 2001 From: Matthew Wire Date: Mon, 27 Apr 2020 21:20:41 +0100 Subject: [PATCH 3/3] Fix test results to check actual values that are written to the DB --- api/v3/examples/Contribution/Create.ex.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/api/v3/examples/Contribution/Create.ex.php b/api/v3/examples/Contribution/Create.ex.php index 1d47cb3a30d3..8e79ff1dc389 100644 --- a/api/v3/examples/Contribution/Create.ex.php +++ b/api/v3/examples/Contribution/Create.ex.php @@ -58,11 +58,11 @@ function contribution_create_expectedresult() { 'financial_type_id' => '1', 'contribution_page_id' => '1', 'payment_instrument_id' => '4', - 'receive_date' => '20120511000000', - 'non_deductible_amount' => '', - 'total_amount' => '100', - 'fee_amount' => 0, - 'net_amount' => '100', + 'receive_date' => '2012-05-11 00:00:00', + 'non_deductible_amount' => '0.00', + 'total_amount' => '100.00', + 'fee_amount' => '0.00', + 'net_amount' => '100.00', 'trxn_id' => '12345', 'invoice_id' => '67890', 'invoice_number' => '', @@ -74,7 +74,7 @@ function contribution_create_expectedresult() { 'source' => 'SSF', 'amount_level' => '', 'contribution_recur_id' => '', - 'is_test' => '', + 'is_test' => '0', 'is_pay_later' => '1', 'contribution_status_id' => '2', 'address_id' => '', @@ -83,7 +83,7 @@ function contribution_create_expectedresult() { 'creditnote_id' => '', 'tax_amount' => '', 'revenue_recognition_date' => '', - 'is_template' => '', + 'is_template' => '0', 'contribution_type_id' => '1', ], ],