[ready-for-review]CRM-20193, Draw currency from better table on Batch Transaction form #9930
Conversation
pradpnayak
commented
Mar 5, 2017
•
pradpnayak
commented
- CRM-20193: Draw currency from better table on Batch Transaction form
…financial_trxn table and have name field table alias ---------------------------------------- * CRM-20193: Draw currency from better table on Batch Transaction form https://issues.civicrm.org/jira/browse/CRM-20193
…the LEFT JOIN ---------------------------------------- * CRM-20193: Draw currency from better table on Batch Transaction form https://issues.civicrm.org/jira/browse/CRM-20193
---------------------------------------- * CRM-20193: Draw currency from better table on Batch Transaction form https://issues.civicrm.org/jira/browse/CRM-20193
| @@ -648,10 +648,11 @@ public static function getBatchFinancialItems($entityID, $returnValues, $notPres | |||
| } | |||
|
|
|||
| $from = "civicrm_financial_trxn | |||
| LEFT JOIN civicrm_entity_financial_trxn ON civicrm_entity_financial_trxn.financial_trxn_id = civicrm_financial_trxn.id | |||
| INNER JOIN civicrm_entity_financial_trxn ON civicrm_entity_financial_trxn.financial_trxn_id = civicrm_financial_trxn.id | |||
| INNER JOIN civicrm_contribution ON (civicrm_contribution.id = civicrm_entity_financial_trxn.entity_id | |||
There was a problem hiding this comment.
@pradpnayak is there much of an issue here switching from LEFT to INNER JOIN?
There was a problem hiding this comment.
I suggested these changes because the LEFT JOINs were misleading given https://github.com/civicrm/civicrm-core/pull/9930/files#diff-4edf3d9f312c7543c00ef7d38bf21044L726 which forces the existence of a record in both of these tables.
There was a problem hiding this comment.
Happy with that explanation, That makes sense, Just thought worth posing the question
There was a problem hiding this comment.
So, how does that tie in with
"This is relevant as the form is intended to allow financial data from other types of financial information as well eg grants, general journal entries. While these have entries in the civicrm_financial_trxn table, they don't have related civicrm_contributions, and a currency is still required."
I feel like that INNER JOIN would exclude them from being pulled in. I read that as a bad thing - but perhaps it is a good thing?
There was a problem hiding this comment.
So to me, this was a code cleanup for maintainability.
There was a problem hiding this comment.
OK - I've worked through the clean up & it all makes sense, and is just clean-up as opposed to functionality change, as you say, except for the removal of
AND civicrm_entity_batch.entity_table = 'civicrm_financial_trxn'
which I'm still not clear about the reason for
eileenmcnaughton
changed the title
|
I just removed the core team part, since anyone could be the reviewer. You should still specify core-team if you are funding core team to do the review |
| if (!$notPresent) { | ||
| $where = " ( civicrm_entity_batch.batch_id = {$entityID} | ||
| AND civicrm_entity_batch.entity_table = 'civicrm_financial_trxn' |
There was a problem hiding this comment.
So why did this line get removed
There was a problem hiding this comment.
Since its already added as condition in JOIN clause
https://github.com/civicrm/civicrm-core/pull/9930/files#diff-4edf3d9f312c7543c00ef7d38bf21044R654
There was a problem hiding this comment.
Yep, but as a left join not an INNER join, so removing that will remove the criteria
There was a problem hiding this comment.
Joe pointed out that this line is redundant because the line about is from the same table, & hence the left join would mean the left join would filter out the id & it would be NULL not = $entityID
| @@ -722,24 +723,15 @@ public static function getBatchFinancialItems($entityID, $returnValues, $notPres | |||
| } | |||
| if (!empty($query->_where[0])) { | |||
| $where = implode(' AND ', $query->_where[0]) . | |||
| " AND civicrm_entity_batch.batch_id IS NULL | |||
| AND civicrm_entity_financial_trxn.entity_table = 'civicrm_contribution'"; | |||
There was a problem hiding this comment.
OK - this line is redundant due to inner join above
[ready-for-review]CRM-20193, Draw currency from better table on Batch Transaction form
