Merge pull request #5810 from keithjgrant/use-endpoint

Add useRequest hook

Reviewed-by: https://github.com/apps/softwarefactory-project-zuul

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

@@ -37,7 +37,6 @@ function Inventory({ i18n, setBreadcrumb }) {
   useEffect(() => {
     async function fetchData() {
       try {
-        setHasContentLoading(true);
         const { data } = await InventoriesAPI.readDetail(match.params.id);
         setBreadcrumb(data);
         setInventory(data);

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

@@ -1,4 +1,4 @@
-import React, { useState, useEffect, useRef } from 'react';
+import React, { useCallback, useEffect } from 'react';
 import { Link, useHistory } from 'react-router-dom';
 import { withI18n } from '@lingui/react';
 import { t } from '@lingui/macro';
@@ -11,37 +11,28 @@ import DeleteButton from '@components/DeleteButton';
 import ContentError from '@components/ContentError';
 import ContentLoading from '@components/ContentLoading';
 import { InventoriesAPI } from '@api';
+import useRequest from '@util/useRequest';
 import { Inventory } from '../../../types';
 
 function InventoryDetail({ inventory, i18n }) {
-  const [instanceGroups, setInstanceGroups] = useState([]);
-  const [hasContentLoading, setHasContentLoading] = useState(true);
-  const [contentError, setContentError] = useState(null);
   const history = useHistory();
-  const isMounted = useRef(null);
+
+  const {
+    result: instanceGroups,
+    isLoading,
+    error,
+    request: fetchInstanceGroups,
+  } = useRequest(
+    useCallback(async () => {
+      const { data } = await InventoriesAPI.readInstanceGroups(inventory.id);
+      return data.results;
+    }, [inventory.id]),
+    []
+  );
 
   useEffect(() => {
-    isMounted.current = true;
-    (async () => {
-      setHasContentLoading(true);
-      try {
-        const { data } = await InventoriesAPI.readInstanceGroups(inventory.id);
-        if (!isMounted.current) {
-          return;
-        }
-        setInstanceGroups(data.results);
-      } catch (err) {
-        setContentError(err);
-      } finally {
-        if (isMounted.current) {
-          setHasContentLoading(false);
-        }
-      }
-    })();
-    return () => {
-      isMounted.current = false;
-    };
-  }, [inventory.id]);
+    fetchInstanceGroups();
+  }, [fetchInstanceGroups]);
 
   const deleteInventory = async () => {
     await InventoriesAPI.destroy(inventory.id);
@@ -53,12 +44,12 @@ function InventoryDetail({ inventory, i18n }) {
     user_capabilities: userCapabilities,
   } = inventory.summary_fields;
 
-  if (hasContentLoading) {
+  if (isLoading) {
     return <ContentLoading />;
   }
 
-  if (contentError) {
-    return <ContentError error={contentError} />;
+  if (error) {
+    return <ContentError error={error} />;
   }
 
   return (

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

@@ -1,10 +1,11 @@
-import React, { useEffect, useState } from 'react';
+import React, { useState, useEffect, useCallback } from 'react';
 import { useLocation, useRouteMatch } from 'react-router-dom';
 import { withI18n } from '@lingui/react';
 import { t } from '@lingui/macro';
 import { Card, PageSection } from '@patternfly/react-core';
 
 import { OrganizationsAPI } from '@api';
+import useRequest from '@util/useRequest';
 import AlertModal from '@components/AlertModal';
 import DataListToolbar from '@components/DataListToolbar';
 import ErrorDetail from '@components/ErrorDetail';
@@ -25,64 +26,69 @@ const QS_CONFIG = getQSConfig('organization', {
 function OrganizationsList({ i18n }) {
   const location = useLocation();
   const match = useRouteMatch();
-  const [contentError, setContentError] = useState(null);
-  const [deletionError, setDeletionError] = useState(null);
-  const [hasContentLoading, setHasContentLoading] = useState(true);
-  const [itemCount, setItemCount] = useState(0);
-  const [organizations, setOrganizations] = useState([]);
-  const [orgActions, setOrgActions] = useState(null);
+
   const [selected, setSelected] = useState([]);
+  const [deletionError, setDeletionError] = useState(null);
 
   const addUrl = `${match.url}/add`;
-  const canAdd = orgActions && orgActions.POST;
-  const isAllSelected =
-    selected.length === organizations.length && selected.length > 0;
 
-  const loadOrganizations = async ({ search }) => {
-    const params = parseQueryString(QS_CONFIG, search);
-    setContentError(null);
-    setHasContentLoading(true);
-    try {
-      const [
-        {
-          data: { count, results },
-        },
-        {
-          data: { actions },
-        },
-      ] = await Promise.all([
+  const {
+    result: { organizations, organizationCount, actions },
+    error: contentError,
+    isLoading: isOrgsLoading,
+    request: fetchOrganizations,
+  } = useRequest(
+    useCallback(async () => {
+      const params = parseQueryString(QS_CONFIG, location.search);
+      const [orgs, orgActions] = await Promise.all([
         OrganizationsAPI.read(params),
-        loadOrganizationActions(),
+        OrganizationsAPI.readOptions(),
       ]);
-      setItemCount(count);
-      setOrganizations(results);
-      setOrgActions(actions);
-      setSelected([]);
-    } catch (error) {
-      setContentError(error);
-    } finally {
-      setHasContentLoading(false);
+      return {
+        organizations: orgs.data.results,
+        organizationCount: orgs.data.count,
+        actions: orgActions.data.actions,
+      };
+    }, [location]),
+    {
+      organizations: [],
+      organizationCount: 0,
+      actions: {},
     }
-  };
+  );
 
-  const loadOrganizationActions = () => {
-    if (orgActions) {
-      return Promise.resolve({ data: { actions: orgActions } });
+  const {
+    isLoading: isDeleteLoading,
+    error: dError,
+    request: deleteOrganizations,
+  } = useRequest(
+    useCallback(async () => {
+      return Promise.all(
+        selected.map(({ id }) => OrganizationsAPI.destroy(id))
+      );
+    }, [selected])
+  );
+
+  useEffect(() => {
+    if (dError) {
+      setDeletionError(dError);
     }
-    return OrganizationsAPI.readOptions();
-  };
+  }, [dError]);
+
+  useEffect(() => {
+    fetchOrganizations();
+  }, [fetchOrganizations]);
 
   const handleOrgDelete = async () => {
-    setHasContentLoading(true);
-    try {
-      await Promise.all(selected.map(({ id }) => OrganizationsAPI.destroy(id)));
-    } catch (error) {
-      setDeletionError(error);
-    } finally {
-      await loadOrganizations(location);
-    }
+    await deleteOrganizations();
+    await fetchOrganizations();
   };
 
+  const hasContentLoading = isDeleteLoading || isOrgsLoading;
+  const canAdd = actions && actions.POST;
+  const isAllSelected =
+    selected.length === organizations.length && selected.length > 0;
+
   const handleSelectAll = isSelected => {
     if (isSelected) {
       setSelected(organizations);
@@ -99,14 +105,6 @@ function OrganizationsList({ i18n }) {
     }
   };
 
-  const handleDeleteErrorClose = () => {
-    setDeletionError(null);
-  };
-
-  useEffect(() => {
-    loadOrganizations(location);
-  }, [location]); // eslint-disable-line react-hooks/exhaustive-deps
-
   return (
     <>
       <PageSection>
@@ -115,7 +113,7 @@ function OrganizationsList({ i18n }) {
             contentError={contentError}
             hasContentLoading={hasContentLoading}
             items={organizations}
-            itemCount={itemCount}
+            itemCount={organizationCount}
             pluralizedItemName="Organizations"
             qsConfig={QS_CONFIG}
             onRowClick={handleSelect}
@@ -179,7 +177,7 @@ function OrganizationsList({ i18n }) {
         isOpen={deletionError}
         variant="danger"
         title={i18n._(t`Error!`)}
-        onClose={handleDeleteErrorClose}
+        onClose={() => setDeletionError(null)}
       >
         {i18n._(t`Failed to delete one or more organizations.`)}
         <ErrorDetail error={deletionError} />

awx/ui_next/src/util/useRequest.js

@@ -0,0 +1,49 @@
+import { useEffect, useState, useRef, useCallback } from 'react';
+
+/*
+ * The useRequest hook accepts a request function and returns an object with
+ * four values:
+ *   request: a function to call to invoke the request
+ *   result: the value returned from the request function (once invoked)
+ *   isLoading: boolean state indicating whether the request is in active/in flight
+ *   error: any caught error resulting from the request
+ *
+ * The hook also accepts an optional second parameter which is a default
+ * value to set as result before the first time the request is made.
+ */
+export default function useRequest(makeRequest, initialValue) {
+  const [result, setResult] = useState(initialValue);
+  const [error, setError] = useState(null);
+  const [isLoading, setIsLoading] = useState(false);
+  const isMounted = useRef(null);
+
+  useEffect(() => {
+    isMounted.current = true;
+    return () => {
+      isMounted.current = false;
+    };
+  }, []);
+
+  return {
+    result,
+    error,
+    isLoading,
+    request: useCallback(async () => {
+      setIsLoading(true);
+      try {
+        const response = await makeRequest();
+        if (isMounted.current) {
+          setResult(response);
+        }
+      } catch (err) {
+        if (isMounted.current) {
+          setError(err);
+        }
+      } finally {
+        if (isMounted.current) {
+          setIsLoading(false);
+        }
+      }
+    }, [makeRequest]),
+  };
+}

awx/ui_next/src/util/useRequest.test.jsx

@@ -0,0 +1,109 @@
+import React from 'react';
+import { act } from 'react-dom/test-utils';
+import { mount } from 'enzyme';
+import useRequest from './useRequest';
+
+function TestInner() {
+  return <div />;
+}
+function Test({ makeRequest, initialValue = {} }) {
+  const request = useRequest(makeRequest, initialValue);
+  return <TestInner {...request} />;
+}
+
+describe('useRequest', () => {
+  test('should return initial value as result', async () => {
+    const makeRequest = jest.fn();
+    makeRequest.mockResolvedValue({ data: 'foo' });
+    const wrapper = mount(
+      <Test
+        makeRequest={makeRequest}
+        initialValue={{
+          initial: true,
+        }}
+      />
+    );
+
+    expect(wrapper.find('TestInner').prop('result')).toEqual({ initial: true });
+  });
+
+  test('should return result', async () => {
+    const makeRequest = jest.fn();
+    makeRequest.mockResolvedValue({ data: 'foo' });
+    const wrapper = mount(<Test makeRequest={makeRequest} />);
+
+    await act(async () => {
+      wrapper.find('TestInner').invoke('request')();
+    });
+    wrapper.update();
+    expect(wrapper.find('TestInner').prop('result')).toEqual({ data: 'foo' });
+  });
+
+  test('should is isLoading flag', async () => {
+    const makeRequest = jest.fn();
+    let resolve;
+    const promise = new Promise(r => {
+      resolve = r;
+    });
+    makeRequest.mockReturnValue(promise);
+    const wrapper = mount(<Test makeRequest={makeRequest} />);
+
+    await act(async () => {
+      wrapper.find('TestInner').invoke('request')();
+    });
+    wrapper.update();
+    expect(wrapper.find('TestInner').prop('isLoading')).toEqual(true);
+    await act(async () => {
+      resolve({ data: 'foo' });
+    });
+    wrapper.update();
+    expect(wrapper.find('TestInner').prop('isLoading')).toEqual(false);
+    expect(wrapper.find('TestInner').prop('result')).toEqual({ data: 'foo' });
+  });
+
+  test('should invoke request function', async () => {
+    const makeRequest = jest.fn();
+    makeRequest.mockResolvedValue({ data: 'foo' });
+    const wrapper = mount(<Test makeRequest={makeRequest} />);
+
+    expect(makeRequest).not.toHaveBeenCalled();
+    await act(async () => {
+      wrapper.find('TestInner').invoke('request')();
+    });
+    wrapper.update();
+    expect(makeRequest).toHaveBeenCalledTimes(1);
+  });
+
+  test('should return error thrown from request function', async () => {
+    const error = new Error('error');
+    const makeRequest = () => {
+      throw error;
+    };
+    const wrapper = mount(<Test makeRequest={makeRequest} />);
+
+    await act(async () => {
+      wrapper.find('TestInner').invoke('request')();
+    });
+    wrapper.update();
+    expect(wrapper.find('TestInner').prop('error')).toEqual(error);
+  });
+
+  test('should not update state after unmount', async () => {
+    const makeRequest = jest.fn();
+    let resolve;
+    const promise = new Promise(r => {
+      resolve = r;
+    });
+    makeRequest.mockReturnValue(promise);
+    const wrapper = mount(<Test makeRequest={makeRequest} />);
+
+    expect(makeRequest).not.toHaveBeenCalled();
+    await act(async () => {
+      wrapper.find('TestInner').invoke('request')();
+    });
+    wrapper.unmount();
+    await act(async () => {
+      resolve({ data: 'foo' });
+    });
+  });
+});