From: colemanw Date: Sun, 1 Oct 2023 18:04:35 +0000 (-0400) Subject: SearchKit - Fix and add test for scenario where the same display is used twice on... X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=db8a7f7784fb5444582a2b7ce6cd1e355698df8d;p=civicrm-core.git SearchKit - Fix and add test for scenario where the same display is used twice on the same form --- diff --git a/Civi/Api4/Service/Spec/Provider/OptionGroupCreationSpecProvider.php b/Civi/Api4/Service/Spec/Provider/OptionGroupCreationSpecProvider.php new file mode 100644 index 0000000000..738a80b614 --- /dev/null +++ b/Civi/Api4/Service/Spec/Provider/OptionGroupCreationSpecProvider.php @@ -0,0 +1,38 @@ +getFieldByName('name')->setRequired(FALSE); + $spec->getFieldByName('title')->setRequiredIf('empty($values.name)'); + } + + /** + * @inheritDoc + */ + public function applies($entity, $action) { + return $entity === 'OptionGroup' && $action === 'create'; + } + +} diff --git a/ext/afform/mock/ang/testDisplaysSharingSameFieldset.aff.html b/ext/afform/mock/ang/testDisplaysSharingSameFieldset.aff.html new file mode 100644 index 0000000000..9133093287 --- /dev/null +++ b/ext/afform/mock/ang/testDisplaysSharingSameFieldset.aff.html @@ -0,0 +1,7 @@ +
+
+ +
+ + +
diff --git a/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php b/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php index 427a34f717..010951ad33 100644 --- a/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php +++ b/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php @@ -1388,14 +1388,17 @@ abstract class AbstractRunAction extends \Civi\Api4\Generic\AbstractAction { if ($filterAttr && is_string($filterAttr) && $filterAttr[0] === '{') { foreach (\CRM_Utils_JS::decode($filterAttr) as $filterKey => $filterVal) { // Automatically apply filters from the markup if they have a value - if ($filterVal !== NULL) { + // Only do this if there's one instance of the display on the form + if ($afform['searchDisplay']['count'] === 1 && $filterVal !== NULL) { unset($this->filters[$filterKey]); if ($this->hasValue($filterVal)) { $this->applyFilter(explode(',', $filterKey), $filterVal); } } // If it's a javascript variable it will have come back from decode() as NULL; - // whitelist it to allow it to be passed to this api from javascript. + // Or if there's more than one instance of the display on the form, they might + // use different filters. + // Just whitelist it so the value passed in will be accepted. else { $filterKeys[] = $filterKey; } @@ -1423,22 +1426,37 @@ abstract class AbstractRunAction extends \Civi\Api4\Generic\AbstractAction { if (empty($afform['layout'])) { return FALSE; } + $afform['searchDisplay'] = NULL; // Get all search display fieldsets (which will have an empty value for the af-fieldset attribute) $fieldsets = \CRM_Utils_Array::findAll($afform['layout'], ['af-fieldset' => '']); // As a fallback, search the entire afform in case the search display is not in a fieldset $fieldsets['form'] = $afform['layout']; - // Validate that the afform contains this search display + // Search for one or more instance of this search display foreach ($fieldsets as $key => $fieldset) { - $afform['searchDisplay'] = \CRM_Utils_Array::findAll( - $fieldset, - ['#tag' => $this->display['type:name'], 'search-name' => $this->savedSearch['name'], 'display-name' => $this->display['name']] - )[0] ?? NULL; + if ($key === 'form' && $afform['searchDisplay']) { + // Already found in a fieldset, don't search the whole form + continue; + } + $displays = \CRM_Utils_Array::findAll( + $fieldset, + ['#tag' => $this->display['type:name'], 'search-name' => $this->savedSearch['name'], 'display-name' => $this->display['name']] + ); + if (!$displays) { + continue; + } + // Already found, just increment the count if ($afform['searchDisplay']) { + $afform['searchDisplay']['count'] += count($displays); + } + else { + $afform['searchDisplay'] = $displays[0]; + $afform['searchDisplay']['count'] = count($displays); // Set the fieldset for this display (if it is in one and we haven't fallen back to the whole form) + // TODO: This just uses the first fieldset, but there could be multiple. Potentially could use filters to match it. $afform['searchDisplay']['fieldset'] = $key === 'form' ? [] : $fieldset; - return $this->_afform = $afform; } } + $this->_afform = $afform; } return $this->_afform; } diff --git a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchAfformTest.php b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchAfformTest.php index 377cb8d7b9..311de97de9 100644 --- a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchAfformTest.php +++ b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchAfformTest.php @@ -5,6 +5,8 @@ use Civi\Api4\Action\Afform\Save; use Civi\Api4\Afform; use Civi\Api4\Contact; use Civi\Api4\Email; +use Civi\Api4\OptionGroup; +use Civi\Api4\OptionValue; use Civi\Api4\Phone; use Civi\Api4\SavedSearch; use Civi\Api4\SearchDisplay; @@ -33,7 +35,7 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn /** * Test running a searchDisplay within an afform. */ - public function testRunWithAfform() { + public function testRunWithAfform(): void { $search = SavedSearch::create(FALSE) ->setValues([ 'name' => 'TestContactEmailSearch', @@ -180,7 +182,7 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn $this->assertCount(1, $result); } - public function testRunMultipleSearchForm() { + public function testRunMultipleSearchForm(): void { $email = uniqid('tester@'); Contact::create(FALSE) @@ -230,8 +232,8 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn ) ->execute(); - $contactEmailSearch = SavedSearch::create(FALSE) - ->setValues([ + $contactEmailSearch = SavedSearch::save(FALSE) + ->addRecord([ 'name' => 'TestContactEmailSearch', 'label' => 'TestContactEmailSearch', 'api_entity' => 'Contact', @@ -257,10 +259,11 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn 'having' => [], ], ]) + ->setMatch(['name']) ->execute()->first(); - $contactEmailDisplay = SearchDisplay::create(FALSE) - ->setValues([ + $contactEmailDisplay = SearchDisplay::save(FALSE) + ->addRecord([ 'name' => 'TestContactEmailDisplay', 'label' => 'TestContactEmailDisplay', 'saved_search_id.name' => 'TestContactEmailSearch', @@ -291,11 +294,12 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn ], 'acl_bypass' => FALSE, ]) + ->setMatch(['name']) ->execute()->first(); foreach (['Email', 'Phone'] as $entity) { - SavedSearch::create(FALSE) - ->setValues([ + SavedSearch::save(FALSE) + ->addRecord([ 'name' => 'TestSearchFor' . $entity, 'label' => 'TestSearchFor' . $entity, 'api_entity' => $entity, @@ -312,6 +316,7 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn 'having' => [], ], ]) + ->setMatch(['name']) ->execute(); } @@ -349,9 +354,9 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn $this->assertCount(1, $result); } - public function testSearchReferencesToAfform() { - $search = SavedSearch::create(FALSE) - ->setValues([ + public function testSearchReferencesToAfform(): void { + $search = SavedSearch::save(FALSE) + ->addRecord([ 'name' => 'TestSearchToDelete', 'label' => 'TestSearchToDelete', 'api_entity' => 'Contact', @@ -360,10 +365,11 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn 'select' => ['id'], ], ]) + ->setMatch(['name']) ->execute()->first(); - $display = SearchDisplay::create(FALSE) - ->setValues([ + $display = SearchDisplay::save(FALSE) + ->addRecord([ 'name' => 'TestDisplayToDelete', 'label' => 'TestDisplayToDelete', 'saved_search_id.name' => 'TestSearchToDelete', @@ -380,6 +386,7 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn ], 'acl_bypass' => FALSE, ]) + ->setMatch(['saved_search_id', 'name']) ->execute()->first(); // The search should have one reference (its display) @@ -443,4 +450,97 @@ class SearchAfformTest extends \PHPUnit\Framework\TestCase implements HeadlessIn $this->assertCount(0, Afform::get(FALSE)->addWhere('name', 'CONTAINS', 'TestAfformToDelete')->execute()); } + public function testDisplaysSharingSameFieldset(): void { + OptionGroup::save(FALSE) + ->addRecord([ + 'title' => 'search_test_options', + ]) + ->setMatch(['title']) + ->execute(); + OptionValue::save(FALSE) + ->setDefaults(['option_group_id.name' => 'search_test_options']) + ->addRecord([ + 'label' => 'option_a', + 'value' => 'a', + ]) + ->addRecord([ + 'label' => 'option_b', + 'value' => 'b', + ]) + ->addRecord([ + 'label' => 'option_c', + 'value' => 'c', + 'is_active' => FALSE, + ]) + ->setMatch(['name', 'option_group_id']) + ->execute(); + + $search = SavedSearch::save(FALSE) + ->addRecord([ + 'name' => 'testDisplaysSharingSameFieldset', + 'label' => 'testDisplaysSharingSameFieldset', + 'api_entity' => 'OptionValue', + 'api_params' => [ + 'version' => 4, + 'select' => ['value'], + ], + ]) + ->setMatch(['name']) + ->execute()->first(); + + $display = SearchDisplay::save(FALSE) + ->addRecord([ + 'name' => 'testDisplaysSharingSameFieldset', + 'label' => 'testDisplaysSharingSameFieldset', + 'saved_search_id.name' => 'testDisplaysSharingSameFieldset', + 'type' => 'table', + 'settings' => [ + 'columns' => [ + [ + 'key' => 'value', + 'type' => 'field', + ], + ], + ], + 'acl_bypass' => FALSE, + ]) + ->setMatch(['saved_search_id', 'name']) + ->execute()->first(); + + $baseParams = [ + 'return' => 'page:1', + 'savedSearch' => $search['name'], + 'display' => $display['name'], + 'afform' => 'testDisplaysSharingSameFieldset', + 'filters' => ['option_group_id:name' => 'search_test_options'], + ]; + + // Afform has 2 copies of the same display, with different values for the filter is_active + // This should allow the is_active filters to be set in the params + $params = $baseParams; + $params['filters']['is_active'] = TRUE; + $result = civicrm_api4('SearchDisplay', 'run', $params); + $this->assertCount(2, $result); + + $params = $baseParams; + $params['filters']['is_active'] = FALSE; + $result = civicrm_api4('SearchDisplay', 'run', $params); + $this->assertCount(1, $result); + $this->assertEquals('c', $result[0]['columns'][0]['val']); + + // Because the 2 displays share a fieldset, the filter field should work on both + $params = $baseParams; + $params['filters']['is_active'] = TRUE; + $params['filters']['label'] = 'b'; + $result = civicrm_api4('SearchDisplay', 'run', $params); + $this->assertCount(1, $result); + $this->assertEquals('b', $result[0]['columns'][0]['val']); + + $params = $baseParams; + $params['filters']['is_active'] = FALSE; + $params['filters']['label'] = 'b'; + $result = civicrm_api4('SearchDisplay', 'run', $params); + $this->assertCount(0, $result); + } + }