fix: minor UX fixes for installing extensions (microsoft#4308)

* use plugin id for page links * add api to install local extension * disable add button when no extension selected * fix settings page redirect issue * normalize search query for extensions * revert api to install local extension * fix shimmer row when no extensions installed * add test for unsuccessul install * fix type error
lei9444 · Oct 1, 2020 · 6e543ea · 6e543ea
1 parent 97392ed
commit 6e543ea
Show file tree

Hide file tree

Showing 11 changed files with 113 additions and 38 deletions.
diff --git a/Composer/packages/client/src/pages/setting/SettingsPage.tsx b/Composer/packages/client/src/pages/setting/SettingsPage.tsx
@@ -89,8 +89,6 @@ const SettingPage: React.FC<RouteComponentProps> = () => {
   useEffect(() => {
     if (!projectId && location.pathname.indexOf('/settings/bot/') !== -1) {
       navigate('/settings/application');
-    } else {
-      navigate(links[0].url);
     }
   }, [projectId]);
 

diff --git a/Composer/packages/client/src/pages/setting/extensions/ExtensionSearchResults.tsx b/Composer/packages/client/src/pages/setting/extensions/ExtensionSearchResults.tsx
@@ -3,14 +3,15 @@
 
 /** @jsx jsx */
 import { jsx, css } from '@emotion/core';
-import React from 'react';
+import React, { useRef } from 'react';
 import formatMessage from 'format-message';
 import {
   DetailsListLayoutMode,
   SelectionMode,
   IColumn,
   CheckboxVisibility,
   ConstrainMode,
+  Selection,
 } from 'office-ui-fabric-react/lib/DetailsList';
 import { ScrollablePane } from 'office-ui-fabric-react/lib/ScrollablePane';
 import { Sticky } from 'office-ui-fabric-react/lib/Sticky';
@@ -44,6 +45,13 @@ const noResultsStyles = css`
 
 const ExtensionSearchResults: React.FC<ExtensionSearchResultsProps> = (props) => {
   const { results, isSearching, onSelect } = props;
+  const selection = useRef(
+    new Selection({
+      onSelectionChanged: () => {
+        onSelect(selection.getSelection()[0] as ExtensionSearchResult);
+      },
+    })
+  ).current;
 
   const searchColumns: IColumn[] = [
     {
@@ -101,9 +109,9 @@ const ExtensionSearchResults: React.FC<ExtensionSearchResultsProps> = (props) =>
           enableShimmer={isSearching}
           items={noResultsFound ? [{}] : results}
           layoutMode={DetailsListLayoutMode.justified}
+          selection={selection}
           selectionMode={SelectionMode.single}
           shimmerLines={8}
-          onActiveItemChanged={(item) => onSelect(item)}
           onRenderDetailsHeader={(headerProps, defaultRender) => {
             if (defaultRender) {
               return <Sticky>{defaultRender(headerProps)}</Sticky>;

diff --git a/Composer/packages/client/src/pages/setting/extensions/Extensions.tsx b/Composer/packages/client/src/pages/setting/extensions/Extensions.tsx
@@ -158,15 +158,15 @@ const Extensions: React.FC<RouteComponentProps> = () => {
   }, []);
 
   const shownItems = () => {
-    if (extensions.length === 0) {
-      // render no installed message
-      return [{}];
-    } else if (isUpdating === true) {
+    if (isUpdating === true) {
       // extension is being added, render a shimmer row at end of list
       return [...extensions, null];
     } else if (typeof isUpdating === 'string') {
       // extension is being removed or updated, show shimmer for that row
       return extensions.map((e) => (e.id === isUpdating ? null : e));
+    } else if (extensions.length === 0) {
+      // render no installed message
+      return [{}];
     } else {
       return extensions;
     }
@@ -186,20 +186,25 @@ const Extensions: React.FC<RouteComponentProps> = () => {
         selection={selection}
         selectionMode={SelectionMode.multiple}
         onRenderRow={(rowProps, defaultRender) => {
-          if (extensions.length === 0) {
-            return (
-              <div css={noExtensionsStyles}>
-                <p>{formatMessage('No extensions installed')}</p>
-              </div>
-            );
-          }
+          if (rowProps && defaultRender) {
+            if (isUpdating) {
+              return defaultRender(rowProps);
+            }
+
+            if (extensions.length === 0) {
+              return (
+                <div css={noExtensionsStyles}>
+                  <p>{formatMessage('No extensions installed')}</p>
+                </div>
+              );
+            }
 
-          if (defaultRender && rowProps) {
             const customStyles: Partial<IDetailsRowStyles> = {
               root: {
-                color: rowProps?.item?.enabled ? undefined : NeutralColors.gray90,
+                color: rowProps.item?.enabled ? undefined : NeutralColors.gray90,
               },
             };
+
             return <DetailsRow {...rowProps} styles={customStyles} />;
           }
 

diff --git a/Composer/packages/client/src/pages/setting/extensions/InstallExtensionDialog.tsx b/Composer/packages/client/src/pages/setting/extensions/InstallExtensionDialog.tsx
@@ -96,7 +96,7 @@ const InstallExtensionDialog: React.FC<InstallExtensionDialogProps> = (props) =>
       </div>
       <DialogFooter>
         <DefaultButton onClick={onDismiss}>Cancel</DefaultButton>
-        <PrimaryButton disabled={false} onClick={onSubmit}>
+        <PrimaryButton disabled={!selectedExtension} onClick={onSubmit}>
           {formatMessage('Add')}
         </PrimaryButton>
       </DialogFooter>

diff --git a/Composer/packages/client/src/recoilModel/types.ts b/Composer/packages/client/src/recoilModel/types.ts
@@ -46,6 +46,8 @@ type ExtensionPublishContribution = {
 };
 
 export type ExtensionPageContribution = {
+  /** plugin id */
+  id: string;
   bundleId: string;
   label: string;
   icon?: string;

diff --git a/Composer/packages/client/src/utils/hooks.ts b/Composer/packages/client/src/utils/hooks.ts
@@ -33,7 +33,7 @@ export const useLinks = () => {
   const pluginPages = extensions.reduce((pages, p) => {
     const pagesConfig = p.contributes?.views?.pages;
     if (Array.isArray(pagesConfig) && pagesConfig.length > 0) {
-      pages.push(...pagesConfig);
+      pages.push(...pagesConfig.map((page) => ({ ...page, id: p.id })));
     }
     return pages;
   }, [] as ExtensionPageContribution[]);

diff --git a/Composer/packages/client/src/utils/pageLinks.ts b/Composer/packages/client/src/utils/pageLinks.ts
@@ -72,7 +72,7 @@ export const topLinks = (projectId: string, openedDialogId: string, pluginPages:
   if (pluginPages.length > 0) {
     pluginPages.forEach((p) => {
       links.push({
-        to: `page/${p.bundleId}`,
+        to: `page/${p.id}`,
         iconName: p.icon ?? 'StatusCircleQuestionMark',
         labelName: p.label,
         exact: true,

diff --git a/Composer/packages/extension/src/manager/manager.ts b/Composer/packages/extension/src/manager/manager.ts
@@ -4,7 +4,7 @@
 import path from 'path';
 
 import glob from 'globby';
-import { readJson, ensureDir } from 'fs-extra';
+import { readJson, ensureDir, existsSync } from 'fs-extra';
 
 import { ExtensionContext } from '../extensionContext';
 import logger from '../logger';
@@ -77,6 +77,7 @@ class ExtensionManager {
    * Installs a remote extension via NPM
    * @param name The name of the extension to install
    * @param version The version of the extension to install
+   * @returns id of installed package
    */
   public async installRemote(name: string, version?: string) {
     const packageNameAndVersion = version ? `${name}@${version}` : `${name}@latest`;
@@ -97,6 +98,8 @@ class ExtensionManager {
       if (packageJson) {
         const extensionPath = path.resolve(this.remoteDir, 'node_modules', name);
         this.manifest.updateExtensionConfig(name, getExtensionMetadata(extensionPath, packageJson));
+
+        return packageJson.name;
       } else {
         throw new Error(`Unable to install ${packageNameAndVersion}`);
       }
@@ -108,6 +111,37 @@ class ExtensionManager {
     }
   }
 
+  /**
+   * Installs a local extension at path
+   * @param path Path of directory where extension is
+   */
+  public async installLocal(extPath: string) {
+    try {
+      const packageJsonPath = path.join(extPath, 'package.json');
+
+      if (!existsSync(packageJsonPath)) {
+        throw new Error(`Extension not found at path: ${extPath}`);
+      }
+
+      const packageJson = await readJson(packageJsonPath);
+
+      log('Linking %s', packageJson.name);
+      await npm('link', '.', {}, { cwd: extPath });
+
+      log('Installing %s@local to %s', packageJson.name, this.remoteDir);
+      await npm('link', packageJson.name, { '--prefix': this.remoteDir }, { cwd: this.remoteDir });
+
+      const extensionPath = path.resolve(this.remoteDir, 'node_modules', packageJson.name);
+      this.manifest.updateExtensionConfig(packageJson.name, getExtensionMetadata(extensionPath, packageJson));
+
+      return packageJson.name;
+    } catch (err) {
+      log('%s', err.msg ?? err.stderr ?? err);
+      // eslint-disable-next-line no-console
+      console.error(err);
+    }
+  }
+
   public async load(id: string) {
     const metadata = this.manifest.getExtensionConfig(id);
     try {
@@ -174,11 +208,14 @@ class ExtensionManager {
    */
   public async search(query: string) {
     await this.updateSearchCache();
+    const normalizedQuery = query.toLowerCase();
 
     const results = Array.from(this.searchCache.values()).filter((result) => {
       return (
         !this.find(result.id) &&
-        [result.id, result.description, ...result.keywords].some((target) => target.includes(query))
+        [result.id, result.description, ...result.keywords].some((target) =>
+          target.toLowerCase().includes(normalizedQuery)
+        )
       );
     });
 

diff --git a/Composer/packages/extension/src/utils/npm.ts b/Composer/packages/extension/src/utils/npm.ts
@@ -12,7 +12,7 @@ type NpmOutput = {
   stderr: string;
   code: number;
 };
-type NpmCommand = 'install' | 'uninstall' | 'search';
+type NpmCommand = 'install' | 'uninstall' | 'search' | 'link';
 type NpmOptions = {
   [key: string]: string;
 };

diff --git a/Composer/packages/server/src/controllers/__tests__/extensions.test.ts b/Composer/packages/server/src/controllers/__tests__/extensions.test.ts
@@ -166,21 +166,40 @@ describe('adding an extension', () => {
     expect(ExtensionManager.installRemote).toHaveBeenCalledWith(id, 'some-version');
   });
 
-  it('loads the extension', async () => {
-    await ExtensionsController.addExtension({ body: { id } } as Request, res);
+  describe('installed successfully', () => {
+    beforeEach(() => {
+      (ExtensionManager.installRemote as jest.Mock).mockResolvedValue(id);
+    });
+
+    it('loads the extension', async () => {
+      await ExtensionsController.addExtension({ body: { id } } as Request, res);
+
+      expect(ExtensionManager.load).toHaveBeenCalledWith(id);
+    });
 
-    expect(ExtensionManager.load).toHaveBeenCalledWith(id);
+    it('returns the extension', async () => {
+      (ExtensionManager.find as jest.Mock).mockReturnValue(mockExtension1);
+      await ExtensionsController.addExtension({ body: { id } } as Request, res);
+
+      expect(ExtensionManager.find).toHaveBeenCalledWith(id);
+      expect(res.json).toHaveBeenCalledWith({
+        ...mockExtension1,
+        bundles: undefined,
+        path: undefined,
+      });
+    });
   });
 
-  it('returns the extension', async () => {
-    (ExtensionManager.find as jest.Mock).mockReturnValue(mockExtension1);
-    await ExtensionsController.addExtension({ body: { id } } as Request, res);
+  describe('install fails', () => {
+    beforeEach(() => {
+      (ExtensionManager.installRemote as jest.Mock).mockResolvedValue(undefined);
+    });
 
-    expect(ExtensionManager.find).toHaveBeenCalledWith(id);
-    expect(res.json).toHaveBeenCalledWith({
-      ...mockExtension1,
-      bundles: undefined,
-      path: undefined,
+    it('returns an error', async () => {
+      await ExtensionsController.addExtension({ body: { id } } as Request, res);
+
+      expect(res.status).toHaveBeenCalledWith(500);
+      expect(res.json).toHaveBeenCalledWith({ error: expect.any(String) });
     });
   });
 });

diff --git a/Composer/packages/server/src/controllers/extensions.ts b/Composer/packages/server/src/controllers/extensions.ts
@@ -9,6 +9,7 @@ interface AddExtensionRequest extends Request {
   body: {
     id?: string;
     version?: string;
+    path?: string;
   };
 }
 
@@ -60,10 +61,15 @@ export async function addExtension(req: AddExtensionRequest, res: Response) {
     return;
   }
 
-  await ExtensionManager.installRemote(id, version);
-  await ExtensionManager.load(id);
-  const extension = ExtensionManager.find(id);
-  res.json(presentExtension(extension));
+  const extensionId = await ExtensionManager.installRemote(id, version);
+
+  if (extensionId) {
+    await ExtensionManager.load(extensionId);
+    const extension = ExtensionManager.find(extensionId);
+    res.json(presentExtension(extension));
+  } else {
+    res.status(500).json({ error: 'Unable to install extension.' });
+  }
 }
 
 export async function toggleExtension(req: ToggleExtensionRequest, res: Response) {