[SR] Single and multiple repository delete

[jen-huang]

Adds single and multiple repository delete endpoint and UIs.

There is a known issue for repositories with commas in their name where loading their detail panel results in a 404 due to an ES issue: elastic/elasticsearch#40817. Repositories with commas in their name also can't be deleted using this new delete endpoint. Since the fix on ES side will likely be putting some character restrictions for the name, I'm ignoring that edge case for now.

Confirmation modal for multiple delete by selecting repositories from table:

Confirmation modal for single delete:

Success and error toasts:

[elasticmachine]

Pinging @elastic/es-ui

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[cjcenizal]

Tested locally, code LGTM! I had a couple questions and suggestions, the most significant of which was a suggested change to the structure of the notification logic.

[cjcenizal]

Just a thought... what if we used the string type everywhere instead of Repository['name']? What would the impact be on readability and type-checking?

Personally, I think seeing string would be easier to understand. And I don't see any downside, because it simplifies the requirements of many variable types because they'll only need to "know" about strings, not repositories. But my mental model of this code is incomplete as is my knowledge of TS, so maybe I'm just not seeing the meaning/value behind Repository['name'].

I can imagine the argument that it will allow us to change the type of name to some other value without needing to update all of our types everywhere, but I think that's a very unlikely scenario. Is there another benefit?

[sebelga]

I agree with CJ, I would have simply written (repositoriesDeleted: string[]) as it is very unlikely that the Repository['name'] changes from string to another type. Then it could also be applied on L18 (and basically everywhere).

[cjcenizal]

Can we rename success to itemsDeleted here and in the API endpoint? I find it a little easier to understand, there's precedent since we do this for the CCR API and I've updated the Remote Clusters API to use this name too. Similarly, can we rename data.error to data.errors? This will help differentiate it from error.

Also what do you think of destructuring data at the top here? I think it should be safe, since both success and error are always defined on the response.

And last thing... I'm finding it difficult to follow the parallel logic branches of the two error-types. I know it might also seem a bit more verbose, but if we use a catch to separate the two branches then personally I would also find it easier to follow the logic. This would be possible if we update deleteRepositories to throw an error instead of returning it. Here's the general idea of what I'm thinking:

// This helper is extracted onto line 22
function getErrorTitle(i18n, count, name) {
  const hasMultipleErrors = count > 1;
  if (hasMultipleErrors) {
    return i18n.translate(
      'xpack.snapshotRestore.deleteRepository.errorMultipleNotificationTitle',
      {
        defaultMessage: 'Error removing {count} repositories',
        values: {
          count,
        },
      }
    );

  return i18n.translate('xpack.snapshotRestore.deleteRepository.errorSingleNotificationTitle', {
    defaultMessage: "Error removing repository '{name}'",
    values: { name },
  });
}

// ...

    deleteRepositories(repositoriesToDelete)
      // Destructure the 200 response into success and error (or itemsDeleted and errors)
      .then(({ success, error }) => { 
        // Surface success notifications
        if (success.length) {
          const hasMultipleSuccesses = success.length > 1;
          const successMessage = hasMultipleSuccesses
            ? i18n.translate(
                'xpack.snapshotRestore.deleteRepository.successMultipleNotificationTitle',
                {
                  defaultMessage: 'Removed {count} repositories',
                  values: { count: success.length },
                }
              )
            : i18n.translate(
                'xpack.snapshotRestore.deleteRepository.successSingleNotificationTitle',
                {
                  defaultMessage: "Removed repository '{name}'",
                  values: { name: success[0] },
                }
              );

          toastNotifications.addSuccess(successMessage);

          if (onSuccessCallback.current) {
            onSuccessCallback.current([...success]);
          }
        }

        if (error.length) {
          const errorTitle = getErrorTitle(i18n, error.length, error[0]);
          toastNotifications.addDanger(errorTitle);
        }
      })
      .catch(error => {
        const errorTitle = getErrorTitle(i18n, repositoriesToDelete.length, repositoriesToDelete[0]);
        toastNotifications.addDanger(errorTitle);
      });

[cjcenizal]

It looks like data.errors is a typo here and on line 97, since we're expecting data.error right?

[cjcenizal]

I find this name a bit confusing because I imagine that the modal is where the user confirms the delete action. But if I understand the code correctly, we're using confirmDeleteRepository to open the confirmation modal. Can we rename this to communicate that we're prompting the user for confirmation? How about promptUserToDeleteRepository or simply promptUser since the context of "deleting repositories" is already established by the component name?

[sebelga]

You don't need to type twice everything 😊 This is the beauty of Typescript, once you typed the source, the consumer get the types. So this can simply be

<RepositoryDeleteProvider>
  {confirmDeleteRepository => {
    return (

Then by doing that, we realize that you have a wrong type for DeleteRepository on L18 of "repository_delete_provider.tsx".

It should be

type DeleteRepository = (names: string[], onSuccess: OnSuccessCallback) => void;

And then we realize that we don't need to type the arguments of the function, as you already gave it a type

// "names" and "onSuccess" are types through "DeleteRepository"
const confirmDeleteRepository: DeleteRepository = (names, onSuccess = () => undefined): void => {
  setIsModalOpen(true);
  setRepositoryNames(names);
  onSuccessCallback = onSuccess;
};

And suddenly you start loving Typescript 😄

[cjcenizal]

Minor nit, but maybe onRepositoryDeleted (past tense) is clearer? This also complements the name of its parameter, repositoriesDeleted.

[cjcenizal]

Minor nit: map returns a new array, so I think you can remove the wrapping array literal and remove the destructuring. Same on line 43.

[sebelga]

Overall LGTM @jen-huang ! I did not test it locally but made a few comments about style and Typescript. 😊

[sebelga]

|| !repositoryNames || !repositoryNames.length should never happen. If it does, it means that there is a problem at line L38 & L39. I would not set the modal open on L38 if that happens and console log a warning that this should not happen and return early.

[sebelga]

Do we need this line and reset the array? As it is closed it is hidden anyway.

[sebelga]

Do we really need to use this hook instead of a simple let onSuccessCallBack; ? Something like this

let onSuccessCallback: OnSuccessCallback;

const confirmDeleteRepository: DeleteRepository = (
  names: string[],
  onSuccess: OnSuccessCallback = () => undefined
): void => {
  setIsModalOpen(true);
  setRepositoryNames(names);
  onSuccessCallback = onSuccess;
};

[sebelga]

With the above change, the if would not be necessary and would simply be onSuccessCallback([...data.success]);

[sebelga]

You don't need to type twice everything 😊 This is the beauty of Typescript, once you typed the source, the consumer get the types. So this can simply be

<RepositoryDeleteProvider>
  {confirmDeleteRepository => {
    return (

Then by doing that, we realize that you have a wrong type for DeleteRepository on L18 of "repository_delete_provider.tsx".

It should be

type DeleteRepository = (names: string[], onSuccess: OnSuccessCallback) => void;

And then we realize that we don't need to type the arguments of the function, as you already gave it a type

// "names" and "onSuccess" are types through "DeleteRepository"
const confirmDeleteRepository: DeleteRepository = (names, onSuccess = () => undefined): void => {
  setIsModalOpen(true);
  setRepositoryNames(names);
  onSuccessCallback = onSuccess;
};

And suddenly you start loving Typescript 😄

[sebelga]

I personally read this better (and it's shorter). But it's just a preference 😊

const response = await getAllHandler(mockRequest, callWithRequest, mockResponseToolkit);
expect(response).toEqual(expectedResponse);

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request