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

diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php
index caa9e9a22b67d48800710bb58ee7282d7741321f..da7867ad26d32c347ea5dca602945dc3939ffbc5 100644 (file)
--- 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 e16d84ea11c34f414a3bc297491bed0060fdfa77..843aa7ccc1941631d60147455b66764ccc5f93bc 100644 (file)
--- 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 133af7e0275fdb0c238105334504e55df4ca5260..0d219e8147915be257a53f597b418382edb39dda 100644 (file)
--- 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']);
   }
 
   /**