CRM-21026 fix issue with ContributionCount not including disabled fin… #10824

seamuslee001 · 2017-08-06T22:56:40Z

…ancialtypes

Overview

At present when calling ContributionCount it by default due to the addition of is_active = 1 clause in the pseduoConstant function excludes contributions with Disabled Financial Types

Before

Contribution count on tab is incorrect

After

Contribution count is correct

Technical Details

If the PR introduces noteworthy technical changes, please describe them here. Provide code snippets if necessary

Comments

seamuslee001 · 2017-08-06T22:57:22Z

@eileenmcnaughton @monishdeb @JoeMurray @ineffyble does this look right to all of you? nb i'll fix the CRM number in the new wrapper functions docblocks

…ancialtypes Fix references and move flushing of cache to all financialTypes for the safe side

eileenmcnaughton · 2017-08-07T00:32:32Z

CRM/Contribute/BAO/Contribution.php

@@ -604,8 +604,8 @@ public static function create(&$params, $ids = array()) {
      if ($retrieveRequired == 1) {
        $contribution->find(TRUE);
      }
-      $contributionTypes = CRM_Contribute_PseudoConstant::financialType();
-      $title = CRM_Contact_BAO_Contact::displayName($contribution->contact_id) . ' - (' . CRM_Utils_Money::format($contribution->total_amount, $contribution->currency) . ' ' . ' - ' . $contributionTypes[$contribution->financial_type_id] . ')';
+      $contributionType = CRM_Contribute_PseudoConstant::financialType($contribution->financial_type_id);


we should rename to financialType when touching fields like this

eileenmcnaughton · 2017-08-07T00:34:02Z

CRM/Financial/BAO/FinancialType.php

+   * CRM-21026
+   * Wrapper aroung getAvaliableFinancialTypes to get all including disabled FinancialTypes
+   * @param array $financialTypes
+   *   (reference ) an array of financial types


passing as reference is done badly in Civi code so I discourage it. In this case it is also returned & not required & in fact I think the parameter should not be pass in at all but rather initiated as an empty array within the function

@eileenmcnaughton hmm i'll see what i can do

eileenmcnaughton · 2017-08-07T00:34:25Z

CRM/Financial/BAO/FinancialType.php

+   *
+   * @return array
+   */
+  public static function getAllEnabledAvailableFinancialTypes(&$financialTypes = NULL, $action = CRM_Core_Action::VIEW, $resetCache = FALSE) {


same comments about financialTypes

seamuslee001 · 2017-08-07T01:10:23Z

@eileenmcnaughton i have i think managed to get rid of the passing by reference for the new functions but the old function that we are wrapping around will be difficult to remove the passing by reference

These are all the places its used

api/v3/LineItem.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types, CRM_Core_Action::DELETE);
api/v3/Contribution.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types, $op);
CRM/Member/Form/MembershipType.php:      array('' => ts('- select -')) + CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, $this->_action), TRUE, array('class' => 'crm-select2')
CRM/Member/Form/Membership.php:      array('' => ts('- select -')) + CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, $this->_action)
CRM/Member/BAO/Membership.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::ADD);
CRM/Financial/BAO/FinancialType.php:    $fTypes = self::getAvailableFinancialTypes($financialTypes, $action, $resetCache, TRUE);
CRM/Financial/BAO/FinancialType.php:    $fTypes = self::getAvailableFinancialTypes($financialTypes, $action, $resetCache);
CRM/Financial/BAO/FinancialType.php:  public static function getAvailableFinancialTypes(&$financialTypes = NULL, $action = CRM_Core_Action::VIEW, $resetCache = FALSE, $includeDisabled = FALSE) {
CRM/Financial/BAO/FinancialType.php:        self::getAvailableFinancialTypes($types);
CRM/Financial/BAO/FinancialType.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::UPDATE);
CRM/Event/Form/ManageEvent/Fee.php:      CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::ADD);
CRM/Event/Form/EventFees.php:          CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, $form->_action);
CRM/Contribute/Form/ContributionPage/Settings.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::ADD);
CRM/Contribute/Form/ContributionPage/AddProduct.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialType, CRM_Core_Action::ADD);
CRM/Contribute/Form/Contribution.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, $this->_action);
CRM/Contribute/BAO/Query.php:        CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Contribute/BAO/Query.php:        CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Contribute/BAO/Query.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::VIEW);
CRM/Contribute/BAO/Premium.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::ADD);
CRM/Contribute/BAO/Contribution.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Contribute/BAO/Contribution.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Contact/BAO/Query.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Report/Form/Contribute/Detail.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/Repeat.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/Recur.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/Lybunt.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/Bookkeeping.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/Summary.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/TopDonor.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/SoftCredit.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form/Contribute/Sybunt.php:            'options' => CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes(),
CRM/Report/Form.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Price/Page/Field.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Price/BAO/PriceFieldValue.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Price/BAO/PriceFieldValue.php:      CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
CRM/Price/BAO/LineItem.php:      CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types, $op);
CRM/Price/BAO/LineItem.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types);
CRM/Price/BAO/PriceSet.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialType, CRM_Core_Action::ADD);
tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types);
tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php:    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types);

ineffyble · 2017-08-07T01:15:12Z

Does this fix https://issues.civicrm.org/jira/browse/CRM-17773 ?

If not, this would be a good opportunity to address it.

eileenmcnaughton · 2017-08-07T01:20:18Z

@seamuslee001 yeah the old function needs to stay out of scope !

eileenmcnaughton · 2017-08-07T01:21:30Z

CRM/Financial/BAO/FinancialType.php

+   *
+   * @return array
+   */
+  public static function getAllAvailableFinancialTypes($financialTypes = NULL, $action = CRM_Core_Action::VIEW, $resetCache = FALSE) {


I don't think we should pass in $financialTypes at all - this function should do what it says & get all

eileenmcnaughton · 2017-08-07T01:21:38Z

CRM/Financial/BAO/FinancialType.php

+   *
+   * @return array
+   */
+  public static function getAllEnabledAvailableFinancialTypes($financialTypes = NULL, $action = CRM_Core_Action::VIEW, $resetCache = FALSE) {


eileenmcnaughton · 2017-08-07T01:21:57Z

CRM/Financial/BAO/FinancialType.php

+  }
+
+  /**
+   * CRM-21026


I don't think we need the CRM here - if we do it should be after the main descriptor

seamuslee001 · 2017-08-07T06:32:41Z

@eileenmcnaughton i think i have tidied it up now

seamuslee001 · 2017-08-07T06:33:29Z

CRM/Financial/BAO/FinancialType.php

+  public static function getAllAvailableFinancialTypes($action = CRM_Core_Action::VIEW, $resetCache = FALSE) {
+    // Flush pseudoconstant cache
+    CRM_Contribute_PseudoConstant::flush('financialType');
+    $thisIsAUselessVariableButSolvesPHPError = NULL;


Note that this is needed because PHP7.1 especially will only allow for variables to be passed by reference

Note spelling error on line 249: "getAvaliableFinancialTypes" s/b "getAvailableFinancialTypes" (also on line 266 below).

Thanks @laryn fixed that

seamuslee001 · 2017-08-07T06:33:38Z

CRM/Financial/BAO/FinancialType.php

+   * @return array
+   */
+  public static function getAllEnabledAvailableFinancialTypes($action = CRM_Core_Action::VIEW, $resetCache = FALSE) {
+    $thisIsAUselessVariableButSolvesPHPError = NULL;


Same Comment as L260

seamuslee001 · 2017-08-07T06:34:17Z

@ineffyble I think this will provide the foundations for fixing CRM-17773

seamuslee001 · 2017-08-07T08:36:22Z

Jenkins re test this please

…h changing to new wrapper functions

seamuslee001 · 2017-08-07T21:32:38Z

@monishdeb @eileenmcnaughton i think this is ready for merging has been tested by Laryn

eileenmcnaughton · 2017-08-09T00:03:28Z

I'm happy with the code approach & happy @laryn has tested this, so am merging.

CRM-21026 fix issue with ContributionCount not including disabled fin…

d73f286

…ancialtypes Fix references and move flushing of cache to all financialTypes for the safe side

seamuslee001 force-pushed the CRM-21026 branch from 614676d to d73f286 Compare August 6, 2017 23:39

eileenmcnaughton reviewed Aug 7, 2017

View reviewed changes

Attempt to get rid of pasing by reference and change variable as needed

d51d109

eileenmcnaughton reviewed Aug 7, 2017

View reviewed changes

Code updates as per Eileen

354355b

seamuslee001 commented Aug 7, 2017

View reviewed changes

Minor comment fixes and further reduction of passing by reference wit…

160a7ef

…h changing to new wrapper functions

eileenmcnaughton merged commit ded3543 into civicrm:master Aug 9, 2017

eileenmcnaughton deleted the CRM-21026 branch August 9, 2017 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21026 fix issue with ContributionCount not including disabled fin… #10824

CRM-21026 fix issue with ContributionCount not including disabled fin… #10824

seamuslee001 commented Aug 6, 2017

seamuslee001 commented Aug 6, 2017

eileenmcnaughton Aug 7, 2017

seamuslee001 Aug 7, 2017

eileenmcnaughton Aug 7, 2017

seamuslee001 Aug 7, 2017

eileenmcnaughton Aug 7, 2017

seamuslee001 commented Aug 7, 2017

ineffyble commented Aug 7, 2017

eileenmcnaughton commented Aug 7, 2017

eileenmcnaughton Aug 7, 2017

eileenmcnaughton Aug 7, 2017

eileenmcnaughton Aug 7, 2017

seamuslee001 commented Aug 7, 2017

seamuslee001 Aug 7, 2017

laryn Aug 7, 2017 •

edited

Loading

seamuslee001 Aug 7, 2017

seamuslee001 Aug 7, 2017

seamuslee001 commented Aug 7, 2017

seamuslee001 commented Aug 7, 2017

seamuslee001 commented Aug 7, 2017

eileenmcnaughton commented Aug 9, 2017

CRM-21026 fix issue with ContributionCount not including disabled fin… #10824

CRM-21026 fix issue with ContributionCount not including disabled fin… #10824

Conversation

seamuslee001 commented Aug 6, 2017

Overview

Before

After

Technical Details

Comments

seamuslee001 commented Aug 6, 2017

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

seamuslee001 commented Aug 7, 2017

ineffyble commented Aug 7, 2017

eileenmcnaughton commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Aug 7, 2017

Choose a reason for hiding this comment

laryn Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Aug 7, 2017

seamuslee001 commented Aug 7, 2017

seamuslee001 commented Aug 7, 2017

eileenmcnaughton commented Aug 9, 2017

laryn Aug 7, 2017 •

edited

Loading