From d15f7b76fa994a02d8ddfb1e27de6924825d5499 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Mon, 27 Jan 2020 10:20:47 -0800 Subject: [PATCH 1/6] add useEndpoint hook --- .../InventoryDetail/InventoryDetail.jsx | 42 ++++++------------- .../Inventory/InventoryDetail/useEndpoint.js | 40 ++++++++++++++++++ 2 files changed, 52 insertions(+), 30 deletions(-) create mode 100644 awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js diff --git a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx index 3788dc130f..a9d558624c 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 } from 'react'; import { Link, useHistory } from 'react-router-dom'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; @@ -11,37 +11,19 @@ import DeleteButton from '@components/DeleteButton'; import ContentError from '@components/ContentError'; import ContentLoading from '@components/ContentLoading'; import { InventoriesAPI } from '@api'; +import useEndpoint from './useEndpoint'; 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); - 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]); + const { results: instanceGroups, isLoading, error } = useEndpoint( + useCallback(async () => { + const { data } = await InventoriesAPI.readInstanceGroups(inventory.id); + return data.results; + }, [inventory.id]), + inventory.id + ); const deleteInventory = async () => { await InventoriesAPI.destroy(inventory.id); @@ -53,12 +35,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/Inventory/InventoryDetail/useEndpoint.js b/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js new file mode 100644 index 0000000000..9c45d7e32e --- /dev/null +++ b/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js @@ -0,0 +1,40 @@ +import { useEffect, useState, useRef } from 'react'; + +export default function useEndpoint(fetch) { + const [results, setResults] = useState([]); + const [error, setError] = useState(null); + const [isLoading, setIsLoading] = useState(true); + const isMounted = useRef(null); + + useEffect(() => { + isMounted.current = true; + (async () => { + // Do we want this set here or not? Can result in extra + // unmounting/re-mounting of child components + setIsLoading(true); + try { + const fetchedResults = await fetch(); + if (isMounted.current) { + setResults(fetchedResults); + } + } catch (err) { + if (isMounted.current) { + setError(err); + } + } finally { + if (isMounted.current) { + setIsLoading(false); + } + } + })(); + return () => { + isMounted.current = false; + }; + }, [fetch]); + + return { + results, + isLoading, + error, + }; +} From aaf371ee23775bd68e9ca02ed2ad3c6e393f6ad8 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Wed, 29 Jan 2020 09:14:32 -0800 Subject: [PATCH 2/6] add useFetch demo --- .../src/screens/Inventory/Inventory.jsx | 3 +- .../InventoryDetail/InventoryDetail.jsx | 26 +++- .../Inventory/InventoryDetail/useEndpoint.js | 41 +++++- .../OrganizationList/OrganizationList.jsx | 135 ++++++++++-------- 4 files changed, 142 insertions(+), 63 deletions(-) diff --git a/awx/ui_next/src/screens/Inventory/Inventory.jsx b/awx/ui_next/src/screens/Inventory/Inventory.jsx index 3a445e0cf6..5a3d26179f 100644 --- a/awx/ui_next/src/screens/Inventory/Inventory.jsx +++ b/awx/ui_next/src/screens/Inventory/Inventory.jsx @@ -37,7 +37,8 @@ function Inventory({ i18n, setBreadcrumb }) { useEffect(() => { async function fetchData() { try { - setHasContentLoading(true); + // TODO: delete next line + // 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 a9d558624c..d7cca7727a 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, { useCallback } 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,20 +11,38 @@ import DeleteButton from '@components/DeleteButton'; import ContentError from '@components/ContentError'; import ContentLoading from '@components/ContentLoading'; import { InventoriesAPI } from '@api'; -import useEndpoint from './useEndpoint'; +import useEndpoint, { useFetch } from './useEndpoint'; import { Inventory } from '../../../types'; function InventoryDetail({ inventory, i18n }) { const history = useHistory(); - const { results: instanceGroups, isLoading, error } = useEndpoint( + // initial approach with useEndpoint() + // const { results: instanceGroups, isLoading, error } = useEndpoint( + // useCallback(async () => { + // const { data } = await InventoriesAPI.readInstanceGroups(inventory.id); + // return data.results; + // }, [inventory.id]) + // ); + + // more versatile approach with useFetch() + const { + result: instanceGroups, + isLoading, + error, + fetch: fetchInstanceGroups, + } = useFetch( useCallback(async () => { const { data } = await InventoriesAPI.readInstanceGroups(inventory.id); return data.results; }, [inventory.id]), - inventory.id + [] ); + useEffect(() => { + fetchInstanceGroups(inventory.id); + }, [fetchInstanceGroups, inventory.id]); + const deleteInventory = async () => { await InventoriesAPI.destroy(inventory.id); history.push(`/inventories`); diff --git a/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js b/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js index 9c45d7e32e..4ace382a1a 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js +++ b/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js @@ -1,5 +1,6 @@ -import { useEffect, useState, useRef } from 'react'; +import { useEffect, useState, useRef, useCallback } from 'react'; +// Initial approach (useEffect baked in) export default function useEndpoint(fetch) { const [results, setResults] = useState([]); const [error, setError] = useState(null); @@ -38,3 +39,41 @@ export default function useEndpoint(fetch) { error, }; } + +// more versatile approach (returns function to make API request) +export function useFetch(fetch, 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, + fetch: useCallback(async () => { + setIsLoading(true); + try { + const response = await fetch(); + if (isMounted.current) { + setResult(response); + } + } catch (err) { + if (isMounted.current) { + setError(err); + } + } finally { + if (isMounted.current) { + setIsLoading(false); + } + } + }, [fetch]), + }; +} diff --git a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx index 827b7dd092..6abe157d03 100644 --- a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx +++ b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx @@ -1,4 +1,4 @@ -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'; @@ -13,6 +13,10 @@ import PaginatedDataList, { ToolbarDeleteButton, } from '@components/PaginatedDataList'; import { getQSConfig, parseQueryString } from '@util/qs'; +// TODO: move useEndpoint to better location +import useEndpoint, { + useFetch, +} from '../../Inventory/InventoryDetail/useEndpoint'; import OrganizationListItem from './OrganizationListItem'; @@ -25,64 +29,89 @@ 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 { + // results: organizations, + // error: contentError, + // isLoading: isOrgsLoading, + // } = useEndpoint( + // useCallback(async () => { + // const params = parseQueryString(QS_CONFIG, location.search); + // const [ + // { + // data: { count, results }, + // }, + // { + // data: { actions }, + // }, + // ] = await Promise.all([ + // OrganizationsAPI.read(params), + // loadOrganizationActions(), + // ]); + // return { + // organizations: results, + // count, + // actions, + // }; + // }, [location]) + // ); + + const { + result: { organizations = {}, organizationCount, actions }, + error: contentError, + isLoading: isOrgsLoading, + fetch: fetchOrganizations, + } = useFetch( + 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 } }); - } - return OrganizationsAPI.readOptions(); - }; + const { + isLoading: isDeleteLoading, + // error: deletionError, + fetch: deleteOrganizations, + } = useFetch( + useCallback(async () => { + return Promise.all( + selected.map(({ id }) => OrganizationsAPI.destroy(id)) + ); + }, [selected]) + ); + + 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 +128,6 @@ function OrganizationsList({ i18n }) { } }; - const handleDeleteErrorClose = () => { - setDeletionError(null); - }; - - useEffect(() => { - loadOrganizations(location); - }, [location]); // eslint-disable-line react-hooks/exhaustive-deps - return ( <> @@ -115,7 +136,7 @@ function OrganizationsList({ i18n }) { contentError={contentError} hasContentLoading={hasContentLoading} items={organizations} - itemCount={itemCount} + itemCount={organizationCount} pluralizedItemName="Organizations" qsConfig={QS_CONFIG} onRowClick={handleSelect} @@ -179,7 +200,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.`)} From ef2fa26126dab170e6ee77b8e41f787c684fd125 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Thu, 30 Jan 2020 16:13:19 -0800 Subject: [PATCH 3/6] rename useFetch to useRequest --- awx/ui_next/src/api/index.js | 2 + awx/ui_next/src/api/useRequest.js | 38 +++++++++ .../InventoryDetail/InventoryDetail.jsx | 16 +--- .../Inventory/InventoryDetail/useEndpoint.js | 79 ------------------- .../OrganizationList/OrganizationList.jsx | 40 ++-------- 5 files changed, 48 insertions(+), 127 deletions(-) create mode 100644 awx/ui_next/src/api/useRequest.js delete mode 100644 awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js diff --git a/awx/ui_next/src/api/index.js b/awx/ui_next/src/api/index.js index f9acea4211..2fd939ea69 100644 --- a/awx/ui_next/src/api/index.js +++ b/awx/ui_next/src/api/index.js @@ -24,6 +24,7 @@ import UnifiedJobs from './models/UnifiedJobs'; import Users from './models/Users'; import WorkflowJobs from './models/WorkflowJobs'; import WorkflowJobTemplates from './models/WorkflowJobTemplates'; +import useRequest from './useRequest'; const AdHocCommandsAPI = new AdHocCommands(); const ConfigAPI = new Config(); @@ -79,4 +80,5 @@ export { UsersAPI, WorkflowJobsAPI, WorkflowJobTemplatesAPI, + useRequest, }; diff --git a/awx/ui_next/src/api/useRequest.js b/awx/ui_next/src/api/useRequest.js new file mode 100644 index 0000000000..909dfda683 --- /dev/null +++ b/awx/ui_next/src/api/useRequest.js @@ -0,0 +1,38 @@ +import { useEffect, useState, useRef, useCallback } from 'react'; + +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/screens/Inventory/InventoryDetail/InventoryDetail.jsx b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx index d7cca7727a..c48057f648 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx +++ b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx @@ -10,28 +10,18 @@ import { VariablesDetail } from '@components/CodeMirrorInput'; import DeleteButton from '@components/DeleteButton'; import ContentError from '@components/ContentError'; import ContentLoading from '@components/ContentLoading'; -import { InventoriesAPI } from '@api'; -import useEndpoint, { useFetch } from './useEndpoint'; +import { InventoriesAPI, useRequest } from '@api'; import { Inventory } from '../../../types'; function InventoryDetail({ inventory, i18n }) { const history = useHistory(); - // initial approach with useEndpoint() - // const { results: instanceGroups, isLoading, error } = useEndpoint( - // useCallback(async () => { - // const { data } = await InventoriesAPI.readInstanceGroups(inventory.id); - // return data.results; - // }, [inventory.id]) - // ); - - // more versatile approach with useFetch() const { result: instanceGroups, isLoading, error, - fetch: fetchInstanceGroups, - } = useFetch( + request: fetchInstanceGroups, + } = useRequest( useCallback(async () => { const { data } = await InventoriesAPI.readInstanceGroups(inventory.id); return data.results; diff --git a/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js b/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js deleted file mode 100644 index 4ace382a1a..0000000000 --- a/awx/ui_next/src/screens/Inventory/InventoryDetail/useEndpoint.js +++ /dev/null @@ -1,79 +0,0 @@ -import { useEffect, useState, useRef, useCallback } from 'react'; - -// Initial approach (useEffect baked in) -export default function useEndpoint(fetch) { - const [results, setResults] = useState([]); - const [error, setError] = useState(null); - const [isLoading, setIsLoading] = useState(true); - const isMounted = useRef(null); - - useEffect(() => { - isMounted.current = true; - (async () => { - // Do we want this set here or not? Can result in extra - // unmounting/re-mounting of child components - setIsLoading(true); - try { - const fetchedResults = await fetch(); - if (isMounted.current) { - setResults(fetchedResults); - } - } catch (err) { - if (isMounted.current) { - setError(err); - } - } finally { - if (isMounted.current) { - setIsLoading(false); - } - } - })(); - return () => { - isMounted.current = false; - }; - }, [fetch]); - - return { - results, - isLoading, - error, - }; -} - -// more versatile approach (returns function to make API request) -export function useFetch(fetch, 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, - fetch: useCallback(async () => { - setIsLoading(true); - try { - const response = await fetch(); - if (isMounted.current) { - setResult(response); - } - } catch (err) { - if (isMounted.current) { - setError(err); - } - } finally { - if (isMounted.current) { - setIsLoading(false); - } - } - }, [fetch]), - }; -} diff --git a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx index 6abe157d03..3f571d9a55 100644 --- a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx +++ b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx @@ -4,7 +4,7 @@ import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import { Card, PageSection } from '@patternfly/react-core'; -import { OrganizationsAPI } from '@api'; +import { OrganizationsAPI, useRequest } from '@api'; import AlertModal from '@components/AlertModal'; import DataListToolbar from '@components/DataListToolbar'; import ErrorDetail from '@components/ErrorDetail'; @@ -13,10 +13,6 @@ import PaginatedDataList, { ToolbarDeleteButton, } from '@components/PaginatedDataList'; import { getQSConfig, parseQueryString } from '@util/qs'; -// TODO: move useEndpoint to better location -import useEndpoint, { - useFetch, -} from '../../Inventory/InventoryDetail/useEndpoint'; import OrganizationListItem from './OrganizationListItem'; @@ -35,38 +31,12 @@ function OrganizationsList({ i18n }) { const addUrl = `${match.url}/add`; - // const { - // results: organizations, - // error: contentError, - // isLoading: isOrgsLoading, - // } = useEndpoint( - // useCallback(async () => { - // const params = parseQueryString(QS_CONFIG, location.search); - // const [ - // { - // data: { count, results }, - // }, - // { - // data: { actions }, - // }, - // ] = await Promise.all([ - // OrganizationsAPI.read(params), - // loadOrganizationActions(), - // ]); - // return { - // organizations: results, - // count, - // actions, - // }; - // }, [location]) - // ); - const { result: { organizations = {}, organizationCount, actions }, error: contentError, isLoading: isOrgsLoading, - fetch: fetchOrganizations, - } = useFetch( + request: fetchOrganizations, + } = useRequest( useCallback(async () => { const params = parseQueryString(QS_CONFIG, location.search); const [orgs, orgActions] = await Promise.all([ @@ -89,8 +59,8 @@ function OrganizationsList({ i18n }) { const { isLoading: isDeleteLoading, // error: deletionError, - fetch: deleteOrganizations, - } = useFetch( + request: deleteOrganizations, + } = useRequest( useCallback(async () => { return Promise.all( selected.map(({ id }) => OrganizationsAPI.destroy(id)) From 370a7f9b250f03ac028bccfe6800ab1a51485a72 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Fri, 31 Jan 2020 11:39:23 -0800 Subject: [PATCH 4/6] move useRequest to util folder, add tests --- awx/ui_next/src/api/index.js | 2 - .../InventoryDetail/InventoryDetail.jsx | 3 +- .../OrganizationList/OrganizationList.jsx | 11 +- awx/ui_next/src/{api => util}/useRequest.js | 0 awx/ui_next/src/util/useRequest.test.jsx | 109 ++++++++++++++++++ 5 files changed, 120 insertions(+), 5 deletions(-) rename awx/ui_next/src/{api => util}/useRequest.js (100%) create mode 100644 awx/ui_next/src/util/useRequest.test.jsx diff --git a/awx/ui_next/src/api/index.js b/awx/ui_next/src/api/index.js index 2fd939ea69..f9acea4211 100644 --- a/awx/ui_next/src/api/index.js +++ b/awx/ui_next/src/api/index.js @@ -24,7 +24,6 @@ import UnifiedJobs from './models/UnifiedJobs'; import Users from './models/Users'; import WorkflowJobs from './models/WorkflowJobs'; import WorkflowJobTemplates from './models/WorkflowJobTemplates'; -import useRequest from './useRequest'; const AdHocCommandsAPI = new AdHocCommands(); const ConfigAPI = new Config(); @@ -80,5 +79,4 @@ export { UsersAPI, WorkflowJobsAPI, WorkflowJobTemplatesAPI, - useRequest, }; diff --git a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx index c48057f648..8d3f2eb8fb 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx +++ b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx @@ -10,7 +10,8 @@ import { VariablesDetail } from '@components/CodeMirrorInput'; import DeleteButton from '@components/DeleteButton'; import ContentError from '@components/ContentError'; import ContentLoading from '@components/ContentLoading'; -import { InventoriesAPI, useRequest } from '@api'; +import { InventoriesAPI } from '@api'; +import useRequest from '@util/useRequest'; import { Inventory } from '../../../types'; function InventoryDetail({ inventory, i18n }) { diff --git a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx index 3f571d9a55..f2b37daada 100644 --- a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx +++ b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx @@ -4,7 +4,8 @@ import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import { Card, PageSection } from '@patternfly/react-core'; -import { OrganizationsAPI, useRequest } from '@api'; +import { OrganizationsAPI } from '@api'; +import useRequest from '@util/useRequest'; import AlertModal from '@components/AlertModal'; import DataListToolbar from '@components/DataListToolbar'; import ErrorDetail from '@components/ErrorDetail'; @@ -58,7 +59,7 @@ function OrganizationsList({ i18n }) { const { isLoading: isDeleteLoading, - // error: deletionError, + error: dError, request: deleteOrganizations, } = useRequest( useCallback(async () => { @@ -68,6 +69,12 @@ function OrganizationsList({ i18n }) { }, [selected]) ); + useEffect(() => { + if (dError) { + setDeletionError(dError); + } + }, [dError]); + useEffect(() => { fetchOrganizations(); }, [fetchOrganizations]); diff --git a/awx/ui_next/src/api/useRequest.js b/awx/ui_next/src/util/useRequest.js similarity index 100% rename from awx/ui_next/src/api/useRequest.js rename to awx/ui_next/src/util/useRequest.js 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' }); + }); + }); +}); From 638e8c7add2597a30993aee6e11289434c94ce4b Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Mon, 3 Feb 2020 09:43:06 -0800 Subject: [PATCH 5/6] delete dead code/comments & add useRequest docstring --- awx/ui_next/src/screens/Inventory/Inventory.jsx | 2 -- .../Inventory/InventoryDetail/InventoryDetail.jsx | 4 ++-- awx/ui_next/src/util/useRequest.js | 11 +++++++++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/awx/ui_next/src/screens/Inventory/Inventory.jsx b/awx/ui_next/src/screens/Inventory/Inventory.jsx index 5a3d26179f..6e8f55c60b 100644 --- a/awx/ui_next/src/screens/Inventory/Inventory.jsx +++ b/awx/ui_next/src/screens/Inventory/Inventory.jsx @@ -37,8 +37,6 @@ function Inventory({ i18n, setBreadcrumb }) { useEffect(() => { async function fetchData() { try { - // TODO: delete next line - // 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 8d3f2eb8fb..628d5d71e7 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx +++ b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx @@ -31,8 +31,8 @@ function InventoryDetail({ inventory, i18n }) { ); useEffect(() => { - fetchInstanceGroups(inventory.id); - }, [fetchInstanceGroups, inventory.id]); + fetchInstanceGroups(); + }, [fetchInstanceGroups]); const deleteInventory = async () => { await InventoriesAPI.destroy(inventory.id); diff --git a/awx/ui_next/src/util/useRequest.js b/awx/ui_next/src/util/useRequest.js index 909dfda683..f6e9426dc8 100644 --- a/awx/ui_next/src/util/useRequest.js +++ b/awx/ui_next/src/util/useRequest.js @@ -1,5 +1,16 @@ 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); From eceeeea22d10ed503fc51b237796e9e85072fe6c Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Mon, 3 Feb 2020 12:55:53 -0800 Subject: [PATCH 6/6] remove unneeded default value --- .../screens/Organization/OrganizationList/OrganizationList.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx index f2b37daada..3051861a05 100644 --- a/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx +++ b/awx/ui_next/src/screens/Organization/OrganizationList/OrganizationList.jsx @@ -33,7 +33,7 @@ function OrganizationsList({ i18n }) { const addUrl = `${match.url}/add`; const { - result: { organizations = {}, organizationCount, actions }, + result: { organizations, organizationCount, actions }, error: contentError, isLoading: isOrgsLoading, request: fetchOrganizations,