From: eileen Date: Fri, 22 Nov 2019 07:23:48 +0000 (+1300) Subject: [REF] Move wrangling of Front end form contribution param for autoRenew back to form X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=bdcbbfeaed0a0adf5384509c3dd311e5a7af1b55;p=civicrm-core.git [REF] Move wrangling of Front end form contribution param for autoRenew back to form The processAmount function is really problematic because it's trying to do several disparate things & is called from all over the place with unclear intent. From looking at it I see it does a few things. It reformats line items, it does some obscure & likely flawed filtering, it generates a total cost & a tax cost and it generates a very specifically formatted array of autoreneal properties, The way I see this going is 1) Move the formatting of the autorenewal back to the calling form (this PR) & simplify it 2) Split out the foreach so it goes through once & formats - this can be shoved out to a separate function - and then it goes through the formatted array & calculates total_amount & tax_amount - we should have a wrapper function that just returns these & we might see that is most of what is needed 3) Move all that awful partial_payment_total stuff back to the event form. Note that we are working to entirely remove that from here are it makes so much less than no sense. 4) Calculates amount_level text - that should have it's own function. It worth noting all of this does very little intensive work - a DB lookup or 2 that could be cached & an iteration through a very small array so it would be fine to have 3 functions - - getAmountTotal - getTaxTotal - getAmountText that each go through the same process of generating a formatted array from price_x => 5 etc rather than trying to pass the array around for 'performance' or to 'save work'. From previous refactorings I would suggest we add an Order class where by you set the price fields & then you can call 'getLineItems' - but that is a few steps after this.... --- diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php index a668fae830..880dd56e71 100644 --- a/CRM/Contribute/Form/Contribution.php +++ b/CRM/Contribute/Form/Contribution.php @@ -1380,7 +1380,7 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP // as a point of fragility rather than a logical 'if' clause. if ($priceSetId) { CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'], - $submittedValues, $lineItem[$priceSetId], NULL, $priceSetId); + $submittedValues, $lineItem[$priceSetId], $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. diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php index dbb5e45ca4..4d9534b811 100644 --- a/CRM/Contribute/Form/Contribution/Confirm.php +++ b/CRM/Contribute/Form/Contribution/Confirm.php @@ -1967,7 +1967,7 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr } $priceFields = $priceFields[$priceSetID]['fields']; $lineItems = []; - CRM_Price_BAO_PriceSet::processAmount($priceFields, $paramsProcessedForForm, $lineItems, 'civicrm_contribution', $priceSetID); + $form->processAmountAndGetAutoRenew($priceFields, $paramsProcessedForForm, $lineItems, $priceSetID); $form->_lineItem = [$priceSetID => $lineItems]; $membershipPriceFieldIDs = []; foreach ((array) $lineItems as $lineItem) { diff --git a/CRM/Contribute/Form/Contribution/Main.php b/CRM/Contribute/Form/Contribution/Main.php index 2e102f83ee..6f7d403abc 100644 --- a/CRM/Contribute/Form/Contribution/Main.php +++ b/CRM/Contribute/Form/Contribution/Main.php @@ -1124,12 +1124,14 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu } } } - $component = ''; + if ($this->_membershipBlock) { - $component = 'membership'; + $this->processAmountAndGetAutoRenew($this->_values['fee'], $params, $lineItem[$priceSetId], $priceSetId); + } + else { + CRM_Price_BAO_PriceSet::processAmount($this->_values['fee'], $params, $lineItem[$priceSetId], $priceSetId); } - CRM_Price_BAO_PriceSet::processAmount($this->_values['fee'], $params, $lineItem[$priceSetId], $component, $priceSetId); if ($params['tax_amount']) { $this->set('tax_amount', $params['tax_amount']); } diff --git a/CRM/Contribute/Form/ContributionBase.php b/CRM/Contribute/Form/ContributionBase.php index 5dc6a299ff..0eaf2fbb0e 100644 --- a/CRM/Contribute/Form/ContributionBase.php +++ b/CRM/Contribute/Form/ContributionBase.php @@ -1403,4 +1403,33 @@ class CRM_Contribute_Form_ContributionBase extends CRM_Core_Form { return $params['amount'] ?? 0; } + /** + * Wrapper for processAmount that also sets autorenew. + * + * @param $fields + * This is the output of the function CRM_Price_BAO_PriceSet::getSetDetail($priceSetID, FALSE, FALSE); + * And, it would make sense to introduce caching into that function and call it from here rather than + * require the $fields array which is passed from pillar to post around the form in order to pass it in here. + * @param array $params + * Params reflecting form input e.g with fields 'price_5' => 7, 'price_8' => array(7, 8) + * @param $lineItems + * Line item array to be altered. + * @param int $priceSetID + */ + public function processAmountAndGetAutoRenew($fields, &$params, &$lineItems, $priceSetID = NULL) { + CRM_Price_BAO_PriceSet::processAmount($fields, $params, $lineItems, $priceSetID); + $autoRenew = []; + $autoRenew[0] = $autoRenew[1] = $autoRenew[2] = 0; + foreach ($lineItems as $lineItem) { + if (!empty($lineItem['auto_renew']) && + is_numeric($lineItem['auto_renew']) + ) { + $autoRenew[$lineItem['auto_renew']] += $lineItem['line_total']; + } + } + if (count($autoRenew) > 1) { + $params['autoRenew'] = $autoRenew; + } + } + } diff --git a/CRM/Event/Form/Registration/Confirm.php b/CRM/Event/Form/Registration/Confirm.php index fbe1a07ce2..4ea74efa92 100644 --- a/CRM/Event/Form/Registration/Confirm.php +++ b/CRM/Event/Form/Registration/Confirm.php @@ -375,6 +375,9 @@ class CRM_Event_Form_Registration_Confirm extends CRM_Event_Form_Registration { /** * Process the form submission. + * + * @throws \CiviCRM_API3_Exception + * @throws \CRM_Core_Exception */ public function postProcess() { $now = date('YmdHis'); diff --git a/CRM/Member/Form/Membership.php b/CRM/Member/Form/Membership.php index 848c8ecd99..07951efc4c 100644 --- a/CRM/Member/Form/Membership.php +++ b/CRM/Member/Form/Membership.php @@ -1153,7 +1153,7 @@ class CRM_Member_Form_Membership extends CRM_Member_Form { // END Fix for dev/core/issues/860 CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'], - $formValues, $lineItem[$this->_priceSetId], NULL, $this->_priceSetId); + $formValues, $lineItem[$this->_priceSetId], $this->_priceSetId); if (!empty($formValues['tax_amount'])) { $params['tax_amount'] = $formValues['tax_amount']; diff --git a/CRM/Member/Form/MembershipRenewal.php b/CRM/Member/Form/MembershipRenewal.php index 513f18014e..36aecfce2d 100644 --- a/CRM/Member/Form/MembershipRenewal.php +++ b/CRM/Member/Form/MembershipRenewal.php @@ -597,7 +597,7 @@ class CRM_Member_Form_MembershipRenewal extends CRM_Member_Form { $lineItem = []; $this->_params = $this->setPriceSetParameters($this->_params); CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'], - $this->_params, $lineItem[$this->_priceSetId], NULL, $this->_priceSetId + $this->_params, $lineItem[$this->_priceSetId], $this->_priceSetId ); //CRM-11529 for quick config backoffice transactions //when financial_type_id is passed in form, update the diff --git a/CRM/Price/BAO/PriceSet.php b/CRM/Price/BAO/PriceSet.php index ec54607426..4807191593 100644 --- a/CRM/Price/BAO/PriceSet.php +++ b/CRM/Price/BAO/PriceSet.php @@ -660,12 +660,9 @@ WHERE id = %1"; * Params reflecting form input e.g with fields 'price_5' => 7, 'price_8' => array(7, 8) * @param $lineItem * Line item array to be altered. - * @param string $component - * This parameter appears to only be relevant to determining whether memberships should be auto-renewed. - * (and is effectively a boolean for 'is_membership' which could be calculated from the line items.) * @param int $priceSetID */ - public static function processAmount($fields, &$params, &$lineItem, $component = '', $priceSetID = NULL) { + public static function processAmount($fields, &$params, &$lineItem, $priceSetID = NULL) { // using price set $totalPrice = $totalTax = 0; // CRM-18701 Sometimes the amount in the price set is overridden by the amount on the form. @@ -675,10 +672,6 @@ WHERE id = %1"; // set up (which allows a free form field). $amount_override = NULL; - if ($component) { - $autoRenew = []; - $autoRenew[0] = $autoRenew[1] = $autoRenew[2] = 0; - } if ($priceSetID) { $priceFields = self::filterPriceFieldsFromParams($priceSetID, $params); if (count($priceFields) == 1) { @@ -730,15 +723,6 @@ WHERE id = %1"; } } $totalPrice += $lineItem[$optionValueId]['line_total'] + CRM_Utils_Array::value('tax_amount', $lineItem[$optionValueId]); - if ( - $component && - // auto_renew exists and is empty in some workflows, which php treat as a 0 - // and hence we explicitly check to see if auto_renew is numeric - isset($lineItem[$optionValueId]['auto_renew']) && - is_numeric($lineItem[$optionValueId]['auto_renew']) - ) { - $autoRenew[$lineItem[$optionValueId]['auto_renew']] += $lineItem[$optionValueId]['line_total']; - } break; case 'Select': @@ -750,13 +734,6 @@ WHERE id = %1"; $lineItem = self::setLineItem($field, $lineItem, $optionValueId, $totalTax); } $totalPrice += $lineItem[$optionValueId]['line_total'] + CRM_Utils_Array::value('tax_amount', $lineItem[$optionValueId]); - if ( - $component && - isset($lineItem[$optionValueId]['auto_renew']) && - is_numeric($lineItem[$optionValueId]['auto_renew']) - ) { - $autoRenew[$lineItem[$optionValueId]['auto_renew']] += $lineItem[$optionValueId]['line_total']; - } break; case 'CheckBox': @@ -767,13 +744,6 @@ WHERE id = %1"; $lineItem = self::setLineItem($field, $lineItem, $optionId, $totalTax); } $totalPrice += $lineItem[$optionId]['line_total'] + CRM_Utils_Array::value('tax_amount', $lineItem[$optionId]); - if ( - $component && - isset($lineItem[$optionId]['auto_renew']) && - is_numeric($lineItem[$optionId]['auto_renew']) - ) { - $autoRenew[$lineItem[$optionId]['auto_renew']] += $lineItem[$optionId]['line_total']; - } } break; } @@ -819,16 +789,6 @@ WHERE id = %1"; $params['amount'] = $totalPrice; $params['tax_amount'] = $totalTax; - if ($component) { - foreach ($autoRenew as $dontCare => $eachAmount) { - if (!$eachAmount) { - unset($autoRenew[$dontCare]); - } - } - if (count($autoRenew) > 1) { - $params['autoRenew'] = $autoRenew; - } - } } /**