CRM-20025 [ready for review] Prevented query from fetching contacts without contributions

[Edzelopez]

---

• CRM-20025: DB Error on email task for advanced search for contributions
  https://issues.civicrm.org/jira/browse/CRM-20025

[Edzelopez]

There is an issue while fetching the contributions using advanced search, wherein the contacts without a single contribution are also returned in the results. This causes a problem when the results are viewed as contributions since the query returns NULL value for id. Steps to replicate on dmaster in issue.

[eileenmcnaughton]

I've taken a good look at what is happening & this is misery inducing code.

Basically any other way that you access the contribution search (e.g api, direct search) it adds the filter on is_test = 0, which achieves this + a little bit more.

I think it's awful that contribution-exists filtering works 'almost by accident' & the Contribution_BAO_Query::where() clause should add an is not null on id whenever mode = CONTRIBUTE. However, there is some risk in that I suppose, so it's probably worth adding a code comment to the where() function explaining that is what should be done, but we are too chicken to do that!

We should probably change this fix to pass in the is_test parameter, (if not set) since even if we fix the id thing here we will still be incorrectly getting test contributions as well as live ones - ie something like

if (!isset($queryParams['contribution_is_test'])) {
$queryParams['contribution_is_test'] = 0;
}

(that's not quite the format of that array but you get the idea)

[Edzelopez]

I agree @eileenmcnaughton

The filter does in some cases add the is_test = 0 parameter which prevents the issue above. Although, in some cases it doesn't, even after following the same steps which is weird. The change you mentioned above does make sense though, so I will modify the patch. Thanks for reviewing!

[Edzelopez]

@eileenmcnaughton I have made the changes now. Could you please check if this is what you were indicating should be done?

[eileenmcnaughton]

yes, that seems fine. I probably would have extracted it into a function for readability, but happy to merge as is.