(dev/core#217) PrevNext - Make getSelectedContacts() more portable
authorTim Otten <totten@civicrm.org>
Mon, 2 Jul 2018 23:22:06 +0000 (16:22 -0700)
committerTim Otten <totten@civicrm.org>
Wed, 25 Jul 2018 00:31:11 +0000 (17:31 -0700)
commit0dbe04b1ee4683224f68c2a7e26e989e4b85e99a
tree5bca3ee13d59039aa66590bbbd0167452f8738f9
parent40ddbe99be49f36c08d24f2955ecf09b06dc4ef6
(dev/core#217) PrevNext - Make getSelectedContacts() more portable

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.
CRM/Contact/Form/Task.php
CRM/Core/BAO/PrevNextCache.php