Merge pull request #7469 from jeff-phillips-18/fix-section-sort

Bug 1906768: Fix to correctly sort nav items dependent on non-plugin items
openshift · Dec 11, 2020 · 791c866 · 791c866
2 parents 1d880c0 + 4d98f26
commit 791c866
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 15 deletions.
diff --git a/frontend/__tests__/components/nav/navSortUtils.spec.ts b/frontend/__tests__/components/nav/navSortUtils.spec.ts
@@ -1,4 +1,7 @@
-import { getSortedNavItems } from '@console/internal/components/nav/navSortUtils';
+import {
+  getSortedNavItems,
+  sortExtensionItems,
+} from '@console/internal/components/nav/navSortUtils';
 import { LoadedExtension, NavItem, NavSection, SeparatorNavItem } from '@console/plugin-sdk/src';
 
 const mockNavItems: LoadedExtension<NavSection | NavItem | SeparatorNavItem>[] = [
@@ -70,6 +73,8 @@ const mockNavItems: LoadedExtension<NavSection | NavItem | SeparatorNavItem>[] =
   },
 ];
 
+const indexOfId = (id, sortedItems) => sortedItems.map((i) => i.properties.id).indexOf(id);
+
 describe('perspective-nav insertPositionedItems', () => {
   it('should order items that are not positioned', () => {
     const sortedItems = getSortedNavItems(mockNavItems);
@@ -81,42 +86,63 @@ describe('perspective-nav insertPositionedItems', () => {
   it('should order items that are positioned', () => {
     mockNavItems[0].properties.insertAfter = 'test2';
     let sortedItems = getSortedNavItems(mockNavItems);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test1')).toBe(1);
+    expect(indexOfId('test1', sortedItems)).toBe(1);
 
     delete mockNavItems[0].properties.insertAfter;
     mockNavItems[0].properties.insertBefore = 'test5';
     sortedItems = getSortedNavItems(mockNavItems);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test1')).toBe(3);
+    expect(indexOfId('test1', sortedItems)).toBe(3);
 
     delete mockNavItems[0].properties.insertBefore;
     mockNavItems[0].properties.insertAfter = ['x', 'y', 'test3', 'z'];
     sortedItems = getSortedNavItems(mockNavItems);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test1')).toBe(2);
+    expect(indexOfId('test1', sortedItems)).toBe(2);
 
     delete mockNavItems[0].properties.insertAfter;
     mockNavItems[0].properties.insertBefore = ['x', 'y', 'test3', 'z'];
     sortedItems = getSortedNavItems(mockNavItems);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test1')).toBe(1);
+    expect(indexOfId('test1', sortedItems)).toBe(1);
 
     // Before takes precedence
     mockNavItems[0].properties.insertAfter = 'test6';
     sortedItems = getSortedNavItems(mockNavItems);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test1')).toBe(1);
+    expect(indexOfId('test1', sortedItems)).toBe(1);
   });
 
   it('should order items that are positioned on positioned items', () => {
     delete mockNavItems[0].properties.insertBefore;
     mockNavItems[0].properties.insertAfter = 'test6';
     mockNavItems[5].properties.insertAfter = 'test4';
     let sortedItems = getSortedNavItems(mockNavItems);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test6')).toBe(3);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test1')).toBe(4);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test7')).toBe(6);
+    expect(indexOfId('test6', sortedItems)).toBe(3);
+    expect(indexOfId('test1', sortedItems)).toBe(4);
+    expect(indexOfId('test7', sortedItems)).toBe(6);
 
     mockNavItems[6].properties.insertBefore = 'test1';
     sortedItems = getSortedNavItems(mockNavItems);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test6')).toBe(3);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test1')).toBe(5);
-    expect(sortedItems.map((i) => i.properties.id).indexOf('test7')).toBe(4);
+    expect(indexOfId('test6', sortedItems)).toBe(3);
+    expect(indexOfId('test1', sortedItems)).toBe(5);
+    expect(indexOfId('test7', sortedItems)).toBe(4);
+  });
+
+  it('should sort dependency items correctly', () => {
+    mockNavItems.forEach((i) => {
+      delete i.properties.insertAfter;
+      delete i.properties.insertBefore;
+    });
+    mockNavItems[0].properties.insertAfter = 'test4';
+    mockNavItems[2].properties.insertAfter = 'test5';
+    mockNavItems[5].properties.insertAfter = 'test7';
+    mockNavItems[6].properties.insertBefore = 'test1';
+
+    const sortedItems = sortExtensionItems(mockNavItems);
+    // test1 depends on test4
+    expect(indexOfId('test1', sortedItems)).toBeGreaterThan(indexOfId('test4', sortedItems));
+    // test3 depends on test5
+    expect(indexOfId('test3', sortedItems)).toBeGreaterThan(indexOfId('test5', sortedItems));
+    // test6 depends on test7
+    expect(indexOfId('test6', sortedItems)).toBeGreaterThan(indexOfId('test7', sortedItems));
+    // test7 depends on test1
+    expect(indexOfId('test7', sortedItems)).toBeGreaterThan(indexOfId('test1', sortedItems));
   });
 });
diff --git a/frontend/public/components/nav/navSortUtils.ts b/frontend/public/components/nav/navSortUtils.ts
@@ -84,3 +84,30 @@ export const getSortedNavItems = (navItems: (PluginNavSection | NavItem | Separa
   insertPositionedItems(positionedItems, sortedItems);
   return sortedItems;
 };
+
+export const sortExtensionItems = (
+  extensionItems: (PluginNavSection | NavItem | SeparatorNavItem)[],
+) => {
+  // Mapped by item id
+  const mappedIds = extensionItems.reduce((mem, i) => {
+    mem[i.properties.id] = i;
+    return mem;
+  }, {});
+
+  // determine all dependencies for a given id
+  const dependencies = (i) => {
+    const { insertBefore, insertAfter } = mappedIds[i].properties;
+    const before = Array.isArray(insertBefore) ? insertBefore : [insertBefore];
+    const after = Array.isArray(insertAfter) ? insertAfter : [insertAfter];
+    const dependencyIds = [...before, ...after];
+    return dependencyIds.reduce((acc, index) => {
+      if (index) {
+        // Add this dependency and its dependencies
+        return [...acc, index, ...dependencies(index)];
+      }
+      return acc;
+    }, []);
+  };
+
+  return extensionItems.sort((a, b) => dependencies(b.properties.id).indexOf(a.properties.id) * -1);
+};
diff --git a/frontend/public/components/nav/section.tsx b/frontend/public/components/nav/section.tsx
@@ -15,7 +15,7 @@ import { RootState } from '../../redux';
 import { featureReducerName, flagPending, FeatureState } from '../../reducers/features';
 import { stripBasePath } from '../utils';
 import { stripNS, createLink } from './items';
-import { getSortedNavItems } from './navSortUtils';
+import { sortExtensionItems } from './navSortUtils';
 
 const navSectionStateToProps = (
   state: RootState,
@@ -32,6 +32,37 @@ const navSectionStateToProps = (
   };
 };
 
+const findChildIndex = (id: string, Children: React.ReactElement[]) =>
+  Children.findIndex((c) => c.props.id === id);
+
+const mergePluginChild = (
+  Children: React.ReactElement[],
+  pluginChild: React.ReactElement,
+  insertBefore?: string | string[],
+  insertAfter?: string | string[],
+) => {
+  let index = -1;
+  const before = Array.isArray(insertBefore) ? insertBefore : [insertBefore];
+  const after = Array.isArray(insertAfter) ? insertAfter : [insertAfter];
+  let count = 0;
+  while (count < before.length && index < 0) {
+    index = findChildIndex(before[count++], Children);
+  }
+  count = 0;
+  while (count < after.length && index < 0) {
+    index = findChildIndex(after[count++], Children);
+    if (index >= 0) {
+      index += 1;
+    }
+  }
+
+  if (index >= 0) {
+    Children.splice(index, 0, pluginChild);
+  } else {
+    Children.push(pluginChild);
+  }
+};
+
 export const NavSection = connect(navSectionStateToProps)(
   withExtensions<NavSectionExtensionProps>({
     navItemExtensions: isNavItem,
@@ -158,11 +189,18 @@ export const NavSection = connect(navSectionStateToProps)(
         getChildren() {
           const { id, title, children, activePerspective: perspective } = this.props;
           const Children = React.Children.map(children, this.mapChild) || [];
-          const childItems = getSortedNavItems(this.getNavItemExtensions(perspective, title, id));
+          const childItems = sortExtensionItems(this.getNavItemExtensions(perspective, title, id));
 
           childItems.forEach((item) => {
             const pluginChild = this.mapChild(createLink(item as NavItem));
-            Children.push(pluginChild);
+            if (pluginChild) {
+              mergePluginChild(
+                Children,
+                pluginChild,
+                item.properties.insertBefore,
+                item.properties.insertAfter,
+              );
+            }
           });
 
           return Children;