Merge pull request #10292 from mabashian/8824-role-modal

Fixes bug where user/team role add modal state is not cleared on close SUMMARY link #8824 I modeled these changes after the pattern that existed between ResourceAccessList and AddResourceRole. The key to fixing this bug is that the component that renders the wizard needs to be unmounted when the wizard closes so that the state, etc get cleared out before the next time the wizard is opened. In order to achieve that I needed to decouple the add button from the wizard. The sort of weird part of this pattern (and this exists in the ResourceAccessList as well) is error handling. We pass the error back and set that to state before rendering the modal which isn't quite as clean as having the request made out at the list level and leveraging our hooks for error handling but I decided to just get in and get out and not worry about trying to refactor too much. Here it is in action: ISSUE TYPE Bugfix Pull Request COMPONENT NAME UI Reviewed-by: Kersom <None>
2026-01-11 18:09:57 -03:30 · 2021-06-03 15:52:40 +00:00 · 2021-06-03 15:52:40 +00:00 · 8adb53b5a8
commit 8adb53b5a8
parent 92401e5328 fa7a459e50
6 changed files with 101 additions and 96 deletions
--- a/awx/ui_next/src/components/AddRole/SelectResourceStep.jsx
+++ b/awx/ui_next/src/components/AddRole/SelectResourceStep.jsx
@ -100,7 +100,9 @@ function SelectResourceStep({
        headerRow={
          <HeaderRow qsConfig={QS_Config(sortColumns)}>
            {sortColumns.map(({ name, key }) => (
-              <HeaderCell sortKey={key}>{name}</HeaderCell>
+              <HeaderCell sortKey={key} key={key}>
+                {name}
+              </HeaderCell>
            ))}
          </HeaderRow>
        }
--- a/awx/ui_next/src/components/CheckboxListItem/CheckboxListItem.jsx
+++ b/awx/ui_next/src/components/CheckboxListItem/CheckboxListItem.jsx
@ -43,7 +43,7 @@ const CheckboxListItem = ({

      {columns?.length > 0 ? (
        columns.map(col => (
-          <Td aria-label={col.name} dataLabel={col.key}>
+          <Td aria-label={col.name} dataLabel={col.key} key={col.key}>
            {item[col.key]}
          </Td>
        ))
--- a/awx/ui_next/src/components/UserAndTeamAccessAdd/UserAndTeamAccessAdd.jsx
+++ b/awx/ui_next/src/components/UserAndTeamAccessAdd/UserAndTeamAccessAdd.jsx
@ -1,14 +1,9 @@
-import React, { useState, useCallback, useEffect, useContext } from 'react';
-
+import React, { useState, useCallback } from 'react';
 import { t } from '@lingui/macro';
 import { useParams } from 'react-router-dom';
 import styled from 'styled-components';
-import { Button, DropdownItem } from '@patternfly/react-core';
-import { KebabifiedContext } from '../../contexts/Kebabified';
-import useRequest, { useDismissableError } from '../../util/useRequest';
+import useRequest from '../../util/useRequest';
 import SelectableCard from '../SelectableCard';
-import AlertModal from '../AlertModal';
-import ErrorDetail from '../ErrorDetail';
 import Wizard from '../Wizard/Wizard';
 import useSelected from '../../util/useSelected';
 import SelectResourceStep from '../AddRole/SelectResourceStep';
@ -22,21 +17,20 @@ const Grid = styled.div`
  grid-template-columns: repeat(auto-fill, minmax(250px, 1fr));
 `;

-function UserAndTeamAccessAdd({ title, onFetchData, apiModel }) {
+function UserAndTeamAccessAdd({
+  title,
+  onFetchData,
+  apiModel,
+  onClose,
+  onError,
+}) {
  const [selectedResourceType, setSelectedResourceType] = useState(null);
-  const [isWizardOpen, setIsWizardOpen] = useState(false);
  const [stepIdReached, setStepIdReached] = useState(1);
  const { id: userId } = useParams();
-  const { isKebabified, onKebabModalChange } = useContext(KebabifiedContext);
  const {
    selected: resourcesSelected,
    handleSelect: handleResourceSelect,
  } = useSelected([]);
-  useEffect(() => {
-    if (isKebabified) {
-      onKebabModalChange(isWizardOpen);
-    }
-  }, [isKebabified, isWizardOpen, onKebabModalChange]);

  const {
    selected: rolesSelected,
@ -60,13 +54,10 @@ function UserAndTeamAccessAdd({ title, onFetchData, apiModel }) {

      await Promise.all(roleRequests);
      onFetchData();
-      setIsWizardOpen(false);
    }, [onFetchData, rolesSelected, apiModel, userId, resourcesSelected]),
    {}
  );

-  const { error, dismissError } = useDismissableError(saveError);
-
  const steps = [
    {
      id: 1,
@ -128,56 +119,22 @@ function UserAndTeamAccessAdd({ title, onFetchData, apiModel }) {
    },
  ];

-  if (error) {
-    return (
-      <AlertModal
-        aria-label={t`Associate role error`}
-        isOpen={error}
-        variant="error"
-        title={t`Error!`}
-        onClose={dismissError}
-      >
-        {t`Failed to associate role`}
-        <ErrorDetail error={error} />
-      </AlertModal>
-    );
+  if (saveError) {
+    onError(saveError);
+    onClose();
  }

  return (
-    <>
-      {isKebabified ? (
-        <DropdownItem
-          key="add"
-          component="button"
-          aria-label={t`Add`}
-          onClick={() => setIsWizardOpen(true)}
-        >
-          {t`Add`}
-        </DropdownItem>
-      ) : (
-        <Button
-          ouiaId="access-add-button"
-          variant="primary"
-          aria-label={t`Add`}
-          onClick={() => setIsWizardOpen(true)}
-          key="add"
-        >
-          {t`Add`}
-        </Button>
-      )}
-      {isWizardOpen && (
-        <Wizard
-          isOpen={isWizardOpen}
-          title={title}
-          steps={steps}
-          onClose={() => setIsWizardOpen(false)}
-          onNext={({ id }) =>
-            setStepIdReached(stepIdReached < id ? id : stepIdReached)
-          }
-          onSave={handleWizardSave}
-        />
-      )}
-    </>
+    <Wizard
+      isOpen
+      title={title}
+      steps={steps}
+      onClose={onClose}
+      onNext={({ id }) =>
+        setStepIdReached(stepIdReached < id ? id : stepIdReached)
+      }
+      onSave={handleWizardSave}
+    />
  );
 }

--- a/awx/ui_next/src/components/UserAndTeamAccessAdd/UserAndTeamAccessAdd.test.jsx
+++ b/awx/ui_next/src/components/UserAndTeamAccessAdd/UserAndTeamAccessAdd.test.jsx
@ -9,6 +9,9 @@ import UserAndTeamAccessAdd from './UserAndTeamAccessAdd';

 jest.mock('../../api');

+const onError = jest.fn();
+const onClose = jest.fn();
+
 describe('<UserAndTeamAccessAdd/>', () => {
  const resources = {
    data: {
@ -57,24 +60,21 @@ describe('<UserAndTeamAccessAdd/>', () => {
        <UserAndTeamAccessAdd
          apiModel={UsersAPI}
          onFetchData={() => {}}
+          onClose={onClose}
          title="Add user permissions"
+          onError={onError}
        />
      );
    });
-    await waitForElement(wrapper, 'Button[aria-label="Add"]');
+    wrapper.update();
  });
  afterEach(() => {
    jest.resetAllMocks();
  });
  test('should mount properly', async () => {
-    expect(wrapper.find('Button[aria-label="Add"]').length).toBe(1);
-    act(() => wrapper.find('Button[aria-label="Add"]').prop('onClick')());
-    wrapper.update();
    expect(wrapper.find('PFWizard').length).toBe(1);
  });
  test('should disable steps', async () => {
-    act(() => wrapper.find('Button[aria-label="Add"]').prop('onClick')());
-    wrapper.update();
    expect(wrapper.find('Button[type="submit"]').prop('isDisabled')).toBe(true);
    expect(
      wrapper
@ -122,8 +122,6 @@ describe('<UserAndTeamAccessAdd/>', () => {
    JobTemplatesAPI.read.mockResolvedValue(resources);
    JobTemplatesAPI.readOptions.mockResolvedValue(options);
    UsersAPI.associateRole.mockResolvedValue({});
-    act(() => wrapper.find('Button[aria-label="Add"]').prop('onClick')());
-    wrapper.update();
    await act(async () =>
      wrapper.find('SelectableCard[label="Job templates"]').prop('onClick')({
        fetchItems: JobTemplatesAPI.read,
@ -179,15 +177,16 @@ describe('<UserAndTeamAccessAdd/>', () => {
    await expect(UsersAPI.associateRole).toHaveBeenCalled();
  });

-  test('should close wizard', async () => {
-    act(() => wrapper.find('Button[aria-label="Add"]').prop('onClick')());
+  test('should close wizard on cancel', async () => {
+    await act(async () =>
+      wrapper.find('Button[children="Cancel"]').prop('onClick')()
+    );
    wrapper.update();
-    act(() => wrapper.find('PFWizard').prop('onClose')());
-    wrapper.update();
-    expect(wrapper.find('PFWizard').length).toBe(0);
+    expect(onClose).toHaveBeenCalledTimes(1);
  });

  test('should throw error', async () => {
+    expect(onError).toHaveBeenCalledTimes(0);
    JobTemplatesAPI.read.mockResolvedValue(resources);
    JobTemplatesAPI.readOptions.mockResolvedValue(options);
    UsersAPI.associateRole.mockRejectedValue(
@ -210,9 +209,6 @@ describe('<UserAndTeamAccessAdd/>', () => {
      }),
    }));

-    act(() => wrapper.find('Button[aria-label="Add"]').prop('onClick')());
-    wrapper.update();
-
    await act(async () =>
      wrapper.find('SelectableCard[label="Job templates"]').prop('onClick')({
        fetchItems: JobTemplatesAPI.read,
@ -261,6 +257,6 @@ describe('<UserAndTeamAccessAdd/>', () => {

    await expect(UsersAPI.associateRole).toHaveBeenCalled();
    wrapper.update();
-    expect(wrapper.find('AlertModal').length).toBe(1);
+    expect(onError).toHaveBeenCalled();
  });
 });
--- a/awx/ui_next/src/screens/Team/TeamRoles/TeamRolesList.jsx
+++ b/awx/ui_next/src/screens/Team/TeamRoles/TeamRolesList.jsx
@ -1,8 +1,6 @@
 import React, { useCallback, useEffect, useState } from 'react';
 import { useLocation } from 'react-router-dom';
-
 import { t } from '@lingui/macro';
-
 import {
  Button,
  EmptyState,
@ -22,6 +20,7 @@ import { getQSConfig, parseQueryString } from '../../../util/qs';
 import ErrorDetail from '../../../components/ErrorDetail';
 import AlertModal from '../../../components/AlertModal';
 import TeamRoleListItem from './TeamRoleListItem';
+import { ToolbarAddButton } from '../../../components/PaginatedDataList';
 import UserAndTeamAccessAdd from '../../../components/UserAndTeamAccessAdd/UserAndTeamAccessAdd';

 const QS_CONFIG = getQSConfig('roles', {
@ -33,6 +32,8 @@ const QS_CONFIG = getQSConfig('roles', {
 function TeamRolesList({ me, team }) {
  const { search } = useLocation();
  const [roleToDisassociate, setRoleToDisassociate] = useState(null);
+  const [showAddModal, setShowAddModal] = useState(false);
+  const [associateError, setAssociateError] = useState(null);

  const {
    isLoading,
@ -165,10 +166,10 @@ function TeamRolesList({ me, team }) {
            additionalControls={[
              ...(canAdd
                ? [
-                    <UserAndTeamAccessAdd
-                      apiModel={TeamsAPI}
-                      onFetchData={fetchRoles}
-                      title={t`Add team permissions`}
+                    <ToolbarAddButton
+                      ouiaId="role-add-button"
+                      key="add"
+                      onClick={() => setShowAddModal(true)}
                    />,
                  ]
                : []),
@ -192,7 +193,18 @@ function TeamRolesList({ me, team }) {
          />
        )}
      />
-
+      {showAddModal && (
+        <UserAndTeamAccessAdd
+          apiModel={TeamsAPI}
+          onFetchData={() => {
+            setShowAddModal(false);
+            fetchRoles();
+          }}
+          title={t`Add team permissions`}
+          onClose={() => setShowAddModal(false)}
+          onError={err => setAssociateError(err)}
+        />
+      )}
      {roleToDisassociate && (
        <AlertModal
          aria-label={t`Disassociate role`}
@ -228,6 +240,18 @@ function TeamRolesList({ me, team }) {
          </div>
        </AlertModal>
      )}
+      {associateError && (
+        <AlertModal
+          aria-label={t`Associate role error`}
+          isOpen={associateError}
+          variant="error"
+          title={t`Error!`}
+          onClose={() => setAssociateError(null)}
+        >
+          {t`Failed to associate role`}
+          <ErrorDetail error={associateError} />
+        </AlertModal>
+      )}
      {disassociationError && (
        <AlertModal
          isOpen={disassociationError}
--- a/awx/ui_next/src/screens/User/UserRoles/UserRolesList.jsx
+++ b/awx/ui_next/src/screens/User/UserRoles/UserRolesList.jsx
@ -18,7 +18,7 @@ import PaginatedTable, {
 } from '../../../components/PaginatedTable';
 import ErrorDetail from '../../../components/ErrorDetail';
 import AlertModal from '../../../components/AlertModal';
-
+import { ToolbarAddButton } from '../../../components/PaginatedDataList';
 import DatalistToolbar from '../../../components/DataListToolbar';
 import UserRolesListItem from './UserRolesListItem';
 import UserAndTeamAccessAdd from '../../../components/UserAndTeamAccessAdd/UserAndTeamAccessAdd';
@ -34,6 +34,8 @@ const QS_CONFIG = getQSConfig('roles', {
 function UserRolesList({ user }) {
  const { search } = useLocation();
  const [roleToDisassociate, setRoleToDisassociate] = useState(null);
+  const [showAddModal, setShowAddModal] = useState(false);
+  const [associateError, setAssociateError] = useState(null);

  const {
    isLoading,
@ -178,10 +180,10 @@ function UserRolesList({ user }) {
            additionalControls={[
              ...(canAdd
                ? [
-                    <UserAndTeamAccessAdd
-                      apiModel={UsersAPI}
-                      onFetchData={fetchRoles}
-                      title={t`Add user permissions`}
+                    <ToolbarAddButton
+                      ouiaId="role-add-button"
+                      key="add"
+                      onClick={() => setShowAddModal(true)}
                    />,
                  ]
                : []),
@ -189,6 +191,18 @@ function UserRolesList({ user }) {
          />
        )}
      />
+      {showAddModal && (
+        <UserAndTeamAccessAdd
+          apiModel={UsersAPI}
+          onFetchData={() => {
+            setShowAddModal(false);
+            fetchRoles();
+          }}
+          title={t`Add user permissions`}
+          onClose={() => setShowAddModal(false)}
+          onError={err => setAssociateError(err)}
+        />
+      )}
      {roleToDisassociate && (
        <AlertModal
          aria-label={t`Disassociate role`}
@ -224,6 +238,18 @@ function UserRolesList({ user }) {
          </div>
        </AlertModal>
      )}
+      {associateError && (
+        <AlertModal
+          aria-label={t`Associate role error`}
+          isOpen={associateError}
+          variant="error"
+          title={t`Error!`}
+          onClose={() => setAssociateError(null)}
+        >
+          {t`Failed to associate role`}
+          <ErrorDetail error={associateError} />
+        </AlertModal>
+      )}
      {disassociationError && (
        <AlertModal
          isOpen={disassociationError}