From 08bb71df6323ed35753dc9470ae5edc9b76a56a7 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Wed, 9 Sep 2020 15:10:15 +0200 Subject: [PATCH] [Snapshot & Restore] fix pre existing policy with no existing repository (#76861) * implement fix and add callout with copy * added test * make callout a danger callout and revise copy * block the form if we have a repo, but it does not exist * added test to assert that form wizard blocks on validation for not found repo * fix types and add a doc comment * move callout to above the form Co-authored-by: Elastic Machine --- .../helpers/policy_form.helpers.ts | 2 + .../client_integration/policy_add.test.ts | 5 ++ .../client_integration/policy_edit.test.ts | 38 +++++++++++++++ .../policy_form/steps/step_logistics.tsx | 46 ++++++++++++++++++- .../services/validation/validate_policy.ts | 16 ++++++- 5 files changed, 103 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/policy_form.helpers.ts b/x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/policy_form.helpers.ts index a3ab829ab642c..ab2963223f678 100644 --- a/x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/policy_form.helpers.ts +++ b/x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/policy_form.helpers.ts @@ -56,4 +56,6 @@ export type PolicyFormTestSubjects = | 'showAdvancedCronLink' | 'snapshotNameInput' | 'dataStreamBadge' + | 'repositoryNotFoundWarning' + | 'repositorySelect' | 'submitButton'; diff --git a/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_add.test.ts b/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_add.test.ts index 17a745fafcc26..6026771654f8c 100644 --- a/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_add.test.ts +++ b/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_add.test.ts @@ -62,6 +62,11 @@ describe('', () => { expect(find('nextButton').props().disabled).toBe(true); }); + test('should not show repository-not-found warning', () => { + const { exists } = testBed; + expect(exists('repositoryNotFoundWarning')).toBe(false); + }); + describe('form validation', () => { describe('logistics (step 1)', () => { test('should require a policy name', async () => { diff --git a/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_edit.test.ts b/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_edit.test.ts index c0897dc02af35..cd0ccee5c0fac 100644 --- a/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_edit.test.ts +++ b/x-pack/plugins/snapshot_restore/__jest__/client_integration/policy_edit.test.ts @@ -59,6 +59,44 @@ describe('', () => { expect(find('pageTitle').text()).toEqual('Edit policy'); }); + describe('policy with pre-existing repository that was deleted', () => { + beforeEach(async () => { + httpRequestsMockHelpers.setGetPolicyResponse({ policy: POLICY_EDIT }); + httpRequestsMockHelpers.setLoadIndicesResponse({ + indices: ['my_index'], + dataStreams: ['my_data_stream'], + }); + httpRequestsMockHelpers.setLoadRepositoriesResponse({ + repositories: [{ name: 'this-is-a-new-repository' }], + }); + + testBed = await setup(); + + await act(async () => { + await nextTick(); + testBed.component.update(); + }); + }); + + test('should show repository-not-found warning', () => { + const { exists, find } = testBed; + expect(exists('repositoryNotFoundWarning')).toBe(true); + // The select should be an empty string to allow users to select a new repository + expect(find('repositorySelect').props().value).toBe(''); + }); + + describe('validation', () => { + test('should block navigating to next step', () => { + const { exists, find, actions } = testBed; + actions.clickNextButton(); + // Assert that we are still on the repository configuration step + expect(exists('repositoryNotFoundWarning')).toBe(true); + // The select should be an empty string to allow users to select a new repository + expect(find('repositorySelect').props().value).toBe(''); + }); + }); + }); + /** * As the "edit" policy component uses the same form underneath that * the "create" policy, we won't test it again but simply make sure that diff --git a/x-pack/plugins/snapshot_restore/public/application/components/policy_form/steps/step_logistics.tsx b/x-pack/plugins/snapshot_restore/public/application/components/policy_form/steps/step_logistics.tsx index 8a7338f4db4e7..f825c7b1f3d98 100644 --- a/x-pack/plugins/snapshot_restore/public/application/components/policy_form/steps/step_logistics.tsx +++ b/x-pack/plugins/snapshot_restore/public/application/components/policy_form/steps/step_logistics.tsx @@ -18,6 +18,8 @@ import { EuiLink, EuiSpacer, EuiText, + EuiCallOut, + EuiCode, } from '@elastic/eui'; import { Repository } from '../../../../../common/types'; @@ -54,6 +56,10 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ const { i18n, history } = useServices(); + const [showRepositoryNotFoundWarning, setShowRepositoryNotFoundWarning] = useState( + false + ); + // State for touched inputs const [touched, setTouched] = useState({ name: false, @@ -256,13 +262,26 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ } } + const doesRepositoryExist = + !!policy.repository && + repositories.some((r: { name: string }) => r.name === policy.repository); + + if (!doesRepositoryExist && !errors.repository) { + updatePolicy(policy, { repositoryDoesNotExist: true }); + } + + if (showRepositoryNotFoundWarning !== !doesRepositoryExist) { + setShowRepositoryNotFoundWarning(!doesRepositoryExist); + } + return ( ({ value: name, text: name, }))} - value={policy.repository || repositories[0].name} + hasNoInitialSelection={!doesRepositoryExist} + value={!doesRepositoryExist ? '' : policy.repository} onBlur={() => setTouched({ ...touched, repository: true })} onChange={(e) => { updatePolicy( @@ -541,8 +560,31 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ - + {showRepositoryNotFoundWarning && ( + <> + + + } + color="danger" + iconType="alert" + > + {policy.repository} }} + /> + + + )} + + {renderNameField()} {renderSnapshotNameField()} {renderRepositoryField()} diff --git a/x-pack/plugins/snapshot_restore/public/application/services/validation/validate_policy.ts b/x-pack/plugins/snapshot_restore/public/application/services/validation/validate_policy.ts index 4314b703722f6..b371ec9f8fe82 100644 --- a/x-pack/plugins/snapshot_restore/public/application/services/validation/validate_policy.ts +++ b/x-pack/plugins/snapshot_restore/public/application/services/validation/validate_policy.ts @@ -41,6 +41,12 @@ export interface ValidatePolicyHelperData { * are not configuring this value - like when they are on a previous step. */ validateIndicesCount?: boolean; + + /** + * A policy might be configured with a repository that no longer exists. We want the form to + * block in this case because just having a repository configured is not enough for validity. + */ + repositoryDoesNotExist?: boolean; } export const validatePolicy = ( @@ -50,7 +56,13 @@ export const validatePolicy = ( const i18n = textService.i18n; const { name, snapshotName, schedule, repository, config, retention } = policy; - const { managedRepository, isEditing, policyName, validateIndicesCount } = validationHelperData; + const { + managedRepository, + isEditing, + policyName, + validateIndicesCount, + repositoryDoesNotExist, + } = validationHelperData; const validation: PolicyValidation = { isValid: true, @@ -99,7 +111,7 @@ export const validatePolicy = ( ); } - if (isStringEmpty(repository)) { + if (isStringEmpty(repository) || repositoryDoesNotExist) { validation.errors.repository.push( i18n.translate('xpack.snapshotRestore.policyValidation.repositoryRequiredErrorMessage', { defaultMessage: 'Repository is required.',