(dev/core#217) PrevNext - Migrate selection methods #12556
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The contract feels quirky -- e.g. * Who would guess that `markSelection()` defaults to `$action == 'unselect'`? * What's the point of accepting `$entity_table` if it's never used? Fortunately, this function is only called from `CRM_Contact_Page_AJAX::selectUnselectContacts`, so it's fairly easy to audit and see that: * The `$action` is always passed in -- it never relies on the default value. * The `$entity_table` is never specified explicitly -- it always relies on the default value.
1. Improve docblock formatting 2. The `$entity_table` is never passed in. You can see this by grepping universe for `getSelection`.
This function is used one time -- when you run a search, select some
contacts, and perform a task (like "Delete contacts"), the
`CRM/Contact/Form/Task.php` displays a table with the names of the selected
contacts.
This patch makes the logic portable -- so that it can work regardless of
whether selections are stored in MySQL or Redis.
Before
------
* The contacts are selected `FROM civicrm_prevnext_cache WHERE cacheKey LIKE {$key}% AND cacheKey NOT LIKE {$key}_alphabet%`.
* The contact names come from `civicrm_prevnext_cache.data`, which has been
pre-filled with either `civicrm_contact.sort_name` (for most contact
searches) or `civicrm_contact.display_name` (for campaign respondent
searches).
After
-----
* The selections are chosen with `FROM civicrm_prevnext_cache WHERE cacheKey LIKE {$key}%`.
* The contact names are loaded directly from `civicrm_contact.sort_name`.
Comments
--------
* The use of wildcards with `cacheKey` seems like a code-smell suggest a
somewhat deeper problem in how `cacheKey` is understood. However, for our
purposes, it shouldn't matter.
* The before and after queries are very similar in how they use
`cacheKey`... and slightly different. (Ugh.) Is the new one better or
worse? Well, look at how `CRM_Contact_Form_Task` uses `getSelection()`
(for finding contact IDs, which feed into the task logic) and
`getSelectedContacts()` (for finding contact names, which are displayed to
user). If the subtle difference in `cacheKey` filtering matters, then our
UX is buggy because we're showing the user one contact-list (based on old
`getSelectedContacts()`) and we're executing on a different contact-list
(based on the old `getSelection()`).
* Is the old technique for getting names (querying
`civicrm_prevnext_cache.data`) better than the new technique (querying
civicrm_contact.sort_name)? I haven't benchmarked, but I'm skepitcal.
Both techniques transfer the full `O(n)` list from mysql to php.
In typical usage, the size of `n` is limited by what an admin is
willing to click through in the UI (which is probably a few hundred
IDs). The contact tables is indexed by ID. Maybe... if you manually
check several thousand records, it might make a small difference. But
if you're clicking that many, then other things are also getting more
expensive (like the actual task). In this case, it feels like an
unnecessary optimization.
|
(Standard links)
|
| * A list of contact IDs to (un)select. | ||
| * To unselect all contact IDs, use NULL. | ||
| */ | ||
| public function markSelection($cacheKey, $action, $cIds = NULL) { |
There was a problem hiding this comment.
$cIds is updated to the (better) $ids in the Interface & is inconsistent
There was a problem hiding this comment.
Agree $ids is nicer. I've added a revision to my big branch to use $ids. There are some other cleanups which touch that file, so it'll show up towards the end of the queue.
|
I noted one little thing which can be a follow up tidy up but this looks good to me. Merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Migrate methods for managing the selected contacts from
CRM_Core_BAO_PrevNextCachetoCRM_Core_PrevNextCache_{Interface,Sql}.This is a cherry-pick from #12377. See also: dev/core#217.
Before
markSelection()is a function inCRM_Core_BAO_PrevNextCache.getSelection() is a function inCRM_Core_BAO_PrevNextCache.getSelectedContacts()is a function inCRM_Core_BAO_PrevNextCache. It selects contact ID's based onFROM civicrm_prevnext_cache WHERE cacheKey LIKE {$key}% AND cacheKey NOT LIKE {$key}_alphabet%and returns their names.After
markSelection()is a function inCRM_Core_PrevNextCache_{Interface,Sql}. The unused parameter$entity_tableis no longer passed.getSelection() is a function inCRM_Core_PrevNextCache_{Interface,Sql}. The unused parameter$entity_tableis no longer passed.getSelectedContactNames()is a function inCRM_Contact_Form_Task. It selects contact ID's based ongetSelection()(akaFROM civicrm_prevnext_cache WHERE cacheKey LIKE {$key}%) and returns their names. (See commit notes for more critical evaluation of this change.)Technical Details
markSelection()is only called byCRM_Contact_Page_AJAX::selectUnselectContacts(akacivicrm/ajax/markSelection).civicrm/ajax/markSelection.getSelection()is called by:CRM_Contact_Page_AJAX::selectUnselectContacts(akacivicrm/ajax/markSelection).CRM_Campaign_Form_Task,CRM_Contact_Form_Search, andCRM_Contact_Form_Taskcom.imba.sendgrid. To smoot this transition, we leave the old BAO function as a stub.getSelectedContacts()is only called byCRM_Contact_Form_Task.