From 6da36b58a2a0a594f5f31d2291a77ee1b6b397a9 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Mon, 7 Sep 2020 12:23:41 +0200 Subject: [PATCH 1/7] implement fix and add callout with copy --- .../policy_form/steps/step_logistics.tsx | 72 +++++++++++++------ 1 file changed, 50 insertions(+), 22 deletions(-) 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..99b33d8f6d41c 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,7 @@ import { EuiLink, EuiSpacer, EuiText, + EuiCallOut, } from '@elastic/eui'; import { Repository } from '../../../../../common/types'; @@ -256,29 +257,56 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ } } + const policyRepositoryExists = + !!policy.repository && + repositories.some((r: { name: string }) => r.name === policy.repository); + return ( - ({ - value: name, - text: name, - }))} - value={policy.repository || repositories[0].name} - onBlur={() => setTouched({ ...touched, repository: true })} - onChange={(e) => { - updatePolicy( - { - repository: e.target.value, - }, - { - managedRepository, - isEditing, - policyName: policy.name, - } - ); - }} - fullWidth - data-test-subj="repositorySelect" - /> + <> + {!policyRepositoryExists && ( + <> + + } + color="warning" + iconType="alert" + > + + + + + )} + ({ + value: name, + text: name, + }))} + hasNoInitialSelection={!policyRepositoryExists} + value={!policyRepositoryExists ? '' : policy.repository} + onBlur={() => setTouched({ ...touched, repository: true })} + onChange={(e) => { + updatePolicy( + { + repository: e.target.value, + }, + { + managedRepository, + isEditing, + policyName: policy.name, + } + ); + }} + fullWidth + data-test-subj="repositorySelect" + /> + ); }; From f0721cc9036c7fb9041c92249325a0f7b69c3777 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Mon, 7 Sep 2020 12:34:44 +0200 Subject: [PATCH 2/7] added test --- .../helpers/policy_form.helpers.ts | 2 ++ .../client_integration/policy_add.test.ts | 5 ++++ .../client_integration/policy_edit.test.ts | 27 +++++++++++++++++++ .../policy_form/steps/step_logistics.tsx | 1 + 4 files changed, 35 insertions(+) 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 dc568161d4fb4..f7d6a60703bc4 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 @@ -67,6 +67,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 a5b2ec73b85cd..e05ff5c92afd4 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 @@ -52,6 +52,33 @@ 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(''); + }); + }); + /** * 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 99b33d8f6d41c..88ed1909ebf4e 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 @@ -266,6 +266,7 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ {!policyRepositoryExists && ( <> Date: Tue, 8 Sep 2020 10:18:32 +0200 Subject: [PATCH 3/7] make callout a danger callout and revise copy --- .../components/policy_form/steps/step_logistics.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 88ed1909ebf4e..ee3ee035ff29f 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 @@ -19,6 +19,7 @@ import { EuiSpacer, EuiText, EuiCallOut, + EuiCode, } from '@elastic/eui'; import { Repository } from '../../../../../common/types'; @@ -270,15 +271,16 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ title={ } - color="warning" + color="danger" iconType="alert" > {policy.repository} }} /> From dcf611de76d4923d8b08625f471fa2d252491689 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Sep 2020 10:25:48 +0200 Subject: [PATCH 4/7] block the form if we have a repo, but it does not exist --- .../components/policy_form/steps/step_logistics.tsx | 6 +++++- .../application/services/validation/validate_policy.ts | 10 ++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) 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 ee3ee035ff29f..7656d5ba7c65d 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 @@ -262,6 +262,10 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ !!policy.repository && repositories.some((r: { name: string }) => r.name === policy.repository); + if (!policyRepositoryExists && !errors.repository) { + updatePolicy(policy, { repositoryDoesNotExist: true }); + } + return ( <> {!policyRepositoryExists && ( @@ -279,7 +283,7 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ > {policy.repository} }} /> 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..c114c9a1601aa 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 @@ -50,7 +50,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 +105,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.', From 71b86d7ee66dab61ef7ccd4f63b2b968fea386b1 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Sep 2020 10:53:56 +0200 Subject: [PATCH 5/7] added test to assert that form wizard blocks on validation for not found repo --- .../__jest__/client_integration/policy_edit.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 e05ff5c92afd4..7c095256bd10f 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 @@ -77,6 +77,17 @@ describe('', () => { // 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(''); + }); + }); }); /** From 0a646d5fa8ae4f0255a08e1f2ea78a5893dad55b Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Sep 2020 11:24:04 +0200 Subject: [PATCH 6/7] fix types and add a doc comment --- .../application/services/validation/validate_policy.ts | 6 ++++++ 1 file changed, 6 insertions(+) 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 c114c9a1601aa..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 = ( From 4bb3af68eac8c3b6501090017ec4d96e0e5f7be2 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Wed, 9 Sep 2020 12:14:36 +0200 Subject: [PATCH 7/7] move callout to above the form --- .../policy_form/steps/step_logistics.tsx | 107 ++++++++++-------- 1 file changed, 57 insertions(+), 50 deletions(-) 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 7656d5ba7c65d..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 @@ -56,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, @@ -258,62 +262,42 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ } } - const policyRepositoryExists = + const doesRepositoryExist = !!policy.repository && repositories.some((r: { name: string }) => r.name === policy.repository); - if (!policyRepositoryExists && !errors.repository) { + if (!doesRepositoryExist && !errors.repository) { updatePolicy(policy, { repositoryDoesNotExist: true }); } + if (showRepositoryNotFoundWarning !== !doesRepositoryExist) { + setShowRepositoryNotFoundWarning(!doesRepositoryExist); + } + return ( - <> - {!policyRepositoryExists && ( - <> - - } - color="danger" - iconType="alert" - > - {policy.repository} }} - /> - - - - )} - ({ - value: name, - text: name, - }))} - hasNoInitialSelection={!policyRepositoryExists} - value={!policyRepositoryExists ? '' : policy.repository} - onBlur={() => setTouched({ ...touched, repository: true })} - onChange={(e) => { - updatePolicy( - { - repository: e.target.value, - }, - { - managedRepository, - isEditing, - policyName: policy.name, - } - ); - }} - fullWidth - data-test-subj="repositorySelect" - /> - + ({ + value: name, + text: name, + }))} + hasNoInitialSelection={!doesRepositoryExist} + value={!doesRepositoryExist ? '' : policy.repository} + onBlur={() => setTouched({ ...touched, repository: true })} + onChange={(e) => { + updatePolicy( + { + repository: e.target.value, + }, + { + managedRepository, + isEditing, + policyName: policy.name, + } + ); + }} + fullWidth + data-test-subj="repositorySelect" + /> ); }; @@ -576,8 +560,31 @@ export const PolicyStepLogistics: React.FunctionComponent = ({ - + {showRepositoryNotFoundWarning && ( + <> + + + } + color="danger" + iconType="alert" + > + {policy.repository} }} + /> + + + )} + + {renderNameField()} {renderSnapshotNameField()} {renderRepositoryField()}