CRM-17213 add tests for group search #8645
Conversation
This test ensures we don't break the group search while fixing CRM-17213
eileenmcnaughton
force-pushed
the
group_query
branch
4 times, most recently
from
91a92dc to
b49e891
Compare
Note that I added a group refresh to pick up the 'hard-adds'. I think the correct place to deal with this is in fact to alter the GroupContact Add functionality - ie.
IF (CRM_Core_DAO::executeQuery('SELECT id FROM civicrm_group_contact_cache WHERE group_id = 1 LIMIT 1)) {
// Add the contact just added to the group to the group_contact_cache table.
}
I started that a bit but I think it should be dealt with separately after this is resolved
eileenmcnaughton
force-pushed
the
group_query
branch
from
b49e891 to
485a3a1
Compare
|
test this please |
eileenmcnaughton
force-pushed
the
group_query
branch
4 times, most recently
from
f69d4ee to
0e716db
Compare
Note that in testing this I checked 1) searching in search builder with groups as a criteria (checked that correct groups show & only 'Added' ones) 2) Exporting - groups show per above when selected as an export field 3) Groups tab on a contact correctly shows added & removed groups 4) Api calls per tests 5) Manage groups - clicking through shows correct status for all members and it is possible to alter the group criteria to include 'Removed' and they show with the correct status
eileenmcnaughton
force-pushed
the
group_query
branch
from
0e716db to
3875e6b
Compare
|
CRM-17123 further removal of damaging OR query.
|
|
@seamuslee001 this is probably the most important patch in performance to QA - it provides a resolution to the group performance issue that would otherwise block ahem 'certain sites' upgrading to 4.7... |
|
Just noting have done some manual tests against a pretty oldish smart group created with search builder, previous situation it would time out on me when running query. Now it comes back perfect. Just also noting that the group_id value was '(400)' It was correctly selected in the search builder form however in the quill it was group(s) in (400) I would have expected the qill to translate to the group title not the raw value. However performance much much improved |
|
@seamuslee001 let me know when you feel it is OK to merge. As I mentioned in #8645 (comment) I tried to think of all the places where the way in which group title is rendered would make a difference. I have also deployed this live on one site |
|
Attaching screenshot of where qill isn't properly translated |
|
I could not create a group that did this - but I am aware from Seamus this is a legacy group. I notice in the legacy group form values there are braces around the group number - ie value";a:3:{i:1;a:5:{i:0;s:0:"";i:1;s:5:"(400)";
change it? |
|
@seamuslee001 I'm going to merge this as the only issue you found is minor & affects legacy groups & then tackle that in a separate PR if that's OK with you? We have another PR that has already started to conflict with this one so it would be good to get these changes in since they are mostly tested & QAd |
|
@eileenmcnaughton merge away |
Please add review comments to civicrm#8645 Note that in testing this I checked 1) searching in search builder with groups as a criteria (checked that correct groups show & only 'Added' ones) 2) Exporting - groups show per above when selected as an export field 3) Groups tab on a contact correctly shows added & removed groups 4) Api calls per tests 5) Manage groups - clicking through shows correct status for all members and it is possible to alter the group criteria to include 'Removed' and they show with the correct status Change-Id: I1304eaced73262f66030010bfcce05bc7d70d0b5
Please add review comments to civicrm#8645 Note that in testing this I checked 1) searching in search builder with groups as a criteria (checked that correct groups show & only 'Added' ones) 2) Exporting - groups show per above when selected as an export field 3) Groups tab on a contact correctly shows added & removed groups 4) Api calls per tests 5) Manage groups - clicking through shows correct status for all members and it is possible to alter the group criteria to include 'Removed' and they show with the correct status Change-Id: I1304eaced73262f66030010bfcce05bc7d70d0b5
This test ensures we don't break the group search while fixing CRM-17213