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

Simplify / Cleanup Contribution BAO create #17115

Closed
wants to merge 3 commits into from
+30 −75
Closed

Simplify / Cleanup Contribution BAO create #17115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc29dfd
Retrieve full contribution object once so we don't have to keep check…
mattwire Apr 18, 2020
328b5ef
Fix undefined index on financial_type_id for api_v3_ContributionTest:…
mattwire Apr 27, 2020
e74f01c
Fix test results to check actual values that are written to the DB
mattwire Apr 27, 2020
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
2 changes: 1 addition & 1 deletion CRM/Activity/BAO/Activity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
86 changes: 21 additions & 65 deletions CRM/Contribute/BAO/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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'])
) {
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -493,91 +497,45 @@ 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'];
}
CRM_Core_BAO_Note::add($noteParams);
}

// make entry in batch entity batch table
Copy link
Contributor

Choose a reason for hiding this comment

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

So all of this is to do a load if we don't have all the contribution fields - maybe a helper function that identifies if there IS missing required info

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);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire why on earth do we do this if batch_id is present? It looks to me an awful lot like logic to support a specific form layer flow - I just added #17120 to figure out if it is tested at any point if so where it is hit from - I'm guessing we can likely remove this

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it does get hit at least in one place https://test.civicrm.org/job/CiviCRM-Core-PR/33520/testReport/junit/(root)/CRM_Contribute_Form_SearchTest/testBatchFilter/

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - it's not clear the loading that is happening adds any value though!


CRM_Contribute_BAO_ContributionSoft::processSoftContribution($params, $contribution);

$transaction->commit();

$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
if (empty($params['skipRecentView'])) {
$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',
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - so we need these for addRecent. Maybe we just need a function to addContributionRecent that loads extra if need-be

'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)) {
Expand Down Expand Up @@ -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 = "
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -3589,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'
);
}
Expand All @@ -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 = [
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -3837,7 +3794,6 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
}
unset($params['line_item']);
self::$_trxnIDs = NULL;
return $return;
}

/**
Expand Down
3 changes: 1 addition & 2 deletions CRM/Core/BAO/FinancialTrxn.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
14 changes: 7 additions & 7 deletions api/v3/examples/Contribution/Create.ex.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' => '',
Expand All @@ -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' => '',
Expand All @@ -83,7 +83,7 @@ function contribution_create_expectedresult() {
'creditnote_id' => '',
'tax_amount' => '',
'revenue_recognition_date' => '',
'is_template' => '',
'is_template' => '0',
'contribution_type_id' => '1',
],
],
Expand Down