CRM-15908 saved searches that use multi-select custom groups break during 4.4 -> 4.5 upgrade

I'm still banging my head against this one.

In an effort to fix our smart groups - I'm temporarily patching our sites with this code that adjusts for this problem at the code level rather than the data level. It's far safer to implement, but I don't think it's the right long term solution:

diff --git a/CRM/Core/BAO/CustomQuery.php b/CRM/Core/BAO/CustomQuery.php
index 8ca1620..920182c 100644
— a/CRM/Core/BAO/CustomQuery.php
+++ b/CRM/Core/BAO/CustomQuery.php
@@ -364,6 +364,26 @@ SELECT label, value
$qillValue = '';
$sqlOP = $wildcard ? ' OR ' : ' AND ';
$sqlValue = array();
+ // Pre 4.5 custom fields with multiple options are in the format:
+ // array(value1 => '', value2 => 1, value3 => '', value4 => 1, CiviCRM_OP_OR => 1 )
+ // indicating that value2 and value4 are chosen.
+ // 4.5 changes the structure to just include the values chosen, e.g.
+ // array(value2, value4).
+ // The presence of the CiviCRM_OP_OR operator should be a clue as to which format
+ // we are gettting.
+ if(array_key_exists('CiviCRM_OP_OR', $value)) {
+ // Overwrite the sqlOP variable
+ if($value['CiviCRM_OP_OR'] == 1)

+ else

+ // Now unset it
+ unset($value['civiCRM_OP_OR']);
+ // And re-create the $value array
+ $value = array_keys($value, 1);
+ }
foreach ($value as $num => &$v) {
$sep = count($value) > (1 + $num) ? ', ' : (' ' . ($wildcard ? ts('OR') : ts('AND')) . ' ');
$qillValue .= ($num ? $sep : '') . $options[$v];

This file has three lines - each line is an example of a pre 4.5 form_value field that uses the old structure.

I'm un-assigning this in the hopes of getting another set of eyes to help resolve the problem. It's quite vexing.

I've patched ourpowerbase with the code to avert the major problems we're having. However, I think fixing the data is the real way to go.

Monish - can you take a look at this, and depending on the nature of the fix make a call about whether we should fix for 4.5.7 or 4.6. Thx!

I agree the db upgrade script is the way to go - and that code looks right to me.
I'd recommend adapting Jamie's code to run in batches in our upgrader and then try it out.

Submitted the PR https://github.com/civicrm/civicrm-core/pull/5153

This is very strange that when I checked the serialized data on my local against 4.4.5 I got a different data-structure
custom_NN = array('value1', 'value4', 'value5', 'CiviCRM_OP_OR') so I did the changes to get the correct format onto
custom_NN = array('value1', 'value4', 'value5')
custom_NN_operator = 'or'

Can you please verify my changes ? Thanks

Monish

Submitted a new PR https://github.com/civicrm/civicrm-core/pull/5280. Closed and covered flaws mentioned in earlier PR

Jamie, do you still have a 4.5 db you could test this with? We are getting very close to release date so would be great to get it verified asap.

Thanks for all the work on this - I will test today and report back results.

Sorry for the delay. I've tested the code and ran into some problems:

• I have field names in the format: custom_45_-1. The code parses the custom field id as -1 instead of 45.
• For the OP to be OR - in the old format - it has to both have the key CiviCRM_OP_OR and it has to have that value set to 1. This code just checks for the key
• The code seems to update the saved_search_table every time it finds a field that needs updating rather than once after all fields have been identified.

I just opened a new pull request here:

https://github.com/civicrm/civicrm-core/pull/5326

This seems to work on the databases I tested and it tries to also account for the data model Monish discovered (which does not exist in our data).

Thanks Jamie for improvising it I tested it on all kind of custom field element and it works for me.
#1 My bad I didn't find that format in 4.4.1. May exist on prior versions
#2The reason why I added switch case to run code on basis of html element type because I was not sure if there is any data model which can have value(not key) as 1 other than checkbox. But I guess CiviCRM_OP_OR as a key confirms the data-model is for checkbox ONLY.

There's only one thing I need to clarify. There is a special use-case where I found (reason why I needed $fieldID) if the user only select OR operator in other words data model would be
array (CiviCRM_OP_OR => 1), In this case the new data model expects all value to be selected and perform OR operation to all values

I did this to handle it
//If only Or operator has been chosen means we need to select all values and
+ //so to execute OR operation between these values according to new data structure
+ if (count($data_value) == 0) {
+ $customOption = CRM_Core_BAO_CustomOption::getCustomOption($fieldID);
+ foreach ($customOption as $option)

+}

I think its a miss in the latest change.

Ok, I've merged Jamie's PR - Monish can you submit another PR for additional changes?

Thanks Coleman for merging and Monish for the review!

I was confused by the use case where the user selects OR and does not have any fields. I'm probably wrong (since I didn't have the data to check it) - but I thought that these two would produce the same query:

1. OR is selected and no values (nothing would be added to the query)
2. OR is selected and all values are selected (and query would be generated to search for all possible values)

Maybe in example two - records with no value would be omitted - so that would be different?

Sorry for delay and not cited the special use-case with screenshot, please check http://snag.gy/Uh0aV.jpg . So this is the scenario when old data-model appears like array(CiviCRM_OP_OR => 1) where operator should be applied on all values in new data model

Submitted the PR https://github.com/civicrm/civicrm-core/pull/5472 to support this use-case

Hi Monisha - Thanks for the follow up.

I just did some testing on a 4.4 site that I have. On the 4.4 site, I used the "Best time to contact" field which has three options: Morning, Afternoon, Evening. It's a checkbox style option.

I first filled out the form just like Monish did in the screen shot above - in other words, I didn't choose any of the options and I did put an x in the "Check to match ANY; uncheck to match ALL" box. After submitting I got all the records in the database (in other words - this search did not filter records in 4.4). In fact, it didn't even limit the results to the contacts that have a corresponding field in the custom value table. It really did return all the contacts in the database (this seems like the right behavior - no values are chosen, no filtering should happen).

Next, I did a search on the same field, but this time I selected all the options and put an x in the "Check to match ANY; uncheck to match ALL" box. This time, I only got 60 records. Turns out, there are only 60 records in the database in which this field has a value at all, so the search returned all of them. Also, this seems like the right behavior.

I then experimented on an upgraded version of the site. In 4.5, you can't chose All vs. Any if you don't select at least two options (which seems right). So, I selected all the options and then choose "Any" and I got 60 records. This seems like perfectly sensible behavior.

Now here's the problem that I think we will run into with this patch:

Suppose I saved a search with several different criteria. In the course of creating this search - I accidentally clicked the "Check to match ANY; uncheck to match ALL box" for the "Best time to contact" field and didn't select any values. In my 4.4 site, this will have no impact on the search results.

However, if we transform the search with the patch provided, in the 4.5 site the results will suddenly be filtered down to the 60 records with a value in this field.

jamie

Jamie - really appreciate your attention to detail with this!
So do you guys think this issue is complete without merging the new PR, or is there anything missing?

I have merged the PR and closing this issue after discussion with Jamie