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

[SR] Delete and execute SLM policies #41934

Merged
merged 7 commits into from
Aug 1, 2019
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ export { SectionError } from './section_error';
export { SectionLoading } from './section_loading';
export { SnapshotDeleteProvider } from './snapshot_delete_provider';
export { RestoreSnapshotForm } from './restore_snapshot_form';
export { PolicyExecuteProvider } from './policy_execute_provider';
export { PolicyDeleteProvider } from './policy_delete_provider';
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React, { Fragment, useRef, useState } from 'react';
import { EuiConfirmModal, EuiOverlayMask } from '@elastic/eui';
import { useAppDependencies } from '../index';
import { deletePolicies } from '../services/http';

interface Props {
children: (deletePolicy: DeletePolicy) => React.ReactElement;
}

export type DeletePolicy = (names: string[], onSuccess?: OnSuccessCallback) => void;

type OnSuccessCallback = (policiesDeleted: string[]) => void;

export const PolicyDeleteProvider: React.FunctionComponent<Props> = ({ children }) => {
const {
core: {
i18n,
notification: { toastNotifications },
},
} = useAppDependencies();
const { FormattedMessage } = i18n;
const [policyNames, setPolicyNames] = useState<string[]>([]);
const [isModalOpen, setIsModalOpen] = useState<boolean>(false);
const onSuccessCallback = useRef<OnSuccessCallback | null>(null);

const deletePolicyPrompt: DeletePolicy = (names, onSuccess = () => undefined) => {
if (!names || !names.length) {
throw new Error('No policy names specified for deletion');
}
setIsModalOpen(true);
setPolicyNames(names);
onSuccessCallback.current = onSuccess;
};

const closeModal = () => {
setIsModalOpen(false);
setPolicyNames([]);
};

const deletePolicy = () => {
const policiesToDelete = [...policyNames];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we create a copy of policyNames here? Does something bad happen if we delete policyNames directly?

deletePolicies(policiesToDelete).then(({ data, error }) => {
const { itemsDeleted, errors } = data || { itemsDeleted: undefined, errors: undefined };
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we could also assign a default value to the data parameter above, which would be a bit terser. We could also set a default value to itemsDeleted and errors, which would make lines 52 and 72 terser as well.

    deletePolicies(policiesToDelete).then(({ data = {}, error }) => {
      const { itemsDeleted = [], errors = [] } = data;
// ln 52
      if (itemsDeleted.length) {
// ln 72
      if (error || errors.length) {

I just realized that this won't quite work with the current logic for displaying error messages, but I have a suggestion that could make it work in the next comment.


// Surface success notifications
if (itemsDeleted && itemsDeleted.length) {
const hasMultipleSuccesses = itemsDeleted.length > 1;
const successMessage = hasMultipleSuccesses
? i18n.translate('xpack.snapshotRestore.deletePolicy.successMultipleNotificationTitle', {
defaultMessage: 'Deleted {count} policies',
values: { count: itemsDeleted.length },
})
: i18n.translate('xpack.snapshotRestore.deletePolicy.successSingleNotificationTitle', {
defaultMessage: "Deleted policy '{name}'",
values: { name: itemsDeleted[0] },
});
toastNotifications.addSuccess(successMessage);
if (onSuccessCallback.current) {
onSuccessCallback.current([...itemsDeleted]);
}
}

// Surface error notifications
// `error` is generic server error
// `data.errors` are specific errors with removing particular policy(ies)
if (error || (errors && errors.length)) {
Copy link
Contributor

@cjcenizal cjcenizal Aug 1, 2019

Choose a reason for hiding this comment

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

I was having trouble following this logic because it contains two parallel conditional branches. How about something like this to make the two branches easier to discern and compare?

      function createErrorMessageMultiple(count) {
        return i18n.translate('xpack.snapshotRestore.deletePolicy.errorMultipleNotificationTitle', {
          defaultMessage: 'Error deleting {count} policies',
          values: { count },
        });
      }

      function createErrorMessageSingle(name) {
        return i18n.translate('xpack.snapshotRestore.deletePolicy.errorSingleNotificationTitle', {
          defaultMessage: "Error deleting policy '{name}'",
          values: { name },
        });
      }

      // Surface error notifications
      let errorMessage;

      if (errors.length) {
        // `data.errors` are specific errors with removing particular policy(ies)
        errorMessage = errors.length > 1
          ? createErrorMessageMultiple(errors.length)
          : createErrorMessageSingle(errors[0].name);
      } else if (error) {
        // `error` is generic server error
        errorMessage = policiesToDelete.length > 1
          ? createErrorMessageMultiple(policiesToDelete.length)
          : createErrorMessageSingle(policiesToDelete[0])
      }

      if (errorMessage) {
        toastNotifications.addDanger(errorMessage);
      }

const hasMultipleErrors =
(errors && errors.length > 1) || (error && policiesToDelete.length > 1);
const errorMessage = hasMultipleErrors
? i18n.translate('xpack.snapshotRestore.deletePolicy.errorMultipleNotificationTitle', {
defaultMessage: 'Error deleting {count} policies',
values: {
count: (errors && errors.length) || policiesToDelete.length,
},
})
: i18n.translate('xpack.snapshotRestore.deletePolicy.errorSingleNotificationTitle', {
defaultMessage: "Error deleting policy '{name}'",
values: { name: (errors && errors[0].name) || policiesToDelete[0] },
});
toastNotifications.addDanger(errorMessage);
}
});
closeModal();
};

const renderModal = () => {
if (!isModalOpen) {
return null;
}

const isSingle = policyNames.length === 1;

return (
<EuiOverlayMask>
<EuiConfirmModal
title={
isSingle ? (
<FormattedMessage
id="xpack.snapshotRestore.deletePolicy.confirmModal.deleteSingleTitle"
defaultMessage="Delete policy '{name}'?"
values={{ name: policyNames[0] }}
/>
) : (
<FormattedMessage
id="xpack.snapshotRestore.deletePolicy.confirmModal.deleteMultipleTitle"
defaultMessage="Delete {count} policies?"
values={{ count: policyNames.length }}
/>
)
}
onCancel={closeModal}
onConfirm={deletePolicy}
cancelButtonText={
<FormattedMessage
id="xpack.snapshotRestore.deletePolicy.confirmModal.cancelButtonLabel"
defaultMessage="Cancel"
/>
}
confirmButtonText={
<FormattedMessage
id="xpack.snapshotRestore.deletePolicy.confirmModal.confirmButtonLabel"
defaultMessage="Delete {count, plural, one {policy} other {policies}}"
values={{ count: policyNames.length }}
/>
}
buttonColor="danger"
data-test-subj="srdeletePolicyConfirmationModal"
>
{!isSingle ? (
<Fragment>
<p>
<FormattedMessage
id="xpack.snapshotRestore.deletePolicy.confirmModal.deleteMultipleListDescription"
defaultMessage="You are about to delete these policies:"
/>
</p>
<ul>
{policyNames.map(name => (
<li key={name}>{name}</li>
))}
</ul>
</Fragment>
) : null}
</EuiConfirmModal>
</EuiOverlayMask>
);
};

return (
<Fragment>
{children(deletePolicyPrompt)}
{renderModal()}
</Fragment>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React, { Fragment, useRef, useState } from 'react';
import { EuiConfirmModal, EuiOverlayMask } from '@elastic/eui';
import { useAppDependencies } from '../index';
import { executePolicy as executePolicyRequest } from '../services/http';

interface Props {
children: (executePolicy: ExecutePolicy) => React.ReactElement;
}

export type ExecutePolicy = (name: string, onSuccess?: OnSuccessCallback) => void;

type OnSuccessCallback = () => void;

export const PolicyExecuteProvider: React.FunctionComponent<Props> = ({ children }) => {
const {
core: {
i18n,
notification: { toastNotifications },
},
} = useAppDependencies();
const { FormattedMessage } = i18n;
const [policyName, setPolicyName] = useState<string>('');
const [isModalOpen, setIsModalOpen] = useState<boolean>(false);
const onSuccessCallback = useRef<OnSuccessCallback | null>(null);

const executePolicyPrompt: ExecutePolicy = (name, onSuccess = () => undefined) => {
if (!name || !name.length) {
throw new Error('No policy name specified for execution');
}
setIsModalOpen(true);
setPolicyName(name);
onSuccessCallback.current = onSuccess;
};

const closeModal = () => {
setIsModalOpen(false);
setPolicyName('');
};
const executePolicy = () => {
executePolicyRequest(policyName).then(({ data, error }) => {
const { snapshotName } = data || { snapshotName: undefined };

// Surface success notification
if (snapshotName) {
const successMessage = i18n.translate(
'xpack.snapshotRestore.executePolicy.successNotificationTitle',
{
defaultMessage: "Policy '{name}' is running",
values: { name: policyName },
}
);
toastNotifications.addSuccess(successMessage);
if (onSuccessCallback.current) {
onSuccessCallback.current();
}
}

// Surface error notifications
if (error) {
const errorMessage = i18n.translate(
'xpack.snapshotRestore.executePolicy.errorNotificationTitle',
{
defaultMessage: "Error running policy '{name}'",
values: { name: policyName },
}
);
toastNotifications.addDanger(errorMessage);
}
});
closeModal();
};

const renderModal = () => {
if (!isModalOpen) {
return null;
}

return (
<EuiOverlayMask>
<EuiConfirmModal
title={
<FormattedMessage
id="xpack.snapshotRestore.executePolicy.confirmModal.executePolicyTitle"
defaultMessage="Run policy '{name}'?"
values={{ name: policyName }}
/>
}
onCancel={closeModal}
onConfirm={executePolicy}
cancelButtonText={
<FormattedMessage
id="xpack.snapshotRestore.executePolicy.confirmModal.cancelButtonLabel"
defaultMessage="Cancel"
/>
}
confirmButtonText={
<FormattedMessage
id="xpack.snapshotRestore.executePolicy.confirmModal.confirmButtonLabel"
defaultMessage="Run"
/>
}
data-test-subj="srExecutePolicyConfirmationModal"
>
<p>
<FormattedMessage
id="xpack.snapshotRestore.executePolicy.confirmModal.executeDescription"
defaultMessage="A snapshot will be taken immediately using this policy configuration."
/>
</p>
</EuiConfirmModal>
</EuiOverlayMask>
);
};

return (
<Fragment>
{children(executePolicyPrompt)}
{renderModal()}
</Fragment>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ export const RepositoryDeleteProvider: React.FunctionComponent<Props> = ({ child

const deleteRepository = () => {
const repositoriesToDelete = [...repositoryNames];
deleteRepositories(repositoriesToDelete).then(({ data: { itemsDeleted, errors }, error }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

while working on policy deletion, I found that for server errors such as 404 or 500, which don't return a data object, early destructuring here causes an undefined object error, so no toast notification is shown. I adjusted this for policy deletion and fixed it for other delete providers in this app such as here.

deleteRepositories(repositoriesToDelete).then(({ data, error }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be fixed like this?

// Default `data` to an empty object if it's not provided, which will have
// undefined `itemsDeleted` and `errors` properties.
deleteRepositories(repositoriesToDelete).then(({ data: { itemsDeleted, errors } = {}, error }) => {

const { itemsDeleted, errors } = data || { itemsDeleted: undefined, errors: undefined };

// Surface success notifications
if (itemsDeleted && itemsDeleted.length) {
const hasMultipleSuccesses = itemsDeleted.length > 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ export const SnapshotDeleteProvider: React.FunctionComponent<Props> = ({ childre
const deleteSnapshot = () => {
const snapshotsToDelete = [...snapshotIds];
setIsDeleting(true);
deleteSnapshots(snapshotsToDelete).then(({ data: { itemsDeleted, errors }, error }) => {
deleteSnapshots(snapshotsToDelete).then(({ data, error }) => {
const { itemsDeleted, errors } = data || { itemsDeleted: undefined, errors: undefined };

// Wait until request is done to close modal; deleting snapshots take longer due to their sequential nature
closeModal();
setIsDeleting(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,6 @@ export const UIM_POLICY_LIST_LOAD = 'policy_list_load';
export const UIM_POLICY_SHOW_DETAILS_CLICK = 'policy_show_details_click';
export const UIM_POLICY_DETAIL_PANEL_SUMMARY_TAB = 'policy_detail_panel_summary_tab';
export const UIM_POLICY_DETAIL_PANEL_HISTORY_TAB = 'policy_detail_panel_last_success_tab';
export const UIM_POLICY_EXECUTE = 'policy_execute';
export const UIM_POLICY_DELETE = 'policy_delete';
export const UIM_POLICY_DELETE_MANY = 'policy_delete_many';
Loading