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

Jump to bottom

[ready-for-core-team-review]CRM-16189, added code to validate financial type for contribution #8586

Merged
merged 10 commits into from
Aug 1, 2016
Merged

[ready-for-core-team-review]CRM-16189, added code to validate financial type for contribution #8586

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3adac15
CRM-16189, added code to validate financial type for contribution
pradpnayak Jun 15, 2016
c9b6813
CRM-16189, highlight form field but show error in pop up
pradpnayak Jun 21, 2016
70a9df1
--CRM-16189, updated function name
pradpnayak Jun 26, 2016
52d479e
CRM-16189 Changed return type from string to bool and updated functions
Edzelopez Jul 5, 2016
40e78f0
CRM-16189 Fixed unit test
Edzelopez Jul 7, 2016
c9a5a6e
CRM-16189, fixed unit test
pradpnayak Jul 12, 2016
c849d76
CRM-16189, updated the docblock and checked isnull for previous recog…
pradpnayak Jul 20, 2016
ab6c306
CRM-16189, updated docblock
pradpnayak Jul 21, 2016
9603974
CRM-16189, updated function to throw exception in BAO and catch in fo…
pradpnayak Jul 27, 2016
36937e7
CRM-16189, fixed test
pradpnayak Jul 27, 2016
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
5 changes: 5 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 @@ -177,6 +177,11 @@ public static function add(&$params, $ids = array()) {
$params['prevContribution'] = self::getValues(array('id' => $contributionID), CRM_Core_DAO::$_nullArray, CRM_Core_DAO::$_nullArray);
}

// CRM-16189
if (CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($params, $contributionID)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

move the throwing of exception down into CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred() so that the $error string only needs to be specified once.

$error = ts('Revenue recognition date can only be specified if the financial type selected has a deferred revenue account configured. Please have an administrator set up the deferred revenue account at Administer > CiviContribute > Financial Accounts, then configure it for financial types at Administer > CiviContribution > Financial Types, Accounts');
Copy link
Contributor

Choose a reason for hiding this comment

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

Much nicer!

throw new CRM_Core_Exception($error);
}
if ($contributionID) {
CRM_Utils_Hook::pre('edit', 'Contribution', $contributionID, $params);
}
Expand Down
5 changes: 5 additions & 0 deletions CRM/Contribute/Form/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,11 @@ public static function formRule($fields, $files, $self) {
) {
$errors['revenue_recognition_date'] = ts('Month and Year are required field for Revenue Recognition.');
}
// CRM-16189
if (CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($fields, $self->_id, $self->_priceSet['fields'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

refactor as per comment on https://github.com/civicrm/civicrm-core/pull/8586/files#diff-7a5b0e2d131dabc49178460fc63328c0R181

$errors['financial_type_id'] = ' ';
$errors['_qf_default'] = ts('Revenue recognition date can only be specified if the financial type selected has a deferred revenue account configured. Please have an administrator set up the deferred revenue account at Administer > CiviContribute > Financial Accounts, then configure it for financial types at Administer > CiviContribution > Financial Types, Accounts');
}
$errors = array_merge($errors, $softErrors);
return $errors;
}
Expand Down
27 changes: 18 additions & 9 deletions CRM/Financial/BAO/FinancialAccount.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,37 +358,39 @@ public static function validateFinancialAccount($financialAccountId, $financialA

/**
* Validate Financial Type has Deferred Revenue account relationship
* with Financial Account
* with Financial Account.
*
* @param array $params
* Holds submitted formvalues and params from api for updating/adding contribution.
*
* @param int $contributionID
* Contribution ID
*
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic of this function is off IMHO.

First, if there is a $contributionID passed in, shouldn't that take precedence over the $params, which appear to be ones that could be passed to create a contribution. In other words, the actual contribution should be treated as more authoritative than the parameters to create it. Please check its values first and ignore params altogether if contribution is passed in. Don't assume inside side function anything about relationship between params and contribution.

Second, iirc when a contribution is created when financial_type is specified for the contribution but not for the line items, then the civicrm_contribution.financial_type_id is used to set the civicrm_line_item.financial_type_id. The values for the line item financial types are more authoritative than civicrm_contribution's, so they should be checked first before the contribution's when checking for deferred revenue accounts that derive from the params. More specifically, if there is no contribution and one ends up checking the params, then one should check if there is a deferred revenue account for any of the line_item and return true if there is. Only if there is at least one line item in params without a financial type specified, should one check if there is a deferred revenue account for the param's contribution.financial_type_id (L385 is thus wrong, since that check should be done after not before checking the line items).

Copy link
Contributor

@JoeMurray JoeMurray Jul 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, Pradeep to update call signature docblock to indicate that $params are being used to either create or to update the contribution, so they take precedence over contribution. Once that's done, I'm okay with the algorithm.

* @param obj $form
* @param array $priceSetFields
* Array of price fields of a price set.
*
* @return string
* @return bool
*
*/
public static function checkFinancialTypeHasDeferred($params, $contributionID = NULL, $form = NULL) {
public static function checkFinancialTypeHasDeferred($params, $contributionID = NULL, $priceSetFields = NULL) {
if (!CRM_Contribute_BAO_Contribution::checkContributeSettings('deferred_revenue_enabled')) {
return FALSE;
}
$recognitionDate = CRM_Utils_Array::value('revenue_recognition_date', $params);
if (!(!CRM_Utils_System::isNull($recognitionDate)
|| ($contributionID && $params['prevContribution']->revenue_recognition_date))
|| ($contributionID && !CRM_Utils_System::isNull($params['prevContribution']->revenue_recognition_date)))
) {
return FALSE;
Copy link
Contributor

@JoeMurray JoeMurray Jul 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

On L378, stylistically, I'd prefer to see a test around $params['prevContribution']->revenue_recognition_date), eg !CRM_Utils_System::isNull($params['prevContribution']->revenue_recognition_date))
Also, same for around $contributionID

}

$message = ts('Revenue recognition date can only be specified if the financial type selected has a deferred revenue account configured. Please have an administrator set up the deferred revenue account at Administer > CiviContribute > Financial Accounts, then configure it for financial types at Administer > CiviContribution > Financial Types, Accounts');
$lineItems = CRM_Utils_Array::value('line_item', $params);
$financialTypeID = CRM_Utils_Array::value('financial_type_id', $params);
if (!$financialTypeID) {
$financialTypeID = $params['prevContribution']->financial_type_id;
}
if (($contributionID || !empty($params['price_set_id'])) && empty($lineItems)) {
if (!$contributionID) {
CRM_Price_BAO_PriceSet::processAmount($form->_priceSet['fields'],
CRM_Price_BAO_PriceSet::processAmount($priceSetFields,
$params, $items);
}
else {
Expand All @@ -403,13 +405,13 @@ public static function checkFinancialTypeHasDeferred($params, $contributionID =
foreach ($lineItems as $lineItem) {
foreach ($lineItem as $items) {
if (!array_key_exists($items['financial_type_id'], $deferredFinancialType)) {
return $message;
return TRUE;
}
}
}
}
elseif (!array_key_exists($financialTypeID, $deferredFinancialType)) {
return $message;
return TRUE;
}
return FALSE;
}
Expand All @@ -419,6 +421,13 @@ public static function checkFinancialTypeHasDeferred($params, $contributionID =
* with Financial Account.
*
* @param int $financialTypeId
* Financial Type Id.
*
* @param int $entityID
* Holds id for PriceSet/PriceField/PriceFieldValue.
*
* @param string $entity
* Entity like PriceSet/PriceField/PriceFieldValue.
*
* @return bool
*
Expand Down
23 changes: 15 additions & 8 deletions tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,9 @@ public function testcheckFinancialTypeHasDeferred() {
$cid = $this->individualCreate();
$params = array(
'contact_id' => $cid,
'receive_date' => '2010-01-20',
'receive_date' => '2016-01-20',
'total_amount' => 100,
'financial_type_id' => 3,
'financial_type_id' => 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: So it appears that this is Event Fee financial type (https://github.com/civicrm/civicrm-core/blob/master/xml/templates/civicrm_data.tpl#L128) which has a deferred revenue account (https://github.com/civicrm/civicrm-core/blob/master/xml/templates/civicrm_data.tpl#L889) defined for it at
https://github.com/civicrm/civicrm-core/blob/master/xml/templates/civicrm_data.tpl#L1700 so the created contribution should have a financial type with a deferred revenue account.

'revenue_recognition_date' => date('Ymd', strtotime("+1 month")),
'line_items' => array(
array(
Expand All @@ -264,6 +264,7 @@ public function testcheckFinancialTypeHasDeferred() {
'qty' => 1,
'unit_price' => 100,
'line_total' => 100,
'financial_type_id' => 4,
),
array(
'entity_table' => 'civicrm_contribution',
Expand All @@ -273,17 +274,23 @@ public function testcheckFinancialTypeHasDeferred() {
'qty' => 1,
'unit_price' => 200,
'line_total' => 200,
'financial_type_id' => 1,
'financial_type_id' => 4,
),
),
'params' => array(),
),
),
);
$contribution = CRM_Contribute_BAO_Contribution::create($params);
$valid = CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($params, $contribution->id);
$message = "Revenue recognition date can only be specified if the financial type selected has a deferred revenue account configured. Please have an administrator set up the deferred revenue account at Administer > CiviContribute > Financial Accounts, then configure it for financial types at Administer > CiviContribution > Financial Types, Accounts";
$this->assertEquals($valid, $message, "The messages do not match");
$valid = CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($params);
Copy link
Contributor

Choose a reason for hiding this comment

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

check for catching an exception

$this->assertFalse($valid, "This should have been false");
$params = array(
'contact_id' => $cid,
'receive_date' => '2016-01-20',
'total_amount' => 100,
'financial_type_id' => 1,
'revenue_recognition_date' => date('Ymd', strtotime("+1 month")),
);
$valid = CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($params);
Copy link
Contributor

Choose a reason for hiding this comment

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

check for catching an exception

$this->assertTrue($valid, "This should have been true.");
}

/**
Expand Down