diff --git a/awx/ui_next/src/screens/Inventory/Inventory.jsx b/awx/ui_next/src/screens/Inventory/Inventory.jsx index 3a445e0cf6..6e8f55c60b 100644 --- a/awx/ui_next/src/screens/Inventory/Inventory.jsx +++ b/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); diff --git a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx index 3788dc130f..628d5d71e7 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx +++ b/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 ; } - if (contentError) { - return ; + if (error) { + return ; } return ( diff --git a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx index 827b7dd092..3051861a05 100644 --- a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx +++ b/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 ( <> @@ -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.`)} diff --git a/awx/ui_next/src/util/useRequest.js b/awx/ui_next/src/util/useRequest.js new file mode 100644 index 0000000000..f6e9426dc8 --- /dev/null +++ b/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]), + }; +} diff --git a/awx/ui_next/src/util/useRequest.test.jsx b/awx/ui_next/src/util/useRequest.test.jsx new file mode 100644 index 0000000000..1cf7ed955b --- /dev/null +++ b/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
; +} +function Test({ makeRequest, initialValue = {} }) { + const request = useRequest(makeRequest, initialValue); + return ; +} + +describe('useRequest', () => { + test('should return initial value as result', async () => { + const makeRequest = jest.fn(); + makeRequest.mockResolvedValue({ data: 'foo' }); + const wrapper = mount( + + ); + + 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(); + + 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(); + + 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(); + + 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(); + + 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(); + + expect(makeRequest).not.toHaveBeenCalled(); + await act(async () => { + wrapper.find('TestInner').invoke('request')(); + }); + wrapper.unmount(); + await act(async () => { + resolve({ data: 'foo' }); + }); + }); +});