From 2eee38586eabb3f50d1ac094a8dbb5784137ff33 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 23 Sep 2021 09:57:43 -0400 Subject: [PATCH] SearchKit - Keep randomly sorted results stable across pages SearchKit allows ORDER BY RAND() but this can cause unexpected reshuffling when using the pager, editing-in-place or bulk-updating records. The solution is to generate a seed on the client-side when the display initializes, and re-use it every time results are fetched. This keeps the order stable, only reshuffling when the browser reloads the page. --- Civi/Api4/Query/SqlFunction.php | 5 +- Civi/Api4/Query/SqlFunctionRAND.php | 7 +- .../SearchDisplay/AbstractRunAction.php | 4 ++ .../Civi/Api4/Action/SearchDisplay/Run.php | 8 +++ .../traits/searchDisplayBaseTrait.service.js | 5 +- .../api/v4/SearchDisplay/SearchRunTest.php | 71 +++++++++++++++++++ 6 files changed, 97 insertions(+), 3 deletions(-) diff --git a/Civi/Api4/Query/SqlFunction.php b/Civi/Api4/Query/SqlFunction.php index fd10f80cca..b1d1c94093 100644 --- a/Civi/Api4/Query/SqlFunction.php +++ b/Civi/Api4/Query/SqlFunction.php @@ -68,7 +68,10 @@ abstract class SqlFunction extends SqlExpression { ]; if ($param['max_expr'] && (!$param['name'] || $param['name'] === $prefix)) { $exprs = $this->captureExpressions($arg, $param['must_be'], TRUE); - if (count($exprs) < $param['min_expr'] || count($exprs) > $param['max_expr']) { + if ( + (count($exprs) < $param['min_expr'] || count($exprs) > $param['max_expr']) && + !(!$exprs && $param['optional']) + ) { throw new \API_Exception('Incorrect number of arguments for SQL function ' . static::getName()); } $this->args[$idx]['expr'] = $exprs; diff --git a/Civi/Api4/Query/SqlFunctionRAND.php b/Civi/Api4/Query/SqlFunctionRAND.php index 7e834ea55e..8853973eb0 100644 --- a/Civi/Api4/Query/SqlFunctionRAND.php +++ b/Civi/Api4/Query/SqlFunctionRAND.php @@ -19,7 +19,12 @@ class SqlFunctionRAND extends SqlFunction { protected static $category = self::CATEGORY_MATH; protected static function params(): array { - return []; + return [ + [ + 'optional' => TRUE, + 'must_be' => ['SqlNumber'], + ], + ]; } /** diff --git a/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php b/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php index 88451018d2..faf18ec786 100644 --- a/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php +++ b/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php @@ -349,6 +349,10 @@ abstract class AbstractRunAction extends \Civi\Api4\Generic\AbstractAction { $orderBy = []; foreach ($currentSort ?: $defaultSort as $item) { + // Apply seed to random sorting + if ($item[0] === 'RAND()' && isset($this->seed)) { + $item[0] = 'RAND(' . $this->seed . ')'; + } $orderBy[$item[0]] = $item[1]; } return $orderBy; diff --git a/ext/search_kit/Civi/Api4/Action/SearchDisplay/Run.php b/ext/search_kit/Civi/Api4/Action/SearchDisplay/Run.php index 3ba06edbd3..4d09c7e81e 100644 --- a/ext/search_kit/Civi/Api4/Action/SearchDisplay/Run.php +++ b/ext/search_kit/Civi/Api4/Action/SearchDisplay/Run.php @@ -22,6 +22,14 @@ class Run extends AbstractRunAction { */ protected $limit; + /** + * Integer used as a seed when ordering by RAND(). + * This keeps the order stable enough to use a pager with random sorting. + * + * @var int + */ + protected $seed; + /** * @param \Civi\Api4\Generic\Result $result * @throws \API_Exception diff --git a/ext/search_kit/ang/crmSearchDisplay/traits/searchDisplayBaseTrait.service.js b/ext/search_kit/ang/crmSearchDisplay/traits/searchDisplayBaseTrait.service.js index 5a06852757..89752172d5 100644 --- a/ext/search_kit/ang/crmSearchDisplay/traits/searchDisplayBaseTrait.service.js +++ b/ext/search_kit/ang/crmSearchDisplay/traits/searchDisplayBaseTrait.service.js @@ -4,7 +4,8 @@ // Trait provides base methods and properties common to all search display types angular.module('crmSearchDisplay').factory('searchDisplayBaseTrait', function(crmApi4) { var ts = CRM.ts('org.civicrm.search_kit'), - runCount = 0; + runCount = 0, + seed = Date.now(); // Replace tokens keyed to rowData. // Pass view=true to replace with view value, otherwise raw value is used. @@ -73,6 +74,7 @@ var ctrl = this; this.limit = this.settings.limit; this.sort = this.settings.sort ? _.cloneDeep(this.settings.sort) : []; + this.seed = Date.now(); this.getResults = _.debounce(function() { $scope.$apply(function() { @@ -129,6 +131,7 @@ display: this.display, sort: this.sort, limit: this.limit, + seed: this.seed, filters: _.assign({}, (this.afFieldset ? this.afFieldset.getFieldData() : {}), this.filters), afform: this.afFieldset ? this.afFieldset.getFormName() : null }; diff --git a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php index c9bb14a4a9..20158ed149 100644 --- a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php +++ b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php @@ -497,4 +497,75 @@ class SearchRunTest extends \PHPUnit\Framework\TestCase implements HeadlessInter $this->assertStringContainsString('failed', $error); } + /** + * Test running a searchDisplay with random sorting. + */ + public function testSortByRand() { + $lastName = uniqid(__FUNCTION__); + $sampleData = [ + ['first_name' => 'One', 'last_name' => $lastName], + ['first_name' => 'Two', 'last_name' => $lastName], + ['first_name' => 'Three', 'last_name' => $lastName], + ['first_name' => 'Four', 'last_name' => $lastName], + ]; + Contact::save(FALSE)->setRecords($sampleData)->execute(); + + $params = [ + 'checkPermissions' => FALSE, + 'return' => 'page:1', + 'savedSearch' => [ + 'api_entity' => 'Contact', + 'api_params' => [ + 'version' => 4, + 'select' => ['id', 'first_name', 'last_name'], + 'where' => [['last_name', '=', $lastName]], + ], + ], + 'display' => [ + 'type' => 'list', + 'label' => '', + 'settings' => [ + 'limit' => 20, + 'pager' => TRUE, + 'columns' => [ + [ + 'key' => 'first_name', + 'label' => 'First Name', + 'dataType' => 'String', + 'type' => 'field', + ], + ], + 'sort' => [ + ['RAND()', 'ASC'], + ], + ], + ], + 'afform' => NULL, + ]; + + // Without seed, results are returned in unpredictable order + // (hard to test this, but we can at least assert we get the correct number of results back) + $unseeded = civicrm_api4('SearchDisplay', 'run', $params); + $this->assertCount(4, $unseeded); + + // Seed must be an integer + $params['seed'] = 'hello'; + try { + civicrm_api4('SearchDisplay', 'run', $params); + $this->fail(); + } + catch (\API_Exception $e) { + } + + // With a random seed, results should be shuffled in stable order + $params['seed'] = 12345678987654321; + $seeded = civicrm_api4('SearchDisplay', 'run', $params); + + // Same seed, same order every time + for ($i = 0; $i <= 9; ++$i) { + $repeat = civicrm_api4('SearchDisplay', 'run', $params); + $this->assertEquals($seeded->column('id'), $repeat->column('id')); + } + } + } -- 2.25.1