finos#1074 fix state updates issue in useFilterBar hook (finos#1135)

- fixes it by removing the use of useEffect to listen for changes in filters and active indices and then call onApplyFilter. Now I've made the calls to onApplyFilter explicit making it easier to maintain and read. - with this change I've also fixed a bug in filter deletion: Cause: we used to delete an active filter's index from active filter indices array without adjusting the rest of the indices. Required adjustment: if a filter gets deleted from filters array then the index of all the filters on the right are decremented by 1. But we weren't making this adjustment with active indices array. - and have added component tests to prevent this filter deletion bug from being re-introduced.
UBS-IB · Jan 22, 2024 · 190d1bc · 190d1bc
1 parent b1fe998
commit 190d1bc
Show file tree

Hide file tree

Showing 6 changed files with 236 additions and 116 deletions.
diff --git a/.gitignore b/.gitignore
@@ -5,11 +5,7 @@
 /.idea/
 target/
 /gc.log
-/toolbox/toolbox.iml
-/example/**/*.iml
-/vuu-ui/vuu-ui.iml
-/vuu/vuu.iml
-/vuu-parent.iml
+/**/*.iml
 /vuu-book/src/.DS_Store
 .DS_Store
 

diff --git a/vuu-ui/packages/vuu-filters/src/__tests__/__component__/filter-bar/Filterbar.cy.tsx b/vuu-ui/packages/vuu-filters/src/__tests__/__component__/filter-bar/Filterbar.cy.tsx
@@ -227,6 +227,87 @@ describe("The mouse user", () => {
       });
     });
   });
+
+  describe("WHEN adds two filters", () => {
+    const filter1 = {
+      column: "currency",
+      op: "!=",
+      value: "USD",
+      name: 'currency != "USD"',
+    };
+    const filter2 = {
+      column: "currency",
+      op: "!=",
+      value: "CAD",
+      name: 'currency != "CAD"',
+    };
+
+    beforeEach(() => {
+      const onFiltersChanged = cy.stub().as("filtersChangedHandler");
+      const onApplyFilter = cy.stub().as("applyFilterHandler");
+      cy.mount(
+        <DefaultFilterBar
+          onApplyFilter={onApplyFilter}
+          onFiltersChanged={onFiltersChanged}
+        />
+      );
+      cy.get(ADD_BUTTON).realClick();
+      clickListItems(filter1.column, filter1.op, filter1.value);
+      clickButton("APPLY AND SAVE");
+      waitUntilEditableLabelIsFocused(".vuuFilterPill");
+      cy.realPress("Enter");
+
+      cy.get(ADD_BUTTON).realClick();
+      clickListItems(filter2.column, filter2.op, filter2.value);
+      clickButton("APPLY AND SAVE");
+      waitUntilEditableLabelIsFocused(".vuuFilterPill");
+      cy.realPress("Enter");
+    });
+
+    it("THEN filtersChangedHandler & applyFilterHandler callbacks are invoked with correct values", () => {
+      cy.get("@filtersChangedHandler").should("be.calledWith", [
+        filter1,
+        filter2,
+      ]);
+
+      cy.get("@applyFilterHandler").should("be.calledWith", {
+        filter: 'currency != "USD" and currency != "CAD"',
+        filterStruct: { op: "and", filters: [filter1, filter2] },
+      });
+    });
+
+    it("AND WHEN second filter is deleted THEN changes are correctly applied", () => {
+      findOverflowItem('[data-index="1"]')
+        .find(".vuuFilterPillMenu")
+        .realClick();
+      clickButton("Delete");
+      clickButton("Remove");
+
+      cy.get("@filtersChangedHandler").should("be.calledWithExactly", [
+        filter1,
+      ]);
+      cy.get("@applyFilterHandler").should("be.calledWithExactly", {
+        filter: 'currency != "USD"',
+        filterStruct: filter1,
+      });
+    });
+
+    it("AND WHEN first filter is deleted THEN changes are correctly applied", () => {
+      findOverflowItem('[data-index="0"]')
+        .find(".vuuFilterPillMenu")
+        .realClick();
+      clickButton("Delete");
+      clickButton("Remove");
+
+      cy.get("@filtersChangedHandler").should("be.calledWithExactly", [
+        filter2,
+      ]);
+      cy.get("@applyFilterHandler").should("be.calledWithExactly", {
+        filter: 'currency != "CAD"',
+        filterStruct: filter2,
+      });
+    });
+  });
 });
 
 describe("The keyboard user", () => {

diff --git a/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterBar.ts b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterBar.ts
@@ -23,7 +23,6 @@ import {
   KeyboardEventHandler,
   RefObject,
   useCallback,
-  useEffect,
   useMemo,
   useRef,
   useState,
@@ -65,8 +64,6 @@ export const useFilterBar = ({
 }: FilterBarHookProps) => {
   const addButtonRef = useRef<HTMLButtonElement>(null);
   const editingFilter = useRef<Filter | undefined>();
-  const [activeFilterIndex, setActiveFilterIndex] =
-    useState<number[]>(activeFilterIdexProp);
   const [showMenu, setShowMenu] = useState(showMenuProp);
   const [editFilter, setEditFilter] = useState<
     Partial<Filter> | FilterWithPartialClause | undefined
@@ -78,13 +75,25 @@ export const useFilterBar = ({
     [columnDescriptors]
   );
 
+  const applyFilter = useCallback(
+    (filter?: Filter) => {
+      const query = filter ? filterAsQuery(filter, { columnsByName }) : "";
+      onApplyFilter({ filter: query, filterStruct: filter });
+    },
+    [columnsByName, onApplyFilter]
+  );
+
   const {
+    activeFilterIndex,
     filters,
     onAddFilter,
     onChangeFilter,
     onDeleteFilter,
     onRenameFilter,
+    onChangeActiveFilterIndex,
   } = useFilters({
+    activeFilterIndex: activeFilterIdexProp,
+    applyFilter,
     filters: filtersProp,
     onFiltersChanged,
     tableSchema,
@@ -142,29 +151,9 @@ export const useFilterBar = ({
     [containerRef]
   );
 
-  const applyFilter = useCallback(
-    (filter?: Filter) => {
-      const query = filter ? filterAsQuery(filter, { columnsByName }) : "";
-      onApplyFilter({ filter: query, filterStruct: filter });
-    },
-    [columnsByName, onApplyFilter]
-  );
-
   const deleteConfirmed = useCallback(
     (filter: Filter) => {
-      const indexOfFilter = filters.indexOf(filter);
-      if (activeFilterIndex.includes(indexOfFilter)) {
-        // deselect filter
-        setActiveFilterIndex(
-          activeFilterIndex.filter((i) => i !== indexOfFilter)
-        );
-      }
-      const indexOfDeletedFilter = onDeleteFilter?.(filter);
-      if (activeFilterIndex.includes(indexOfDeletedFilter)) {
-        setActiveFilterIndex((indices) =>
-          indices.filter((i) => i !== indexOfDeletedFilter)
-        );
-      }
+      onDeleteFilter(filter);
 
       // TODO move focus to next/previous filter
       requestAnimationFrame(() => {
@@ -173,7 +162,7 @@ export const useFilterBar = ({
         }
       });
     },
-    [activeFilterIndex, filters, focusFilterPill, onDeleteFilter]
+    [focusFilterPill, onDeleteFilter]
   );
 
   const getDeletePrompt = useMemo(
@@ -263,9 +252,8 @@ export const useFilterBar = ({
       if (existing === undefined) {
         const idx = onAddFilter(edited);
         editPillLabel(idx);
-        return idx;
       } else {
-        return onChangeFilter(existing, edited);
+        onChangeFilter(existing, edited);
       }
     },
     [editPillLabel, onAddFilter, onChangeFilter]
@@ -276,8 +264,7 @@ export const useFilterBar = ({
       switch (menuId) {
         case "apply-save": {
           const editedFilter = editFilter as Filter;
-          const idx = addIfNewElseUpdate(editedFilter, editingFilter.current);
-          setActiveFilterIndex(appendIfNotPresent(idx));
+          addIfNewElseUpdate(editedFilter, editingFilter.current);
           setEditFilter(undefined);
           editingFilter.current = undefined;
           setShowMenu(false);
@@ -307,31 +294,12 @@ export const useFilterBar = ({
     [editFilter, addIfNewElseUpdate]
   );
 
-  useEffect(() => {
-    if (activeFilterIndex.length > 0) {
-      const activeFilters = activeFilterIndex.map<Filter>(
-        (index) => filters[index]
-      );
-      if (activeFilters.length === 1) {
-        const [filter] = activeFilters;
-        applyFilter(filter);
-      } else {
-        applyFilter({
-          op: "and",
-          filters: activeFilters,
-        });
-      }
-    } else {
-      applyFilter();
-    }
-  }, [activeFilterIndex, applyFilter, filters]);
-
   const handleChangeActiveFilterIndex = useCallback<ActiveItemChangeHandler>(
     (itemIndex) => {
-      setActiveFilterIndex(itemIndex);
+      onChangeActiveFilterIndex(itemIndex);
       onChangeActiveFilterIndexProp?.(itemIndex);
     },
-    [onChangeActiveFilterIndexProp]
+    [onChangeActiveFilterIndexProp, onChangeActiveFilterIndex]
   );
 
   const handleClickAddFilter = useCallback(() => {
@@ -350,7 +318,6 @@ export const useFilterBar = ({
 
   const handleChangeFilterClause = useCallback(
     (idx: number) => (filterClause: Partial<FilterClause>) => {
-      console.log(`handleCHangeFilterClause ${JSON.stringify(filterClause)}`);
       if (filterClause !== undefined) {
         setEditFilter((ef) => replaceClause(ef, filterClause, idx));
         setShowMenu(true);
@@ -477,9 +444,6 @@ export const useFilterBar = ({
   };
 };
 
-const appendIfNotPresent = (n: number) => (ns: number[]) =>
-  ns.includes(n) ? ns : ns.concat(n);
-
 function columnDescriptorsByName(
   columns: ColumnDescriptor[]
 ): ColumnDescriptorsByName {

diff --git a/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterState.ts b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterState.ts
@@ -0,0 +1,67 @@
+import { useCallback, useState } from "react";
+import { Filter } from "@finos/vuu-filter-types";
+import { useControlled } from "@finos/vuu-ui-controls";
+
+export interface FilterStateHookProps {
+  activeFilterIndex: number[];
+  applyFilter: (f?: Filter) => void;
+  defaultFilters?: Filter[];
+  filters?: Filter[];
+}
+
+export function useFilterState({
+  activeFilterIndex: activeFilterIdexProp,
+  applyFilter,
+  defaultFilters,
+  filters: filtersProp,
+}: FilterStateHookProps) {
+  const [filters, setFilters] = useControlled<Filter[]>({
+    controlled: filtersProp,
+    default: defaultFilters ?? [],
+    name: "useFilters",
+    state: "Filters",
+  });
+
+  const [activeIndices, setActiveIndices] =
+    useState<number[]>(activeFilterIdexProp);
+
+  const onApplyFilter = useCallback(
+    ({ activeIndices, filters }: FilterState) => {
+      if (activeIndices.length > 0) {
+        const activeFilters = activeIndices.map((i) => filters[i]);
+        if (activeFilters.length === 1) {
+          const [filter] = activeFilters;
+          applyFilter(filter);
+        } else {
+          applyFilter({ op: "and", filters: activeFilters });
+        }
+      } else {
+        applyFilter();
+      }
+    },
+    [applyFilter]
+  );
+
+  const onFilterStateChange = useCallback(
+    ({ filters, activeIndices }: FilterState) => {
+      setFilters(filters);
+      setActiveIndices(activeIndices);
+      onApplyFilter({ filters, activeIndices });
+    },
+    [onApplyFilter]
+  );
+
+  const handleActiveIndicesChange = useCallback(
+    (indices: number[]) =>
+      onFilterStateChange({ filters, activeIndices: indices }),
+    [filters, onFilterStateChange]
+  );
+
+  return {
+    filterState: { activeIndices, filters },
+    onActiveIndicesChange: handleActiveIndicesChange,
+    onFilterStateChange,
+  };
+}
+
+type FilterState = { filters: Filter[]; activeIndices: number[] };