Merge pull request #10416 from mabashian/10400-search-keys

Addresses bug where advanced search by groups wasn't working on host list

SUMMARY
link #10400
OK so this got a bit more complicated than I wanted it to but I think on the whole this is a nice improvement.  In the initial bug report it was noted that we were ignoring the or/not operators when searching on a related key (in this case it was groups).  We were also ignoring the modifier at the end (icontains, exact, startswith, etc).  Since we were always defaulting to __search for these types of keys this was actually valid.  The API doesn't like us attempting to do something like ?or__groups__search=foo or ?groups__search__icontains because they aren't valid.  So, I changed the UX a little bit.  A user can still do a fuzzy search on a related key but the prefix is disabled in this case:

I changed the third dropdown (specifically for related keys) to contain the following options: search, id, name__icontains which I think would be the three most general cases.  id and name__icontains do allow for prefixing (or, not, and).
This should make searching on related keys a little bit easier and a little bit more flexible since name and id weren't possible in the UI before.
Once place where this is a little different is the host filter in the smart inventory form.  Using __search currently throws an error when the user attempts to save.  I believe this is an API bug and will file it as such but for now I prevent users from attempting to use __search by removing it as an option for this particular list.  When this bug gets fixed in the API we can remove this logic.
I also noticed that there was a bug where or__ search terms were being converted to and when using the Smart Inventory button on the host list so I fixed that.
Here's the fix in action:

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

UI

Reviewed-by: Kersom <None>
Reviewed-by: Tiago Góes <tiago.goes2009@gmail.com>

awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx

@@ -40,6 +40,7 @@ function DataListToolbar({
   qsConfig,
   pagination,
   enableNegativeFiltering,
+  enableRelatedFuzzyFiltering,
 }) {
   const showExpandCollapse = onCompact && onExpand;
   const [isKebabOpen, setIsKebabOpen] = useState(false);
@@ -92,6 +93,7 @@ function DataListToolbar({
               onShowAdvancedSearch={onShowAdvancedSearch}
               onRemove={onRemove}
               enableNegativeFiltering={enableNegativeFiltering}
+              enableRelatedFuzzyFiltering={enableRelatedFuzzyFiltering}
             />
           </ToolbarItem>
           {sortColumns && (
@@ -173,6 +175,7 @@ DataListToolbar.propTypes = {
   onSort: PropTypes.func,
   additionalControls: PropTypes.arrayOf(PropTypes.node),
   enableNegativeFiltering: PropTypes.bool,
+  enableRelatedFuzzyFiltering: PropTypes.bool,
 };
 
 DataListToolbar.defaultProps = {
@@ -192,6 +195,7 @@ DataListToolbar.defaultProps = {
   onSort: null,
   additionalControls: [],
   enableNegativeFiltering: true,
+  enableRelatedFuzzyFiltering: true,
 };
 
 export default DataListToolbar;

awx/ui_next/src/components/Lookup/HostFilterLookup.jsx

@@ -120,6 +120,7 @@ function HostFilterLookup({
   organizationId,
   value,
   enableNegativeFiltering,
+  enableRelatedFuzzyFiltering,
 }) {
   const history = useHistory();
   const location = useLocation();
@@ -370,6 +371,7 @@ function HostFilterLookup({
                 {...props}
                 fillWidth
                 enableNegativeFiltering={enableNegativeFiltering}
+                enableRelatedFuzzyFiltering={enableRelatedFuzzyFiltering}
               />
             )}
             toolbarSearchColumns={searchColumns}
@@ -404,6 +406,7 @@ HostFilterLookup.propTypes = {
   organizationId: number,
   value: string,
   enableNegativeFiltering: bool,
+  enableRelatedFuzzyFiltering: bool,
 };
 HostFilterLookup.defaultProps = {
   isValid: true,
@@ -412,6 +415,7 @@ HostFilterLookup.defaultProps = {
   organizationId: null,
   value: '',
   enableNegativeFiltering: true,
+  enableRelatedFuzzyFiltering: true,
 };
 
 export default withRouter(HostFilterLookup);

awx/ui_next/src/components/Lookup/shared/HostFilterUtils.jsx

@@ -7,9 +7,18 @@ export function toSearchParams(string = '') {
   if (string === '') {
     return {};
   }
-  return string
+
-    .replace(/^\?/, '')
+  const readableParamsStr = string.replace(/^\?/, '').replace(/&/g, ' and ');
-    .replace(/&/g, ' and ')
+  const orArr = readableParamsStr.split(/ or /);
+
+  if (orArr.length > 1) {
+    orArr.forEach((str, index) => {
+      orArr[index] = `or__${str}`;
+    });
+  }
+
+  return orArr
+    .join(' and ')
     .split(/ and | or /)
     .map(s => s.split('='))
     .reduce((searchParams, [k, v]) => {

awx/ui_next/src/components/Lookup/shared/HostFilterUtils.test.jsx

@@ -33,6 +33,20 @@ describe('toSearchParams', () => {
     };
     expect(toSearchParams(string)).toEqual(paramsObject);
   });
+  test('should take a host filter string separated by or and return search params object with or', () => {
+    string = 'foo=bar or foo=baz';
+    paramsObject = {
+      or__foo: ['bar', 'baz'],
+    };
+    expect(toSearchParams(string)).toEqual(paramsObject);
+  });
+  test('should take a host filter string with or and return search params object with or', () => {
+    string = 'or__foo=1&or__foo=2';
+    paramsObject = {
+      or__foo: ['1', '2'],
+    };
+    expect(toSearchParams(string)).toEqual(paramsObject);
+  });
 });
 
 describe('toQueryString', () => {
@@ -108,6 +122,13 @@ describe('toHostFilter', () => {
       'name=foo or name__contains=bar or name__iexact=foo'
     );
   });
+
+  test('should return a host filter with or conditional when value is array', () => {
+    const object = {
+      or__groups__id: ['1', '2'],
+    };
+    expect(toHostFilter(object)).toEqual('groups__id=1 or groups__id=2');
+  });
 });
 
 describe('removeNamespacedKeys', () => {

awx/ui_next/src/components/Search/AdvancedSearch.jsx

@@ -1,7 +1,6 @@
 import 'styled-components/macro';
-import React, { useState } from 'react';
+import React, { useEffect, useState } from 'react';
 import PropTypes from 'prop-types';
-
 import { t } from '@lingui/macro';
 import {
   Button,
@@ -33,6 +32,7 @@ function AdvancedSearch({
   relatedSearchableKeys,
   maxSelectHeight,
   enableNegativeFiltering,
+  enableRelatedFuzzyFiltering,
 }) {
   // TODO: blocked by pf bug, eventually separate these into two groups in the select
   // for now, I'm spreading set to get rid of duplicate keys...when they are grouped
@@ -48,30 +48,40 @@ function AdvancedSearch({
   const [lookupSelection, setLookupSelection] = useState(null);
   const [keySelection, setKeySelection] = useState(null);
   const [searchValue, setSearchValue] = useState('');
+  const [relatedSearchKeySelected, setRelatedSearchKeySelected] = useState(
+    false
+  );
   const config = useConfig();
 
+  useEffect(() => {
+    if (
+      keySelection &&
+      relatedSearchableKeys.indexOf(keySelection) > -1 &&
+      searchableKeys.indexOf(keySelection) === -1
+    ) {
+      setLookupSelection('name__icontains');
+      setRelatedSearchKeySelected(true);
+    } else {
+      setLookupSelection(null);
+      setRelatedSearchKeySelected(false);
+    }
+  }, [keySelection]); // eslint-disable-line react-hooks/exhaustive-deps
+
+  useEffect(() => {
+    if (lookupSelection === 'search') {
+      setPrefixSelection(null);
+    }
+  }, [lookupSelection]); // eslint-disable-line react-hooks/exhaustive-deps
+
   const handleAdvancedSearch = e => {
     // keeps page from fully reloading
     e.preventDefault();
 
     if (searchValue) {
       const actualPrefix = prefixSelection === 'and' ? null : prefixSelection;
-      let actualSearchKey;
+      const actualSearchKey = [actualPrefix, keySelection, lookupSelection]
-      // TODO: once we are able to group options for the key typeahead, we will
-      // probably want to be able to which group a key was clicked in for duplicates,
-      // rather than checking to make sure it's not in both for this appending
-      // __search logic
-      if (
-        relatedSearchableKeys.indexOf(keySelection) > -1 &&
-        searchableKeys.indexOf(keySelection) === -1 &&
-        keySelection.indexOf('__') === -1
-      ) {
-        actualSearchKey = `${keySelection}__search`;
-      } else {
-        actualSearchKey = [actualPrefix, keySelection, lookupSelection]
         .filter(val => !!val)
         .join('__');
-      }
       onSearch(actualSearchKey, searchValue);
       setSearchValue('');
     }
@@ -83,8 +93,7 @@ function AdvancedSearch({
     }
   };
 
-  return (
+  const renderSetType = () => (
-    <AdvancedGroup>
     <Select
       ouiaId="set-type-typeahead"
       aria-label={t`Set type select`}
@@ -99,6 +108,7 @@ function AdvancedSearch({
       placeholderText={t`Set type`}
       maxHeight={maxSelectHeight}
       noResultsFoundText={t`No results found`}
+      isDisabled={lookupSelection === 'search'}
     >
       <SelectOption
         id="and-option-select"
@@ -121,33 +131,47 @@ function AdvancedSearch({
         />
       )}
     </Select>
+  );
+
+  const renderRelatedLookupType = () => (
     <Select
-        ouiaId="set-key-typeahead"
+      ouiaId="set-lookup-typeahead"
-        aria-label={t`Key select`}
+      aria-label={t`Related search type`}
-        className="keySelect"
+      className="lookupSelect"
       variant={SelectVariant.typeahead}
-        typeAheadAriaLabel={t`Key typeahead`}
+      typeAheadAriaLabel={t`Related search type typeahead`}
-        onToggle={setIsKeyDropdownOpen}
+      onToggle={setIsLookupDropdownOpen}
-        onSelect={(event, selection) => setKeySelection(selection)}
+      onSelect={(event, selection) => setLookupSelection(selection)}
-        onClear={() => setKeySelection(null)}
+      selections={lookupSelection}
-        selections={keySelection}
+      isOpen={isLookupDropdownOpen}
-        isOpen={isKeyDropdownOpen}
+      placeholderText={t`Related search type`}
-        placeholderText={t`Key`}
-        isCreatable
-        onCreateOption={setKeySelection}
       maxHeight={maxSelectHeight}
       noResultsFoundText={t`No results found`}
     >
-        {allKeys.map(optionKey => (
       <SelectOption
-            key={optionKey}
+        id="name-option-select"
-            value={optionKey}
+        key="name__icontains"
-            id={`select-option-${optionKey}`}
+        value="name__icontains"
-          >
+        description={t`Fuzzy search on name field.`}
-            {optionKey}
+      />
-          </SelectOption>
+      <SelectOption
-        ))}
+        id="id-option-select"
+        key="id"
+        value="id"
+        description={t`Exact search on id field.`}
+      />
+      {enableRelatedFuzzyFiltering && (
+        <SelectOption
+          id="search-option-select"
+          key="search"
+          value="search"
+          description={t`Fuzzy search on id, name or description fields.`}
+        />
+      )}
     </Select>
+  );
+
+  const renderLookupType = () => (
     <Select
       ouiaId="set-lookup-typeahead"
       aria-label={t`Lookup select`}
@@ -175,6 +199,7 @@ function AdvancedSearch({
         value="iexact"
         description={t`Case-insensitive version of exact.`}
       />
+
       <SelectOption
         id="contains-option-select"
         key="contains"
@@ -260,6 +285,49 @@ function AdvancedSearch({
         description={t`Check whether the given field's value is present in the list provided; expects a comma-separated list of items.`}
       />
     </Select>
+  );
+
+  return (
+    <AdvancedGroup>
+      {lookupSelection === 'search' ? (
+        <Tooltip
+          content={t`Set type disabled for related search field fuzzy searches`}
+        >
+          {renderSetType()}
+        </Tooltip>
+      ) : (
+        renderSetType()
+      )}
+      <Select
+        ouiaId="set-key-typeahead"
+        aria-label={t`Key select`}
+        className="keySelect"
+        variant={SelectVariant.typeahead}
+        typeAheadAriaLabel={t`Key typeahead`}
+        onToggle={setIsKeyDropdownOpen}
+        onSelect={(event, selection) => setKeySelection(selection)}
+        onClear={() => setKeySelection(null)}
+        selections={keySelection}
+        isOpen={isKeyDropdownOpen}
+        placeholderText={t`Key`}
+        isCreatable
+        onCreateOption={setKeySelection}
+        maxHeight={maxSelectHeight}
+        noResultsFoundText={t`No results found`}
+      >
+        {allKeys.map(optionKey => (
+          <SelectOption
+            key={optionKey}
+            value={optionKey}
+            id={`select-option-${optionKey}`}
+          >
+            {optionKey}
+          </SelectOption>
+        ))}
+      </Select>
+      {relatedSearchKeySelected
+        ? renderRelatedLookupType()
+        : renderLookupType()}
       <InputGroup>
         <TextInput
           data-cy="advanced-search-text-input"
@@ -303,6 +371,7 @@ AdvancedSearch.propTypes = {
   relatedSearchableKeys: PropTypes.arrayOf(PropTypes.string),
   maxSelectHeight: PropTypes.string,
   enableNegativeFiltering: PropTypes.bool,
+  enableRelatedFuzzyFiltering: PropTypes.bool,
 };
 
 AdvancedSearch.defaultProps = {
@@ -310,6 +379,7 @@ AdvancedSearch.defaultProps = {
   relatedSearchableKeys: [],
   maxSelectHeight: '300px',
   enableNegativeFiltering: true,
+  enableRelatedFuzzyFiltering: true,
 };
 
 export default AdvancedSearch;

awx/ui_next/src/components/Search/AdvancedSearch.test.jsx

@@ -209,6 +209,29 @@ describe('<AdvancedSearch />', () => {
         .prop('onKeyDown')({ key: 'Enter', preventDefault: jest.fn });
     });
     wrapper.update();
+    expect(advancedSearchMock).toBeCalledWith('baz__name__icontains', 'bar');
+    jest.clearAllMocks();
+    act(() => {
+      wrapper.find('Select[aria-label="Key select"]').invoke('onCreateOption')(
+        'baz'
+      );
+    });
+    wrapper.update();
+    act(() => {
+      wrapper
+        .find('Select[aria-label="Related search type"]')
+        .invoke('onSelect')({}, 'search');
+      wrapper
+        .find('TextInputBase[aria-label="Advanced search value input"]')
+        .invoke('onChange')('bar');
+    });
+    wrapper.update();
+    act(() => {
+      wrapper
+        .find('TextInputBase[aria-label="Advanced search value input"]')
+        .prop('onKeyDown')({ key: 'Enter', preventDefault: jest.fn });
+    });
+    wrapper.update();
     expect(advancedSearchMock).toBeCalledWith('baz__search', 'bar');
   });
 
@@ -217,7 +240,7 @@ describe('<AdvancedSearch />', () => {
     wrapper = mountWithContexts(
       <AdvancedSearch
         onSearch={advancedSearchMock}
-        searchableKeys={[]}
+        searchableKeys={['foo']}
         relatedSearchableKeys={[]}
       />
     );
@@ -230,6 +253,9 @@ describe('<AdvancedSearch />', () => {
         {},
         'foo'
       );
+    });
+    wrapper.update();
+    act(() => {
       wrapper.find('Select[aria-label="Lookup select"]').invoke('onSelect')(
         {},
         'exact'
@@ -253,7 +279,7 @@ describe('<AdvancedSearch />', () => {
     wrapper = mountWithContexts(
       <AdvancedSearch
         onSearch={advancedSearchMock}
-        searchableKeys={[]}
+        searchableKeys={['foo']}
         relatedSearchableKeys={[]}
       />
     );
@@ -265,6 +291,9 @@ describe('<AdvancedSearch />', () => {
       wrapper.find('Select[aria-label="Key select"]').invoke('onCreateOption')(
         'foo'
       );
+    });
+    wrapper.update();
+    act(() => {
       wrapper.find('Select[aria-label="Lookup select"]').invoke('onSelect')(
         {},
         'exact'
@@ -305,6 +334,9 @@ describe('<AdvancedSearch />', () => {
       wrapper.find('Select[aria-label="Key select"]').invoke('onCreateOption')(
         'foo'
       );
+    });
+    wrapper.update();
+    act(() => {
       wrapper.find('Select[aria-label="Lookup select"]').invoke('onSelect')(
         {},
         'exact'
@@ -363,4 +395,34 @@ describe('<AdvancedSearch />', () => {
       selectOptions.find('SelectOption[id="and-option-select"]').prop('value')
     ).toBe('and');
   });
+
+  test('Remove search option from related search type', () => {
+    wrapper = mountWithContexts(
+      <AdvancedSearch
+        onSearch={jest.fn}
+        searchableKeys={['foo', 'bar']}
+        relatedSearchableKeys={['bar', 'baz']}
+        enableRelatedFuzzyFiltering={false}
+      />
+    );
+    act(() => {
+      wrapper.find('Select[aria-label="Key select"]').invoke('onCreateOption')(
+        'baz'
+      );
+    });
+    wrapper.update();
+    wrapper
+      .find('Select[aria-label="Related search type"] SelectToggle')
+      .simulate('click');
+    const selectOptions = wrapper.find(
+      'Select[aria-label="Related search type"] SelectOption'
+    );
+    expect(selectOptions).toHaveLength(2);
+    expect(
+      selectOptions.find('SelectOption[id="name-option-select"]').prop('value')
+    ).toBe('name__icontains');
+    expect(
+      selectOptions.find('SelectOption[id="id-option-select"]').prop('value')
+    ).toBe('id');
+  });
 });

awx/ui_next/src/components/Search/Search.jsx

@@ -43,6 +43,7 @@ function Search({
   isDisabled,
   maxSelectHeight,
   enableNegativeFiltering,
+  enableRelatedFuzzyFiltering,
 }) {
   const [isSearchDropdownOpen, setIsSearchDropdownOpen] = useState(false);
   const [searchKey, setSearchKey] = useState(
@@ -207,6 +208,7 @@ function Search({
               relatedSearchableKeys={relatedSearchableKeys}
               maxSelectHeight={maxSelectHeight}
               enableNegativeFiltering={enableNegativeFiltering}
+              enableRelatedFuzzyFiltering={enableRelatedFuzzyFiltering}
             />
           )) ||
             (options && (
@@ -327,6 +329,7 @@ Search.propTypes = {
   isDisabled: PropTypes.bool,
   maxSelectHeight: PropTypes.string,
   enableNegativeFiltering: PropTypes.bool,
+  enableRelatedFuzzyFiltering: PropTypes.bool,
 };
 
 Search.defaultProps = {
@@ -335,6 +338,7 @@ Search.defaultProps = {
   isDisabled: false,
   maxSelectHeight: '300px',
   enableNegativeFiltering: true,
+  enableRelatedFuzzyFiltering: true,
 };
 
 export default withRouter(Search);

awx/ui_next/src/screens/Host/HostList/HostList.jsx

@@ -1,9 +1,7 @@
 import React, { useEffect, useCallback } from 'react';
 import { useHistory, useLocation, useRouteMatch } from 'react-router-dom';
-
 import { t } from '@lingui/macro';
 import { Card, PageSection } from '@patternfly/react-core';
-
 import { HostsAPI } from '../../../api';
 import AlertModal from '../../../components/AlertModal';
 import DataListToolbar from '../../../components/DataListToolbar';
@@ -46,8 +44,15 @@ function HostList() {
     }
   });
 
-  const hasNonDefaultSearchParams =
+  const hasInvalidHostFilterKeys = () => {
-    Object.keys(nonDefaultSearchParams).length > 0;
+    const nonDefaultSearchKeys = Object.keys(nonDefaultSearchParams);
+    return (
+      nonDefaultSearchKeys.filter(searchKey => searchKey.startsWith('not__'))
+        .length > 0 ||
+      nonDefaultSearchKeys.filter(searchKey => searchKey.endsWith('__search'))
+        .length > 0
+    );
+  };
 
   const {
     result: { hosts, count, actions, relatedSearchableKeys, searchableKeys },
@@ -185,7 +190,11 @@ function HostList() {
                 ...(canAdd
                   ? [
                       <SmartInventoryButton
-                        isDisabled={!hasNonDefaultSearchParams}
+                        hasInvalidKeys={hasInvalidHostFilterKeys()}
+                        isDisabled={
+                          Object.keys(nonDefaultSearchParams).length === 0 ||
+                          hasInvalidHostFilterKeys()
+                        }
                         onClick={() => handleSmartInventoryClick()}
                       />,
                     ]

awx/ui_next/src/screens/Host/HostList/SmartInventoryButton.jsx

@@ -1,13 +1,24 @@
 import React from 'react';
-import { func } from 'prop-types';
+import { bool, func } from 'prop-types';
 import { Button, DropdownItem, Tooltip } from '@patternfly/react-core';
-
 import { t } from '@lingui/macro';
 import { useKebabifiedMenu } from '../../../contexts/Kebabified';
 
-function SmartInventoryButton({ onClick, isDisabled }) {
+function SmartInventoryButton({ onClick, isDisabled, hasInvalidKeys }) {
   const { isKebabified } = useKebabifiedMenu();
 
+  const renderTooltipContent = () => {
+    if (hasInvalidKeys) {
+      return t`Some search modifiers like not__ and __search are not supported in Smart Inventory host filters.  Remove these to create a new Smart Inventory with this filter.`;
+    }
+    if (isDisabled) {
+      return t`Enter at least one search filter to create a new Smart Inventory`;
+    }
+
+    return t`Create a new Smart Inventory with the applied filter`;
+  };
+
+  const renderContent = () => {
     if (isKebabified) {
       return (
         <DropdownItem
@@ -22,16 +33,6 @@ function SmartInventoryButton({ onClick, isDisabled }) {
     }
 
     return (
-    <Tooltip
-      key="smartInventory"
-      content={
-        !isDisabled
-          ? t`Create a new Smart Inventory with the applied filter`
-          : t`Enter at least one search filter to create a new Smart Inventory`
-      }
-      position="top"
-    >
-      <div>
       <Button
         ouiaId="smart-inventory-button"
         onClick={onClick}
@@ -41,12 +42,28 @@ function SmartInventoryButton({ onClick, isDisabled }) {
       >
         {t`Smart Inventory`}
       </Button>
-      </div>
+    );
+  };
+
+  return (
+    <Tooltip
+      key="smartInventory"
+      content={renderTooltipContent()}
+      position="top"
+    >
+      <div>{renderContent()}</div>
     </Tooltip>
   );
 }
 SmartInventoryButton.propTypes = {
+  hasInvalidKeys: bool,
+  isDisabled: bool,
   onClick: func.isRequired,
 };
 
+SmartInventoryButton.defaultProps = {
+  hasInvalidKeys: false,
+  isDisabled: false,
+};
+
 export default SmartInventoryButton;

awx/ui_next/src/screens/Inventory/shared/SmartInventoryForm.jsx

@@ -82,6 +82,7 @@ const SmartInventoryFormFields = ({ inventory }) => {
         isValid={!hostFilterMeta.touched || !hostFilterMeta.error}
         isDisabled={!organizationField.value}
         enableNegativeFiltering={false}
+        enableRelatedFuzzyFiltering={false}
       />
       <InstanceGroupsLookup
         value={instanceGroupsField.value}