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.

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

[Snapshot + Restore] Add callout when restoring snapshot #95104

Merged
merged 7 commits into from
Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions src/core/public/doc_links/doc_links_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ export class DocLinksService {
registerSourceOnly: `${ELASTICSEARCH_DOCS}snapshots-register-repository.html#snapshots-source-only-repository`,
registerUrl: `${ELASTICSEARCH_DOCS}snapshots-register-repository.html#snapshots-read-only-repository`,
restoreSnapshot: `${ELASTICSEARCH_DOCS}snapshots-restore-snapshot.html`,
restoreSnapshotApi: `${ELASTICSEARCH_DOCS}restore-snapshot-api.html#restore-snapshot-api-request-body`,
},
ingest: {
pipelines: `${ELASTICSEARCH_DOCS}ingest.html`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { act } from 'react-dom/test-utils';

import { registerTestBed, TestBed, TestBedConfig } from '@kbn/test/jest';
import { RestoreSnapshot } from '../../../public/application/sections/restore_snapshot';
Expand All @@ -23,11 +24,19 @@ const initTestBed = registerTestBed<RestoreSnapshotFormTestSubject>(
);

const setupActions = (testBed: TestBed<RestoreSnapshotFormTestSubject>) => {
const { find } = testBed;
const { find, component, form } = testBed;
return {
findDataStreamCallout() {
return find('dataStreamWarningCallOut');
},

toggleGlobalState() {
act(() => {
form.toggleEuiSwitch('includeGlobalStateSwitch');
});

component.update();
},
};
};

Expand All @@ -48,4 +57,6 @@ export const setup = async (): Promise<RestoreSnapshotTestBed> => {

export type RestoreSnapshotFormTestSubject =
| 'snapshotRestoreStepLogistics'
| 'includeGlobalStateSwitch'
| 'systemIndicesInfoCallOut'
| 'dataStreamWarningCallOut';
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { act } from 'react-dom/test-utils';

import { nextTick, pageHelpers, setupEnvironment } from './helpers';
import { pageHelpers, setupEnvironment } from './helpers';
import { RestoreSnapshotTestBed } from './helpers/restore_snapshot.helpers';
import * as fixtures from '../../test/fixtures';

Expand All @@ -20,11 +21,15 @@ describe('<RestoreSnapshot />', () => {
afterAll(() => {
server.restore();
});

describe('with data streams', () => {
beforeEach(async () => {
httpRequestsMockHelpers.setGetSnapshotResponse(fixtures.getSnapshot());
testBed = await setup();
await nextTick();

await act(async () => {
testBed = await setup();
});

testBed.component.update();
});

Expand All @@ -37,8 +42,10 @@ describe('<RestoreSnapshot />', () => {
describe('without data streams', () => {
beforeEach(async () => {
httpRequestsMockHelpers.setGetSnapshotResponse(fixtures.getSnapshot({ totalDataStreams: 0 }));
testBed = await setup();
await nextTick();
await act(async () => {
testBed = await setup();
});

testBed.component.update();
});

Expand All @@ -47,4 +54,25 @@ describe('<RestoreSnapshot />', () => {
expect(exists('dataStreamWarningCallOut')).toBe(false);
});
});

describe('global state', () => {
beforeEach(async () => {
httpRequestsMockHelpers.setGetSnapshotResponse(fixtures.getSnapshot());
await act(async () => {
testBed = await setup();
});

testBed.component.update();
});

it('shows an info callout when include_global_state is enabled', () => {
const { exists, actions } = testBed;

expect(exists('systemIndicesInfoCallOut')).toBe(false);

actions.toggleGlobalState();

expect(exists('systemIndicesInfoCallOut')).toBe(true);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ export const PolicyStepSettings: React.FunctionComponent<StepProps> = ({
description={
<FormattedMessage
id="xpack.snapshotRestore.policyForm.stepSettings.includeGlobalStateDescription"
defaultMessage="Stores the global cluster state as part of the snapshot."
defaultMessage="Stores the global cluster state and system indices as part of the snapshot."
/>
}
fullWidth
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import React, { Fragment, useState } from 'react';
import { FormattedMessage } from '@kbn/i18n/react';
import semverGt from 'semver/functions/gt';
import {
EuiButtonEmpty,
EuiDescribedFormGroup,
Expand Down Expand Up @@ -38,6 +39,8 @@ import { DataStreamsGlobalStateCallOut } from './data_streams_global_state_call_

import { DataStreamsAndIndicesListHelpText } from './data_streams_and_indices_list_help_text';

import { SystemIndicesOverwrittenCallOut } from './system_indices_overwritten_callout';

export const RestoreSnapshotStepLogistics: React.FunctionComponent<StepProps> = ({
snapshotDetails,
restoreSettings,
Expand All @@ -50,6 +53,7 @@ export const RestoreSnapshotStepLogistics: React.FunctionComponent<StepProps> =
indices: unfilteredSnapshotIndices,
dataStreams: snapshotDataStreams = [],
includeGlobalState: snapshotIncludeGlobalState,
version,
} = snapshotDetails;

const snapshotIndices = unfilteredSnapshotIndices.filter(
Expand Down Expand Up @@ -564,11 +568,24 @@ export const RestoreSnapshotStepLogistics: React.FunctionComponent<StepProps> =
</EuiTitle>
}
description={
<FormattedMessage
id="xpack.snapshotRestore.restoreForm.stepLogistics.includeGlobalStateDescription"
defaultMessage="Restores templates that don’t currently exist in the cluster and overrides
templates with the same name. Also restores persistent settings."
/>
<>
<FormattedMessage
id="xpack.snapshotRestore.restoreForm.stepLogistics.includeGlobalStateDescription"
defaultMessage="Restores templates that don’t currently exist in the cluster and overrides
templates with the same name. Also restores persistent settings and all system indices."
/>

{/* Only display callout if include_global_state is enabled and the snapshot was created by ES 7.12+
* Note: Once we support features states in the UI, we will also need to add a check here for that
* See https://github.com/elastic/kibana/issues/95128 more details
*/}
{includeGlobalState && semverGt(version, '7.12.0') && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the explanation about features states, it's very helpful to understand the code

<>
<EuiSpacer size="xs" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This spacing looked a little tight to me in the screenshot. Might be worth checking with Michael?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdefazio would you mind taking a look? Screenshot in PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just based on looking at the screenshot, here's a rough guideline: (Spacing is tight, but we want to make sure there's more space between the button and the callout).

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed to spacing from xs --> s above the callout, and from l --> xl between the callout and the button. Let me know what you think. @mdefazio I'm not sure if you made a mistake when you said to "drop" 1-2 sizes in the screenshot, when in the original comment you said there should be more space between the button and callout. It's possible I misunderstood the screenshot though 😅 .

Screen Shot 2021-03-23 at 10 04 55 PM

Another idea - we could simplify the callout and only have a title. Something like this (not committed):
Screen Shot 2021-03-23 at 10 06 28 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

My apologies for the poor direction 🤦 . I was simply trying to say that yes, there should be more space between the text and the callout, and it should be a bit less than the space between the button and the callout. So I didn't mean to say you had to change the space between the button and the callout (but the new screenshots look fine)

I like the callout with only the title. It's a bit less obtrusive and feels more straightforward.

<SystemIndicesOverwrittenCallOut />
</>
)}
</>
}
fullWidth
>
Expand All @@ -594,6 +611,7 @@ export const RestoreSnapshotStepLogistics: React.FunctionComponent<StepProps> =
checked={includeGlobalState === undefined ? false : includeGlobalState}
onChange={(e) => updateRestoreSettings({ includeGlobalState: e.target.checked })}
disabled={!snapshotIncludeGlobalState}
data-test-subj="includeGlobalStateSwitch"
/>
</EuiFormRow>
</EuiDescribedFormGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import React, { FunctionComponent } from 'react';
import { EuiCallOut, EuiLink } from '@elastic/eui';

import { useCore } from '../../../../app_context';

export const SystemIndicesOverwrittenCallOut: FunctionComponent = () => {
const { docLinks } = useCore();

return (
<EuiCallOut
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about accessibility, I wonder if screen-readers announce the content of this callout when the user enables the global state toggle? If not then I think we should consider rendering the callout even when the toggle is disabled. If the callout is too prominent to show at all times, maybe we could just present it as standard description text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing this up! I've added an aria-live attribute so that it will be read by the screenreader when it is rendered.

If not then I think we should consider rendering the callout even when the toggle is disabled. If the callout is too prominent to show at all times, maybe we could just present it as standard description text.

If there is a general concern that the callout is too prominent, I've also included an example of just using the title in the callout: #95104 (comment). Adding it as a standard description text is a possibility as well.

data-test-subj="systemIndicesInfoCallOut"
title={i18n.translate(
'xpack.snapshotRestore.restoreForm.stepLogistics.systemIndicesCallOut.title',
{
defaultMessage: 'System indices will be overwritten',
}
)}
iconType="pin"
size="s"
>
<FormattedMessage
id="xpack.snapshotRestore.restoreForm.stepLogistics.systemIndicesCallOut.description"
defaultMessage="When this snapshot is restored, system indices will be overwritten with data from the snapshot. {learnMoreLink}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original recommended text also included: ... If this is not what you want, set include_global_state to false. This seems self-explanatory as the message only appears in the UI when global state is enabled, so I did not include it. Adding a comment here though in case anyone thinks it would be helpful to add back.

values={{
learnMoreLink: (
<EuiLink target="_blank" href={docLinks.links.snapshotRestore.restoreSnapshotApi}>
{i18n.translate(
'xpack.snapshotRestore.restoreForm.stepLogistics.systemIndicesCallOut.learnMoreLink',
{ defaultMessage: 'Learn more' }
)}
</EuiLink>
),
}}
/>
</EuiCallOut>
);
};