Address PR feedback. Refactors a bit of unit test coverage to move away from testing state. Re-organized some of the structure of the user list tests to be slightly more efficient.

2026-01-14 11:20:39 -03:30 · 2019-11-11 11:57:39 -05:00 · 2019-11-11 11:57:39 -05:00 · ab4fba7ce9
commit ab4fba7ce9
parent deb6e58397
13 changed files with 111 additions and 104 deletions
--- a/awx/ui_next/src/components/FormField/PasswordField.jsx
+++ b/awx/ui_next/src/components/FormField/PasswordField.jsx
@ -73,19 +73,13 @@ PasswordField.propTypes = {
  id: PropTypes.string.isRequired,
  name: PropTypes.string.isRequired,
  label: PropTypes.string.isRequired,
-  type: PropTypes.string,
  validate: PropTypes.func,
  isRequired: PropTypes.bool,
-  tooltip: PropTypes.node,
-  tooltipMaxWidth: PropTypes.string,
 };

 PasswordField.defaultProps = {
-  type: 'text',
  validate: () => {},
  isRequired: false,
-  tooltip: null,
-  tooltipMaxWidth: '',
 };

 export default withI18n()(PasswordField);
--- a/awx/ui_next/src/screens/Team/TeamList/TeamListItem.jsx
+++ b/awx/ui_next/src/screens/Team/TeamList/TeamListItem.jsx
@ -49,7 +49,7 @@ class TeamListItem extends React.Component {
              <DataListCell key="organization">
                {team.summary_fields.organization && (
                  <Fragment>
-                    <b style={{ marginRight: '20px' }}>
+                    <b css={{ marginRight: '20px' }}>
                      {i18n._(t`Organization`)}
                    </b>
                    <Link
--- a/awx/ui_next/src/screens/User/User.jsx
+++ b/awx/ui_next/src/screens/User/User.jsx
@ -18,6 +18,13 @@ import UserTeams from './UserTeams';
 import UserTokens from './UserTokens';
 import { UsersAPI } from '@api';

+const CardHeader = styled(PFCardHeader)`
+  --pf-c-card--first-child--PaddingTop: 0;
+  --pf-c-card--child--PaddingLeft: 0;
+  --pf-c-card--child--PaddingRight: 0;
+  position: relative;
+`;
+
 class User extends Component {
  constructor(props) {
    super(props);
@ -82,13 +89,6 @@ class User extends Component {
      { name: i18n._(t`Tokens`), link: `${match.url}/tokens`, id: 4 },
    ];

-    const CardHeader = styled(PFCardHeader)`
-      --pf-c-card--first-child--PaddingTop: 0;
-      --pf-c-card--child--PaddingLeft: 0;
-      --pf-c-card--child--PaddingRight: 0;
-      position: relative;
-    `;
-
    let cardHeader = (
      <CardHeader style={{ padding: 0 }}>
        <RoutedTabs
--- a/awx/ui_next/src/screens/User/User.test.jsx
+++ b/awx/ui_next/src/screens/User/User.test.jsx
@ -23,14 +23,14 @@ async function getUsers() {
  };
 }

-describe.only('<User />', () => {
+describe('<User />', () => {
  test('initially renders succesfully', () => {
    UsersAPI.readDetail.mockResolvedValue({ data: mockDetails });
    UsersAPI.read.mockImplementation(getUsers);
    mountWithContexts(<User setBreadcrumb={() => {}} me={mockMe} />);
  });

-  test('notifications tab shown for admins', async done => {
+  test('notifications tab shown for admins', async () => {
    UsersAPI.readDetail.mockResolvedValue({ data: mockDetails });
    UsersAPI.read.mockImplementation(getUsers);

@ -38,10 +38,9 @@ describe.only('<User />', () => {
      <User setBreadcrumb={() => {}} me={mockMe} />
    );
    await waitForElement(wrapper, '.pf-c-tabs__item', el => el.length === 5);
-    done();
  });

-  test('should show content error when user attempts to navigate to erroneous route', async done => {
+  test('should show content error when user attempts to navigate to erroneous route', async () => {
    const history = createMemoryHistory({
      initialEntries: ['/users/1/foobar'],
    });
@ -64,6 +63,5 @@ describe.only('<User />', () => {
      }
    );
    await waitForElement(wrapper, 'ContentError', el => el.length === 1);
-    done();
  });
 });
--- a/awx/ui_next/src/screens/User/UserAdd/UserAdd.test.jsx
+++ b/awx/ui_next/src/screens/User/UserAdd/UserAdd.test.jsx
@ -19,7 +19,7 @@ describe('<UserAdd />', () => {
      first_name: 'System',
      last_name: 'Administrator',
      password: 'password',
-      useranization: 1,
+      organization: 1,
      is_superuser: true,
      is_system_auditor: false,
    };
@ -63,7 +63,7 @@ describe('<UserAdd />', () => {
      first_name: 'System',
      last_name: 'Administrator',
      password: 'password',
-      useranization: 1,
+      organization: 1,
      is_superuser: true,
      is_system_auditor: false,
    };
--- a/awx/ui_next/src/screens/User/UserEdit/UserEdit.test.jsx
+++ b/awx/ui_next/src/screens/User/UserEdit/UserEdit.test.jsx
@ -16,7 +16,7 @@ describe('<UserEdit />', () => {
    first_name: 'System',
    last_name: 'Administrator',
    password: 'password',
-    useranization: 1,
+    organization: 1,
    is_superuser: true,
    is_system_auditor: false,
  };
--- a/awx/ui_next/src/screens/User/UserList/UserList.jsx
+++ b/awx/ui_next/src/screens/User/UserList/UserList.jsx
@ -161,10 +161,16 @@ class UsersList extends Component {
                  isSearchable: true,
                },
                {
-                  name: i18n._(t`Created`),
-                  key: 'created',
+                  name: i18n._(t`First Name`),
+                  key: 'first_name',
                  isSortable: true,
-                  isNumeric: true,
+                  isSearchable: true,
+                },
+                {
+                  name: i18n._(t`Last Name`),
+                  key: 'last_name',
+                  isSortable: true,
+                  isSearchable: true,
                },
              ]}
              renderToolbar={props => (
--- a/awx/ui_next/src/screens/User/UserList/UserList.test.jsx
+++ b/awx/ui_next/src/screens/User/UserList/UserList.test.jsx
@ -6,6 +6,8 @@ import UsersList, { _UsersList } from './UserList';

 jest.mock('@api');

+let wrapper;
+const loadUsers = jest.spyOn(_UsersList.prototype, 'loadUsers');
 const mockUsers = [
  {
    id: 1,
@ -79,15 +81,22 @@ const mockUsers = [
  },
 ];

-describe('<UsersList />', () => {
-  beforeEach(() => {
-    UsersAPI.read.mockResolvedValue({
-      data: {
-        count: mockUsers.length,
-        results: mockUsers,
-      },
-    });
+beforeAll(() => {
+  UsersAPI.read.mockResolvedValue({
+    data: {
+      count: mockUsers.length,
+      results: mockUsers,
+    },
+  });
+});

+afterEach(() => {
+  jest.clearAllMocks();
+  wrapper.unmount();
+});
+
+describe('UsersList with full permissions', () => {
+  beforeAll(() => {
    UsersAPI.readOptions.mockResolvedValue({
      data: {
        actions: {
@ -98,8 +107,8 @@ describe('<UsersList />', () => {
    });
  });

-  afterEach(() => {
-    jest.clearAllMocks();
+  beforeEach(() => {
+    wrapper = mountWithContexts(<UsersList />);
  });

  test('initially renders successfully', () => {
@ -111,9 +120,7 @@ describe('<UsersList />', () => {
    );
  });

-  test('Users are retrieved from the api and the components finishes loading', async done => {
-    const loadUsers = jest.spyOn(_UsersList.prototype, 'loadUsers');
-    const wrapper = mountWithContexts(<UsersList />);
+  test('Users are retrieved from the api and the components finishes loading', async () => {
    await waitForElement(
      wrapper,
      'UsersList',
@ -125,54 +132,67 @@ describe('<UsersList />', () => {
      'UsersList',
      el => el.state('hasContentLoading') === false
    );
-    done();
  });

-  test('handleSelect is called when a user list item is selected', async done => {
-    const handleSelect = jest.spyOn(_UsersList.prototype, 'handleSelect');
-    const wrapper = mountWithContexts(<UsersList />);
+  test('Selects one team when row is checked', async () => {
    await waitForElement(
      wrapper,
      'UsersList',
      el => el.state('hasContentLoading') === false
    );
-    await wrapper
-      .find('input#select-user-1')
-      .closest('DataListCheck')
+    expect(
+      wrapper
+        .find('input[type="checkbox"]')
+        .findWhere(n => n.prop('checked') === true).length
+    ).toBe(0);
+    wrapper
+      .find('UserListItem')
+      .at(0)
+      .find('DataListCheck')
      .props()
-      .onChange();
-    expect(handleSelect).toBeCalled();
-    await waitForElement(
-      wrapper,
-      'UsersList',
-      el => el.state('selected').length === 1
-    );
-    done();
+      .onChange(true);
+    wrapper.update();
+    expect(
+      wrapper
+        .find('input[type="checkbox"]')
+        .findWhere(n => n.prop('checked') === true).length
+    ).toBe(1);
  });

-  test('handleSelectAll is called when select all checkbox is clicked', async done => {
-    const handleSelectAll = jest.spyOn(_UsersList.prototype, 'handleSelectAll');
-    const wrapper = mountWithContexts(<UsersList />);
+  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);
-    expect(handleSelectAll).toBeCalled();
-    await waitForElement(
-      wrapper,
-      'UsersList',
-      el => el.state('selected').length === 2
-    );
-    done();
+    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.update();
+    expect(
+      wrapper
+        .find('input[type="checkbox"]')
+        .findWhere(n => n.prop('checked') === true).length
+    ).toBe(0);
  });

-  test('delete button is disabled if user does not have delete capabilities on a selected user', async done => {
-    const wrapper = mountWithContexts(<UsersList />);
+  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,
@ -192,12 +212,10 @@ describe('<UsersList />', () => {
      'ToolbarDeleteButton * button',
      el => el.getDOMNode().disabled === true
    );
-    done();
  });

  test('api is called to delete users for each selected user.', () => {
    UsersAPI.destroy = jest.fn();
-    const wrapper = mountWithContexts(<UsersList />);
    wrapper.find('UsersList').setState({
      users: mockUsers,
      itemCount: 2,
@ -209,7 +227,7 @@ describe('<UsersList />', () => {
    expect(UsersAPI.destroy).toHaveBeenCalledTimes(2);
  });

-  test('error is shown when user not successfully deleted from api', async done => {
+  test('error is shown when user not successfully deleted from api', async () => {
    UsersAPI.destroy.mockRejectedValue(
      new Error({
        response: {
@ -221,7 +239,6 @@ describe('<UsersList />', () => {
        },
      })
    );
-    const wrapper = mountWithContexts(<UsersList />);
    wrapper.find('UsersList').setState({
      users: mockUsers,
      itemCount: 1,
@ -235,12 +252,9 @@ describe('<UsersList />', () => {
      'Modal',
      el => el.props().isOpen === true && el.props().title === 'Error!'
    );
-
-    done();
  });

-  test('Add button shown for users without ability to POST', async done => {
-    const wrapper = mountWithContexts(<UsersList />);
+  test('Add button shown for users with ability to POST', async () => {
    await waitForElement(
      wrapper,
      'UsersList',
@ -252,10 +266,11 @@ describe('<UsersList />', () => {
      el => el.state('hasContentLoading') === false
    );
    expect(wrapper.find('ToolbarAddButton').length).toBe(1);
-    done();
  });
+});

-  test('Add button hidden for users without ability to POST', async done => {
+describe('UsersList without full permissions', () => {
+  test('Add button hidden for users without ability to POST', async () => {
    UsersAPI.readOptions.mockResolvedValue({
      data: {
        actions: {
@ -263,7 +278,8 @@ describe('<UsersList />', () => {
        },
      },
    });
-    const wrapper = mountWithContexts(<UsersList />);
+
+    wrapper = mountWithContexts(<UsersList />);
    await waitForElement(
      wrapper,
      'UsersList',
@ -275,6 +291,5 @@ describe('<UsersList />', () => {
      el => el.state('hasContentLoading') === false
    );
    expect(wrapper.find('ToolbarAddButton').length).toBe(0);
-    done();
  });
 });
--- a/awx/ui_next/src/screens/User/UserList/UserListItem.jsx
+++ b/awx/ui_next/src/screens/User/UserList/UserListItem.jsx
@ -49,9 +49,7 @@ class UserListItem extends React.Component {
              <DataListCell key="first-name">
                {user.first_name && (
                  <Fragment>
-                    <b style={{ marginRight: '20px' }}>
-                      {i18n._(t`First Name`)}
-                    </b>
+                    <b css={{ marginRight: '20px' }}>{i18n._(t`First Name`)}</b>
                    {user.first_name}
                  </Fragment>
                )}
@ -59,9 +57,7 @@ class UserListItem extends React.Component {
              <DataListCell key="last-name">
                {user.last_name && (
                  <Fragment>
-                    <b style={{ marginRight: '20px' }}>
-                      {i18n._(t`Last Name`)}
-                    </b>
+                    <b css={{ marginRight: '20px' }}>{i18n._(t`Last Name`)}</b>
                    {user.last_name}
                  </Fragment>
                )}
--- a/awx/ui_next/src/screens/User/UserList/UserListItem.test.jsx
+++ b/awx/ui_next/src/screens/User/UserList/UserListItem.test.jsx
@ -7,9 +7,15 @@ import { mountWithContexts } from '@testUtils/enzymeHelpers';
 import mockDetails from '../data.user.json';
 import UserListItem from './UserListItem';

-describe('<UserListItem />', () => {
-  test('initially renders succesfully', () => {
-    mountWithContexts(
+let wrapper;
+
+afterEach(() => {
+  wrapper.unmount();
+});
+
+describe('UserListItem with full permissions', () => {
+  beforeEach(() => {
+    wrapper = mountWithContexts(
      <I18nProvider>
        <MemoryRouter initialEntries={['/users']} initialIndex={0}>
          <UserListItem
@ -22,23 +28,17 @@ describe('<UserListItem />', () => {
      </I18nProvider>
    );
  });
+  test('initially renders succesfully', () => {
+    expect(wrapper.length).toBe(1);
+  });
  test('edit button shown to users with edit capabilities', () => {
-    const wrapper = mountWithContexts(
-      <I18nProvider>
-        <MemoryRouter initialEntries={['/users']} initialIndex={0}>
-          <UserListItem
-            user={mockDetails}
-            detailUrl="/user/1"
-            isSelected
-            onSelect={() => {}}
-          />
-        </MemoryRouter>
-      </I18nProvider>
-    );
    expect(wrapper.find('PencilAltIcon').exists()).toBeTruthy();
  });
+});
+
+describe('UserListItem without full permissions', () => {
  test('edit button hidden from users without edit capabilities', () => {
-    const wrapper = mountWithContexts(
+    wrapper = mountWithContexts(
      <I18nProvider>
        <MemoryRouter initialEntries={['/users']} initialIndex={0}>
          <UserListItem
--- a/awx/ui_next/src/screens/User/shared/UserForm.jsx
+++ b/awx/ui_next/src/screens/User/shared/UserForm.jsx
@ -6,7 +6,7 @@ import { Formik, Field } from 'formik';
 import { Form, FormGroup } from '@patternfly/react-core';
 import AnsibleSelect from '@components/AnsibleSelect';
 import FormActionGroup from '@components/FormActionGroup/FormActionGroup';
-import FormField, { FieldTooltip, PasswordField } from '@components/FormField';
+import FormField, { PasswordField } from '@components/FormField';
 import FormRow from '@components/FormRow';
 import OrganizationLookup from '@components/Lookup/OrganizationLookup';
 import { required, requiredEmail } from '@util/validators';
@ -169,9 +169,8 @@ function UserForm(props) {
                    helperTextInvalid={form.errors.user_type}
                    isRequired
                    isValid={isValid}
-                    label={i18n._(t`Job Type`)}
+                    label={i18n._(t`User Type`)}
                  >
-                    <FieldTooltip content={i18n._(t`Fill me in.`)} />
                    <AnsibleSelect
                      isValid={isValid}
                      id="user-type"
--- a/awx/ui_next/src/screens/User/shared/UserForm.test.jsx
+++ b/awx/ui_next/src/screens/User/shared/UserForm.test.jsx
@ -53,7 +53,7 @@ describe('<UserForm />', () => {
    expect(wrapper.find('FormGroup[label="Password"]').length).toBe(1);
    expect(wrapper.find('FormGroup[label="Confirm Password"]').length).toBe(1);
    expect(wrapper.find('FormGroup[label="Organization"]').length).toBe(1);
-    expect(wrapper.find('FormGroup[label="Job Type"]').length).toBe(1);
+    expect(wrapper.find('FormGroup[label="User Type"]').length).toBe(1);
  });

  test('edit form hides org field', async () => {
--- a/awx/ui_next/src/types.js
+++ b/awx/ui_next/src/types.js
@ -176,7 +176,6 @@ export const Job = shape({
  artifacts: shape({}),
 });

-<<<<<<< HEAD
 export const Host = shape({
  id: number.isRequired,
  type: oneOf(['host']),