-
-
Notifications
You must be signed in to change notification settings - Fork 824
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.
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
Add / make fit for purpose email.getlist api call #16993
Conversation
(Standard links)
|
3128c94
to
dace9ba
Compare
dace9ba
to
9190330
Compare
Cool. I like the idea of a "fallback" field; like a poor-man's UNION. |
@colemanw I thought about having it as an array or similar but it felt like complexity that might never be needed & could be added later |
$request['params'][$defaults['search_field_fallback']] = $request['params'][$defaults['search_field']]; | ||
unset($request['params'][$defaults['search_field']]); | ||
$request['params']['options']['limit'] -= $result['count']; | ||
$result2 = civicrm_api3($entity, 'get', $request['params']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add something to the params here that excludes the original results.
NOT IN(array_keys($results))
would work partially, but it would only exclude the current page.
What about a "NOT LIKE" clause to exclude any results that would have been returned by searching on the original field.
The reason I bring this up is because we are dealing with a paging situation so duplicate results can mess up the pager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colemanw - that makes sense. In my testing it had no negative performance impact adding that - however my testing did demonstrate that the way it was doing the ordering WAS having a performance impact and that for this to perform well the sort_field needs to be the same as the filter field. I'm on the fence about doing a post-sort in php - as of now its
- sort_name matches, ordered by sort_name, labeled by sort_name padded with
- email matches, ordered by email, labeled by sort_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton this is looking really good. My one hesitation is that the 'NOT IN'
approach will possibly return redundant results because of the pager. Ex:
- Type a search query
- It returns 10 results.
- Scroll to the end and another 10 results appears (page 2)
- Scroll down again and 5 results are fetched by the main
$request
. It then fires the fallback request, using'NOT IN'
to exclude the 5 current results. - Since it did not exclude results from pages 1 & 2 (only the five contacts from page 3 were excluded) you'll get duplicates on page 3+.
- On page 4 you can get even more duplicates since main request will return 0 results, nothing will be excluded.
If it's not a big performance hit, the 'NOT LIKE'
method could fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it was OK in the performance tests below so trying it
I did some speed testing and we actually have to be a bit careful here - adding the filter above is OK but what is NOT OK is sorting by a field other than the filter. When you do that it uses the sort filter as the index and then does an unindexed filter. I'm pasting the results below. Note that the time for most queries was only between 10% and 50% of the original time when I tested without the is_deleted_sort_name index but the pattern of which were quicker was the same. SELECT SQL_NO_CACHE a.id as #10 rows in set (33.70 sec) With sort_name NOT LIKE 'bob%'SELECT SQL_NO_CACHE a.id as With email.id NOT IN (20,40,60,100,120)SELECT SQL_NO_CACHE a.id as ORDER BY #basic - order by email 10 rows in set (0.01 sec)With sort_name NOT LIKE 'go%' - order by emailSELECT SQL_NO_CACHE a.id as With email.id NOT IN (20,40,60,100,120) order by emailSELECT SQL_NO_CACHE a.id as ORDER BY a.email
#10 rows in set (0.01 sec) #first query - filter sort name, order display name 10 rows in set (15.02 sec)#first query - filter sort name, order sort name #first query - filter sort name, order sort name, correct casting Note that .02 sec was halved by dropping alter table civicrm_contact DROP INDEX index_is_deleted_sort_name; |
Ok I think I follow. So the upshot of your testing is that the default order_by should be set to something reasonably performant, yes? |
9190330
to
74a71fd
Compare
The function CRM_Contact_Page_AJAX::getContactEmail is one of our earlier ajax attempts & this approach has been largely replaced with entity Reference fields. In order to switch over we need to bring Email.getlist api to parity which means 1) searching on sortname first, if less than 10 results on emails include emails 2) appropriate respect for includeWildCardInName (this should already be in the generic getlist) 3) filter out on_hold, is_deceased, do_not_email 4) acl support (should already be part of the api). The trickiest of these to support is the first - because we need to avoid using a non-performant OR My current solution is the idea of a fallback field to search if the search results are less than the limit. in most cases this won't require a second query but when it does it should be fairly quick.
74a71fd
to
30420aa
Compare
retest this please |
@colemanw it passed! |
This introduces a new, as-yet-unused feature, with good test cover, so pretty safe to merge. Code & test looks good. |
Overview
Brings email.getlist api to functional parity with old ad hoc CRM_Contact_Page_AJAX::getContactEmail so we can switch calls over & deprecate / remove that
Before
Email.getlist api not really usable for entity reference fields due to gaps below
After
Parity with the legacy function
Technical Details
The function CRM_Contact_Page_AJAX::getContactEmail is one of our earlier ajax attempts & this approach has been largely
replaced with entity Reference fields. In order to switch over we need to bring Email.getlist api to parity which means
The trickiest of these to support is the first - because we need to avoid using a non-performant OR
My current solution is the idea of a fallback field to search if the search results are less than the limit.
in most cases this won't require a second query but when it does it should be fairly quick.
Comments
A notable gap is the ability to filter on groups & tags which don't easily join onto email.get in apiv3. I have left out of scope for now
Note this can be tested on the bcc field on #16936 (not the submit part as I haven't done that side yet). The current PR in that cleanup chain is #16954