[Fleet] Changed rename tag feature to include a Rename button (#135901)

* fixed bug with debounce * changed auto update to rename button * improved tag update messages * fixed tests * rename on enter key, made rename button smaller * improved logic so that tag will be renamed in place * rename tag when clicking outside * using event types * removed rename button Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
2025-04-23 09:19:04 -04:00 · 2022-07-08 16:50:00 +02:00 · 2022-07-08 16:50:00 +02:00 · dd7d92f282
commit dd7d92f282
parent 45e7fe6ead
7 changed files with 296 additions and 64 deletions
--- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.test.tsx
+++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.test.tsx
@ -6,17 +6,12 @@
 */

 import React from 'react';
-import { debounce } from 'lodash';
 import { render, fireEvent, waitFor } from '@testing-library/react';

 import { useUpdateTags } from '../hooks';

 import { TagOptions } from './tag_options';

-jest.mock('lodash', () => ({
-  debounce: jest.fn(),
-}));
-
 jest.mock('../hooks', () => ({
  useUpdateTags: jest.fn().mockReturnValue({
    bulkUpdateTags: jest.fn(),
@ -33,10 +28,6 @@ describe('TagOptions', () => {
    mockBulkUpdateTags.mockReset();
    mockBulkUpdateTags.mockResolvedValue({});
    isTagHovered = true;
-    (debounce as jest.Mock).mockImplementationOnce((fn) => (newName: string) => {
-      fn(newName);
-      onTagsUpdated();
-    });
  });

  const renderComponent = () => {
@ -58,17 +49,24 @@ describe('TagOptions', () => {
    });
  });

-  it('should delete tag when button is clicked', () => {
+  it('should delete tag when delete button is clicked', () => {
    const result = renderComponent();

    fireEvent.click(result.getByRole('button'));

    fireEvent.click(result.getByText('Delete tag'));

-    expect(mockBulkUpdateTags).toHaveBeenCalledWith('tags:agent', [], ['agent'], expect.anything());
+    expect(mockBulkUpdateTags).toHaveBeenCalledWith(
+      'tags:agent',
+      [],
+      ['agent'],
+      expect.anything(),
+      'Tag deleted',
+      'Tag delete failed'
+    );
  });

-  it('should rename tag when name input is changed', async () => {
+  it('should rename tag when enter key is pressed', async () => {
    const result = renderComponent();

    fireEvent.click(result.getByRole('button'));
@ -77,13 +75,17 @@ describe('TagOptions', () => {
    fireEvent.input(nameInput, {
      target: { value: 'newName' },
    });
+    fireEvent.keyDown(nameInput, {
+      key: 'Enter',
+    });

-    expect(onTagsUpdated).toHaveBeenCalled();
    expect(mockBulkUpdateTags).toHaveBeenCalledWith(
      'tags:agent',
      ['newName'],
      ['agent'],
-      expect.anything()
+      expect.anything(),
+      'Tag renamed',
+      'Tag rename failed'
    );
  });
 });
--- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.tsx
+++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.tsx
@ -5,8 +5,8 @@
 * 2.0.
 */

-import React, { useMemo, useState, useEffect } from 'react';
-import { debounce } from 'lodash';
+import type { MouseEvent, ChangeEvent } from 'react';
+import React, { useState, useEffect } from 'react';
 import {
  EuiButtonEmpty,
  EuiButtonIcon,
@ -30,29 +30,65 @@ interface Props {
 export const TagOptions: React.FC<Props> = ({ tagName, isTagHovered, onTagsUpdated }: Props) => {
  const [tagOptionsVisible, setTagOptionsVisible] = useState<boolean>(false);
  const [tagOptionsButton, setTagOptionsButton] = useState<HTMLElement>();
-
  const [tagMenuButtonVisible, setTagMenuButtonVisible] = useState<boolean>(isTagHovered);
+  const [updatedName, setUpdatedName] = useState<string | undefined>(tagName);

  useEffect(() => {
    setTagMenuButtonVisible(isTagHovered || tagOptionsVisible);
  }, [isTagHovered, tagOptionsVisible]);

-  const [updatedName, setUpdatedName] = useState<string | undefined>(tagName);
+  useEffect(() => {
+    setUpdatedName(tagName);
+  }, [tagName]);

-  const closePopover = () => setTagOptionsVisible(false);
+  const closePopover = (isDelete = false) => {
+    setTagOptionsVisible(false);
+    if (isDelete) {
+      handleDelete();
+    } else {
+      handleRename(updatedName);
+    }
+  };

  const updateTagsHook = useUpdateTags();
+  const bulkUpdateTags = updateTagsHook.bulkUpdateTags;

  const TAGS_QUERY = 'tags:{name}';

-  const debouncedSendRenameTag = useMemo(
-    () =>
-      debounce((newName: string) => {
-        const kuery = TAGS_QUERY.replace('{name}', tagName);
-        updateTagsHook.bulkUpdateTags(kuery, [newName], [tagName], () => onTagsUpdated());
-      }, 1000),
-    [onTagsUpdated, tagName, updateTagsHook]
-  );
+  const handleRename = (newName?: string) => {
+    if (newName === tagName || !newName) {
+      return;
+    }
+    const kuery = TAGS_QUERY.replace('{name}', tagName);
+    bulkUpdateTags(
+      kuery,
+      [newName],
+      [tagName],
+      () => onTagsUpdated(),
+      i18n.translate('xpack.fleet.renameAgentTags.successNotificationTitle', {
+        defaultMessage: 'Tag renamed',
+      }),
+      i18n.translate('xpack.fleet.renameAgentTags.errorNotificationTitle', {
+        defaultMessage: 'Tag rename failed',
+      })
+    );
+  };
+
+  const handleDelete = () => {
+    const kuery = TAGS_QUERY.replace('{name}', tagName);
+    updateTagsHook.bulkUpdateTags(
+      kuery,
+      [],
+      [tagName],
+      () => onTagsUpdated(),
+      i18n.translate('xpack.fleet.deleteAgentTags.successNotificationTitle', {
+        defaultMessage: 'Tag deleted',
+      }),
+      i18n.translate('xpack.fleet.deleteAgentTags.errorNotificationTitle', {
+        defaultMessage: 'Tag delete failed',
+      })
+    );
+  };

  return (
    <>
@ -63,8 +99,8 @@ export const TagOptions: React.FC<Props> = ({ tagName, isTagHovered, onTagsUpdat
            defaultMessage: 'Tag Options',
          })}
          color="text"
-          onClick={(event: any) => {
-            setTagOptionsButton(event.target);
+          onClick={(event: MouseEvent<HTMLButtonElement>) => {
+            setTagOptionsButton(event.currentTarget);
            setTagOptionsVisible(!tagOptionsVisible);
          }}
        />
@ -84,13 +120,14 @@ export const TagOptions: React.FC<Props> = ({ tagName, isTagHovered, onTagsUpdat
                })}
                value={updatedName}
                required
-                onChange={(e: any) => {
-                  const newName = e.target.value;
-                  setUpdatedName(newName);
-                  if (!newName) {
-                    return;
+                onKeyDown={(e: { key: string }) => {
+                  if (e.key === 'Enter') {
+                    closePopover();
                  }
-                  debouncedSendRenameTag(newName);
+                }}
+                onChange={(e: ChangeEvent<HTMLInputElement>) => {
+                  const newName = e.currentTarget.value;
+                  setUpdatedName(newName);
                }}
              />
            </EuiFlexItem>
@ -99,9 +136,7 @@ export const TagOptions: React.FC<Props> = ({ tagName, isTagHovered, onTagsUpdat
                size="s"
                color="danger"
                onClick={() => {
-                  const kuery = TAGS_QUERY.replace('{name}', tagName);
-                  updateTagsHook.bulkUpdateTags(kuery, [], [tagName], () => onTagsUpdated());
-                  closePopover();
+                  closePopover(true);
                }}
              >
                <EuiIcon type="trash" />{' '}
--- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.test.tsx
+++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.test.tsx
@ -57,7 +57,13 @@ describe('TagsAddRemove', () => {

    fireEvent.click(getTag('tag2'));

-    expect(mockUpdateTags).toHaveBeenCalledWith('agent1', ['tag1', 'tag2'], expect.anything());
+    expect(mockUpdateTags).toHaveBeenCalledWith(
+      'agent1',
+      ['tag1', 'tag2'],
+      expect.anything(),
+      undefined,
+      undefined
+    );
  });

  it('should remove selected tag when previously selected', async () => {
@ -69,7 +75,13 @@ describe('TagsAddRemove', () => {

    fireEvent.click(getTag('tag1'));

-    expect(mockUpdateTags).toHaveBeenCalledWith('agent1', [], expect.anything());
+    expect(mockUpdateTags).toHaveBeenCalledWith(
+      'agent1',
+      [],
+      expect.anything(),
+      undefined,
+      undefined
+    );
  });

  it('should add new tag when not found in search and button clicked', () => {
@ -82,7 +94,13 @@ describe('TagsAddRemove', () => {

    fireEvent.click(result.getAllByText('Create a new tag "newTag"')[0].closest('button')!);

-    expect(mockUpdateTags).toHaveBeenCalledWith('agent1', ['tag1', 'newTag'], expect.anything());
+    expect(mockUpdateTags).toHaveBeenCalledWith(
+      'agent1',
+      ['tag1', 'newTag'],
+      expect.anything(),
+      'Tag created',
+      'Tag creation failed'
+    );
  });

  it('should add selected tag when previously unselected - bulk selection', async () => {
@ -94,7 +112,14 @@ describe('TagsAddRemove', () => {

    fireEvent.click(getTag('tag2'));

-    expect(mockBulkUpdateTags).toHaveBeenCalledWith('', ['tag2'], [], expect.anything());
+    expect(mockBulkUpdateTags).toHaveBeenCalledWith(
+      '',
+      ['tag2'],
+      [],
+      expect.anything(),
+      undefined,
+      undefined
+    );
  });

  it('should remove selected tag when previously selected - bulk selection', async () => {
@ -110,7 +135,9 @@ describe('TagsAddRemove', () => {
      ['agent1', 'agent2'],
      [],
      ['tag1'],
-      expect.anything()
+      expect.anything(),
+      undefined,
+      undefined
    );
  });

@ -124,7 +151,14 @@ describe('TagsAddRemove', () => {

    fireEvent.click(result.getAllByText('Create a new tag "newTag"')[0].closest('button')!);

-    expect(mockBulkUpdateTags).toHaveBeenCalledWith('query', ['newTag'], [], expect.anything());
+    expect(mockBulkUpdateTags).toHaveBeenCalledWith(
+      'query',
+      ['newTag'],
+      [],
+      expect.anything(),
+      'Tag created',
+      'Tag creation failed'
+    );
  });

  it('should make tag options button visible on mouse enter', async () => {
--- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.tsx
+++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.tsx
@ -64,15 +64,29 @@ export const TagsAddRemove: React.FC<Props> = ({
    setLabels(labelsFromTags(allTags));
  }, [allTags, labelsFromTags]);

-  const updateTags = async (tagsToAdd: string[], tagsToRemove: string[]) => {
+  const updateTags = async (
+    tagsToAdd: string[],
+    tagsToRemove: string[],
+    successMessage?: string,
+    errorMessage?: string
+  ) => {
    if (agentId) {
      updateTagsHook.updateTags(
        agentId,
        difference(selectedTags, tagsToRemove).concat(tagsToAdd),
-        () => onTagsUpdated()
+        () => onTagsUpdated(),
+        successMessage,
+        errorMessage
      );
    } else {
-      updateTagsHook.bulkUpdateTags(agents!, tagsToAdd, tagsToRemove, () => onTagsUpdated());
+      updateTagsHook.bulkUpdateTags(
+        agents!,
+        tagsToAdd,
+        tagsToRemove,
+        () => onTagsUpdated(),
+        successMessage,
+        errorMessage
+      );
    }
  };

@ -136,7 +150,16 @@ export const TagsAddRemove: React.FC<Props> = ({
                if (!searchValue) {
                  return;
                }
-                updateTags([searchValue], []);
+                updateTags(
+                  [searchValue],
+                  [],
+                  i18n.translate('xpack.fleet.createAgentTags.successNotificationTitle', {
+                    defaultMessage: 'Tag created',
+                  }),
+                  i18n.translate('xpack.fleet.createAgentTags.errorNotificationTitle', {
+                    defaultMessage: 'Tag creation failed',
+                  })
+                );
              }}
            >
              <EuiIcon type="plus" />{' '}
--- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.tsx
+++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.tsx
@ -18,37 +18,51 @@ export const useUpdateTags = () => {
  const { notifications } = useStartServices();

  const wrapRequest = useCallback(
-    async (requestFn: () => Promise<any>, onSuccess: () => void) => {
+    async (
+      requestFn: () => Promise<any>,
+      onSuccess: () => void,
+      successMessage?: string,
+      errorMessage?: string
+    ) => {
      try {
        const res = await requestFn();

        if (res.error) {
          throw res.error;
        }
-        const successMessage = i18n.translate(
-          'xpack.fleet.updateAgentTags.successNotificationTitle',
-          {
+        const message =
+          successMessage ??
+          i18n.translate('xpack.fleet.updateAgentTags.successNotificationTitle', {
            defaultMessage: 'Tags updated',
-          }
-        );
-        notifications.toasts.addSuccess(successMessage);
+          });
+        notifications.toasts.addSuccess(message);

        onSuccess();
      } catch (error) {
-        const errorMessage = i18n.translate('xpack.fleet.updateAgentTags.errorNotificationTitle', {
-          defaultMessage: 'Tags update failed',
-        });
-        notifications.toasts.addError(error, { title: errorMessage });
+        const errorTitle =
+          errorMessage ??
+          i18n.translate('xpack.fleet.updateAgentTags.errorNotificationTitle', {
+            defaultMessage: 'Tags update failed',
+          });
+        notifications.toasts.addError(error, { title: errorTitle });
      }
    },
    [notifications.toasts]
  );

  const updateTags = useCallback(
-    async (agentId: string, newTags: string[], onSuccess: () => void) => {
+    async (
+      agentId: string,
+      newTags: string[],
+      onSuccess: () => void,
+      successMessage?: string,
+      errorMessage?: string
+    ) => {
      await wrapRequest(
        async () => await sendPutAgentTagsUpdate(agentId, { tags: newTags }),
-        onSuccess
+        onSuccess,
+        successMessage,
+        errorMessage
      );
    },
    [wrapRequest]
@ -59,11 +73,15 @@ export const useUpdateTags = () => {
      agents: string[] | string,
      tagsToAdd: string[],
      tagsToRemove: string[],
-      onSuccess: () => void
+      onSuccess: () => void,
+      successMessage?: string,
+      errorMessage?: string
    ) => {
      await wrapRequest(
        async () => await sendPostBulkAgentTagsUpdate({ agents, tagsToAdd, tagsToRemove }),
-        onSuccess
+        onSuccess,
+        successMessage,
+        errorMessage
      );
    },
    [wrapRequest]
--- a/x-pack/plugins/fleet/server/services/agents/update_agent_tags.test.ts
+++ b/x-pack/plugins/fleet/server/services/agents/update_agent_tags.test.ts
@ -0,0 +1,103 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import type { ElasticsearchClientMock } from '@kbn/core/server/mocks';
+import { elasticsearchServiceMock } from '@kbn/core/server/mocks';
+
+import { updateAgentTags } from './update_agent_tags';
+
+describe('update_agent_tags', () => {
+  let esClient: ElasticsearchClientMock;
+
+  beforeEach(() => {
+    esClient = elasticsearchServiceMock.createInternalClient();
+    esClient.mget.mockResolvedValue({
+      docs: [
+        {
+          _id: 'agent1',
+          _source: {
+            tags: ['one', 'two', 'three'],
+          },
+        } as any,
+      ],
+    });
+    esClient.bulk.mockResolvedValue({
+      items: [],
+    } as any);
+  });
+
+  function expectTagsInEsBulk(tags: string[]) {
+    expect(esClient.bulk).toHaveBeenCalledWith(
+      expect.objectContaining({
+        body: [
+          expect.anything(),
+          {
+            doc: expect.objectContaining({
+              tags,
+            }),
+          },
+        ],
+      })
+    );
+  }
+
+  it('should replace tag in middle place when one add and one remove tag', async () => {
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['newName'], ['two']);
+
+    expectTagsInEsBulk(['one', 'newName', 'three']);
+  });
+
+  it('should replace tag in first place when one add and one remove tag', async () => {
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['newName'], ['one']);
+
+    expectTagsInEsBulk(['newName', 'two', 'three']);
+  });
+
+  it('should replace tag in last place when one add and one remove tag', async () => {
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['newName'], ['three']);
+
+    expectTagsInEsBulk(['one', 'two', 'newName']);
+  });
+
+  it('should add tag when tagsToRemove does not exist', async () => {
+    esClient.mget.mockResolvedValue({
+      docs: [
+        {
+          _id: 'agent1',
+          _source: {},
+        } as any,
+      ],
+    });
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['newName'], ['three']);
+
+    expectTagsInEsBulk(['newName']);
+  });
+
+  it('should remove duplicate tags', async () => {
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['one'], ['two']);
+
+    expectTagsInEsBulk(['one', 'three']);
+  });
+
+  it('should add tag at the end when no tagsToRemove', async () => {
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['newName'], []);
+
+    expectTagsInEsBulk(['one', 'two', 'three', 'newName']);
+  });
+
+  it('should add tag at the end when tagsToRemove not in existing tags', async () => {
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['newName'], ['dummy']);
+
+    expectTagsInEsBulk(['one', 'two', 'three', 'newName']);
+  });
+
+  it('should add tag at the end when multiple tagsToRemove', async () => {
+    await updateAgentTags(esClient, { agentIds: ['agent1'] }, ['newName'], ['one', 'two']);
+
+    expectTagsInEsBulk(['three', 'newName']);
+  });
+});
--- a/x-pack/plugins/fleet/server/services/agents/update_agent_tags.ts
+++ b/x-pack/plugins/fleet/server/services/agents/update_agent_tags.ts
@ -87,12 +87,29 @@ async function updateTagsBatch(
 ): Promise<{ items: BulkActionResult[] }> {
  const errors: Record<Agent['id'], Error> = { ...outgoingErrors };

+  const getNewTags = (agent: Agent): string[] => {
+    const existingTags = agent.tags ?? [];
+
+    if (tagsToAdd.length === 1 && tagsToRemove.length === 1) {
+      const removableTagIndex = existingTags.indexOf(tagsToRemove[0]);
+      if (removableTagIndex > -1) {
+        const newTags = uniq([
+          ...existingTags.slice(0, removableTagIndex),
+          tagsToAdd[0],
+          ...existingTags.slice(removableTagIndex + 1),
+        ]);
+        return newTags;
+      }
+    }
+    return uniq(difference(existingTags, tagsToRemove).concat(tagsToAdd));
+  };
+
  await bulkUpdateAgents(
    esClient,
    givenAgents.map((agent) => ({
      agentId: agent.id,
      data: {
-        tags: uniq(difference(agent.tags ?? [], tagsToRemove).concat(tagsToAdd)),
+        tags: getNewTags(agent),
      },
    }))
  );