[wip] CRM-19585, added function to Calculate Tax for each item when Financi… #9685
Conversation
| * @param int $contributionId | ||
| * | ||
| */ | ||
| public static function calculateTaxAfterChangeInFinancialTypeForLineItems($lineItem, $contributionId) { |
There was a problem hiding this comment.
I think this method name and its description are confusing. Usually, the method name should describe what it does and not when it should be executed. It says it calculates the Tax after the Financial Type is changed, but looking at the code, it looks like it can do the calculation anytime, even if the Financial Type hasn't been changed.
There was a problem hiding this comment.
Pradeep, there is merit in @davialexandre comment, but the code at some level needs to be either reversing out financial_items for both the line_item and its tax and posting new entries (I think that is the spec at https://wiki.civicrm.org/confluence/display/CRM/CiviAccounts+Data+Flow but Confluence is down at the moment) or maybe (I don't think so) creating a difference transaction for financial_line_item for both line item and tax. Could you respond to his comment? Thx.
| * Calculate Tax for each item when Financial Type is changed. | ||
| * | ||
| * @param array $lineItem | ||
| * |
There was a problem hiding this comment.
no need to this empty line here
There was a problem hiding this comment.
https://www.drupal.org/node/1354#functions suggests a blank line between args and no blank line after last one.
| * @param array $lineItem | ||
| * | ||
| * @param int $contributionId | ||
| * |
There was a problem hiding this comment.
no need for this empty line here
There was a problem hiding this comment.
Actually, https://www.drupal.org/node/1354#functions suggests a blank line between args and no blank line after last one.
| * | ||
| * @param int $contributionId | ||
| * | ||
| */ |
There was a problem hiding this comment.
Agreed. Partially fixed in updated PR.
| @@ -5457,4 +5457,25 @@ public static function createProportionalFinancialEntries($entityParams, $lineIt | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Calculate Tax for each item when Financial Type is changed. | |||
| * | |||
There was a problem hiding this comment.
It would be nice to add the logic of the calculation to the method description. Otherwise, we have to read the code in order to understand what is going on
There was a problem hiding this comment.
This function is just repeatedly calling a different function that does the calculation. I don't think it should be documenting details that is not responsible for.
| /** | ||
| * Calculate Tax for each item when Financial Type is changed. | ||
| * | ||
| * @param array $lineItem |
There was a problem hiding this comment.
It would be nice to know what are the values expected in this $lineItem array
| * @param int $contributionId | ||
| * | ||
| */ | ||
| public static function calculateTaxAfterChangeInFinancialTypeForLineItems($lineItem, $contributionId) { |
There was a problem hiding this comment.
What about adding tests to this new method?
There was a problem hiding this comment.
Pradeep, you should respond to each comment indicating if you accept it or provide reason(s) for disagreeing. In this case, indicate that you have added a test in updated PR.
…al Type 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
6264f10 to
ea4c834
Compare
|
Jenkins, test this please |
|
jenkins, retest this please |
|
setting to WIP, needs it's own ticket & steps to replicate the problem this is trying to solve |
CRM/Contribute/BAO/Contribution.php
Outdated
| * @param int $contributionId | ||
| * the id of the contribution | ||
| * | ||
| * @return array |
There was a problem hiding this comment.
This is incorrect. Could you indicate the data type and precision of the returned contribution total tax amount.
There was a problem hiding this comment.
Thanks for the input @JoeMurray - I hadn't gone down to the code level on these PRs yet, because I wanted to start from an understanding of what they are trying to fix
|
I agree with need for issue and context. |
|
Pradeep, in general, please respond to each comment separately indicating if you have accepted or why not. Although comments become dated when you submit a new PR, everyone needs to manage the feedback to track if everything has been fixed or not; hence the need for individual replies rather than an updated PR and silence on the comments. |
|
@pradpnayak could you please rebase this and also merge in the branch |
…al Type is changed.
https://issues.civicrm.org/jira/browse/CRM-19585