From 10ba9ef853e5f739004d7c8478a02ac13544e904 Mon Sep 17 00:00:00 2001 From: Janne Savolainen Date: Fri, 17 Jun 2022 15:07:26 +0300 Subject: [PATCH] Add behaviour for tray menu items originating from extensions (#5609) --- ...and-tab-navigation-for-extensions.test.tsx | 2 +- ...-characters-in-page-registrations.test.tsx | 2 +- .../navigate-to-extension-page.test.tsx | 2 +- ...to-extension-specific-preferences.test.tsx | 4 +- ...vigation-to-telemetry-preferences.test.tsx | 4 +- ...nu-item-originating-from-extension.test.ts | 134 ++++++++++++++++++ .../tray-menu-item-registrator.injectable.ts | 25 ++-- src/main/tray/tray-menu-items.injectable.ts | 19 --- src/main/tray/tray-menu-items.test.ts | 120 ---------------- .../test-utils/get-application-builder.tsx | 132 +++++++++++------ 10 files changed, 248 insertions(+), 196 deletions(-) create mode 100644 src/behaviours/tray/clicking-tray-menu-item-originating-from-extension.test.ts delete mode 100644 src/main/tray/tray-menu-items.injectable.ts delete mode 100644 src/main/tray/tray-menu-items.test.ts diff --git a/src/behaviours/cluster/sidebar-and-tab-navigation-for-extensions.test.tsx b/src/behaviours/cluster/sidebar-and-tab-navigation-for-extensions.test.tsx index eb7fd0bc05..8ef472f20d 100644 --- a/src/behaviours/cluster/sidebar-and-tab-navigation-for-extensions.test.tsx +++ b/src/behaviours/cluster/sidebar-and-tab-navigation-for-extensions.test.tsx @@ -46,7 +46,7 @@ describe("cluster - sidebar and tab navigation for extensions", () => { const getRendererExtensionFake = getRendererExtensionFakeFor(applicationBuilder); const testExtension = getRendererExtensionFake(extensionStubWithSidebarItems); - await applicationBuilder.addExtensions(testExtension); + await applicationBuilder.extensions.renderer.enable(testExtension); }); describe("given no state for expanded sidebar items exists, and navigated to child sidebar item, when rendered", () => { diff --git a/src/behaviours/extension-special-characters-in-page-registrations.test.tsx b/src/behaviours/extension-special-characters-in-page-registrations.test.tsx index 2320d28a1b..20c29d82b5 100644 --- a/src/behaviours/extension-special-characters-in-page-registrations.test.tsx +++ b/src/behaviours/extension-special-characters-in-page-registrations.test.tsx @@ -23,7 +23,7 @@ describe("extension special characters in page registrations", () => { extensionWithPagesHavingSpecialCharacters, ); - await applicationBuilder.addExtensions(testExtension); + await applicationBuilder.extensions.renderer.enable(testExtension); rendered = await applicationBuilder.render(); }); diff --git a/src/behaviours/navigate-to-extension-page.test.tsx b/src/behaviours/navigate-to-extension-page.test.tsx index 35c47b9076..255a8951ba 100644 --- a/src/behaviours/navigate-to-extension-page.test.tsx +++ b/src/behaviours/navigate-to-extension-page.test.tsx @@ -27,7 +27,7 @@ describe("navigate to extension page", () => { extensionWithPagesHavingParameters, ); - await applicationBuilder.addExtensions(testExtension); + await applicationBuilder.extensions.renderer.enable(testExtension); rendered = await applicationBuilder.render(); diff --git a/src/behaviours/preferences/navigation-to-extension-specific-preferences.test.tsx b/src/behaviours/preferences/navigation-to-extension-specific-preferences.test.tsx index f44fb9bf20..de97d67bcf 100644 --- a/src/behaviours/preferences/navigation-to-extension-specific-preferences.test.tsx +++ b/src/behaviours/preferences/navigation-to-extension-specific-preferences.test.tsx @@ -44,11 +44,11 @@ describe("preferences - navigation to extension specific preferences", () => { }); describe("when extension with specific preferences is enabled", () => { - beforeEach(() => { + beforeEach(async () => { const getRendererExtensionFake = getRendererExtensionFakeFor(applicationBuilder); const testExtension = getRendererExtensionFake(extensionStubWithExtensionSpecificPreferenceItems); - applicationBuilder.addExtensions(testExtension); + await applicationBuilder.extensions.renderer.enable(testExtension); }); it("renders", () => { diff --git a/src/behaviours/preferences/navigation-to-telemetry-preferences.test.tsx b/src/behaviours/preferences/navigation-to-telemetry-preferences.test.tsx index f16c2d809b..42ea181ddc 100644 --- a/src/behaviours/preferences/navigation-to-telemetry-preferences.test.tsx +++ b/src/behaviours/preferences/navigation-to-telemetry-preferences.test.tsx @@ -50,7 +50,7 @@ describe("preferences - navigation to telemetry preferences", () => { const getRendererExtensionFake = getRendererExtensionFakeFor(applicationBuilder); const testExtensionWithTelemetryPreferenceItems = getRendererExtensionFake(extensionStubWithTelemetryPreferenceItems); - applicationBuilder.addExtensions( + applicationBuilder.extensions.renderer.enable( testExtensionWithTelemetryPreferenceItems, ); }); @@ -105,7 +105,7 @@ describe("preferences - navigation to telemetry preferences", () => { ], }); - applicationBuilder.addExtensions( + applicationBuilder.extensions.renderer.enable( testExtensionWithTelemetryPreferenceItems, ); diff --git a/src/behaviours/tray/clicking-tray-menu-item-originating-from-extension.test.ts b/src/behaviours/tray/clicking-tray-menu-item-originating-from-extension.test.ts new file mode 100644 index 0000000000..249673ef83 --- /dev/null +++ b/src/behaviours/tray/clicking-tray-menu-item-originating-from-extension.test.ts @@ -0,0 +1,134 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ +import { LensMainExtension } from "../../extensions/lens-main-extension"; +import type { TrayMenuRegistration } from "../../main/tray/tray-menu-registration"; +import type { ApplicationBuilder } from "../../renderer/components/test-utils/get-application-builder"; +import { getApplicationBuilder } from "../../renderer/components/test-utils/get-application-builder"; +import loggerInjectable from "../../common/logger.injectable"; +import type { Logger } from "../../common/logger"; + +describe("clicking tray menu item originating from extension", () => { + let applicationBuilder: ApplicationBuilder; + let logErrorMock: jest.Mock; + + beforeEach(async () => { + applicationBuilder = getApplicationBuilder(); + + applicationBuilder.beforeApplicationStart(({ mainDi }) => { + logErrorMock = jest.fn(); + + mainDi.override(loggerInjectable, () => ({ error: logErrorMock }) as unknown as Logger); + }); + + await applicationBuilder.render(); + }); + + describe("when extension is enabled", () => { + let someExtension: SomeTestExtension; + let clickMock: jest.Mock; + + beforeEach(async () => { + clickMock = jest.fn(); + + someExtension = new SomeTestExtension({ + id: "some-extension-id", + trayMenus: [{ label: "some-label", click: clickMock }], + }); + + await applicationBuilder.extensions.main.enable(someExtension); + }); + + it("when item is clicked, triggers the click handler", () => { + applicationBuilder.tray.click( + "some-label-tray-menu-item-for-extension-some-extension-id-instance-1", + ); + + expect(clickMock).toHaveBeenCalled(); + }); + + describe("given click handler throws synchronously, when item is clicked", () => { + beforeEach(() => { + clickMock.mockImplementation(() => { + throw new Error("some-error"); + }); + + applicationBuilder.tray.click( + "some-label-tray-menu-item-for-extension-some-extension-id-instance-1", + ); + }); + + it("logs the error", () => { + expect(logErrorMock).toHaveBeenCalledWith( + '[TRAY]: Clicking of tray item "some-label" from extension "some-extension-id" failed.', + expect.any(Error), + ); + }); + }); + + describe("given click handler rejects asynchronously, when item is clicked", () => { + beforeEach(() => { + clickMock.mockImplementation(() => Promise.reject("some-rejection")); + + applicationBuilder.tray.click( + "some-label-tray-menu-item-for-extension-some-extension-id-instance-1", + ); + }); + + it("logs the error", () => { + expect(logErrorMock).toHaveBeenCalledWith( + '[TRAY]: Clicking of tray item "some-label" from extension "some-extension-id" failed.', + "some-rejection", + ); + }); + }); + + describe("when extension is disabled", () => { + beforeEach(() => { + applicationBuilder.extensions.main.disable(someExtension); + }); + + it("does not have the tray menu item from extension", () => { + applicationBuilder.extensions.main.disable(someExtension); + + expect( + applicationBuilder.tray.get( + "some-label-tray-menu-item-for-extension-some-extension-id-instance-1", + ), + ).toBeNull(); + }); + + // Note: Motivation here is to make sure that enabling same extension does not throw + it("when extension is re-enabled, has the tray menu item from extension", async () => { + await applicationBuilder.extensions.main.enable(someExtension); + + expect( + applicationBuilder.tray.get( + "some-label-tray-menu-item-for-extension-some-extension-id-instance-2", + ), + ).not.toBeNull(); + }); + }); + + }); +}); + +class SomeTestExtension extends LensMainExtension { + constructor({ id, trayMenus }: { + id: string; + trayMenus: TrayMenuRegistration[]; + }) { + super({ + id, + absolutePath: "irrelevant", + isBundled: false, + isCompatible: false, + isEnabled: false, + manifest: { name: id, version: "some-version", engines: { lens: "^5.5.0" }}, + manifestPath: "irrelevant", + }); + + this.trayMenus = trayMenus; + } +} diff --git a/src/main/tray/tray-menu-item/tray-menu-item-registrator.injectable.ts b/src/main/tray/tray-menu-item/tray-menu-item-registrator.injectable.ts index 6cbd9e5d33..86d57c541c 100644 --- a/src/main/tray/tray-menu-item/tray-menu-item-registrator.injectable.ts +++ b/src/main/tray/tray-menu-item/tray-menu-item-registrator.injectable.ts @@ -55,20 +55,27 @@ const toItemInjectablesFor = (extension: LensMainExtension, installationCounter: label: computed(() => registration.label || ""), tooltip: registration.toolTip, - click: pipeline( - () => { - registration.click?.(registration); - }, + click: () => { + const decorated = pipeline( + registration.click || (() => {}), - withErrorLoggingFor(() => `[TRAY]: Clicking of tray item "${trayItemId}" from extension "${extension.sanitizedExtensionId}" failed.`), + withErrorLoggingFor( + () => + `[TRAY]: Clicking of tray item "${trayItemId}" from extension "${extension.sanitizedExtensionId}" failed.`, + ), - // TODO: Find out how to improve typing so that instead of - // x => withErrorSuppression(x) there could only be withErrorSuppression - (x) => withErrorSuppression(x), - ), + // TODO: Find out how to improve typing so that instead of + // x => withErrorSuppression(x) there could only be withErrorSuppression + x => withErrorSuppression(x), + ); + + return decorated(registration); + }, enabled: computed(() => !!registration.enabled), visible: computed(() => true), + + extension, }), injectionToken: trayMenuItemInjectionToken, diff --git a/src/main/tray/tray-menu-items.injectable.ts b/src/main/tray/tray-menu-items.injectable.ts deleted file mode 100644 index c008c123e0..0000000000 --- a/src/main/tray/tray-menu-items.injectable.ts +++ /dev/null @@ -1,19 +0,0 @@ -/** - * Copyright (c) OpenLens Authors. All rights reserved. - * Licensed under MIT License. See LICENSE in root directory for more information. - */ -import { getInjectable } from "@ogre-tools/injectable"; -import { computed } from "mobx"; -import mainExtensionsInjectable from "../../extensions/main-extensions.injectable"; - -const trayItemsInjectable = getInjectable({ - id: "tray-items", - - instantiate: (di) => { - const extensions = di.inject(mainExtensionsInjectable); - - return computed(() => extensions.get().flatMap(extension => extension.trayMenus)); - }, -}); - -export default trayItemsInjectable; diff --git a/src/main/tray/tray-menu-items.test.ts b/src/main/tray/tray-menu-items.test.ts deleted file mode 100644 index fb4f29f763..0000000000 --- a/src/main/tray/tray-menu-items.test.ts +++ /dev/null @@ -1,120 +0,0 @@ -/** - * Copyright (c) OpenLens Authors. All rights reserved. - * Licensed under MIT License. See LICENSE in root directory for more information. - */ -import type { DiContainer } from "@ogre-tools/injectable"; -import { LensMainExtension } from "../../extensions/lens-main-extension"; -import trayItemsInjectable from "./tray-menu-items.injectable"; -import type { IComputedValue } from "mobx"; -import { computed, ObservableMap, runInAction } from "mobx"; -import { getDiForUnitTesting } from "../getDiForUnitTesting"; -import mainExtensionsInjectable from "../../extensions/main-extensions.injectable"; -import type { TrayMenuRegistration } from "./tray-menu-registration"; - -describe("tray-menu-items", () => { - let di: DiContainer; - let trayMenuItems: IComputedValue; - let extensionsStub: ObservableMap; - - beforeEach(async () => { - di = getDiForUnitTesting({ doGeneralOverrides: true }); - - extensionsStub = new ObservableMap(); - - di.override( - mainExtensionsInjectable, - () => computed(() => [...extensionsStub.values()]), - ); - - trayMenuItems = di.inject(trayItemsInjectable); - }); - - it("does not have any items yet", () => { - expect(trayMenuItems.get()).toHaveLength(0); - }); - - describe("when extension is enabled", () => { - beforeEach(() => { - const someExtension = new SomeTestExtension({ - id: "some-extension-id", - trayMenus: [{ label: "tray-menu-from-some-extension" }], - }); - - runInAction(() => { - extensionsStub.set("some-extension-id", someExtension); - }); - }); - - it("has tray menu items", () => { - expect(trayMenuItems.get()).toEqual([ - { - label: "tray-menu-from-some-extension", - }, - ]); - }); - - it("when disabling extension, does not have tray menu items", () => { - runInAction(() => { - extensionsStub.delete("some-extension-id"); - }); - - expect(trayMenuItems.get()).toHaveLength(0); - }); - - describe("when other extension is enabled", () => { - beforeEach(() => { - const someOtherExtension = new SomeTestExtension({ - id: "some-extension-id", - trayMenus: [{ label: "some-label-from-second-extension" }], - }); - - runInAction(() => { - extensionsStub.set("some-other-extension-id", someOtherExtension); - }); - }); - - it("has tray menu items for both extensions", () => { - expect(trayMenuItems.get()).toEqual([ - { - label: "tray-menu-from-some-extension", - }, - - { - label: "some-label-from-second-extension", - }, - ]); - }); - - it("when extension is disabled, still returns tray menu items for extensions that are enabled", () => { - runInAction(() => { - extensionsStub.delete("some-other-extension-id"); - }); - - expect(trayMenuItems.get()).toEqual([ - { - label: "tray-menu-from-some-extension", - }, - ]); - }); - }); - }); -}); - -class SomeTestExtension extends LensMainExtension { - constructor({ id, trayMenus }: { - id: string; - trayMenus: TrayMenuRegistration[]; - }) { - super({ - id, - absolutePath: "irrelevant", - isBundled: false, - isCompatible: false, - isEnabled: false, - manifest: { name: id, version: "some-version", engines: { lens: "^5.5.0" }}, - manifestPath: "irrelevant", - }); - - this.trayMenus = trayMenus; - } -} diff --git a/src/renderer/components/test-utils/get-application-builder.tsx b/src/renderer/components/test-utils/get-application-builder.tsx index 9154299b38..7b526123d9 100644 --- a/src/renderer/components/test-utils/get-application-builder.tsx +++ b/src/renderer/components/test-utils/get-application-builder.tsx @@ -6,7 +6,7 @@ import type { LensRendererExtension } from "../../../extensions/lens-renderer-ex import rendererExtensionsInjectable from "../../../extensions/renderer-extensions.injectable"; import currentlyInClusterFrameInjectable from "../../routes/currently-in-cluster-frame.injectable"; import { extensionRegistratorInjectionToken } from "../../../extensions/extension-loader/extension-registrator-injection-token"; -import type { IObservableArray } from "mobx"; +import type { IObservableArray, ObservableSet } from "mobx"; import { computed, observable, runInAction } from "mobx"; import { renderFor } from "./renderFor"; import React from "react"; @@ -24,7 +24,7 @@ import type { ClusterStore } from "../../../common/cluster-store/cluster-store"; import mainExtensionsInjectable from "../../../extensions/main-extensions.injectable"; import currentRouteComponentInjectable from "../../routes/current-route-component.injectable"; import { pipeline } from "@ogre-tools/fp"; -import { flatMap, compact, join, get, filter, map, matches, find } from "lodash/fp"; +import { flatMap, compact, join, get, filter, map, matches } from "lodash/fp"; import preferenceNavigationItemsInjectable from "../+preferences/preferences-navigation/preference-navigation-items.injectable"; import navigateToPreferencesInjectable from "../../../common/front-end-routing/routes/preferences/navigate-to-preferences.injectable"; import type { MenuItemOpts } from "../../../main/menu/application-menu-items.injectable"; @@ -56,13 +56,31 @@ import { openMenu } from "react-select-event"; import userEvent from "@testing-library/user-event"; import { StatusBar } from "../status-bar/status-bar"; import lensProxyPortInjectable from "../../../main/lens-proxy/lens-proxy-port.injectable"; +import type { LensMainExtension } from "../../../extensions/lens-main-extension"; +import trayMenuItemsInjectable from "../../../main/tray/tray-menu-item/tray-menu-items.injectable"; +import type { LensExtension } from "../../../extensions/lens-extension"; type Callback = (dis: DiContainers) => void | Promise; +type EnableExtensions = (...extensions: T[]) => Promise; +type DisableExtensions = (...extensions: T[]) => void; + export interface ApplicationBuilder { dis: DiContainers; setEnvironmentToClusterFrame: () => ApplicationBuilder; - addExtensions: (...extensions: LensRendererExtension[]) => Promise; + + extensions: { + renderer: { + enable: EnableExtensions; + disable: DisableExtensions; + }; + + main: { + enable: EnableExtensions; + disable: DisableExtensions; + }; + }; + allowKubeResource: (resourceName: KubeResource) => ApplicationBuilder; beforeApplicationStart: (callback: Callback) => ApplicationBuilder; beforeRender: (callback: Callback) => ApplicationBuilder; @@ -135,7 +153,8 @@ export const getApplicationBuilder = () => { const beforeApplicationStartCallbacks: Callback[] = []; const beforeRenderCallbacks: Callback[] = []; - const extensionsState = observable.array(); + const rendererExtensionsState = observable.set(); + const mainExtensionsState = observable.set(); rendererDi.override(subscribeStoresInjectable, () => () => () => {}); @@ -174,14 +193,13 @@ export const getApplicationBuilder = () => { ); rendererDi.override(rendererExtensionsInjectable, () => - computed(() => extensionsState), + computed(() => [...rendererExtensionsState]), ); mainDi.override(mainExtensionsInjectable, () => - computed(() => []), + computed(() => [...mainExtensionsState]), ); - let trayMenuItemsStateFake: TrayMenuItem[]; let trayMenuIconPath: string; mainDi.override(electronTrayInjectable, () => ({ @@ -191,9 +209,7 @@ export const getApplicationBuilder = () => { trayMenuIconPath = iconPaths.normal; }, stop: () => {}, - setMenuItems: (items) => { - trayMenuItemsStateFake = items; - }, + setMenuItems: () => {}, setIconPath: (path) => { trayMenuIconPath = path; }, @@ -202,6 +218,43 @@ export const getApplicationBuilder = () => { let allowedResourcesState: IObservableArray; let rendered: RenderResult; + const enableExtensionsFor = ( + extensionState: T, + di: DiContainer, + ) => { + let index = 0; + + return async (...extensions: LensExtension[]) => { + const extensionRegistrators = di.injectMany( + extensionRegistratorInjectionToken, + ); + + const addAndEnableExtensions = async () => { + index++; + + const registratorPromises = extensions.flatMap((extension) => + extensionRegistrators.map((registrator) => + registrator(extension, index), + ), + ); + + await Promise.all(registratorPromises); + + runInAction(() => { + extensions.forEach((extension) => { + extensionState.add(extension); + }); + }); + }; + + if (rendered) { + await addAndEnableExtensions(); + } else { + builder.beforeRender(addAndEnableExtensions); + } + }; + }; + const builder: ApplicationBuilder = { dis, @@ -242,18 +295,20 @@ export const getApplicationBuilder = () => { tray: { get: (id: string) => { - return trayMenuItemsStateFake.find(matches({ id })) ?? null; + const trayMenuItems = mainDi.inject(trayMenuItemsInjectable).get(); + + return trayMenuItems.find(matches({ id })) ?? null; }, getIconPath: () => trayMenuIconPath, + click: async (id: string) => { - const menuItem = pipeline( - trayMenuItemsStateFake, - find((menuItem) => menuItem.id === id), - ); + const trayMenuItems = mainDi.inject(trayMenuItemsInjectable).get(); + + const menuItem = trayMenuItems.find(matches({ id })) ?? null; if (!menuItem) { const availableIds = pipeline( - trayMenuItemsStateFake, + trayMenuItems, filter(item => !!item.click), map(item => item.id), join(", "), @@ -345,32 +400,16 @@ export const getApplicationBuilder = () => { return builder; }, - addExtensions: async (...extensions) => { - const extensionRegistrators = rendererDi.injectMany( - extensionRegistratorInjectionToken, - ); + extensions: { + renderer: { + enable: enableExtensionsFor(rendererExtensionsState, rendererDi), + disable: disableExtensionsFor(rendererExtensionsState), + }, - const addAndEnableExtensions = async () => { - const registratorPromises = extensions.flatMap((extension) => - extensionRegistrators.map((registrator) => registrator(extension, 1)), - ); - - await Promise.all(registratorPromises); - - runInAction(() => { - extensions.forEach((extension) => { - extensionsState.push(extension); - }); - }); - }; - - if (rendered) { - await addAndEnableExtensions(); - } else { - builder.beforeRender(addAndEnableExtensions); - } - - return builder; + main: { + enable: enableExtensionsFor(mainExtensionsState, mainDi), + disable: disableExtensionsFor(mainExtensionsState), + }, }, allowKubeResource: (resourceName) => { @@ -494,3 +533,14 @@ function toFlatChildren(parentId: string | null | undefined): ToFlatChildren { ), ]; } + +const disableExtensionsFor = + (extensionState: T) => + + (...extensions: LensExtension[]) => { + extensions.forEach((extension) => { + runInAction(() => { + extensionState.delete(extension); + }); + }); + };