From c039f658064fd0d34307da1be38932f6f3b65a4e Mon Sep 17 00:00:00 2001 From: eileenmcnaugton Date: Fri, 23 Oct 2015 12:07:28 +1300 Subject: [PATCH] CRM-17397 fix enotices on amount_level After looking at this I think the 'right' answer is to always go to the same function to find out what the amount_level should be - rather than calculate it on the forms and / or tack it onto the function that derives the line_items I made this change on the form that was experiencing an issue & added a test. Elsewhere I just added comments --- CRM/Batch/Form/Entry.php | 8 +- CRM/Contribute/Form/AdditionalPayment.php | 4 + CRM/Contribute/Form/Contribution.php | 4 + CRM/Contribute/Form/Contribution/Confirm.php | 4 + CRM/Contribute/Form/Contribution/Main.php | 4 + CRM/Contribute/Form/ContributionBase.php | 4 + CRM/Event/Cart/Form/Checkout/Payment.php | 4 + CRM/Event/Form/Registration.php | 19 +++++ .../Registration/AdditionalParticipant.php | 9 +- CRM/Event/Form/Registration/Register.php | 7 +- CRM/Price/BAO/PriceSet.php | 85 +++++++++++++++++++ CRM/Utils/Cache.php | 2 +- .../CRM/Event/BAO/AdditionalPaymentTest.php | 53 ++---------- tests/phpunit/CRM/Price/BAO/PriceSetTest.php | 63 ++++++++++++++ tests/phpunit/CiviTest/CiviUnitTestCase.php | 47 +++++++++- 15 files changed, 257 insertions(+), 60 deletions(-) create mode 100644 tests/phpunit/CRM/Price/BAO/PriceSetTest.php diff --git a/CRM/Batch/Form/Entry.php b/CRM/Batch/Form/Entry.php index 68dafe06c2..a5a023d754 100755 --- a/CRM/Batch/Form/Entry.php +++ b/CRM/Batch/Form/Entry.php @@ -506,12 +506,18 @@ class CRM_Batch_Form_Entry extends CRM_Core_Form { $lineItem = array(); CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'], $value, $lineItem[$priceSetId]); - //unset amount level since we always use quick config price set + // @todo - stop setting amount level in this function & call the CRM_Price_BAO_PriceSet::getAmountLevel + // function to get correct amount level consistently. Remove setting of the amount level in + // CRM_Price_BAO_PriceSet::processAmount. Extend the unit tests in CRM_Price_BAO_PriceSetTest + // to cover all variants. unset($value['amount_level']); //CRM-11529 for back office transactions //when financial_type_id is passed in form, update the //line items with the financial type selected in form + // @todo - create a price set or price field per financial type & simply choose the appropriate + // price field rather than working around the fact that each price_field is supposed to have a financial + // type & we are allowing that to be overridden. if (!empty($value['financial_type_id']) && !empty($lineItem[$priceSetId])) { foreach ($lineItem[$priceSetId] as &$values) { $values['financial_type_id'] = $value['financial_type_id']; diff --git a/CRM/Contribute/Form/AdditionalPayment.php b/CRM/Contribute/Form/AdditionalPayment.php index e802fd2f11..49bd5e98a0 100644 --- a/CRM/Contribute/Form/AdditionalPayment.php +++ b/CRM/Contribute/Form/AdditionalPayment.php @@ -484,6 +484,10 @@ class CRM_Contribute_Form_AdditionalPayment extends CRM_Contribute_Form_Abstract } $this->_params['ip_address'] = CRM_Utils_System::ipAddress(); $this->_params['amount'] = $this->_params['total_amount']; + // @todo - stop setting amount level in this function & call the CRM_Price_BAO_PriceSet::getAmountLevel + // function to get correct amount level consistently. Remove setting of the amount level in + // CRM_Price_BAO_PriceSet::processAmount. Extend the unit tests in CRM_Price_BAO_PriceSetTest + // to cover all variants. $this->_params['amount_level'] = 0; $this->_params['currencyID'] = CRM_Utils_Array::value('currency', $this->_params, diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php index 7b00a75e11..b742655b09 100644 --- a/CRM/Contribute/Form/Contribution.php +++ b/CRM/Contribute/Form/Contribution.php @@ -1096,6 +1096,10 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP } $this->_params['amount'] = $this->_params['total_amount']; + // @todo - stop setting amount level in this function & call the CRM_Price_BAO_PriceSet::getAmountLevel + // function to get correct amount level consistently. Remove setting of the amount level in + // CRM_Price_BAO_PriceSet::processAmount. Extend the unit tests in CRM_Price_BAO_PriceSetTest + // to cover all variants. $this->_params['amount_level'] = 0; $this->_params['description'] = ts("Contribution submitted by a staff person using contributor's credit card"); $this->_params['currencyID'] = CRM_Utils_Array::value('currency', diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php index db2bf893c8..e0a9f95279 100644 --- a/CRM/Contribute/Form/Contribution/Confirm.php +++ b/CRM/Contribute/Form/Contribution/Confirm.php @@ -1762,6 +1762,10 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr } if ($isQuickConfig && !empty($this->_params["price_{$priceField->id}"])) { if ($this->_values['fee'][$priceField->id]['html_type'] != 'Text') { + // @todo - stop setting amount level in this function & call the CRM_Price_BAO_PriceSet::getAmountLevel + // function to get correct amount level consistently. Remove setting of the amount level in + // CRM_Price_BAO_PriceSet::processAmount. Extend the unit tests in CRM_Price_BAO_PriceSetTest + // to cover all variants. $this->_params['amount_level'] = CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceFieldValue', $this->_params["price_{$priceField->id}"], 'label'); } diff --git a/CRM/Contribute/Form/Contribution/Main.php b/CRM/Contribute/Form/Contribution/Main.php index 735544f20f..d03eb0557a 100644 --- a/CRM/Contribute/Form/Contribution/Main.php +++ b/CRM/Contribute/Form/Contribution/Main.php @@ -951,6 +951,10 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu $amountID = CRM_Utils_Array::value('amount', $params); if ($amountID) { + // @todo - stop setting amount level in this function & call the CRM_Price_BAO_PriceSet::getAmountLevel + // function to get correct amount level consistently. Remove setting of the amount level in + // CRM_Price_BAO_PriceSet::processAmount. Extend the unit tests in CRM_Price_BAO_PriceSetTest + // to cover all variants. $params['amount_level'] = CRM_Utils_Array::value('label', $formValues[$amountID]); $amount = CRM_Utils_Array::value('value', $formValues[$amountID]); } diff --git a/CRM/Contribute/Form/ContributionBase.php b/CRM/Contribute/Form/ContributionBase.php index 2da894c39f..1f11cabdea 100644 --- a/CRM/Contribute/Form/ContributionBase.php +++ b/CRM/Contribute/Form/ContributionBase.php @@ -511,6 +511,10 @@ class CRM_Contribute_Form_ContributionBase extends CRM_Core_Form { )); } + // @todo - stop setting amount level in this function & call the CRM_Price_BAO_PriceSet::getAmountLevel + // function to get correct amount level consistently. Remove setting of the amount level in + // CRM_Price_BAO_PriceSet::processAmount. Extend the unit tests in CRM_Price_BAO_PriceSetTest + // to cover all variants. if (isset($this->_params['amount_other']) || isset($this->_params['selectMembership'])) { $this->_params['amount_level'] = ''; } diff --git a/CRM/Event/Cart/Form/Checkout/Payment.php b/CRM/Event/Cart/Form/Checkout/Payment.php index bbbac759d9..4a61f11a24 100644 --- a/CRM/Event/Cart/Form/Checkout/Payment.php +++ b/CRM/Event/Cart/Form/Checkout/Payment.php @@ -254,6 +254,10 @@ class CRM_Event_Cart_Form_Checkout_Payment extends CRM_Event_Cart_Form_Cart { $price_set_amount = array(); CRM_Price_BAO_PriceSet::processAmount($price_set['fields'], $event_price_values, $price_set_amount); $cost = $event_price_values['amount']; + // @todo - stop setting amount level in this function & call the CRM_Price_BAO_PriceSet::getAmountLevel + // function to get correct amount level consistently. Remove setting of the amount level in + // CRM_Price_BAO_PriceSet::processAmount. Extend the unit tests in CRM_Price_BAO_PriceSetTest + // to cover all variants. $amount_level = $event_price_values['amount_level']; $price_details[$price_set_id] = $price_set_amount; } diff --git a/CRM/Event/Form/Registration.php b/CRM/Event/Form/Registration.php index 3f858c7683..c7a2049780 100644 --- a/CRM/Event/Form/Registration.php +++ b/CRM/Event/Form/Registration.php @@ -1499,4 +1499,23 @@ WHERE v.option_group_id = g.id } } + /** + * Get the amount level for the event payment. + * + * The amount level is the string stored on the contribution record that describes the purchase. + * + * @param array $params + * @param int|null $discountID + * + * @return string + */ + protected function getAmountLevel($params, $discountID) { + // @todo move handling of discount ID to the BAO function - preferably by converting it to a price_set with + // time settings. + if (!empty($this->_values['discount'][$discountID])) { + return $this->_values['discount'][$discountID][$params['amount']]['label']; + } + return CRM_Price_BAO_PriceSet::getAmountLevelText($params); + } + } diff --git a/CRM/Event/Form/Registration/AdditionalParticipant.php b/CRM/Event/Form/Registration/AdditionalParticipant.php index 9aadd5978d..2a639c172a 100644 --- a/CRM/Event/Form/Registration/AdditionalParticipant.php +++ b/CRM/Event/Form/Registration/AdditionalParticipant.php @@ -30,13 +30,10 @@ * * @package CRM * @copyright CiviCRM LLC (c) 2004-2015 - * $Id$ - * */ /** - * This class generates form components for processing Event - * + * This class generates form components for processing Event. */ class CRM_Event_Form_Registration_AdditionalParticipant extends CRM_Event_Form_Registration { @@ -662,14 +659,12 @@ class CRM_Event_Form_Registration_AdditionalParticipant extends CRM_Event_Form_R //added for discount $discountId = CRM_Core_BAO_Discount::findSet($this->_eventId, 'civicrm_event'); - + $params['amount_level'] = $this->getAmountLevel($params, $discountId); if (!empty($this->_values['discount'][$discountId])) { $params['discount_id'] = $discountId; - $params['amount_level'] = $this->_values['discount'][$discountId][$params['amount']]['label']; $params['amount'] = $this->_values['discount'][$discountId][$params['amount']]['value']; } elseif (empty($params['priceSetId'])) { - $params['amount_level'] = $this->_values['fee'][$params['amount']]['label']; $params['amount'] = $this->_values['fee'][$params['amount']]['value']; } else { diff --git a/CRM/Event/Form/Registration/Register.php b/CRM/Event/Form/Registration/Register.php index 988190f01c..498e1b3dde 100644 --- a/CRM/Event/Form/Registration/Register.php +++ b/CRM/Event/Form/Registration/Register.php @@ -1024,20 +1024,17 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration { //added for discount $discountId = CRM_Core_BAO_Discount::findSet($this->_eventId, 'civicrm_event'); - + $params['amount_level'] = $this->getAmountLevel($params, $discountId); if (!empty($this->_values['discount'][$discountId])) { $params['discount_id'] = $discountId; - $params['amount_level'] = $this->_values['discount'][$discountId][$params['amount']]['label']; - $params['amount'] = $this->_values['discount'][$discountId][$params['amount']]['value']; } elseif (empty($params['priceSetId'])) { if (!empty($params['amount'])) { - $params['amount_level'] = $this->_values['fee'][$params['amount']]['label']; $params['amount'] = $this->_values['fee'][$params['amount']]['value']; } else { - $params['amount_level'] = $params['amount'] = ''; + $params['amount'] = ''; } } else { diff --git a/CRM/Price/BAO/PriceSet.php b/CRM/Price/BAO/PriceSet.php index 25429bae88..27c979e753 100644 --- a/CRM/Price/BAO/PriceSet.php +++ b/CRM/Price/BAO/PriceSet.php @@ -904,6 +904,7 @@ WHERE id = %1"; // But, in the interests of being careful when capacity is low - avoiding the known default value // will get us by. // Crucially a test has been added so a better solution can be implemented later with some comfort. + // @todo - stop setting amount level in this function & call the getAmountLevel function to retrieve it. if ($values['label'] != ts('Contribution Amount')) { $amount_level[] = $values['label'] . ' - ' . (float) $values['qty']; } @@ -914,6 +915,7 @@ WHERE id = %1"; if ($totalParticipant > 0) { $displayParticipantCount = ' Participant Count -' . $totalParticipant; } + // @todo - stop setting amount level in this function & call the getAmountLevel function to retrieve it. if (!empty($amount_level) && !empty($displayParticipantCount)) { $params['amount_level'] = CRM_Core_DAO::VALUE_SEPARATOR . implode(CRM_Core_DAO::VALUE_SEPARATOR, $amount_level) . $displayParticipantCount . CRM_Core_DAO::VALUE_SEPARATOR; } @@ -931,6 +933,89 @@ WHERE id = %1"; } } + /** + * Get the text to record for amount level. + * + * @param array $params + * Submitted parameters + * - priceSetId is required to be set in the calling function + * (we don't e-notice check it to enforce that - all payments DO have a price set - even if it is the + * default one & this function asks that be set if it is the case). + * + * @return string + * Text for civicrm_contribution.amount_level field. + */ + public static function getAmountLevelText($params) { + $priceSetID = $params['priceSetId']; + $priceFieldSelection = self::filterPriceFieldsFromParams($priceSetID, $params); + $priceFieldMetadata = self::getCachedPriceSetDetail($priceSetID); + $displayParticipantCount = null; + $amount_level = array(); + foreach ($priceFieldMetadata['fields'] as $field) { + if (!empty($priceFieldSelection[$field['id']])) { + if ($field['is_enter_qty']) { + $qtyString = ' - ' . (float) $params['price_' . $field['id']]; + } + // We deliberately & specifically exclude contribution amount as it has a specific meaning. + // ie. it represents the default price field for a contribution. Another approach would be not + // to give it a label if we don't want it to show. + if ($field['label'] != ts('Contribution Amount')) { + $amount_level[] = $field['label'] . $qtyString; + } + } + } + return CRM_Core_DAO::VALUE_SEPARATOR . implode(CRM_Core_DAO::VALUE_SEPARATOR, $amount_level) . $displayParticipantCount . CRM_Core_DAO::VALUE_SEPARATOR; + } + + /** + * Get the fields relevant to the price field from the parameters. + * + * E.g we are looking for price_5 => 7 out of a big array of input parameters. + * + * @param int $priceSetID + * @param array $params + * + * @return array + * Price fields found in the params array + */ + public static function filterPriceFieldsFromParams($priceSetID, $params) { + $priceSet = self::getCachedPriceSetDetail($priceSetID); + $return = array(); + foreach ($priceSet['fields'] as $field) { + if (!empty($params['price_' . $field['id']])) { + $return[$field['id']] = $params['price_' . $field['id']]; + } + } + return $return; + } + + /** + * Wrapper for getSetDetail with caching. + * + * We seem to be passing this array around in a painful way - presumably to avoid the hit + * of loading it - so lets make it callable with caching. + * + * Why not just add caching to the other function? We could do - it just seemed a bit unclear the best caching pattern + * & the function was already pretty fugly. Also, I feel like we need to migrate the interaction with price-sets into + * a more granular interaction - ie. retrieve specific data using specific functions on this class & have the form + * think less about the price sets. + * + * @param int $priceSetID + * + * @return array + */ + public static function getCachedPriceSetDetail($priceSetID) { + $cacheKey = __CLASS__ . __FUNCTION__ . '_' . $priceSetID; + $cache = CRM_Utils_Cache::singleton(); + $values = (array) $cache->get($cacheKey); + if (empty($values)) { + $data = self::getSetDetail($priceSetID); + $values = $data[$priceSetID]; + $cache->set($cacheKey, $values); + } + return $values; + } + /** * Build the price set form. * diff --git a/CRM/Utils/Cache.php b/CRM/Utils/Cache.php index 47f2a2815c..ff40e8ad8f 100644 --- a/CRM/Utils/Cache.php +++ b/CRM/Utils/Cache.php @@ -58,7 +58,7 @@ class CRM_Utils_Cache { /** * Singleton function used to manage this object. * - * @return object + * @return CRM_Utils_Cache_Interface */ public static function &singleton() { if (self::$_singleton === NULL) { diff --git a/tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php b/tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php index 49f5c6fd56..fd27ea81b4 100644 --- a/tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php +++ b/tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php @@ -60,54 +60,17 @@ class CRM_Event_BAO_AdditionalPaymentTest extends CiviUnitTestCase { } /** - * helper function to record participant with paid contribution. - * @param $feeTotal - * @param $actualPaidAmt + * Helper function to record participant with paid contribution. + * + * @param int $feeTotal + * @param int $actualPaidAmt * * @return array * @throws Exception */ - public function _addParticipantWithPayment($feeTotal, $actualPaidAmt) { - // creating price set, price field - $paramsSet['title'] = 'Price Set'; - $paramsSet['name'] = CRM_Utils_String::titleToVar('Price Set'); - $paramsSet['is_active'] = FALSE; - $paramsSet['extends'] = 1; - - $priceset = CRM_Price_BAO_PriceSet::create($paramsSet); - CRM_Price_BAO_PriceSet::addTo('civicrm_event', $this->_eventId, $priceset->id); - $priceSetId = $priceset->id; - - //Checking for priceset added in the table. - $this->assertDBCompareValue('CRM_Price_BAO_PriceSet', $priceSetId, 'title', - 'id', $paramsSet['title'], 'Check DB for created priceset' - ); - $paramsField = array( - 'label' => 'Price Field', - 'name' => CRM_Utils_String::titleToVar('Price Field'), - 'html_type' => 'Text', - 'price' => $feeTotal, - 'option_label' => array('1' => 'Price Field'), - 'option_value' => array('1' => $feeTotal), - 'option_name' => array('1' => $feeTotal), - 'option_weight' => array('1' => 1), - 'option_amount' => array('1' => 1), - 'is_display_amounts' => 1, - 'weight' => 1, - 'options_per_line' => 1, - 'is_active' => array('1' => 1), - 'price_set_id' => $priceset->id, - 'is_enter_qty' => 1, - 'financial_type_id' => CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_FinancialType', 'Event Fee', 'id', 'name'), - ); - - $ids = array(); - $pricefield = CRM_Price_BAO_PriceField::create($paramsField, $ids); - - //Checking for priceset added in the table. - $this->assertDBCompareValue('CRM_Price_BAO_PriceField', $pricefield->id, 'label', - 'id', $paramsField['label'], 'Check DB for created pricefield' - ); + protected function addParticipantWithPayment($feeTotal, $actualPaidAmt) { + $priceSetId = $this->eventPriceSetCreate($feeTotal); + CRM_Price_BAO_PriceSet::addTo('civicrm_event', $this->_eventId, $priceSetId); // create participant record $eventId = $this->_eventId; @@ -174,7 +137,7 @@ class CRM_Event_BAO_AdditionalPaymentTest extends CiviUnitTestCase { $feeAmt = 100; $amtPaid = 60; $balance = $feeAmt - $amtPaid; - list($participant, $contribution) = $this->_addParticipantWithPayment($feeAmt, $amtPaid); + list($participant, $contribution) = $this->addParticipantWithPayment($feeAmt, $amtPaid); $paymentInfo = CRM_Contribute_BAO_Contribution::getPaymentInfo($participant['id'], 'event'); // amount checking diff --git a/tests/phpunit/CRM/Price/BAO/PriceSetTest.php b/tests/phpunit/CRM/Price/BAO/PriceSetTest.php new file mode 100644 index 0000000000..b751056ae3 --- /dev/null +++ b/tests/phpunit/CRM/Price/BAO/PriceSetTest.php @@ -0,0 +1,63 @@ +eventPriceSetCreate(9); + $priceSet = CRM_Price_BAO_PriceSet::getCachedPriceSetDetail($priceSetID); + $field = reset($priceSet['fields']); + $params = array('priceSetId' => $priceSetID, 'price_' . $field['id'] => 1); + $amountLevel = CRM_Price_BAO_PriceSet::getAmountLevelText($params); + $this->assertEquals(CRM_Core_DAO::VALUE_SEPARATOR . 'Price Field - 1' . CRM_Core_DAO::VALUE_SEPARATOR, $amountLevel); + } + +} diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 352d6d68fe..3744a28cf8 100755 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -1721,7 +1721,7 @@ class CiviUnitTestCase extends PHPUnit_Extensions_Database_TestCase { 'registration_end_date' => 20081015, 'max_participants' => 100, 'event_full_text' => 'Sorry! We are already full', - 'is_monetory' => 0, + 'is_monetary' => 0, 'is_active' => 1, 'is_show_location' => 0, ), $params); @@ -3331,4 +3331,49 @@ AND ( TABLE_NAME LIKE 'civicrm_value_%' ) )); } + /** + * Create a price set for an event. + * + * @param int $feeTotal + * + * @return int + * Price Set ID. + */ + protected function eventPriceSetCreate($feeTotal) { + // creating price set, price field + $paramsSet['title'] = 'Price Set'; + $paramsSet['name'] = CRM_Utils_String::titleToVar('Price Set'); + $paramsSet['is_active'] = FALSE; + $paramsSet['extends'] = 1; + + $priceset = CRM_Price_BAO_PriceSet::create($paramsSet); + $priceSetId = $priceset->id; + + //Checking for priceset added in the table. + $this->assertDBCompareValue('CRM_Price_BAO_PriceSet', $priceSetId, 'title', + 'id', $paramsSet['title'], 'Check DB for created priceset' + ); + $paramsField = array( + 'label' => 'Price Field', + 'name' => CRM_Utils_String::titleToVar('Price Field'), + 'html_type' => 'Text', + 'price' => $feeTotal, + 'option_label' => array('1' => 'Price Field'), + 'option_value' => array('1' => $feeTotal), + 'option_name' => array('1' => $feeTotal), + 'option_weight' => array('1' => 1), + 'option_amount' => array('1' => 1), + 'is_display_amounts' => 1, + 'weight' => 1, + 'options_per_line' => 1, + 'is_active' => array('1' => 1), + 'price_set_id' => $priceset->id, + 'is_enter_qty' => 1, + 'financial_type_id' => CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_FinancialType', 'Event Fee', 'id', 'name'), + ); + CRM_Price_BAO_PriceField::create($paramsField); + + return $priceSetId; + } + } -- 2.25.1