From 0dbe04b1ee4683224f68c2a7e26e989e4b85e99a Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Mon, 2 Jul 2018 16:22:06 -0700
Subject: [PATCH] (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      | 25 ++++++++++++++++++++++++-
 CRM/Core/BAO/PrevNextCache.php | 24 ------------------------
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/CRM/Contact/Form/Task.php b/CRM/Contact/Form/Task.php
index 5cacbcc77b..369d594513 100644
--- a/CRM/Contact/Form/Task.php
+++ b/CRM/Contact/Form/Task.php
@@ -273,7 +273,7 @@ class CRM_Contact_Form_Task extends CRM_Core_Form_Task {
     ) {
       $sel = CRM_Utils_Array::value('radio_ts', self::$_searchFormValues);
       $form->assign('searchtype', $sel);
-      $result = CRM_Core_BAO_PrevNextCache::getSelectedContacts();
+      $result = self::getSelectedContactNames();
       $form->assign("value", $result);
     }
 
@@ -477,6 +477,29 @@ class CRM_Contact_Form_Task extends CRM_Core_Form_Task {
     }
   }
 
+  /**
+   * @return array
+   *   List of contact names.
+   *   NOTE: These are raw values from the DB. In current data-model, that means
+   *   they are pre-encoded HTML.
+   */
+  private static function getSelectedContactNames() {
+    $qfKey = CRM_Utils_Request::retrieve('qfKey', 'String');
+    $cacheKey = "civicrm search {$qfKey}";
+
+    $cids = array();
+    // Gymanstic time!
+    foreach (Civi::service('prevnext')->getSelection($cacheKey) as $cacheKey => $values) {
+      $cids = array_unique(array_merge($cids, array_keys($values)));
+    }
+
+    $result = CRM_Utils_SQL_Select::from('civicrm_contact')
+      ->where('id IN (#cids)', ['cids' => $cids])
+      ->execute()
+      ->fetchMap('id', 'sort_name');
+    return $result;
+  }
+
   /**
    * Given this task's list of targets, produce a hidden group.
    *
diff --git a/CRM/Core/BAO/PrevNextCache.php b/CRM/Core/BAO/PrevNextCache.php
index 8b19aef581..c3078140ac 100644
--- a/CRM/Core/BAO/PrevNextCache.php
+++ b/CRM/Core/BAO/PrevNextCache.php
@@ -452,30 +452,6 @@ AND        c.created_date < date_sub( NOW( ), INTERVAL %2 day )
     return Civi::service('prevnext')->getSelection($cacheKey, $action);
   }
 
-  /**
-   * @return array
-   */
-  public static function getSelectedContacts() {
-    $qfKey = CRM_Utils_Request::retrieve('qfKey', 'String');
-    $cacheKey = "civicrm search {$qfKey}";
-    $query = "
-SELECT *
-FROM   civicrm_prevnext_cache
-WHERE  cacheKey LIKE %1
-  AND  is_selected=1
-  AND  cacheKey NOT LIKE %2
-";
-    $params1[1] = array("{$cacheKey}%", 'String');
-    $params1[2] = array("{$cacheKey}_alphabet%", 'String');
-    $dao = CRM_Core_DAO::executeQuery($query, $params1);
-
-    $val = array();
-    while ($dao->fetch()) {
-      $val[] = $dao->data;
-    }
-    return $val;
-  }
-
   /**
    * Flip 2 contacts in the prevNext cache.
    *
-- 
2.25.1