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

[ready for core team review] CRM-20677, used generalized function to retrieve financial account id #10463

Merged
merged 3 commits into from
Jul 14, 2017
Merged

[ready for core team review] CRM-20677, used generalized function to retrieve financial account id #10463

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
53d294b
CRM-20677, used generalized function to retrieve financial account id
pradpnayak Jun 3, 2017
d8fbd79
CRM-20677, updated getInstrumentFinancialAccount() to use api
pradpnayak Jun 3, 2017
02a9c0a
test failure fixes
monishdeb Jul 4, 2017
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
26 changes: 4 additions & 22 deletions CRM/Contribute/BAO/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3667,7 +3667,10 @@ public static function updateFinancialAccounts(&$params, $context = NULL) {
$itemParams['amount'] = self::getMultiplier($params['contribution']->contribution_status_id, $context) * $lineItemDetails['tax_amount'];
$itemParams['description'] = $taxTerm;
if ($lineItemDetails['financial_type_id']) {
$itemParams['financial_account_id'] = self::getFinancialAccountId($lineItemDetails['financial_type_id']);
$itemParams['financial_account_id'] = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount(
$lineItemDetails['financial_type_id'],
'Sales Tax Account is'
);
}
CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds);
}
Expand Down Expand Up @@ -4182,27 +4185,6 @@ public static function getPaymentInfo($id, $component, $getTrxnInfo = FALSE, $us
return $info;
}

/**
* Get financial account id has 'Sales Tax Account is' account relationship with financial type.
*
* @param int $financialTypeId
*
* @return int
* Financial Account Id
*/
public static function getFinancialAccountId($financialTypeId) {
$accountRel = key(CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL, " AND v.name LIKE 'Sales Tax Account is' "));
$searchParams = array(
'entity_table' => 'civicrm_financial_type',
'entity_id' => $financialTypeId,
'account_relationship' => $accountRel,
);
$result = array();
CRM_Financial_BAO_FinancialTypeAccount::retrieve($searchParams, $result);

return CRM_Utils_Array::value('financial_account_id', $result);
}

/**
* Get the tax amount (misnamed function).
*
Expand Down
14 changes: 4 additions & 10 deletions CRM/Financial/BAO/FinancialItem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,25 +105,19 @@ public static function add($lineItem, $contribution, $taxTrxnID = FALSE, $trxnId
$taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings);
$params['amount'] = $lineItem->tax_amount;
$params['description'] = $taxTerm;
$accountRel = key(CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL, " AND v.name LIKE 'Sales Tax Account is' "));
$accountRelName = 'Sales Tax Account is';
}
else {
$accountRelName = 'Income Account is';
if (property_exists($contribution, 'revenue_recognition_date') && !CRM_Utils_System::isNull($contribution->revenue_recognition_date)) {
$accountRelName = 'Deferred Revenue Account is';
}
$accountRel = key(CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL, " AND v.name LIKE '{$accountRelName}' "));
}
if ($lineItem->financial_type_id) {
$searchParams = array(
'entity_table' => 'civicrm_financial_type',
'entity_id' => $lineItem->financial_type_id,
'account_relationship' => $accountRel,
$params['financial_account_id'] = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount(
$lineItem->financial_type_id,
$accountRelName
);

$result = array();
CRM_Financial_BAO_FinancialTypeAccount::retrieve($searchParams, $result);
$params['financial_account_id'] = CRM_Utils_Array::value('financial_account_id', $result);
}
if (empty($trxnId)) {
$trxnId['id'] = CRM_Contribute_BAO_Contribution::$_trxnIDs;
Expand Down
42 changes: 13 additions & 29 deletions CRM/Financial/BAO/FinancialTypeAccount.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,6 @@ public function __construct() {
parent::__construct();
}

/**
* Financial account.
* @var array
*/
private static $financialAccount;

/**
* Fetch object based on array of properties.
*
Expand Down Expand Up @@ -157,30 +151,20 @@ public static function del($financialTypeAccountId, $accountId = NULL) {
* @param int $paymentInstrumentValue
* Payment instrument value.
*
* @return array|null|string
* @return null|int
*/
public static function getInstrumentFinancialAccount($paymentInstrumentValue = NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a small note -- this was technically a change in the function signature (dropping the NULL default). Not a huge deal because it's an internal function, but there was a stale reference in sql/GenerateData.php (which is used for regen.sh) where it calls:

 $financialAccountId = CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount();

To work around it, I grepped forgetInstrumentFinancialAccount and saw a test written with the hard-coded value getInstrumentFinancialAccount(4). Not sure if it's the right thing, but using 4 does seem to get regen.sh running.

if (!self::$financialAccount) {
$query = "SELECT ceft.financial_account_id, cov.value
FROM civicrm_entity_financial_account ceft
INNER JOIN civicrm_option_value cov ON cov.id = ceft.entity_id AND ceft.entity_table = 'civicrm_option_value'
INNER JOIN civicrm_option_group cog ON cog.id = cov.option_group_id
WHERE cog.name = 'payment_instrument' ";

if ($paymentInstrumentValue) {
$query .= " AND cov.value = '{$paymentInstrumentValue}' ";
return CRM_Core_DAO::singleValueQuery($query);
}
else {
$result = CRM_Core_DAO::executeQuery($query);
while ($result->fetch()) {
self::$financialAccount[$result->value] = $result->financial_account_id;
}
return self::$financialAccount;
}
}

return $paymentInstrumentValue ? self::$financialAccount[$paymentInstrumentValue] : self::$financialAccount;
public static function getInstrumentFinancialAccount($paymentInstrumentValue) {
$paymentInstrument = civicrm_api3('OptionValue', 'getsingle', array(
Copy link
Contributor

Choose a reason for hiding this comment

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

@pradpnayak this is what is causing the test to fail would there be a reason why there is 2 payment_instruments with the same option value? Maybe its a test problem

Copy link
Member

Choose a reason for hiding this comment

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

@seamuslee001 you are right and I have fixed that test failure. Thanks for bringing that glitch into notice :)

'return' => array("id"),
'value' => $paymentInstrumentValue,
'option_group_id' => "payment_instrument",
));
$financialAccountId = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount(
$paymentInstrument['id'],
NULL,
'civicrm_option_value'
);
return $financialAccountId;
}

/**
Expand Down
18 changes: 18 additions & 0 deletions tests/phpunit/api/v3/ContributionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,9 @@ public function testCreateUpdateContributionPaymentInstrument() {
$contribution = $this->callAPISuccess('contribution', 'create', $newParams);
$this->assertAPISuccess($contribution);
$this->_checkFinancialTrxn($contribution, 'paymentInstrument', $instrumentId);

// cleanup - delete created payment instrument
$this->_deletedAddedPaymentInstrument();
}

/**
Expand All @@ -1173,6 +1176,9 @@ public function testCreateUpdateNegativeContributionPaymentInstrument() {
$contribution = $this->callAPISuccess('contribution', 'create', $newParams);
$this->assertAPISuccess($contribution);
$this->_checkFinancialTrxn($contribution, 'paymentInstrument', $instrumentId, array('total_amount' => '-100.00'));

// cleanup - delete created payment instrument
$this->_deletedAddedPaymentInstrument();
}

/**
Expand Down Expand Up @@ -3428,6 +3434,18 @@ public function _addPaymentInstrument() {
return $optionValue['values'][$optionValue['id']]['value'];
}

public function _deletedAddedPaymentInstrument() {
$result = $this->callAPISuccess('OptionValue', 'get', array(
'option_group_id' => 'payment_instrument',
'name' => 'Test Card',
'value' => '6',
'is_active' => 1,
));
if ($id = CRM_Utils_Array::value('id', $result)) {
$this->callAPISuccess('OptionValue', 'delete', array('id' => $id));
}
}

/**
* Set up the basic recurring contribution for tests.
*
Expand Down