Merge pull request #7375 from vjanssens/5929-consistent-subforms-in-job-template-forms

Use consistent layout for subforms in job/template forms Reviewed-by: https://github.com/apps/softwarefactory-project-zuul
2026-01-11 10:00:01 -03:30 · 2020-06-29 17:15:23 +00:00 · 2020-06-29 17:15:23 +00:00 · f5d38f57d4
commit f5d38f57d4
parent 8ea31d8cdd 3f33f1c97d
10 changed files with 206 additions and 180 deletions
--- a/awx/ui_next/README.md
+++ b/awx/ui_next/README.md
@ -8,7 +8,7 @@ The API development server will need to be running. See [CONTRIBUTING.md](../../

 ```shell
 # Start the ui development server. While running, the ui will be reachable
-# at https://127.0.01:3001 and updated automatically when code changes.
+# at https://127.0.0.1:3001 and updated automatically when code changes.
 npm --prefix=awx/ui_next start
 ```

--- a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx
+++ b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx
@ -16,6 +16,8 @@ function JobTemplateAdd() {
      initialInstanceGroups,
      credentials,
      webhook_credential,
+      webhook_key,
+      webhook_url,
      ...remainingValues
    } = values;

--- a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx
+++ b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx
@ -172,9 +172,7 @@ describe('<JobTemplateAdd />', () => {
      playbook: 'Baz',
      inventory: 2,
      webhook_credential: undefined,
-      webhook_key: '',
      webhook_service: '',
-      webhook_url: '',
    });
  });

--- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx
+++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx
@ -101,6 +101,8 @@ class JobTemplateEdit extends Component {
      initialInstanceGroups,
      credentials,
      webhook_credential,
+      webhook_key,
+      webhook_url,
      ...remainingValues
    } = values;

--- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx
+++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx
@ -271,7 +271,8 @@ describe('<JobTemplateEdit />', () => {
    delete expected.id;
    delete expected.type;
    delete expected.related;
-    expected.webhook_url = `${window.location.origin}${mockJobTemplate.related.webhook_receiver}`;
+    delete expected.webhook_key;
+    delete expected.webhook_url;
    expect(JobTemplatesAPI.update).toHaveBeenCalledWith(1, expected);
    expect(JobTemplatesAPI.disassociateLabel).toHaveBeenCalledTimes(2);
    expect(JobTemplatesAPI.associateLabel).toHaveBeenCalledTimes(4);
--- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx
+++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx
@ -9,6 +9,7 @@ import {
  Switch,
  Checkbox,
  TextInput,
+  Title,
 } from '@patternfly/react-core';
 import ContentError from '../../../components/ContentError';
 import ContentLoading from '../../../components/ContentLoading';
@ -27,6 +28,7 @@ import {
  FormColumnLayout,
  FormFullWidthLayout,
  FormCheckboxLayout,
+  SubFormLayout,
 } from '../../../components/FormLayout';
 import { VariablesField } from '../../../components/CodeMirrorInput';
 import { required } from '../../../util/validators';
@ -547,30 +549,53 @@ function JobTemplateForm({
                </FormCheckboxLayout>
              </FormGroup>
            </FormFullWidthLayout>
-            <WebhookSubForm
-              enableWebhooks={enableWebhooks}
-              templateType={template.type}
-            />
-            {allowCallbacks && (
+
+            {(allowCallbacks || enableWebhooks) && (
              <>
-                {callbackUrl && (
-                  <FormGroup
-                    label={i18n._(t`Provisioning Callback URL`)}
-                    fieldId="template-callback-url"
-                  >
-                    <TextInput
-                      id="template-callback-url"
-                      isDisabled
-                      value={callbackUrl}
-                    />
-                  </FormGroup>
-                )}
-                <FormField
-                  id="template-host-config-key"
-                  name="host_config_key"
-                  label={i18n._(t`Host Config Key`)}
-                  validate={allowCallbacks ? required(null, i18n) : null}
-                />
+                <SubFormLayout>
+                  {allowCallbacks && (
+                    <>
+                      <Title size="md" headingLevel="h4">
+                        {i18n._(t`Provisioning Callback details`)}
+                      </Title>
+                      <FormColumnLayout>
+                        {callbackUrl && (
+                          <FormGroup
+                            label={i18n._(t`Provisioning Callback URL`)}
+                            fieldId="template-callback-url"
+                          >
+                            <TextInput
+                              id="template-callback-url"
+                              isDisabled
+                              value={callbackUrl}
+                            />
+                          </FormGroup>
+                        )}
+                        <FormField
+                          id="template-host-config-key"
+                          name="host_config_key"
+                          label={i18n._(t`Host Config Key`)}
+                          validate={
+                            allowCallbacks ? required(null, i18n) : null
+                          }
+                        />
+                      </FormColumnLayout>
+                    </>
+                  )}
+
+                  {allowCallbacks && enableWebhooks && <br />}
+
+                  {enableWebhooks && (
+                    <>
+                      <Title size="md" headingLevel="h4">
+                        {i18n._(t`Webhook details`)}
+                      </Title>
+                      <FormColumnLayout>
+                        <WebhookSubForm templateType={template.type} />
+                      </FormColumnLayout>
+                    </>
+                  )}
+                </SubFormLayout>
              </>
            )}
          </FormColumnLayout>
--- a/awx/ui_next/src/screens/Template/shared/WebhookSubForm.jsx
+++ b/awx/ui_next/src/screens/Template/shared/WebhookSubForm.jsx
@ -23,7 +23,7 @@ import {
  CredentialTypesAPI,
 } from '../../../api';

-function WebhookSubForm({ i18n, enableWebhooks, templateType }) {
+function WebhookSubForm({ i18n, templateType }) {
  const { id } = useParams();

  const { pathname } = useLocation();
@ -36,6 +36,7 @@ function WebhookSubForm({ i18n, enableWebhooks, templateType }) {
    webhookServiceHelpers,
  ] = useField('webhook_service');

+  // eslint-disable-next-line no-unused-vars
  const [webhookUrlField, webhookUrlMeta, webhookUrlHelpers] = useField(
    'webhook_url'
  );
@ -71,21 +72,6 @@ function WebhookSubForm({ i18n, enableWebhooks, templateType }) {
    loadCredentialType();
  }, [loadCredentialType]);

-  useEffect(() => {
-    if (enableWebhooks) {
-      webhookServiceHelpers.setValue(webhookServiceMeta.initialValue);
-      webhookUrlHelpers.setValue(webhookUrlMeta.initialValue);
-      webhookKeyHelpers.setValue(webhookKeyMeta.initialValue);
-      webhookCredentialHelpers.setValue(webhookCredentialMeta.initialValue);
-    } else {
-      webhookServiceHelpers.setValue('');
-      webhookUrlHelpers.setValue('');
-      webhookKeyHelpers.setValue('');
-      webhookCredentialHelpers.setValue(null);
-    }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [enableWebhooks]);
-
  const { request: fetchWebhookKey, error: webhookKeyError } = useRequest(
    useCallback(async () => {
      const updateWebhookKey =
@ -134,108 +120,106 @@ function WebhookSubForm({ i18n, enableWebhooks, templateType }) {
    return <ContentLoading />;
  }
  return (
-    enableWebhooks && (
-      <FormColumnLayout>
-        <FormGroup
-          name="webhook_service"
-          fieldId="webhook_service"
-          helperTextInvalid={webhookServiceMeta.error}
-          label={i18n._(t`Webhook Service`)}
-        >
-          <FieldTooltip content={i18n._(t`Select a webhook service.`)} />
-          <AnsibleSelect
-            {...webhookServiceField}
-            id="webhook_service"
-            data={webhookServiceOptions}
-            onChange={(event, val) => {
-              webhookServiceHelpers.setValue(val);
-              webhookUrlHelpers.setValue(
-                pathname.endsWith('/add')
-                  ? i18n
-                      ._(t`a new webhook url will be generated on save.`)
-                      .toUpperCase()
-                  : `${origin}/api/v2/${templateType}s/${id}/${val}/`
-              );
-              if (val === webhookServiceMeta.initialValue || val === '') {
-                webhookKeyHelpers.setValue(webhookKeyMeta.initialValue);
-                webhookCredentialHelpers.setValue(
-                  webhookCredentialMeta.initialValue
-                );
-              } else {
-                webhookKeyHelpers.setValue(
-                  i18n
-                    ._(t`a new webhook key will be generated on save.`)
+    <FormColumnLayout>
+      <FormGroup
+        name="webhook_service"
+        fieldId="webhook_service"
+        helperTextInvalid={webhookServiceMeta.error}
+        label={i18n._(t`Webhook Service`)}
+      >
+        <FieldTooltip content={i18n._(t`Select a webhook service.`)} />
+        <AnsibleSelect
+          {...webhookServiceField}
+          id="webhook_service"
+          data={webhookServiceOptions}
+          onChange={(event, val) => {
+            webhookServiceHelpers.setValue(val);
+            webhookUrlHelpers.setValue(
+              pathname.endsWith('/add')
+                ? i18n
+                    ._(t`a new webhook url will be generated on save.`)
                    .toUpperCase()
-                );
-                webhookCredentialHelpers.setValue(null);
-              }
-            }}
+                : `${origin}/api/v2/${templateType}s/${id}/${val}/`
+            );
+            if (val === webhookServiceMeta.initialValue || val === '') {
+              webhookKeyHelpers.setValue(webhookKeyMeta.initialValue);
+              webhookCredentialHelpers.setValue(
+                webhookCredentialMeta.initialValue
+              );
+            } else {
+              webhookKeyHelpers.setValue(
+                i18n
+                  ._(t`a new webhook key will be generated on save.`)
+                  .toUpperCase()
+              );
+              webhookCredentialHelpers.setValue(null);
+            }
+          }}
+        />
+      </FormGroup>
+      <>
+        <FormGroup
+          type="text"
+          fieldId="jt-webhookURL"
+          label={i18n._(t`Webhook URL`)}
+          name="webhook_url"
+        >
+          <FieldTooltip
+            content={i18n._(
+              t`Webhook services can launch jobs with this workflow job template by making a POST request to this URL.`
+            )}
+          />
+          <TextInput
+            id="t-webhookURL"
+            aria-label={i18n._(t`Webhook URL`)}
+            value={webhookUrlField.value}
+            isReadOnly
          />
        </FormGroup>
-        <>
-          <FormGroup
-            type="text"
-            fieldId="jt-webhookURL"
-            label={i18n._(t`Webhook URL`)}
-            name="webhook_url"
-          >
-            <FieldTooltip
-              content={i18n._(
-                t`Webhook services can launch jobs with this workflow job template by making a POST request to this URL.`
-              )}
-            />
-            <TextInput
-              id="t-webhookURL"
-              aria-label={i18n._(t`Webhook URL`)}
-              value={webhookUrlField.value}
-              isReadOnly
-            />
-          </FormGroup>
-          <FormGroup
-            label={i18n._(t`Webhook Key`)}
-            fieldId="template-webhook_key"
-          >
-            <FieldTooltip
-              content={i18n._(
-                t`Webhook services can use this as a shared secret.`
-              )}
-            />
-            <InputGroup>
-              <TextInput
-                id="template-webhook_key"
-                isReadOnly
-                aria-label="wfjt-webhook-key"
-                value={webhookKeyField.value}
-              />
-              <Button
-                isDisabled={isUpdateKeyDisabled}
-                variant="tertiary"
-                aria-label={i18n._(t`Update webhook key`)}
-                onClick={changeWebhookKey}
-              >
-                <SyncAltIcon />
-              </Button>
-            </InputGroup>
-          </FormGroup>
-        </>
-
-        {credTypeId && (
-          <CredentialLookup
-            label={i18n._(t`Webhook Credential`)}
-            tooltip={i18n._(
-              t`Optionally select the credential to use to send status updates back to the webhook service.`
+        <FormGroup
+          label={i18n._(t`Webhook Key`)}
+          fieldId="template-webhook_key"
+        >
+          <FieldTooltip
+            content={i18n._(
+              t`Webhook services can use this as a shared secret.`
            )}
-            credentialTypeId={credTypeId}
-            onChange={value => {
-              webhookCredentialHelpers.setValue(value || null);
-            }}
-            isValid={!webhookCredentialMeta.error}
-            helperTextInvalid={webhookCredentialMeta.error}
-            value={webhookCredentialField.value}
          />
-        )}
-      </FormColumnLayout>
-    )
+          <InputGroup>
+            <TextInput
+              id="template-webhook_key"
+              isReadOnly
+              aria-label="wfjt-webhook-key"
+              value={webhookKeyField.value}
+            />
+            <Button
+              isDisabled={isUpdateKeyDisabled}
+              variant="tertiary"
+              aria-label={i18n._(t`Update webhook key`)}
+              onClick={changeWebhookKey}
+            >
+              <SyncAltIcon />
+            </Button>
+          </InputGroup>
+        </FormGroup>
+      </>
+
+      {credTypeId && (
+        <CredentialLookup
+          label={i18n._(t`Webhook Credential`)}
+          tooltip={i18n._(
+            t`Optionally select the credential to use to send status updates back to the webhook service.`
+          )}
+          credentialTypeId={credTypeId}
+          onChange={value => {
+            webhookCredentialHelpers.setValue(value || null);
+          }}
+          isValid={!webhookCredentialMeta.error}
+          helperTextInvalid={webhookCredentialMeta.error}
+          value={webhookCredentialField.value}
+        />
+      )}
+    </FormColumnLayout>
  );
 }
 export default withI18n()(WebhookSubForm);
--- a/awx/ui_next/src/screens/Template/shared/WebhookSubForm.test.jsx
+++ b/awx/ui_next/src/screens/Template/shared/WebhookSubForm.test.jsx
@ -34,7 +34,7 @@ describe('<WebhookSubForm />', () => {
      wrapper = mountWithContexts(
        <Route path="templates/:templateType/:id/edit">
          <Formik initialValues={initialValues}>
-            <WebhookSubForm enableWebhooks templateType="job_template" />
+            <WebhookSubForm templateType="job_template" />
          </Formik>
        </Route>,
        {
@ -103,7 +103,7 @@ describe('<WebhookSubForm />', () => {
              webhook_key: 'A NEW WEBHOOK KEY WILL BE GENERATED ON SAVE.',
            }}
          >
-            <WebhookSubForm enableWebhooks templateType="job_template" />
+            <WebhookSubForm templateType="job_template" />
          </Formik>
        </Route>,
        {
@ -133,10 +133,7 @@ describe('<WebhookSubForm />', () => {
      newWrapper = mountWithContexts(
        <Route path="templates/:templateType/:id/edit">
          <Formik initialValues={{ ...initialValues, webhook_url }}>
-            <WebhookSubForm
-              enableWebhooks
-              templateType="workflow_job_template"
-            />
+            <WebhookSubForm templateType="workflow_job_template" />
          </Formik>
        </Route>,
        {
--- a/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.jsx
+++ b/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.jsx
@ -4,7 +4,13 @@ import PropTypes, { shape } from 'prop-types';

 import { withI18n } from '@lingui/react';
 import { useField, withFormik } from 'formik';
-import { Form, FormGroup, Checkbox, TextInput } from '@patternfly/react-core';
+import {
+  Form,
+  FormGroup,
+  Checkbox,
+  TextInput,
+  Title,
+} from '@patternfly/react-core';
 import { required } from '../../../util/validators';

 import FieldWithPrompt from '../../../components/FieldWithPrompt';
@ -16,6 +22,7 @@ import {
  FormColumnLayout,
  FormFullWidthLayout,
  FormCheckboxLayout,
+  SubFormLayout,
 } from '../../../components/FormLayout';
 import OrganizationLookup from '../../../components/Lookup/OrganizationLookup';
 import { InventoryLookup } from '../../../components/Lookup';
@ -175,39 +182,47 @@ function WorkflowJobTemplateForm({
          )}
        />
      </FormFullWidthLayout>
-      <FormCheckboxLayout fieldId="options" isInline label={i18n._(t`Options`)}>
-        <Checkbox
-          aria-label={i18n._(t`Enable Webhook`)}
-          label={
-            <span>
-              {i18n._(t`Enable Webhook`)}
-              &nbsp;
-              <FieldTooltip
-                content={i18n._(
-                  t`Enable Webhook for this workflow job template.`
-                )}
-              />
-            </span>
-          }
-          id="wfjt-enabled-webhooks"
-          isChecked={enableWebhooks}
-          onChange={checked => {
-            setEnableWebhooks(checked);
-          }}
-        />
-        <CheckboxField
-          name="allow_simultaneous"
-          id="allow_simultaneous"
-          tooltip={i18n._(
-            t`If enabled, simultaneous runs of this workflow job template will be allowed.`
-          )}
-          label={i18n._(t`Enable Concurrent Jobs`)}
-        />
-      </FormCheckboxLayout>
-      <WebhookSubForm
-        enableWebhooks={enableWebhooks}
-        templateType={template.type}
-      />
+      <FormGroup fieldId="options" label={i18n._(t`Options`)}>
+        <FormCheckboxLayout isInline>
+          <Checkbox
+            aria-label={i18n._(t`Enable Webhook`)}
+            label={
+              <span>
+                {i18n._(t`Enable Webhook`)}
+                &nbsp;
+                <FieldTooltip
+                  content={i18n._(
+                    t`Enable Webhook for this workflow job template.`
+                  )}
+                />
+              </span>
+            }
+            id="wfjt-enabled-webhooks"
+            isChecked={enableWebhooks}
+            onChange={checked => {
+              setEnableWebhooks(checked);
+            }}
+          />
+          <CheckboxField
+            name="allow_simultaneous"
+            id="allow_simultaneous"
+            tooltip={i18n._(
+              t`If enabled, simultaneous runs of this workflow job template will be allowed.`
+            )}
+            label={i18n._(t`Enable Concurrent Jobs`)}
+          />
+        </FormCheckboxLayout>
+      </FormGroup>
+
+      {enableWebhooks && (
+        <SubFormLayout>
+          <Title size="md" headingLevel="h4">
+            {i18n._(t`Webhook details`)}
+          </Title>
+          <WebhookSubForm templateType={template.type} />
+        </SubFormLayout>
+      )}
+
      {submitError && <FormSubmitError error={submitError} />}
      <FormActionGroup onCancel={handleCancel} onSubmit={handleSubmit} />
    </Form>
--- a/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.test.jsx
+++ b/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.test.jsx
@ -44,7 +44,9 @@ describe('<WorkflowJobTemplateForm/>', () => {
    related: {
      webhook_receiver: '/api/v2/workflow_job_templates/57/gitlab/',
    },
+    webhook_credential: null,
    webhook_key: 'sdfghjklmnbvcdsew435678iokjhgfd',
+    webhook_service: 'github',
  };

  beforeEach(async () => {