Several fixes + refactor to Collection methods

server/api/documents.js

@@ -235,17 +235,15 @@ router.post('documents.update', auth(), async ctx => {
   if (text) document.text = text;
   document.lastModifiedById = user.id;
 
-  const [updatedDocument, updatedCollection] = await Promise.all([
-    document.save(),
-    collection.type === 'atlas'
-      ? await collection.updateDocument(document)
-      : Promise.resolve(),
-  ]);
+  await document.save();
+  if (collection.type === 'atlas') {
+    await collection.updateDocument(document);
+  }
 
-  updatedDocument.collection = updatedCollection;
+  document.collection = collection;
 
   ctx.body = {
-    data: await presentDocument(ctx, updatedDocument),
+    data: await presentDocument(ctx, document),
   };
 });
 
@@ -258,6 +256,10 @@ router.post('documents.move', auth(), async ctx => {
 
   const user = ctx.state.user;
   const document = await Document.findById(id);
+  const collection = await Collection.findById(document.atlasId);
+
+  if (collection.type !== 'atlas')
+    throw httpErrors.BadRequest("This document can't be moved");
 
   if (!document || document.teamId !== user.teamId) throw httpErrors.NotFound();
 
@@ -277,16 +279,10 @@ router.post('documents.move', auth(), async ctx => {
   document.parentDocumentId = parentDocument;
   await document.save();
 
-  const collection = await Collection.findById(document.atlasId);
-  if (collection.type === 'atlas') {
-    await collection.deleteDocument(document);
-    await collection.addDocumentToStructure(document, index);
-  }
+  await collection.moveDocument(document, index);
   // Update collection
   document.collection = collection;
 
-  document.collection = collection;
-
   ctx.body = {
     data: await presentDocument(ctx, document),
   };
@@ -311,9 +307,9 @@ router.post('documents.delete', auth(), async ctx => {
       );
     }
 
-    // Delete all children
+    // Delete document and all of its children
     try {
-      await collection.deleteDocument(document);
+      await collection.removeDocument(document);
     } catch (e) {
       throw httpErrors.BadRequest('Error while deleting');
     }

server/models/Collection.js

@@ -94,13 +94,11 @@ Collection.associate = models => {
 // Instance methods
 
 Collection.prototype.getUrl = function() {
-  // const slugifiedName = slug(this.name);
-  // return `/${slugifiedName}-c${this.urlId}`;
   return `/collections/${this.id}`;
 };
 
 Collection.prototype.getDocumentsStructure = async function() {
-  // Lazy fill this.documentStructure
+  // Lazy fill this.documentStructure - TMP for internal release
   if (!this.documentStructure) {
     this.documentStructure = this.navigationTree.children;
 
@@ -121,26 +119,35 @@ Collection.prototype.getDocumentsStructure = async function() {
   return this.documentStructure;
 };
 
-Collection.prototype.addDocumentToStructure = async function(document, index) {
+Collection.prototype.addDocumentToStructure = async function(
+  document,
+  index,
+  options = {}
+) {
   if (!this.documentStructure) return;
 
+  // If moving existing document with children, use existing structure to
+  // keep everything in shape and not loose documents
+  const documentJson = {
+    ...document.toJSON(),
+    ...options.documentJson,
+  };
+
   if (!document.parentDocumentId) {
     this.documentStructure.splice(
-      index || this.documentStructure.length,
+      index !== undefined ? index : this.documentStructure.length,
       0,
-      document.toJSON()
+      documentJson
     );
-    // Sequelize doesn't seem to set the value with splice on JSONB field
-    this.documentStructure = this.documentStructure;
   } else {
     // Recursively place document
     const placeDocument = documentList => {
       return documentList.map(childDocument => {
         if (document.parentDocumentId === childDocument.id) {
           childDocument.children.splice(
-            index || childDocument.children.length,
+            index !== undefined ? index : childDocument.children.length,
             0,
-            document.toJSON()
+            documentJson
           );
         } else {
           childDocument.children = placeDocument(childDocument.children);
@@ -152,10 +159,16 @@ Collection.prototype.addDocumentToStructure = async function(document, index) {
     this.documentStructure = placeDocument(this.documentStructure);
   }
 
+  // Sequelize doesn't seem to set the value with splice on JSONB field
+  this.documentStructure = this.documentStructure;
   await this.save();
+
   return this;
 };
 
+/**
+ * Update document's title and url in the documentStructure
+ */
 Collection.prototype.updateDocument = async function(updatedDocument) {
   if (!this.documentStructure) return;
   const { id } = updatedDocument;
@@ -179,30 +192,83 @@ Collection.prototype.updateDocument = async function(updatedDocument) {
   return this;
 };
 
-Collection.prototype.deleteDocument = async function(document) {
+/**
+ * moveDocument is combination of removing the document from the structure
+ * and placing it back the the new location with the existing children.
+ */
+Collection.prototype.moveDocument = async function(document, index) {
   if (!this.documentStructure) return;
 
-  const deleteFromChildren = (children, id) => {
-    if (_.find(children, { id })) {
-      _.remove(children, { id });
-    } else {
-      children = children.map(childDocument => {
+  const documentJson = await this.removeDocument(document, {
+    deleteDocument: false,
+  });
+  await this.addDocumentToStructure(document, index, { documentJson });
+
+  return this;
+};
+
+type DeleteDocumentOptions = {
+  deleteDocument: boolean,
+};
+
+/**
+ * removeDocument is used for both deleting documents (deleteDocument: true)
+ * and removing them temporarily from the structure while they are being moved
+ * (deleteDocument: false).
+ */
+Collection.prototype.removeDocument = async function(
+  document,
+  options: DeleteDocumentOptions = { deleteDocument: true }
+) {
+  if (!this.documentStructure) return;
+  let returnValue;
+
+  // Helper to destroy all child documents for a document
+  const deleteChildren = async documentId => {
+    const childDocuments = await Document.findAll({
+      where: { parentDocumentId: documentId },
+    });
+    childDocuments.forEach(async child => {
+      await deleteChildren(child.id);
+      await child.destroy();
+    });
+  };
+
+  // Prune, and destroy if needed, from the document structure
+  const deleteFromChildren = async (children, id) => {
+    children = await Promise.all(
+      children.map(async childDocument => {
         return {
           ...childDocument,
-          children: deleteFromChildren(childDocument.children, id),
+          children: await deleteFromChildren(childDocument.children, id),
         };
-      });
+      })
+    );
+
+    const match = _.find(children, { id });
+    if (match) {
+      if (!options.deleteDocument && !returnValue) returnValue = match;
+      _.remove(children, { id });
+
+      if (options.deleteDocument) {
+        const childDocument = await Document.findById(id);
+        // Delete the actual document
+        await childDocument.destroy();
+        // Delete all child documents
+        await deleteChildren(id);
       }
+    }
+
     return children;
   };
 
-  this.documentStructure = deleteFromChildren(
+  this.documentStructure = await deleteFromChildren(
     this.documentStructure,
     document.id
   );
 
-  await this.save();
-  return this;
+  if (options.deleteDocument) await this.save();
+  return returnValue;
 };
 
 export default Collection;

server/models/Collection.test.js

@@ -0,0 +1,275 @@
+/* eslint-disable flowtype/require-valid-file-annotation */
+import { flushdb, seed } from '../test/support';
+import Collection from '../models/Collection';
+import Document from '../models/Document';
+
+beforeEach(flushdb);
+beforeEach(jest.resetAllMocks);
+
+describe('#getUrl', () => {
+  test('should return correct url for the collection', () => {
+    const collection = new Collection({ id: '1234' });
+    expect(collection.getUrl()).toBe('/collections/1234');
+  });
+});
+
+describe('#addDocumentToStructure', async () => {
+  test('should add as last element without index', async () => {
+    const { collection } = await seed();
+    const newDocument = new Document({
+      id: '5',
+      title: 'New end node',
+      parentDocumentId: null,
+    });
+
+    await collection.addDocumentToStructure(newDocument);
+    expect(collection.documentStructure.length).toBe(3);
+    expect(collection.documentStructure[2].id).toBe('5');
+  });
+
+  test('should add with an index', async () => {
+    const { collection } = await seed();
+    const newDocument = new Document({
+      id: '5',
+      title: 'New end node',
+      parentDocumentId: null,
+    });
+
+    await collection.addDocumentToStructure(newDocument, 1);
+    expect(collection.documentStructure.length).toBe(3);
+    expect(collection.documentStructure[1].id).toBe('5');
+  });
+
+  test('should add as a child if with parent', async () => {
+    const { collection, document } = await seed();
+    const newDocument = new Document({
+      id: '5',
+      title: 'New end node',
+      parentDocumentId: document.id,
+    });
+
+    await collection.addDocumentToStructure(newDocument, 1);
+    expect(collection.documentStructure.length).toBe(2);
+    expect(collection.documentStructure[1].id).toBe(document.id);
+    expect(collection.documentStructure[1].children.length).toBe(1);
+    expect(collection.documentStructure[1].children[0].id).toBe('5');
+  });
+
+  test('should add as a child if with parent with index', async () => {
+    const { collection, document } = await seed();
+    const newDocument = new Document({
+      id: '5',
+      title: 'node',
+      parentDocumentId: document.id,
+    });
+    const secondDocument = new Document({
+      id: '6',
+      title: 'New start node',
+      parentDocumentId: document.id,
+    });
+
+    await collection.addDocumentToStructure(newDocument);
+    await collection.addDocumentToStructure(secondDocument, 0);
+    expect(collection.documentStructure.length).toBe(2);
+    expect(collection.documentStructure[1].id).toBe(document.id);
+    expect(collection.documentStructure[1].children.length).toBe(2);
+    expect(collection.documentStructure[1].children[0].id).toBe('6');
+  });
+
+  describe('options: documentJson', async () => {
+    test("should append supplied json over document's own", async () => {
+      const { collection } = await seed();
+      const newDocument = new Document({
+        id: '5',
+        title: 'New end node',
+        parentDocumentId: null,
+      });
+
+      await collection.addDocumentToStructure(newDocument, undefined, {
+        documentJson: {
+          children: [
+            {
+              id: '7',
+              title: 'Totally fake',
+              children: [],
+            },
+          ],
+        },
+      });
+      expect(collection.documentStructure[2].children.length).toBe(1);
+      expect(collection.documentStructure[2].children[0].id).toBe('7');
+    });
+  });
+});
+
+describe('#updateDocument', () => {
+  test("should update root document's data", async () => {
+    const { collection, document } = await seed();
+
+    document.title = 'Updated title';
+    await document.save();
+
+    await collection.updateDocument(document);
+
+    expect(collection.documentStructure[1].title).toBe('Updated title');
+  });
+
+  test("should update child document's data", async () => {
+    const { collection, document } = await seed();
+    // Add a child for testing
+    const newDocument = await Document.create({
+      parentDocumentId: document.id,
+      atlasId: collection.id,
+      teamId: collection.teamId,
+      userId: collection.creatorId,
+      lastModifiedById: collection.creatorId,
+      createdById: collection.creatorId,
+      title: 'Child document',
+      text: 'content',
+    });
+    await collection.addDocumentToStructure(newDocument);
+
+    newDocument.title = 'Updated title';
+    await newDocument.save();
+
+    await collection.updateDocument(newDocument);
+
+    expect(collection.documentStructure[1].children[0].title).toBe(
+      'Updated title'
+    );
+  });
+});
+
+describe('#moveDocument', () => {
+  test('should move a document without children', async () => {
+    const { collection, document } = await seed();
+
+    expect(collection.documentStructure[1].id).toBe(document.id);
+    await collection.moveDocument(document, 0);
+    expect(collection.documentStructure[0].id).toBe(document.id);
+  });
+
+  test('should move a document with children', async () => {
+    const { collection, document } = await seed();
+
+    // Add a child for testing
+    const newDocument = await Document.create({
+      parentDocumentId: document.id,
+      atlasId: collection.id,
+      teamId: collection.teamId,
+      userId: collection.creatorId,
+      lastModifiedById: collection.creatorId,
+      createdById: collection.creatorId,
+      title: 'Child document',
+      text: 'content',
+    });
+    await collection.addDocumentToStructure(newDocument);
+
+    await collection.moveDocument(document, 0);
+    expect(collection.documentStructure[0].children[0].id).toBe(newDocument.id);
+  });
+});
+
+describe('#removeDocument', () => {
+  const destroyMock = jest.fn();
+
+  test('should save if removing', async () => {
+    const { collection, document } = await seed();
+    jest.spyOn(collection, 'save');
+
+    await collection.removeDocument(document);
+    expect(collection.save).toBeCalled();
+  });
+
+  test('should remove documents from root', async () => {
+    const { collection, document } = await seed();
+
+    await collection.removeDocument(document);
+    expect(collection.documentStructure.length).toBe(1);
+
+    // Verify that the document was removed
+    const collectionDocuments = await Document.findAndCountAll({
+      where: {
+        atlasId: collection.id,
+      },
+    });
+    expect(collectionDocuments.count).toBe(1);
+  });
+
+  test('should remove a document with child documents', async () => {
+    const { collection, document } = await seed();
+
+    // Add a child for testing
+    const newDocument = await Document.create({
+      parentDocumentId: document.id,
+      atlasId: collection.id,
+      teamId: collection.teamId,
+      userId: collection.creatorId,
+      lastModifiedById: collection.creatorId,
+      createdById: collection.creatorId,
+      title: 'Child document',
+      text: 'content',
+    });
+    await collection.addDocumentToStructure(newDocument);
+    expect(collection.documentStructure[1].children.length).toBe(1);
+
+    // Remove the document
+    await collection.removeDocument(document);
+    expect(collection.documentStructure.length).toBe(1);
+    const collectionDocuments = await Document.findAndCountAll({
+      where: {
+        atlasId: collection.id,
+      },
+    });
+    expect(collectionDocuments.count).toBe(1);
+  });
+
+  test('should remove a child document', async () => {
+    const { collection, document } = await seed();
+
+    // Add a child for testing
+    const newDocument = await Document.create({
+      parentDocumentId: document.id,
+      atlasId: collection.id,
+      teamId: collection.teamId,
+      userId: collection.creatorId,
+      lastModifiedById: collection.creatorId,
+      createdById: collection.creatorId,
+      title: 'Child document',
+      text: 'content',
+    });
+    await collection.addDocumentToStructure(newDocument);
+    expect(collection.documentStructure.length).toBe(2);
+    expect(collection.documentStructure[1].children.length).toBe(1);
+
+    // Remove the document
+    await collection.removeDocument(newDocument);
+
+    expect(collection.documentStructure.length).toBe(2);
+    expect(collection.documentStructure[0].children.length).toBe(0);
+    expect(collection.documentStructure[1].children.length).toBe(0);
+
+    const collectionDocuments = await Document.findAndCountAll({
+      where: {
+        atlasId: collection.id,
+      },
+    });
+    expect(collectionDocuments.count).toBe(2);
+  });
+
+  describe('options: deleteDocument = false', () => {
+    test('should remove documents from the structure but not destroy them from the DB', async () => {
+      const { collection, document } = await seed();
+      jest.spyOn(collection, 'save');
+
+      const removedNode = await collection.removeDocument(document, {
+        deleteDocument: false,
+      });
+      expect(collection.documentStructure.length).toBe(1);
+      expect(destroyMock).not.toBeCalled();
+      expect(collection.save).not.toBeCalled();
+      expect(removedNode.id).toBe(document.id);
+      expect(removedNode.children).toEqual([]);
+    });
+  });
+});