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

pradpnayak · 2016-06-15T20:51:29Z

CRM-16189: Improve support for Accrual Method bookkeeping
https://issues.civicrm.org/jira/browse/CRM-16189

eileenmcnaughton · 2016-06-15T23:54:27Z

CRM/Contribute/BAO/Contribution.php

@@ -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
+    $error = CRM_Financial_BAO_FinancialAccount::checkForValidFinancialType($params, $contributionID);


this seems to be a new function so let's have it throw an exception. In general we prefer BAO functions (& lots of other things) to throw exceptions rather than return errors - which leads to ugly stuff like this. The form layer should catch & handle the exceptions

colemanw · 2016-07-04T23:23:20Z

CRM/Contribute/Form/Contribution.php

@@ -998,7 +998,12 @@ public static function formRule($fields, $files, $self) {
        $errors['trxn_id'] = ts('Transaction ID\'s must be unique. Transaction \'%1\' already exists in your database.', array(1 => $fields['trxn_id']));
      }
    }
-
+    // CRM-16189
+    $errorMessage = CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($fields, $self->_id, $self);


IMO checkFinancialTypeHasDeferred should be updated to return a boolean and the message should be generated here.

Edzelopez · 2016-07-05T11:11:42Z

@colemanw, are the test errors related?

eileenmcnaughton · 2016-07-05T21:38:48Z

CRM/Contribute/BAO/Contribution.php

-    $error = CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($params, $contributionID);
-    if ($error) {
+    if (CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($params, $contributionID)) {
+      $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');


Much nicer!

colemanw · 2016-07-05T22:14:52Z

I don't think so. This does need rebasing though. @pradpnayak or @Edzelopez

Edzelopez · 2016-07-06T06:23:34Z

Hi @colemanw, I've rebased the PR.

colemanw · 2016-07-06T14:08:28Z

jenkins, retest this please

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189 Conflicts: CRM/Contribute/Form/Contribution.php

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

Edzelopez · 2016-07-07T15:49:47Z

jenkins, retest this please

JoeMurray · 2016-07-07T17:14:30Z

Edsel / Pradeep, could you check the tests for add and edit are different and appropriate wrt to storing value of opening balances?

colemanw · 2016-07-07T20:19:54Z

@Edzelopez or @pradpnayak these 2 test failures do look relevant.

JoeMurray · 2016-07-07T22:01:44Z

tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php

      'total_amount' => 100,
-      'financial_type_id' => 3,
+      'financial_type_id' => 4,


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.

monishdeb · 2016-07-08T08:48:58Z

Jenkin, test this please

monishdeb · 2016-07-08T09:28:35Z

@pradpnayak there is one related test failure https://test.civicrm.org/job/CiviCRM-Core-PR/10564/

This should have been true.
Failed asserting that false is true. ```

JoeMurray · 2016-07-08T18:04:51Z

@pradpnayak check with @Edzelopez - I think he did some refactoring work on this today based on comments but didn't commit because it caused various test failures.

---------------------------------------- * CRM-16189: https://issues.civicrm.org/jira/browse/CRM-16189

pradpnayak · 2016-07-13T12:38:29Z

https://test.civicrm.org/job/CiviCRM-Core-PR/10619/
Is failure related to changes in PR?

JoeMurray · 2016-07-19T20:19:38Z

Pradeep, checkFinancialTypeHasDeferred still needs to be rewritten to address my comments so far as I can see.

…nition date ---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

JoeMurray · 2016-07-20T15:36:08Z

CRM/Financial/BAO/FinancialAccount.php

   *
-   * @return string
+   * @return bool
   *
   */
  public static function checkFinancialTypeHasDeferred($params, $contributionID = NULL, $form = NULL) {


I don't think it is good style to pass $form object to BAO. And I don't see where $form is used in function. Can we remove it from call signature?

Its used later part in code to generate line items When creating new contribution from backoffice.

Okay, I see it now at L393. @colemanw could you comment on what to do here - is this acceptable practice, or should the relevant work to deal with Priceset be done in the form and then passed in to this function?

@JoeMurray @pradpnayak the former - never do anything to a form outside the form class it you can help it!

I'm confused what you mean by 'the former': this is 'acceptable practice' to read but not modify contents of the form? Or did you mean 'the latter', the form should do the work with the Priceset so that more detailed arg than the form would be passed into this function. Thanks for the feedback @eileenmcnaughton

I think it would be cleaner to change the variable to $priceSet and pass in $form->_priceSet instead of the entire $form.

JoeMurray · 2016-07-20T15:38:34Z

Could you update call signature docblock for validateFinancialType()?

JoeMurray · 2016-07-20T20:26:40Z

@eileenmcnaughton are you doing anything with Auth.net right now that might cause the error here? Or is it likely something in this PR?

eileenmcnaughton · 2016-07-20T20:29:49Z

That test has an intermittent failure which is time dependent - that does not look like the failure but it could have arisen in a previous PR without people noticing it due to the intermittent failure - it's probably work checking test history on test.civicrm.org

JoeMurray · 2016-07-20T20:50:52Z

jenkins, retest this please

updated to code follow CiviCRM standard ---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

pradpnayak · 2016-07-21T10:32:10Z

Updated Code

JoeMurray · 2016-07-21T18:51:54Z

Coleman, I think this is ready for merge, and is part of accrual accounting functionality for this release

JoeMurray · 2016-07-22T14:17:37Z

CRM/Contribute/BAO/Contribution.php

@@ -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)) {


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

…rm level ---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

pradpnayak · 2016-07-27T13:09:38Z

Added fix

pradpnayak · 2016-07-27T16:10:19Z

jenkins, retest this please

JoeMurray · 2016-08-01T22:11:06Z

jenkins, retest this please

JoeMurray · 2016-08-01T22:12:26Z

@yashodha If it says 'Some checks haven't completed yet' after 5 days, and trying to retest doesn't change that, what is the next step? I would like this PR to be committed if it passes tests.

eileenmcnaughton · 2016-08-01T22:18:01Z

maybe ping @totten or @mlutfy on why the test isn't kicking off?

totten · 2016-08-01T22:56:25Z

Hmm, it looks like the test ran https://test.civicrm.org/job/CiviCRM-Core-PR/10966/ which reports one failure, CRM_Contact_BAO_ContactType_ContactTest.testCRM19133. (Aside: I'm not familiar with that test, but I've seen it sporadically in the past couple days...)

JoeMurray · 2016-08-01T23:00:13Z

As well, that error seems unrelated to the code in the PR. @colemanw I think this is ready to merge.

civicrm-builder added the master label Jun 15, 2016

eileenmcnaughton reviewed Jun 15, 2016
View reviewed changes

pradpnayak force-pushed the CRM-16189-17 branch 2 times, most recently from 7261e5a to 1e9e935 Compare June 26, 2016 23:55

pradpnayak changed the title ~~CRM-16189, added code to validate financial type for contribution~~ [ready-for-core-team-review]CRM-16189, added code to validate financial type for contribution Jul 2, 2016

colemanw reviewed Jul 4, 2016
View reviewed changes

eileenmcnaughton reviewed Jul 5, 2016
View reviewed changes

Edzelopez force-pushed the CRM-16189-17 branch from 2e9f74d to 3d0cc8f Compare July 6, 2016 06:23

JoeMurray mentioned this pull request Jul 7, 2016

Invitation: CiviCRM 4.7.10 release August 2016 civicrm/release-management#4

Closed

pradpnayak and others added 5 commits July 7, 2016 15:41

CRM-16189, added code to validate financial type for contribution

3adac15

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189 Conflicts: CRM/Contribute/Form/Contribution.php

CRM-16189, highlight form field but show error in pop up

c9b6813

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

--CRM-16189, updated function name

70a9df1

CRM-16189 Changed return type from string to bool and updated functions

52d479e

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

CRM-16189 Fixed unit test

40e78f0

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

Edzelopez force-pushed the CRM-16189-17 branch from 3d0cc8f to 40e78f0 Compare July 7, 2016 11:15

JoeMurray reviewed Jul 7, 2016
View reviewed changes

CRM-16189, fixed unit test

c9a5a6e

---------------------------------------- * CRM-16189: https://issues.civicrm.org/jira/browse/CRM-16189

pradpnayak closed this Jul 16, 2016

pradpnayak deleted the CRM-16189-17 branch July 16, 2016 02:31

pradpnayak restored the CRM-16189-17 branch July 16, 2016 02:32

pradpnayak reopened this Jul 16, 2016

CRM-16189, updated the docblock and checked isnull for previous recog…

c849d76

…nition date ---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

JoeMurray reviewed Jul 20, 2016
View reviewed changes

CRM-16189, updated docblock

ab6c306

updated to code follow CiviCRM standard ---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

JoeMurray reviewed Jul 22, 2016
View reviewed changes

pradpnayak added 2 commits July 27, 2016 18:25

CRM-16189, updated function to throw exception in BAO and catch in fo…

9603974

…rm level ---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

CRM-16189, fixed test

36937e7

---------------------------------------- * CRM-16189: Improve support for Accrual Method bookkeeping https://issues.civicrm.org/jira/browse/CRM-16189

JoeMurray added the merge on pass label Aug 1, 2016

colemanw merged commit 02c14d7 into civicrm:master Aug 1, 2016

pradpnayak deleted the CRM-16189-17 branch August 2, 2016 19:55

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

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

Conversation

pradpnayak commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Edzelopez commented Jul 5, 2016

Choose a reason for hiding this comment

colemanw commented Jul 5, 2016

Edzelopez commented Jul 6, 2016 • edited Loading

colemanw commented Jul 6, 2016

Edzelopez commented Jul 7, 2016

JoeMurray commented Jul 7, 2016

colemanw commented Jul 7, 2016

Choose a reason for hiding this comment

monishdeb commented Jul 8, 2016

monishdeb commented Jul 8, 2016 • edited Loading

JoeMurray commented Jul 8, 2016

pradpnayak commented Jul 13, 2016

JoeMurray commented Jul 19, 2016

Choose a reason for hiding this comment

pradpnayak Jul 20, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeMurray commented Jul 20, 2016

JoeMurray commented Jul 20, 2016

eileenmcnaughton commented Jul 20, 2016

JoeMurray commented Jul 20, 2016

pradpnayak commented Jul 21, 2016

JoeMurray commented Jul 21, 2016

Choose a reason for hiding this comment

pradpnayak commented Jul 27, 2016

pradpnayak commented Jul 27, 2016

JoeMurray commented Aug 1, 2016

JoeMurray commented Aug 1, 2016

eileenmcnaughton commented Aug 1, 2016

totten commented Aug 1, 2016 • edited Loading

JoeMurray commented Aug 1, 2016

Edzelopez commented Jul 6, 2016 •

edited

Loading

monishdeb commented Jul 8, 2016 •

edited

Loading

totten commented Aug 1, 2016 •

edited

Loading