Merge pull request #3033 from blink1073/saveas-readonly-modified

Allow saveAs for readOnly files

packages/docmanager-extension/src/index.ts

@@ -231,12 +231,12 @@ function addCommands(app: JupyterLab, docManager: IDocumentManager, palette: ICo
 
   commands.addCommand(CommandIDs.saveAs, {
     label: 'Save As...',
-    caption: 'Save with new path and create checkpoint',
+    caption: 'Save with new path',
     isEnabled,
     execute: () => {
       if (isEnabled()) {
         let context = docManager.contextForWidget(app.shell.currentWidget);
-        return context.saveAs().then(() => context.createCheckpoint());
+        return context.saveAs();
       }
     }
   });

packages/docmanager/src/savehandler.ts

@@ -121,9 +121,10 @@ class SaveHandler implements IDisposable {
       return;
     }
 
-    // Bail if the model is not dirty or it is read only, or the dialog
+    // Bail if the model is not dirty or the file is not writable, or the dialog
     // is already showing.
-    if (!context.model.dirty || context.model.readOnly || this._inDialog) {
+    let writable = context.contentsModel && context.contentsModel.writable;
+    if (!writable || !context.model.dirty || this._inDialog) {
       return;
     }
 

packages/docregistry/src/context.ts

@@ -200,9 +200,6 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
    */
   save(): Promise<void> {
     let model = this._model;
-    if (model.readOnly) {
-      return Promise.reject(new Error('Read only'));
-    }
     let content: JSONValue;
     if (this._factory.fileFormat === 'json') {
       content = model.toJSON();
@@ -257,9 +254,20 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
       if (this.isDisposed || !newPath) {
         return;
       }
-      this._path = newPath;
-      this.session.setName(newPath.split('/').pop()!);
-      return this.session.setPath(newPath).then(() => this.save());
+      if (newPath === this._path) {
+        return this.save();
+      }
+      // Make sure the path does not exist.
+      return this._manager.ready.then(() => {
+        return this._manager.contents.get(newPath);
+      }).then(() => {
+        return this._maybeOverWrite(newPath);
+      }).catch(err => {
+        if (!err.xhr || err.xhr.status !== 404) {
+          throw err;
+        }
+        return this._finishSaveAs(newPath);
+      });
     });
   }
 
@@ -413,7 +421,10 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
   /**
    * Handle a change to a session property.
    */
-  private _onSessionChanged() {
+  private _onSessionChanged(sender: IClientSession, type: string): void {
+    if (type !== 'path') {
+      return;
+    }
     let path = this.session.path;
     if (path !== this._path) {
       this._path = path;
@@ -438,9 +449,6 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
     };
     let mod = this._contentsModel ? this._contentsModel.last_modified : null;
     this._contentsModel = newModel;
-    if (!newModel.writable) {
-      this._model.readOnly = true;
-    }
     if (!mod || newModel.last_modified !== mod) {
       this._fileChanged.emit(newModel);
     }
@@ -452,21 +460,8 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
   private _populate(): Promise<void> {
     this._isPopulated = true;
 
-    // Add a checkpoint if none exists and the file is not read only.
-    let promise = Promise.resolve(void 0);
-    if (!this._model.readOnly) {
-      promise = this.listCheckpoints().then(checkpoints => {
-        if (!this.isDisposed && !checkpoints && !this._model.readOnly) {
-          return this.createCheckpoint().then(() => { /* no-op */ });
-        }
-      }).catch(err => {
-        // Handle a read-only folder.
-        if (err.message !== 'Forbidden') {
-          throw err;
-        }
-      });
-    }
-    return promise.then(() => {
+    // Add a checkpoint if none exists and the file is writable.
+    return this._maybeCheckpoint(false).then(() => {
       if (this.isDisposed) {
         return;
       }
@@ -509,13 +504,40 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
       }
       return this._manager.contents.save(path, options);
     }, (err) => {
-      if (err.xhr.status === 404) {
+      if (err.xhr && err.xhr.status === 404) {
         return this._manager.contents.save(path, options);
       }
       throw err;
     });
   }
 
+  /**
+   * Add a checkpoint the file is writable.
+   */
+  private _maybeCheckpoint(force: boolean): Promise<void> {
+    let writable = this._contentsModel && this._contentsModel.writable;
+    let promise = Promise.resolve(void 0);
+    if (!writable) {
+      return promise;
+    }
+    if (force) {
+      promise = this.createCheckpoint();
+    } else {
+      promise = this.listCheckpoints().then(checkpoints => {
+        writable = this._contentsModel && this._contentsModel.writable;
+        if (!this.isDisposed && !checkpoints.length && writable) {
+          return this.createCheckpoint().then(() => { /* no-op */ });
+        }
+      });
+    }
+    return promise.catch(err => {
+      // Handle a read-only folder.
+      if (!err.xhr || err.xhr.status !== 403) {
+        throw err;
+      }
+    });
+  }
+
   /**
    * Handle a time conflict.
    */
@@ -545,6 +567,41 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
     });
   }
 
+  /**
+   * Handle a time conflict.
+   */
+  private _maybeOverWrite(path: string): Promise<void> {
+    let body = `"${path}" already exists. Do you want to replace it?`;
+    let overwriteBtn = Dialog.warnButton({ label: 'OVERWRITE' });
+    return showDialog({
+      title: 'File Overwrite?', body,
+      buttons: [Dialog.cancelButton(), overwriteBtn]
+    }).then(result => {
+      if (this.isDisposed) {
+        return Promise.reject('Disposed');
+      }
+      if (result.button.label === 'OVERWRITE') {
+        return this._manager.contents.delete(path).then(() => {
+          this._finishSaveAs(path);
+        });
+      }
+    });
+  }
+
+  /**
+   * Finish a saveAs operation given a new path.
+   */
+  private _finishSaveAs(newPath: string): Promise<void> {
+    this._path = newPath;
+    return this.session.setPath(newPath).then(() => {
+      this.session.setName(newPath.split('/').pop()!);
+      return this.save();
+    }).then(() => {
+      this._pathChanged.emit(this._path);
+      return this._maybeCheckpoint(true);
+    });
+  }
+
   private _manager: ServiceManager.IManager;
   private _opener: (widget: Widget) => void;
   private _model: T;

test/src/docmanager/savehandler.spec.ts

@@ -43,6 +43,7 @@ describe('docregistry/savehandler', () => {
   beforeEach(() => {
     context = new Context({ manager, factory, path: uuid() + '.txt' });
     handler = new SaveHandler({ context, manager });
+    return context.save();
   });
 
   afterEach(() => {

test/src/docregistry/context.spec.ts

@@ -3,6 +3,10 @@
 
 import expect = require('expect.js');
 
+import {
+  uuid
+} from '@jupyterlab/coreutils';
+
 import {
   Contents, ServiceManager
 } from '@jupyterlab/services';
@@ -16,7 +20,7 @@ import {
 } from '@jupyterlab/docregistry';
 
 import {
-  waitForDialog, acceptDialog
+  waitForDialog, acceptDialog, dismissDialog
 } from '../utils';
 
 
@@ -35,7 +39,7 @@ describe('docregistry/context', () => {
     let context: Context<DocumentRegistry.IModel>;
 
     beforeEach(() => {
-      context = new Context({ manager, factory, path: 'foo' });
+      context = new Context({ manager, factory, path: uuid() + '.txt' });
     });
 
     afterEach(() => {
@@ -47,7 +51,7 @@ describe('docregistry/context', () => {
     describe('#constructor()', () => {
 
       it('should create a new context', () => {
-        context = new Context({ manager, factory, path: 'bar' });
+        context = new Context({ manager, factory, path: uuid() + '.txt' });
         expect(context).to.be.a(Context);
       });
 
@@ -71,12 +75,13 @@ describe('docregistry/context', () => {
     describe('#fileChanged', () => {
 
       it('should be emitted when the file is saved', (done) => {
+        let path = context.path;
         context.fileChanged.connect((sender, args) => {
           expect(sender).to.be(context);
-          expect(args.name).to.be('foo');
+          expect(args.path).to.be(path);
           done();
         });
-        context.save();
+        context.save().catch(done);
       });
 
     });
@@ -145,7 +150,7 @@ describe('docregistry/context', () => {
     describe('#path', () => {
 
       it('should be the current path for the context', () => {
-        expect(context.path).to.be('foo');
+        expect(typeof context.path).to.be('string');
       });
 
     });
@@ -157,8 +162,9 @@ describe('docregistry/context', () => {
       });
 
       it('should be set after poulation', (done) => {
+        let path = context.path;
         context.ready.then(() => {
-          expect(context.contentsModel.name).to.be('foo');
+          expect(context.contentsModel.path).to.be(path);
           done();
         });
         context.save().catch(done);
@@ -197,9 +203,9 @@ describe('docregistry/context', () => {
 
     describe('#save()', () => {
 
-      it('should save the contents of the file to disk', (done) => {
+      it('should save the contents of the file to disk', () => {
         context.model.fromString('foo');
-        context.save().then(() => {
+        return context.save().then(() => {
           let opts: Contents.IFetchOptions = {
             format: factory.fileFormat,
             type: factory.contentType,
@@ -208,8 +214,7 @@ describe('docregistry/context', () => {
           return manager.contents.get(context.path, opts);
         }).then(model => {
           expect(model.content).to.be('foo');
-          done();
-        }).catch(done);
+        });
       });
 
     });
@@ -217,25 +222,76 @@ describe('docregistry/context', () => {
 
     describe('#saveAs()', () => {
 
-      it('should save the document to a different path chosen by the user', (done) => {
+      it('should save the document to a different path chosen by the user', () => {
+        const newPath = uuid() + '.txt';
         waitForDialog().then(() => {
           let dialog = document.body.getElementsByClassName('jp-Dialog')[0];
           let input = dialog.getElementsByTagName('input')[0];
-          input.value = 'bar';
+          input.value = newPath;
           acceptDialog();
         });
-        context.saveAs().then(() => {
-          expect(context.path).to.be('bar');
-          done();
-        }).catch(done);
+        return context.save().then(() => {
+          return context.saveAs();
+        }).then(() => {
+          expect(context.path).to.be(newPath);
+        });
+      });
+
+      it('should bring up a conflict dialog', () => {
+        const newPath = uuid() + '.txt';
+        waitForDialog().then(() => {
+          let dialog = document.body.getElementsByClassName('jp-Dialog')[0];
+          let input = dialog.getElementsByTagName('input')[0];
+          input.value = newPath;
+          return acceptDialog();
+        }).then(() => {
+          return waitForDialog();
+        }).then(() => {
+          acceptDialog();
+        });
+        return context.save().then(() => {
+          return context.saveAs();
+        }).then(() => {
+          expect(context.path).to.be(newPath);
+        });
+      });
+
+      it('should keep the file if overwrite is aborted', () => {
+        let oldPath = context.path;
+        let newPath = uuid() + '.txt';
+        waitForDialog().then(() => {
+          let dialog = document.body.getElementsByClassName('jp-Dialog')[0];
+          let input = dialog.getElementsByTagName('input')[0];
+          input.value = newPath;
+          return acceptDialog();
+        }).then(() => {
+          return waitForDialog();
+        }).then(() => {
+          dismissDialog();
+        });
+        return context.save().then(() => {
+          return context.saveAs();
+        }).then(() => {
+          expect(context.path).to.be(oldPath);
+        });
+      });
+
+      it('should just save if the file name does not change', () => {
+        acceptDialog();
+        let path = context.path;
+        return context.save().then(() => {
+          return context.saveAs();
+        }).then(() => {
+          expect(context.path).to.be(path);
+        });
       });
 
     });
 
     describe('#revert()', () => {
 
-      it('should revert the contents of the file to the disk', (done) => {
-        manager.contents.save(context.path, {
+      it('should revert the contents of the file to the disk', () => {
+        return manager.contents.save(context.path, {
           type: factory.contentType,
           format: factory.fileFormat,
           content: 'foo'
@@ -244,45 +300,46 @@ describe('docregistry/context', () => {
           return context.revert();
         }).then(() => {
           expect(context.model.toString()).to.be('foo');
-          done();
-        }).catch(done);
+        });
       });
 
     });
 
     describe('#createCheckpoint()', () => {
 
-      it('should create a checkpoint for the file', (done) => {
-        context.createCheckpoint().then(model => {
+      it('should create a checkpoint for the file', () => {
+        return context.save().then(() => {
+          return context.createCheckpoint();
+        }).then(model => {
           expect(model.id).to.be.ok();
           expect(model.last_modified).to.be.ok();
-          done();
-        }).catch(done);
+        });
       });
 
     });
 
     describe('#deleteCheckpoint()', () => {
 
-      it('should delete the given checkpoint', (done) => {
-        context.createCheckpoint().then(model => {
+      it('should delete the given checkpoint', () => {
+        return context.save().then(() => {
+          return context.createCheckpoint();
+        }).then(model => {
           return context.deleteCheckpoint(model.id);
         }).then(() => {
           return context.listCheckpoints();
         }).then(models => {
           expect(models.length).to.be(0);
-          done();
-        }).catch(done);
+        });
       });
 
     });
 
     describe('#restoreCheckpoint()', () => {
 
-      it('should restore the value to the last checkpoint value', (done) => {
+      it('should restore the value to the last checkpoint value', () => {
         context.model.fromString('bar');
         let id = '';
-        context.save().then(() => {
+        return context.save().then(() => {
           return context.createCheckpoint();
         }).then(model => {
           context.model.fromString('foo');
@@ -294,65 +351,63 @@ describe('docregistry/context', () => {
           return context.revert();
         }).then(() => {
           expect(context.model.toString()).to.be('bar');
-          done();
-        }).catch(done);
+        });
       });
 
     });
 
     describe('#listCheckpoints()', () => {
 
-      it('should list the checkpoints for the file', (done) => {
+      it('should list the checkpoints for the file', () => {
         let id = '';
-        context.createCheckpoint().then(model => {
-          id = model.id;
-          return context.listCheckpoints();
-        }).then(models => {
-          for (let model of models) {
-            if (model.id === id) {
-              done();
-              return;
+        return context.save().then(() => {
+          context.createCheckpoint().then(model => {
+            id = model.id;
+            return context.listCheckpoints();
+          }).then(models => {
+            for (let model of models) {
+              if (model.id === id) {
+                return;
+              }
             }
-          }
-        }).catch(done);
+            throw new Error('Model not found');
+          });
+        });
       });
 
     });
 
     describe('#resolveUrl()', () => {
 
-      it('should resolve a relative url to a correct server path', (done) => {
-        context.resolveUrl('./foo').then(path => {
+      it('should resolve a relative url to a correct server path', () => {
+        return context.resolveUrl('./foo').then(path => {
           expect(path).to.be('foo');
-        }).then(done, done);
+        });
       });
 
-      it('should ignore urls that have a protocol', (done) => {
-        context.resolveUrl('http://foo').then(path => {
+      it('should ignore urls that have a protocol', () => {
+        return context.resolveUrl('http://foo').then(path => {
           expect(path).to.be('http://foo');
-          done();
-        }).catch(done);
+        });
       });
 
     });
 
     describe('#getDownloadUrl()', () => {
 
-      it('should resolve an absolute server url to a download url', (done) => {
+      it('should resolve an absolute server url to a download url', () => {
         let contextPromise = context.getDownloadUrl('foo');
         let contentsPromise = manager.contents.getDownloadUrl('foo');
-        Promise.all([contextPromise, contentsPromise])
+        return Promise.all([contextPromise, contentsPromise])
         .then(values => {
           expect(values[0]).to.be(values[1]);
-          done();
-        }).catch(done);
+        });
       });
 
-      it('should ignore urls that have a protocol', (done) => {
-        context.getDownloadUrl('http://foo').then(path => {
+      it('should ignore urls that have a protocol', () => {
+        return context.getDownloadUrl('http://foo').then(path => {
           expect(path).to.be('http://foo');
-          done();
-        }).catch(done);
+        });
       });
 
     });