CRM-19126 Tax amount fix #8835
CRM-19126 Tax amount fix #8835
Conversation
…lues rather than return params
eileenmcnaughton
force-pushed
the
tax_amount
branch
3 times, most recently
from
e08d8f9
to
35ee779
Compare
|
@eileenmcnaughton you got style fixes to do. The reason why I did that was because if your re-calculating tax amount the total amount value may already include the tax amount previously calculated. So there is a possible situation of double taxation |
|
ah so this function adds tax on top of the total_amount? rather than treating a component as tax? |
eileenmcnaughton
force-pushed
the
tax_amount
branch
from
35ee779
to
68737ab
Compare
|
As far as I know yes. Hence my test. On Monday, 8 August 2016, Eileen McNaughton notifications@github.com
|
This includes moving the setting of tax values from the api to the BAO. Unfortunately an awful lot of the tax work was done in the form layer & it would be a big job to figure out which parts can be left to the BAO
eileenmcnaughton
force-pushed
the
tax_amount
branch
from
68737ab
to
2912ed0
Compare
|
@eileenmcnaughton failures look geniune |
| } | ||
|
|
||
| // CRM-16189 | ||
| CRM_Financial_BAO_FinancialAccount::checkFinancialTypeHasDeferred($params, $contributionID); | ||
|
|
||
| if (!isset($params['tax_amount']) && $setPrevContribution && (isset($params['total_amount']) || isset | ||
| ($params['financial_type_id']))) { | ||
| $params = CRM_Contribute_BAO_Contribution::checkTaxAmount($params); |
There was a problem hiding this comment.
@eileenmcnaughton i was finding that the $params being passed to the copyValues was the original and not updated to include the relevant tax amount for some reason
|
I tried editing contributions through the back end forms to make sure there was no change. I managed to configure a financial type that was sales-taxable & changing it back & forth from no-tax to taxed type works. I could not figure out how to change the amount on an existing contribution with a tax_amount - but that issue pre-existed & is in the form layer - not from this
|
|
blah blah (typed by Luke) |
|
:-) |
|
@seamuslee001 let me know if you think we are there with this now it's passing... I did test the backoffice contribution edit |
|
I think this is good to be merged now. |
|
I've been off in nature for the last week, with little Internet. Finally back on some CiviCRM stuff. There sure has been a lot of action here. Pretty impressive to see everyone work together. This all sounds good to me. I look forward to the next update. |
|
One thought on this. If recalculating the taxes. Which financial type/account is used? The one associated at the line-item I hope? Going back on one of my earlier comments in https://issues.civicrm.org/jira/browse/CRM-19126, I recall that it was the one associated with the membership type, which wouldn't be right if that is still happening. The original quick & dirty fix ignoring when total_amount was == 0 sort of circumvented this. But based on my reading of the last set of changes here, this could still now be an issue? |
|
@litespeedmarc Yes tho that was an issue before this and is IMO separate to the issue of total_amount being 0. I think another JIRA ticket should be opened for that and also maybe tag in Joe Murray and Pradeep in as well as they have been working on account stuff so this maybe of an issue to them but this specific issue of the tax amount getting wrongly set when total amount is not set has been fixed |
|
I think you can test against master & see now? |
@seamuslee001 this is my take on what the fix should look like
One thing you did that I wasn't clear about was $params['total_amount'] = $prevContributionValue->total_amount - $prevContributionValue->tax_amount;
I thought the saved total would be right?