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

Jump to bottom

[Alerting UI] Alert and Connector flyouts Save and Save&Test buttons should be active by default. #86708

Merged
merged 16 commits into from
Jan 5, 2021
Merged

[Alerting UI] Alert and Connector flyouts Save and Save&Test buttons should be active by default. #86708

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7a02b30
Alert and Connector flyouts Save and Save&Test buttons should be acti…
YulNaumenko Dec 21, 2020
0e1dcdf
fixed typechecks
YulNaumenko Dec 22, 2020
0dceb43
Merge remote-tracking branch upstream/master
YulNaumenko Dec 22, 2020
b3e89ff
fixed typechecks
YulNaumenko Dec 22, 2020
7a280f4
refactored repeted code
YulNaumenko Dec 22, 2020
aa4b8dd
fixed typechecks
YulNaumenko Dec 22, 2020
46cc2c8
fixed typechecks
YulNaumenko Dec 22, 2020
956393b
fixed typechecks
YulNaumenko Dec 23, 2020
e01d1e6
fixed due to comments
YulNaumenko Dec 23, 2020
7846ebf
Merge remote-tracking branch upstream/master
YulNaumenko Jan 4, 2021
fddf0fe
fixed failing tests
YulNaumenko Jan 4, 2021
64526de
fixed due to comments
YulNaumenko Jan 5, 2021
1bec31d
Merge remote-tracking branch upstream/master
YulNaumenko Jan 5, 2021
5dacd3f
fixed due to comments
YulNaumenko Jan 5, 2021
2eb2b3e
fixed due to comments
YulNaumenko Jan 5, 2021
546f61d
fixed typescript checks
YulNaumenko Jan 5, 2021
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
16 changes: 13 additions & 3 deletions x-pack/plugins/monitoring/public/alerts/alert_form.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ import { act } from 'react-dom/test-utils';
import { coreMock } from 'src/core/public/mocks';
import { actionTypeRegistryMock } from '../../../triggers_actions_ui/public/application/action_type_registry.mock';
import { alertTypeRegistryMock } from '../../../triggers_actions_ui/public/application/alert_type_registry.mock';
import { ValidationResult, Alert } from '../../../triggers_actions_ui/public/types';
import {
ValidationResult,
Alert,
ConnectorValidationResult,
GenericValidationResult,
} from '../../../triggers_actions_ui/public/types';
import { AlertForm } from '../../../triggers_actions_ui/public/application/sections/alert_form/alert_form';
import ActionForm from '../../../triggers_actions_ui/public/application/sections/action_connector_form/action_form';
import { Legacy } from '../legacy_shims';
Expand Down Expand Up @@ -88,8 +93,13 @@ describe('alert_form', () => {
id: 'alert-action-type',
iconClass: '',
selectMessage: '',
validateConnector: validationMethod,
validateParams: validationMethod,
validateConnector: (): ConnectorValidationResult<unknown, unknown> => {
return {};
},
validateParams: (): GenericValidationResult<unknown> => {
const validationResult = { errors: {} };
return validationResult;
},
actionConnectorFields: null,
actionParamsFields: mockedActionParamsFields,
};
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security_solution/public/cases/components/connectors/case/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export function getActionType(): ActionTypeModel {
iconClass: 'securityAnalyticsApp',
selectMessage: i18n.CASE_CONNECTOR_DESC,
actionTypeTitle: i18n.CASE_CONNECTOR_TITLE,
validateConnector: () => ({ errors: {} }),
validateConnector: () => ({ config: { errors: {} }, secrets: { errors: {} } }),
validateParams,
actionConnectorFields: null,
actionParamsFields: lazy(() => import('./fields')),
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/stack_alerts/public/alert_types/threshold/expression.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ export const IndexThresholdAlertTypeExpression: React.FunctionComponent<
fullWidth
name="thresholdTimeField"
data-test-subj="thresholdAlertTimeFieldSelect"
value={timeField}
value={timeField || ''}
onChange={(e) => {
setAlertParams('timeField', e.target.value);
}}
Expand Down
90 changes: 60 additions & 30 deletions ...iggers_actions_ui/public/application/components/builtin_action_types/email/email.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,18 @@ describe('connector validation', () => {
} as EmailActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
from: [],
port: [],
host: [],
user: [],
password: [],
config: {
errors: {
from: [],
port: [],
host: [],
},
},
secrets: {
errors: {
user: [],
password: [],
},
},
});
});
Expand All @@ -78,12 +84,18 @@ describe('connector validation', () => {
} as EmailActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
from: [],
port: [],
host: [],
user: [],
password: [],
config: {
errors: {
from: [],
port: [],
host: [],
},
},
secrets: {
errors: {
user: [],
password: [],
},
},
});
});
Expand All @@ -103,12 +115,18 @@ describe('connector validation', () => {
} as EmailActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
from: [],
port: ['Port is required.'],
host: ['Host is required.'],
user: [],
password: [],
config: {
errors: {
from: [],
port: ['Port is required.'],
host: ['Host is required.'],
},
},
secrets: {
errors: {
user: [],
password: [],
},
},
});
});
Expand All @@ -132,12 +150,18 @@ describe('connector validation', () => {
} as EmailActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
from: [],
port: [],
host: [],
user: [],
password: ['Password is required when username is used.'],
config: {
errors: {
from: [],
port: [],
host: [],
},
},
secrets: {
errors: {
user: [],
password: ['Password is required when username is used.'],
},
},
});
});
Expand All @@ -161,12 +185,18 @@ describe('connector validation', () => {
} as EmailActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
from: [],
port: [],
host: [],
user: ['Username is required when password is used.'],
password: [],
config: {
errors: {
from: [],
port: [],
host: [],
},
},
secrets: {
errors: {
user: ['Username is required when password is used.'],
password: [],
},
},
});
});
Expand Down
44 changes: 28 additions & 16 deletions ...ns/triggers_actions_ui/public/application/components/builtin_action_types/email/email.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
*/
import { lazy } from 'react';
import { i18n } from '@kbn/i18n';
import { ActionTypeModel, ValidationResult } from '../../../../types';
import {
ActionTypeModel,
ConnectorValidationResult,
GenericValidationResult,
} from '../../../../types';
import { EmailActionParams, EmailConfig, EmailSecrets, EmailActionConnector } from '../types';

export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, EmailActionParams> {
Expand All @@ -25,18 +29,25 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
defaultMessage: 'Send to email',
}
),
validateConnector: (action: EmailActionConnector): ValidationResult => {
const validationResult = { errors: {} };
const errors = {
validateConnector: (
action: EmailActionConnector
): ConnectorValidationResult<Omit<EmailConfig, 'secure' | 'hasAuth'>, EmailSecrets> => {
const configErrors = {
from: new Array<string>(),
port: new Array<string>(),
host: new Array<string>(),
};
const secretsErrors = {
user: new Array<string>(),
password: new Array<string>(),
};
validationResult.errors = errors;

const validationResult = {
config: { errors: configErrors },
secrets: { errors: secretsErrors },
};
if (!action.config.from) {
errors.from.push(
configErrors.from.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.requiredFromText',
{
Expand All @@ -46,7 +57,7 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
);
}
if (action.config.from && !action.config.from.trim().match(mailformat)) {
errors.from.push(
configErrors.from.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.formatFromText',
{
Expand All @@ -56,7 +67,7 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
);
}
if (!action.config.port) {
errors.port.push(
configErrors.port.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.requiredPortText',
{
Expand All @@ -66,7 +77,7 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
);
}
if (!action.config.host) {
errors.host.push(
configErrors.host.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.requiredHostText',
{
Expand All @@ -76,7 +87,7 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
);
}
if (action.config.hasAuth && !action.secrets.user && !action.secrets.password) {
errors.user.push(
secretsErrors.user.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.requiredAuthUserNameText',
{
Expand All @@ -86,7 +97,7 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
);
}
if (action.config.hasAuth && !action.secrets.user && !action.secrets.password) {
errors.password.push(
secretsErrors.password.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.requiredAuthPasswordText',
{
Expand All @@ -96,7 +107,7 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
);
}
if (action.secrets.user && !action.secrets.password) {
errors.password.push(
secretsErrors.password.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.requiredPasswordText',
{
Expand All @@ -106,7 +117,7 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
);
}
if (!action.secrets.user && action.secrets.password) {
errors.user.push(
secretsErrors.user.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.error.requiredUserText',
{
Expand All @@ -117,16 +128,17 @@ export function getActionType(): ActionTypeModel<EmailConfig, EmailSecrets, Emai
}
return validationResult;
},
validateParams: (actionParams: EmailActionParams): ValidationResult => {
const validationResult = { errors: {} };
validateParams: (
actionParams: EmailActionParams
): GenericValidationResult<EmailActionParams> => {
const errors = {
to: new Array<string>(),
cc: new Array<string>(),
bcc: new Array<string>(),
message: new Array<string>(),
subject: new Array<string>(),
};
validationResult.errors = errors;
const validationResult = { errors };
if (
(!(actionParams.to instanceof Array) || actionParams.to.length === 0) &&
(!(actionParams.cc instanceof Array) || actionParams.cc.length === 0) &&
Expand Down
2 changes: 1 addition & 1 deletion ...s_actions_ui/public/application/components/builtin_action_types/email/email_connector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export const EmailActionConnectorFields: React.FunctionComponent<
}
)}
disabled={readOnly}
checked={hasAuth}
checked={hasAuth || false}
Copy link
Contributor

Choose a reason for hiding this comment

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

hasAuth is boolean, so I can't figure out why || false is needed.... 🤔
If it can be undefined then something seems to be wrong in the typing 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current implementation, the initial alert object has all user input fields as undefined to be able to not trigger validation on the form opening (each input field has a check on undefined to not set a validation error in this case). When onblur event on the field or click save we are triggering invalidating the values - basically setting it to null instead of undefined.
Do you think we should consider another approach for implementing UI inputs validation triggering?

onChange={(e) => {
editActionConfig('hasAuth', e.target.checked);
if (!e.target.checked) {
Expand Down
18 changes: 14 additions & 4 deletions ..._actions_ui/public/application/components/builtin_action_types/es_index/es_index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,13 @@ describe('index connector validation', () => {
} as EsIndexActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
index: [],
config: {
errors: {
index: [],
},
},
secrets: {
errors: {},
},
});
});
Expand All @@ -62,8 +67,13 @@ describe('index connector validation with minimal config', () => {
} as EsIndexActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
index: [],
config: {
errors: {
index: [],
},
},
secrets: {
errors: {},
},
});
});
Expand Down
24 changes: 15 additions & 9 deletions ...ggers_actions_ui/public/application/components/builtin_action_types/es_index/es_index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
*/
import { lazy } from 'react';
import { i18n } from '@kbn/i18n';
import { ActionTypeModel, ValidationResult } from '../../../../types';
import {
ActionTypeModel,
GenericValidationResult,
ConnectorValidationResult,
} from '../../../../types';
import { EsIndexActionConnector, EsIndexConfig, IndexActionParams } from '../types';

export function getActionType(): ActionTypeModel<EsIndexConfig, unknown, IndexActionParams> {
Expand All @@ -24,14 +28,15 @@ export function getActionType(): ActionTypeModel<EsIndexConfig, unknown, IndexAc
defaultMessage: 'Index data',
}
),
validateConnector: (action: EsIndexActionConnector): ValidationResult => {
const validationResult = { errors: {} };
const errors = {
validateConnector: (
action: EsIndexActionConnector
): ConnectorValidationResult<Pick<EsIndexConfig, 'index'>, unknown> => {
const configErrors = {
index: new Array<string>(),
};
validationResult.errors = errors;
const validationResult = { config: { errors: configErrors }, secrets: { errors: {} } };
if (!action.config.index) {
errors.index.push(
configErrors.index.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.indexAction.error.requiredIndexText',
{
Expand All @@ -44,12 +49,13 @@ export function getActionType(): ActionTypeModel<EsIndexConfig, unknown, IndexAc
},
actionConnectorFields: lazy(() => import('./es_index_connector')),
actionParamsFields: lazy(() => import('./es_index_params')),
validateParams: (actionParams: IndexActionParams): ValidationResult => {
const validationResult = { errors: {} };
validateParams: (
actionParams: IndexActionParams
): GenericValidationResult<IndexActionParams> => {
const errors = {
documents: new Array<string>(),
};
validationResult.errors = errors;
const validationResult = { errors };
if (!actionParams.documents?.length || Object.keys(actionParams.documents[0]).length === 0) {
errors.documents.push(
i18n.translate(
Expand Down
Loading