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

add error callout on watch details page #36139

Merged
merged 2 commits into from
May 8, 2019
Merged

add error callout on watch details page #36139

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4154583
add error callout on watch details page
alisonelizabeth May 6, 2019
178429c
address review feedback
alisonelizabeth May 7, 2019
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
5 changes: 3 additions & 2 deletions x-pack/plugins/watcher/common/constants/pagination.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

export const PAGINATION: { [key: string]: number } = {
PAGE_SIZE: 20,
export const PAGINATION: { initialPageSize: number; pageSizeOptions: number[] } = {
initialPageSize: 10,
pageSizeOptions: [10, 50, 100],
};
9 changes: 2 additions & 7 deletions x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { Moment } from 'moment';
import chrome from 'ui/chrome';
import { MANAGEMENT_BREADCRUMB } from 'ui/management';

import { REFRESH_INTERVALS } from '../../../../common/constants';
import { REFRESH_INTERVALS, PAGINATION } from '../../../../common/constants';
import { listBreadcrumb } from '../../../lib/breadcrumbs';
import { getPageErrorCode, PageError, DeleteWatchesModal, WatchStatus } from '../../../components';
import { loadWatches } from '../../../lib/api';
Expand Down Expand Up @@ -160,11 +160,6 @@ const WatchListUi = ({ intl }: { intl: InjectedIntl }) => {
onSelectionChange: setSelection,
};

const pagination = {
initialPageSize: 10,
pageSizeOptions: [10, 50, 100],
};

const searchConfig = {
box: {
incremental: true,
Expand Down Expand Up @@ -277,7 +272,7 @@ const WatchListUi = ({ intl }: { intl: InjectedIntl }) => {
itemId="id"
columns={columns}
search={searchConfig}
pagination={pagination}
pagination={PAGINATION}
sorting={true}
selection={selectionConfig}
isSelectable={true}
Expand Down
39 changes: 33 additions & 6 deletions x-pack/plugins/watcher/public/sections/watch_status/components/watch_detail.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,20 @@ import {
EuiTitle,
EuiButtonEmpty,
EuiToolTip,
EuiCallOut,
} from '@elastic/eui';
import { loadWatchDetail, ackWatchAction } from '../../../lib/api';
import { getPageErrorCode, WatchStatus } from '../../../components';
import { PAGINATION } from '../../../../common/constants';

const WatchDetailUi = ({ watchId }: { watchId: string }) => {
const pagination = {
initialPageSize: 10,
pageSizeOptions: [10, 50, 100],
};

const { error, data: watchDetail, isLoading } = loadWatchDetail(watchId);

const [actionStatuses, setActionStatuses] = useState<any[]>([]);
const [isActionStatusLoading, setIsActionStatusLoading] = useState<boolean>(false);

const actionErrors = watchDetail && watchDetail.watchErrors.actionErrors;

useEffect(
() => {
if (watchDetail) {
Expand Down Expand Up @@ -134,6 +133,34 @@ const WatchDetailUi = ({ watchId }: { watchId: string }) => {

<EuiSpacer size="s" />

{actionErrors && (
<EuiCallOut
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice call! I think this is a big improvement over the modal. Is it possible for there to be a lot of actions, and they could all have errors? In this case, the callout would become very tall, and it would be difficult to correlate an error in the callout with a row in the actions table.

If you agree, then I think a more usable solution would be to make each "Config error" status a link which, when clicked, opens a flyout containing a danger callout like this one, except it only contains the errors for that specific action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought of that as well. That's a good point.

Latest changes:

Screen Shot 2019-05-06 at 10 40 49 PM

Screen Shot 2019-05-06 at 10 40 58 PM

title={i18n.translate('xpack.watcher.sections.watchDetail.actionErrorsCalloutTitle', {
defaultMessage: 'This watch contains action errors.',
})}
color="danger"
iconType="cross"
>
{Object.keys(actionErrors).map((action: string) => (
<Fragment key={action}>
<EuiText size="xs">
Copy link
Contributor

@cjcenizal cjcenizal May 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If we go with the flyout solution, we can put this content in the title of the callout.

image

You could even add logic to wrap the error message in a <p> if there's only one error, or to use a list if there's more than one. This would avoid the strange appearance of a single bullet.

<h4>{action}</h4>
<ul>
{actionErrors[action].map(
(actionError: { message: string }, errorIndex: number) => (
<li key={`action-error-${errorIndex}`}>{actionError.message}</li>
)
)}
</ul>
</EuiText>
<EuiSpacer size="s" />
</Fragment>
))}
</EuiCallOut>
)}

<EuiSpacer size="s" />

<EuiTitle size="s">
<h2>
<FormattedMessage
Expand All @@ -149,7 +176,7 @@ const WatchDetailUi = ({ watchId }: { watchId: string }) => {
items={actionStatuses}
itemId="id"
columns={columns}
pagination={pagination}
pagination={PAGINATION}
sorting={true}
loading={isLoading}
message={
Expand Down
8 changes: 2 additions & 6 deletions x-pack/plugins/watcher/public/sections/watch_status/components/watch_history.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
EuiTitle,
} from '@elastic/eui';

import { PAGINATION } from '../../../../common/constants';
import { goToWatchList } from '../../../lib/navigation';
import { getPageErrorCode, PageError, WatchStatus, DeleteWatchesModal } from '../../../components';
import {
Expand Down Expand Up @@ -109,11 +110,6 @@ const WatchHistoryUi = ({ intl, watchId }: { intl: InjectedIntl; watchId: string
return <PageError errorCode={errorCode} id={watchId} />;
}

const pagination = {
initialPageSize: 10,
pageSizeOptions: [10, 50, 100],
};

const columns = [
{
field: 'startTime',
Expand Down Expand Up @@ -347,7 +343,7 @@ const WatchHistoryUi = ({ intl, watchId }: { intl: InjectedIntl; watchId: string
<EuiInMemoryTable
items={history}
columns={columns}
pagination={pagination}
pagination={PAGINATION}
sorting={true}
loading={isLoading}
message={
Expand Down
48 changes: 43 additions & 5 deletions x-pack/plugins/watcher/server/models/action/jira_action.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { BaseAction } from './base_action';
import { ACTION_TYPES, ERROR_CODES } from '../../../common/constants';
import { i18n } from '@kbn/i18n';
import { get } from 'lodash';

export class JiraAction extends BaseAction {
constructor(props, errors) {
Expand Down Expand Up @@ -72,9 +73,9 @@ export class JiraAction extends BaseAction {
const { errors } = this.validateJson(json.actionJson);

Object.assign(props, {
projectKey: json.actionJson.jira.fields.project.key,
issueType: json.actionJson.jira.fields.issuetype.name,
summary: json.actionJson.jira.fields.summary,
projectKey: get(json, 'actionJson.jira.fields.project.key'),
issueType: get(json, 'actionJson.jira.fields.issuetype.name'),
summary: get(json, 'actionJson.jira.fields.summary'),
});

const action = new JiraAction(props, errors);
Expand All @@ -98,7 +99,32 @@ export class JiraAction extends BaseAction {
json.jira = {};
}

if (!json.jira.fields.project.key) {
if (!json.jira.fields) {
errors.push({
code: ERROR_CODES.ERR_PROP_MISSING,
message: i18n.translate('xpack.watcher.models.jiraAction.actionJsonJiraFieldsPropertyMissingBadRequestMessage', {
defaultMessage: 'json argument must contain an {actionJsonJiraFieldsKey} property',
values: {
actionJsonJiraProjectKey: 'actionJson.jira.fields'
}
}),
});
return { errors };
}

if (!json.jira.fields.project) {
errors.push({
code: ERROR_CODES.ERR_PROP_MISSING,
message: i18n.translate('xpack.watcher.models.jiraAction.actionJsonJiraProjectPropertyMissingBadRequestMessage', {
defaultMessage: 'json argument must contain an {actionJsonJiraProject} property',
values: {
actionJsonJiraProject: 'actionJson.jira.fields.project'
}
}),
});
}

if (json.jira.fields.project && !json.jira.fields.project.key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine a user getting frustrated if they fix json.jira.fields only to be yelled at for not having json.jira.fields.project, and then fix json.jira.fields.project only to be yelled at for not having json.jira.fields.project.key. I think it might be a better UX if we just check for get(json, 'actionJson.jira.fields.project.key') and then tell them they need json.jira.fields.project.key. WDYT?

This comment applies to the issuetype validation below, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point :) Fixed!

errors.push({
code: ERROR_CODES.ERR_PROP_MISSING,
message: i18n.translate('xpack.watcher.models.jiraAction.actionJsonJiraProjectKeyPropertyMissingBadRequestMessage', {
Expand All @@ -110,7 +136,19 @@ export class JiraAction extends BaseAction {
});
}

if (!json.jira.fields.issuetype.name) {
if (!json.jira.fields.issuetype) {
errors.push({
code: ERROR_CODES.ERR_PROP_MISSING,
message: i18n.translate('xpack.watcher.models.jiraAction.actionJsonJiraIssueTypePropertyMissingBadRequestMessage', {
defaultMessage: 'json argument must contain an {actionJsonJiraIssueTypeKey} property',
values: {
actionJsonJiraIssueTypeKey: 'actionJson.jira.fields.issuetype'
}
}),
});
}

if (json.jira.fields.issuetype && !json.jira.fields.issuetype.name) {
errors.push({
code: ERROR_CODES.ERR_PROP_MISSING,
message: i18n.translate('xpack.watcher.models.jiraAction.actionJsonJiraIssueTypePropertyMissingBadRequestMessage', {
Expand Down