Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[REF] calculate 'amount' on ContributionPage in a shared way in one scenario #15810

Merged
merged 1 commit into from
Nov 14, 2019
Merged

[REF] calculate 'amount' on ContributionPage in a shared way in one scenario #15810

merged 1 commit into from
Nov 14, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 11, 2019 •
edited
Loading

Overview

Baby step towards making code logical in Contribution form

Before

Tests 'making up' the 'amount' value rather than calculating it.

After

In one specific scenario (form is not quick config and a membership is selected) 'amount' is optional in ContributionPage.submit and calculated using a shared function (shared with the 'Main' form).

Technical Details

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

So far the issues have been in the test + us permitting something that doesn't work on the form - ie #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

Comments

In further digging it seems the lines I extracted are never invoked - I added a comment but want to do further cleanups so will not add to this PR

@civibot
Copy link

civibot bot commented Nov 11, 2019

(Standard links)

@civibot civibot bot added the master label Nov 11, 2019
eileenmcnaughton
eileenmcnaughton commented Nov 11, 2019
View reviewed changes
CRM/Contribute/Form/Contribution/Main.php
);
}
$membershipTypeValues = $this->_membershipTypeValues[$params['selectMembership']];
$memFee = $membershipTypeValues['minimum_fee'];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$memFee is replaced further down by this->hasSeparateMembershipPaymentAmount($params))

@eileenmcnaughton eileenmcnaughton changed the title [REF] calculate 'amount' on ContributionPage in a shared way in one s… [REF] calculate 'amount' on ContributionPage in a shared way in one scenario Nov 11, 2019
eileenmcnaughton
eileenmcnaughton commented Nov 11, 2019
View reviewed changes
CRM/Contribute/Form/Contribution/Confirm.php
@@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This submit function is accessed by the ContributionPage.submit api & a whole load of tests via that, but not from the form submission process

@magnolia61
Copy link
Contributor

magnolia61 commented Nov 12, 2019 •
edited
Loading

This is a function to calculate the main contribution amount (I guess from the line item selection), which I think is useful because lately I sometimes see the contribution amount change to the amount paid or not change when the price set selection is changed.

Are there also already standard functions to calculate the a) Total Amount Fee b) Total Amount Paid & c) Total Amount Due (balance) from the financial transactions? Probably obvious question, but I am pretty new here so forgive me :-)

---- functions I found so far and and I am not sure of which does what:

getBalanceTrxnAmt
getTotalPayments
getPartialPaymentWithType
getContributionBalance

@eileenmcnaughton
Copy link
Contributor Author

@magnolia61 it's more like a micro-step towards consolidating the code in that direction. As you know from the payment.create saga it takes a lot of steps to unravel that has been spread over lots of classes

@eileenmcnaughton
[REF] calculate 'amount' on ContributionPage in a shared way in one s…
ec57bf8 
…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
@magnolia61
Copy link
Contributor

magnolia61 commented Nov 12, 2019 •
edited
Loading

@magnolia61 it's more like a micro-step towards consolidating the code in that direction. As you know from the payment.create saga it takes a lot of steps to unravel that has been spread over lots of classes

...as the main spaghetti shrinks. ;-)

the saga continues...
good luck!

mattwire
mattwire reviewed Nov 14, 2019
View reviewed changes
tests/phpunit/api/v3/ContributionPageTest.php
@@ -488,7 +486,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],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton Interesting that the tests were wrong here. That helps me with #15555 which has similar "issues".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep - there is so much wrong :-(

@mattwire
Copy link
Contributor

getBalanceTrxnAmt
getTotalPayments
getPartialPaymentWithType
getContributionBalance

@eileenmcnaughton @magnolia61 Also see https://lab.civicrm.org/dev/financial/issues/103 - there is quite a mess of these functions being called (multiple times, in different ways) just to generate the data required for displaying a contribution. I'm sure they could be cleaned up / consolidated ideally into an API - see Contribution.getBalance for an initial experiment at that : https://lab.civicrm.org/extensions/mjwshared/blob/master/api/v3/Contribution/Getbalance.php

@mattwire mattwire merged commit 49f7295 into civicrm:master Nov 14, 2019
@seamuslee001 seamuslee001 deleted the mem_fix branch November 15, 2019 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwire mattwire mattwire left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @magnolia61 @mattwire