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

Extract getContributionBalance function, use that rather than wrapper… #13151

Merged
merged 1 commit into from
Nov 30, 2018
Merged

Extract getContributionBalance function, use that rather than wrapper… #13151

Show file tree
Hide file tree
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
33 changes: 33 additions & 0 deletions CRM/Contribute/BAO/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4168,6 +4168,39 @@ public static function getPaymentInfo($id, $component, $getTrxnInfo = FALSE, $us
return $info;
}

/**
* Get the outstanding balance on a contribution.
*
* @param int $contributionId
* @param float $contributionTotal
* Optional amount to override the saved amount paid (e.g if calculating what it WILL be).
*
* @return float
*/
public static function getContributionBalance($contributionId, $contributionTotal = NULL) {

if ($contributionTotal === NULL) {
$contributionTotal = CRM_Price_BAO_LineItem::getLineTotal($contributionId);
}
$statusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');
$refundStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Refunded');

$sqlFtTotalAmt = "
SELECT SUM(ft.total_amount)
FROM civicrm_financial_trxn ft
INNER JOIN civicrm_entity_financial_trxn eft ON (ft.id = eft.financial_trxn_id AND eft.entity_table = 'civicrm_contribution' AND eft.entity_id = {$contributionId})
WHERE ft.is_payment = 1
AND ft.status_id IN ({$statusId}, {$refundStatusId})
";

$ftTotalAmt = CRM_Core_DAO::singleValueQuery($sqlFtTotalAmt);
Copy link
Member

Choose a reason for hiding this comment

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

you can further optimize it via:

$ftTotalAmt = CRM_Core_BAO_FinancialTrxn::getTotalPayments($contributionId);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monishdeb ok - wanna do that as a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monishdeb I just revisited this to see if I should make this suggested change in this PR (now it's Monday :-))

But there is a minor change between this and the function you suggest calling - this function looks at completed & refunded whereas that one only looks at completed.

The getTotalPayments function is called from 2 places & my feeling is that those places would be better served by also looking at refunds - per this - but I think we should analyse that so a follow up PR makes more sense. I think @jitendrapurohit had actually suggested changing one of those places in his recent PR

Copy link
Member

Choose a reason for hiding this comment

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

Submitted followup PR #13187

if (!$ftTotalAmt) {
$ftTotalAmt = 0;
}
$currency = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionId, 'currency');
return CRM_Utils_Money::subtractCurrencies($contributionTotal, $ftTotalAmt, $currency);
}

/**
* Get the tax amount (misnamed function).
*
Expand Down
8 changes: 4 additions & 4 deletions CRM/Contribute/Form/Contribution/Main.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1313,13 +1313,13 @@ public function assignFormVariablesByContributionID() {
CRM_Core_Error::statusBounce(ts("Returning since there is no contact attached to this contribution id."));
}

$payment = CRM_Contribute_BAO_Contribution::getPaymentInfo($this->_ccid, 'contribution');
$paymentBalance = CRM_Contribute_BAO_Contribution::getContributionBalance($this->_ccid);
//bounce if the contribution is not pending.
if (empty($payment['balance'])) {
if ((int) $paymentBalance <= 0) {
CRM_Core_Error::statusBounce(ts("Returning since contribution has already been handled."));
}
if (!empty($payment['total'])) {
$this->_pendingAmount = $payment['total'];
if (!empty($paymentBalance)) {
$this->_pendingAmount = $paymentBalance;
$this->assign('pendingAmount', $this->_pendingAmount);
}

Expand Down
25 changes: 3 additions & 22 deletions CRM/Core/BAO/FinancialTrxn.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,26 +466,10 @@ public static function getPartialPaymentWithType($entityId, $entityName = 'parti
$financialTypeId = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionId, 'financial_type_id');

if ($contributionId && $financialTypeId) {
$statusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');
$refundStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Refunded');

if (empty($lineItemTotal)) {
$lineItemTotal = CRM_Price_BAO_LineItem::getLineTotal($contributionId);
}
$sqlFtTotalAmt = "
SELECT SUM(ft.total_amount)
FROM civicrm_financial_trxn ft
INNER JOIN civicrm_entity_financial_trxn eft ON (ft.id = eft.financial_trxn_id AND eft.entity_table = 'civicrm_contribution' AND eft.entity_id = {$contributionId})
WHERE ft.is_payment = 1
AND ft.status_id IN ({$statusId}, {$refundStatusId})
";

$ftTotalAmt = CRM_Core_DAO::singleValueQuery($sqlFtTotalAmt);
if (!$ftTotalAmt) {
$ftTotalAmt = 0;
}
$currency = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionId, 'currency');
$value = $paymentVal = CRM_Utils_Money::subtractCurrencies($lineItemTotal, $ftTotalAmt, $currency);
$value = CRM_Contribute_BAO_Contribution::getContributionBalance($contributionId, $lineItemTotal);

$paymentVal = $value;
if ($returnType) {
$value = array();
if ($paymentVal < 0) {
Expand All @@ -494,9 +478,6 @@ public static function getPartialPaymentWithType($entityId, $entityName = 'parti
elseif ($paymentVal > 0) {
$value['amount_owed'] = $paymentVal;
}
elseif ($lineItemTotal == $ftTotalAmt) {
$value['full_paid'] = $ftTotalAmt;
}
Copy link
Member

Choose a reason for hiding this comment

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

lol and I didn't relaised it's not used anwhere in Civi but still we are having a separate attribute.

$ grep -irn "full_paid" CRM/
CRM//Core/BAO/FinancialTrxn.php:498:          $value['full_paid'] = $ftTotalAmt;

Agree with this change

}
}
return $value;
Expand Down
5 changes: 2 additions & 3 deletions tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,16 +207,15 @@ public function testAddPartialPayment() {
$amtPaid = 60;
$balance = $feeAmt - $amtPaid;
$result = $this->addParticipantWithPayment($feeAmt, $amtPaid);
extract($result);
$paymentInfo = CRM_Contribute_BAO_Contribution::getPaymentInfo($participant['id'], 'event');
$paymentInfo = CRM_Contribute_BAO_Contribution::getPaymentInfo($result['participant']['id'], 'event');
Copy link
Member

Choose a reason for hiding this comment

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

better :)


// amount checking
$this->assertEquals(round($paymentInfo['total']), $feeAmt, 'Total amount recorded is not proper');
$this->assertEquals(round($paymentInfo['paid']), $amtPaid, 'Amount paid is not proper');
$this->assertEquals(round($paymentInfo['balance']), $balance, 'Balance amount is not proper');

// status checking
$this->assertEquals($participant['participant_status_id'], 14, 'Status record is not proper for participant');
$this->assertEquals($result['participant']['participant_status_id'], 14, 'Status record is not proper for participant');
$this->assertEquals($result['contribution']['contribution_status_id'], 8, 'Status record is not proper for contribution');
}

Expand Down