Fix creating files in custom drives, fix ContentsManagerMock (jupyterlab#15291)

* Fix create files in custom drives

* fix creating new notebooks

* update method name

* fix handling of localpath

* fix handling of drives in ContentsManagerMock

* fix handling of drives in ContentsManagerMock

* Add test for creating new files in drives

* fix typo

* Fix delete

* Fix restoreCheckpoints

Author: jtpio

packages/filebrowser/src/browser.ts

@@ -2,6 +2,7 @@
 // Distributed under the terms of the Modified BSD License.
 
 import { showErrorMessage } from '@jupyterlab/apputils';
+import { PathExt } from '@jupyterlab/coreutils';
 import { IDocumentManager } from '@jupyterlab/docmanager';
 import { Contents, ServerConnection } from '@jupyterlab/services';
 import { ITranslator, nullTranslator } from '@jupyterlab/translation';
@@ -252,6 +253,11 @@ export class FileBrowser extends SidePanel {
   private async _createNew(
     options: Contents.ICreateOptions
   ): Promise<Contents.IModel> {
+    // normalize the path if the file is created from a custom drive
+    if (options.path) {
+      const localPath = this._manager.services.contents.localPath(options.path);
+      options.path = this._toDrivePath(this.model.driveName, localPath);
+    }
     try {
       const model = await this._manager.newUntitled(options);
       await this.listing.selectItemByName(model.name, true);
@@ -403,6 +409,23 @@ export class FileBrowser extends SidePanel {
     }
   }
 
+  /**
+   * Given a drive name and a local path, return the full
+   * drive path which includes the drive name and the local path.
+   *
+   * @param driveName: the name of the drive
+   * @param localPath: the local path on the drive.
+   *
+   * @returns the full drive path
+   */
+  private _toDrivePath(driveName: string, localPath: string): string {
+    if (driveName === '') {
+      return localPath;
+    } else {
+      return `${driveName}:${PathExt.removeSlash(localPath)}`;
+    }
+  }
+
   protected listing: DirListing;
   protected crumbs: BreadCrumbs;
   protected mainPanel: Panel;

packages/filebrowser/test/browser.spec.ts

@@ -10,6 +10,7 @@ import { DocumentManager, IDocumentManager } from '@jupyterlab/docmanager';
 import { DocumentRegistry, TextModelFactory } from '@jupyterlab/docregistry';
 import { ServiceManager } from '@jupyterlab/services';
 import { signalToPromise } from '@jupyterlab/testing';
+import { Drive } from '@jupyterlab/services';
 import { ServiceManagerMock } from '@jupyterlab/services/lib/testutils';
 import { DocumentWidgetOpenerMock } from '@jupyterlab/docregistry/lib/testutils';
 import { simulate } from 'simulate-event';
@@ -130,3 +131,67 @@ describe('filebrowser/browser', () => {
     });
   });
 });
+
+describe('FileBrowser with Drives', () => {
+  const DRIVE_NAME = 'TestDrive';
+  let fileBrowser: TestFileBrowser;
+  let manager: IDocumentManager;
+  let serviceManager: ServiceManager.IManager;
+  let registry: DocumentRegistry;
+  let model: FilterFileBrowserModel;
+
+  beforeAll(async () => {
+    const opener = new DocumentWidgetOpenerMock();
+
+    registry = new DocumentRegistry({
+      textModelFactory: new TextModelFactory()
+    });
+    serviceManager = new ServiceManagerMock();
+    manager = new DocumentManager({
+      registry,
+      opener,
+      manager: serviceManager
+    });
+
+    const drive = new Drive({
+      name: DRIVE_NAME,
+      serverSettings: serviceManager.serverSettings
+    });
+    serviceManager.contents.addDrive(drive);
+    model = new FilterFileBrowserModel({ manager, driveName: drive.name });
+  });
+
+  beforeEach(() => {
+    const options: FileBrowser.IOptions = {
+      model,
+      id: ''
+    };
+    fileBrowser = new TestFileBrowser(options);
+    Widget.attach(fileBrowser, document.body);
+  });
+
+  describe('#createNewFile', () => {
+    it('should create the file in the drive', async () => {
+      const created = fileBrowser.createNewFile({ ext: '.txt' });
+      await signalToPromise(fileBrowser.renameCalled);
+      const editNode = document.querySelector(`.${EDITOR_CLASS}`);
+      if (!editNode) {
+        throw new Error('Edit node not found');
+      }
+      const itemNode = Array.from(
+        document.querySelectorAll(`.${ITEM_CLASS}`)
+      ).find(el => {
+        return el.contains(editNode);
+      });
+      if (!itemNode) {
+        throw new Error('Item node not found');
+      }
+      simulate(editNode, 'keydown', {
+        keyCode: 13,
+        key: 'Enter'
+      });
+      const fileModel = await created;
+      expect(fileModel.path).toContain(DRIVE_NAME);
+    });
+  });
+});

packages/notebook-extension/src/index.ts

@@ -48,7 +48,10 @@ import { IDocumentManager } from '@jupyterlab/docmanager';
 import { ToolbarItems as DocToolbarItems } from '@jupyterlab/docmanager-extension';
 import { DocumentRegistry, IDocumentWidget } from '@jupyterlab/docregistry';
 import { ISearchProviderRegistry } from '@jupyterlab/documentsearch';
-import { IDefaultFileBrowser } from '@jupyterlab/filebrowser';
+import {
+  IDefaultFileBrowser,
+  IFileBrowserFactory
+} from '@jupyterlab/filebrowser';
 import { ILauncher } from '@jupyterlab/launcher';
 import {
   ILSPCodeExtractorsManager,
@@ -363,7 +366,8 @@ const trackerPlugin: JupyterFrontEndPlugin<INotebookTracker> = {
     ISettingRegistry,
     ISessionContextDialogs,
     ITranslator,
-    IFormRendererRegistry
+    IFormRendererRegistry,
+    IFileBrowserFactory
   ],
   activate: activateNotebookHandler,
   autoStart: true
@@ -1594,7 +1598,8 @@ function activateNotebookHandler(
   settingRegistry: ISettingRegistry | null,
   sessionDialogs_: ISessionContextDialogs | null,
   translator_: ITranslator | null,
-  formRegistry: IFormRendererRegistry | null
+  formRegistry: IFormRendererRegistry | null,
+  filebrowserFactory: IFileBrowserFactory | null
 ): INotebookTracker {
   const translator = translator_ ?? nullTranslator;
   const sessionDialogs =
@@ -1957,7 +1962,9 @@ function activateNotebookHandler(
     caption: trans.__('Create a new notebook'),
     icon: args => (args['isPalette'] ? undefined : notebookIcon),
     execute: args => {
-      const cwd = (args['cwd'] as string) || (defaultBrowser?.model.path ?? '');
+      const currentBrowser =
+        filebrowserFactory?.tracker.currentWidget ?? defaultBrowser;
+      const cwd = (args['cwd'] as string) || (currentBrowser?.model.path ?? '');
       const kernelId = (args['kernelId'] as string) || '';
       const kernelName = (args['kernelName'] as string) || '';
       return createNew(cwd, kernelId, kernelName);

packages/services/src/testutils.ts

@@ -307,31 +307,48 @@ export const SessionConnectionMock = jest.fn<
  * A mock contents manager.
  */
 export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
-  const files = new Map<string, Contents.IModel>();
+  const files = new Map<string, Map<string, Contents.IModel>>();
   const dummy = new ContentsManager();
   const checkpoints = new Map<string, Contents.ICheckpointModel>();
   const checkPointContent = new Map<string, string>();
 
   const baseModel = Private.createFile({ type: 'directory' });
-  files.set('', { ...baseModel, path: '', name: '' });
+  // create the default drive
+  files.set(
+    '',
+    new Map<string, Contents.IModel>([
+      ['', { ...baseModel, path: '', name: '' }]
+    ])
+  );
 
   const thisObject: Contents.IManager = {
     ...jest.requireActual('@jupyterlab/services'),
     newUntitled: jest.fn(options => {
-      const model = Private.createFile(options || {});
-      files.set(model.path, model);
+      const driveName = dummy.driveName(options?.path || '');
+      const localPath = dummy.localPath(options?.path || '');
+      // create the test file without the drive name
+      const createOptions = { ...options, path: localPath };
+      const model = Private.createFile(createOptions || {});
+      // re-add the drive name to the model
+      const drivePath = driveName ? `${driveName}:${model.path}` : model.path;
+      const driveModel = {
+        ...model,
+        path: drivePath
+      };
+      files.get(driveName)!.set(model.path, driveModel);
       fileChangedSignal.emit({
         type: 'new',
         oldValue: null,
-        newValue: model
+        newValue: driveModel
       });
-      return Promise.resolve(model);
+      return Promise.resolve(driveModel);
     }),
     createCheckpoint: jest.fn(path => {
       const lastModified = new Date().toISOString();
       const data = { id: UUID.uuid4(), last_modified: lastModified };
       checkpoints.set(path, data);
-      checkPointContent.set(path, files.get(path)?.content);
+      // TODO: handle drives
+      checkPointContent.set(path, files.get('')!.get(path)?.content);
       return Promise.resolve(data);
     }),
     listCheckpoints: jest.fn(path => {
@@ -352,7 +369,8 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
       if (!checkpoints.has(path)) {
         return Private.makeResponseError(404);
       }
-      (files.get(path) as any).content = checkPointContent.get(path);
+      // TODO: handle drives
+      (files.get('')!.get(path) as any).content = checkPointContent.get(path);
       return Promise.resolve();
     }),
     getSharedModelFactory: jest.fn(() => {
@@ -368,11 +386,14 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
       return dummy.resolvePath(root, path);
     }),
     get: jest.fn((path, options) => {
-      path = Private.fixSlash(path);
-      if (!files.has(path)) {
+      const driveName = dummy.driveName(path);
+      const localPath = dummy.localPath(path);
+      const drive = files.get(driveName)!;
+      path = Private.fixSlash(localPath);
+      if (!drive.has(path)) {
         return Private.makeResponseError(404);
       }
-      const model = files.get(path)!;
+      const model = drive.get(path)!;
       const overrides: { hash?: string; last_modified?: string } = {};
       if (path == 'random-hash.txt') {
         overrides.hash = Math.random().toString();
@@ -385,10 +406,11 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
       if (model.type === 'directory') {
         if (options?.content !== false) {
           const content: Contents.IModel[] = [];
-          files.forEach(fileModel => {
+          drive.forEach(fileModel => {
+            const localPath = dummy.localPath(fileModel.path);
             if (
               // If file path is under this directory, add it to contents array.
-              PathExt.dirname(fileModel.path) == model.path &&
+              PathExt.dirname(localPath) == model.path &&
               // But the directory should exclude itself from the contents array.
               fileModel !== model
             ) {
@@ -408,16 +430,20 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
       return dummy.driveName(path);
     }),
     rename: jest.fn((oldPath, newPath) => {
-      oldPath = Private.fixSlash(oldPath);
-      newPath = Private.fixSlash(newPath);
-      if (!files.has(oldPath)) {
+      const driveName = dummy.driveName(oldPath);
+      const drive = files.get(driveName)!;
+      let oldLocalPath = dummy.localPath(oldPath);
+      let newLocalPath = dummy.localPath(newPath);
+      oldLocalPath = Private.fixSlash(oldLocalPath);
+      newLocalPath = Private.fixSlash(newLocalPath);
+      if (!drive.has(oldLocalPath)) {
         return Private.makeResponseError(404);
       }
-      const oldValue = files.get(oldPath)!;
-      files.delete(oldPath);
-      const name = PathExt.basename(newPath);
-      const newValue = { ...oldValue, name, path: newPath };
-      files.set(newPath, newValue);
+      const oldValue = drive.get(oldPath)!;
+      drive.delete(oldPath);
+      const name = PathExt.basename(newLocalPath);
+      const newValue = { ...oldValue, name, path: newLocalPath };
+      drive.set(newPath, newValue);
       fileChangedSignal.emit({
         type: 'rename',
         oldValue,
@@ -426,12 +452,15 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
       return Promise.resolve(newValue);
     }),
     delete: jest.fn(path => {
-      path = Private.fixSlash(path);
-      if (!files.has(path)) {
+      const driveName = dummy.driveName(path);
+      const localPath = dummy.localPath(path);
+      const drive = files.get(driveName)!;
+      path = Private.fixSlash(localPath);
+      if (!drive.has(path)) {
         return Private.makeResponseError(404);
       }
-      const oldValue = files.get(path)!;
-      files.delete(path);
+      const oldValue = drive.get(path)!;
+      drive.delete(path);
       fileChangedSignal.emit({
         type: 'delete',
         oldValue,
@@ -445,21 +474,22 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
       }
       path = Private.fixSlash(path);
       const timeStamp = new Date().toISOString();
-      if (files.has(path)) {
+      const drive = files.get(dummy.driveName(path))!;
+      if (drive.has(path)) {
         const updates =
           path == 'frozen-time-and-hash.txt'
             ? {}
             : {
                 last_modified: timeStamp,
                 hash: timeStamp
               };
-        files.set(path, {
-          ...files.get(path)!,
+        drive.set(path, {
+          ...drive.get(path)!,
           ...options,
           ...updates
         });
       } else {
-        files.set(path, {
+        drive.set(path, {
           path,
           name: PathExt.basename(path),
           content: '',
@@ -477,15 +507,21 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
       fileChangedSignal.emit({
         type: 'save',
         oldValue: null,
-        newValue: files.get(path)!
+        newValue: drive.get(path)!
       });
-      return Promise.resolve(files.get(path)!);
+      return Promise.resolve(drive.get(path)!);
     }),
     getDownloadUrl: jest.fn(path => {
       return dummy.getDownloadUrl(path);
     }),
     addDrive: jest.fn(drive => {
       dummy.addDrive(drive);
+      files.set(
+        drive.name,
+        new Map<string, Contents.IModel>([
+          ['', { ...baseModel, path: '', name: '' }]
+        ])
+      );
     }),
     dispose: jest.fn()
   };