From 2b3088183009979c064378f2eb5d5d756e52fa19 Mon Sep 17 00:00:00 2001 From: Dave Greenberg Date: Mon, 5 Oct 2015 14:11:35 -0700 Subject: [PATCH] CRM-17340 - Fix handling of Amount label for back-office quick config contributions. ---------------------------------------- * CRM-17340: Fix unexpected values stored in total_amount field for default price set https://issues.civicrm.org/jira/browse/CRM-17340 --- CRM/Contribute/Form/Contribution.php | 17 +++++++++++++- CRM/Price/BAO/PriceSet.php | 22 +++++++++++++++++-- .../CRM/Contribute/Form/ContributionTest.php | 4 +++- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php index caa9e9a22b..da7867ad26 100644 --- a/CRM/Contribute/Form/Contribution.php +++ b/CRM/Contribute/Form/Contribution.php @@ -1376,6 +1376,8 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP $lineID = key($line); $priceSetId = CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceField', CRM_Utils_Array::value('price_field_id', $line[$lineID]), 'price_set_id'); $quickConfig = CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceSet', $priceSetId, 'is_quick_config'); + // Why do we do this? Seems like a like a wrapper for old functionality - but single line price sets & quick + // config should be treated the same. if ($quickConfig) { CRM_Price_BAO_LineItem::deleteLineItems($this->_id, 'civicrm_contribution'); } @@ -1393,11 +1395,15 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP $submittedValues['price_' . $fieldID] = 1; } + // Every contribution has a price-set - the only reason it shouldn't be set is if we are dealing with + // quick config (very very arguably) & yet we see that this could still be quick config so this should be understood + // as a point of fragility rather than a logical 'if' clause. if ($priceSetId) { CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'], $submittedValues, $lineItem[$priceSetId]); - // Unset tax amount for offline 'is_quick_config' contribution. + // @todo WHY - quick config was conceived as a quick way to configure contribution forms. + // this is an example of 'other' functionality being hung off it. if ($this->_priceSet['is_quick_config'] && !array_key_exists($submittedValues['financial_type_id'], CRM_Core_PseudoConstant::getTaxRates()) ) { @@ -1427,8 +1433,12 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP } } } + if (!$priceSetId && !empty($submittedValues['total_amount']) && $this->_id) { // CRM-10117 update the line items for participants. + // @todo - if we are completing a contribution then the api call + // civicrm_api3('Contribution', 'completetransaction') should take care of + // all associated updates rather than replicating them on the form layer. if ($pId) { $entityTable = 'participant'; $entityID = $pId; @@ -1456,6 +1466,8 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP $this->_priceSetId = CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceField', $lineItems[$itemId]['price_field_id'], 'price_set_id'); } + // @todo see above - new functionality has been inappropriately added to the quick config concept + // and new functionality has been added onto the form layer rather than the BAO :-( if ($this->_priceSetId && CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceSet', $this->_priceSetId, 'is_quick_config')) { //CRM-16833: Ensure tax is applied only once for membership conribution, when status changed.(e.g Pending to Completed). $componentDetails = CRM_Contribute_BAO_Contribution::getComponentDetails($this->_id); @@ -1493,6 +1505,9 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP //CRM-11529 for quick config back office transactions //when financial_type_id is passed in form, update the //line items with the financial type selected in form + // NOTE that this IS still a legitimate use of 'quick-config' for contributions under the current DB but + // we should look at having a price field per contribution type & then there would be little reason + // for the back-office contribution form postProcess to know if it is a quick-config form. if ($isQuickConfig && !empty($submittedValues['financial_type_id']) && CRM_Utils_Array::value($this->_priceSetId, $lineItem) ) { foreach ($lineItem[$this->_priceSetId] as &$values) { diff --git a/CRM/Price/BAO/PriceSet.php b/CRM/Price/BAO/PriceSet.php index e16d84ea11..843aa7ccc1 100644 --- a/CRM/Price/BAO/PriceSet.php +++ b/CRM/Price/BAO/PriceSet.php @@ -880,7 +880,23 @@ WHERE id = %1"; if (is_array($lineItem)) { foreach ($lineItem as $values) { $totalParticipant += $values['participant_count']; - $amount_level[] = $values['label'] . ' - ' . (float) $values['qty']; + // This is a bit nasty. The logic of 'quick config' was because price set configuration was + // (and still is) too difficult to replace the 'quick config' price set configuration on the contribution + // page. + // + // However, because the quick config concept existed all sorts of logic was hung off it + // and function behaviour sometimes depends on whether 'price set' is set - although actually it + // is always set at the functional level. In this case we are dealing with the default 'quick config' + // price set having a label of 'Contribution Amount' which could wind up creating a 'funny looking' label. + // The correct answer is probably for it to have an empty label in the DB - the label is never shown so it is a + // place holder. + // + // 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. + if ($values['label'] != ts('Contribution Amount')) { + $amount_level[] = $values['label'] . ' - ' . (float) $values['qty']; + } } } @@ -888,7 +904,9 @@ WHERE id = %1"; if ($totalParticipant > 0) { $displayParticipantCount = ' Participant Count -' . $totalParticipant; } - $params['amount_level'] = CRM_Core_DAO::VALUE_SEPARATOR . implode(CRM_Core_DAO::VALUE_SEPARATOR, $amount_level) . $displayParticipantCount . CRM_Core_DAO::VALUE_SEPARATOR; + 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; + } $params['amount'] = CRM_Utils_Money::format($totalPrice, NULL, NULL, TRUE); $params['tax_amount'] = $totalTax; if ($component) { diff --git a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php index 133af7e027..0d219e8147 100644 --- a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php @@ -159,7 +159,8 @@ class CRM_Contribute_Form_ContributionTest extends CiviUnitTestCase { 'contribution_status_id' => 1, ), CRM_Core_Action::ADD); - $this->callAPISuccessGetCount('Contribution', array('contact_id' => $this->_individualId), 1); + $contribution = $this->callAPISuccessGetSingle('Contribution', array('contact_id' => $this->_individualId)); + $this->assertEmpty($contribution['amount_level']); } /** @@ -289,6 +290,7 @@ class CRM_Contribute_Form_ContributionTest extends CiviUnitTestCase { $this->assertEquals(.08, $contribution['fee_amount']); $this->assertEquals(49.92, $contribution['net_amount']); $this->assertEquals('tx', $contribution['trxn_id']); + $this->assertEmpty($contribution['amount_level']); } /** -- 2.25.1