dev/core#841 event income report template does not exclude trashed contacts #13928
Conversation
|
(Standard links)
|
|
@lcdservices : tested PR, works as expected |
| JOIN civicrm_contact | ||
| ON civicrm_participant.contact_id = civicrm_contact.id | ||
| AND civicrm_contact.is_deleted = 0 | ||
| WHERE civicrm_participant.event_id IN ({$eventID}) |
There was a problem hiding this comment.
Since you are using INNER JOIN on single table can we directly add where clause to Join clause?
JOIN civicrm_contact
ON civicrm_participant.contact_id = civicrm_contact.id
AND civicrm_contact.is_deleted = 0
AND civicrm_participant.event_id IN ({$eventID})
| {$activeParticipantClause} | ||
| AND civicrm_participant.is_test = 0 | ||
| ) civicrm_participant | ||
| ON civicrm_participant.event_id = civicrm_event.id |
There was a problem hiding this comment.
Just worried since civicrm_participant.event_id is not indexed and might impact performance?
How about this
LEFT JOIN civicrm_participant ON ( civicrm_event.id = civicrm_participant.event_id
{$activeParticipantClause} AND civicrm_participant.is_test = 0 )
LEFT JOIN civicrm_contact
ON civicrm_participant.contact_id = civicrm_contact.id
WHERE civicrm_contact.is_deleted = 0 AND civicrm_event.id IN( {$eventID}) {$groupBy}
OR
LEFT JOIN civicrm_participant ON ( civicrm_event.id = civicrm_participant.event_id
{$activeParticipantClause} AND civicrm_participant.is_test = 0 )
JOIN civicrm_contact
ON civicrm_participant.contact_id = civicrm_contact.id
AND civicrm_contact.is_deleted = 0
WHERE civicrm_event.id IN( {$eventID}) {$groupBy}
There was a problem hiding this comment.
@pradpnayak I think "civicrm_participant.event_id is not indexed" is incorrect - the field looks to be indexed on my install
Also from my tests moving things between JOIN and WHERE like that probably won't change performance because mysql can still figure out that the contact_id is the best index to use in this case for the join on the contact table - but the only way to be sure is to test the queries on a biggish DB
There was a problem hiding this comment.
@eileenmcnaughton by civicrm_participant.event_id I meant its actually not joining the main civicrm_participant table, Its a join made to subquery so thought it won't have indexing (sorry if i am wrong).
There was a problem hiding this comment.
@pradpnayak I think the answer is maybe - mysql optimisation engine figures some things out & not others so we'd need to test against mysql to see
| JOIN civicrm_contact c | ||
| ON civicrm_participant.contact_id = c.id | ||
| AND c.is_deleted = 0 | ||
| WHERE civicrm_participant.event_id IN ({$eventID}) |
There was a problem hiding this comment.
Again replacing WHERE with AND?
| JOIN civicrm_contact c | ||
| ON civicrm_participant.contact_id = c.id | ||
| AND c.is_deleted = 0 | ||
| WHERE civicrm_participant.event_id IN ({$eventID}) |
There was a problem hiding this comment.
Again replacing WHERE with AND?
| JOIN civicrm_contact c | ||
| ON civicrm_participant.contact_id = c.id | ||
| AND c.is_deleted = 0 | ||
| WHERE civicrm_participant.event_id IN ({$eventID}) |
There was a problem hiding this comment.
Again replacing WHERE with AND?
| OR pp.participant_id = civicrm_participant.registered_by_id) | ||
| LEFT JOIN civicrm_contribution c | ||
| ON pp.contribution_id = c.id | ||
| JOIN civicrm_contact ct |
There was a problem hiding this comment.
Can we move this join just after FROM civicrm_participant?
This report is currently excluded from unit tests by virtue of it being declared as skipped in api_v3_ReportTemplateTest We have a PR civicrm#13928 to alter test logic. I'm really not comfortable making any logic changes to the report until it is under testing. Extracting buildQuery removes one of the blockers to bringing it under testing (remaining are moving most / all logic from postProcess to beginPostProcessCommon and fixing up the setPager function - which probably just means removing it
|
I'm really not comfortable making any logic changes to this report without bringing it under testing. I can accept the sort of 'add a single straight forward field' mini-features @yashodha has been doing without a test as long as the report already has tests & currently only 3 reports are not tested at all - this being one.... This PR #14098 extracts out buildQuery like the other reports - it doesn't quite get us to 'under test' but I think it's a necessary step in the direction |
|
Closing for now (PR queue cleanup). Please re-open when ready for a new round of review. |
Overview
Trashed contacts are reflected in the Event Income report
Before
Register a contact for an event. Trash the contact. Review the Event Income report for that event. The trashed contact will be counted in the report stats.
After
Trashed contact is no longer counted.