Fix Glossary tree hierarchy state (#18703)

* fix the tree state on edit glossary term

* add playwright tests

* fix permission loading

* fix tests

* adding timeout for in review state

* adding timeout for in review state

* increasing timeout

* fix tests

* update tests

* add a poll for checking tasks created

* testing response

* adding limit

* fix tests

* change user for testing

* Catch Exceptions on the Consumer to avoid losing records

---------

Co-authored-by: Pablo Takara <pjt1991@gmail.com>

openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowEventConsumer.java

@@ -1,9 +1,12 @@
 package org.openmetadata.service.governance.workflows;
 
+import static org.openmetadata.schema.entity.events.SubscriptionDestination.SubscriptionType.GOVERNANCE_WORKFLOW_CHANGE_EVENT;
+
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.tuple.Pair;
 import org.openmetadata.schema.entity.events.EventSubscription;
 import org.openmetadata.schema.entity.events.SubscriptionDestination;
 import org.openmetadata.schema.type.ChangeEvent;
@@ -13,6 +16,7 @@ import org.openmetadata.schema.type.Include;
 import org.openmetadata.service.Entity;
 import org.openmetadata.service.apps.bundles.changeEvent.Destination;
 import org.openmetadata.service.events.errors.EventPublisherException;
+import org.openmetadata.service.exception.CatalogExceptionMessage;
 import org.openmetadata.service.resources.feeds.MessageParser;
 
 @Slf4j
@@ -46,6 +50,7 @@ public class WorkflowEventConsumer implements Destination<ChangeEvent> {
   @Override
   public void sendMessage(ChangeEvent event) throws EventPublisherException {
     // NOTE: We are only consuming ENTITY related events.
+    try {
       EventType eventType = event.getEventType();
 
       if (validEventTypes.contains(eventType)) {
@@ -63,6 +68,16 @@ public class WorkflowEventConsumer implements Destination<ChangeEvent> {
 
         WorkflowHandler.getInstance().triggerWithSignal(signal, variables);
       }
+    } catch (Exception exc) {
+      String message =
+          CatalogExceptionMessage.eventPublisherFailedToPublish(
+              GOVERNANCE_WORKFLOW_CHANGE_EVENT, event, exc.getMessage());
+      LOG.error(message);
+      throw new EventPublisherException(
+          CatalogExceptionMessage.eventPublisherFailedToPublish(
+              GOVERNANCE_WORKFLOW_CHANGE_EVENT, exc.getMessage()),
+          Pair.of(subscriptionDestination.getId(), event));
+    }
   }
 
   @Override

openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts

@@ -61,11 +61,13 @@ import {
   selectActiveGlossaryTerm,
   selectColumns,
   toggleAllColumnsSelection,
+  updateGlossaryTermDataFromTree,
   validateGlossaryTerm,
   verifyAllColumns,
   verifyColumnsVisibility,
   verifyGlossaryDetails,
   verifyGlossaryTermAssets,
+  verifyTaskCreated,
 } from '../../utils/glossary';
 import { sidebarClick } from '../../utils/sidebar';
 import { TaskDetails } from '../../utils/task';
@@ -74,32 +76,32 @@ import { performUserLogin } from '../../utils/user';
 const user1 = new UserClass();
 const user2 = new UserClass();
 const team = new TeamClass();
+const user3 = new UserClass();
 
 test.describe('Glossary tests', () => {
   test.beforeAll(async ({ browser }) => {
     const { afterAction, apiContext } = await performAdminLogin(browser);
     await user2.create(apiContext);
     await user1.create(apiContext);
+    await user3.create(apiContext);
     team.data.users = [user2.responseData.id];
     await team.create(apiContext);
     await afterAction();
   });
 
-  test.fixme(
-    'Glossary & terms creation for reviewer as user',
-    async ({ browser }) => {
+  test('Glossary & terms creation for reviewer as user', async ({
+    browser,
+  }) => {
     test.slow(true);
 
-      const { page, afterAction, apiContext } = await performAdminLogin(
-        browser
-      );
+    const { page, afterAction, apiContext } = await performAdminLogin(browser);
     const { page: page1, afterAction: afterActionUser1 } =
-        await performUserLogin(browser, user1);
+      await performUserLogin(browser, user3);
     const glossary1 = new Glossary();
     glossary1.data.owners = [{ name: 'admin', type: 'user' }];
     glossary1.data.mutuallyExclusive = true;
     glossary1.data.reviewers = [
-        { name: `${user1.data.firstName}${user1.data.lastName}`, type: 'user' },
+      { name: `${user3.data.firstName}${user3.data.lastName}`, type: 'user' },
     ];
     glossary1.data.terms = [new GlossaryTerm(glossary1)];
 
@@ -118,10 +120,13 @@ test.describe('Glossary tests', () => {
       'Approve Glossary Term from Glossary Listing for reviewer user',
       async () => {
         await redirectToHomePage(page1);
-          // wait for 15 seconds as the flowable which creates task is triggered every 10 seconds
-          await page1.waitForTimeout(15000);
         await sidebarClick(page1, SidebarItem.GLOSSARY);
         await selectActiveGlossary(page1, glossary1.data.name);
+        await verifyTaskCreated(
+          page1,
+          glossary1.data.fullyQualifiedName,
+          glossary1.data.terms[0].data
+        );
 
         await approveGlossaryTermTask(page1, glossary1.data.terms[0].data);
         await redirectToHomePage(page1);
@@ -139,25 +144,20 @@ test.describe('Glossary tests', () => {
 
     await glossary1.delete(apiContext);
     await afterAction();
-    }
-  );
+  });
 
-  test.fixme(
-    'Glossary & terms creation for reviewer as team',
-    async ({ browser }) => {
+  test('Glossary & terms creation for reviewer as team', async ({
+    browser,
+  }) => {
     test.slow(true);
 
-      const { page, afterAction, apiContext } = await performAdminLogin(
-        browser
-      );
+    const { page, afterAction, apiContext } = await performAdminLogin(browser);
     const { page: page1, afterAction: afterActionUser1 } =
       await performUserLogin(browser, user2);
 
     const glossary2 = new Glossary();
     glossary2.data.owners = [{ name: 'admin', type: 'user' }];
-      glossary2.data.reviewers = [
-        { name: team.data.displayName, type: 'team' },
-      ];
+    glossary2.data.reviewers = [{ name: team.data.displayName, type: 'team' }];
     glossary2.data.terms = [new GlossaryTerm(glossary2)];
 
     await test.step('Create Glossary', async () => {
@@ -176,10 +176,15 @@ test.describe('Glossary tests', () => {
       'Approve Glossary Term from Glossary Listing for reviewer team',
       async () => {
         await redirectToHomePage(page1);
-          // wait for 15 seconds as the flowable which creates task is triggered every 10 seconds
-          await page1.waitForTimeout(15000);
         await sidebarClick(page1, SidebarItem.GLOSSARY);
         await selectActiveGlossary(page1, glossary2.data.name);
+
+        await verifyTaskCreated(
+          page1,
+          glossary2.data.fullyQualifiedName,
+          glossary2.data.terms[0].data
+        );
+
         await approveGlossaryTermTask(page1, glossary2.data.terms[0].data);
 
         await redirectToHomePage(page1);
@@ -197,8 +202,7 @@ test.describe('Glossary tests', () => {
 
     await glossary2.delete(apiContext);
     await afterAction();
-    }
-  );
+  });
 
   test('Update Glossary and Glossary Term', async ({ browser }) => {
     test.slow(true);
@@ -789,9 +793,8 @@ test.describe('Glossary tests', () => {
         '[data-testid="viewer-container"]'
       );
 
-      await expect(viewerContainerText).toContain('Updated description');
+      expect(viewerContainerText).toContain('Updated description');
     } finally {
-      await user1.delete(apiContext);
       await glossary1.delete(apiContext);
       await afterAction();
     }
@@ -836,9 +839,8 @@ test.describe('Glossary tests', () => {
         '[data-testid="viewer-container"]'
       );
 
-      await expect(viewerContainerText).toContain('Updated description');
+      expect(viewerContainerText).toContain('Updated description');
     } finally {
-      await user1.delete(apiContext);
       await glossaryTerm1.delete(apiContext);
       await glossary1.delete(apiContext);
       await afterAction();
@@ -870,6 +872,7 @@ test.describe('Glossary tests', () => {
     } finally {
       await glossary1.delete(apiContext);
       await afterAction();
+      await afterActionUser1();
     }
   });
 
@@ -1063,10 +1066,60 @@ test.describe('Glossary tests', () => {
     }
   });
 
+  test('Glossary Term Update in Glossary Page should persist tree', async ({
+    browser,
+  }) => {
+    const { page, afterAction, apiContext } = await performAdminLogin(browser);
+    const glossary1 = new Glossary();
+    const glossaryTerm1 = new GlossaryTerm(glossary1);
+    await glossary1.create(apiContext);
+    await glossaryTerm1.create(apiContext);
+    const glossaryTerm2 = new GlossaryTerm(
+      glossary1,
+      glossaryTerm1.responseData.fullyQualifiedName
+    );
+    await glossaryTerm2.create(apiContext);
+    const glossaryTerm3 = new GlossaryTerm(
+      glossary1,
+      glossaryTerm2.responseData.fullyQualifiedName
+    );
+    await glossaryTerm3.create(apiContext);
+
+    try {
+      await sidebarClick(page, SidebarItem.GLOSSARY);
+      await selectActiveGlossary(page, glossary1.data.displayName);
+      await page.getByTestId('expand-collapse-all-button').click();
+
+      await expect(
+        page.getByRole('cell', { name: glossaryTerm1.data.displayName })
+      ).toBeVisible();
+
+      await expect(
+        page.getByRole('cell', { name: glossaryTerm2.data.displayName })
+      ).toBeVisible();
+
+      await expect(
+        page.getByRole('cell', { name: glossaryTerm3.data.displayName })
+      ).toBeVisible();
+
+      await updateGlossaryTermDataFromTree(
+        page,
+        glossaryTerm2.responseData.fullyQualifiedName
+      );
+    } finally {
+      await glossaryTerm3.delete(apiContext);
+      await glossaryTerm2.delete(apiContext);
+      await glossaryTerm1.delete(apiContext);
+      await glossary1.delete(apiContext);
+      await afterAction();
+    }
+  });
+
   test.afterAll(async ({ browser }) => {
     const { afterAction, apiContext } = await performAdminLogin(browser);
     await user1.delete(apiContext);
     await user2.delete(apiContext);
+    await user3.create(apiContext);
     await team.delete(apiContext);
     await afterAction();
   });

openmetadata-ui/src/main/resources/ui/playwright/utils/glossary.ts

@@ -37,6 +37,12 @@ import { addMultiOwner } from './entity';
 import { sidebarClick } from './sidebar';
 import { TaskDetails, TASK_OPEN_FETCH_LINK } from './task';
 
+type TaskEntity = {
+  entityRef: {
+    name: string;
+  };
+};
+
 const GLOSSARY_NAME_VALIDATION_ERROR = 'Name size must be between 1 and 128';
 
 export const descriptionBox =
@@ -469,8 +475,47 @@ export const fillGlossaryTermDetails = async (
   }
 };
 
-const validateGlossaryTermTask = async (page: Page, term: GlossaryTermData) => {
+export const verifyTaskCreated = async (
+  page: Page,
+  glossaryFqn: string,
+  glossaryTermData: GlossaryTermData
+) => {
+  const { apiContext } = await getApiContext(page);
+  const entityLink = encodeURIComponent(`<#E::glossary::${glossaryFqn}>`);
+
+  await expect
+    .poll(
+      async () => {
+        const response = await apiContext
+          .get(
+            `/api/v1/feed?entityLink=${entityLink}&type=Task&taskStatus=Open`
+          )
+          .then((res) => res.json());
+
+        const arr = response.data.map(
+          (item: TaskEntity) => item.entityRef.name
+        );
+
+        return arr;
+      },
+      {
+        // Custom expect message for reporting, optional.
+        message: 'To get the last run execution status as success',
+        timeout: 200_000,
+        intervals: [30_000],
+      }
+    )
+    .toContain(glossaryTermData.name);
+};
+
+export const validateGlossaryTermTask = async (
+  page: Page,
+  term: GlossaryTermData
+) => {
+  const taskCountRes = page.waitForResponse('/api/v1/feed/count?*');
   await page.click('[data-testid="activity_feed"]');
+  await taskCountRes;
+
   const taskFeeds = page.waitForResponse(TASK_OPEN_FETCH_LINK);
   await page
     .getByTestId('global-setting-left-panel')
@@ -504,6 +549,37 @@ export const approveGlossaryTermTask = async (
   await toastNotification(page, /Task resolved successfully/);
 };
 
+// Show the glossary term edit modal from glossary page tree.
+// Update the description and verify the changes.
+export const updateGlossaryTermDataFromTree = async (
+  page: Page,
+  termFqn: string
+) => {
+  // eslint-disable-next-line no-useless-escape
+  const escapedFqn = termFqn.replace(/\"/g, '\\"');
+  const termRow = page.locator(`[data-row-key="${escapedFqn}"]`);
+  await termRow.getByTestId('edit-button').click();
+
+  await page.waitForSelector('[role="dialog"].edit-glossary-modal');
+
+  await expect(
+    page.locator('[role="dialog"].edit-glossary-modal')
+  ).toBeVisible();
+  await expect(page.locator('.ant-modal-title')).toContainText(
+    'Edit Glossary Term'
+  );
+
+  await page.locator(descriptionBox).fill('Updated description');
+
+  const glossaryTermResponse = page.waitForResponse('/api/v1/glossaryTerms/*');
+  await page.getByTestId('save-glossary-term').click();
+  await glossaryTermResponse;
+
+  await expect(
+    termRow.getByRole('cell', { name: 'Updated description' })
+  ).toBeVisible();
+};
+
 export const validateGlossaryTerm = async (
   page: Page,
   term: GlossaryTermData,
@@ -516,13 +592,6 @@ export const validateGlossaryTerm = async (
 
   await expect(page.locator(termSelector)).toContainText(term.name);
   await expect(page.locator(statusSelector)).toContainText(status);
-
-  if (status === 'Draft') {
-    // wait for 15 seconds as the flowable which creates task is triggered every 10 seconds
-    await page.waitForTimeout(15000);
-    await validateGlossaryTermTask(page, term);
-    await page.click('[data-testid="terms"]');
-  }
 };
 
 export const createGlossaryTerm = async (

openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryDetails/GlossaryDetails.component.tsx

@@ -270,7 +270,7 @@ const GlossaryDetails = ({
         {getWidgetFromKeyInternal(widget)}
       </div>
     ));
-  }, [tagsWidget, layout]);
+  }, [tagsWidget, layout, permissions, termsLoading]);
 
   const detailsContent = useMemo(() => {
     return (

openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx

@@ -114,6 +114,10 @@ const GlossaryTermTab = ({
   const [isTableLoading, setIsTableLoading] = useState(false);
   const [isTableHovered, setIsTableHovered] = useState(false);
   const [expandedRowKeys, setExpandedRowKeys] = useState<string[]>([]);
+  const listOfVisibleColumns = useMemo(() => {
+    return ['name', 'description', 'owners', 'status', 'new-term'];
+  }, []);
+
   const [isStatusDropdownVisible, setIsStatusDropdownVisible] =
     useState<boolean>(false);
   const statusOptions = useMemo(
@@ -123,7 +127,7 @@ const GlossaryTermTab = ({
   );
   const [statusDropdownSelection, setStatusDropdownSelections] = useState<
     string[]
-  >(['Approved', 'Draft']);
+  >([Status.Approved, Status.Draft, Status.InReview]);
   const [selectedStatus, setSelectedStatus] = useState<string[]>([
     ...statusDropdownSelection,
   ]);
@@ -296,11 +300,7 @@ const GlossaryTermTab = ({
     }
 
     return data;
-  }, [glossaryTerms, permissions]);
-
-  const listOfVisibleColumns = useMemo(() => {
-    return ['name', 'description', 'owners', 'status', 'new-term'];
-  }, []);
+  }, [permissions]);
 
   const defaultCheckedList = useMemo(
     () =>
@@ -353,7 +353,7 @@ const GlossaryTermTab = ({
 
           return aIndex - bIndex;
         }),
-    [newColumns]
+    [options, newColumns]
   );
 
   const handleColumnSelectionDropdownSave = useCallback(() => {

openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryV1.component.tsx

@@ -41,6 +41,7 @@ import {
   patchGlossaryTerm,
 } from '../../rest/glossaryAPI';
 import { getEntityDeleteMessage } from '../../utils/CommonUtils';
+import { updateGlossaryTermByFqn } from '../../utils/GlossaryUtils';
 import { DEFAULT_ENTITY_PERMISSION } from '../../utils/PermissionsUtils';
 import { showErrorToast } from '../../utils/ToastUtils';
 import { useActivityFeedProvider } from '../ActivityFeed/ActivityFeedProvider/ActivityFeedProvider';
@@ -84,6 +85,7 @@ const GlossaryV1 = ({
   const { getEntityPermission } = usePermissionProvider();
   const [isLoading, setIsLoading] = useState(true);
   const [isTermsLoading, setIsTermsLoading] = useState(false);
+  const [isPermissionLoading, setIsPermissionLoading] = useState(false);
 
   const [isDelete, setIsDelete] = useState<boolean>(false);
 
@@ -97,7 +99,8 @@ const GlossaryV1 = ({
 
   const [editMode, setEditMode] = useState(false);
 
-  const { activeGlossary, setGlossaryChildTerms } = useGlossaryStore();
+  const { activeGlossary, glossaryChildTerms, setGlossaryChildTerms } =
+    useGlossaryStore();
 
   const { id, fullyQualifiedName } = activeGlossary ?? {};
 
@@ -206,6 +209,17 @@ const GlossaryV1 = ({
     setIsEditModalOpen(true);
   };
 
+  const updateGlossaryTermInStore = (updatedTerm: GlossaryTerm) => {
+    const clonedTerms = cloneDeep(glossaryChildTerms);
+    const updatedGlossaryTerms = updateGlossaryTermByFqn(
+      clonedTerms,
+      updatedTerm.fullyQualifiedName ?? '',
+      updatedTerm as ModifiedGlossary
+    );
+
+    setGlossaryChildTerms(updatedGlossaryTerms);
+  };
+
   const updateGlossaryTerm = async (
     currentData: GlossaryTerm,
     updatedData: GlossaryTerm
@@ -217,8 +231,10 @@ const GlossaryV1 = ({
         throw t('server.entity-updating-error', {
           entity: t('label.glossary-term'),
         });
+      } else {
+        updateGlossaryTermInStore(response);
+        setIsEditModalOpen(false);
       }
-      onTermModalSuccess();
     } catch (error) {
       if (
         (error as AxiosError).response?.status === HTTP_STATUS_CODE.CONFLICT
@@ -336,18 +352,29 @@ const GlossaryV1 = ({
     shouldRefreshTerms && loadGlossaryTerms(true);
   };
 
+  const initPermissions = async () => {
+    setIsPermissionLoading(true);
+    const permissionFetch = isGlossaryActive
+      ? fetchGlossaryPermission
+      : fetchGlossaryTermPermission;
+
+    try {
+      if (isVersionsView) {
+        isGlossaryActive
+          ? setGlossaryPermission(VERSION_VIEW_GLOSSARY_PERMISSION)
+          : setGlossaryTermPermission(VERSION_VIEW_GLOSSARY_PERMISSION);
+      } else {
+        await permissionFetch();
+      }
+    } finally {
+      setIsPermissionLoading(false);
+    }
+  };
+
   useEffect(() => {
     if (id && !action) {
       loadGlossaryTerms();
-      if (isGlossaryActive) {
-        isVersionsView
-          ? setGlossaryPermission(VERSION_VIEW_GLOSSARY_PERMISSION)
-          : fetchGlossaryPermission();
-      } else {
-        isVersionsView
-          ? setGlossaryTermPermission(VERSION_VIEW_GLOSSARY_PERMISSION)
-          : fetchGlossaryTermPermission();
-      }
+      initPermissions();
     }
   }, [id, isGlossaryActive, isVersionsView, action]);
 
@@ -355,8 +382,9 @@ const GlossaryV1 = ({
     <ImportGlossary glossaryName={selectedData.fullyQualifiedName ?? ''} />
   ) : (
     <>
-      {isLoading && <Loader />}
+      {(isLoading || isPermissionLoading) && <Loader />}
       {!isLoading &&
+        !isPermissionLoading &&
         !isEmpty(selectedData) &&
         (isGlossaryActive ? (
           <GlossaryDetails

openmetadata-ui/src/main/resources/ui/src/utils/GlossaryUtils.tsx

@@ -163,6 +163,30 @@ export const getGlossaryBreadcrumbs = (fqn: string) => {
   return breadcrumbList;
 };
 
+export const updateGlossaryTermByFqn = (
+  glossaryTerms: ModifiedGlossary[],
+  fqn: string,
+  newValue: ModifiedGlossary
+): ModifiedGlossary[] => {
+  return glossaryTerms.map((term) => {
+    if (term.fullyQualifiedName === fqn) {
+      return newValue;
+    }
+    if (term.children) {
+      return {
+        ...term,
+        children: updateGlossaryTermByFqn(
+          term.children as ModifiedGlossary[],
+          fqn,
+          newValue
+        ),
+      };
+    }
+
+    return term;
+  }) as ModifiedGlossary[];
+};
+
 // This function finds and gives you the glossary term you're looking for.
 // You can then use this term or update its information in the Glossary or Term with it's reference created
 // Reference will only be created if withReference is true