-
-
Notifications
You must be signed in to change notification settings - Fork 824
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.
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
CRM-20276, CRM-20257, CRM-20259, CRM-20260, CRM-20262 : Sale Tax issues #10172
Conversation
monishdeb
commented
Apr 17, 2017
•
edited by civicrm-builder
Loading
edited by civicrm-builder
- CRM-19585: Sales tax issues
- CRM-20276: When editing a contribution the value in civicrm_financial_item_amount is not updated
- CRM-20257: Contribution Receipts doesn't include tax details
- CRM-20259: Update to Contribution with multiple priceset via api updates the total amount and tax amount
- CRM-20260: Incorrect information is stored in civicrm_financial_item table
- CRM-20262: Financial records are incorrectly recorded when there is change in contribution
$this->assertEquals($taxAmount, 0, 'Amount does not match.'); | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do proper docblock
@Monish are these new functions called from anywhere? I would prefer to review/merge a complete PR that has the functions being used. |
@monishdeb tests are failing with this in the console:
I've merged 2 other PRs into this one but perhaps it's still not complete? What's missing? |
CRM/Contribute/BAO/Contribution.php
Outdated
'previous_line_total' => CRM_Utils_Array::value('line_total', CRM_Utils_Array::value($fieldValues['id'], $previousLineItem)), | ||
'item_amount' => $itemAmount, | ||
); | ||
$amount = self::calcluateFinancialItemAmount($params, $amountParams, $context, $fieldValues); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colemanw its a typo calculateFinancialItemAmount
instead of calcluateFinancialItemAmount
deaf750
to
8f6592f
Compare
Jenkins, please retest. |
@monishdeb @colemanw can we push this forward? What needs to be done to get this merged? Monish, could you start by getting it to pass all tests? |
…al Type is changed.
…r contribution is changed. ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585
---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585
---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585
…for contribution like Amount, FT, PI ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585
@JoeMurray I have now rebased the PR. Earlier, it took me time to understand the code and tried to fix the related test build failures, but without any success. So I discussed with @pradpnayak and he agrees to take a look at it. |
@monishdeb @pradpnayak this still has failing tests. |
Labelling WIP due to failing tests. |
@monishdeb @pradpnayak please update to get it to pass tests. |
@pradpnayak I am unable to fix the related test build failures. Can you please take a look? |
@monishdeb I'm pretty sure this needs a replacement PR - that may have already happened & this has just not been closed. It's unclear what this is trying to fix |
Agree with you. Will create replacement PR(s) addressing issues respectively. Closing this for now. |