(dev/core#217) PrevNext - Allow swapping getPositions (etal) for contact-search #12558

totten · 2018-07-25T03:49:41Z

Overview

The PrevNext BAO provides three functions (getPositions(), deleteItem(), and getCount()) which are used in two disjoint use-cases -- contact-search and dedupe/merge. This PR makes the methods swappable but only for purposes of the contact-search use-case.

Relations:

This is a cherry-pick from (WIP) (dev/core#217) Allow replacement of PrevNextCache implementation for search screens #12377.
To avoid merge-conflicts, this PR extends/depends on (dev/core#217) PrevNext - Migrate selection methods #12556. In reviewing the commit list, you can recognize the items relevant to this PR as the last three commits ("Allow swapping ... for purposes of ...")
Main issue: dev/core#217.

Before

getPositions(), deleteItem(), and getCount() are defined in CRM_Core_BAO_PrevNextCache.
Both contact-search and dedupe/merge use-cases use CRM_Core_BAO_PrevNextCache.

After

getPositions(), deleteItem(), and getCount() are defined in CRM_Core_BAO_PrevNextCache.
For the dedupe/merge use-case, one still calls the original CRM_Core_BAO_PrevNextCache.
getPositions(), deleteItem(), and getCount() are defined in CRM_Core_PrevNextCache_{Interface,Sql}. (These basically just delegate to the BAO.)
For the contact-search use-case, one now calls the newer CRM_Core_PrevNextCache_{Interface,Sql}.

…ce interface

…e interface

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.

…s of contact-search The `getPositions()` function is used by both contact-search and dedupe-merge use-cases. ``` CRM/Contact/Form/Merge.php: $pos = CRM_Core_BAO_PrevNextCache::getPositions($cacheKey, $this->_cid, $this->_oid, $this->_mergeId, $join, $where, $flip); CRM/Contact/Form/Merge.php: $pos = CRM_Core_BAO_PrevNextCache::getPositions($cacheKey, NULL, NULL, $this->_mergeId, $join, $where); CRM/Contact/Page/View.php: $pos = CRM_Core_BAO_PrevNextCache::getPositions("civicrm search $qfKey", $this->_contactId, $this->_contactId); ``` Our aim in developing `CRM_Core_PrevNextCache_Interface` is to allow contact-search to swap a MySQL backend with a Redis backend -- and dedupe-merge should continue as-is (whether or not Redis is available). This basically means: * Contact-search switches to using `Civi::service('prevnext')->getPositions()` * Dedupe-merge continues using `CRM_Core_BAO_PrevNextCache::getPositions()` Note that the `Interface::getPositions()` is simpler than the BAO's variant. This is good because: * Contact-search doesn't need as many parameters. * Dedupe-merge still needs all the parameters. * Adding all parameters would make it hard to implement on other backends. (This is esp true of SQL-style options `$join` and `$where`.)

…of contact-search The `deleteItem()` function is used by both contact-search and dedupe-merge use-cases. We can classify several of these just based on the files: * Contact-search use-cases. (These should be updated to use the interface.) * `CRM/Campaign/Selector/Search.php: CRM_Core_BAO_PrevNextCache::deleteItem(NULL, $cacheKey, 'civicrm_contact');` * `CRM/Contact/Form/Search.php: CRM_Core_BAO_PrevNextCache::deleteItem(NULL, $cacheKey);` * `CRM/Contact/Selector.php: CRM_Core_BAO_PrevNextCache::deleteItem(NULL, $cacheKey, 'civicrm_contact');` * Dedupe-merge use-cases. (These should contiue using the BAO.) * `CRM/Contact/Form/DedupeRules.php: CRM_Core_BAO_PrevNextCache::deleteItem(NULL, $cacheKey);` * `CRM/Contact/Page/DedupeFind.php: CRM_Core_BAO_PrevNextCache::deleteItem(NULL, CRM_Dedupe_Merger::getMergeCacheKeyString($rgid, $gid, $criteria))` * `CRM/Dedupe/Merger.php: CRM_Core_BAO_PrevNextCache::deleteItem(NULL, "{$cacheKeyString}_stats");` Additionally, there are two oddballs which are harder to categorize. * `CRM_Contact_BAO_Contact_Utils::clearContactCaches($isEmptyPrevNextTable = FALSE)` deletes *all* prev-next cache-records (`CRM_Core_BAO_PrevNextCache::deleteItem();`). It only does so in one scenario (as part of `CRM/Contact/Import/Form/Preview.php`), which has this explanatory comment: "Clear all caches, forcing any searches to recheck the ACLs or group membership as the import may have changed it." * `CRM_Contact_BAO_Contact::deleteContact(...)` deletes any prev-next cache-records which reference a specific contact (`CRM_Core_BAO_PrevNextCache::deleteItem($id)`). I suppose this provides a low-grade form of referential integrity. Part of me thinks those should be re-considered (e.g. to use a hook/event -- and reduce the coupling between `Contact` and `PrevNext` subsystems). However, for purposes of dev/core#217, it seems OK to send `deleteItem(...)` to both BAO (SQL-only) and service (SQL-or-memory) variants.

… contact-search The `getCount()` function is used by both contact-search and dedupe-merge use-cases. * Contact-search * `CRM/Contact/Selector.php: $countRow = CRM_Core_BAO_PrevNextCache::getCount($cacheKey, NULL, "entity_table = 'civicrm_contact'");` * Dedupe-merge * `CRM/Contact/Page/AJAX.php: $iTotal = CRM_Core_BAO_PrevNextCache::getCount($cacheKeyString, $join, $whereClause, '=', $queryParams);` * `CRM/Contact/Page/DedupeMerge.php: $total = CRM_Core_BAO_PrevNextCache::getCount($cacheKeyString, NULL, ($onlyProcessSelected ? "pn.is_selected = 1" : NULL));` Our aim in developing `CRM_Core_PrevNextCache_Interface` is to allow contact-search to swap a MySQL backend with a Redis backend -- and dedupe-merge should continue as-is (whether or not Redis is available). This basically means: * Contact-search switches to using `Civi::service('prevnext')->getCount()` * Dedupe-merge continues using `CRM_Core_BAO_PrevNextCache::getCount()` Note that the `Interface::getCount()` is simpler than the BAO's variant. This is good because: * Contact-search doesn't need as many parameters. * Dedupe-merge still needs all the parameters. * Adding all parameters would make it hard to implement on other backends. (This is esp true of SQL-style options `$join` and `$where`.)

civibot · 2018-07-25T03:49:42Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2018-08-01T07:43:30Z

I just reviewed this - all except the last 3 commits are merged already. This seems to stll be mergeable so I'm merging to get the last 3 commits in

totten added 8 commits July 24, 2018 17:31

(dev/core#217) PrevNext - Migrate markSelection() from BAO to servi…

0b8038a

…ce interface

(dev/core#217) PrevNext - Migrate getSelection() from BAO to servic…

b799470

…e interface

(dev/core#217) PrevNext - Sanitize the getSelection() contract

40ddbe9

1. Improve docblock formatting 2. The `$entity_table` is never passed in. You can see this by grepping universe for `getSelection`.

civicrm-builder added the master label Jul 25, 2018

eileenmcnaughton merged commit 7471f8f into civicrm:master Aug 1, 2018

totten deleted the master-prevnext-misc branch August 14, 2018 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(dev/core#217) PrevNext - Allow swapping getPositions (etal) for contact-search #12558

(dev/core#217) PrevNext - Allow swapping getPositions (etal) for contact-search #12558

totten commented Jul 25, 2018

civibot bot commented Jul 25, 2018

eileenmcnaughton commented Aug 1, 2018

(dev/core#217) PrevNext - Allow swapping getPositions (etal) for contact-search #12558

(dev/core#217) PrevNext - Allow swapping getPositions (etal) for contact-search #12558

Conversation

totten commented Jul 25, 2018

Overview

Before

After

civibot bot commented Jul 25, 2018

eileenmcnaughton commented Aug 1, 2018