Fix bug where tax_amount is miscalculated on membership renewals #16772

eileenmcnaughton · 2020-03-14T00:43:58Z

Overview

Fixes a bug where tax amount is incorrectly calculated when the total amount is not left at the default on the membership renewal page.

This bug affects both membership & membership renewal pages. This PR only fixes for membership renewal
but the goal is to fix for membership too once this is merged.

Before

In order to replicate

enable tax & invoicing
configure a financial type with sales tax - e.g 10%
renew a membership, entering an amount other than the default membership amount. - eg. $50
view the membership - tax amount will be the same as it would be if the default amount had been used.

After

Correct tax calculation - total_amount = $50, tax_amount = $5, line_total = $45 - this is consistent with contribution page overrides

Technical Details

This is a fairly 'big' fix in that it
introduces a new class with new methods to get the information otherwise provided by
CRM_Price_BAO_PriceSet::processAmount. processAmount is one of those functions that is hard to read
and understand & has been hacked to do various things. Code that calls it is hard
to read because of this. Having a cleaner way to operate has been a goal for a while. We
have plans to build up the Order api to be much more usable and used. However, we really
need to build up sensible helpers before we can address that. This PR adds a class
that has a whole lot of functions that are more readable to do what the processAmount otherwise
does. The goal is to deprecate processAmount.

I started on MembershipRenewal as

it has a bug currently and
it is pretty simple in that it doesn't actually support pricesets - which is especially
helpful when testing :-)
doesn't interact with anything else - ie. although the new class is a lot of code it interacts with existing code in a very limited way - by providing more accurate line items & tax_amount

Comments

Tangental bugs I noticed

regression in master on adding financial accounts - I will do a separate PR
tax term not assigned to the template - out of scope for now but I believe a pre-hook type
structure should exist, assigning this to the template when enabled (as part of moving some code to
a core-extension).
the form shows the tax amount & does not update it - this is pretty big already so that is also out of scope for now

civibot · 2020-03-14T00:44:01Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2020-03-14T00:48:28Z

tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php

@@ -89,6 +89,7 @@ public function setUp() {
      'fixed_period_start_day' => '101',
      'fixed_period_rollover_day' => '1231',
      'relationship_type_id' => 20,
+      'min_fee' => 100,


Actually this didn't seem to 'work' & the underlying is still set to 0 - since we are testing overrides that is OK for now

seamuslee001 · 2020-03-14T00:51:10Z

ping @JoeMurray @monishdeb @petednz @stesi561 I think you all have interest in this I think

eileenmcnaughton · 2020-03-14T00:51:58Z

Yes - also @mattwire & @pradpnayak & @jitendrapurohit - since there is a code approach change in here

eileenmcnaughton · 2020-03-14T03:12:09Z

Test fails look like leakage - Test Result (3 failures / +3)
CRM_Member_Form_MembershipRenewalTest.testSubmitPayLater
CRM_Member_Form_MembershipRenewalTest.testSubmitPayLaterWithBilling
CRM_Member_Form_MembershipRenewalTest.testSubmitComplete

will dig

CRM/Financial/BAO/Order.php

In order to replicate 1) enable tax & invoicing 2) configure a financial type with sales tax 3) renew a membership, entering an amount other than the default membership amount. 4) view the membership - tax amount will be the same as it would be if the default amount had been used. Tangental bugs I noticed 1) regression in master on adding financial accounts - I will do a separate PR 2) tax term not assigned to the template - out of scope for now but I believe a pre-hook type structure should exist, assigning this to the template when enabled (as part of moving some code to a core-extension). 3) the form shows the tax amount & does not update it - this is pretty big already so that is also out of scope for now This bug affects both membership & membership renewal pages. This PR only fixes for membership renewal but the goal is to fix for membership too once this is merged. It is a fairly 'bug' fix in that it introduces a new class with new methods to get the information otherwise provided by CRM_Price_BAO_PriceSet::processAmount. processAmount is one of those functions that is hard to read and understand & has been hacked to do various things. Code that calls it is hard to read because of this. Having a cleaner way to operate has been a goal for a while. We have plans to build up the Order api to be much more usable and used. However, we really need to build up sensible helpers before we can address that. This PR adds a class that has a whole lot of functions that are more readable to do what the processAmount otherwise does. The goal is to deprecate processAmount. I started on MembershipRenewal as 1) it has a bug currently and 2) it is pretty simple in that it doesn't actually support pricesets - which is especially helpful when testing :-)

eileenmcnaughton · 2020-03-14T20:58:08Z

Thanks @mattwire I've addressed the things you tested

eileenmcnaughton · 2020-03-20T04:07:09Z

@monishdeb I'm keen for you to look at this PR - it's hopefully the start of a readability improvement across a few forms

monishdeb · 2020-03-20T05:13:33Z

I will, thanks for pointing this out. Added in my todo list as it will need me some through review .
Great work :)

kcristiano · 2020-04-09T23:57:34Z

CRM/Member/Form.php

@@ -453,9 +453,9 @@ protected static function getPriceSetID($params) {
    $priceSetID = CRM_Utils_Array::value('price_set_id', $params);


@eileenmcnaughton You'll need a rebase. this is now $priceSetID = $params['price_set_id'] ?? NULL;

It doesn't seem stale yet below?

(I can fix after if need be)

I could not apply the patch without editing on master. I am surprised this PR does not show that it has conflicts - could be me.

kcristiano · 2020-04-10T00:14:06Z

@eileenmcnaughton I did an r-run

I can reproduce the error and the fix does exactly what you describe.

I did notice that we have an issue when you view the contribution:

But that is unrelated to this PR.

It does bother me that the admin tax calculation is still wrong, if the total amount is $50 the tax should be 4.55 and the fee is 45.45, but as you say this is consistent with the current behavior so out of scope.

eileenmcnaughton · 2020-04-10T00:19:16Z

@kcristiano that view error is the taxTerm not being assigned to the template.

I've been mulling it a bit. I think the taxTerrm should either be

assigned always as part of the preProcess function based on whether one is configured for he site OR
if we really think it IS part of 'enableTaxAndInvoicing' rather than a 'free setting' then we should create a core-extension that assigns it in the pre-hook if taxAndInvoicing is enabled.

I go back & forwards a bit here as I don't think 'tax and invoicing' is a logical combo. To enable tax you need to create other things as well as that (configure accounts) & my gut says the 2 are not tightly linked .

kcristiano · 2020-04-10T00:43:55Z

@eileenmcnaughton I agree my comments should not be a blocker to merging. I think this is better in and we will have time to test and add on other improvements.

civibot bot added the master label Mar 14, 2020

eileenmcnaughton force-pushed the mem_tax branch from ee49b13 to 765c501 Compare March 14, 2020 00:44

eileenmcnaughton commented Mar 14, 2020

View reviewed changes

eileenmcnaughton added the has-test label Mar 14, 2020

eileenmcnaughton force-pushed the mem_tax branch 4 times, most recently from 76844ce to db2a8d3 Compare March 14, 2020 06:49

mattwire reviewed Mar 14, 2020

View reviewed changes

CRM/Financial/BAO/Order.php Outdated Show resolved Hide resolved

mattwire reviewed Mar 14, 2020

View reviewed changes

CRM/Financial/BAO/Order.php Outdated Show resolved Hide resolved

mattwire reviewed Mar 14, 2020

View reviewed changes

CRM/Financial/BAO/Order.php Outdated Show resolved Hide resolved

eileenmcnaughton force-pushed the mem_tax branch from db2a8d3 to 7aa7890 Compare March 14, 2020 20:53

agileware-justin mentioned this pull request Mar 19, 2020

CIVICRM-932: Displaying & Updating tax amount in membership creation and renewal correctly. #12548

Closed

kcristiano reviewed Apr 9, 2020

View reviewed changes

seamuslee001 merged commit dbbe381 into civicrm:master Apr 10, 2020

seamuslee001 deleted the mem_tax branch April 10, 2020 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug where tax_amount is miscalculated on membership renewals #16772

Fix bug where tax_amount is miscalculated on membership renewals #16772

eileenmcnaughton commented Mar 14, 2020 •

edited

Loading

civibot bot commented Mar 14, 2020

eileenmcnaughton Mar 14, 2020

seamuslee001 commented Mar 14, 2020

eileenmcnaughton commented Mar 14, 2020

eileenmcnaughton commented Mar 14, 2020

eileenmcnaughton commented Mar 14, 2020

eileenmcnaughton commented Mar 20, 2020

monishdeb commented Mar 20, 2020

kcristiano Apr 9, 2020

eileenmcnaughton Apr 10, 2020

eileenmcnaughton Apr 10, 2020

kcristiano Apr 10, 2020

kcristiano commented Apr 10, 2020 •

edited

Loading

eileenmcnaughton commented Apr 10, 2020

kcristiano commented Apr 10, 2020

		@@ -453,9 +453,9 @@ protected static function getPriceSetID($params) {
		$priceSetID = CRM_Utils_Array::value('price_set_id', $params);

Fix bug where tax_amount is miscalculated on membership renewals #16772

Fix bug where tax_amount is miscalculated on membership renewals #16772

Conversation

eileenmcnaughton commented Mar 14, 2020 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Mar 14, 2020

eileenmcnaughton Mar 14, 2020

Choose a reason for hiding this comment

seamuslee001 commented Mar 14, 2020

eileenmcnaughton commented Mar 14, 2020

eileenmcnaughton commented Mar 14, 2020

eileenmcnaughton commented Mar 14, 2020

eileenmcnaughton commented Mar 20, 2020

monishdeb commented Mar 20, 2020

kcristiano Apr 9, 2020

Choose a reason for hiding this comment

eileenmcnaughton Apr 10, 2020

Choose a reason for hiding this comment

eileenmcnaughton Apr 10, 2020

Choose a reason for hiding this comment

kcristiano Apr 10, 2020

Choose a reason for hiding this comment

kcristiano commented Apr 10, 2020 • edited Loading

eileenmcnaughton commented Apr 10, 2020

kcristiano commented Apr 10, 2020

eileenmcnaughton commented Mar 14, 2020 •

edited

Loading

kcristiano commented Apr 10, 2020 •

edited

Loading