Skip to content

Commit

Permalink
[Snapshot & Restore] fix pre existing policy with no existing reposit…
Browse files Browse the repository at this point in the history
…ory (elastic#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 <elasticmachine@users.noreply.github.com>
  • Loading branch information
jloleysens and elasticmachine committed Sep 9, 2020
1 parent 69b3ba6 commit 08bb71d
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,6 @@ export type PolicyFormTestSubjects =
| 'showAdvancedCronLink'
| 'snapshotNameInput'
| 'dataStreamBadge'
| 'repositoryNotFoundWarning'
| 'repositorySelect'
| 'submitButton';
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ describe('<PolicyAdd />', () => {
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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,44 @@ describe('<PolicyEdit />', () => {
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import {
EuiLink,
EuiSpacer,
EuiText,
EuiCallOut,
EuiCode,
} from '@elastic/eui';

import { Repository } from '../../../../../common/types';
Expand Down Expand Up @@ -54,6 +56,10 @@ export const PolicyStepLogistics: React.FunctionComponent<StepProps> = ({

const { i18n, history } = useServices();

const [showRepositoryNotFoundWarning, setShowRepositoryNotFoundWarning] = useState<boolean>(
false
);

// State for touched inputs
const [touched, setTouched] = useState({
name: false,
Expand Down Expand Up @@ -256,13 +262,26 @@ export const PolicyStepLogistics: React.FunctionComponent<StepProps> = ({
}
}

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 (
<EuiSelect
options={repositories.map(({ name }: Repository) => ({
value: name,
text: name,
}))}
value={policy.repository || repositories[0].name}
hasNoInitialSelection={!doesRepositoryExist}
value={!doesRepositoryExist ? '' : policy.repository}
onBlur={() => setTouched({ ...touched, repository: true })}
onChange={(e) => {
updatePolicy(
Expand Down Expand Up @@ -541,8 +560,31 @@ export const PolicyStepLogistics: React.FunctionComponent<StepProps> = ({
</EuiButtonEmpty>
</EuiFlexItem>
</EuiFlexGroup>
<EuiSpacer size="l" />

{showRepositoryNotFoundWarning && (
<>
<EuiSpacer size="m" />
<EuiCallOut
data-test-subj="repositoryNotFoundWarning"
title={
<FormattedMessage
id="xpack.snapshotRestore.policyForm.stepLogistics.selectRepository.policyRepositoryNotFoundTitle"
defaultMessage="Repository not found"
/>
}
color="danger"
iconType="alert"
>
<FormattedMessage
id="xpack.snapshotRestore.policyForm.stepLogistics.selectRepository.policyRepositoryNotFoundDescription"
defaultMessage="Repository {repo} does not exist. Please select an existing repository."
values={{ repo: <EuiCode>{policy.repository}</EuiCode> }}
/>
</EuiCallOut>
</>
)}

<EuiSpacer size="l" />
{renderNameField()}
{renderSnapshotNameField()}
{renderRepositoryField()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand All @@ -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,
Expand Down Expand Up @@ -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.',
Expand Down

0 comments on commit 08bb71d

Please sign in to comment.