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.

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

[7.x] Fix UX in alerting UI forms when errors occur (#59444) #59853

Merged
merged 2 commits into from
Mar 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ describe('action_connector_form', () => {
actionTypeName={'my-action-type-name'}
connector={initialConnector}
dispatch={() => {}}
serverError={null}
errors={{ name: [] }}
actionTypeRegistry={deps.actionTypeRegistry}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ interface ActionConnectorProps {
connector: ActionConnector;
dispatch: React.Dispatch<ReducerAction>;
actionTypeName: string;
serverError: {
serverError?: {
body: { message: string; error: string };
} | null;
};
errors: IErrorObject;
actionTypeRegistry: TypeRegistry<ActionTypeModel>;
}
Expand Down Expand Up @@ -110,7 +110,7 @@ export const ActionConnectorForm = ({
const FieldsComponent = actionTypeRegistered.actionConnectorFields;

return (
<EuiForm isInvalid={serverError !== null} error={serverError?.body.message}>
<EuiForm isInvalid={!!serverError} error={serverError?.body.message}>
<EuiFormRow
id="actionName"
fullWidth
Expand All @@ -125,6 +125,7 @@ export const ActionConnectorForm = ({
>
<EuiFieldText
fullWidth
autoFocus={true}
isInvalid={errors.name.length > 0 && connector.name !== undefined}
name="name"
placeholder="Untitled"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ export const ConnectorAddFlyout = ({
setConnector(initialConnector);
}, [setAddFlyoutVisibility, initialConnector]);

const [serverError, setServerError] = useState<{
body: { message: string; error: string };
} | null>(null);
const canSave = hasSaveActionsCapability(capabilities);

if (!addFlyoutVisible) {
Expand Down Expand Up @@ -105,7 +102,6 @@ export const ConnectorAddFlyout = ({
actionTypeName={actionType.name}
connector={connector}
dispatch={dispatch}
serverError={serverError}
errors={errors}
actionTypeRegistry={actionTypeRegistry}
/>
Expand All @@ -131,7 +127,17 @@ export const ConnectorAddFlyout = ({
return savedConnector;
})
.catch(errorRes => {
setServerError(errorRes);
toastNotifications.addDanger(
i18n.translate(
'xpack.triggersActionsUI.sections.addConnectorForm.updateErrorNotificationText',
{
defaultMessage: 'Failed to create connector: {message}',
values: {
message: errorRes.body?.message ?? '',
},
}
)
);
return undefined;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,17 @@ export const ConnectorAddModal = ({
const setConnector = (value: any) => {
dispatch({ command: { type: 'setConnector' }, payload: { key: 'connector', value } });
};
const [serverError, setServerError] = useState<{
body: { message: string; error: string };
} | null>(null);
const [serverError, setServerError] = useState<
| {
body: { message: string; error: string };
}
| undefined
>(undefined);

const closeModal = useCallback(() => {
setAddModalVisibility(false);
setConnector(initialConnector);
setServerError(null);
setServerError(undefined);
}, [initialConnector, setAddModalVisibility]);

if (!addModalVisible) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@ export const ConnectorEditFlyout = ({
connector: { ...initialConnector, secrets: {} },
});
const [isSaving, setIsSaving] = useState<boolean>(false);
const [serverError, setServerError] = useState<{
body: { message: string; error: string };
} | null>(null);

if (!editFlyoutVisible) {
return null;
Expand Down Expand Up @@ -85,7 +82,17 @@ export const ConnectorEditFlyout = ({
return savedConnector;
})
.catch(errorRes => {
setServerError(errorRes);
toastNotifications.addDanger(
i18n.translate(
'xpack.triggersActionsUI.sections.editConnectorForm.updateErrorNotificationText',
{
defaultMessage: 'Failed to update connector: {message}',
values: {
message: errorRes.body?.message ?? '',
},
}
)
);
return undefined;
});

Expand Down Expand Up @@ -124,7 +131,6 @@ export const ConnectorEditFlyout = ({
<EuiFlyoutBody>
<ActionConnectorForm
connector={connector}
serverError={serverError}
errors={errors}
actionTypeName={connector.actionType}
dispatch={dispatch}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,8 @@ export const AlertAdd = ({
const closeFlyout = useCallback(() => {
setAddFlyoutVisibility(false);
setAlert(initialAlert);
setServerError(null);
}, [initialAlert, setAddFlyoutVisibility]);

const [serverError, setServerError] = useState<{
body: { message: string; error: string };
} | null>(null);

if (!addFlyoutVisible) {
return null;
}
Expand Down Expand Up @@ -110,20 +105,24 @@ export const AlertAdd = ({
async function onSaveAlert(): Promise<Alert | undefined> {
try {
const newAlert = await createAlert({ http, alert });
if (toastNotifications) {
toastNotifications.addSuccess(
i18n.translate('xpack.triggersActionsUI.sections.alertAdd.saveSuccessNotificationText', {
defaultMessage: "Saved '{alertName}'",
values: {
alertName: newAlert.name,
},
})
);
}
toastNotifications.addSuccess(
i18n.translate('xpack.triggersActionsUI.sections.alertAdd.saveSuccessNotificationText', {
defaultMessage: "Saved '{alertName}'",
values: {
alertName: newAlert.name,
},
})
);
return newAlert;
} catch (errorRes) {
setServerError(errorRes);
return undefined;
toastNotifications.addDanger(
i18n.translate('xpack.triggersActionsUI.sections.alertAdd.saveErrorNotificationText', {
defaultMessage: 'Failed to save alert: {message}',
values: {
message: errorRes.body?.message ?? '',
},
})
);
}
}

Expand Down Expand Up @@ -161,7 +160,6 @@ export const AlertAdd = ({
alert={alert}
dispatch={dispatch}
errors={errors}
serverError={serverError}
canChangeTrigger={canChangeTrigger}
/>
</EuiFlyoutBody>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@ const alertTypeRegistry = alertTypeRegistryMock.create();
describe('alert_edit', () => {
let deps: any;
let wrapper: ReactWrapper<any>;
let mockedCoreSetup: ReturnType<typeof coreMock.createSetup>;

beforeEach(() => {
mockedCoreSetup = coreMock.createSetup();
});

async function setup() {
const mockes = coreMock.createSetup();
deps = {
toastNotifications: mockes.notifications.toasts,
http: mockes.http,
uiSettings: mockes.uiSettings,
toastNotifications: mockedCoreSetup.notifications.toasts,
http: mockedCoreSetup.http,
uiSettings: mockedCoreSetup.uiSettings,
actionTypeRegistry: actionTypeRegistry as any,
alertTypeRegistry: alertTypeRegistry as any,
};
Expand Down Expand Up @@ -129,4 +133,22 @@ describe('alert_edit', () => {
expect(wrapper.find('[data-test-subj="editAlertFlyoutTitle"]').exists()).toBeTruthy();
expect(wrapper.find('[data-test-subj="saveEditedAlertButton"]').exists()).toBeTruthy();
});

it('displays a toast message on save for server errors', async () => {
mockedCoreSetup.http.get.mockResolvedValue([]);
await setup();
const err = new Error() as any;
err.body = {};
err.body.message = 'Fail message';
mockedCoreSetup.http.put.mockRejectedValue(err);
await act(async () => {
wrapper
.find('[data-test-subj="saveEditedAlertButton"]')
.first()
.simulate('click');
});
expect(mockedCoreSetup.notifications.toasts.addDanger).toHaveBeenCalledWith(
'Failed to save alert: Fail message'
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,8 @@ export const AlertEdit = ({

const closeFlyout = useCallback(() => {
setEditFlyoutVisibility(false);
setServerError(null);
}, [setEditFlyoutVisibility]);

const [serverError, setServerError] = useState<{
body: { message: string; error: string };
} | null>(null);

if (!editFlyoutVisible) {
return null;
}
Expand Down Expand Up @@ -103,7 +98,16 @@ export const AlertEdit = ({
}
return newAlert;
} catch (errorRes) {
setServerError(errorRes);
if (toastNotifications) {
toastNotifications.addDanger(
i18n.translate('xpack.triggersActionsUI.sections.alertEdit.saveErrorNotificationText', {
defaultMessage: 'Failed to save alert: {message}',
values: {
message: errorRes.body?.message ?? '',
},
})
);
}
}
}

Expand Down Expand Up @@ -137,13 +141,7 @@ export const AlertEdit = ({
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<AlertForm
alert={alert}
dispatch={dispatch}
errors={errors}
serverError={serverError}
canChangeTrigger={false}
/>
<AlertForm alert={alert} dispatch={dispatch} errors={errors} canChangeTrigger={false} />
</EuiFlyoutBody>
<EuiFlyoutFooter>
<EuiFlexGroup justifyContent="spaceBetween">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,7 @@ describe('alert_form', () => {
uiSettings: deps!.uiSettings,
}}
>
<AlertForm
alert={initialAlert}
dispatch={() => {}}
errors={{ name: [] }}
serverError={null}
/>
<AlertForm alert={initialAlert} dispatch={() => {}} errors={{ name: [] }} />
</AlertsContextProvider>
);

Expand Down Expand Up @@ -170,12 +165,7 @@ describe('alert_form', () => {
uiSettings: deps!.uiSettings,
}}
>
<AlertForm
alert={initialAlert}
dispatch={() => {}}
errors={{ name: [] }}
serverError={null}
/>
<AlertForm alert={initialAlert} dispatch={() => {}} errors={{ name: [] }} />
</AlertsContextProvider>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,10 @@ interface AlertFormProps {
alert: Alert;
dispatch: React.Dispatch<AlertReducerAction>;
errors: IErrorObject;
serverError: {
body: { message: string; error: string };
} | null;
canChangeTrigger?: boolean; // to hide Change trigger button
}

export const AlertForm = ({
alert,
canChangeTrigger = true,
dispatch,
errors,
serverError,
}: AlertFormProps) => {
export const AlertForm = ({ alert, canChangeTrigger = true, dispatch, errors }: AlertFormProps) => {
const alertsContext = useAlertsContext();
const { http, toastNotifications, alertTypeRegistry, actionTypeRegistry } = alertsContext;

Expand Down Expand Up @@ -276,7 +267,7 @@ export const AlertForm = ({
);

return (
<EuiForm isInvalid={serverError !== null} error={serverError?.body.message}>
<EuiForm>
<EuiFlexGrid columns={2}>
<EuiFlexItem>
<EuiFormRow
Expand All @@ -293,6 +284,7 @@ export const AlertForm = ({
>
<EuiFieldText
fullWidth
autoFocus={true}
isInvalid={errors.name.length > 0 && alert.name !== undefined}
compressed
name="name"
Expand Down