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

Jump to bottom

Fix Custom field date query builder to work with 'raw' high & low params #15719

Merged
merged 3 commits into from
Nov 4, 2019
Merged

Fix Custom field date query builder to work with 'raw' high & low params #15719

merged 3 commits into from
Nov 4, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Preliminary query fixes to support the new datepicker format

Before

Many tech hurdles in switching datepicker over

After

They are melting away

Technical Details

Once we convert to datePicker we will be passing in parameters like custom_3_high, custom_3_low & custom_3_relative.

Currently we pass in custom_3_to & custom_3_from and custom_3_relative. This adds support for the new high & low
withoutt touching support for previously working or switching the field across (as yet). This paves the way for
the datepicker conversion. After a lot of brain pain I concluded the fundamental problem was the whereTables
from the CustomQuery were being merged in but actually then lost when where() is called.

Comments

@civibot
Copy link

civibot bot commented Nov 4, 2019

(Standard links)

@civibot civibot bot added the master label Nov 4, 2019
seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
CRM/Contact/BAO/Query.php
@@ -2077,12 +2076,12 @@ public function whereClause($isForcePrimaryEmailOnly = NULL) {
}

if ($this->_customQuery) {
$this->_whereTables = array_merge($this->_whereTables, $this->_customQuery->_whereTables);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes ok i gues that makes sense

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
CRM/Contact/BAO/Query.php
@@ -5260,8 +5259,8 @@ public function dateQueryBuilder(
// Special handling for contact table as it has a known alias in advanced search.
$tableName = 'contact_a';
}
if ($name == "{$fieldName}_low" ||
$name == "{$fieldName}_high"
if ($name === "{$fieldName}_low" ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
CRM/Core/BAO/CustomField.php Outdated
@@ -1064,14 +1064,15 @@ public static function deleteField($field) {
* @param int $entityId
*
* @return string
* @throws \Exception
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes tho i think this has already been merged

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
CRM/Core/BAO/CustomQuery.php Outdated
@@ -148,7 +148,7 @@ public function getFields() {
* @param array $locationSpecificFields
*/
public function __construct($ids, $contactSearch = FALSE, $locationSpecificFields = []) {
$this->_ids = &$ids;
$this->_ids = $ids;
Copy link
Contributor

Choose a reason for hiding this comment

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

ok sure

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
CRM/Core/BAO/CustomQuery.php Outdated
@@ -215,45 +215,26 @@ public function select() {

foreach (array_keys($this->_ids) as $id) {
$field = $this->_fields[$id];

if ($this->_contactSearch && $field['search_table'] === 'contact_a') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes makes sense

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
CRM/Core/BAO/CustomQuery.php
@@ -415,7 +396,9 @@ public function where() {
break;

case 'Date':
if (substr($name, -9, 9) !== '_relative') {
if (substr($name, -9, 9) !== '_relative'
&& substr($name, -4, 4) !== '_low'
Copy link
Contributor

Choose a reason for hiding this comment

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

ok makes sense becuase we are just caring about it being relative now

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
CRM/Core/BAO/CustomQuery.php
@@ -485,6 +468,25 @@ protected function joinCustomTableForField($field) {
$join = "\nLEFT JOIN $name ON $name.entity_id = {$field['search_table']}.id";
$this->_tables[$name] = $this->_tables[$name] ?? $join;
$this->_whereTables[$name] = $this->_whereTables[$name] ?? $join;

$joinTable = $field['search_table'];
Copy link
Contributor

Choose a reason for hiding this comment

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

ok sure looks like a straight extraction from above

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
$this->assertEquals("date field is This calendar year (between January 1st, " . date('Y') . " 12:00 AM and December 31st, " . date('Y') . " 11:59 PM)", $queryObj->_qill[0][0]);
$queryObj = new CRM_Core_BAO_CustomQuery($params);
$this->assertEquals('date field is This calendar year (between January 1st, ' . date('Y') . " 12:00 AM and December 31st, " . date('Y') . " 11:59 PM)", $queryObj->_qill[0][0]);
$queryObj = new CRM_Contact_BAO_Query($params);
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton i wonder about this does changing this have any meaningful impact on what we are testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in a good way - comments on the commit 2605f83

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
$queryObj = new CRM_Core_BAO_CustomQuery($params);
$queryObj->Query();
$params = CRM_Contact_BAO_Query::convertFormValues($formValues);
$queryObj = new CRM_Contact_BAO_Query($params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as earlier comment on L56

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
$queryObj = new CRM_Core_BAO_CustomQuery($params);
$queryObj->Query();
$params = CRM_Contact_BAO_Query::convertFormValues($formValues);
$queryObj = new CRM_Contact_BAO_Query($params);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto as per L56

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
'Int' => 2,
'Float' => 12.123,
'Money' => 91.21,
$dataSet = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
];
foreach ($datas as $type => $data) {
foreach ($dataSet as $type => $values) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yes

seamuslee001
seamuslee001 reviewed Nov 4, 2019
View reviewed changes
tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
@@ -186,55 +230,57 @@ public function testSearchCustomDataFromAndTo() {
);
$customFieldName = 'custom_' . $customField['id'];

$expectedValue = ($isDate) ? '"20150606235959"' : (($type == 'Money') ? $data : "\"$data\"");
$expectedQillValue = ($isDate) ? "'June 6th, 2015 11:59 PM'" : $data;
$expectedValue = $values['sql_string'] ?? $data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
$queryObj->_where[0][0]
);
$this->assertEquals($queryObj->_qill[0][0],
"$type field " . $wierdStringThatMeansGreaterEquals . " $expectedQillValue"
"$type field $toQillValue"
Copy link
Contributor

Choose a reason for hiding this comment

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

yes

tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
$formValues = [
$customFieldName . '_from' => $data,
$customFieldName . '_from' => $values['value'],
Copy link
Contributor

Choose a reason for hiding this comment

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

yes

tests/phpunit/CRM/Core/BAO/CustomQueryTest.php Outdated
*
* @throws \CRM_Core_Exception
*/
public function testAddressCustomFields() {
Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@seamuslee001
Copy link
Contributor

@eileenmcnaughton jsut a question can CRM_Core_BAO_CustomQuery be called ever outside of the context of CRM_Contact_BAO_Query ?

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 I didn't find any instances

@eileenmcnaughton eileenmcnaughton force-pushed the custom_query_fix_join branch 2 times, most recently from 41bc86f to 40fd594 Compare November 4, 2019 04:46
@seamuslee001
Copy link
Contributor

@eileenmcnaughton in that case then i think changing to CRM_Contact_BAO_Query makes sense in that case it does make it ok to switch in the tests then

@eileenmcnaughton eileenmcnaughton mentioned this pull request Nov 4, 2019
Merged
@eileenmcnaughton eileenmcnaughton force-pushed the custom_query_fix_join branch 3 times, most recently from 6aa9454 to dcda996 Compare November 4, 2019 07:48
@eileenmcnaughton
Move all thte code that relates to joining tables/constructing From t…
410e376 
…o one place.

This could definitely be cleaned up more - but the goal of this step is just to move all the code that
constructs the from code into one place, so it can be called more sensibly.

Tested in the newly written testAddressCustomFields
@eileenmcnaughton
Fix Custom field date query builder to work with 'raw' high & low params
2206409 
Once we convert to datePicker we will be passing in parameters like custom_3_high, custom_3_low & custom_3_relative.

Currently we pass in custom_3_to & custom_3_from and custom_3_relative. This adds support for the new high & low
withoutt touching support for previously working or switching the field across (as yet). This paves the way for
the datepicker conversion. After a lot of brain pain I concluded the fundamental problem was the whereTables
from the CustomQuery were being merged in but actually then lost when where() is called.
@eileenmcnaughton
Further join tests & fixes
56d1630 
Turns out the carefully constructed join was being whomped - but only sometimes
@seamuslee001
Copy link
Contributor

I agree with all these changes and they are well backed up with unit tests merging

@seamuslee001 seamuslee001 merged commit 5ea2abd into civicrm:master Nov 4, 2019
@seamuslee001 seamuslee001 deleted the custom_query_fix_join branch November 4, 2019 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seamuslee001 seamuslee001 seamuslee001 left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eileenmcnaughton @seamuslee001