Skip to content

Commit

Permalink
[REF] calculate 'amount' on ContributionPage in a shared way in one s…
Browse files Browse the repository at this point in the history 
…cenario

I have discovered a lot of tests are creating invalid contributions - civicrm#15706

So far the issues have been in the test + us permitting something that doesn't work on the form - ie civicrm#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
  • Loading branch information
@eileenmcnaughton
eileenmcnaughton committed Nov 11, 2019
1 parent 3bba381 commit 5771f46
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 30 deletions.
3 changes: 3 additions & 0 deletions CRM/Contribute/Form/Contribution/Confirm.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1933,6 +1933,9 @@ public static function submit($params) {
$_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.
Expand Down
37 changes: 20 additions & 17 deletions CRM/Contribute/Form/Contribution/Main.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,8 @@ public function postProcess() {
*
* @param array $params
* Submitted values.
*
* @throws \CiviCRM_API3_Exception
*/
public function submit($params) {
//carry campaign from profile.
Expand Down Expand Up @@ -1079,19 +1081,9 @@ public function submit($params) {
}

$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;
Expand Down Expand Up @@ -1208,12 +1200,9 @@ public function submit($params) {
$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();
Expand Down Expand Up @@ -1330,11 +1319,25 @@ public function assignFormVariablesByContributionID() {
* Function for unit tests on the postProcess function.
*
* @param array $params
*
* @throws \CiviCRM_API3_Exception
*/
public function testSubmit($params) {
$_SERVER['REQUEST_METHOD'] = 'GET';
$this->controller = new CRM_Contribute_Controller_Contribution();
$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'];
}

}
24 changes: 24 additions & 0 deletions CRM/Contribute/Form/ContributionBase.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1394,4 +1394,28 @@ protected function getPaymentProcessorObject() {
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;
}

}
24 changes: 11 additions & 13 deletions tests/phpunit/api/v3/ContributionPageTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ public function testSubmitMembershipBlockNotSeparatePayment() {
'billing_first_name' => 'Billy',
'billing_middle_name' => 'Goat',
'billing_last_name' => 'Gruff',
'selectMembership' => $this->_ids['membership_type'],
'selectMembership' => $this->_ids['membership_type'][0],

];

Expand All @@ -357,11 +357,10 @@ public function testSubmitMembershipBlockNotSeparatePaymentProcessorInstantRenew
$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',
Expand Down Expand Up @@ -404,11 +403,10 @@ public function testSubmitMembershipBlockNotSeparatePaymentWithEmail() {
$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',
Expand Down Expand Up @@ -444,7 +442,7 @@ public function testSubmitMembershipBlockNotSeparatePaymentZeroDollarsWithEmail(
'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'],
];
Expand Down Expand Up @@ -480,7 +478,7 @@ public function testSubmitMembershipBlockIsSeparatePaymentPayLaterWithEmail() {
'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',
];

Expand All @@ -503,14 +501,14 @@ public function testSubmitMembershipBlockIsSeparatePayment() {
$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]);
Expand Down Expand Up @@ -1203,7 +1201,7 @@ public function testSubmitMembershipPriceSetPaymentPaymentProcessorSeparatePayme
'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',
Expand Down Expand Up @@ -1258,7 +1256,7 @@ public function testSubmitMembershipPriceSetPaymentPaymentProcessorRecurDelayed(
'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',
Expand Down Expand Up @@ -1343,7 +1341,7 @@ public function testSubmitMembershipIsSeparatePaymentNotRecur() {
'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',
Expand Down

0 comments on commit 5771f46

Please sign in to comment.