Testing Improvements and Refactoring

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroup/InventoryGroup.jsx

@@ -14,8 +14,8 @@ import InventoryGroupDetail from '../InventoryGroupDetail/InventoryGroupDetail';
 
 function InventoryGroups({ i18n, match, setBreadcrumb, inventory, history }) {
   const [inventoryGroup, setInventoryGroup] = useState(null);
-  const [hasContentLoading, setContentLoading] = useState(true);
-  const [hasContentError, setHasContentError] = useState(false);
+  const [hasContentLoading, setHasContentLoading] = useState(true);
+  const [contentError, setHasContentError] = useState(null);
 
   useEffect(() => {
     const loadData = async () => {
@@ -26,12 +26,18 @@ function InventoryGroups({ i18n, match, setBreadcrumb, inventory, history }) {
       } catch (err) {
         setHasContentError(err);
       } finally {
-        setContentLoading(false);
+        setHasContentLoading(false);
       }
     };
 
     loadData();
-  }, [match.params.groupId, setBreadcrumb, inventory]);
+  }, [
+    history.location.pathname,
+    match.params.groupId,
+    inventory,
+    setBreadcrumb,
+  ]);
+
   const tabsArray = [
     {
       name: i18n._(t`Return to Groups`),
@@ -46,7 +52,7 @@ function InventoryGroups({ i18n, match, setBreadcrumb, inventory, history }) {
       id: 0,
     },
     {
-      name: i18n._(t`RelatedGroups`),
+      name: i18n._(t`Related Groups`),
       link: `/inventories/inventory/${inventory.id}/groups/${inventoryGroup &&
         inventoryGroup.id}/nested_groups`,
       id: 1,
@@ -58,26 +64,28 @@ function InventoryGroups({ i18n, match, setBreadcrumb, inventory, history }) {
       id: 2,
     },
   ];
-  if (hasContentError) {
-    return <ContentError />;
+  if (contentError) {
+    return <ContentError error={contentError} />;
   }
   if (hasContentLoading) {
     return <ContentLoading />;
   }
-  let cardHeader = hasContentLoading ? null : (
-    <CardHeader style={{ padding: 0 }}>
-      <RoutedTabs history={history} tabsArray={tabsArray} />
-      <CardCloseButton
-        linkTo={`/inventories/inventory/${inventory.id}/group`}
-      />
-    </CardHeader>
-  );
+
+  let cardHeader = null;
   if (
-    !history.location.pathname.includes('groups/') ||
-    history.location.pathname.endsWith('edit')
+    history.location.pathname.includes('groups/') &&
+    !history.location.pathname.endsWith('edit')
   ) {
-    cardHeader = null;
+    cardHeader = (
+      <CardHeader style={{ padding: 0 }}>
+        <RoutedTabs history={history} tabsArray={tabsArray} />
+        <CardCloseButton
+          linkTo={`/inventories/inventory/${inventory.id}/group`}
+        />
+      </CardHeader>
+    );
   }
+
   return (
     <>
       {cardHeader}

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroup/InventoryGroup.test.jsx

@@ -45,26 +45,20 @@ describe('<InventoryGroup />', () => {
         }
       );
     });
+    await waitForElement(wrapper, 'ContentLoading', el => el.length === 0);
   });
   afterEach(() => {
     wrapper.unmount();
   });
   test('renders successfully', async () => {
-    await act(async () => {
-      waitForElement(wrapper, 'ContentLoading', el => el.length === 0);
-    });
     expect(wrapper.length).toBe(1);
-    expect(wrapper.find('button[aria-label="Return to Groups"]').length).toBe(
-      1
-    );
   });
-  test('expect Return to Groups tab to exist', async () => {
-    await act(async () => {
-      waitForElement(wrapper, 'ContentLoading', el => el.length === 0);
-    });
-    expect(wrapper.length).toBe(1);
+  test('expect all tabs to exist, including Return to Groups', async () => {
     expect(wrapper.find('button[aria-label="Return to Groups"]').length).toBe(
       1
     );
+    expect(wrapper.find('button[aria-label="Details"]').length).toBe(1);
+    expect(wrapper.find('button[aria-label="Related Groups"]').length).toBe(1);
+    expect(wrapper.find('button[aria-label="Hosts"]').length).toBe(1);
   });
 });

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroupAdd/InventoryGroupAdd.jsx

@@ -7,8 +7,10 @@ import { Card } from '@patternfly/react-core';
 import InventoryGroupForm from '../InventoryGroupForm/InventoryGroupForm';
 
 function InventoryGroupsAdd({ history, inventory, setBreadcrumb }) {
-  useEffect(() => setBreadcrumb(inventory), [inventory, setBreadcrumb]);
   const [error, setError] = useState(null);
+
+  useEffect(() => setBreadcrumb(inventory), [inventory, setBreadcrumb]);
+
   const handleSubmit = async values => {
     values.inventory = inventory.id;
     try {
@@ -18,9 +20,11 @@ function InventoryGroupsAdd({ history, inventory, setBreadcrumb }) {
       setError(err);
     }
   };
+
   const handleCancel = () => {
     history.push(`/inventories/inventory/${inventory.id}/groups`);
   };
+
   return (
     <Card>
       <InventoryGroupForm

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroupAdd/InventoryGroupAdd.test.jsx

@@ -1,8 +1,10 @@
 import React from 'react';
+import { Route } from 'react-router-dom';
+
 import { GroupsAPI } from '@api';
 import { createMemoryHistory } from 'history';
 import { act } from 'react-dom/test-utils';
-import { mountWithContexts, waitForElement } from '@testUtils/enzymeHelpers';
+import { mountWithContexts } from '@testUtils/enzymeHelpers';
 
 import InventoryGroupAdd from './InventoryGroupAdd';
 
@@ -13,19 +15,19 @@ describe('<InventoryGroupAdd />', () => {
   let history;
   beforeEach(async () => {
     history = createMemoryHistory({
-      initialEntries: ['/inventories/inventory/1/groups'],
+      initialEntries: ['/inventories/inventory/1/groups/add'],
     });
     await act(async () => {
       wrapper = mountWithContexts(
-        <InventoryGroupAdd
-          setBreadcrumb={() => {}}
-          inventory={{ inventory: { id: 1 } }}
+        <Route
+          path="/inventories/inventory/:id/groups/add"
+          component={() => (
+            <InventoryGroupAdd setBreadcrumb={() => {}} inventory={{ id: 1 }} />
+          )}
         />,
         {
           context: {
-            router: {
-              history,
-            },
+            router: { history, route: { location: history.location } },
           },
         }
       );
@@ -38,17 +40,12 @@ describe('<InventoryGroupAdd />', () => {
     expect(wrapper.length).toBe(1);
   });
   test('cancel should navigate user to Inventory Groups List', async () => {
-    await act(async () => {
-      waitForElement(wrapper, 'isLoading', el => el.length === 0);
-    });
+    wrapper.find('button[aria-label="Cancel"]').simulate('click');
     expect(history.location.pathname).toEqual(
       '/inventories/inventory/1/groups'
     );
   });
   test('handleSubmit should call api', async () => {
-    await act(async () => {
-      waitForElement(wrapper, 'isLoading', el => el.length === 0);
-    });
     await act(async () => {
       wrapper.find('InventoryGroupForm').prop('handleSubmit')({
         name: 'Bar',

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroupDetail/InventoryGroupDetail.jsx

@@ -29,6 +29,9 @@ const ActionButtonWrapper = styled.div`
   }
 `;
 function InventoryGroupDetail({ i18n, history, match, inventoryGroup }) {
+  const {
+    summary_fields: { created_by, modified_by },
+  } = inventoryGroup;
   const [error, setError] = useState(false);
   const [isDeleteModalOpen, setIsDeleteModalOpen] = useState(false);
 
@@ -41,52 +44,7 @@ function InventoryGroupDetail({ i18n, history, match, inventoryGroup }) {
       setError(err);
     }
   };
-  if (error) {
-    return (
-      <AlertModal
-        variant="danger"
-        title={i18n._(t`Error!`)}
-        isOpen={error}
-        onClose={() => setError(false)}
-      >
-        {i18n._(t`Failed to delete group ${inventoryGroup.name}.`)}
-        <ErrorDetail error={error} />
-      </AlertModal>
-    );
-  }
-  if (isDeleteModalOpen) {
-    return (
-      <AlertModal
-        variant="danger"
-        title={i18n._(t`Delete Inventory Group`)}
-        isOpen={isDeleteModalOpen}
-        onClose={() => setIsDeleteModalOpen(false)}
-        actions={[
-          <Button
-            key="delete"
-            variant="danger"
-            aria-label={i18n._(t`confirm delete`)}
-            onClick={handleDelete}
-          >
-            {i18n._(t`Delete`)}
-          </Button>,
-          <Button
-            key="cancel"
-            variant="secondary"
-            aria-label={i18n._(t`cancel delete`)}
-            onClick={() => setIsDeleteModalOpen(false)}
-          >
-            {i18n._(t`Cancel`)}
-          </Button>,
-        ]}
-      >
-        {i18n._(t`Are you sure you want to delete:`)}
-        <br />
-        <strong>{inventoryGroup.name}</strong>
-        <br />
-      </AlertModal>
-    );
-  }
+
   return (
     <CardBody style={{ paddingTop: '20px' }}>
       <DetailList gutter="sm">
@@ -104,32 +62,32 @@ function InventoryGroupDetail({ i18n, history, match, inventoryGroup }) {
         label={i18n._(t`Variables`)}
       />
       <DetailList>
-        <Detail
-          label={i18n._(t`Created`)}
-          value={
-            <span>
-              {i18n._(t`${formatDateString(inventoryGroup.created)} by`)}{' '}
-              <Link
-                to={`/users/${inventoryGroup.summary_fields.created_by.id}`}
-              >
-                {inventoryGroup.summary_fields.created_by.username}
-              </Link>
-            </span>
-          }
-        />
-        <Detail
-          label={i18n._(t`Modified`)}
-          value={
-            <span>
-              {i18n._(t`${formatDateString(inventoryGroup.modified)} by`)}{' '}
-              <Link
-                to={`/users/${inventoryGroup.summary_fields.modified_by.id}`}
-              >
-                {inventoryGroup.summary_fields.modified_by.username}
-              </Link>
-            </span>
-          }
-        />
+        {created_by && created_by.username && (
+          <Detail
+            label={i18n._(t`Created`)}
+            value={
+              <span>
+                {i18n._(t`${formatDateString(inventoryGroup.created)} by`)}{' '}
+                <Link to={`/users/${created_by.id}`}>
+                  {created_by.username}
+                </Link>
+              </span>
+            }
+          />
+        )}
+        {modified_by && modified_by.username && (
+          <Detail
+            label={i18n._(t`Modified`)}
+            value={
+              <span>
+                {i18n._(t`${formatDateString(inventoryGroup.modified)} by`)}{' '}
+                <Link to={`/users/${modified_by.id}`}>
+                  {modified_by.username}
+                </Link>
+              </span>
+            }
+          />
+        )}
       </DetailList>
       <ActionButtonWrapper>
         <Button
@@ -151,6 +109,48 @@ function InventoryGroupDetail({ i18n, history, match, inventoryGroup }) {
           {i18n._(t`Delete`)}
         </Button>
       </ActionButtonWrapper>
+      {isDeleteModalOpen && (
+        <AlertModal
+          variant="danger"
+          title={i18n._(t`Delete Inventory Group`)}
+          isOpen={isDeleteModalOpen}
+          onClose={() => setIsDeleteModalOpen(false)}
+          actions={[
+            <Button
+              key="delete"
+              variant="danger"
+              aria-label={i18n._(t`confirm delete`)}
+              onClick={handleDelete}
+            >
+              {i18n._(t`Delete`)}
+            </Button>,
+            <Button
+              key="cancel"
+              variant="secondary"
+              aria-label={i18n._(t`cancel delete`)}
+              onClick={() => setIsDeleteModalOpen(false)}
+            >
+              {i18n._(t`Cancel`)}
+            </Button>,
+          ]}
+        >
+          {i18n._(t`Are you sure you want to delete:`)}
+          <br />
+          <strong>{inventoryGroup.name}</strong>
+          <br />
+        </AlertModal>
+      )}
+      {error && (
+        <AlertModal
+          variant="danger"
+          title={i18n._(t`Error!`)}
+          isOpen={error}
+          onClose={() => setError(false)}
+        >
+          {i18n._(t`Failed to delete group ${inventoryGroup.name}.`)}
+          <ErrorDetail error={error} />
+        </AlertModal>
+      )}
     </CardBody>
   );
 }

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroupDetail/InventoryGroupDetail.test.jsx

@@ -33,7 +33,7 @@ describe('<InventoryGroupDetail />', () => {
   beforeEach(async () => {
     await act(async () => {
       history = createMemoryHistory({
-        initialEntries: ['/inventories/inventory/1/groups/1/edit'],
+        initialEntries: ['/inventories/inventory/1/groups/1/details'],
       });
       wrapper = mountWithContexts(
         <Route
@@ -69,7 +69,7 @@ describe('<InventoryGroupDetail />', () => {
     expect(GroupsAPI.destroy).toBeCalledWith(1);
   });
   test('should navigate user to edit form on edit button click', async () => {
-    wrapper.find('button[aria-label="Edit"]').prop('onClick');
+    wrapper.find('button[aria-label="Edit"]').simulate('click');
     expect(history.location.pathname).toEqual(
       '/inventories/inventory/1/groups/1/edit'
     );

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroupEdit/InventoryGroupEdit.jsx

@@ -11,19 +11,20 @@ function InventoryGroupEdit({ history, inventoryGroup, inventory, match }) {
   const handleSubmit = async values => {
     try {
       await GroupsAPI.update(match.params.groupId, values);
+      history.push(
+        `/inventories/inventory/${inventory.id}/groups/${inventoryGroup.id}`
+      );
     } catch (err) {
       setError(err);
-    } finally {
-      history.push(
-        `/inventories/inventory/${inventory.id}/groups/${inventoryGroup.id}/details`
-      );
     }
   };
+
   const handleCancel = () => {
     history.push(
-      `/inventories/inventory/${inventory.id}/groups/${inventoryGroup.id}/details`
+      `/inventories/inventory/${inventory.id}/groups/${inventoryGroup.id}`
     );
   };
+
   return (
     <InventoryGroupForm
       error={error}

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroupEdit/InventroyGroupEdit.test.jsx

@@ -1,8 +1,9 @@
 import React from 'react';
+import { Route } from 'react-router-dom';
 import { GroupsAPI } from '@api';
 import { createMemoryHistory } from 'history';
 import { act } from 'react-dom/test-utils';
-import { mountWithContexts, waitForElement } from '@testUtils/enzymeHelpers';
+import { mountWithContexts } from '@testUtils/enzymeHelpers';
 
 import InventoryGroupEdit from './InventoryGroupEdit';
 
@@ -19,13 +20,19 @@ describe('<InventoryGroupEdit />', () => {
   let history;
   beforeEach(async () => {
     history = createMemoryHistory({
-      initialEntries: ['/inventories/1/groups'],
+      initialEntries: ['/inventories/inventory/1/groups/2/edit'],
     });
     await act(async () => {
       wrapper = mountWithContexts(
-        <InventoryGroupEdit
-          setBreadcrumb={jest.fn()}
-          inventory={{ inventory: { id: 1 } }}
+        <Route
+          path="/inventories/inventory/:id/groups/:groupId/edit"
+          component={() => (
+            <InventoryGroupEdit
+              setBreadcrumb={() => {}}
+              inventory={{ id: 1 }}
+              inventoryGroup={{ id: 2 }}
+            />
+          )}
         />,
         {
           context: {
@@ -35,6 +42,7 @@ describe('<InventoryGroupEdit />', () => {
                 match: {
                   params: { groupId: 13 },
                 },
+                location: history.location,
               },
             },
           },
@@ -49,11 +57,12 @@ describe('<InventoryGroupEdit />', () => {
     expect(wrapper.length).toBe(1);
   });
   test('cancel should navigate user to Inventory Groups List', async () => {
-    await waitForElement(wrapper, 'isLoading', el => el.length === 0);
-    expect(history.location.pathname).toEqual('/inventories/1/groups');
+    wrapper.find('button[aria-label="Cancel"]').simulate('click');
+    expect(history.location.pathname).toEqual(
+      '/inventories/inventory/1/groups/2'
+    );
   });
   test('handleSubmit should call api', async () => {
-    await waitForElement(wrapper, 'isLoading', el => el.length === 0);
     wrapper.find('InventoryGroupForm').prop('handleSubmit')({
       name: 'Bar',
       description: 'Ansible',

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroupForm/InventoryGroupForm.test.jsx

@@ -26,7 +26,6 @@ describe('<InventoryGroupForm />', () => {
     expect(wrapper.length).toBe(1);
   });
   test('should render values for the fields that have them', () => {
-    expect(wrapper.length).toBe(1);
     expect(wrapper.find("FormGroup[label='Name']").length).toBe(1);
     expect(wrapper.find("FormGroup[label='Description']").length).toBe(1);
     expect(wrapper.find("VariablesField[label='Variables']").length).toBe(1);

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroups.jsx

@@ -11,37 +11,32 @@ import InventoryGroupsList from './InventoryGroupsList';
 function InventoryGroups({ setBreadcrumb, inventory, location, match }) {
   return (
     <Switch>
-      {[
-        <Route
-          key="list"
-          path="/inventories/inventory/:id/groups"
-          render={() => {
-            return <InventoryGroupsList location={location} match={match} />;
-          }}
-        />,
-        <Route
-          key="add"
-          path="/inventories/inventory/:id/groups/add"
-          render={() => {
-            return (
-              <InventoryGroupAdd
-                setBreadcrumb={setBreadcrumb}
-                inventory={inventory}
-              />
-            );
-          }}
-        />,
-        <Route
-          key="details"
-          path="/inventories/inventory/:id/groups/:groupId/"
-          render={() => (
-            <InventoryGroup
-              inventory={inventory}
+      <Route
+        key="add"
+        path="/inventories/inventory/:id/groups/add"
+        render={() => {
+          return (
+            <InventoryGroupAdd
               setBreadcrumb={setBreadcrumb}
+              inventory={inventory}
             />
-          )}
-        />,
-      ]}
+          );
+        }}
+      />
+      <Route
+        key="details"
+        path="/inventories/inventory/:id/groups/:groupId/"
+        render={() => (
+          <InventoryGroup inventory={inventory} setBreadcrumb={setBreadcrumb} />
+        )}
+      />
+      <Route
+        key="list"
+        path="/inventories/inventory/:id/groups"
+        render={() => {
+          return <InventoryGroupsList location={location} match={match} />;
+        }}
+      />
     </Switch>
   );
 }

awx/ui_next/src/screens/Inventory/InventoryGroups/InventoryGroups.test.jsx

@@ -4,7 +4,7 @@ import { Route } from 'react-router-dom';
 import { createMemoryHistory } from 'history';
 import { mountWithContexts, waitForElement } from '@testUtils/enzymeHelpers';
 import { InventoriesAPI, GroupsAPI } from '@api';
-import InventoryGroups from './InventoryGroups';
+import InventoryGroupsList from './InventoryGroupsList';
 
 jest.mock('@api');
 
@@ -50,7 +50,7 @@ const mockGroups = [
   },
 ];
 
-describe('<InventoryGroups />', () => {
+describe('<InventoryGroupsList />', () => {
   let wrapper;
 
   beforeEach(async () => {
@@ -75,7 +75,7 @@ describe('<InventoryGroups />', () => {
       wrapper = mountWithContexts(
         <Route
           path="/inventories/inventory/:id/groups"
-          component={() => <InventoryGroups />}
+          component={() => <InventoryGroupsList />}
         />,
         {
           context: {
@@ -88,7 +88,7 @@ describe('<InventoryGroups />', () => {
   });
 
   test('initially renders successfully', () => {
-    expect(wrapper.find('InventoryGroups').length).toBe(1);
+    expect(wrapper.find('InventoryGroupsList').length).toBe(1);
   });
 
   test('should fetch groups from api and render them in the list', async () => {
@@ -147,7 +147,7 @@ describe('<InventoryGroups />', () => {
       Promise.reject(new Error())
     );
     await act(async () => {
-      wrapper = mountWithContexts(<InventoryGroups />);
+      wrapper = mountWithContexts(<InventoryGroupsList />);
     });
     await waitForElement(wrapper, 'ContentError', el => el.length === 1);
   });