[Backport workspace] Left navigation menu adjustment (#192)

src/core/public/chrome/chrome_service.mock.ts

@@ -43,9 +43,9 @@ const createStartContractMock = () => {
   const startContract: DeeplyMockedKeys<InternalChromeStart> = {
     getHeaderComponent: jest.fn(),
     navLinks: {
+      setNavLinks: jest.fn(),
       getNavLinks$: jest.fn(),
-      getFilteredNavLinks$: jest.fn(),
-      setFilteredNavLinks: jest.fn(),
+      getAllNavLinks$: jest.fn(),
       has: jest.fn(),
       get: jest.fn(),
       getAll: jest.fn(),

src/core/public/chrome/chrome_service.tsx

@@ -277,7 +277,7 @@ export class ChromeService {
           homeHref={http.basePath.prepend('/app/home')}
           isVisible$={this.isVisible$}
           opensearchDashboardsVersion={injectedMetadata.getOpenSearchDashboardsVersion()}
-          navLinks$={navLinks.getFilteredNavLinks$()}
+          navLinks$={navLinks.getNavLinks$()}
           customNavLink$={customNavLink$.pipe(takeUntil(this.stop$))}
           recentlyAccessed$={recentlyAccessed.get$()}
           navControlsLeft$={navControls.getLeft$()}

src/core/public/chrome/nav_links/nav_links_service.test.ts

@@ -32,18 +32,12 @@ import { NavLinksService } from './nav_links_service';
 import { take, map, takeLast } from 'rxjs/operators';
 import { App } from '../../application';
 import { BehaviorSubject } from 'rxjs';
+import { ChromeNavLink } from 'opensearch-dashboards/public';
 
 const availableApps = new Map([
   ['app1', { id: 'app1', order: 0, title: 'App 1', icon: 'app1' }],
-  [
-    'app2',
-    {
-      id: 'app2',
-      order: -10,
-      title: 'App 2',
-      euiIconType: 'canvasApp',
-    },
-  ],
+  ['app2', { id: 'app2', order: -10, title: 'App 2', euiIconType: 'canvasApp' }],
+  ['app3', { id: 'app3', order: 10, title: 'App 3', icon: 'app3' }],
   ['chromelessApp', { id: 'chromelessApp', order: 20, title: 'Chromless App', chromeless: true }],
 ]);
 
@@ -66,7 +60,110 @@ describe('NavLinksService', () => {
     start = service.start({ application: mockAppService, http: mockHttp });
   });
 
-  describe('#getNavLinks$()', () => {
+  describe('#getAllNavLinks$()', () => {
+    it('does not include `chromeless` applications', async () => {
+      expect(
+        await start
+          .getAllNavLinks$()
+          .pipe(
+            take(1),
+            map((links) => links.map((l) => l.id))
+          )
+          .toPromise()
+      ).not.toContain('chromelessApp');
+    });
+
+    it('sorts navLinks by `order` property', async () => {
+      expect(
+        await start
+          .getAllNavLinks$()
+          .pipe(
+            take(1),
+            map((links) => links.map((l) => l.id))
+          )
+          .toPromise()
+      ).toEqual(['app2', 'app1', 'app3']);
+    });
+
+    it('emits multiple values', async () => {
+      const navLinkIds$ = start.getAllNavLinks$().pipe(map((links) => links.map((l) => l.id)));
+      const emittedLinks: string[][] = [];
+      navLinkIds$.subscribe((r) => emittedLinks.push(r));
+      start.update('app1', { href: '/foo' });
+
+      service.stop();
+      expect(emittedLinks).toEqual([
+        ['app2', 'app1', 'app3'],
+        ['app2', 'app1', 'app3'],
+      ]);
+    });
+
+    it('completes when service is stopped', async () => {
+      const last$ = start.getAllNavLinks$().pipe(takeLast(1)).toPromise();
+      service.stop();
+      await expect(last$).resolves.toBeInstanceOf(Array);
+    });
+  });
+
+  describe('#getNavLinks$() when non null', () => {
+    // set nav links, nav link with order smaller than 0 will be filtered
+    beforeEach(() => {
+      const navLinks = new Map<string, ChromeNavLink>();
+      start.getAllNavLinks$().subscribe((links) =>
+        links.forEach((link) => {
+          if (link.order !== undefined && link.order >= 0) {
+            navLinks.set(link.id, link);
+          }
+        })
+      );
+      start.setNavLinks(navLinks);
+    });
+
+    it('does not include `app2` applications', async () => {
+      expect(
+        await start
+          .getNavLinks$()
+          .pipe(
+            take(1),
+            map((links) => links.map((l) => l.id))
+          )
+          .toPromise()
+      ).not.toContain('app2');
+    });
+
+    it('sorts navLinks by `order` property', async () => {
+      expect(
+        await start
+          .getNavLinks$()
+          .pipe(
+            take(1),
+            map((links) => links.map((l) => l.id))
+          )
+          .toPromise()
+      ).toEqual(['app1', 'app3']);
+    });
+
+    it('emits multiple values', async () => {
+      const navLinkIds$ = start.getNavLinks$().pipe(map((links) => links.map((l) => l.id)));
+      const emittedLinks: string[][] = [];
+      navLinkIds$.subscribe((r) => emittedLinks.push(r));
+      start.update('app1', { href: '/foo' });
+
+      service.stop();
+      expect(emittedLinks).toEqual([
+        ['app1', 'app3'],
+        ['app1', 'app3'],
+      ]);
+    });
+
+    it('completes when service is stopped', async () => {
+      const last$ = start.getNavLinks$().pipe(takeLast(1)).toPromise();
+      service.stop();
+      await expect(last$).resolves.toBeInstanceOf(Array);
+    });
+  });
+
+  describe('#getNavLinks$() when null', () => {
     it('does not include `chromeless` applications', async () => {
       expect(
         await start
@@ -79,7 +176,19 @@ describe('NavLinksService', () => {
       ).not.toContain('chromelessApp');
     });
 
-    it('sorts navlinks by `order` property', async () => {
+    it('include `app2` applications', async () => {
+      expect(
+        await start
+          .getNavLinks$()
+          .pipe(
+            take(1),
+            map((links) => links.map((l) => l.id))
+          )
+          .toPromise()
+      ).toContain('app2');
+    });
+
+    it('sorts navLinks by `order` property', async () => {
       expect(
         await start
           .getNavLinks$()
@@ -88,7 +197,7 @@ describe('NavLinksService', () => {
             map((links) => links.map((l) => l.id))
           )
           .toPromise()
-      ).toEqual(['app2', 'app1']);
+      ).toEqual(['app2', 'app1', 'app3']);
     });
 
     it('emits multiple values', async () => {
@@ -99,8 +208,8 @@ describe('NavLinksService', () => {
 
       service.stop();
       expect(emittedLinks).toEqual([
-        ['app2', 'app1'],
-        ['app2', 'app1'],
+        ['app2', 'app1', 'app3'],
+        ['app2', 'app1', 'app3'],
       ]);
     });
 
@@ -123,7 +232,7 @@ describe('NavLinksService', () => {
 
   describe('#getAll()', () => {
     it('returns a sorted array of navlinks', () => {
-      expect(start.getAll().map((l) => l.id)).toEqual(['app2', 'app1']);
+      expect(start.getAll().map((l) => l.id)).toEqual(['app2', 'app1', 'app3']);
     });
   });
 
@@ -148,7 +257,7 @@ describe('NavLinksService', () => {
             map((links) => links.map((l) => l.id))
           )
           .toPromise()
-      ).toEqual(['app2', 'app1']);
+      ).toEqual(['app2', 'app1', 'app3']);
     });
 
     it('does nothing on chromeless applications', async () => {
@@ -161,7 +270,7 @@ describe('NavLinksService', () => {
             map((links) => links.map((l) => l.id))
           )
           .toPromise()
-      ).toEqual(['app2', 'app1']);
+      ).toEqual(['app2', 'app1', 'app3']);
     });
 
     it('removes all other links', async () => {

src/core/public/chrome/nav_links/nav_links_service.ts

@@ -54,14 +54,14 @@ export interface ChromeNavLinks {
   getNavLinks$(): Observable<Array<Readonly<ChromeNavLink>>>;
 
   /**
-   * Set an observable for a sorted list of filtered navlinks.
+   * Get an observable for a sorted list of all navlinks.
    */
-  getFilteredNavLinks$(): Observable<Array<Readonly<ChromeNavLink>>>;
+  getAllNavLinks$(): Observable<Array<Readonly<ChromeNavLink>>>;
 
   /**
-   * Set filtered navlinks.
+   * Set navlinks.
    */
-  setFilteredNavLinks(filteredNavLinks: ReadonlyMap<string, ChromeNavLink>): void;
+  setNavLinks(navLinks: ReadonlyMap<string, ChromeNavLink>): void;
 
   /**
    * Get the state of a navlink at this point in time.
@@ -126,9 +126,6 @@ type LinksUpdater = (navLinks: Map<string, NavLinkWrapper>) => Map<string, NavLi
 
 export class NavLinksService {
   private readonly stop$ = new ReplaySubject(1);
-  private filteredNavLinks$ = new BehaviorSubject<ReadonlyMap<string, ChromeNavLink> | undefined>(
-    undefined
-  );
 
   public start({ application, http }: StartDeps): ChromeNavLinks {
     const appLinks$ = application.applications$.pipe(
@@ -145,7 +142,10 @@ export class NavLinksService {
     // manual link modifications to be able to re-apply then after every
     // availableApps$ changes.
     const linkUpdaters$ = new BehaviorSubject<LinksUpdater[]>([]);
-    const navLinks$ = new BehaviorSubject<ReadonlyMap<string, NavLinkWrapper>>(new Map());
+    const displayedNavLinks$ = new BehaviorSubject<ReadonlyMap<string, ChromeNavLink> | undefined>(
+      undefined
+    );
+    const allNavLinks$ = new BehaviorSubject<ReadonlyMap<string, NavLinkWrapper>>(new Map());
 
     combineLatest([appLinks$, linkUpdaters$])
       .pipe(
@@ -154,42 +154,40 @@ export class NavLinksService {
         })
       )
       .subscribe((navLinks) => {
-        navLinks$.next(navLinks);
+        allNavLinks$.next(navLinks);
       });
 
     const forceAppSwitcherNavigation$ = new BehaviorSubject(false);
 
     return {
       getNavLinks$: () => {
-        return navLinks$.pipe(map(sortNavLinks), takeUntil(this.stop$));
+        return combineLatest([allNavLinks$, displayedNavLinks$]).pipe(
+          map(([allNavLinks, displayedNavLinks]) =>
+            displayedNavLinks === undefined ? sortLinks(allNavLinks) : sortLinks(displayedNavLinks)
+          ),
+          takeUntil(this.stop$)
+        );
       },
 
-      setFilteredNavLinks: (filteredNavLinks: ReadonlyMap<string, ChromeNavLink>) => {
-        this.filteredNavLinks$.next(filteredNavLinks);
+      setNavLinks: (navLinks: ReadonlyMap<string, ChromeNavLink>) => {
+        displayedNavLinks$.next(navLinks);
       },
 
-      getFilteredNavLinks$: () => {
-        return combineLatest([navLinks$, this.filteredNavLinks$]).pipe(
-          map(([navLinks, filteredNavLinks]) =>
-            filteredNavLinks === undefined
-              ? sortNavLinks(navLinks)
-              : sortChromeNavLinks(filteredNavLinks)
-          ),
-          takeUntil(this.stop$)
-        );
+      getAllNavLinks$: () => {
+        return allNavLinks$.pipe(map(sortLinks), takeUntil(this.stop$));
       },
 
       get(id: string) {
-        const link = navLinks$.value.get(id);
+        const link = allNavLinks$.value.get(id);
         return link && link.properties;
       },
 
       getAll() {
-        return sortNavLinks(navLinks$.value);
+        return sortLinks(allNavLinks$.value);
       },
 
       has(id: string) {
-        return navLinks$.value.has(id);
+        return allNavLinks$.value.has(id);
       },
 
       showOnly(id: string) {
@@ -237,16 +235,9 @@ export class NavLinksService {
   }
 }
 
-function sortNavLinks(navLinks: ReadonlyMap<string, NavLinkWrapper>) {
-  return sortBy(
-    [...navLinks.values()].map((link) => link.properties),
-    'order'
-  );
-}
-
-function sortChromeNavLinks(chromeNavLinks: ReadonlyMap<string, ChromeNavLink>) {
+function sortLinks(links: ReadonlyMap<string, NavLinkWrapper | ChromeNavLink>) {
   return sortBy(
-    [...chromeNavLinks.values()].map((link) => link as Readonly<ChromeNavLink>),
+    [...links.values()].map((link) => ('properties' in link ? link.properties : link)),
     'order'
   );
 }