Skip to content

[CCR] Improve form error handling and general UX #29419

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

Merged
merged 13 commits into from
Jan 30, 2019
Merged

[CCR] Improve form error handling and general UX #29419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
83d6fd4
Avoid parsing empty string to integer + don't allow negative value fo…
sebelga Jan 22, 2019
ff69183
Add missing "type" property to FormEntryRow
sebelga Jan 23, 2019
7cd920c
Fix translation display in remote cluster dropdown
sebelga Jan 28, 2019
6442d95
Fix wrong endpoint for auto-follow stats + show api server error in a…
sebelga Jan 28, 2019
c4147f8
Improve UX when remote cluster api requests fail
sebelga Jan 28, 2019
3bda0df
Move header follower indices & auto-follow patterns header to their l…
sebelga Jan 28, 2019
79b3590
Refactor Remote clusters list component for better errors handling
sebelga Jan 28, 2019
9f35f2a
Fix empty prompt in follower indices & auto-follow patterns list
sebelga Jan 28, 2019
3cc75ea
Merge branch 'feature/ccr' into fix/ccr_bugs
sebelga Jan 29, 2019
867c1c3
Small refactor
sebelga Jan 29, 2019
a0c2b92
Merge branch 'feature/ccr' into fix/ccr_bugs
sebelga Jan 30, 2019
9a30779
Update <RemoteClusterList /> test snapshot
sebelga Jan 30, 2019
c223154
Fix regression in Auto-follow pattern list header
sebelga Jan 30, 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
2 changes: 1 addition & 1 deletion src/server/sass/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import fs from 'fs';
import sass from 'node-sass';
import autoprefixer from 'autoprefixer';
import postcss from 'postcss';
import postcssUrl from 'postcss-url';
import postcssUrl from 'postcss-url'; // eslint-disable-line import/no-unresolved
import mkdirp from 'mkdirp';
import chalk from 'chalk';
import isPathInside from 'is-path-inside';
Expand Down
3 changes: 2 additions & 1 deletion ...ross_cluster_replication/public/app/components/follower_index_form/follower_index_form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ export const FollowerIndexForm = injectI18n(
<Fragment>
<EuiSpacer size="s"/>
{advancedSettingsFields.map((advancedSetting) => {
const { field, title, description, label, helpText, defaultValue } = advancedSetting;
const { field, title, description, label, helpText, defaultValue, type } = advancedSetting;
return (
<FormEntryRow
key={field}
Expand All @@ -539,6 +539,7 @@ export const FollowerIndexForm = injectI18n(
)}
label={label}
helpText={helpText}
type={type}
areErrorsVisible={areErrorsVisible}
onValueUpdate={this.onFieldsChange}
/>
Expand Down
9 changes: 8 additions & 1 deletion x-pack/plugins/cross_cluster_replication/public/app/components/form_entry_row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,14 @@ export class FormEntryRow extends PureComponent {
onFieldChange = (value) => {
const { field, onValueUpdate, type } = this.props;
const isNumber = type === 'number';
onValueUpdate({ [field]: isNumber ? parseInt(value, 10) : value });

let valueParsed = value;

if (isNumber) {
valueParsed = !!value ? Math.max(0, parseInt(value, 10)) : value; // make sure we don't send NaN value or a negative number
}

onValueUpdate({ [field]: valueParsed });
}

renderField = (isInvalid) => {
Expand Down
11 changes: 4 additions & 7 deletions x-pack/plugins/cross_cluster_replication/public/app/components/remote_clusters_form_field.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,10 @@ export const RemoteClustersFormField = injectI18n(
const hasClusters = Boolean(remoteClusters.length);
const remoteClustersOptions = hasClusters ? remoteClusters.map(({ name, isConnected }) => ({
value: name,
text: isConnected ? name : (
<FormattedMessage
id="xpack.crossClusterReplication.forms.remoteClusterDropdownNotConnected"
defaultMessage="{name} (not connected)"
values={{ name }}
/>
),
text: isConnected ? name : this.props.intl.formatMessage({
id: 'xpack.crossClusterReplication.forms.remoteClusterDropdownNotConnected',
defaultMessage: '{name} (not connected)',
}, { name }),
'data-test-subj': `option-${name}`
})) : [];
const errorMessage = this.renderErrorMessage();
Expand Down
4 changes: 2 additions & 2 deletions ...lication/public/app/sections/auto_follow_pattern_add/auto_follow_pattern_add.container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import { AutoFollowPatternAdd as AutoFollowPatternAddView } from './auto_follow_
const scope = SECTIONS.AUTO_FOLLOW_PATTERN;

const mapStateToProps = (state) => ({
apiStatus: getApiStatus(scope)(state),
apiError: getApiError(scope)(state),
apiStatus: getApiStatus(`${scope}-save`)(state),
apiError: getApiError(`${scope}-save`)(state),
});

const mapDispatchToProps = dispatch => ({
Expand Down
13 changes: 2 additions & 11 deletions ...luster_replication/public/app/sections/auto_follow_pattern_add/auto_follow_pattern_add.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
AutoFollowPatternPageTitle,
RemoteClustersProvider,
SectionLoading,
SectionError,
} from '../../components';

export const AutoFollowPatternAdd = injectI18n(
Expand All @@ -41,7 +40,7 @@ export const AutoFollowPatternAdd = injectI18n(
}

render() {
const { saveAutoFollowPattern, apiStatus, apiError, intl, match: { url: currentUrl } } = this.props;
const { saveAutoFollowPattern, apiStatus, apiError, match: { url: currentUrl } } = this.props;

return (
<EuiPageContent>
Expand All @@ -67,20 +66,12 @@ export const AutoFollowPatternAdd = injectI18n(
);
}

if (error) {
const title = intl.formatMessage({
id: 'xpack.crossClusterReplication.autoFollowPatternCreateForm.loadingRemoteClustersErrorTitle',
defaultMessage: 'Error loading remote clusters',
});
return <SectionError title={title} error={error} />;
}

return (
<AutoFollowPatternForm
apiStatus={apiStatus}
apiError={apiError}
currentUrl={currentUrl}
remoteClusters={remoteClusters}
remoteClusters={error ? [] : remoteClusters}
saveAutoFollowPattern={saveAutoFollowPattern}
/>
);
Expand Down
12 changes: 2 additions & 10 deletions ...ster_replication/public/app/sections/auto_follow_pattern_edit/auto_follow_pattern_edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export const AutoFollowPatternEdit = injectI18n(
}

render() {
const { saveAutoFollowPattern, apiStatus, apiError, autoFollowPattern, intl, match: { url: currentUrl } } = this.props;
const { saveAutoFollowPattern, apiStatus, apiError, autoFollowPattern, match: { url: currentUrl } } = this.props;

return (
<EuiPageContent
Expand Down Expand Up @@ -154,20 +154,12 @@ export const AutoFollowPatternEdit = injectI18n(
);
}

if (error) {
const title = intl.formatMessage({
id: 'xpack.crossClusterReplication.autoFollowPatternEditForm.loadingRemoteClustersErrorTitle',
defaultMessage: 'Error loading remote clusters',
});
return <SectionError title={title} error={error} />;
}

return (
<AutoFollowPatternForm
apiStatus={apiStatus.save}
apiError={apiError.save}
currentUrl={currentUrl}
remoteClusters={remoteClusters}
remoteClusters={error ? [] : remoteClusters}
autoFollowPattern={autoFollowPattern}
saveAutoFollowPattern={saveAutoFollowPattern}
/>
Expand Down
13 changes: 2 additions & 11 deletions ...ns/cross_cluster_replication/public/app/sections/follower_index_add/follower_index_add.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
FollowerIndexPageTitle,
RemoteClustersProvider,
SectionLoading,
SectionError,
} from '../../components';

export const FollowerIndexAdd = injectI18n(
Expand All @@ -41,7 +40,7 @@ export const FollowerIndexAdd = injectI18n(
}

render() {
const { saveFollowerIndex, clearApiError, apiStatus, apiError, intl, match: { url: currentUrl } } = this.props;
const { saveFollowerIndex, clearApiError, apiStatus, apiError, match: { url: currentUrl } } = this.props;

return (
<EuiPageContent
Expand Down Expand Up @@ -70,20 +69,12 @@ export const FollowerIndexAdd = injectI18n(
);
}

if (error) {
Copy link
Contributor

@cjcenizal cjcenizal Jan 29, 2019

Choose a reason for hiding this comment

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

With this removed, we now show a prompt to the user when there's an error:

image

I'm a bit on the fence about this because it seems inaccurate and could possibly seem "deceptive" to the user. Imagine clicking the "Add remote cluster" button, completing the action, and then being redirected back here only to see this prompt again. At that point I'd be pretty surprised and confused -- there's something wrong but I wouldn't know what. What are your thoughts on this UX?

Copy link
Contributor Author

@sebelga sebelga Jan 29, 2019

Choose a reason for hiding this comment

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

Well, the previous UX was: the user enters follower indices, he clicks on "Add a follower index" and instead of accessing a form he had a red callout saying that the remote clusters could not be loaded. Unless he looks at the documentation and knows about the remote cluster field, that error message was not very helpful for the user, blocking him from accessing the form.

Also, if the list of clusters can't be loaded, he probably won't be able either to add a remote cluster. So that would help him understand better this message here in the add follower index form.

const title = intl.formatMessage({
id: 'xpack.crossClusterReplication.followerIndexCreateForm.loadingRemoteClustersErrorTitle',
defaultMessage: 'Error loading remote clusters',
});
return <SectionError title={title} error={error} />;
}

return (
<FollowerIndexForm
apiStatus={apiStatus}
apiError={apiError}
currentUrl={currentUrl}
remoteClusters={remoteClusters}
remoteClusters={error ? [] : remoteClusters}
saveFollowerIndex={saveFollowerIndex}
clearApiError={clearApiError}
/>
Expand Down
6 changes: 1 addition & 5 deletions .../cross_cluster_replication/public/app/sections/follower_index_edit/follower_index_edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,17 +242,13 @@ export const FollowerIndexEdit = injectI18n(
);
}

if (error) {
remoteClusters = [];
}

return (
<FollowerIndexForm
followerIndex={rest}
apiStatus={apiStatus.save}
apiError={apiError.save}
currentUrl={currentUrl}
remoteClusters={remoteClusters}
remoteClusters={error ? [] : remoteClusters}
saveFollowerIndex={this.saveFollowerIndex}
clearApiError={clearApiError}
/>
Expand Down
110 changes: 90 additions & 20 deletions ...replication/public/app/sections/home/auto_follow_pattern_list/auto_follow_pattern_list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,19 @@
import React, { PureComponent, Fragment } from 'react';
import PropTypes from 'prop-types';
import { injectI18n, FormattedMessage } from '@kbn/i18n/react';
import { EuiButton, EuiEmptyPrompt } from '@elastic/eui';
import {
EuiButton,
EuiEmptyPrompt,
EuiFlexGroup,
EuiFlexItem,
EuiText,
EuiSpacer,
} from '@elastic/eui';

import routing from '../../../services/routing';
import { extractQueryParams } from '../../../services/query_params';
import { API_STATUS } from '../../../constants';
import { SectionLoading, SectionError } from '../../../components';
import { SectionLoading, SectionError, SectionUnauthorized } from '../../../components';
import { AutoFollowPatternTable, DetailPanel } from './components';

const REFRESH_RATE_MS = 30000;
Expand Down Expand Up @@ -88,6 +95,79 @@ export const AutoFollowPatternList = injectI18n(
clearInterval(this.interval);
}

renderHeader() {
const { isAuthorized } = this.props;
return (
<Fragment>
<EuiFlexGroup justifyContent="spaceBetween" alignItems="flexStart">
<EuiFlexItem grow={false}>
<EuiText>
<p>
<FormattedMessage
Copy link
Contributor

@cjcenizal cjcenizal Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like the wrong content was copied here. It should be this (note the button is incorrect too):

image

Copy link
Contributor Author

@sebelga sebelga Jan 30, 2019

Choose a reason for hiding this comment

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

Good catch!

id="xpack.crossClusterReplication.autoFollowPatternList.autoFollowPatternsDescription"
defaultMessage="Auto-follow patterns replicate leader indices from a remote
cluster to follower indices on the local cluster."
/>
</p>
</EuiText>
</EuiFlexItem>

<EuiFlexItem grow={false}>
{isAuthorized && (
<EuiButton
{...routing.getRouterLinkProps('/auto_follow_patterns/add')}
fill
iconType="plusInCircle"
>
<FormattedMessage
id="xpack.crossClusterReplication.autoFollowPatternList.addAutoFollowPatternButtonLabel"
defaultMessage="Create an auto-follow pattern"
/>
</EuiButton>
)}
</EuiFlexItem>
</EuiFlexGroup>

<EuiSpacer />
</Fragment>
);
}

renderContent(isEmpty) {
const { apiError, isAuthorized, intl } = this.props;
if (!isAuthorized) {
return (
<SectionUnauthorized
title={(
<FormattedMessage
id="xpack.crossClusterReplication.autoFollowPatternList.permissionErrorTitle"
defaultMessage="Permission error"
/>
)}
>
<FormattedMessage
id="xpack.crossClusterReplication.autoFollowPatternList.noPermissionText"
defaultMessage="You do not have permission to view or add auto-follow patterns."
/>
</SectionUnauthorized>
);
}

if (apiError) {
const title = intl.formatMessage({
id: 'xpack.crossClusterReplication.autoFollowPatternList.loadingErrorTitle',
defaultMessage: 'Error loading auto-follow patterns',
});
return <SectionError title={title} error={apiError} />;
}

if (isEmpty) {
return this.renderEmpty();
}

return this.renderList();
}

renderEmpty() {
return (
<EuiEmptyPrompt
Expand Down Expand Up @@ -156,25 +236,15 @@ export const AutoFollowPatternList = injectI18n(
}

render() {
const { autoFollowPatterns, apiStatus, apiError, isAuthorized, intl } = this.props;

if (!isAuthorized) {
return null;
}

if (apiStatus === API_STATUS.IDLE && !autoFollowPatterns.length) {
return this.renderEmpty();
}

if (apiError) {
const title = intl.formatMessage({
id: 'xpack.crossClusterReplication.autoFollowPatternList.loadingErrorTitle',
defaultMessage: 'Error loading auto-follow patterns',
});
return <SectionError title={title} error={apiError} />;
}
const { autoFollowPatterns, apiStatus, } = this.props;
const isEmpty = apiStatus === API_STATUS.IDLE && !autoFollowPatterns.length;

return this.renderList();
return (
<Fragment>
{!isEmpty && this.renderHeader()}
{this.renderContent(isEmpty)}
</Fragment>
);
}
}
);
Loading