dev/core#389 Fix custom data relative date searching #14625
Conversation
|
(Standard links)
|
15c1e08 to
d9efbd5
Compare
| @@ -6919,7 +6910,9 @@ protected function buildRelativeDateQuery(&$values) { | |||
| $tableName = $fieldSpec['table_name']; | |||
| $filters = CRM_Core_OptionGroup::values('relative_date_filters'); | |||
| $grouping = CRM_Utils_Array::value(3, $values); | |||
| $this->_tables[$tableName] = $this->_whereTables[$tableName] = 1; | |||
| // If the table value is already set for a custom field it will be more nuanced than just '1'. | |||
| $this->_tables[$tableName] = $this->_tables[$tableName] ?? 1; | |||
There was a problem hiding this comment.
@seamuslee001 that is php 7.0 syntax - see https://stackoverflow.com/questions/34571330/php-ternary-operator-vs-null-coalescing-operator - it avoids enotices without lots of isset...
|
I'll check the fails... |
|
Jenkins re test this please |
|
@eileenmcnaughton can you rebase please |
c9d7aba to
798697c
Compare
|
@eileenmcnaughton test failures look related |
|
Did some simple tests and the PR works as expected. |
798697c to
bc6d843
Compare
This addresses a fairly old regression where smart groups based on relative dates stopped being relative
bc6d843 to
69d0728
Compare
|
Test fails were a real bug - have fixed (& altered one test to reflect changed expectations) |
69d0728 to
7f17570
Compare
|
@seamuslee001 do you think we can get this merged before the rc? I probably has changed enough since @samuelsov tested to require a new r-run though :-( |
|
i have re-perfomered an r-run and confirmed that both old and new format of smart group appear to be working sensibly and in tandem and the smart group values that were saved in the newer format look pretty standard, Tests have also been expanded with this PR and we saw some test failures before where they were legit so we know we have good test coverage here merging |
|
Thanks @seamuslee001 nice to have this fixed |
|
@seamuslee001 just noting we are not fixing previously saved groups :-( This field is still on jcalendar so perhaps we should priorise moving it over & we might fix some in the process since the format looks like it would be picked up - https://lab.civicrm.org/dev/core/issues/389#note_17643 |
|
@eileenmcnaughton what would be your suggestion then? |
|
@seamuslee001 so I don't think we have to prioritise adding an upgrade script just because we have fixed new saves - but it makes sense to try to incorporate it into our 'get rid of jcalendar' effort |
Overview
This addresses a fairly old regression where smart groups based on relative dates for custom fields stopped being relative & started using the date as of the query being saved.
Note this will not fix previously saved groups
Before
Create a smart group based on a custom field using relative dates (to do this on a default data set you need to alter marriage date custom field to support range searching and also ensure some contacts have this data. Today is a good date range to choose!
Check the smart group can be reloaded.
Check that when the date changes the search contents do too...
After
The smart group can still be created but it no longer stores any fixed dates in the saved_search.form_value field. The 'relative_dates' field is also no longer used for any fields that use datepicker
a:8:{i:0;a:5:{i:0;s:8:"entryURL";i:1;s:1:"=";i:2;s:105:"http://dmaster.local/civicrm/admin/custom/group/field/update?action=update&reset=1&gid=1&id=3";i:3;i:0;i:4;i:0;}i:1;a:5:{i:0;s:21:"group_search_selected";i:1;s:1:"=";i:2;s:5:"group";i:3;i:0;i:4;i:0;}i:2;a:5:{i:0;s:16:"privacy_operator";i:1;s:1:"=";i:2;s:2:"OR";i:3;i:0;i:4;i:0;}i:3;a:5:{i:0;s:14:"privacy_toggle";i:1;s:1:"=";i:2;s:1:"1";i:3;i:0;i:4;i:0;}i:4;a:5:{i:0;s:18:"custom_11_operator";i:1;s:1:"=";i:2;s:2:"or";i:3;i:0;i:4;i:0;}i:5;a:5:{i:0;s:17:"custom_3_relative";i:1;s:1:"=";i:2;s:9:"this.year";i:3;i:0;i:4;i:0;}i:6;a:5:{i:0;s:8:"operator";i:1;s:1:"=";i:2;s:3:"AND";i:3;i:0;i:4;i:0;}i:7;a:5:{i:0;s:14:"component_mode";i:1;s:1:"=";i:2;s:1:"1";i:3;i:0;i:4;i:0;}}
Technical Details
This does not fix any existing groups (& I am leaving that out of scope for safety & because it is an old regression & people may have 'taken steps' but any new ones saved will work, hence no upgrade script
Comments
https://lab.civicrm.org/dev/core/issues/389