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
loixlab 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 8 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…
Jan 22, 2019
ff69183
Add missing "type" property to FormEntryRow
Jan 23, 2019
7cd920c
Fix translation display in remote cluster dropdown
Jan 28, 2019
6442d95
Fix wrong endpoint for auto-follow stats + show api server error in a…
Jan 28, 2019
c4147f8
Improve UX when remote cluster api requests fail
Jan 28, 2019
3bda0df
Move header follower indices & auto-follow patterns header to their l…
Jan 28, 2019
79b3590
Refactor Remote clusters list component for better errors handling
Jan 28, 2019
9f35f2a
Fix empty prompt in follower indices & auto-follow patterns list
Jan 28, 2019
3cc75ea
Merge branch 'feature/ccr' into fix/ccr_bugs
Jan 29, 2019
867c1c3
Small refactor
Jan 29, 2019
a0c2b92
Merge branch 'feature/ccr' into fix/ccr_bugs
Jan 30, 2019
9a30779
Update <RemoteClusterList /> test snapshot
Jan 30, 2019
c223154
Fix regression in Auto-follow pattern list header
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
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
10 changes: 9 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,15 @@ 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 ? parseInt(value, 10) : value; // make sure we don't send NaN value

@loixlab loixlab Jan 28, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This would have been better with a Math.max(0, parseInt(value, 10)) and removing the next line. I'll update it.

valueParsed = valueParsed && valueParsed < 0 ? 0 : valueParsed; // make sure we don't send negative value
}

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) {

@cjcenizal cjcenizal Jan 29, 2019

Copy link
Copy Markdown
Contributor

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?

@loixlab loixlab Jan 29, 2019

Copy link
Copy Markdown
Contributor Author

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
102 changes: 82 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,71 @@ 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

@cjcenizal cjcenizal Jan 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@loixlab loixlab Jan 30, 2019

Copy link
Copy Markdown
Contributor Author

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.followerIndexList.followerIndicesDescription"
defaultMessage="Followers replicate operations from the leader index to the follower index."
/>
</p>
</EuiText>
</EuiFlexItem>

<EuiFlexItem grow={false}>
{isAuthorized && (
<EuiButton
{...routing.getRouterLinkProps('/follower_indices/add')}
fill
iconType="plusInCircle"
>
<FormattedMessage
id="xpack.crossClusterReplication.followerIndexList.addFollowerButtonLabel"
defaultMessage="Create a follower index"
/>
</EuiButton>
)}
</EuiFlexItem>
</EuiFlexGroup>

<EuiSpacer />
</Fragment>
);
}

renderContent(isEmpty) {
const { apiError, isAuthorized, intl } = this.props;
if (!isAuthorized) {
return (
<SectionUnauthorized>
<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 +228,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();
}
const { autoFollowPatterns, apiStatus, } = this.props;
const isEmpty = apiStatus === API_STATUS.IDLE && !autoFollowPatterns.length;

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

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