From ec57bf8741042cc600a7d0daa38b514700fbce88 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 11 Nov 2019 17:47:15 +1300 Subject: [PATCH] [REF] calculate 'amount' on ContributionPage in a shared way in one scenario I have discovered a lot of tests are creating invalid contributions - https://github.com/civicrm/civicrm-core/pull/15706 So far the issues have been in the test + us permitting something that doesn't work on the form - ie https://github.com/civicrm/civicrm-core/pull/15771 I'm trying to work through them all & then we can ideally validate payments in general. In this case it turns out that because 'amount' is currently a 'required' parameter the tests have 'any value' stuck in there. In a real submission it would be calculated so I'm trying to share the code that would do that with the path used by the test (& in this case the api) and to move towards getting the tests valid --- CRM/Contribute/Form/Contribution/Confirm.php | 3 ++ CRM/Contribute/Form/Contribution/Main.php | 37 ++++++++++--------- CRM/Contribute/Form/ContributionBase.php | 24 ++++++++++++ tests/phpunit/api/v3/ContributionPageTest.php | 24 ++++++------ 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php index 1ad0f41d23..749c1fd7e8 100644 --- a/CRM/Contribute/Form/Contribution/Confirm.php +++ b/CRM/Contribute/Form/Contribution/Confirm.php @@ -1933,6 +1933,9 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr $_SERVER['REQUEST_METHOD'] = 'GET'; $form->controller = new CRM_Contribute_Controller_Contribution(); $params['invoiceID'] = md5(uniqid(rand(), TRUE)); + + // We want to move away from passing in amount as it is calculated by the actually-submitted params. + $params['amount'] = $params['amount'] ?? $form->getMainContributionAmount($params); $paramsProcessedForForm = $form->_params = self::getFormParams($params['id'], $params); $form->_amount = $params['amount']; // hack these in for test support. diff --git a/CRM/Contribute/Form/Contribution/Main.php b/CRM/Contribute/Form/Contribution/Main.php index 0eea574178..b454fd75de 100644 --- a/CRM/Contribute/Form/Contribution/Main.php +++ b/CRM/Contribute/Form/Contribution/Main.php @@ -1020,6 +1020,8 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu * * @param array $params * Submitted values. + * + * @throws \CiviCRM_API3_Exception */ public function submit($params) { //carry campaign from profile. @@ -1079,19 +1081,9 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu } $params['separate_amount'] = $params['amount']; - $memFee = NULL; - if (!empty($params['selectMembership'])) { - if (empty($this->_membershipTypeValues)) { - $this->_membershipTypeValues = CRM_Member_BAO_Membership::buildMembershipTypeValues($this, - (array) $params['selectMembership'] - ); - } - $membershipTypeValues = $this->_membershipTypeValues[$params['selectMembership']]; - $memFee = $membershipTypeValues['minimum_fee']; - if (!$params['amount'] && !$this->_separateMembershipPayment) { - $params['amount'] = $memFee ? $memFee : 0; - } - } + // @todo - stepping through the code indicates that amount is always set before this point so it never matters. + // Move more of the above into this function... + $params['amount'] = $this->getMainContributionAmount($params); //If the membership & contribution is used in contribution page & not separate payment $memPresent = $membershipLabel = $fieldOption = $is_quick_config = NULL; $proceFieldAmount = 0; @@ -1208,12 +1200,9 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu $params['description'] = ts('Online Contribution') . ': ' . ((!empty($this->_pcpInfo['title']) ? $this->_pcpInfo['title'] : $title)); $params['button'] = $this->controller->getButtonName(); // required only if is_monetary and valid positive amount - // @todo it seems impossible for $memFee to be greater than 0 & $params['amount'] not to - // be & by requiring $memFee down here we make it harder to do a sensible refactoring of the function - // above (ie. extract the amount in a small function). if ($this->_values['is_monetary'] && !empty($this->_paymentProcessor) && - ((float ) $params['amount'] > 0.0 || $memFee > 0.0) + ((float) $params['amount'] > 0.0 || $this->hasSeparateMembershipPaymentAmount($params)) ) { // The concept of contributeMode is deprecated - as should be the 'is_monetary' setting. $this->setContributeMode(); @@ -1330,6 +1319,8 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu * Function for unit tests on the postProcess function. * * @param array $params + * + * @throws \CiviCRM_API3_Exception */ public function testSubmit($params) { $_SERVER['REQUEST_METHOD'] = 'GET'; @@ -1337,4 +1328,16 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu $this->submit($params); } + /** + * Has a separate membership payment amount been configured. + * + * @param array $params + * + * @return mixed + * @throws \CiviCRM_API3_Exception + */ + protected function hasSeparateMembershipPaymentAmount($params) { + return $this->_separateMembershipPayment && (int) CRM_Member_BAO_MembershipType::getMembershipType($params['selectMembership'])['minimum_fee']; + } + } diff --git a/CRM/Contribute/Form/ContributionBase.php b/CRM/Contribute/Form/ContributionBase.php index d82b960a1b..c80f3f6ec7 100644 --- a/CRM/Contribute/Form/ContributionBase.php +++ b/CRM/Contribute/Form/ContributionBase.php @@ -1394,4 +1394,28 @@ class CRM_Contribute_Form_ContributionBase extends CRM_Core_Form { return new CRM_Core_Payment_Manual(); } + /** + * Get the amount for the main contribution. + * + * The goal is to expand this function so that all the argy-bargy of figuring out the amount + * winds up here as the main spaghetti shrinks. + * + * If there is a separate membership contribution this is the 'other one'. Otherwise there + * is only one. + * + * @param $params + * + * @return float + * + * @throws \CiviCRM_API3_Exception + */ + protected function getMainContributionAmount($params) { + if (!empty($params['selectMembership'])) { + if (empty($params['amount']) && !$this->_separateMembershipPayment) { + return CRM_Member_BAO_MembershipType::getMembershipType($params['selectMembership'])['minimum_fee'] ?? 0; + } + } + return $params['amount'] ?? 0; + } + } diff --git a/tests/phpunit/api/v3/ContributionPageTest.php b/tests/phpunit/api/v3/ContributionPageTest.php index 6e75c6ee83..887ffe9d63 100644 --- a/tests/phpunit/api/v3/ContributionPageTest.php +++ b/tests/phpunit/api/v3/ContributionPageTest.php @@ -376,7 +376,7 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { 'billing_first_name' => 'Billy', 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], ]; $this->callAPIAndDocument('ContributionPage', 'submit', $submitParams, __FUNCTION__, __FILE__, 'submit contribution page', NULL); @@ -397,11 +397,10 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { $submitParams = [ 'price_' . $this->_ids['price_field'][0] => reset($this->_ids['price_field_value']), 'id' => (int) $this->_ids['contribution_page'], - 'amount' => 10, 'billing_first_name' => 'Billy', 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], 'payment_processor_id' => 1, 'credit_card_number' => '4111111111111111', 'credit_card_type' => 'Visa', @@ -446,11 +445,10 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { $submitParams = [ 'price_' . $this->_ids['price_field'][0] => reset($this->_ids['price_field_value']), 'id' => (int) $this->_ids['contribution_page'], - 'amount' => 10, 'billing_first_name' => 'Billy', 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], 'email-Primary' => 'billy-goat@the-bridge.net', 'payment_processor_id' => $this->_paymentProcessor['id'], 'credit_card_number' => '4111111111111111', @@ -488,7 +486,7 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { 'billing_first_name' => 'Billy', 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruffier', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], 'email-Primary' => 'billy-goat@the-new-bridge.net', 'payment_processor_id' => $this->params['payment_processor_id'], ]; @@ -526,7 +524,7 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', 'is_pay_later' => 1, - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], 'email-Primary' => 'billy-goat@the-bridge.net', ]; @@ -551,14 +549,14 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { $submitParams = [ 'price_' . $this->_ids['price_field'][0] => reset($this->_ids['price_field_value']), 'id' => (int) $this->_ids['contribution_page'], - 'amount' => 10, 'billing_first_name' => 'Billy', 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], + 'amount' => 10, ]; - $this->callAPISuccess('contribution_page', 'submit', $submitParams); + $this->callAPISuccess('ContributionPage', 'submit', $submitParams); $contributions = $this->callAPISuccess('contribution', 'get', ['contribution_page_id' => $this->_ids['contribution_page']]); $this->assertCount(2, $contributions['values']); $lines = $this->callAPISuccess('LineItem', 'get', ['sequential' => 1]); @@ -1273,7 +1271,7 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', 'email' => 'billy@goat.gruff', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], 'payment_processor_id' => 1, 'credit_card_number' => '4111111111111111', 'credit_card_type' => 'Visa', @@ -1330,7 +1328,7 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', 'email' => 'billy@goat.gruff', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], 'payment_processor_id' => 1, 'credit_card_number' => '4111111111111111', 'credit_card_type' => 'Visa', @@ -1417,7 +1415,7 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { 'billing_middle_name' => 'Goat', 'billing_last_name' => 'Gruff', 'email' => 'billy@goat.gruff', - 'selectMembership' => $this->_ids['membership_type'], + 'selectMembership' => $this->_ids['membership_type'][0], 'payment_processor_id' => 1, 'credit_card_number' => '4111111111111111', 'credit_card_type' => 'Visa', -- 2.25.1