assorted ui_next search phase 1 pr feedback updates

- remove unnecessary displayAll prop from ChipGroup
- update notification api fn to be 2 with no boolean param
- fix params passing to api functions

awx/ui_next/SEARCH.md

@@ -108,8 +108,6 @@ Similar to the way the list grabs data based on changes to the react-router para
 
 Currently the filter tags do not display the key, though that data is available and they could very easily do so.
 
-Some small changes were made to our custom ChipGroup component to make this possible--a new displayAll bool prop which bypasses all chip hiding based on number of chips.
-
 ## QS Updates (and supporting duplicate keys)
 
 The logic that was updated to handle search tags can be found in the qs.js util file.

awx/ui_next/src/api/mixins/Notifications.mixin.js

@@ -14,14 +14,14 @@ const NotificationsMixin = parent =>
     readNotificationTemplatesSuccess(id, params) {
       return this.http.get(
         `${this.baseUrl}${id}/notification_templates_success/`,
-        params
+        { params }
       );
     }
 
     readNotificationTemplatesError(id, params) {
       return this.http.get(
         `${this.baseUrl}${id}/notification_templates_error/`,
-        params
+        { params }
       );
     }
 
@@ -54,43 +54,58 @@ const NotificationsMixin = parent =>
     }
 
     /**
-     * This is a helper method meant to simplify setting the "on" or "off" status of
+     * This is a helper method meant to simplify setting the "on" status of
      * a related notification.
      *
      * @param[resourceId] - id of the base resource
      * @param[notificationId] - id of the notification
      * @param[notificationType] - the type of notification, options are "success" and "error"
-     * @param[associationState] - Boolean for associating or disassociating, options are true or false
      */
-    // eslint-disable-next-line max-len
+    associateNotificationTemplate(
-    updateNotificationTemplateAssociation(
       resourceId,
       notificationId,
-      notificationType,
+      notificationType
-      associationState
     ) {
-      if (notificationType === 'success' && associationState === true) {
+      if (notificationType === 'success') {
         return this.associateNotificationTemplatesSuccess(
           resourceId,
           notificationId
         );
       }
 
-      if (notificationType === 'success' && associationState === false) {
+      if (notificationType === 'error') {
-        return this.disassociateNotificationTemplatesSuccess(
-          resourceId,
-          notificationId
-        );
-      }
-
-      if (notificationType === 'error' && associationState === true) {
         return this.associateNotificationTemplatesError(
           resourceId,
           notificationId
         );
       }
 
-      if (notificationType === 'error' && associationState === false) {
+      throw new Error(
+        `Unsupported notificationType for association: ${notificationType}`
+      );
+    }
+
+    /**
+     * This is a helper method meant to simplify setting the "off" status of
+     * a related notification.
+     *
+     * @param[resourceId] - id of the base resource
+     * @param[notificationId] - id of the notification
+     * @param[notificationType] - the type of notification, options are "success" and "error"
+     */
+    disassociateNotificationTemplate(
+      resourceId,
+      notificationId,
+      notificationType
+    ) {
+      if (notificationType === 'success') {
+        return this.disassociateNotificationTemplatesSuccess(
+          resourceId,
+          notificationId
+        );
+      }
+
+      if (notificationType === 'error') {
         return this.disassociateNotificationTemplatesError(
           resourceId,
           notificationId
@@ -98,7 +113,7 @@ const NotificationsMixin = parent =>
       }
 
       throw new Error(
-        `Unsupported notificationType, associationState combination: ${notificationType}, ${associationState}`
+        `Unsupported notificationType for disassociation: ${notificationType}`
       );
     }
   };

awx/ui_next/src/components/Chip/ChipGroup.jsx

@@ -1,15 +1,9 @@
 import React, { useState } from 'react';
-import { number, bool } from 'prop-types';
+import { number } from 'prop-types';
 import styled from 'styled-components';
 import Chip from './Chip';
 
-const ChipGroup = ({
+const ChipGroup = ({ children, className, showOverflowAfter, ...props }) => {
-  children,
-  className,
-  showOverflowAfter,
-  displayAll,
-  ...props
-}) => {
   const [isExpanded, setIsExpanded] = useState(!showOverflowAfter);
   const toggleIsOpen = () => setIsExpanded(!isExpanded);
 
@@ -26,8 +20,8 @@ const ChipGroup = ({
 
   return (
     <ul className={`pf-c-chip-group ${className}`} {...props}>
-      {displayAll ? mappedChildren : mappedChildren.slice(0, numToShow)}
+      {!showOverflowAfter ? mappedChildren : mappedChildren.slice(0, numToShow)}
-      {!displayAll && showOverflowToggle && (
+      {showOverflowAfter && showOverflowToggle && (
         <Chip isOverflowChip onClick={toggleIsOpen} component="li">
           {isExpanded ? expandedText : collapsedText}
         </Chip>
@@ -37,11 +31,9 @@ const ChipGroup = ({
 };
 ChipGroup.propTypes = {
   showOverflowAfter: number,
-  displayAll: bool,
 };
 ChipGroup.defaultProps = {
   showOverflowAfter: null,
-  displayAll: false,
 };
 
 export default styled(ChipGroup)`

awx/ui_next/src/components/FilterTags/FilterTags.jsx

@@ -40,7 +40,6 @@ const FilterTags = ({
 }) => {
   const queryParams = parseQueryString(qsConfig, location.search);
   const queryParamsArr = [];
-  const displayAll = true;
   const nonDefaultParams = filterDefaultParams(
     Object.keys(queryParams),
     qsConfig
@@ -59,7 +58,7 @@ const FilterTags = ({
         <ResultCount>{`${itemCount} results`}</ResultCount>
         <VerticalSeparator />
         <FilterLabel>{i18n._(t`Active Filters:`)}</FilterLabel>
-        <ChipGroup displayAll={displayAll}>
+        <ChipGroup>
           {queryParamsArr.map(({ key, value }) => (
             <Chip
               className="searchTagChip"

awx/ui_next/src/components/PaginatedDataList/PaginatedDataList.jsx

@@ -41,7 +41,9 @@ class PaginatedDataList extends React.Component {
     const { history, qsConfig } = this.props;
     const { search } = history.location;
     const oldParams = parseQueryString(qsConfig, search);
-    this.pushHistoryState(addParams(qsConfig, oldParams, { page_size: pageSize }));
+    this.pushHistoryState(
+      addParams(qsConfig, oldParams, { page_size: pageSize })
+    );
   }
 
   pushHistoryState(params) {

awx/ui_next/src/screens/Organization/OrganizationAccess/OrganizationAccess.jsx

@@ -75,7 +75,7 @@ class OrganizationAccess extends React.Component {
       } = await OrganizationsAPI.readAccessList(organization.id, params);
       this.setState({ itemCount, accessRecords });
     } catch (err) {
-      this.setState({ cotentError: err });
+      this.setState({ contentError: err });
     } finally {
       this.setState({ hasContentLoading: false });
     }

awx/ui_next/src/screens/Organization/OrganizationAccess/__snapshots__/OrganizationAccessItem.test.jsx.snap

@@ -714,7 +714,6 @@ exports[`<OrganizationAccessItem /> initially renders succesfully 1`] = `
                                               >
                                                 <ChipGroup
                                                   className="ChipGroup-sc-10zu8t0-0 ldAQsx"
-                                                  displayAll={false}
                                                   showOverflowAfter={null}
                                                 >
                                                   <ul

awx/ui_next/src/screens/Organization/OrganizationNotifications/OrganizationNotifications.jsx

@@ -151,12 +151,19 @@ class OrganizationNotifications extends Component {
 
     this.setState({ toggleLoading: true });
     try {
-      await OrganizationsAPI.updateNotificationTemplateAssociation(
+      if (isCurrentlyOn) {
-        id,
+        await OrganizationsAPI.disassociateNotificationTemplate(
-        notificationId,
+          id,
-        status,
+          notificationId,
-        !isCurrentlyOn
+          status
-      );
+        );
+      } else {
+        await OrganizationsAPI.associateNotificationTemplate(
+          id,
+          notificationId,
+          status
+        );
+      }
       this.setState(stateUpdateFunction);
     } catch (err) {
       this.setState({ toggleError: true });

awx/ui_next/src/screens/Organization/OrganizationNotifications/OrganizationNotifications.test.jsx

@@ -96,9 +96,11 @@ describe('<OrganizationNotifications />', () => {
       .find('Switch')
       .at(0)
       .prop('onChange')();
-    expect(
+    expect(OrganizationsAPI.associateNotificationTemplate).toHaveBeenCalledWith(
-      OrganizationsAPI.updateNotificationTemplateAssociation
+      1,
-    ).toHaveBeenCalledWith(1, 2, 'success', true);
+      2,
+      'success'
+    );
     await sleep(0);
     wrapper.update();
     expect(
@@ -122,9 +124,11 @@ describe('<OrganizationNotifications />', () => {
       .find('Switch')
       .at(1)
       .prop('onChange')();
-    expect(
+    expect(OrganizationsAPI.associateNotificationTemplate).toHaveBeenCalledWith(
-      OrganizationsAPI.updateNotificationTemplateAssociation
+      1,
-    ).toHaveBeenCalledWith(1, 1, 'error', true);
+      1,
+      'error'
+    );
     await sleep(0);
     wrapper.update();
     expect(
@@ -149,8 +153,8 @@ describe('<OrganizationNotifications />', () => {
       .at(0)
       .prop('onChange')();
     expect(
-      OrganizationsAPI.updateNotificationTemplateAssociation
+      OrganizationsAPI.disassociateNotificationTemplate
-    ).toHaveBeenCalledWith(1, 1, 'success', false);
+    ).toHaveBeenCalledWith(1, 1, 'success');
     await sleep(0);
     wrapper.update();
     expect(
@@ -175,8 +179,8 @@ describe('<OrganizationNotifications />', () => {
       .at(1)
       .prop('onChange')();
     expect(
-      OrganizationsAPI.updateNotificationTemplateAssociation
+      OrganizationsAPI.disassociateNotificationTemplate
-    ).toHaveBeenCalledWith(1, 2, 'error', false);
+    ).toHaveBeenCalledWith(1, 2, 'error');
     await sleep(0);
     wrapper.update();
     expect(

awx/ui_next/src/util/qs.js

@@ -320,10 +320,7 @@ const getRemainingNewParams = (mergedParams, newParams) =>
  * @return {object} query param object
  */
 export function addParams(config, oldParams, paramsToAdd) {
-  const namespacedOldParams = namespaceParams(
+  const namespacedOldParams = namespaceParams(config.namespace, oldParams);
-    config.namespace,
-    oldParams
-  );
   const namespacedParamsToAdd = namespaceParams(config.namespace, paramsToAdd);
   const namespacedDefaultParams = namespaceParams(
     config.namespace,

awx/ui_next/src/util/qs.test.js

@@ -403,7 +403,11 @@ describe('qs (qs.js)', () => {
         defaultParams: { page: 1, page_size: 15 },
         integerFields: ['page', 'page_size'],
       };
-      const oldParams = { baz: ['bar', 'bang', 'bust'], page: 3, page_size: 15 };
+      const oldParams = {
+        baz: ['bar', 'bang', 'bust'],
+        page: 3,
+        page_size: 15,
+      };
       const newParams = { baz: 'bar' };
       expect(removeParams(config, oldParams, newParams)).toEqual({
         baz: ['bang', 'bust'],
@@ -433,7 +437,12 @@ describe('qs (qs.js)', () => {
         defaultParams: { page: 1, page_size: 15 },
         integerFields: ['page', 'page_size'],
       };
-      const oldParams = { baz: ['bar', 'bang', 'bust'], pat: 'pal', page: 3, page_size: 15 };
+      const oldParams = {
+        baz: ['bar', 'bang', 'bust'],
+        pat: 'pal',
+        page: 3,
+        page_size: 15,
+      };
       const newParams = { baz: 'bust', pat: 'pal' };
       expect(removeParams(config, oldParams, newParams)).toEqual({
         baz: ['bar', 'bang'],
@@ -491,7 +500,11 @@ describe('qs (qs.js)', () => {
         defaultParams: { page: 1, page_size: 15 },
         integerFields: ['page', 'page_size'],
       };
-      const oldParams = { baz: ['bar', 'bang', 'bust'], page: 3, page_size: 15 };
+      const oldParams = {
+        baz: ['bar', 'bang', 'bust'],
+        page: 3,
+        page_size: 15,
+      };
       const newParams = { baz: 'bar' };
       expect(removeParams(config, oldParams, newParams)).toEqual({
         baz: ['bang', 'bust'],
@@ -521,7 +534,12 @@ describe('qs (qs.js)', () => {
         defaultParams: { page: 1, page_size: 15 },
         integerFields: ['page', 'page_size'],
       };
-      const oldParams = { baz: ['bar', 'bang', 'bust'], pat: 'pal', page: 3, page_size: 15 };
+      const oldParams = {
+        baz: ['bar', 'bang', 'bust'],
+        pat: 'pal',
+        page: 3,
+        page_size: 15,
+      };
       const newParams = { baz: 'bust', pat: 'pal' };
       expect(removeParams(config, oldParams, newParams)).toEqual({
         baz: ['bar', 'bang'],

awx/ui_next/testUtils/apiReusable.jsx

@@ -8,23 +8,36 @@ export function describeNotificationMixin (Model, name) {
       jest.clearAllMocks();
     });
 
-    const parameters = [
+    const parameters = ['success', 'error'];
-      ['success', true],
+
-      ['success', false],
+    parameters.forEach((type) => {
-      ['error', true],
+      const label = `[notificationType=${type}, associationState=true`;
-      ['error', false],
+      const testName = `associateNotificationTemplate ${label} makes expected http calls`;
-    ];
-    parameters.forEach(([type, state]) => {
-      const label = `[notificationType=${type}, associationState=${state}]`;
-      const testName = `updateNotificationTemplateAssociation ${label} makes expected http calls`;
 
       test(testName, async (done) => {
-        await ModelAPI.updateNotificationTemplateAssociation(1, 21, type, state);
+        await ModelAPI.associateNotificationTemplate(1, 21, type);
 
         const expectedPath = `${ModelAPI.baseUrl}1/notification_templates_${type}/`;
         expect(mockHttp.post).toHaveBeenCalledTimes(1);
 
-        const expectedParams = state ? { id: 21 } : { id: 21, disassociate: true };
+        const expectedParams = { id: 21 };
+        expect(mockHttp.post.mock.calls.pop()).toEqual([expectedPath, expectedParams]);
+
+        done();
+      });
+    });
+
+    parameters.forEach((type) => {
+      const label = `[notificationType=${type}, associationState=false`;
+      const testName = `disassociateNotificationTemplate ${label} makes expected http calls`;
+
+      test(testName, async (done) => {
+        await ModelAPI.disassociateNotificationTemplate(1, 21, type);
+
+        const expectedPath = `${ModelAPI.baseUrl}1/notification_templates_${type}/`;
+        expect(mockHttp.post).toHaveBeenCalledTimes(1);
+
+        const expectedParams = { id: 21, disassociate: true };
         expect(mockHttp.post.mock.calls.pop()).toEqual([expectedPath, expectedParams]);
 
         done();