Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Search][A11Y] Playground -> Open AI form #202071

Merged
merged 5 commits into from
Dec 16, 2024
+163 −11
Merged

[Search][A11Y] Playground -> Open AI form #202071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
382501b
Adding header text when all fields are required in the form
Samiul-TheSoccerFan Nov 29, 2024
3dc0bc1
Updating wanring message with callout
Samiul-TheSoccerFan Dec 2, 2024
bb9f46c
adding error message in the connector form
Samiul-TheSoccerFan Dec 10, 2024
d5b647e
Update to hide error message when submitted in both add and edit mode
Samiul-TheSoccerFan Dec 13, 2024
31fc87e
Merge branch 'main' into a11y-playground-openai
elasticmachine Dec 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 66 additions & 2 deletions .../public/application/sections/action_connector_form/create_connector_flyout/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import React, { lazy } from 'react';

import { actionTypeRegistryMock } from '../../../action_type_registry.mock';
import userEvent from '@testing-library/user-event';
import { waitFor, act } from '@testing-library/react';
import { waitFor, act, screen } from '@testing-library/react';
import CreateConnectorFlyout from '.';
import { AppMockRenderer, createAppMockRenderer } from '../../test_utils';
import { TECH_PREVIEW_LABEL } from '../../translations';
Expand Down Expand Up @@ -426,7 +426,7 @@ describe('CreateConnectorFlyout', () => {

describe('Submitting', () => {
it('creates a connector correctly', async () => {
const { getByTestId } = appMockRenderer.render(
const { getByTestId, queryByTestId } = appMockRenderer.render(
<CreateConnectorFlyout
actionTypeRegistry={actionTypeRegistry}
onClose={onClose}
Expand Down Expand Up @@ -469,6 +469,70 @@ describe('CreateConnectorFlyout', () => {
name: 'My test',
secrets: {},
});
expect(queryByTestId('connector-form-header-error-label')).not.toBeInTheDocument();
});

it('show error message in the form header', async () => {
appMockRenderer.render(
<CreateConnectorFlyout
actionTypeRegistry={actionTypeRegistry}
onClose={onClose}
onConnectorCreated={onConnectorCreated}
onTestConnector={onTestConnector}
/>
);

await userEvent.click(await screen.findByTestId(`${actionTypeModel.id}-card`));
expect(await screen.findByTestId('test-connector-text-field')).toBeInTheDocument();

await userEvent.type(
await screen.findByTestId('test-connector-text-field'),
'My text field',
{
delay: 100,
}
);

await userEvent.click(await screen.findByTestId('create-connector-flyout-save-btn'));
expect(onClose).not.toHaveBeenCalled();
expect(onConnectorCreated).not.toHaveBeenCalled();
expect(await screen.findByTestId('connector-form-header-error-label')).toBeInTheDocument();
});

it('removes error message from the form header', async () => {
appMockRenderer.render(
<CreateConnectorFlyout
actionTypeRegistry={actionTypeRegistry}
onClose={onClose}
onConnectorCreated={onConnectorCreated}
onTestConnector={onTestConnector}
/>
);

await userEvent.click(await screen.findByTestId(`${actionTypeModel.id}-card`));
expect(await screen.findByTestId('test-connector-text-field')).toBeInTheDocument();

await userEvent.type(
await screen.findByTestId('test-connector-text-field'),
'My text field',
{
delay: 100,
}
);

await userEvent.click(await screen.findByTestId('create-connector-flyout-save-btn'));
expect(onClose).not.toHaveBeenCalled();
expect(onConnectorCreated).not.toHaveBeenCalled();
expect(await screen.findByTestId('connector-form-header-error-label')).toBeInTheDocument();

await userEvent.type(await screen.findByTestId('nameInput'), 'My test', {
delay: 100,
});

await userEvent.click(await screen.findByTestId('create-connector-flyout-save-btn'));
expect(onClose).toHaveBeenCalled();
expect(onConnectorCreated).toHaveBeenCalled();
expect(screen.queryByTestId('connector-form-header-error-label')).not.toBeInTheDocument();
});

it('runs pre submit validator correctly', async () => {
Expand Down
24 changes: 24 additions & 0 deletions ...ns_ui/public/application/sections/action_connector_form/create_connector_flyout/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React, { memo, ReactNode, useCallback, useEffect, useRef, useState } from
import {
EuiButton,
EuiButtonGroup,
EuiCallOut,
EuiFlexGroup,
EuiFlexItem,
EuiFlyout,
Expand All @@ -18,6 +19,7 @@ import {

import { getConnectorCompatibility } from '@kbn/actions-plugin/common';
import { FormattedMessage } from '@kbn/i18n-react';
import { i18n } from '@kbn/i18n';
import {
ActionConnector,
ActionType,
Expand Down Expand Up @@ -60,6 +62,7 @@ const CreateConnectorFlyoutComponent: React.FC<CreateConnectorFlyoutProps> = ({
const [actionType, setActionType] = useState<ActionType | null>(null);
const [hasActionsUpgradeableByTrial, setHasActionsUpgradeableByTrial] = useState<boolean>(false);
const canSave = hasSaveActionsCapability(capabilities);
const [showFormErrors, setShowFormErrors] = useState<boolean>(false);

const [preSubmitValidationErrorMessage, setPreSubmitValidationErrorMessage] =
useState<ReactNode>(null);
Expand Down Expand Up @@ -106,6 +109,7 @@ const CreateConnectorFlyoutComponent: React.FC<CreateConnectorFlyoutProps> = ({

const setResetForm = (reset: ResetForm) => {
resetConnectorForm.current = reset;
setShowFormErrors(false);
};

const onChangeGroupAction = (id: string) => {
Expand All @@ -127,6 +131,7 @@ const CreateConnectorFlyoutComponent: React.FC<CreateConnectorFlyoutProps> = ({

const validateAndCreateConnector = useCallback(async () => {
setPreSubmitValidationErrorMessage(null);
setShowFormErrors(false);

const { isValid, data } = await submit();
if (!isMounted.current) {
Expand Down Expand Up @@ -159,6 +164,8 @@ const CreateConnectorFlyoutComponent: React.FC<CreateConnectorFlyoutProps> = ({

const createdConnector = await createConnector(validConnector);
return createdConnector;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a form has only required fields (no optional fields)

Having the code here will not meet this requirement. As is the message is triggered whenever the form is not valid.

We should not display a message saying all fields are required when some are optional.

Screenshot 2024-12-04 at 10 36 09

This change will affect all connectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I restrict this change only to gen AI connectors that we use in playground as most of these have required fields, or should all connectors have the same behavior?

cc: @mdefazio

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adcoelho What if we make the message more generic?

"There are required fields missing" or "Some fields are not valid"

Ideally we will have some message at the top letting the user know there are errors in the form. (which I think is the point of the PR?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we make the message more generic?
"There are required fields missing" or "Some fields are not valid"

These would imo make more sense but I am also trying to be cautious here about a UI change that would affect all connectors. We didn't discuss it internally and weren't aware. But it doesn't seem like a big deal, I'll bring it up in our sync(today).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we make the message more generic?
"There are required fields missing" or "Some fields are not valid"

These would imo make more sense but I am also trying to be cautious here about a UI change that would affect all connectors. We didn't discuss it internally and weren't aware. But it doesn't seem like a big deal, I'll bring it up in our sync(today).

Thank you @adcoelho. Please keep us informed about the team's decision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, getting back to this @mdefazio

What if we make the message more generic?
"There are required fields missing" or "Some fields are not valid"

Won't your suggestion then go against the original comment from @MichaelMarcialis that originated this task and where he explicitly says:

If the form has one or more optional fields, no additional affordance is necessary, beyond marking the optional fields.

?

This change only seems to make sense if it applies to connectors where all fields are required and if not we should leave things as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general idea is that if there are any errors in the form, a header should appear at the top with a message like, There are some errors while submitting. Individual fields or actions can then display their specific error messages for more details. However, I'll let @mdefazio confirm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct—this is about errors in the form. We removed the message "All fields required" that appears initially prior to submission since this wouldn't make sense for all Connector forms.

Related, the original comment is around noting which fields are required, prior to form submission, not about error handling.

If i'm not mistaken, the benefit of this additional callout, is to indicate that there are errors in the form, which may be out of view initially, and provide a landmark and message to screen readers.

If we make this message generic, meaning simply "There are fields with errors.", or "Some fields were not valid", etc., it allows us to show this regardless if all fields are required or not—so for every connector type form.

Someone can correct me on the need and benefit of this, but that is my understanding. I acknowledge that the Connector framework is unique based on the dynamic nature of the forms, so this is a tricky one to start off with for this pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, my comment wasn't saying that an error callout such a this can't be shown on form submission. I was just stating that a simple note stating that "All items are required" (shown before form submission) isn't to be used on a form that has one or more optional elements. If ya'll feel this form is lengthy, with some elements with validation errors falling out of view, and could benefit from the inclusion of an additional callout indicating errors, I'll leave that to your discretion.

setShowFormErrors(true);
}
}, [submit, preSubmitValidator, createConnector]);

Expand Down Expand Up @@ -228,6 +235,23 @@ const CreateConnectorFlyoutComponent: React.FC<CreateConnectorFlyoutProps> = ({
<EuiSpacer size="xs" />
</>
)}
{showFormErrors && (
<>
<EuiCallOut
size="s"
color="danger"
iconType="warning"
data-test-subj="connector-form-header-error-label"
title={i18n.translate(
'xpack.triggersActionsUI.sections.actionConnectorAdd.headerFormLabel',
{
defaultMessage: 'There are errors in the form',
}
)}
/>
<EuiSpacer size="m" />
</>
)}
<ConnectorForm
actionTypeModel={actionTypeModel}
connector={initialConnector}
Expand Down
38 changes: 36 additions & 2 deletions ...ui/public/application/sections/action_connector_form/edit_connector_flyout/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import React, { lazy } from 'react';

import { actionTypeRegistryMock } from '../../../action_type_registry.mock';
import userEvent from '@testing-library/user-event';
import { waitFor, act } from '@testing-library/react';
import { waitFor, act, screen } from '@testing-library/react';
import EditConnectorFlyout from '.';
import { ActionConnector, EditConnectorTabs, GenericValidationResult } from '../../../../types';
import { AppMockRenderer, createAppMockRenderer } from '../../test_utils';
Expand Down Expand Up @@ -415,7 +415,7 @@ describe('EditConnectorFlyout', () => {

describe('Submitting', () => {
it('updates the connector correctly', async () => {
const { getByTestId } = appMockRenderer.render(
const { getByTestId, queryByTestId } = appMockRenderer.render(
<EditConnectorFlyout
actionTypeRegistry={actionTypeRegistry}
onClose={onClose}
Expand Down Expand Up @@ -459,6 +459,7 @@ describe('EditConnectorFlyout', () => {
name: 'My test',
secrets: {},
});
expect(queryByTestId('connector-form-header-error-label')).not.toBeInTheDocument();
});

it('updates connector form field with latest value', async () => {
Expand Down Expand Up @@ -555,6 +556,39 @@ describe('EditConnectorFlyout', () => {
});
});

it('show error message in the form header', async () => {
appMockRenderer.render(
<EditConnectorFlyout
actionTypeRegistry={actionTypeRegistry}
onClose={onClose}
connector={connector}
onConnectorUpdated={onConnectorUpdated}
/>
);

expect(await screen.findByTestId('test-connector-text-field')).toBeInTheDocument();
await userEvent.clear(screen.getByTestId('nameInput'));
await userEvent.click(screen.getByTestId('edit-connector-flyout-save-btn'));
expect(await screen.findByTestId('connector-form-header-error-label')).toBeInTheDocument();
});

it('removes error message from the form header', async () => {
appMockRenderer.render(
<EditConnectorFlyout
actionTypeRegistry={actionTypeRegistry}
onClose={onClose}
connector={connector}
onConnectorUpdated={onConnectorUpdated}
/>
);

await userEvent.clear(screen.getByTestId('nameInput'));
await userEvent.type(screen.getByTestId('nameInput'), 'My new name');
await userEvent.type(screen.getByTestId('test-connector-secret-text-field'), 'password');
await userEvent.click(screen.getByTestId('edit-connector-flyout-save-btn'));
expect(screen.queryByTestId('connector-form-header-error-label')).not.toBeInTheDocument();
});

it('runs pre submit validator correctly', async () => {
const errorActionTypeModel = actionTypeRegistryMock.createMockActionTypeModel({
actionConnectorFields: lazy(() => import('../connector_error_mock')),
Expand Down
44 changes: 37 additions & 7 deletions ...ions_ui/public/application/sections/action_connector_form/edit_connector_flyout/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,14 @@
*/

import React, { memo, ReactNode, useCallback, useEffect, useRef, useState } from 'react';
import { EuiFlyout, EuiFlyoutBody, EuiButton, EuiConfirmModal } from '@elastic/eui';
import {
EuiFlyout,
EuiFlyoutBody,
EuiButton,
EuiConfirmModal,
EuiCallOut,
EuiSpacer,
} from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n-react';
import { i18n } from '@kbn/i18n';
import { ActionTypeExecutorResult, isActionTypeExecutorResult } from '@kbn/actions-plugin/common';
Expand Down Expand Up @@ -62,6 +69,7 @@ const EditConnectorFlyoutComponent: React.FC<EditConnectorFlyoutProps> = ({
const canSave = hasSaveActionsCapability(capabilities);
const { isLoading: isUpdatingConnector, updateConnector } = useUpdateConnector();
const { isLoading: isExecutingConnector, executeConnector } = useExecuteConnector();
const [showFormErrors, setShowFormErrors] = useState<boolean>(false);

const [preSubmitValidationErrorMessage, setPreSubmitValidationErrorMessage] =
useState<ReactNode>(null);
Expand Down Expand Up @@ -90,6 +98,7 @@ const EditConnectorFlyoutComponent: React.FC<EditConnectorFlyoutProps> = ({
if (nextPage === EditConnectorTabs.Configuration && testExecutionResult !== none) {
setTestExecutionResult(none);
}
setShowFormErrors(false);
setTab(nextPage);
},
[testExecutionResult, setTestExecutionResult]
Expand Down Expand Up @@ -146,6 +155,7 @@ const EditConnectorFlyoutComponent: React.FC<EditConnectorFlyoutProps> = ({

const onClickSave = useCallback(async () => {
setPreSubmitValidationErrorMessage(null);
setShowFormErrors(false);

const { isValid, data } = await submit();
if (!isMounted.current) {
Expand Down Expand Up @@ -194,6 +204,8 @@ const EditConnectorFlyoutComponent: React.FC<EditConnectorFlyoutProps> = ({
}

return updatedConnector;
} else {
setShowFormErrors(true);
}
}, [
onConnectorUpdated,
Expand All @@ -218,6 +230,23 @@ const EditConnectorFlyoutComponent: React.FC<EditConnectorFlyoutProps> = ({
<>
{isEdit && (
<>
{showFormErrors && (
<>
<EuiCallOut
size="s"
color="danger"
iconType="warning"
data-test-subj="connector-form-header-error-label"
title={i18n.translate(
'xpack.triggersActionsUI.sections.editConnectorForm.headerFormLabel',
{
defaultMessage: 'There are errors in the form',
}
)}
/>
<EuiSpacer size="m" />
</>
)}
<ConnectorForm
actionTypeModel={actionTypeModel}
connector={getConnectorWithoutSecrets(connector)}
Expand Down Expand Up @@ -265,17 +294,18 @@ const EditConnectorFlyoutComponent: React.FC<EditConnectorFlyoutProps> = ({
);
}, [
connector,
docLinks.links.alerting.preconfiguredConnectors,
actionTypeModel,
isEdit,
docLinks.links.alerting.preconfiguredConnectors,
hasErrors,
isFormModified,
isSaved,
isSaving,
showFormErrors,
onFormModifiedChange,
preSubmitValidationErrorMessage,
showButtons,
isSaved,
isSaving,
onClickSave,
onFormModifiedChange,
isFormModified,
hasErrors,
]);

const renderTestTab = useCallback(() => {
Expand Down
Loading