Skip to content
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.

Sign up for GitHub

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

[NFC] add comments & extract function in contribution search #9716

Merged
merged 4 commits into from
Jan 23, 2017
Merged

[NFC] add comments & extract function in contribution search #9716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
686df92
CRM-19815 extract soft credit fields for readability
eileenmcnaughton Jan 4, 2017
59f9e5a
CRM-19830 add test to lock in test contract
eileenmcnaughton Jan 4, 2017
ef32adf
CRM-19815 tidy test & lock in return of financial_type
eileenmcnaughton Jan 8, 2017
8f165fa
Comment fixes
eileenmcnaughton Jan 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CRM/Batch/BAO/Batch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,8 @@ public static function getBatchFinancialItems($entityID, $returnValues, $notPres
$values['contribution_date_high'] = $date['to'];
}
$searchParams = CRM_Contact_BAO_Query::convertFormValues($values);
// @todo the use of defaultReturnProperties means the search will be inefficient
// as slow-unneeded properties are included.
$query = new CRM_Contact_BAO_Query($searchParams,
CRM_Contribute_BAO_Query::defaultReturnProperties(CRM_Contact_BAO_Query::MODE_CONTRIBUTE,
FALSE
Expand Down
32 changes: 26 additions & 6 deletions CRM/Contact/BAO/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,10 @@ public function __construct(

/**
* Function which actually does all the work for the constructor.
*
* @param string $apiEntity
* The api entity being called.
* This sort-of duplicates $mode in a confusing way. Probably not by design.
*/
public function initialize($apiEntity = NULL) {
$this->_select = array();
Expand Down Expand Up @@ -525,7 +529,7 @@ public function initialize($apiEntity = NULL) {
* versus search builder.
*
* The direction we are going is having the form convert values to a standardised format &
* moving away from wierd & wonderful where clause switches.
* moving away from weird & wonderful where clause switches.
*
* Fix and handle contact deletion nicely.
*
Expand Down Expand Up @@ -588,14 +592,18 @@ public function buildParamsLookup() {
$this->_paramLookup[$value[0]] = array();
}
if ($value[0] !== 'group') {
// Just trying to unravel how group interacts here! This whole function is wieid.
// Just trying to unravel how group interacts here! This whole function is weird.
$this->_paramLookup[$value[0]][] = $value;
}
}
}

/**
* Some composite fields do not appear in the fields array hack to make them part of the query.
*
* @param $apiEntity
* The api entity being called.
* This sort-of duplicates $mode in a confusing way. Probably not by design.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nicer.

A suggestion on docblock style going-forward (not meant as a complaint about this): @param $apiEntity looks like a typical case where I'd try to include an example as part of the doc, as in:

 * @param string $apiEntity
 *   The api entity being called. Ex: 'Contact' or 'ContributionPage'.

As readers who are tuned-in to the nuances of Civi's data modelling (SQL/DAO/BAO/API), you and I probably interpret apiEntity as "an entity name in the camel-case notation of APIv3" (ContributionPage), but less experienced contributors might interpret "entity" more ambiguously. (ContributionPage, contribution_page, api/v3/ContributionPage.php, civicrm_api3_contribution_page_*, CRM_Contribute_DAO_ContributionPage, and civicrm_contribution_page are all ways to identify the same entity... and all those ways would actually show up if you were tracing an API call.)

An example value is a really simple/clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree - although when I add stuff to pre-existing comments I don't feel the need to be that thorough

*/
public function addSpecialFields($apiEntity) {
static $special = array('contact_type', 'contact_sub_type', 'sort_name', 'display_name');
Expand Down Expand Up @@ -624,6 +632,10 @@ public function addSpecialFields($apiEntity) {
* clauses. Note that since the where clause introduces new
* tables, the initial attempt also retrieves all variables used
* in the params list
*
* @param string $apiEntity
* The api entity being called.
* This sort-of duplicates $mode in a confusing way. Probably not by design.
*/
public function selectClause($apiEntity = NULL) {

Expand Down Expand Up @@ -666,6 +678,9 @@ public function selectClause($apiEntity = NULL) {
if (in_array($name, array('groups', 'tags', 'notes'))
&& isset($this->_returnProperties[substr($name, 0, -1)])
) {
// @todo instead of setting make exception to get us into
// an if clause that has handling for these fields buried with in it
// move the handling to here.
$makeException = TRUE;
}

Expand Down Expand Up @@ -865,13 +880,15 @@ public function selectClause($apiEntity = NULL) {
}
}
elseif ($name === 'tags') {
//@todo move this handling outside the big IF & ditch $makeException
$this->_useGroupBy = TRUE;
$this->_select[$name] = "GROUP_CONCAT(DISTINCT(civicrm_tag.name)) as tags";
$this->_element[$name] = 1;
$this->_tables['civicrm_tag'] = 1;
$this->_tables['civicrm_entity_tag'] = 1;
}
elseif ($name === 'groups') {
//@todo move this handling outside the big IF & ditch $makeException
$this->_useGroupBy = TRUE;
// Duplicates will be created here but better to sort them out in php land.
$this->_select[$name] = "
Expand All @@ -889,6 +906,7 @@ public function selectClause($apiEntity = NULL) {
);
}
elseif ($name === 'notes') {
//@todo move this handling outside the big IF & ditch $makeException
// if note field is subject then return subject else body of the note
$noteColumn = 'note';
if (isset($noteField) && $noteField == 'note_subject') {
Expand Down Expand Up @@ -2331,7 +2349,6 @@ public function restWhere(&$values) {
}
}


/**
* @param $where
* @param $locType
Expand Down Expand Up @@ -2834,9 +2851,9 @@ public function contactType(&$values) {
}

/**
* Where / qill clause for contact_sub_type
* Where / qill clause for contact_sub_type.
*
* @param $values
* @param array $values
*/
public function contactSubType(&$values) {
list($name, $op, $value, $grouping, $wildcard) = $values;
Expand Down Expand Up @@ -3140,7 +3157,7 @@ public function tagSearch(&$values) {
}

/**
* Where / qill clause for tag
* Where / qill clause for tag.
*
* @param array $values
*/
Expand Down Expand Up @@ -4299,6 +4316,9 @@ public static function getQuery($params = NULL, $returnProperties = NULL, $count
* Should permissions be ignored or should the logged in user's permissions be applied.
* @param int $mode
* This basically correlates to the component.
* @param string $apiEntity
* The api entity being called.
* This sort-of duplicates $mode in a confusing way. Probably not by design.
*
* @return array
*/
Expand Down
162 changes: 87 additions & 75 deletions CRM/Contribute/BAO/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ class CRM_Contribute_BAO_Query extends CRM_Core_BAO_Query {
static $_contribRecurPayment = NULL;

/**
* Function get the import/export fields for contribution.
* Function get the searchable fields for contribution.
*
* This is basically the contribution fields plus some related entity fields.
*
* @param bool $checkPermission
*
Expand Down Expand Up @@ -157,80 +159,6 @@ public static function select(&$query) {
$query->_tables['civicrm_contribution'] = 1;
}

// LCD 716
$includeSoftCredits = self::isSoftCreditOptionEnabled($query->_params);
if (!empty($query->_returnProperties['contribution_soft_credit_name'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_name'] = "civicrm_contact_d.sort_name as contribution_soft_credit_name";
// also include contact id. Will help build hyperlinks
$query->_select['contribution_soft_credit_contact_id'] = "civicrm_contact_d.id as contribution_soft_credit_contact_id";
}
$query->_element['contribution_soft_credit_name'] = 1;
$query->_element['contribution_soft_credit_contact_id'] = 1;

$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_contact_id'])) {
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_amount'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_amount'] = "civicrm_contribution_soft.amount as contribution_soft_credit_amount";
}
$query->_element['contribution_soft_credit_amount'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_type'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_type'] = "contribution_softcredit_type.label as contribution_soft_credit_type";
}
$query->_element['contribution_soft_credit_type'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['contribution_softcredit_type'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_contribution_id'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_contribution_id'] = "civicrm_contribution_soft.contribution_id as contribution_soft_credit_contribution_id";
}
$query->_element['contribution_soft_credit_contribution_id'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_email'])) {
$query->_select['contribution_soft_credit_email'] = "soft_email.email as contribution_soft_credit_email";
$query->_element['contribution_soft_credit_email'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
$query->_tables['civicrm_contribution_soft_email'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_phone'])) {
$query->_select['contribution_soft_credit_email'] = "soft_phone.phone as contribution_soft_credit_phone";
$query->_element['contribution_soft_credit_phone'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
$query->_tables['civicrm_contribution_soft_phone'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_pcp_id'])) {
$query->_select['contribution_soft_credit_pcp_id'] = "civicrm_contribution_soft.pcp_id as contribution_soft_credit_pcp_id";
$query->_element['contribution_soft_credit_pcp_id'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
}

if (!empty($query->_returnProperties['contribution_campaign_title'])) {
$query->_select['contribution_campaign_title'] = "civicrm_campaign.title as contribution_campaign_title";
$query->_element['contribution_campaign_title'] = $query->_tables['civicrm_campaign'] = 1;
Expand All @@ -251,6 +179,8 @@ public static function select(&$query) {
$query->_element['financial_type_id'] = $query->_tables['civicrm_contribution'] = 1;
}
// LCD 716 END

self::addSoftCreditFields($query);
}

/**
Expand Down Expand Up @@ -1207,4 +1137,86 @@ public static function formRule($fields, $files, $form) {
return empty($errors) ? TRUE : $errors;
}

/**
* Add the soft credit fields to the select fields.
*
* Extracted into separate function to improve readability of main select function.
*
* @param $query
*/
private static function addSoftCreditFields(&$query) {
$includeSoftCredits = self::isSoftCreditOptionEnabled($query->_params);
if (!empty($query->_returnProperties['contribution_soft_credit_name'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_name'] = "civicrm_contact_d.sort_name as contribution_soft_credit_name";
// also include contact id. Will help build hyperlinks
$query->_select['contribution_soft_credit_contact_id'] = "civicrm_contact_d.id as contribution_soft_credit_contact_id";
}
$query->_element['contribution_soft_credit_name'] = 1;
$query->_element['contribution_soft_credit_contact_id'] = 1;

$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_contact_id'])) {
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_amount'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_amount'] = "civicrm_contribution_soft.amount as contribution_soft_credit_amount";
}
$query->_element['contribution_soft_credit_amount'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_type'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_type'] = "contribution_softcredit_type.label as contribution_soft_credit_type";
}
$query->_element['contribution_soft_credit_type'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['contribution_softcredit_type'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_contribution_id'])) {
if ($includeSoftCredits) {
$query->_select['contribution_soft_credit_contribution_id'] = "civicrm_contribution_soft.contribution_id as contribution_soft_credit_contribution_id";
}
$query->_element['contribution_soft_credit_contribution_id'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_email'])) {
$query->_select['contribution_soft_credit_email'] = "soft_email.email as contribution_soft_credit_email";
$query->_element['contribution_soft_credit_email'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
$query->_tables['civicrm_contribution_soft_email'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_phone'])) {
$query->_select['contribution_soft_credit_email'] = "soft_phone.phone as contribution_soft_credit_phone";
$query->_element['contribution_soft_credit_phone'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
$query->_tables['civicrm_contribution_soft_contact'] = 1;
$query->_tables['civicrm_contribution_soft_phone'] = 1;
}

if (!empty($query->_returnProperties['contribution_soft_credit_pcp_id'])) {
$query->_select['contribution_soft_credit_pcp_id'] = "civicrm_contribution_soft.pcp_id as contribution_soft_credit_pcp_id";
$query->_element['contribution_soft_credit_pcp_id'] = 1;
$query->_tables['civicrm_contribution'] = 1;
$query->_tables['civicrm_contribution_soft'] = 1;
}
}

}
4 changes: 4 additions & 0 deletions CRM/Core/OptionValue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,10 @@ public static function getFields($mode = '', $contactType = 'Individual') {

$nameTitle = array();
if ($mode == 'contribute') {
// This is part of a move towards standardising option values but we
// should derive them from the fields array so am deprecating it again...
// note that the reason this was needed was that payment_instrument_id was
// not set to exportable.
$nameTitle = array(
'payment_instrument' => array(
'name' => 'payment_instrument',
Expand Down
Loading