From 576fc51216ada591a8cfb1f02e2d5fbd1397869f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Fri, 5 Oct 2018 18:44:46 +0200 Subject: [PATCH 1/5] fix(watch): Add WatchErrors to capture invalid watches Instead of immediately throwing an error if a watch Action is invalid (email for example), we can now pass an option object to the fromUpstreamJson() method and receive back any error in the Watch. The JSON object has now a "watchErrors" object that will be used in the UI to display an error message to the user. fix(watch): Add models in public code to handle watch errors fix(watcher): Add temp styling for error messages fix(watcher): Set actionErrors to null instead of empty object when no error test(watcher): Fix broken tests after adding watchErrors prop on the Watch test(watcher): Add test for WatchError class fix(watcher): Add Watch & Action state for CONFIG_ERROR fix(watcher): move inline style to its scss file fix(watcher): Add Error display modal to show configuration errors fix(watcher): Add accesibility to errors display modal fix(Watcher): Remove table col width breaking IE layout fix(watcher): Add i18n support for errors modal title fix(Watcher): Do not throw Action error on GET calls test(Watcher): Check that correct action type is created from upstreamJson fix(Watcher): Add action instances to payload before saving Watch Angular Json editor was not sending the actions instances so no validation in Node.js was done when creating the instances on the server. Now it correctly create the action instances *before* the POST request to save/edit the Watch test(Watcher): Migrate Action server test to Jest fix(Watcher): Add warning dialog when saving a Watch with a missing 'to' field --- .../watcher/common/constants/action_states.js | 3 + .../watcher/common/constants/error_codes.js | 11 ++ .../plugins/watcher/common/constants/index.js | 1 + .../watcher/common/constants/watch_states.js | 1 + .../action_state_icon/action_state_icon.html | 2 +- .../errors_display_modal.html | 18 +++ .../errors_display_modal.js | 18 +++ .../components/errors_display_modal/index.js | 7 + .../watch_state_icon/watch_state_icon.html | 2 +- .../public/models/action/email_action.js | 5 +- .../public/models/action/logging_action.js | 6 +- .../public/models/action/slack_action.js | 28 +++- .../watcher/public/models/watch/base_watch.js | 49 +++++++ .../public/models/watch_errors/index.js | 7 + .../models/watch_errors/watch_errors.js | 17 +++ .../action_status_table.html | 11 ++ .../action_status_table.js | 16 ++- .../components/watch_detail/watch_detail.html | 2 + .../components/watch_detail/watch_detail.js | 59 ++++++-- .../json_watch_edit/json_watch_edit.js | 78 +++++++++- .../server/models/action/__tests__/action.js | 108 -------------- .../watcher/server/models/action/action.js | 44 +++++- .../server/models/action/action.test.js | 136 ++++++++++++++++++ .../server/models/action/base_action.js | 3 +- .../server/models/action/email_action.js | 76 ++++++---- .../server/models/action/logging_action.js | 50 ++++--- .../server/models/action/slack_action.js | 69 +++++---- .../server/models/action/unknown_action.js | 33 +++-- .../action_status/__tests__/action_status.js | 12 +- .../models/action_status/action_status.js | 5 + .../models/watch/__tests__/base_watch.js | 30 +++- .../watcher/server/models/watch/base_watch.js | 36 ++++- .../watcher/server/models/watch/json_watch.js | 4 +- .../watcher/server/models/watch/watch.js | 4 +- .../server/models/watch_errors/index.js | 7 + .../models/watch_errors/watch_errors.js | 25 ++++ .../models/watch_errors/watch_errors.test.js | 34 +++++ .../__tests__/watch_history_item.js | 3 +- .../watch_status/__tests__/watch_status.js | 11 ++ .../models/watch_status/watch_status.js | 11 +- .../routes/api/watch/register_load_route.js | 14 +- .../routes/api/watches/register_list_route.js | 17 ++- 42 files changed, 825 insertions(+), 248 deletions(-) create mode 100644 x-pack/plugins/watcher/common/constants/error_codes.js create mode 100644 x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.html create mode 100644 x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.js create mode 100644 x-pack/plugins/watcher/public/components/errors_display_modal/index.js create mode 100644 x-pack/plugins/watcher/public/models/watch_errors/index.js create mode 100644 x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js delete mode 100644 x-pack/plugins/watcher/server/models/action/__tests__/action.js create mode 100644 x-pack/plugins/watcher/server/models/action/action.test.js create mode 100644 x-pack/plugins/watcher/server/models/watch_errors/index.js create mode 100644 x-pack/plugins/watcher/server/models/watch_errors/watch_errors.js create mode 100644 x-pack/plugins/watcher/server/models/watch_errors/watch_errors.test.js diff --git a/x-pack/plugins/watcher/common/constants/action_states.js b/x-pack/plugins/watcher/common/constants/action_states.js index 7b8ac59ff336e..0930ad6949142 100644 --- a/x-pack/plugins/watcher/common/constants/action_states.js +++ b/x-pack/plugins/watcher/common/constants/action_states.js @@ -33,4 +33,7 @@ export const ACTION_STATES = { defaultMessage: 'Error' }), + // Action has a configuration error + CONFIG_ERROR: 'Config Error' + }; diff --git a/x-pack/plugins/watcher/common/constants/error_codes.js b/x-pack/plugins/watcher/common/constants/error_codes.js new file mode 100644 index 0000000000000..2fa875549358f --- /dev/null +++ b/x-pack/plugins/watcher/common/constants/error_codes.js @@ -0,0 +1,11 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +export const ERROR_CODES = { + + // Property missing on object + ERR_PROP_MISSING: 'ERR_PROP_MISSING', +}; diff --git a/x-pack/plugins/watcher/common/constants/index.js b/x-pack/plugins/watcher/common/constants/index.js index 3324b9f3cf427..4260688c6eb45 100644 --- a/x-pack/plugins/watcher/common/constants/index.js +++ b/x-pack/plugins/watcher/common/constants/index.js @@ -22,3 +22,4 @@ export { WATCH_STATE_COMMENTS } from './watch_state_comments'; export { WATCH_HISTORY } from './watch_history'; export { WATCH_STATES } from './watch_states'; export { WATCH_TYPES } from './watch_types'; +export { ERROR_CODES } from './error_codes'; diff --git a/x-pack/plugins/watcher/common/constants/watch_states.js b/x-pack/plugins/watcher/common/constants/watch_states.js index e9916aceec1c1..f223e1bd8564e 100644 --- a/x-pack/plugins/watcher/common/constants/watch_states.js +++ b/x-pack/plugins/watcher/common/constants/watch_states.js @@ -24,4 +24,5 @@ export const WATCH_STATES = { defaultMessage: 'Error!' }), + CONFIG_ERROR: 'Config Error!', }; diff --git a/x-pack/plugins/watcher/public/components/action_state_icon/action_state_icon.html b/x-pack/plugins/watcher/public/components/action_state_icon/action_state_icon.html index 05fbcededf4a7..67c8c703aecf0 100644 --- a/x-pack/plugins/watcher/public/components/action_state_icon/action_state_icon.html +++ b/x-pack/plugins/watcher/public/components/action_state_icon/action_state_icon.html @@ -17,6 +17,6 @@ >
diff --git a/x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.html b/x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.html new file mode 100644 index 0000000000000..3d4f25e7abc7d --- /dev/null +++ b/x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.html @@ -0,0 +1,18 @@ +
+
+

{{ vm.title }}

+
+
+
    +
  • {{ error.message }}
    • +
+
+
+ +
+
\ No newline at end of file diff --git a/x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.js b/x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.js new file mode 100644 index 0000000000000..1e17774193ec4 --- /dev/null +++ b/x-pack/plugins/watcher/public/components/errors_display_modal/errors_display_modal.js @@ -0,0 +1,18 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { uiModules } from 'ui/modules'; + +const app = uiModules.get('xpack/watcher'); + +app.controller('WatcherErrorsDisplayController', function WatcherErrorsDisplayController($scope, $modalInstance, params) { + this.title = params.title; + this.errors = params.errors; + + this.close = function close() { + $modalInstance.close(); + }; +}); diff --git a/x-pack/plugins/watcher/public/components/errors_display_modal/index.js b/x-pack/plugins/watcher/public/components/errors_display_modal/index.js new file mode 100644 index 0000000000000..07daabc6bf185 --- /dev/null +++ b/x-pack/plugins/watcher/public/components/errors_display_modal/index.js @@ -0,0 +1,7 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import './errors_display_modal'; diff --git a/x-pack/plugins/watcher/public/components/watch_state_icon/watch_state_icon.html b/x-pack/plugins/watcher/public/components/watch_state_icon/watch_state_icon.html index 5cb1b6fd492a8..0f50077157811 100644 --- a/x-pack/plugins/watcher/public/components/watch_state_icon/watch_state_icon.html +++ b/x-pack/plugins/watcher/public/components/watch_state_icon/watch_state_icon.html @@ -13,6 +13,6 @@ >
diff --git a/x-pack/plugins/watcher/public/models/action/email_action.js b/x-pack/plugins/watcher/public/models/action/email_action.js index fbcfdaa4123e8..31fe6a9405e0b 100644 --- a/x-pack/plugins/watcher/public/models/action/email_action.js +++ b/x-pack/plugins/watcher/public/models/action/email_action.js @@ -24,7 +24,10 @@ export class EmailAction extends BaseAction { Object.assign(result, { to: this.to, subject: this.subject, - body: this.body + body: this.body, + email: { + to: this.to.length ? this.to : undefined, + } }); return result; diff --git a/x-pack/plugins/watcher/public/models/action/logging_action.js b/x-pack/plugins/watcher/public/models/action/logging_action.js index 41fd86d668979..603784053e717 100644 --- a/x-pack/plugins/watcher/public/models/action/logging_action.js +++ b/x-pack/plugins/watcher/public/models/action/logging_action.js @@ -17,9 +17,13 @@ export class LoggingAction extends BaseAction { get upstreamJson() { const result = super.upstreamJson; + const text = !!this.text.trim() ? this.text : undefined; Object.assign(result, { - text: this.text + text, + logging: { + text, + } }); return result; diff --git a/x-pack/plugins/watcher/public/models/action/slack_action.js b/x-pack/plugins/watcher/public/models/action/slack_action.js index 6f8146c24d354..c7552628dba3d 100644 --- a/x-pack/plugins/watcher/public/models/action/slack_action.js +++ b/x-pack/plugins/watcher/public/models/action/slack_action.js @@ -17,12 +17,36 @@ export class SlackAction extends BaseAction { this.text = props.text; } + validate() { + const errors = []; + + if (!this.to.length) { + errors.push({ + message: i18n.translate('xpack.watcher.sections.watchEdit.json.warningPossibleInvalidSlackAction.description', { + defaultMessage: `You are saving a watch with a "Slack" Action but you haven't defined a "to" field. + Unless you have specified a Slack message_default "to" property in your Elasticsearch settings, + this will result in an invalid watch.` + }) + }); + } + + return { errors: errors.length ? errors : null }; + } + get upstreamJson() { const result = super.upstreamJson; - + const message = this.text || this.to.length + ? { + text: this.text, + to: this.to.length ? this.to : undefined + } + : undefined; Object.assign(result, { to: this.to, - text: this.text + text: this.text, + slack: { + message + } }); return result; diff --git a/x-pack/plugins/watcher/public/models/watch/base_watch.js b/x-pack/plugins/watcher/public/models/watch/base_watch.js index 27bf94d14f9c8..ca8149f58e73b 100644 --- a/x-pack/plugins/watcher/public/models/watch/base_watch.js +++ b/x-pack/plugins/watcher/public/models/watch/base_watch.js @@ -8,6 +8,7 @@ import { getSearchValue } from 'plugins/watcher/lib/get_search_value'; import { get, isEqual, remove, map, merge } from 'lodash'; import { Action } from '../action'; import { WatchStatus } from '../watch_status'; +import { WatchErrors } from '../watch_errors'; import { createActionId } from './lib/create_action_id'; import { checkActionIdCollision } from './lib/check_action_id_collision'; import { i18n } from '@kbn/i18n'; @@ -31,6 +32,7 @@ export class BaseWatch { this.name = get(props, 'name', ''); this.isSystemWatch = Boolean(get(props, 'isSystemWatch')); this.watchStatus = WatchStatus.fromUpstreamJson(get(props, 'watchStatus')); + this.watchErrors = WatchErrors.fromUpstreamJson(get(props, 'watchErrors')); const actions = get(props, 'actions', []); this.actions = actions.map(Action.fromUpstreamJson); @@ -76,6 +78,10 @@ export class BaseWatch { remove(this.actions, action); } + resetActions = () => { + this.actions = []; + }; + get displayName() { if (this.isNew) { return i18n.translate('xpack.watcher.models.baseWatch.displayName', { @@ -130,6 +136,49 @@ export class BaseWatch { return isEqual(cleanWatch, cleanOtherWatch); } + /** + * Client validation of the Watch. + * Currently we are *only* validating the Watch "Actions" + */ + validate() { + + // Get the errors from each watch action + const actionsErrors = this.actions.reduce((actionsErrors, action) => { + if (action.validate) { + const { errors } = action.validate(); + if (!errors) { + return actionsErrors; + } + return [...actionsErrors, ...errors]; + } + return actionsErrors; + }, []); + + if (!actionsErrors.length) { + return { warning: null }; + } + + // Concatenate their message + const errorActionsFragment = actionsErrors.reduce((message, error) => ( + !!message + ? `${message}, ${error.message}` + : error.message + ), ''); + + // We are not doing any *blocking* validation in the client, + // so for now we return the errors as a warning + return { + warning: { + message: i18n.translate('xpack.watcher.models.baseWatch.invalidWatchWarningMessageText', { + defaultMessage: 'Warning: {errorActionsFragment} Are you sure you want to save the watch in its current state?', + values: { + errorActionsFragment, + } + }) + } + }; + } + static typeName = i18n.translate('xpack.watcher.models.baseWatch.typeName', { defaultMessage: 'Watch', }); diff --git a/x-pack/plugins/watcher/public/models/watch_errors/index.js b/x-pack/plugins/watcher/public/models/watch_errors/index.js new file mode 100644 index 0000000000000..dec5e51d74e31 --- /dev/null +++ b/x-pack/plugins/watcher/public/models/watch_errors/index.js @@ -0,0 +1,7 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +export { WatchErrors } from './watch_errors'; diff --git a/x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js b/x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js new file mode 100644 index 0000000000000..db8b3ab7f65fb --- /dev/null +++ b/x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js @@ -0,0 +1,17 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { get } from 'lodash'; + +export class WatchErrors { + constructor(props = {}) { + this.actionErrors = get(props, 'actions'); + } + + static fromUpstreamJson(upstreamWatchStatus) { + return new WatchErrors(upstreamWatchStatus); + } +} diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html index 1e89ebd099807..2390582a11c5b 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html @@ -21,6 +21,9 @@ {{ 'xpack.watcher.sections.watchDetail.actionStatusTable.stateColumnLabel' | i18n: { defaultMessage: 'State' } }} + + Errors + @@ -39,6 +42,14 @@ {{ actionStatus.state }} + +
+ {{actionStatusTable.getLabelErrors(actionStatus.id)}} +
+
diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js index 1994c88737876..e986c47fb78c5 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js @@ -16,13 +16,27 @@ app.directive('actionStatusTable', function () { template: template, scope: { actionStatuses: '=', + actionErrors: '=', sortField: '=', sortReverse: '=', onSortChange: '=', onActionAcknowledge: '=', + showErrors: '=' }, bindToController: true, controllerAs: 'actionStatusTable', - controller: class ActionStatusTableController {} + controller: class ActionStatusTableController { + getLabelErrors(actionId) { + const errors = this.actionErrors[actionId]; + const total = errors.length; + + let label = `${total} error`; + if (total > 1) { + label += 's'; + } + + return label; + } + } }; }); diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.html b/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.html index 5e4397a825c63..fa46a0b12097a 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.html +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.html @@ -53,10 +53,12 @@ {{ 'xpack.watcher.sections.watchDetail.noActionsFoundText' | i18n: { defaultMessage: 'No actions found.' } }} diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js b/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js index 41cb264c1d916..30599489c3975 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js @@ -14,8 +14,10 @@ import 'ui/table_info'; import 'plugins/watcher/components/tool_bar_selected_count'; import 'plugins/watcher/services/watch'; import 'plugins/watcher/services/license'; +import 'plugins/watcher/components/errors_display_modal'; import template from './watch_detail.html'; +import errorsDisplayTemplate from 'plugins/watcher/components/errors_display_modal/errors_display_modal.html'; import '../watch_history'; import '../action_status_table'; import { REFRESH_INTERVALS } from 'plugins/watcher/../common/constants'; @@ -33,6 +35,7 @@ app.directive('watchDetail', function ($injector, i18n) { const $filter = $injector.get('$filter'); const orderBy = $filter('orderBy'); + const $modal = $injector.get('$modal'); moment.tz.setDefault(config.get('dateFormat:tz')); @@ -54,38 +57,39 @@ app.directive('watchDetail', function ($injector, i18n) { this.actionStatusTableSortField = 'id'; this.actionStatusTableSortReverse = false; + this.actionErrors = (this.watch.watchErrors && this.watch.watchErrors.actionErrors) || null; - this.omitBreadcrumbPages = [ - 'watch', - this.watch.id - ]; + this.omitBreadcrumbPages = ['watch', this.watch.id]; this.breadcrumb = this.watch.displayName; // Reload watch history periodically - const refreshInterval = $interval(() => this.loadWatchHistory(), REFRESH_INTERVALS.WATCH_HISTORY); + const refreshInterval = $interval( + () => this.loadWatchHistory(), + REFRESH_INTERVALS.WATCH_HISTORY + ); $scope.$on('$destroy', () => $interval.cancel(refreshInterval)); // react to data and UI changes - $scope.$watchMulti([ - 'watchDetail.actionStatusTableSortField', - 'watchDetail.actionStatusTableSortReverse', - ], this.applySortToActionStatusTable); + $scope.$watchMulti( + ['watchDetail.actionStatusTableSortField', 'watchDetail.actionStatusTableSortReverse'], + this.applySortToActionStatusTable + ); } loadWatchHistory = () => { - return watchService.loadWatchHistory(this.watch.id, this.historyRange) + return watchService + .loadWatchHistory(this.watch.id, this.historyRange) .then(watchHistoryItems => { this.isHistoryLoading = false; this.watchHistoryItems = watchHistoryItems; }) .catch(err => { - return licenseService.checkValidity() - .then(() => toastNotifications.addDanger(err)); + return licenseService.checkValidity().then(() => toastNotifications.addDanger(err)); }); - } + }; // update the watch history items when the time range changes - onHistoryRangeChange = (range) => { + onHistoryRangeChange = range => { this.historyRange = range; this.isHistoryLoading = true; return this.loadWatchHistory(); @@ -124,6 +128,33 @@ app.directive('watchDetail', function ($injector, i18n) { }); } + showErrors = (actionId, errors) => { + const errorsModal = $modal.open({ + template: errorsDisplayTemplate, + controller: 'WatcherErrorsDisplayController', + controllerAs: 'vm', + backdrop: 'static', + keyboard: true, + ariaLabelledBy: 'watcher__error-display-modal-title', + resolve: { + params: function () { + return { + title: i18n('xpack.watcher.sections.watchDetail.errorDisplayModalTitleText', { + defaultMessage: 'Errors in the "{actionId}" Action', + values: { actionId } } + ), + errors, + }; + } + } + }); + + errorsModal.result.catch(() => { + // We need to add this empty Promise catch to avoid + // a console error "Possibly unhandled rejection" + }); + } + /** * Event handler methods */ diff --git a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js index 9ff9382f8a999..4696838706694 100644 --- a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js +++ b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js @@ -18,6 +18,7 @@ import '../watch_edit_execute_detail'; import '../watch_edit_actions_execute_summary'; import '../watch_edit_watch_execute_summary'; import 'plugins/watcher/services/license'; +import { ACTION_TYPES } from '../../../../../common/constants'; const app = uiModules.get('xpack/watcher'); @@ -100,14 +101,16 @@ app.directive('jsonWatchEdit', function ($injector, i18n) { } onWatchSave = () => { + this.createActionsForWatch(this.watch); + if (!this.watch.isNew) { - return this.saveWatch(); + return this.validateWatch(); } return this.isExistingWatch() .then(existingWatch => { if (!existingWatch) { - return this.saveWatch(); + return this.validateWatch(); } const confirmModalOptions = { @@ -152,6 +155,23 @@ app.directive('jsonWatchEdit', function ($injector, i18n) { }); } + validateWatch = () => { + const { warning } = this.watch.validate(); + + if (!warning) { + return this.saveWatch(); + } + + const confirmModalOptions = { + onConfirm: this.saveWatch, + confirmButtonText: i18n('xpack.watcher.sections.watchEdit.json.watchErrorsWarning.confirmSaveWatch', { + defaultMessage: 'Yes', + }), + }; + + return confirmModal(warning.message, confirmModalOptions); + } + saveWatch = () => { return watchService.saveWatch(this.watch) .then(() => { @@ -211,6 +231,60 @@ app.directive('jsonWatchEdit', function ($injector, i18n) { // dirtyPrompt.deregister(); kbnUrl.change('/management/elasticsearch/watcher/watches', {}); } + + /** + * Actions instances are not automatically added to the Watch _actions_ Array + * when we add them in the Json editor. + * This method takes takes care of it. + * + * @param watchModel Watch instance + * @return Watch instance + */ + createActionsForWatch(watchInstance) { + watchInstance.resetActions(); + + let action; + let type; + let actionProps; + + Object.keys(watchInstance.watch.actions).forEach((k) => { + action = watchInstance.watch.actions[k]; + type = this.getTypeFromAction(action); + actionProps = this.getPropsFromAction(type, action); + + watchInstance.createAction(type, actionProps); + }); + + return watchInstance; + } + + /** + * Get the type from an action where a key defines its type. + * eg: { email: { ... } } | { slack: { ... } } + * + * @param action A raw action object + * @return {string} The action type + */ + getTypeFromAction(action) { + const actionKeys = Object.keys(action); + let type; + + Object.keys(ACTION_TYPES).forEach((k) => { + if (actionKeys.includes(ACTION_TYPES[k])) { + type = ACTION_TYPES[k]; + } + }); + + return type ? type : ACTION_TYPES.UNKNOWN; + } + + getPropsFromAction(type, action) { + if (type === ACTION_TYPES.SLACK) { + // Slack action has its props inside the "message" object + return action[type].message; + } + return action[type]; + } } }; }); diff --git a/x-pack/plugins/watcher/server/models/action/__tests__/action.js b/x-pack/plugins/watcher/server/models/action/__tests__/action.js deleted file mode 100644 index 747f4feac7b0f..0000000000000 --- a/x-pack/plugins/watcher/server/models/action/__tests__/action.js +++ /dev/null @@ -1,108 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -import expect from 'expect.js'; -import { Action } from '../action'; -import { ACTION_TYPES } from '../../../../common/constants'; - -describe('action', () => { - - describe('Action', () => { - - describe('fromUpstreamJson factory method', () => { - - let upstreamJson; - beforeEach(() => { - upstreamJson = { - id: 'my-action', - actionJson: { - "logging": { - "text": "foo" - } - } - }; - }); - - it(`throws an error if no 'id' property in json`, () => { - delete upstreamJson.id; - expect(Action.fromUpstreamJson).withArgs(upstreamJson) - .to.throwError(/must contain an id property/i); - }); - - it(`throws an error if no 'actionJson' property in json`, () => { - delete upstreamJson.actionJson; - expect(Action.fromUpstreamJson).withArgs(upstreamJson) - .to.throwError(/must contain an actionJson property/i); - }); - - it('returns correct Action instance', () => { - const action = Action.fromUpstreamJson(upstreamJson); - - expect(action.id).to.be(upstreamJson.id); - }); - - }); - - describe('type getter method', () => { - - it(`returns a value from ACTION_TYPES when there is a valid model class`, () => { - const upstreamJson = { - id: 'my-action', - actionJson: { - logging: { - 'text': 'foo' - } - } - }; - const action = Action.fromUpstreamJson(upstreamJson); - - expect(action.type).to.be(ACTION_TYPES.LOGGING); - }); - - it(`returns ACTION_TYPES.UNKNOWN when there is no valid model class`, () => { - const upstreamJson = { - id: 'my-action', - actionJson: { - unknown_action_type: { - 'foo': 'bar' - } - } - }; - const action = Action.fromUpstreamJson(upstreamJson); - - expect(action.type).to.be(ACTION_TYPES.UNKNOWN); - }); - - }); - - describe('downstreamJson getter method', () => { - - let upstreamJson; - beforeEach(() => { - upstreamJson = { - id: 'my-action', - actionJson: { - "logging": { - "text": "foo" - } - } - }; - }); - - it('returns correct JSON for client', () => { - const action = Action.fromUpstreamJson(upstreamJson); - - const json = action.downstreamJson; - - expect(json.id).to.be(action.id); - expect(json.type).to.be(action.type); - }); - - }); - - }); - -}); diff --git a/x-pack/plugins/watcher/server/models/action/action.js b/x-pack/plugins/watcher/server/models/action/action.js index 1d02977d0e684..a5d677518da26 100644 --- a/x-pack/plugins/watcher/server/models/action/action.js +++ b/x-pack/plugins/watcher/server/models/action/action.js @@ -26,7 +26,18 @@ export class Action { } // From Elasticsearch - static fromUpstreamJson(json) { + static fromUpstreamJson(json, options = { throwExceptions: {} }) { + if (!json.id) { + throw badRequest( + i18n.translate('xpack.watcher.models.actionStatus.absenceOfIdPropertyBadRequestMessage', { + defaultMessage: 'json argument must contain an {id} property', + values: { + id: 'id' + } + }), + ); + } + if (!json.actionJson) { throw badRequest( i18n.translate('xpack.watcher.models.action.absenceOfActionJsonPropertyBadRequestMessage', { @@ -40,13 +51,38 @@ export class Action { const type = getActionType(json.actionJson); const ActionType = ActionTypes[type] || UnknownAction; - return ActionType.fromUpstreamJson(json); + + const { action, errors } = ActionType.fromUpstreamJson(json, options); + const doThrowException = options.throwExceptions.Action !== false; + + if (errors && doThrowException) { + this.throwErrors(errors); + } + + return action; } // From Kibana - static fromDownstreamJson(json) { + static fromDownstreamJson(json, options = { throwExceptions: {} }) { const ActionType = ActionTypes[json.type] || UnknownAction; - return ActionType.fromDownstreamJson(json); + const { action, errors } = ActionType.fromDownstreamJson(json); + const doThrowException = options.throwExceptions.Action !== false; + + if (errors && doThrowException) { + this.throwErrors(errors); + } + + return action; + } + + static throwErrors(errors) { + const allMessages = errors.reduce((message, error) => { + if (message) { + return `${message}, ${error.message}`; + } + return error.message; + }, ''); + throw badRequest(allMessages); } } diff --git a/x-pack/plugins/watcher/server/models/action/action.test.js b/x-pack/plugins/watcher/server/models/action/action.test.js new file mode 100644 index 0000000000000..8771c053b574d --- /dev/null +++ b/x-pack/plugins/watcher/server/models/action/action.test.js @@ -0,0 +1,136 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { Action } from './action'; +import { LoggingAction } from './logging_action'; +import { ACTION_TYPES } from '../../../common/constants'; + +jest.mock('./logging_action', () => ({ + LoggingAction: { + fromUpstreamJson: jest.fn(({ id }) => ({ + errors: null, + action: { id, type: 'logging' }, + })), + } +})); + +describe('action', () => { + + describe('Action', () => { + + describe('fromUpstreamJson factory method', () => { + + let upstreamJson; + beforeEach(() => { + upstreamJson = { + id: 'my-action', + actionJson: { + "logging": { + "text": "foo" + } + } + }; + }); + + it(`throws an error if no 'id' property in json`, () => { + delete upstreamJson.id; + expect(() => { + Action.fromUpstreamJson(upstreamJson); + }).toThrowError(/must contain an id property/i); + }); + + it(`throws an error if no 'actionJson' property in json`, () => { + delete upstreamJson.actionJson; + expect(() => { + Action.fromUpstreamJson(upstreamJson); + }).toThrowError(/must contain an actionJson property/i); + }); + + it(`throws an error if an Action is invalid`, () => { + const message = 'Missing prop in Logging Action!'; + + LoggingAction.fromUpstreamJson.mockReturnValueOnce({ + errors: [{ message }], + action: {}, + }); + + expect(() => { + Action.fromUpstreamJson(upstreamJson); + }).toThrowError(message); + }); + + it('returns correct Action instance', () => { + const action = Action.fromUpstreamJson(upstreamJson); + + expect(action.id).toBe(upstreamJson.id); + }); + + }); + + describe('type getter method', () => { + + it(`returns the correct known Action type`, () => { + const options = { throwExceptions: { Action: false } }; + + const upstreamLoggingJson = { id: 'action1', actionJson: { logging: {} } }; + const loggingAction = Action.fromUpstreamJson(upstreamLoggingJson, options); + + const upstreamEmailJson = { id: 'action2', actionJson: { email: {} } }; + const emailAction = Action.fromUpstreamJson(upstreamEmailJson, options); + + const upstreamSlackJson = { id: 'action3', actionJson: { slack: {} } }; + const slackAction = Action.fromUpstreamJson(upstreamSlackJson, options); + + expect(loggingAction.type).toBe(ACTION_TYPES.LOGGING); + expect(emailAction.type).toBe(ACTION_TYPES.EMAIL); + expect(slackAction.type).toBe(ACTION_TYPES.SLACK); + }); + + it(`returns ACTION_TYPES.UNKNOWN when there is no valid model class`, () => { + const upstreamJson = { + id: 'my-action', + actionJson: { + unknown_action_type: { + 'foo': 'bar' + } + } + }; + const action = Action.fromUpstreamJson(upstreamJson); + + expect(action.type).toBe(ACTION_TYPES.UNKNOWN); + }); + + }); + + describe('downstreamJson getter method', () => { + + let upstreamJson; + beforeEach(() => { + upstreamJson = { + id: 'my-action', + actionJson: { + "email": { + "to": "elastic@elastic.co" + } + } + }; + }); + + it('returns correct JSON for client', () => { + + const action = Action.fromUpstreamJson(upstreamJson); + + const json = action.downstreamJson; + + expect(json.id).toBe(action.id); + expect(json.type).toBe(action.type); + }); + + }); + + }); + +}); diff --git a/x-pack/plugins/watcher/server/models/action/base_action.js b/x-pack/plugins/watcher/server/models/action/base_action.js index dc03a76a28515..d85cccee28508 100644 --- a/x-pack/plugins/watcher/server/models/action/base_action.js +++ b/x-pack/plugins/watcher/server/models/action/base_action.js @@ -8,9 +8,10 @@ import { badRequest } from 'boom'; import { i18n } from '@kbn/i18n'; export class BaseAction { - constructor(props) { + constructor(props, errors) { this.id = props.id; this.type = props.type; + this.errors = errors; } get downstreamJson() { diff --git a/x-pack/plugins/watcher/server/models/action/email_action.js b/x-pack/plugins/watcher/server/models/action/email_action.js index 0e45e906238ed..fea678607d12f 100644 --- a/x-pack/plugins/watcher/server/models/action/email_action.js +++ b/x-pack/plugins/watcher/server/models/action/email_action.js @@ -4,15 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ -import { badRequest } from 'boom'; import { BaseAction } from './base_action'; -import { ACTION_TYPES } from '../../../common/constants'; +import { ACTION_TYPES, ERROR_CODES } from '../../../common/constants'; import { i18n } from '@kbn/i18n'; export class EmailAction extends BaseAction { - constructor(props) { + constructor(props, errors) { props.type = ACTION_TYPES.EMAIL; - super(props); + super(props, errors); this.to = props.to; this.subject = props.subject; @@ -34,14 +33,16 @@ export class EmailAction extends BaseAction { // From Kibana static fromDownstreamJson(json) { const props = super.getPropsFromDownstreamJson(json); + const { errors } = this.validateJson(json); Object.assign(props, { to: json.to, subject: json.subject, - body: json.body + body: json.body, }); - return new EmailAction(props); + const action = new EmailAction(props, errors); + return { action, errors }; } // To Elasticsearch @@ -70,29 +71,10 @@ export class EmailAction extends BaseAction { // From Elasticsearch static fromUpstreamJson(json) { const props = super.getPropsFromUpstreamJson(json); - - if (!json.actionJson.email) { - throw badRequest( - i18n.translate('xpack.watcher.models.emailAction.absenceOfActionJsonEmailPropertyBadRequestMessage', { - defaultMessage: 'json argument must contain an {actionJsonEmail} property', - values: { - actionJsonEmail: 'actionJson.email' - } - }), - ); - } - if (!json.actionJson.email.to) { - throw badRequest( - i18n.translate('xpack.watcher.models.emailAction.absenceOfActionJsonEmailToPropertyBadRequestMessage', { - defaultMessage: 'json argument must contain an {actionJsonEmailTo} property', - values: { - actionJsonEmailTo: 'actionJson.email.to' - } - }), - ); - } + const { errors } = this.validateJson(json.actionJson); const optionalFields = {}; + if (json.actionJson.email.subject) { optionalFields.subject = json.actionJson.email.subject; } @@ -106,7 +88,45 @@ export class EmailAction extends BaseAction { ...optionalFields, }); - return new EmailAction(props); + const action = new EmailAction(props, errors); + + return { action, errors }; + } + + static validateJson(json) { + const errors = []; + + if (!json.email) { + const message = i18n.translate('xpack.watcher.models.emailAction.absenceOfActionJsonEmailPropertyBadRequestMessage', { + defaultMessage: 'json argument must contain an {actionJsonEmail} property', + values: { + actionJsonEmail: 'actionJson.email' + } + }); + + errors.push({ + code: ERROR_CODES.ERR_PROP_MISSING, + message + }); + + json.email = {}; + } + + if (!json.email.to) { + const message = i18n.translate('xpack.watcher.models.emailAction.absenceOfActionJsonEmailToPropertyBadRequestMessage', { + defaultMessage: 'json argument must contain an {actionJsonEmailTo} property', + values: { + actionJsonEmailTo: 'actionJson.email.to' + } + }); + + errors.push({ + code: ERROR_CODES.ERR_PROP_MISSING, + message + }); + } + + return { errors: errors.length ? errors : null }; } /* diff --git a/x-pack/plugins/watcher/server/models/action/logging_action.js b/x-pack/plugins/watcher/server/models/action/logging_action.js index fc1f02ad73931..62496d2f85f7b 100644 --- a/x-pack/plugins/watcher/server/models/action/logging_action.js +++ b/x-pack/plugins/watcher/server/models/action/logging_action.js @@ -4,15 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ -import { badRequest } from 'boom'; import { BaseAction } from './base_action'; -import { ACTION_TYPES } from '../../../common/constants'; +import { ACTION_TYPES, ERROR_CODES } from '../../../common/constants'; import { i18n } from '@kbn/i18n'; export class LoggingAction extends BaseAction { - constructor(props) { + constructor(props, errors) { props.type = ACTION_TYPES.LOGGING; - super(props); + super(props, errors); this.text = props.text; } @@ -30,12 +29,14 @@ export class LoggingAction extends BaseAction { // From Kibana static fromDownstreamJson(json) { const props = super.getPropsFromDownstreamJson(json); + const { errors } = this.validateJson(json); Object.assign(props, { text: json.text }); - return new LoggingAction(props); + const action = new LoggingAction(props, errors); + return { action, errors }; } // To Elasticsearch @@ -54,33 +55,46 @@ export class LoggingAction extends BaseAction { // From Elasticsearch static fromUpstreamJson(json) { const props = super.getPropsFromUpstreamJson(json); + const { errors } = this.validateJson(json.actionJson); - if (!json.actionJson.logging) { - throw badRequest( - i18n.translate('xpack.watcher.models.loggingAction.absenceOfActionJsonLoggingPropertyBadRequestMessage', { + Object.assign(props, { + text: json.actionJson.logging.text + }); + + const action = new LoggingAction(props, errors); + return { action, errors }; + } + + static validateJson(json) { + const errors = []; + + if (!json.logging) { + errors.push({ + code: ERROR_CODES.ERR_PROP_MISSING, + message: i18n.translate('xpack.watcher.models.loggingAction.absenceOfActionJsonLoggingPropertyBadRequestMessage', { defaultMessage: 'json argument must contain an {actionJsonLogging} property', values: { actionJsonLogging: 'actionJson.logging' } }), - ); + }); + + json.logging = {}; } - if (!json.actionJson.logging.text) { - throw badRequest( - i18n.translate('xpack.watcher.models.loggingAction.absenceOfActionJsonLoggingTextPropertyBadRequestMessage', { + + if (!json.logging.text) { + errors.push({ + code: ERROR_CODES.ERR_PROP_MISSING, + message: i18n.translate('xpack.watcher.models.loggingAction.absenceOfActionJsonLoggingTextPropertyBadRequestMessage', { defaultMessage: 'json argument must contain an {actionJsonLoggingText} property', values: { actionJsonLoggingText: 'actionJson.logging.text' } }), - ); + }); } - Object.assign(props, { - text: json.actionJson.logging.text - }); - - return new LoggingAction(props); + return { errors: errors.length ? errors : null }; } /* diff --git a/x-pack/plugins/watcher/server/models/action/slack_action.js b/x-pack/plugins/watcher/server/models/action/slack_action.js index 1852cfcde8c23..469aaae7b1368 100644 --- a/x-pack/plugins/watcher/server/models/action/slack_action.js +++ b/x-pack/plugins/watcher/server/models/action/slack_action.js @@ -4,15 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ -import { badRequest } from 'boom'; import { BaseAction } from './base_action'; -import { ACTION_TYPES } from '../../../common/constants'; +import { ACTION_TYPES, ERROR_CODES } from '../../../common/constants'; import { i18n } from '@kbn/i18n'; export class SlackAction extends BaseAction { - constructor(props) { + constructor(props, errors) { props.type = ACTION_TYPES.SLACK; - super(props); + super(props, errors); this.to = props.to; this.text = props.text; @@ -33,12 +32,15 @@ export class SlackAction extends BaseAction { static fromDownstreamJson(json) { const props = super.getPropsFromDownstreamJson(json); + const { errors } = this.validateJson(json); + Object.assign(props, { to: json.to, text: json.text }); - return new SlackAction(props); + const action = new SlackAction(props, errors); + return { action, errors }; } // To Elasticsearch @@ -60,45 +62,50 @@ export class SlackAction extends BaseAction { // From Elasticsearch static fromUpstreamJson(json) { const props = super.getPropsFromUpstreamJson(json); + const { errors } = this.validateJson(json.actionJson); + + Object.assign(props, { + to: json.actionJson.slack.message.to, + text: json.actionJson.slack.message.text + }); - if (!json.actionJson.slack) { - throw badRequest( - i18n.translate('xpack.watcher.models.slackAction.absenceOfActionJsonSlackPropertyBadRequestMessage', { + const action = new SlackAction(props, errors); + + return { action, errors }; + } + + static validateJson(json) { + const errors = []; + + if (!json.slack) { + errors.push({ + code: ERROR_CODES.ERR_PROP_MISSING, + message: i18n.translate('xpack.watcher.models.slackAction.absenceOfActionJsonSlackPropertyBadRequestMessage', { defaultMessage: 'json argument must contain an {actionJsonSlack} property', values: { actionJsonSlack: 'actionJson.slack' } - }), - ); + }) + }); + + json.slack = {}; } - if (!json.actionJson.slack.message) { - throw badRequest( - i18n.translate('xpack.watcher.models.slackAction.absenceOfActionJsonSlackMessagePropertyBadRequestMessage', { + + if (!json.slack.message) { + errors.push({ + code: ERROR_CODES.ERR_PROP_MISSING, + message: i18n.translate('xpack.watcher.models.slackAction.absenceOfActionJsonSlackMessagePropertyBadRequestMessage', { defaultMessage: 'json argument must contain an {actionJsonSlackMessage} property', values: { actionJsonSlackMessage: 'actionJson.slack.message' } - - }), - ); - } - if (!json.actionJson.slack.message.to) { - throw badRequest( - i18n.translate('xpack.watcher.models.slackAction.absenceOfActionJsonSlackMessageToPropertyBadRequestMessage', { - defaultMessage: 'json argument must contain an {actionJsonSlackMessageTo} property', - values: { - actionJsonSlackMessageTo: 'actionJson.slack.message.to' - } }), - ); - } + }); - Object.assign(props, { - to: json.actionJson.slack.message.to, - text: json.actionJson.slack.message.text - }); + json.slack.message = {}; + } - return new SlackAction(props); + return { errors: errors.length ? errors : null }; } /* diff --git a/x-pack/plugins/watcher/server/models/action/unknown_action.js b/x-pack/plugins/watcher/server/models/action/unknown_action.js index 7dfe83a1e4000..af8b827f770aa 100644 --- a/x-pack/plugins/watcher/server/models/action/unknown_action.js +++ b/x-pack/plugins/watcher/server/models/action/unknown_action.js @@ -4,15 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ -import { badRequest } from 'boom'; import { BaseAction } from './base_action'; -import { ACTION_TYPES } from '../../../common/constants'; +import { ACTION_TYPES, ERROR_CODES } from '../../../common/constants'; import { i18n } from '@kbn/i18n'; export class UnknownAction extends BaseAction { - constructor(props) { + constructor(props, errors) { props.type = ACTION_TYPES.UNKNOWN; - super(props); + super(props, errors); this.actionJson = props.actionJson; } @@ -51,23 +50,33 @@ export class UnknownAction extends BaseAction { // From Elasticsearch static fromUpstreamJson(json) { const props = super.getPropsFromUpstreamJson(json); + const { errors } = this.validateJson(json); + + Object.assign(props, { + actionJson: json.actionJson + }); + + const action = new UnknownAction(props, errors); + + return { action, errors }; + } + + static validateJson(json) { + const errors = []; if (!json.actionJson) { - throw badRequest( - i18n.translate('xpack.watcher.models.unknownAction.absenceOfActionJsonPropertyBadRequestMessage', { + errors.push({ + code: ERROR_CODES.ERR_PROP_MISSING, + message: i18n.translate('xpack.watcher.models.unknownAction.absenceOfActionJsonPropertyBadRequestMessage', { defaultMessage: 'json argument must contain an {actionJson} property', values: { actionJson: 'actionJson' } }), - ); + }); } - Object.assign(props, { - actionJson: json.actionJson - }); - - return new UnknownAction(props); + return { errors: errors.length ? errors : null }; } /* diff --git a/x-pack/plugins/watcher/server/models/action_status/__tests__/action_status.js b/x-pack/plugins/watcher/server/models/action_status/__tests__/action_status.js index 460fdd1a3eaf2..b30d29af611d7 100644 --- a/x-pack/plugins/watcher/server/models/action_status/__tests__/action_status.js +++ b/x-pack/plugins/watcher/server/models/action_status/__tests__/action_status.js @@ -36,7 +36,7 @@ describe('action_status', () => { 'timestamp': '2017-03-01T20:55:49.679Z', 'successful': true } - } + }, }; }); @@ -53,7 +53,7 @@ describe('action_status', () => { }); it('returns correct ActionStatus instance', () => { - const actionStatus = ActionStatus.fromUpstreamJson(upstreamJson); + const actionStatus = ActionStatus.fromUpstreamJson({ ...upstreamJson, errors: { foo: 'bar' } }); expect(actionStatus.id).to.be(upstreamJson.id); expect(actionStatus.lastAcknowledged).to @@ -68,6 +68,8 @@ describe('action_status', () => { .eql(moment(upstreamJson.actionStatusJson.last_throttle.timestamp)); expect(actionStatus.lastSuccessfulExecution).to .eql(moment(upstreamJson.actionStatusJson.last_successful_execution.timestamp)); + expect(actionStatus.errors).to + .eql({ foo: 'bar' }); }); }); @@ -106,6 +108,12 @@ describe('action_status', () => { expect(actionStatus.state).to.be(ACTION_STATES.ERROR); }); + it('correctly calculates ACTION_STATES.CONFIG_ERROR', () => { + const actionStatus = ActionStatus.fromUpstreamJson({ ...upstreamJson, errors: { foo: 'bar' } }); + expect(actionStatus.state).to.be(ACTION_STATES.CONFIG_ERROR); + }); + + it(`correctly calculates ACTION_STATES.OK`, () => { upstreamJson.actionStatusJson.ack.state = 'awaits_successful_execution'; const actionStatus = ActionStatus.fromUpstreamJson(upstreamJson); diff --git a/x-pack/plugins/watcher/server/models/action_status/action_status.js b/x-pack/plugins/watcher/server/models/action_status/action_status.js index 30319281e541f..e6f53d0bf764b 100644 --- a/x-pack/plugins/watcher/server/models/action_status/action_status.js +++ b/x-pack/plugins/watcher/server/models/action_status/action_status.js @@ -14,6 +14,7 @@ export class ActionStatus { constructor(props) { this.id = props.id; this.actionStatusJson = props.actionStatusJson; + this.errors = props.errors; this.lastAcknowledged = getMoment(get(this.actionStatusJson, 'ack.timestamp')); this.lastExecution = getMoment(get(this.actionStatusJson, 'last_execution.timestamp')); @@ -31,6 +32,10 @@ export class ActionStatus { return ACTION_STATES.ERROR; } + if (this.errors) { + return ACTION_STATES.CONFIG_ERROR; + } + if (ackState === 'awaits_successful_execution') { return ACTION_STATES.OK; } diff --git a/x-pack/plugins/watcher/server/models/watch/__tests__/base_watch.js b/x-pack/plugins/watcher/server/models/watch/__tests__/base_watch.js index 1316559561f37..9740874cb92ac 100644 --- a/x-pack/plugins/watcher/server/models/watch/__tests__/base_watch.js +++ b/x-pack/plugins/watcher/server/models/watch/__tests__/base_watch.js @@ -11,6 +11,8 @@ import sinon from 'sinon'; const actionFromUpstreamJSONMock = sinon.stub(); const actionFromDownstreamJSONMock = sinon.stub(); const watchStatusFromUpstreamJSONMock = sinon.stub(); +const watchErrorsFromUpstreamJSONMock = sinon.stub(); + class ActionStub { static fromUpstreamJson(...args) { actionFromUpstreamJSONMock(...args); @@ -30,9 +32,17 @@ class WatchStatusStub { } } +class WatchErrorsStub { + static fromUpstreamJson(...args) { + watchErrorsFromUpstreamJSONMock(...args); + return { foo: 'bar' }; + } +} + const { BaseWatch } = proxyquire('../base_watch', { '../action': { Action: ActionStub }, - '../watch_status': { WatchStatus: WatchStatusStub } + '../watch_status': { WatchStatus: WatchStatusStub }, + '../watch_errors': { WatchErrors: WatchErrorsStub }, }); describe('BaseWatch', () => { @@ -57,6 +67,7 @@ describe('BaseWatch', () => { 'type', 'isSystemWatch', 'watchStatus', + 'watchErrors', 'actions' ]; @@ -72,6 +83,7 @@ describe('BaseWatch', () => { it('populates all expected fields', () => { props.watchStatus = 'bar'; props.actions = 'baz'; + props.watchErrors = { actions: 'email' }; const actual = new BaseWatch(props); const expected = { @@ -80,6 +92,7 @@ describe('BaseWatch', () => { type: 'logging', isSystemWatch: false, watchStatus: 'bar', + watchErrors: { actions: 'email' }, actions: 'baz' }; @@ -169,6 +182,12 @@ describe('BaseWatch', () => { prop2: 'prop2' } }, + watchErrors: { + downstreamJson: { + prop1: 'prop1', + prop2: 'prop2' + } + }, actions: [{ downstreamJson: { prop1: 'prop3', @@ -188,14 +207,16 @@ describe('BaseWatch', () => { type: props.type, isSystemWatch: false, watchStatus: props.watchStatus.downstreamJson, + watchErrors: props.watchErrors.downstreamJson, actions: props.actions.map(a => a.downstreamJson) }; expect(actual).to.eql(expected); }); - it('should respect an undefined watchStatus prop', () => { + it('should respect an undefined watchStatus & watchErrors prop', () => { delete props.watchStatus; + delete props.watchErrors; const watch = new BaseWatch(props); const actual = watch.downstreamJson; @@ -206,6 +227,7 @@ describe('BaseWatch', () => { type: props.type, isSystemWatch: false, watchStatus: undefined, + watchErrors: undefined, actions: props.actions.map(a => a.downstreamJson) }; @@ -397,6 +419,7 @@ describe('BaseWatch', () => { 'name', 'watchJson', 'watchStatus', + 'watchErrors', 'actions' ]; @@ -456,6 +479,9 @@ describe('BaseWatch', () => { state: { active: true } + }, + watchErrors: { + foo: 'bar' } })).to.be(true); }); diff --git a/x-pack/plugins/watcher/server/models/watch/base_watch.js b/x-pack/plugins/watcher/server/models/watch/base_watch.js index 1f7f24b63b5df..35c524ec8a576 100644 --- a/x-pack/plugins/watcher/server/models/watch/base_watch.js +++ b/x-pack/plugins/watcher/server/models/watch/base_watch.js @@ -9,6 +9,7 @@ import { badRequest } from 'boom'; import { Action } from '../action'; import { WatchStatus } from '../watch_status'; import { i18n } from '@kbn/i18n'; +import { WatchErrors } from '../watch_errors'; export class BaseWatch { // This constructor should not be used directly. @@ -22,6 +23,7 @@ export class BaseWatch { this.isSystemWatch = false; this.watchStatus = props.watchStatus; + this.watchErrors = props.watchErrors; this.actions = props.actions; } @@ -57,6 +59,7 @@ export class BaseWatch { type: this.type, isSystemWatch: this.isSystemWatch, watchStatus: this.watchStatus ? this.watchStatus.downstreamJson : undefined, + watchErrors: this.watchErrors ? this.watchErrors.downstreamJson : undefined, actions: map(this.actions, (action) => action.downstreamJson) }; @@ -87,7 +90,7 @@ export class BaseWatch { } // from Elasticsearch - static getPropsFromUpstreamJson(json) { + static getPropsFromUpstreamJson(json, options) { if (!json.id) { throw badRequest( i18n.translate('xpack.watcher.models.baseWatch.absenceOfIdPropertyBadRequestMessage', { @@ -135,12 +138,15 @@ export class BaseWatch { const actionsJson = get(watchJson, 'actions', {}); const actions = map(actionsJson, (actionJson, actionId) => { - return Action.fromUpstreamJson({ id: actionId, actionJson }); + return Action.fromUpstreamJson({ id: actionId, actionJson }, options); }); + const watchErrors = WatchErrors.fromUpstreamJson(this.getWatchErrors(actions)); + const watchStatus = WatchStatus.fromUpstreamJson({ id, - watchStatusJson + watchStatusJson, + watchErrors, }); return { @@ -148,7 +154,31 @@ export class BaseWatch { name, watchJson, watchStatus, + watchErrors, actions }; } + + /** + * Retrieve all the errors in the watch + * + * @param {array} actions - Watch actions + */ + static getWatchErrors(actions) { + const watchErrors = {}; + + // Check for errors in Actions + const actionsErrors = actions.reduce((acc, action) => { + if (action.errors) { + acc[action.id] = action.errors; + } + return acc; + }, {}); + + if (Object.keys(actionsErrors).length) { + watchErrors.actions = actionsErrors; + } + + return watchErrors; + } } diff --git a/x-pack/plugins/watcher/server/models/watch/json_watch.js b/x-pack/plugins/watcher/server/models/watch/json_watch.js index de28004be5252..343c76054d2e0 100644 --- a/x-pack/plugins/watcher/server/models/watch/json_watch.js +++ b/x-pack/plugins/watcher/server/models/watch/json_watch.js @@ -48,8 +48,8 @@ export class JsonWatch extends BaseWatch { } // From Elasticsearch - static fromUpstreamJson(json) { - const baseProps = super.getPropsFromUpstreamJson(json); + static fromUpstreamJson(json, options) { + const baseProps = super.getPropsFromUpstreamJson(json, options); const watch = cloneDeep(baseProps.watchJson); if (has(watch, 'metadata.name')) { diff --git a/x-pack/plugins/watcher/server/models/watch/watch.js b/x-pack/plugins/watcher/server/models/watch/watch.js index 34752b55be360..c721abad4cc64 100644 --- a/x-pack/plugins/watcher/server/models/watch/watch.js +++ b/x-pack/plugins/watcher/server/models/watch/watch.js @@ -50,7 +50,7 @@ export class Watch { } // from Elasticsearch - static fromUpstreamJson(json) { + static fromUpstreamJson(json, options) { if (!json.watchJson) { throw badRequest( i18n.translate('xpack.watcher.models.watch.absenceOfWatchJsonPropertyBadRequestMessage', { @@ -65,6 +65,6 @@ export class Watch { const type = getWatchType(json.watchJson); const WatchType = WatchTypes[type]; - return WatchType.fromUpstreamJson(json); + return WatchType.fromUpstreamJson(json, options); } } diff --git a/x-pack/plugins/watcher/server/models/watch_errors/index.js b/x-pack/plugins/watcher/server/models/watch_errors/index.js new file mode 100644 index 0000000000000..dec5e51d74e31 --- /dev/null +++ b/x-pack/plugins/watcher/server/models/watch_errors/index.js @@ -0,0 +1,7 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +export { WatchErrors } from './watch_errors'; diff --git a/x-pack/plugins/watcher/server/models/watch_errors/watch_errors.js b/x-pack/plugins/watcher/server/models/watch_errors/watch_errors.js new file mode 100644 index 0000000000000..fbbe9d5ee6d68 --- /dev/null +++ b/x-pack/plugins/watcher/server/models/watch_errors/watch_errors.js @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +export class WatchErrors { + constructor({ actions } = {}) { + this.actions = actions; + } + + // generate object to send to kibana + get downstreamJson() { + const json = { + actions: this.actions, + }; + + return json; + } + + // generate object from elasticsearch response + static fromUpstreamJson(sections) { + return new WatchErrors(sections); + } +} diff --git a/x-pack/plugins/watcher/server/models/watch_errors/watch_errors.test.js b/x-pack/plugins/watcher/server/models/watch_errors/watch_errors.test.js new file mode 100644 index 0000000000000..edf0c09aa3387 --- /dev/null +++ b/x-pack/plugins/watcher/server/models/watch_errors/watch_errors.test.js @@ -0,0 +1,34 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { WatchErrors } from './watch_errors'; + +describe('watch_errors', () => { + describe('WatchErrors constructor', () => { + it('should set "actions" error', () => { + const watchErrors1 = new WatchErrors(); + const watchErrors2 = new WatchErrors({ actions: { foo: 'bar' } }); + + expect(watchErrors1.actions).toEqual(undefined); + expect(watchErrors2.actions).toEqual({ foo: 'bar' }); + }); + }); + + describe('fromUpstreamJson()', () => { + it('should return WatchErrors instance', () => { + const instance = WatchErrors.fromUpstreamJson(); + + expect(instance instanceof WatchErrors).toBe(true); + }); + + it('should pass errors secctions to the constructor', () => { + const instance = WatchErrors.fromUpstreamJson({ actions: { foo: 'bar' } }); + + expect(instance.actions).toEqual({ foo: 'bar' }); + }); + }); + +}); \ No newline at end of file diff --git a/x-pack/plugins/watcher/server/models/watch_history_item/__tests__/watch_history_item.js b/x-pack/plugins/watcher/server/models/watch_history_item/__tests__/watch_history_item.js index 267a78ee0a230..2d13243f8a6d3 100644 --- a/x-pack/plugins/watcher/server/models/watch_history_item/__tests__/watch_history_item.js +++ b/x-pack/plugins/watcher/server/models/watch_history_item/__tests__/watch_history_item.js @@ -73,7 +73,8 @@ describe('watch_history_item', () => { state: { active: upstreamJson.watchHistoryItemJson.status.state.active } - } + }, + watchErrors: {} }); }); diff --git a/x-pack/plugins/watcher/server/models/watch_status/__tests__/watch_status.js b/x-pack/plugins/watcher/server/models/watch_status/__tests__/watch_status.js index 5c33c5b0239b4..4b1649ab4c1e0 100644 --- a/x-pack/plugins/watcher/server/models/watch_status/__tests__/watch_status.js +++ b/x-pack/plugins/watcher/server/models/watch_status/__tests__/watch_status.js @@ -252,6 +252,17 @@ describe('watch_status', () => { expect(watchStatus.state).to.be(WATCH_STATES.ERROR); }); + it('correctly calculates WATCH_STATE.CONFIG_ERROR', () => { + const watchStatus = WatchStatus.fromUpstreamJson(upstreamJson); + + watchStatus.actionStatuses = [ + { state: ACTION_STATES.OK }, + { state: ACTION_STATES.CONFIG_ERROR } + ]; + + expect(watchStatus.state).to.be(WATCH_STATES.CONFIG_ERROR); + }); + it(`correctly calculates WATCH_STATES.DISABLED when watch is inactive`, () => { const watchStatus = WatchStatus.fromUpstreamJson(upstreamJson); watchStatus.isActive = false; diff --git a/x-pack/plugins/watcher/server/models/watch_status/watch_status.js b/x-pack/plugins/watcher/server/models/watch_status/watch_status.js index d6e6a8dbe1d7e..cf91fe829e90e 100644 --- a/x-pack/plugins/watcher/server/models/watch_status/watch_status.js +++ b/x-pack/plugins/watcher/server/models/watch_status/watch_status.js @@ -31,6 +31,7 @@ export class WatchStatus { this.id = props.id; this.watchState = props.state; this.watchStatusJson = props.watchStatusJson; + this.watchErrors = props.watchErrors || {}; this.isActive = Boolean(get(this.watchStatusJson, 'state.active')); this.lastChecked = getMoment(get(this.watchStatusJson, 'last_checked')); @@ -38,7 +39,11 @@ export class WatchStatus { const actionStatusesJson = get(this.watchStatusJson, 'actions', {}); this.actionStatuses = map(actionStatusesJson, (actionStatusJson, id) => { - const json = { id, actionStatusJson }; + const json = { + id, + actionStatusJson, + errors: this.watchErrors.actions && this.watchErrors.actions[id], + }; return ActionStatus.fromUpstreamJson(json); }); } @@ -58,6 +63,10 @@ export class WatchStatus { return WATCH_STATES.ERROR; } + if (totals[ACTION_STATES.CONFIG_ERROR] > 0) { + return WATCH_STATES.CONFIG_ERROR; + } + const firingTotal = totals[ACTION_STATES.FIRING] + totals[ACTION_STATES.ACKNOWLEDGED] + totals[ACTION_STATES.THROTTLED]; diff --git a/x-pack/plugins/watcher/server/routes/api/watch/register_load_route.js b/x-pack/plugins/watcher/server/routes/api/watch/register_load_route.js index f225169ac25a7..43d60ead3b82f 100644 --- a/x-pack/plugins/watcher/server/routes/api/watch/register_load_route.js +++ b/x-pack/plugins/watcher/server/routes/api/watch/register_load_route.js @@ -30,24 +30,28 @@ export function registerLoadRoute(server) { const id = request.params.id; return fetchWatch(callWithRequest, id) - .then((hit) => { + .then(hit => { const watchJson = get(hit, 'watch'); const watchStatusJson = get(hit, 'status'); const json = { id, watchJson, - watchStatusJson + watchStatusJson, }; - const watch = Watch.fromUpstreamJson(json); + const watch = Watch.fromUpstreamJson(json, { + throwExceptions: { + Action: false, + }, + }); + reply({ watch: watch.downstreamJson }); }) .catch(err => { - // Case: Error from Elasticsearch JS client if (isEsError(err)) { const statusCodeToMessageMap = { - 404: `Watch with id = ${id} not found` + 404: `Watch with id = ${id} not found`, }; return reply(wrapEsError(err, statusCodeToMessageMap)); } diff --git a/x-pack/plugins/watcher/server/routes/api/watches/register_list_route.js b/x-pack/plugins/watcher/server/routes/api/watches/register_list_route.js index a1a29cca4cd8d..a027641e53856 100644 --- a/x-pack/plugins/watcher/server/routes/api/watches/register_list_route.js +++ b/x-pack/plugins/watcher/server/routes/api/watches/register_list_route.js @@ -44,11 +44,18 @@ export function registerListRoute(server) { const watchJson = get(hit, '_source'); const watchStatusJson = get(hit, '_source.status'); - return Watch.fromUpstreamJson({ - id, - watchJson, - watchStatusJson - }); + return Watch.fromUpstreamJson( + { + id, + watchJson, + watchStatusJson, + }, + { + throwExceptions: { + Action: false, + }, + } + ); }); reply({ From 83ffdf75154ca5f1966794dde4d186802db06e8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Thu, 25 Oct 2018 15:23:43 +0200 Subject: [PATCH 2/5] fix(watcher): add code review changes --- .../watcher/common/constants/action_states.js | 4 +++- .../watcher/common/constants/watch_states.js | 5 ++++- .../watcher/public/models/action/slack_action.js | 5 ++--- .../public/models/watch_errors/watch_errors.js | 4 ++-- .../action_status_table/action_status_table.html | 6 +++--- .../action_status_table/action_status_table.js | 15 ++++++++++----- .../components/watch_detail/watch_detail.js | 2 +- .../components/json_watch_edit/json_watch_edit.js | 3 +-- 8 files changed, 26 insertions(+), 18 deletions(-) diff --git a/x-pack/plugins/watcher/common/constants/action_states.js b/x-pack/plugins/watcher/common/constants/action_states.js index 0930ad6949142..e34a5fd37ff3a 100644 --- a/x-pack/plugins/watcher/common/constants/action_states.js +++ b/x-pack/plugins/watcher/common/constants/action_states.js @@ -34,6 +34,8 @@ export const ACTION_STATES = { }), // Action has a configuration error - CONFIG_ERROR: 'Config Error' + CONFIG_ERROR: i18n.translate('xpack.watcher.constants.actionStates.configErrorStateText', { + defaultMessage: 'Config error' + }), }; diff --git a/x-pack/plugins/watcher/common/constants/watch_states.js b/x-pack/plugins/watcher/common/constants/watch_states.js index f223e1bd8564e..1c546139b688c 100644 --- a/x-pack/plugins/watcher/common/constants/watch_states.js +++ b/x-pack/plugins/watcher/common/constants/watch_states.js @@ -24,5 +24,8 @@ export const WATCH_STATES = { defaultMessage: 'Error!' }), - CONFIG_ERROR: 'Config Error!', + CONFIG_ERROR: i18n.translate('xpack.watcher.constants.watchStates.configErrorStateText', { + defaultMessage: 'Config error' + }), + }; diff --git a/x-pack/plugins/watcher/public/models/action/slack_action.js b/x-pack/plugins/watcher/public/models/action/slack_action.js index c7552628dba3d..1ce75dc119674 100644 --- a/x-pack/plugins/watcher/public/models/action/slack_action.js +++ b/x-pack/plugins/watcher/public/models/action/slack_action.js @@ -23,9 +23,8 @@ export class SlackAction extends BaseAction { if (!this.to.length) { errors.push({ message: i18n.translate('xpack.watcher.sections.watchEdit.json.warningPossibleInvalidSlackAction.description', { - defaultMessage: `You are saving a watch with a "Slack" Action but you haven't defined a "to" field. - Unless you have specified a Slack message_default "to" property in your Elasticsearch settings, - this will result in an invalid watch.` + // eslint-disable-next-line max-len + defaultMessage: 'You are about to save a watch with a "Slack" action without a "to" property. Unless you have specified a "to" property in the Slack message_default setting in Elasticsearch, this will result in an invalid watch.' }) }); } diff --git a/x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js b/x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js index db8b3ab7f65fb..6668481f13fdb 100644 --- a/x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js +++ b/x-pack/plugins/watcher/public/models/watch_errors/watch_errors.js @@ -11,7 +11,7 @@ export class WatchErrors { this.actionErrors = get(props, 'actions'); } - static fromUpstreamJson(upstreamWatchStatus) { - return new WatchErrors(upstreamWatchStatus); + static fromUpstreamJson(upstreamWatchErrors) { + return new WatchErrors(upstreamWatchErrors); } } diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html index 2390582a11c5b..3cf5b7c5e6304 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.html @@ -42,11 +42,11 @@ {{ actionStatus.state }}
- -
+ +
{{actionStatusTable.getLabelErrors(actionStatus.id)}}
diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js index e986c47fb78c5..a4198a85bd5ab 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js @@ -9,7 +9,7 @@ import template from './action_status_table.html'; const app = uiModules.get('xpack/watcher'); -app.directive('actionStatusTable', function () { +app.directive('actionStatusTable', function ($injector, i18n) { return { restrict: 'E', replace: true, @@ -30,10 +30,15 @@ app.directive('actionStatusTable', function () { const errors = this.actionErrors[actionId]; const total = errors.length; - let label = `${total} error`; - if (total > 1) { - label += 's'; - } + const label = i18n('xpack.watcher.sections.watchDetail.actionStatusTotalErrors', { + defaultMessage: `{total, number} {total, plural, + one {error} + other {errors} + }`, + values: { + total, + } + }); return label; } diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js b/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js index 30599489c3975..4d3fdea3816e1 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_detail.js @@ -140,7 +140,7 @@ app.directive('watchDetail', function ($injector, i18n) { params: function () { return { title: i18n('xpack.watcher.sections.watchDetail.errorDisplayModalTitleText', { - defaultMessage: 'Errors in the "{actionId}" Action', + defaultMessage: 'Errors in the "{actionId}" action', values: { actionId } } ), errors, diff --git a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js index 4696838706694..f7a917b537a81 100644 --- a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js +++ b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js @@ -234,8 +234,7 @@ app.directive('jsonWatchEdit', function ($injector, i18n) { /** * Actions instances are not automatically added to the Watch _actions_ Array - * when we add them in the Json editor. - * This method takes takes care of it. + * when we add them in the Json editor. This method takes takes care of it. * * @param watchModel Watch instance * @return Watch instance From 5144dca7fdd11df35db50b9ffe1a80a15d22d394 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Thu, 25 Oct 2018 18:09:30 +0200 Subject: [PATCH 3/5] refactor(watcher): Update warning text for possible invalid watch --- .../watcher/public/models/action/slack_action.js | 2 +- .../plugins/watcher/public/models/watch/base_watch.js | 11 +++-------- .../components/json_watch_edit/json_watch_edit.js | 2 +- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/watcher/public/models/action/slack_action.js b/x-pack/plugins/watcher/public/models/action/slack_action.js index 1ce75dc119674..f94bc2f06b19d 100644 --- a/x-pack/plugins/watcher/public/models/action/slack_action.js +++ b/x-pack/plugins/watcher/public/models/action/slack_action.js @@ -24,7 +24,7 @@ export class SlackAction extends BaseAction { errors.push({ message: i18n.translate('xpack.watcher.sections.watchEdit.json.warningPossibleInvalidSlackAction.description', { // eslint-disable-next-line max-len - defaultMessage: 'You are about to save a watch with a "Slack" action without a "to" property. Unless you have specified a "to" property in the Slack message_default setting in Elasticsearch, this will result in an invalid watch.' + defaultMessage: 'This watch has a Slack action without a "to" property. This watch will only be valid if you specified the "to" property in the Slack "message_default" setting in Elasticsearch.' }) }); } diff --git a/x-pack/plugins/watcher/public/models/watch/base_watch.js b/x-pack/plugins/watcher/public/models/watch/base_watch.js index ca8149f58e73b..8363ebaf05cc3 100644 --- a/x-pack/plugins/watcher/public/models/watch/base_watch.js +++ b/x-pack/plugins/watcher/public/models/watch/base_watch.js @@ -159,22 +159,17 @@ export class BaseWatch { } // Concatenate their message - const errorActionsFragment = actionsErrors.reduce((message, error) => ( + const warningMessage = actionsErrors.reduce((message, error) => ( !!message ? `${message}, ${error.message}` : error.message ), ''); // We are not doing any *blocking* validation in the client, - // so for now we return the errors as a warning + // so we return the errors as a _warning_ return { warning: { - message: i18n.translate('xpack.watcher.models.baseWatch.invalidWatchWarningMessageText', { - defaultMessage: 'Warning: {errorActionsFragment} Are you sure you want to save the watch in its current state?', - values: { - errorActionsFragment, - } - }) + message: warningMessage, } }; } diff --git a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js index f7a917b537a81..725d2a186f324 100644 --- a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js +++ b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js @@ -165,7 +165,7 @@ app.directive('jsonWatchEdit', function ($injector, i18n) { const confirmModalOptions = { onConfirm: this.saveWatch, confirmButtonText: i18n('xpack.watcher.sections.watchEdit.json.watchErrorsWarning.confirmSaveWatch', { - defaultMessage: 'Yes', + defaultMessage: 'Save watch', }), }; From e675ea28fefab84af0537459fc0f63a1ac108018 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Thu, 25 Oct 2018 18:14:11 +0200 Subject: [PATCH 4/5] fix(watcher): Change template literals to string literals for i18n --- .../components/action_status_table/action_status_table.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js index a4198a85bd5ab..db058c107d225 100644 --- a/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js +++ b/x-pack/plugins/watcher/public/sections/watch_detail/components/action_status_table/action_status_table.js @@ -31,10 +31,7 @@ app.directive('actionStatusTable', function ($injector, i18n) { const total = errors.length; const label = i18n('xpack.watcher.sections.watchDetail.actionStatusTotalErrors', { - defaultMessage: `{total, number} {total, plural, - one {error} - other {errors} - }`, + defaultMessage: '{total, number} {total, plural, one {error} other {errors}}', values: { total, } From 1fe9b47dca0ff19ccbe00ccf2062277fe9260b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Mon, 29 Oct 2018 13:51:29 +0100 Subject: [PATCH 5/5] refactor(watcher): Make code review changes --- .../json_watch_edit/json_watch_edit.js | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js index 725d2a186f324..c78b57417200c 100644 --- a/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js +++ b/x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit/json_watch_edit.js @@ -104,13 +104,13 @@ app.directive('jsonWatchEdit', function ($injector, i18n) { this.createActionsForWatch(this.watch); if (!this.watch.isNew) { - return this.validateWatch(); + return this.validateAndSaveWatch(); } return this.isExistingWatch() .then(existingWatch => { if (!existingWatch) { - return this.validateWatch(); + return this.validateAndSaveWatch(); } const confirmModalOptions = { @@ -155,21 +155,21 @@ app.directive('jsonWatchEdit', function ($injector, i18n) { }); } - validateWatch = () => { + validateAndSaveWatch = () => { const { warning } = this.watch.validate(); - if (!warning) { - return this.saveWatch(); - } + if (warning) { + const confirmModalOptions = { + onConfirm: this.saveWatch, + confirmButtonText: i18n('xpack.watcher.sections.watchEdit.json.watchErrorsWarning.confirmSaveWatch', { + defaultMessage: 'Save watch', + }), + }; - const confirmModalOptions = { - onConfirm: this.saveWatch, - confirmButtonText: i18n('xpack.watcher.sections.watchEdit.json.watchErrorsWarning.confirmSaveWatch', { - defaultMessage: 'Save watch', - }), - }; + return confirmModal(warning.message, confirmModalOptions); + } - return confirmModal(warning.message, confirmModalOptions); + return this.saveWatch(); } saveWatch = () => {