Merge pull request #9488 from nixocio/ui_fix_rerererender

Fix extra re-render for Job Template Reviewed-by: https://github.com/apps/softwarefactory-project-zuul
ansible · Mar 5, 2021 · 8fb17cf · 8fb17cf
2 parents e4d227a + b7e614b
commit 8fb17cf
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 47 deletions.
diff --git a/awx/ui_next/src/components/Lookup/ExecutionEnvironmentLookup.jsx b/awx/ui_next/src/components/Lookup/ExecutionEnvironmentLookup.jsx
@@ -1,11 +1,11 @@
 import React, { useCallback, useEffect } from 'react';
-import { string, func, bool } from 'prop-types';
+import { string, func, bool, oneOfType, number } from 'prop-types';
 import { useLocation } from 'react-router-dom';
 import { withI18n } from '@lingui/react';
 import { t } from '@lingui/macro';
 import { FormGroup, Tooltip } from '@patternfly/react-core';
 
-import { ExecutionEnvironmentsAPI } from '../../api';
+import { ExecutionEnvironmentsAPI, ProjectsAPI } from '../../api';
 import { ExecutionEnvironment } from '../../types';
 import { getQSConfig, parseQueryString, mergeParams } from '../../util/qs';
 import Popover from '../Popover';
@@ -26,15 +26,38 @@ function ExecutionEnvironmentLookup({
   i18n,
   isDefaultEnvironment,
   isDisabled,
+  onBlur,
   onChange,
   organizationId,
   popoverContent,
+  projectId,
   tooltip,
   value,
-  onBlur,
 }) {
   const location = useLocation();
 
+  const {
+    request: fetchProject,
+    error: fetchProjectError,
+    isLoading: fetchProjectLoading,
+    result: project,
+  } = useRequest(
+    useCallback(async () => {
+      if (!projectId) {
+        return {};
+      }
+      const { data } = await ProjectsAPI.readDetail(projectId);
+      return data;
+    }, [projectId]),
+    {
+      project: null,
+    }
+  );
+
+  useEffect(() => {
+    fetchProject();
+  }, [fetchProject]);
+
   const {
     result: {
       executionEnvironments,
@@ -51,9 +74,10 @@ function ExecutionEnvironmentLookup({
       const globallyAvailableParams = globallyAvailable
         ? { or__organization__isnull: 'True' }
         : {};
-      const organizationIdParams = organizationId
-        ? { or__organization__id: organizationId }
-        : {};
+      const organizationIdParams =
+        organizationId || project?.organization
+          ? { or__organization__id: organizationId }
+          : {};
       const [{ data }, actionsResponse] = await Promise.all([
         ExecutionEnvironmentsAPI.read(
           mergeParams(params, {
@@ -73,7 +97,7 @@ function ExecutionEnvironmentLookup({
           actionsResponse.data.actions?.GET || {}
         ).filter(key => actionsResponse.data.actions?.GET[key].filterable),
       };
-    }, [location, globallyAvailable, organizationId]),
+    }, [location, globallyAvailable, organizationId, project]),
     {
       executionEnvironments: [],
       count: 0,
@@ -95,7 +119,7 @@ function ExecutionEnvironmentLookup({
         onBlur={onBlur}
         onChange={onChange}
         qsConfig={QS_CONFIG}
-        isLoading={isLoading}
+        isLoading={isLoading || fetchProjectLoading}
         isDisabled={isDisabled}
         renderOptionsList={({ state, dispatch, canDelete }) => (
           <OptionsList
@@ -146,7 +170,7 @@ function ExecutionEnvironmentLookup({
         renderLookup()
       )}
 
-      <LookupErrorMessage error={error} />
+      <LookupErrorMessage error={error || fetchProjectError} />
     </FormGroup>
   );
 }
@@ -156,12 +180,16 @@ ExecutionEnvironmentLookup.propTypes = {
   popoverContent: string,
   onChange: func.isRequired,
   isDefaultEnvironment: bool,
+  projectId: oneOfType([number, string]),
+  organizationId: oneOfType([number, string]),
 };
 
 ExecutionEnvironmentLookup.defaultProps = {
   popoverContent: '',
   isDefaultEnvironment: false,
   value: null,
+  projectId: null,
+  organizationId: null,
 };
 
 export default withI18n()(ExecutionEnvironmentLookup);
diff --git a/awx/ui_next/src/components/Lookup/ExecutionEnvironmentLookup.test.jsx b/awx/ui_next/src/components/Lookup/ExecutionEnvironmentLookup.test.jsx
@@ -2,7 +2,7 @@ import React from 'react';
 import { act } from 'react-dom/test-utils';
 import { mountWithContexts } from '../../../testUtils/enzymeHelpers';
 import ExecutionEnvironmentLookup from './ExecutionEnvironmentLookup';
-import { ExecutionEnvironmentsAPI } from '../../api';
+import { ExecutionEnvironmentsAPI, ProjectsAPI } from '../../api';
 
 jest.mock('../../api');
 
@@ -32,6 +32,17 @@ describe('ExecutionEnvironmentLookup', () => {
     ExecutionEnvironmentsAPI.read.mockResolvedValue(
       mockedExecutionEnvironments
     );
+    ProjectsAPI.read.mockResolvedValue({
+      data: {
+        count: 1,
+        results: [
+          {
+            id: 1,
+            name: 'Fuz',
+          },
+        ],
+      },
+    });
   });
 
   afterEach(() => {
@@ -52,14 +63,21 @@ describe('ExecutionEnvironmentLookup', () => {
     await act(async () => {
       wrapper = mountWithContexts(
         <ExecutionEnvironmentLookup
+          isDefaultEnvironment
           value={executionEnvironment}
           onChange={() => {}}
         />
       );
     });
     wrapper.update();
-    expect(ExecutionEnvironmentsAPI.read).toHaveBeenCalledTimes(1);
+    expect(ExecutionEnvironmentsAPI.read).toHaveBeenCalledTimes(2);
     expect(wrapper.find('ExecutionEnvironmentLookup')).toHaveLength(1);
+    expect(
+      wrapper.find('FormGroup[label="Default Execution Environment"]').length
+    ).toBe(1);
+    expect(
+      wrapper.find('FormGroup[label="Execution Environment"]').length
+    ).toBe(0);
   });
 
   test('should fetch execution environments', async () => {
@@ -71,6 +89,12 @@ describe('ExecutionEnvironmentLookup', () => {
         />
       );
     });
-    expect(ExecutionEnvironmentsAPI.read).toHaveBeenCalledTimes(1);
+    expect(ExecutionEnvironmentsAPI.read).toHaveBeenCalledTimes(2);
+    expect(
+      wrapper.find('FormGroup[label="Default Execution Environment"]').length
+    ).toBe(0);
+    expect(
+      wrapper.find('FormGroup[label="Execution Environment"]').length
+    ).toBe(1);
   });
 });
diff --git a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx
@@ -58,7 +58,7 @@ const jobTemplateData = {
   timeout: 0,
   use_fact_cache: false,
   verbosity: '0',
-  execution_environment: { id: 1, name: 'Foo' },
+  execution_environment: { id: 1, name: 'Foo', image: 'localhost.com' },
 };
 
 describe('<JobTemplateAdd />', () => {
@@ -139,7 +139,7 @@ describe('<JobTemplateAdd />', () => {
       wrapper = mountWithContexts(<JobTemplateAdd />);
     });
     await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0);
-    await act(() => {
+    await act(async () => {
       wrapper.find('input#template-name').simulate('change', {
         target: { value: 'Bar', name: 'name' },
       });

diff --git a/awx/ui_next/src/screens/Template/WorkflowJobTemplateAdd/WorkflowJobTemplateAdd.test.jsx b/awx/ui_next/src/screens/Template/WorkflowJobTemplateAdd/WorkflowJobTemplateAdd.test.jsx
@@ -6,6 +6,7 @@ import {
   WorkflowJobTemplatesAPI,
   OrganizationsAPI,
   LabelsAPI,
+  ExecutionEnvironmentsAPI,
 } from '../../../api';
 import { mountWithContexts } from '../../../../testUtils/enzymeHelpers';
 
@@ -15,6 +16,7 @@ jest.mock('../../../api/models/WorkflowJobTemplates');
 jest.mock('../../../api/models/Organizations');
 jest.mock('../../../api/models/Labels');
 jest.mock('../../../api/models/Inventories');
+jest.mock('../../../api/models/ExecutionEnvironments');
 
 describe('<WorkflowJobTemplateAdd/>', () => {
   let wrapper;
@@ -34,6 +36,10 @@ describe('<WorkflowJobTemplateAdd/>', () => {
       },
     });
 
+    ExecutionEnvironmentsAPI.read.mockResolvedValue({
+      data: { results: [{ id: 1, name: 'Foo', image: 'localhost.com' }] },
+    });
+
     await act(async () => {
       history = createMemoryHistory({
         initialEntries: ['/templates/workflow_job_template/add'],
@@ -82,6 +88,11 @@ describe('<WorkflowJobTemplateAdd/>', () => {
         .find('LabelSelect')
         .find('SelectToggle')
         .simulate('click');
+
+      wrapper.find('ExecutionEnvironmentLookup').invoke('onChange')({
+        id: 1,
+        name: 'Foo',
+      });
     });
 
     wrapper.update();
@@ -113,6 +124,7 @@ describe('<WorkflowJobTemplateAdd/>', () => {
       webhook_credential: undefined,
       webhook_service: '',
       webhook_url: '',
+      execution_environment: 1,
     });
 
     expect(WorkflowJobTemplatesAPI.associateLabel).toHaveBeenCalledTimes(1);

diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx
@@ -40,7 +40,7 @@ import {
   ExecutionEnvironmentLookup,
 } from '../../../components/Lookup';
 import Popover from '../../../components/Popover';
-import { JobTemplatesAPI, ProjectsAPI } from '../../../api';
+import { JobTemplatesAPI } from '../../../api';
 import LabelSelect from './LabelSelect';
 import PlaybookSelect from './PlaybookSelect';
 import WebhookSubForm from './WebhookSubForm';
@@ -108,30 +108,6 @@ function JobTemplateForm({
     executionEnvironmentHelpers,
   ] = useField({ name: 'execution_environment' });
 
-  const projectId = projectField.value?.id;
-
-  const {
-    request: fetchProject,
-    error: fetchProjectError,
-    isLoading: fetchProjectLoading,
-    result: projectData,
-  } = useRequest(
-    useCallback(async () => {
-      if (!projectId) {
-        return {};
-      }
-      const { data } = await ProjectsAPI.readDetail(projectId);
-      return data;
-    }, [projectId]),
-    {
-      projectData: null,
-    }
-  );
-
-  useEffect(() => {
-    fetchProject();
-  }, [fetchProject]);
-
   const {
     request: loadRelatedInstanceGroups,
     error: instanceGroupError,
@@ -213,16 +189,12 @@ function JobTemplateForm({
     callbackUrl = `${origin}${path}`;
   }
 
-  if (instanceGroupLoading || fetchProjectLoading) {
+  if (instanceGroupLoading) {
     return <ContentLoading />;
   }
 
-  if (contentError || instanceGroupError || fetchProjectError) {
-    return (
-      <ContentError
-        error={contentError || instanceGroupError || fetchProjectError}
-      />
-    );
+  if (contentError || instanceGroupError) {
+    return <ContentError error={contentError || instanceGroupError} />;
   }
 
   return (
@@ -323,7 +295,7 @@ function JobTemplateForm({
           )}
           globallyAvailable
           isDisabled={!projectField.value}
-          organizationId={projectData?.organization}
+          projectId={projectField.value?.id}
         />
 
         {projectField.value?.allow_override && (