From f775ed3f771023a54000c1e9a55844eb13ede46f Mon Sep 17 00:00:00 2001 From: mabashian Date: Tue, 28 Jul 2020 16:22:51 -0400 Subject: [PATCH 1/3] Converts UserList to functional component --- .../src/screens/User/UserList/UserList.jsx | 353 ++++++++---------- .../screens/User/UserList/UserList.test.jsx | 215 +++++------ 2 files changed, 241 insertions(+), 327 deletions(-) diff --git a/awx/ui_next/src/screens/User/UserList/UserList.jsx b/awx/ui_next/src/screens/User/UserList/UserList.jsx index e64ff44280..88c489c025 100644 --- a/awx/ui_next/src/screens/User/UserList/UserList.jsx +++ b/awx/ui_next/src/screens/User/UserList/UserList.jsx @@ -1,9 +1,8 @@ -import React, { Component, Fragment } from 'react'; -import { withRouter } from 'react-router-dom'; +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 { UsersAPI } from '../../../api'; import AlertModal from '../../../components/AlertModal'; import DataListToolbar from '../../../components/DataListToolbar'; @@ -12,8 +11,8 @@ import PaginatedDataList, { ToolbarAddButton, ToolbarDeleteButton, } from '../../../components/PaginatedDataList'; +import useRequest, { useDeleteItems } from '../../../util/useRequest'; import { getQSConfig, parseQueryString } from '../../../util/qs'; - import UserListItem from './UserListItem'; const QS_CONFIG = getQSConfig('user', { @@ -22,222 +21,174 @@ const QS_CONFIG = getQSConfig('user', { order_by: 'username', }); -class UsersList extends Component { - constructor(props) { - super(props); +function UserList({ i18n }) { + const location = useLocation(); + const match = useRouteMatch(); + const [selected, setSelected] = useState([]); - this.state = { - hasContentLoading: true, - contentError: null, - deletionError: null, + const { + result: { users, itemCount, actions }, + error: contentError, + isLoading, + request: fetchUsers, + } = useRequest( + useCallback(async () => { + const params = parseQueryString(QS_CONFIG, location.search); + const [response, actionsResponse] = await Promise.all([ + UsersAPI.read(params), + UsersAPI.readOptions(), + ]); + return { + users: response.data.results, + itemCount: response.data.count, + actions: actionsResponse.data.actions, + }; + }, [location]), + { users: [], - selected: [], itemCount: 0, - actions: null, - }; - - this.handleSelectAll = this.handleSelectAll.bind(this); - this.handleSelect = this.handleSelect.bind(this); - this.handleUserDelete = this.handleUserDelete.bind(this); - this.handleDeleteErrorClose = this.handleDeleteErrorClose.bind(this); - this.loadUsers = this.loadUsers.bind(this); - } - - componentDidMount() { - this.loadUsers(); - } - - componentDidUpdate(prevProps) { - const { location } = this.props; - if (location !== prevProps.location) { - this.loadUsers(); + actions: {}, } - } + ); - handleSelectAll(isSelected) { - const { users } = this.state; + useEffect(() => { + fetchUsers(); + }, [fetchUsers]); - const selected = isSelected ? [...users] : []; - this.setState({ selected }); - } + const isAllSelected = selected.length === users.length && selected.length > 0; - handleSelect(row) { - const { selected } = this.state; + const { + isLoading: isDeleteLoading, + deleteItems: deleteUsers, + deletionError, + clearDeletionError, + } = useDeleteItems( + useCallback(async () => { + return Promise.all(selected.map(user => UsersAPI.destroy(user.id))); + }, [selected]), + { + qsConfig: QS_CONFIG, + allItemsSelected: isAllSelected, + fetchItems: fetchUsers, + } + ); + const handleUserDelete = async () => { + await deleteUsers(); + setSelected([]); + }; + + const hasContentLoading = isDeleteLoading || isLoading; + const canAdd = actions && actions.POST; + + const handleSelectAll = isSelected => { + setSelected(isSelected ? [...users] : []); + }; + + const handleSelect = row => { if (selected.some(s => s.id === row.id)) { - this.setState({ selected: selected.filter(s => s.id !== row.id) }); + setSelected(selected.filter(s => s.id !== row.id)); } else { - this.setState({ selected: selected.concat(row) }); + setSelected(selected.concat(row)); } - } + }; - handleDeleteErrorClose() { - this.setState({ deletionError: null }); - } - - async handleUserDelete() { - const { selected } = this.state; - - this.setState({ hasContentLoading: true }); - try { - await Promise.all(selected.map(org => UsersAPI.destroy(org.id))); - } catch (err) { - this.setState({ deletionError: err }); - } finally { - await this.loadUsers(); - } - } - - async loadUsers() { - const { location } = this.props; - const { actions: cachedActions } = this.state; - const params = parseQueryString(QS_CONFIG, location.search); - - let optionsPromise; - if (cachedActions) { - optionsPromise = Promise.resolve({ data: { actions: cachedActions } }); - } else { - optionsPromise = UsersAPI.readOptions(); - } - - const promises = Promise.all([UsersAPI.read(params), optionsPromise]); - - this.setState({ contentError: null, hasContentLoading: true }); - try { - const [ - { - data: { count, results }, - }, - { - data: { actions }, - }, - ] = await promises; - this.setState({ - actions, - itemCount: count, - users: results, - selected: [], - }); - } catch (err) { - this.setState({ contentError: err }); - } finally { - this.setState({ hasContentLoading: false }); - } - } - - render() { - const { - actions, - itemCount, - contentError, - hasContentLoading, - deletionError, - selected, - users, - } = this.state; - const { match, i18n } = this.props; - - const canAdd = - actions && Object.prototype.hasOwnProperty.call(actions, 'POST'); - const isAllSelected = - selected.length === users.length && selected.length > 0; - - return ( - - - - ( - , - ] - : []), - , - ]} - /> - )} - renderItem={o => ( - row.id === o.id)} - onSelect={() => this.handleSelect(o)} - /> - )} - emptyStateControls={ - canAdd ? ( - - ) : null - } - /> - - + return ( + <> + + + ( + , + ] + : []), + , + ]} + /> + )} + renderItem={o => ( + row.id === o.id)} + onSelect={() => handleSelect(o)} + /> + )} + emptyStateControls={ + canAdd ? ( + + ) : null + } + /> + + + {deletionError && ( {i18n._(t`Failed to delete one or more users.`)} - - ); - } + )} + + ); } -export { UsersList as _UsersList }; -export default withI18n()(withRouter(UsersList)); +export default withI18n()(UserList); diff --git a/awx/ui_next/src/screens/User/UserList/UserList.test.jsx b/awx/ui_next/src/screens/User/UserList/UserList.test.jsx index 96d763dad5..46cf02c128 100644 --- a/awx/ui_next/src/screens/User/UserList/UserList.test.jsx +++ b/awx/ui_next/src/screens/User/UserList/UserList.test.jsx @@ -1,16 +1,16 @@ import React from 'react'; +import { act } from 'react-dom/test-utils'; import { UsersAPI } from '../../../api'; import { mountWithContexts, waitForElement, } from '../../../../testUtils/enzymeHelpers'; -import UsersList, { _UsersList } from './UserList'; +import UsersList from './UserList'; jest.mock('../../../api'); let wrapper; -const loadUsers = jest.spyOn(_UsersList.prototype, 'loadUsers'); const mockUsers = [ { id: 1, @@ -84,7 +84,8 @@ const mockUsers = [ }, ]; -beforeAll(() => { +beforeEach(() => { + UsersAPI.destroy = jest.fn(); UsersAPI.read.mockResolvedValue({ data: { count: mockUsers.length, @@ -110,146 +111,96 @@ describe('UsersList with full permissions', () => { }); }); - beforeEach(() => { - wrapper = mountWithContexts(); - }); - - test('initially renders successfully', () => { - mountWithContexts( - - ); + beforeEach(async () => { + await act(async () => { + wrapper = mountWithContexts(); + }); + wrapper.update(); }); test('Users are retrieved from the api and the components finishes loading', async () => { await waitForElement(wrapper, 'ContentLoading', el => el.length === 0); - expect(loadUsers).toHaveBeenCalled(); + expect(UsersAPI.read).toHaveBeenCalled(); }); - test('Selects one team when row is checked', async () => { - await waitForElement( - wrapper, - 'UsersList', - el => el.state('hasContentLoading') === false - ); + test('should show add button', () => { + expect(wrapper.find('ToolbarAddButton').length).toBe(1); + }); + + test('should check and uncheck the row item', async () => { expect( - wrapper - .find('input[type="checkbox"]') - .findWhere(n => n.prop('checked') === true).length - ).toBe(0); - wrapper - .find('UserListItem') - .at(0) - .find('DataListCheck') - .props() - .onChange(true); + wrapper.find('DataListCheck[id="select-user-1"]').props().checked + ).toBe(false); + await act(async () => { + wrapper.find('DataListCheck[id="select-user-1"]').invoke('onChange')( + true + ); + }); wrapper.update(); expect( - wrapper - .find('input[type="checkbox"]') - .findWhere(n => n.prop('checked') === true).length - ).toBe(1); - }); - - test('Select all checkbox selects and unselects all rows', async () => { - await waitForElement( - wrapper, - 'UsersList', - el => el.state('hasContentLoading') === false - ); - expect( - wrapper - .find('input[type="checkbox"]') - .findWhere(n => n.prop('checked') === true).length - ).toBe(0); - wrapper - .find('Checkbox#select-all') - .props() - .onChange(true); + wrapper.find('DataListCheck[id="select-user-1"]').props().checked + ).toBe(true); + await act(async () => { + wrapper.find('DataListCheck[id="select-user-1"]').invoke('onChange')( + false + ); + }); wrapper.update(); expect( - wrapper - .find('input[type="checkbox"]') - .findWhere(n => n.prop('checked') === true).length - ).toBe(3); - wrapper - .find('Checkbox#select-all') - .props() - .onChange(false); + wrapper.find('DataListCheck[id="select-user-1"]').props().checked + ).toBe(false); + }); + + test('should check all row items when select all is checked', async () => { + wrapper.find('DataListCheck').forEach(el => { + expect(el.props().checked).toBe(false); + }); + await act(async () => { + wrapper.find('Checkbox#select-all').invoke('onChange')(true); + }); wrapper.update(); - expect( - wrapper - .find('input[type="checkbox"]') - .findWhere(n => n.prop('checked') === true).length - ).toBe(0); + wrapper.find('DataListCheck').forEach(el => { + expect(el.props().checked).toBe(true); + }); + await act(async () => { + wrapper.find('Checkbox#select-all').invoke('onChange')(false); + }); + wrapper.update(); + wrapper.find('DataListCheck').forEach(el => { + expect(el.props().checked).toBe(false); + }); }); - test('delete button is disabled if user does not have delete capabilities on a selected user', async () => { - wrapper.find('UsersList').setState({ - users: mockUsers, - itemCount: 2, - isInitialized: true, - selected: mockUsers.slice(0, 1), + test('should call api delete users for each selected user', async () => { + await act(async () => { + wrapper.find('DataListCheck[id="select-user-1"]').invoke('onChange')(); }); - await waitForElement( - wrapper, - 'ToolbarDeleteButton * button', - el => el.getDOMNode().disabled === false - ); - wrapper.find('UsersList').setState({ - selected: mockUsers, + wrapper.update(); + await act(async () => { + wrapper.find('ToolbarDeleteButton').invoke('onDelete')(); }); - await waitForElement( - wrapper, - 'ToolbarDeleteButton * button', - el => el.getDOMNode().disabled === true - ); + wrapper.update(); + expect(UsersAPI.destroy).toHaveBeenCalledTimes(1); }); - test('api is called to delete users for each selected user.', async () => { - UsersAPI.destroy = jest.fn(); - wrapper.find('UsersList').setState({ - users: mockUsers, - itemCount: 2, - isInitialized: true, - isModalOpen: true, - selected: mockUsers, + test('should show error modal when user is not successfully deleted from api', async () => { + UsersAPI.destroy.mockImplementationOnce(() => Promise.reject(new Error())); + // expect(wrapper.debug()).toBe(false); + expect(wrapper.find('Modal').length).toBe(0); + await act(async () => { + wrapper.find('DataListCheck[id="select-user-1"]').invoke('onChange')(); }); - await wrapper.find('ToolbarDeleteButton').prop('onDelete')(); - expect(UsersAPI.destroy).toHaveBeenCalledTimes(2); - }); - - test('error is shown when user not successfully deleted from api', async () => { - UsersAPI.destroy.mockRejectedValue( - new Error({ - response: { - config: { - method: 'delete', - url: '/api/v2/users/1', - }, - data: 'An error occurred', - }, - }) - ); - wrapper.find('UsersList').setState({ - users: mockUsers, - itemCount: 1, - isInitialized: true, - isModalOpen: true, - selected: mockUsers.slice(0, 1), + wrapper.update(); + await act(async () => { + wrapper.find('ToolbarDeleteButton').invoke('onDelete')(); }); - wrapper.find('ToolbarDeleteButton').prop('onDelete')(); - await waitForElement( - wrapper, - 'Modal', - el => el.props().isOpen === true && el.props().title === 'Error!' - ); - }); - - test('Add button shown for users with ability to POST', async () => { - await waitForElement(wrapper, 'ToolbarAddButton', el => el.length === 1); + wrapper.update(); + expect(wrapper.find('Modal').length).toBe(1); + await act(async () => { + wrapper.find('ModalBoxCloseButton').invoke('onClose')(); + }); + wrapper.update(); + expect(wrapper.find('Modal').length).toBe(0); }); }); @@ -263,9 +214,21 @@ describe('UsersList without full permissions', () => { }, }); - wrapper = mountWithContexts(); - await waitForElement(wrapper, 'ContentLoading', el => el.length === 1); - await waitForElement(wrapper, 'ContentLoading', el => el.length === 0); + await act(async () => { + wrapper = mountWithContexts(); + }); + wrapper.update(); expect(wrapper.find('ToolbarAddButton').length).toBe(0); }); }); + +describe('read call unsuccessful', () => { + test('should show content error when read call unsuccessful', async () => { + UsersAPI.read.mockRejectedValue(new Error()); + await act(async () => { + wrapper = mountWithContexts(); + }); + wrapper.update(); + expect(wrapper.find('ContentError').length).toBe(1); + }); +}); From bc023216896d73c3690fdf78f841f4b48a098f6a Mon Sep 17 00:00:00 2001 From: mabashian Date: Wed, 29 Jul 2020 08:45:06 -0400 Subject: [PATCH 2/3] Adopt useSelected --- .../src/screens/User/UserList/UserList.jsx | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/awx/ui_next/src/screens/User/UserList/UserList.jsx b/awx/ui_next/src/screens/User/UserList/UserList.jsx index 88c489c025..240720cec3 100644 --- a/awx/ui_next/src/screens/User/UserList/UserList.jsx +++ b/awx/ui_next/src/screens/User/UserList/UserList.jsx @@ -1,4 +1,4 @@ -import React, { useState, useEffect, useCallback } from 'react'; +import React, { useEffect, useCallback } from 'react'; import { useLocation, useRouteMatch } from 'react-router-dom'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; @@ -12,6 +12,7 @@ import PaginatedDataList, { ToolbarDeleteButton, } from '../../../components/PaginatedDataList'; import useRequest, { useDeleteItems } from '../../../util/useRequest'; +import useSelected from '../../../util/useSelected'; import { getQSConfig, parseQueryString } from '../../../util/qs'; import UserListItem from './UserListItem'; @@ -24,7 +25,6 @@ const QS_CONFIG = getQSConfig('user', { function UserList({ i18n }) { const location = useLocation(); const match = useRouteMatch(); - const [selected, setSelected] = useState([]); const { result: { users, itemCount, actions }, @@ -55,7 +55,9 @@ function UserList({ i18n }) { fetchUsers(); }, [fetchUsers]); - const isAllSelected = selected.length === users.length && selected.length > 0; + const { selected, isAllSelected, handleSelect, setSelected } = useSelected( + users + ); const { isLoading: isDeleteLoading, @@ -81,18 +83,6 @@ function UserList({ i18n }) { const hasContentLoading = isDeleteLoading || isLoading; const canAdd = actions && actions.POST; - const handleSelectAll = isSelected => { - setSelected(isSelected ? [...users] : []); - }; - - const handleSelect = row => { - if (selected.some(s => s.id === row.id)) { - setSelected(selected.filter(s => s.id !== row.id)); - } else { - setSelected(selected.concat(row)); - } - }; - return ( <> @@ -139,7 +129,9 @@ function UserList({ i18n }) { {...props} showSelectAll isAllSelected={isAllSelected} - onSelectAll={handleSelectAll} + onSelectAll={isSelected => + setSelected(isSelected ? [...users] : []) + } qsConfig={QS_CONFIG} additionalControls={[ ...(canAdd From b11f2f017f581f9d1e3793b72afd76147997bdfd Mon Sep 17 00:00:00 2001 From: mabashian Date: Wed, 29 Jul 2020 11:44:17 -0400 Subject: [PATCH 3/3] Update capitalization to match PF guidelines --- awx/ui_next/src/screens/User/UserList/UserList.jsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/awx/ui_next/src/screens/User/UserList/UserList.jsx b/awx/ui_next/src/screens/User/UserList/UserList.jsx index 240720cec3..20f5570aa6 100644 --- a/awx/ui_next/src/screens/User/UserList/UserList.jsx +++ b/awx/ui_next/src/screens/User/UserList/UserList.jsx @@ -102,11 +102,11 @@ function UserList({ i18n }) { isDefault: true, }, { - name: i18n._(t`First Name`), + name: i18n._(t`First name`), key: 'first_name', }, { - name: i18n._(t`Last Name`), + name: i18n._(t`Last name`), key: 'last_name', }, ]} @@ -116,11 +116,11 @@ function UserList({ i18n }) { key: 'username', }, { - name: i18n._(t`First Name`), + name: i18n._(t`First name`), key: 'first_name', }, { - name: i18n._(t`Last Name`), + name: i18n._(t`Last name`), key: 'last_name', }, ]}