Merge pull request #80 from jorilallo/jori/collection-children

Collection backend refactor

frontend/models/Collection.js

@@ -14,7 +14,7 @@ class Collection {
   id: string;
   name: string;
   type: 'atlas' | 'journal';
-  navigationTree: NavigationNode;
+  documents: Array<NavigationNode>;
   updatedAt: string;
   url: string;
 

frontend/scenes/Collection/Collection.js

@@ -40,7 +40,7 @@ type State = {
           throw new Error('TODO code up non-atlas collections');
 
         this.setState({
-          redirectUrl: collection.navigationTree.url,
+          redirectUrl: collection.documents[0].url,
         });
       })
       .catch(() => {

frontend/scenes/Document/Document.js

@@ -117,11 +117,7 @@ type Props = {
               />
             : <a onClick={this.onEdit}>Edit</a>}
         </HeaderAction>
-        <Menu
+        <Menu store={this.store} document={this.store.document} />
-          store={this.store}
-          document={this.store.document}
-          collectionTree={this.store.collectionTree}
-        />
       </Flex>
     );
 

frontend/scenes/Document/DocumentStore.js

@@ -1,5 +1,5 @@
 // @flow
-import { observable, action, computed, toJS } from 'mobx';
+import { observable, action, computed } from 'mobx';
 import get from 'lodash/get';
 import invariant from 'invariant';
 import { client } from 'utils/ApiClient';
@@ -49,42 +49,22 @@ class DocumentStore {
     return !!this.document && this.document.collection.type === 'atlas';
   }
 
-  @computed get collectionTree(): ?Object {
-    if (
-      this.document &&
-      this.document.collection &&
-      this.document.collection.type === 'atlas'
-    ) {
-      const tree = this.document.collection.navigationTree;
-      const collapseNodes = node => {
-        node.collapsed = this.collapsedNodes.includes(node.id);
-        node.children = node.children.map(childNode => {
-          return collapseNodes(childNode);
-        });
-
-        return node;
-      };
-
-      return collapseNodes(toJS(tree));
-    }
-  }
-
   @computed get pathToDocument(): Array<NavigationNode> {
     let path;
-    const traveler = (node, previousPath) => {
+    const traveler = (nodes, previousPath) => {
-      if (this.document && node.id === this.document.id) {
+      nodes.forEach(childNode => {
-        path = previousPath;
+        const newPath = [...previousPath, childNode];
-        return;
+        if (childNode.id === this.document.id) {
-      } else {
+          path = previousPath;
-        node.children.forEach(childNode => {
+          return;
-          const newPath = [...previousPath, node];
+        } else {
           return traveler(childNode, newPath);
-        });
+        }
-      }
+      });
     };
 
-    if (this.document && this.collectionTree) {
+    if (this.document && this.document.collection.documents) {
-      traveler(this.collectionTree, []);
+      traveler(this.document.collection.documents, []);
       invariant(path, 'Path is not available for collection, abort');
       return path.splice(1);
     }

frontend/scenes/Document/components/Menu.js

@@ -11,7 +11,6 @@ import DocumentStore from '../DocumentStore';
 type Props = {
   history: Object,
   document: DocumentType,
-  collectionTree: ?Object,
   store: DocumentStore,
 };
 
@@ -19,8 +18,9 @@ type Props = {
   props: Props;
 
   onCreateDocument = () => {
-    invariant(this.props.collectionTree, 'collectionTree is not available');
+    // Disabled until created a better API
-    this.props.history.push(`${this.props.collectionTree.url}/new`);
+    // invariant(this.props.collectionTree, 'collectionTree is not available');
+    // this.props.history.push(`${this.props.collectionTree.url}/new`);
   };
 
   onCreateChild = () => {
@@ -55,24 +55,29 @@ type Props = {
 
   render() {
     const document = get(this.props, 'document');
-    const collection = get(document, 'collection.type') === 'atlas';
+    if (document) {
-    const allowDelete =
+      const collection = document.collection;
-      collection &&
+      const allowDelete =
-      document.id !== get(document, 'collection.navigationTree.id');
+        collection &&
+        collection.type === 'atlas' &&
+        collection.documents &&
+        collection.documents.length > 1;
 
-    return (
+      return (
-      <DropdownMenu label={<MoreIcon />}>
+        <DropdownMenu label={<MoreIcon />}>
-        {collection &&
+          {collection &&
-          <div>
+            <div>
-            <MenuItem onClick={this.onCreateDocument}>
+              <MenuItem onClick={this.onCreateDocument}>
-              New document
+                New document
-            </MenuItem>
+              </MenuItem>
-            <MenuItem onClick={this.onCreateChild}>New child</MenuItem>
+              <MenuItem onClick={this.onCreateChild}>New child</MenuItem>
-          </div>}
+            </div>}
-        <MenuItem onClick={this.onExport}>Export</MenuItem>
+          <MenuItem onClick={this.onExport}>Export</MenuItem>
-        {allowDelete && <MenuItem onClick={this.onDelete}>Delete</MenuItem>}
+          {allowDelete && <MenuItem onClick={this.onDelete}>Delete</MenuItem>}
-      </DropdownMenu>
+        </DropdownMenu>
-    );
+      );
+    }
+    return null;
   }
 }
 

frontend/types/index.js

@@ -15,7 +15,6 @@ export type NavigationNode = {
   id: string,
   title: string,
   url: string,
-  collapsed: boolean,
   children: Array<NavigationNode>,
 };
 

package.json

@@ -13,8 +13,9 @@
     "lint:js": "eslint frontend",
     "lint:flow": "flow check",
     "deploy": "git push heroku master",
-    "heroku-postbuild": "npm run build && npm run sequelize db:migrate",
+    "heroku-postbuild": "npm run build && npm run sequelize:migrate",
-    "sequelize": "./node_modules/.bin/sequelize",
+    "sequelize:create-migration": "sequelize migration:create",
+    "sequelize:migrate": "sequelize db:migrate",
     "test": "npm run test:frontend && npm run test:server",
     "test:frontend": "jest",
     "test:server": "jest --config=server/.jest-config --runInBand",

server/api/collections.js

@@ -74,25 +74,4 @@ router.post('collections.list', auth(), pagination(), async ctx => {
   };
 });
 
-router.post('collections.updateNavigationTree', auth(), async ctx => {
-  const { id, tree } = ctx.body;
-  ctx.assertPresent(id, 'id is required');
-
-  const user = ctx.state.user;
-  const atlas = await Collection.findOne({
-    where: {
-      id,
-      teamId: user.teamId,
-    },
-  });
-
-  if (!atlas) throw httpErrors.NotFound();
-
-  await atlas.updateNavigationTree(tree);
-
-  ctx.body = {
-    data: await presentCollection(ctx, atlas, true),
-  };
-});
-
 export default router;

server/api/documents.js

@@ -1,12 +1,11 @@
+// @flow
 import Router from 'koa-router';
 import httpErrors from 'http-errors';
-import { lock } from '../redis';
 import isUUID from 'validator/lib/isUUID';
 
 const URL_REGEX = /^[a-zA-Z0-9-]*-([a-zA-Z0-9]{10,15})$/;
 
 import auth from './middlewares/authentication';
-// import pagination from './middlewares/pagination';
 import { presentDocument } from '../presenters';
 import { Document, Collection } from '../models';
 
@@ -96,10 +95,14 @@ router.post('documents.search', auth(), async ctx => {
 });
 
 router.post('documents.create', auth(), async ctx => {
-  const { collection, title, text, parentDocument } = ctx.body;
+  const { collection, title, text, parentDocument, index } = ctx.body;
   ctx.assertPresent(collection, 'collection is required');
+  ctx.assertUuid(collection, 'collection must be an uuid');
   ctx.assertPresent(title, 'title is required');
   ctx.assertPresent(text, 'text is required');
+  if (parentDocument)
+    ctx.assertUuid(parentDocument, 'parentDocument must be an uuid');
+  if (index) ctx.assertPositiveInteger(index, 'index must be an integer (>=0)');
 
   const user = ctx.state.user;
   const ownerCollection = await Collection.findOne({
@@ -111,47 +114,36 @@ router.post('documents.create', auth(), async ctx => {
 
   if (!ownerCollection) throw httpErrors.BadRequest();
 
-  const document = await (() => {
+  let parentDocumentObj = {};
-    return new Promise(resolve => {
+  if (parentDocument && ownerCollection.type === 'atlas') {
-      lock(ownerCollection.id, 10000, async done => {
+    parentDocumentObj = await Document.findOne({
-        // FIXME: should we validate the existance of parentDocument?
+      where: {
-        let parentDocumentObj = {};
+        id: parentDocument,
-        if (parentDocument && ownerCollection.type === 'atlas') {
+        atlasId: ownerCollection.id,
-          parentDocumentObj = await Document.findOne({
+      },
-            where: {
-              id: parentDocument,
-              atlasId: ownerCollection.id,
-            },
-          });
-        }
-
-        const newDocument = await Document.create({
-          parentDocumentId: parentDocumentObj.id,
-          atlasId: ownerCollection.id,
-          teamId: user.teamId,
-          userId: user.id,
-          lastModifiedById: user.id,
-          createdById: user.id,
-          title,
-          text,
-        });
-
-        // TODO: Move to afterSave hook if possible with imports
-        if (parentDocument && ownerCollection.type === 'atlas') {
-          await ownerCollection.reload();
-          ownerCollection.addNodeToNavigationTree(newDocument);
-          await ownerCollection.save();
-        }
-
-        done(resolve(newDocument));
-      });
     });
-  })();
+  }
+
+  const newDocument = await Document.create({
+    parentDocumentId: parentDocumentObj.id,
+    atlasId: ownerCollection.id,
+    teamId: user.teamId,
+    userId: user.id,
+    lastModifiedById: user.id,
+    createdById: user.id,
+    title,
+    text,
+  });
+
+  if (ownerCollection.type === 'atlas') {
+    await ownerCollection.addDocumentToStructure(newDocument, index);
+  }
 
   ctx.body = {
-    data: await presentDocument(ctx, document, {
+    data: await presentDocument(ctx, newDocument, {
       includeCollection: true,
       includeCollaborators: true,
+      collection: ownerCollection,
     }),
   };
 });
@@ -159,32 +151,72 @@ router.post('documents.create', auth(), async ctx => {
 router.post('documents.update', auth(), async ctx => {
   const { id, title, text } = ctx.body;
   ctx.assertPresent(id, 'id is required');
-  ctx.assertPresent(title, 'title is required');
+  ctx.assertPresent(title || text, 'title or text is required');
-  ctx.assertPresent(text, 'text is required');
 
   const user = ctx.state.user;
   const document = await getDocumentForId(id);
 
-  if (!document || document.teamId !== user.teamId)
+  if (!document || document.teamId !== user.teamId) throw httpErrors.NotFound();
-    throw httpErrors.BadRequest();
 
   // Update document
-  document.title = title;
+  if (title) document.title = title;
-  document.text = text;
+  if (text) document.text = text;
   document.lastModifiedById = user.id;
   await document.save();
 
-  // Update
-  // TODO: Add locking
   const collection = await Collection.findById(document.atlasId);
   if (collection.type === 'atlas') {
-    await collection.updateNavigationTree();
+    await collection.updateDocument(document);
   }
 
   ctx.body = {
     data: await presentDocument(ctx, document, {
       includeCollection: true,
       includeCollaborators: true,
+      collection: collection,
+    }),
+  };
+});
+
+router.post('documents.move', auth(), async ctx => {
+  const { id, parentDocument, index } = ctx.body;
+  ctx.assertPresent(id, 'id is required');
+  if (parentDocument)
+    ctx.assertUuid(parentDocument, 'parentDocument must be an uuid');
+  if (index) ctx.assertPositiveInteger(index, 'index must be an integer (>=0)');
+
+  const user = ctx.state.user;
+  const document = await getDocumentForId(id);
+
+  if (!document || document.teamId !== user.teamId) throw httpErrors.NotFound();
+
+  // Set parent document
+  if (parentDocument) {
+    const parent = await getDocumentForId(parentDocument);
+    if (parent.atlasId !== document.atlasId)
+      throw httpErrors.BadRequest(
+        'Invalid parentDocument (must be same collection)'
+      );
+  }
+
+  if (parentDocument === id)
+    throw httpErrors.BadRequest('Infinite loop detected and prevented!');
+
+  // If no parent document is provided, set it as null (move to root level)
+  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);
+  }
+
+  ctx.body = {
+    data: await presentDocument(ctx, document, {
+      includeCollection: true,
+      includeCollaborators: true,
+      collection: collection,
     }),
   };
 });
@@ -200,17 +232,17 @@ router.post('documents.delete', auth(), async ctx => {
   if (!document || document.teamId !== user.teamId)
     throw httpErrors.BadRequest();
 
-  // TODO: Add locking
   if (collection.type === 'atlas') {
     // Don't allow deletion of root docs
-    if (!document.parentDocumentId) {
+    if (collection.documentStructure.length === 1) {
-      throw httpErrors.BadRequest("Unable to delete atlas's root document");
+      throw httpErrors.BadRequest(
+        "Unable to delete collection's only document"
+      );
     }
 
     // Delete all chilren
     try {
       await collection.deleteDocument(document);
-      await collection.save();
     } catch (e) {
       throw httpErrors.BadRequest('Error while deleting');
     }

server/api/middlewares/validation.js

@@ -1,3 +1,4 @@
+// @flow
 import apiError from '../../errors';
 import validator from 'validator';
 
@@ -9,18 +10,24 @@ export default function validation() {
       }
     };
 
-    ctx.assertEmail = function assertEmail(value, message) {
+    ctx.assertEmail = (value, message) => {
       if (!validator.isEmail(value)) {
         throw apiError(400, 'validation_error', message);
       }
     };
 
-    ctx.assertUuid = function assertUuid(value, message) {
+    ctx.assertUuid = (value, message) => {
       if (!validator.isUUID(value)) {
         throw apiError(400, 'validation_error', message);
       }
     };
 
+    ctx.assertPositiveInteger = (value, message) => {
+      if (!validator.isInt(value, { min: 0 })) {
+        throw apiError(400, 'validation_error', message);
+      }
+    };
+
     return next();
   };
 }

server/migrations/20170603185012-add-collection-documentStructure-migration.js

@@ -0,0 +1,14 @@
+module.exports = {
+  up: (queryInterface, Sequelize) => {
+    queryInterface.renameTable('atlases', 'collections');
+    queryInterface.addColumn('collections', 'documentStructure', {
+      type: Sequelize.JSONB,
+      allowNull: true,
+    });
+  },
+
+  down: (queryInterface, _Sequelize) => {
+    queryInterface.renameTable('collections', 'atlases');
+    queryInterface.removeColumn('atlases', 'documentStructure');
+  },
+};

server/models/Collection.js

@@ -1,3 +1,4 @@
+// @flow
 import slug from 'slug';
 import randomstring from 'randomstring';
 import { DataTypes, sequelize } from '../sequelize';
@@ -9,7 +10,7 @@ slug.defaults.mode = 'rfc3986';
 const allowedCollectionTypes = [['atlas', 'journal']];
 
 const Collection = sequelize.define(
-  'atlas',
+  'collection',
   {
     id: {
       type: DataTypes.UUID,
@@ -26,10 +27,11 @@ const Collection = sequelize.define(
     creatorId: DataTypes.UUID,
 
     /* type: atlas */
-    navigationTree: DataTypes.JSONB,
+    navigationTree: DataTypes.JSONB, // legacy
+    documentStructure: DataTypes.JSONB,
   },
   {
-    tableName: 'atlases',
+    tableName: 'collections',
     paranoid: true,
     hooks: {
       beforeValidate: collection => {
@@ -38,7 +40,7 @@ const Collection = sequelize.define(
       afterCreate: async collection => {
         if (collection.type !== 'atlas') return;
 
-        await Document.create({
+        const document = await Document.create({
           parentDocumentId: null,
           atlasId: collection.id,
           teamId: collection.teamId,
@@ -48,7 +50,7 @@ const Collection = sequelize.define(
           title: 'Introduction',
           text: '# Introduction\n\nLets get started...',
         });
-        await collection.buildStructure();
+        collection.documentStructure = [document.toJSON()];
         await collection.save();
       },
     },
@@ -58,156 +60,114 @@ const Collection = sequelize.define(
         // return `/${slugifiedName}-c${this.urlId}`;
         return `/collections/${this.id}`;
       },
-      async buildStructure() {
-        if (this.navigationTree) return this.navigationTree;
 
-        const getNodeForDocument = async document => {
+      async getDocumentsStructure() {
-          const children = await Document.findAll({
+        // Lazy fill this.documentStructure
-            where: {
+        if (!this.documentStructure) {
-              parentDocumentId: document.id,
+          this.documentStructure = this.navigationTree.children;
-              atlasId: this.id,
+
-            },
+          // Remove parent references from all root documents
+          await this.navigationTree.children.forEach(async ({ id }) => {
+            const document = await Document.findById(id);
+            document.parentDocumentId = null;
+            await document.save();
           });
 
-          const childNodes = [];
+          // Remove root document
-          await Promise.all(
+          const rootDocument = await Document.findById(this.navigationTree.id);
-            children.map(async child => {
+          await rootDocument.destroy();
-              return childNodes.push(await getNodeForDocument(child));
-            })
-          );
 
-          return {
+          await this.save();
-            title: document.title,
-            id: document.id,
-            url: document.getUrl(),
-            children: childNodes,
-          };
-        };
-
-        const rootDocument = await Document.findOne({
-          where: {
-            parentDocumentId: null,
-            atlasId: this.id,
-          },
-        });
-
-        this.navigationTree = await getNodeForDocument(rootDocument);
-        return this.navigationTree;
-      },
-      async updateNavigationTree(tree = this.navigationTree) {
-        const nodeIds = [];
-        nodeIds.push(tree.id);
-
-        const rootDocument = await Document.findOne({
-          where: {
-            id: tree.id,
-            atlasId: this.id,
-          },
-        });
-        if (!rootDocument) throw new Error();
-
-        const newTree = {
-          id: tree.id,
-          title: rootDocument.title,
-          url: rootDocument.getUrl(),
-          children: [],
-        };
-
-        const getIdsForChildren = async children => {
-          const childNodes = [];
-          for (const child of children) {
-            const childDocument = await Document.findOne({
-              where: {
-                id: child.id,
-                atlasId: this.id,
-              },
-            });
-            if (childDocument) {
-              childNodes.push({
-                id: childDocument.id,
-                title: childDocument.title,
-                url: childDocument.getUrl(),
-                children: await getIdsForChildren(child.children),
-              });
-              nodeIds.push(child.id);
-            }
-          }
-          return childNodes;
-        };
-        newTree.children = await getIdsForChildren(tree.children);
-
-        const documents = await Document.findAll({
-          attributes: ['id'],
-          where: {
-            atlasId: this.id,
-          },
-        });
-        const documentIds = documents.map(doc => doc.id);
-
-        if (!_.isEqual(nodeIds.sort(), documentIds.sort())) {
-          throw new Error('Invalid navigation tree');
         }
 
-        this.navigationTree = newTree;
+        return this.documentStructure;
-        await this.save();
-
-        return newTree;
       },
-      async addNodeToNavigationTree(document) {
-        const newNode = {
-          id: document.id,
-          title: document.title,
-          url: document.getUrl(),
-          children: [],
-        };
 
-        const insertNode = node => {
+      async addDocumentToStructure(document, index) {
-          if (document.parentDocumentId === node.id) {
+        if (!this.documentStructure) return;
-            node.children.push(newNode);
+
+        if (!document.parentDocumentId) {
+          this.documentStructure.splice(
+            index || this.documentStructure.length,
+            0,
+            document.toJSON()
+          );
+          // Sequelize doesn't seem to set the value with splice on JSONB field
+          this.documentStructure = this.documentStructure;
+        } else {
+          this.documentStructure = this.documentStructure.map(childDocument => {
+            if (document.parentDocumentId === childDocument.id) {
+              childDocument.children.splice(
+                index || childDocument.children.length,
+                0,
+                document.toJSON()
+              );
+            }
+            return childDocument;
+          });
+        }
+
+        await this.save();
+        return this;
+      },
+
+      async updateDocument(document) {
+        if (!this.documentStructure) return;
+
+        const updateChildren = (children, document) => {
+          const id = document.id;
+
+          if (_.find(children, { id })) {
+            children = children.map(childDocument => {
+              if (childDocument.id === id) {
+                childDocument = {
+                  ...document.toJSON(),
+                  children: childDocument.children,
+                };
+              }
+              return childDocument;
+            });
           } else {
-            node.children = node.children.map(childNode => {
+            children = children.map(childDocument => {
-              return insertNode(childNode);
+              return updateChildren(childDocument.children, id);
             });
           }
-
+          return children;
-          return node;
         };
 
-        this.navigationTree = insertNode(this.navigationTree);
+        this.documentStructure = updateChildren(
-        return this.navigationTree;
+          this.documentStructure,
+          document
+        );
+
+        await this.save();
+        return this;
       },
+
       async deleteDocument(document) {
-        const deleteNodeAndDocument = async (
+        if (!this.documentStructure) return;
-          node,
-          documentId,
-          shouldDelete = false
-        ) => {
-          // Delete node if id matches
-          if (document.id === node.id) shouldDelete = true;
 
-          const newChildren = [];
+        const deleteFromChildren = (children, id) => {
-          node.children.forEach(async childNode => {
+          if (_.find(children, { id })) {
-            const child = await deleteNodeAndDocument(
+            _.remove(children, { id });
-              childNode,
+          } else {
-              documentId,
+            children = children.map(childDocument => {
-              shouldDelete
+              return {
-            );
+                ...childDocument,
-            if (child) newChildren.push(child);
+                children: deleteFromChildren(childDocument.children, id),
-          });
+              };
-          node.children = newChildren;
+            });
-
-          if (shouldDelete) {
-            const doc = await Document.findById(node.id);
-            await doc.destroy();
           }
-
+          return children;
-          return shouldDelete ? null : node;
         };
 
-        this.navigationTree = await deleteNodeAndDocument(
+        this.documentStructure = deleteFromChildren(
-          this.navigationTree,
+          this.documentStructure,
           document.id
         );
+
+        await this.save();
+        return this;
       },
     },
   }

server/models/Document.js

@@ -1,3 +1,4 @@
+// @flow
 import slug from 'slug';
 import _ from 'lodash';
 import randomstring from 'randomstring';
@@ -98,6 +99,16 @@ const Document = sequelize.define(
         const slugifiedTitle = slugify(this.title);
         return `/d/${slugifiedTitle}-${this.urlId}`;
       },
+      toJSON() {
+        // Warning: only use for new documents as order of children is
+        // handled in the collection's documentStructure
+        return {
+          id: this.id,
+          title: this.title,
+          url: this.getUrl(),
+          children: [],
+        };
+      },
     },
   }
 );

server/presenters.js

@@ -40,12 +40,14 @@ export async function presentDocument(ctx, document, options) {
 
   if (options.includeCollection) {
     data.collection = await ctx.cache.get(document.atlasId, async () => {
-      const collection = await Collection.findOne({
+      const collection =
-        where: {
+        options.collection ||
-          id: document.atlasId,
+        (await Collection.findOne({
-        },
+          where: {
-      });
+            id: document.atlasId,
-      return await presentCollection(ctx, collection);
+          },
+        }));
+      return presentCollection(ctx, collection);
     });
   }
 
@@ -92,8 +94,9 @@ export async function presentCollection(
     updatedAt: collection.updatedAt,
   };
 
-  if (collection.type === 'atlas')
+  if (collection.type === 'atlas') {
-    data.navigationTree = collection.navigationTree;
+    data.documents = await collection.getDocumentsStructure();
+  }
 
   if (includeRecentDocuments) {
     const documents = await Document.findAll({