Move document improvements (#927)

* Show all collections in UI

* Introduce command pattern

* Actually remove from previous collection

* Stash

* Fixes: Promises resolved outside of response lifecycle

* 💚

* 💚

* documentMover tests

* Transaction

* Perf. More in transactions

README.md

@@ -84,6 +84,7 @@ Backend is driven by [Koa](http://koajs.com/) (API, web server), [Sequelize](htt
 - `server/models` - Database models (Sequelize)
 - `server/pages` - Server-side rendered public pages (React)
 - `server/presenters` - API responses for database models
+- `server/commands` - Domain logic, currently being refactored from /models
 - `shared` - Code shared between frontend and backend applications
 
 ## Tests

app/components/PathToDocument.js

@@ -2,16 +2,18 @@
 import * as React from 'react';
 import { observer } from 'mobx-react';
 import styled from 'styled-components';
-import { GoToIcon } from 'outline-icons';
+import { GoToIcon, CollectionIcon, PrivateCollectionIcon } from 'outline-icons';
 import Flex from 'shared/components/Flex';
 
 import Document from 'models/Document';
+import Collection from 'models/Collection';
 import type { DocumentPath } from 'stores/CollectionsStore';
 
 type Props = {
   result: DocumentPath,
-  document?: Document,
-  onSuccess?: *,
+  document?: ?Document,
+  collection: ?Collection,
+  onSuccess?: () => void,
   ref?: *,
 };
 
@@ -23,27 +25,28 @@ class PathToDocument extends React.Component<Props> {
     if (!document) return;
 
     if (result.type === 'document') {
-      await document.move(result.id);
-    } else if (
-      result.type === 'collection' &&
-      result.id === document.collection.id
-    ) {
-      await document.move(null);
+      await document.move(result.collectionId, result.id);
     } else {
-      throw new Error('Not implemented yet');
+      await document.move(result.collectionId, null);
     }
 
     if (onSuccess) onSuccess();
   };
 
   render() {
-    const { result, document, ref } = this.props;
+    const { result, collection, document, ref } = this.props;
     const Component = document ? ResultWrapperLink : ResultWrapper;
 
     if (!result) return <div />;
 
     return (
       <Component ref={ref} onClick={this.handleClick} href="" selectable>
+        {collection &&
+          (collection.private ? (
+            <PrivateCollectionIcon color={collection.color} />
+          ) : (
+            <CollectionIcon color={collection.color} />
+          ))}
         {result.path
           .map(doc => <Title key={doc.id}>{doc.title}</Title>)
           .reduce((prev, curr) => [prev, <StyledGoToIcon />, curr])}
@@ -64,7 +67,9 @@ const Title = styled.span`
   text-overflow: ellipsis;
 `;
 
-const StyledGoToIcon = styled(GoToIcon)``;
+const StyledGoToIcon = styled(GoToIcon)`
+  opacity: 0.25;
+`;
 
 const ResultWrapper = styled.div`
   display: flex;

app/models/Collection.js

@@ -24,6 +24,11 @@ export default class Collection extends BaseModel {
   updatedAt: ?string;
   url: string;
 
+  @computed
+  get isPrivate(): boolean {
+    return this.private;
+  }
+
   @computed
   get isEmpty(): boolean {
     return this.documents.length === 0;

app/models/Document.js

@@ -234,8 +234,8 @@ export default class Document extends BaseModel {
     }
   };
 
-  move = (parentDocumentId: ?string) => {
-    return this.store.move(this, parentDocumentId);
+  move = (collectionId: string, parentDocumentId: ?string) => {
+    return this.store.move(this, collectionId, parentDocumentId);
   };
 
   duplicate = () => {

app/scenes/Document/components/DocumentMove.js

@@ -4,7 +4,7 @@ import ReactDOM from 'react-dom';
 import { observable, computed } from 'mobx';
 import { observer, inject } from 'mobx-react';
 import { Search } from 'js-search';
-import { first, last } from 'lodash';
+import { last } from 'lodash';
 import ArrowKeyNavigation from 'boundless-arrow-key-navigation';
 import styled from 'styled-components';
 
@@ -33,19 +33,14 @@ class DocumentMove extends React.Component<Props> {
 
   @computed
   get searchIndex() {
-    const { document, collections } = this.props;
+    const { collections } = this.props;
     const paths = collections.pathsToDocuments;
     const index = new Search('id');
     index.addIndex('title');
 
     // Build index
     const indexeableDocuments = [];
-    paths.forEach(path => {
-      // TMP: For now, exclude paths to other collections
-      if (first(path.path).id !== document.collection.id) return;
-
-      indexeableDocuments.push(path);
-    });
+    paths.forEach(path => indexeableDocuments.push(path));
     index.addDocuments(indexeableDocuments);
 
     return index;
@@ -63,23 +58,22 @@ class DocumentMove extends React.Component<Props> {
       } else {
         // Default results, root of the current collection
         results = [];
-        document.collection.documents.forEach(doc => {
-          const path = collections.getPathForDocument(doc.id);
-          if (doc && path) {
-            results.push(path);
+        collections.orderedData.forEach(collection => {
+          collection.documents.forEach(doc => {
+            const path = collections.getPathForDocument(doc.id);
+            if (doc && path) {
+              results.push(path);
+            }
+          });
+
+          const rootPath = collections.getPathForDocument(collection.id);
+          if (rootPath) {
+            results = [rootPath, ...results];
           }
         });
       }
     }
 
-    if (document && document.parentDocumentId) {
-      // Add root if document does have a parent document
-      const rootPath = collections.getPathForDocument(document.collection.id);
-      if (rootPath) {
-        results = [rootPath, ...results];
-      }
-    }
-
     // Exclude root from search results if document is already at the root
     if (!document.parentDocumentId) {
       results = results.filter(result => result.id !== document.collection.id);
@@ -119,7 +113,12 @@ class DocumentMove extends React.Component<Props> {
     const result = collections.getPathForDocument(document.id);
 
     if (result) {
-      return <PathToDocument result={result} />;
+      return (
+        <PathToDocument
+          result={result}
+          collection={collections.get(result.collectionId)}
+        />
+      );
     }
   }
 
@@ -141,7 +140,7 @@ class DocumentMove extends React.Component<Props> {
                 <Labeled label="Choose a new location">
                   <Input
                     type="text"
-                    placeholder="Filter by document name…"
+                    placeholder="Filter…"
                     onKeyDown={this.handleKeyDown}
                     onChange={this.handleFilter}
                     required
@@ -158,6 +157,7 @@ class DocumentMove extends React.Component<Props> {
                         key={result.id}
                         result={result}
                         document={document}
+                        collection={collections.get(result.collectionId)}
                         ref={ref =>
                           index === 0 && this.setFirstDocumentRef(ref)
                         }

app/stores/BaseStore.js

@@ -65,6 +65,10 @@ export default class BaseStore<T: BaseModel> {
     return this.create(params);
   }
 
+  get(id: string): ?T {
+    return this.data.get(id);
+  }
+
   @action
   async create(params: Object) {
     if (!this.actions.includes('create')) {

app/stores/CollectionsStore.js

@@ -10,9 +10,10 @@ import naturalSort from 'shared/utils/naturalSort';
 
 export type DocumentPathItem = {
   id: string,
+  collectionId: string,
   title: string,
   url: string,
-  type: 'document' | 'collection',
+  type: 'collection' | 'document',
 };
 
 export type DocumentPath = DocumentPathItem & {
@@ -52,20 +53,26 @@ export default class CollectionsStore extends BaseStore<Collection> {
   @computed
   get pathsToDocuments(): DocumentPath[] {
     let results = [];
-    const travelDocuments = (documentList, path) =>
+    const travelDocuments = (documentList, collectionId, path) =>
       documentList.forEach(document => {
         const { id, title, url } = document;
-        const node = { id, title, url, type: 'document' };
+        const node = { id, collectionId, title, url, type: 'document' };
         results.push(concat(path, node));
-        travelDocuments(document.children, concat(path, [node]));
+        travelDocuments(document.children, collectionId, concat(path, [node]));
       });
 
     if (this.isLoaded) {
       this.data.forEach(collection => {
         const { id, name, url } = collection;
-        const node = { id, title: name, url, type: 'collection' };
+        const node = {
+          id,
+          collectionId: id,
+          title: name,
+          url,
+          type: 'collection',
+        };
         results.push([node]);
-        travelDocuments(collection.documents, [node]);
+        travelDocuments(collection.documents, id, [node]);
       });
     }
 

app/stores/DocumentsStore.js

@@ -294,17 +294,20 @@ export default class DocumentsStore extends BaseStore<Document> {
   };
 
   @action
-  move = async (document: Document, parentDocumentId: ?string) => {
+  move = async (
+    document: Document,
+    collectionId: string,
+    parentDocumentId: ?string
+  ) => {
     const res = await client.post('/documents.move', {
       id: document.id,
-      parentDocument: parentDocumentId,
+      collectionId,
+      parentDocumentId,
     });
     invariant(res && res.data, 'Data not available');
 
-    const collection = this.getCollectionForDocument(document);
-    if (collection) collection.refresh();
-
-    return this.add(res.data);
+    res.data.documents.forEach(this.add);
+    res.data.collections.forEach(this.rootStore.collections.add);
   };
 
   @action

server/api/documents.js

@@ -3,7 +3,12 @@ import Router from 'koa-router';
 import Sequelize from 'sequelize';
 import auth from '../middlewares/authentication';
 import pagination from './middlewares/pagination';
-import { presentDocument, presentRevision } from '../presenters';
+import documentMover from '../commands/documentMover';
+import {
+  presentDocument,
+  presentCollection,
+  presentRevision,
+} from '../presenters';
 import { Document, Collection, Share, Star, View, Revision } from '../models';
 import { InvalidRequestError } from '../errors';
 import events from '../events';
@@ -537,39 +542,56 @@ router.post('documents.update', auth(), async ctx => {
 });
 
 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 a uuid');
-  if (index) ctx.assertPositiveInteger(index, 'index must be an integer (>=0)');
+  const { id, collectionId, parentDocumentId, index } = ctx.body;
+  ctx.assertUuid(id, 'id must be a uuid');
+  ctx.assertUuid(collectionId, 'collectionId must be a uuid');
+
+  if (parentDocumentId) {
+    ctx.assertUuid(parentDocumentId, 'parentDocumentId must be a uuid');
+  }
+  if (index) {
+    ctx.assertPositiveInteger(index, 'index must be a positive integer');
+  }
+  if (parentDocumentId === id) {
+    throw new InvalidRequestError(
+      'Infinite loop detected, cannot nest a document inside itself'
+    );
+  }
 
   const user = ctx.state.user;
   const document = await Document.findById(id);
-  authorize(user, 'update', document);
+  authorize(user, 'move', document);
 
-  const collection = document.collection;
-  if (collection.type !== 'atlas')
-    throw new InvalidRequestError('This document can’t be moved');
+  const collection = await Collection.findById(collectionId);
+  authorize(user, 'update', collection);
 
-  // Set parent document
-  if (parentDocument) {
-    const parent = await Document.findById(parentDocument);
+  if (collection.type !== 'atlas' && parentDocumentId) {
+    throw new InvalidRequestError(
+      'Document cannot be nested in this collection type'
+    );
+  }
+
+  if (parentDocumentId) {
+    const parent = await Document.findById(parentDocumentId);
     authorize(user, 'update', parent);
   }
 
-  if (parentDocument === id)
-    throw new InvalidRequestError('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();
-
-  await collection.moveDocument(document, index);
-  // Update collection
-  document.collection = collection;
+  const { documents, collections } = await documentMover({
+    document,
+    collectionId,
+    parentDocumentId,
+    index,
+  });
 
   ctx.body = {
-    data: await presentDocument(ctx, document),
+    data: {
+      documents: await Promise.all(
+        documents.map(document => presentDocument(ctx, document))
+      ),
+      collections: await Promise.all(
+        collections.map(collection => presentCollection(ctx, collection))
+      ),
+    },
   };
 });
 

server/commands/documentMover.js

@@ -0,0 +1,83 @@
+// @flow
+import { Document, Collection } from '../models';
+import { sequelize } from '../sequelize';
+
+export default async function documentMover({
+  document,
+  collectionId,
+  parentDocumentId,
+  index,
+}: {
+  document: Document,
+  collectionId: string,
+  parentDocumentId: string,
+  index?: number,
+}) {
+  let transaction;
+  const result = { collections: [], documents: [] };
+  const collectionChanged = collectionId !== document.collectionId;
+
+  try {
+    transaction = await sequelize.transaction();
+
+    // remove from original collection
+    const collection = await document.getCollection({ transaction });
+    const documentJson = await collection.removeDocumentInStructure(document, {
+      save: false,
+    });
+
+    // if the collection is the same then it will get saved below, this
+    // line prevents a pointless intermediate save from occurring.
+    if (collectionChanged) await collection.save({ transaction });
+
+    // add to new collection (may be the same)
+    document.collectionId = collectionId;
+    document.parentDocumentId = parentDocumentId;
+
+    const newCollection: Collection = collectionChanged
+      ? await Collection.findById(collectionId, { transaction })
+      : collection;
+    await newCollection.addDocumentToStructure(document, index, {
+      documentJson,
+    });
+    result.collections.push(collection);
+
+    // if collection does not remain the same loop through children and change their
+    // collectionId too. This includes archived children, otherwise their collection
+    // would be wrong once restored.
+    if (collectionChanged) {
+      result.collections.push(newCollection);
+
+      const loopChildren = async documentId => {
+        const childDocuments = await Document.findAll({
+          where: { parentDocumentId: documentId },
+        });
+
+        await Promise.all(
+          childDocuments.map(async child => {
+            await loopChildren(child.id);
+            await child.update({ collectionId }, { transaction });
+            child.collection = newCollection;
+            result.documents.push(child);
+          })
+        );
+      };
+
+      await loopChildren(document.id);
+    }
+
+    await document.save({ transaction });
+    document.collection = newCollection;
+    result.documents.push(document);
+
+    await transaction.commit();
+  } catch (err) {
+    if (transaction) {
+      await transaction.rollback();
+    }
+    throw err;
+  }
+
+  // we need to send all updated models back to the client
+  return result;
+}

server/commands/documentMover.test.js

@@ -0,0 +1,85 @@
+/* eslint-disable flowtype/require-valid-file-annotation */
+import documentMover from '../commands/documentMover';
+import { flushdb, seed } from '../test/support';
+import { buildDocument, buildCollection } from '../test/factories';
+
+beforeEach(flushdb);
+
+describe('documentMover', async () => {
+  it('should move within a collection', async () => {
+    const { document, collection } = await seed();
+
+    const response = await documentMover({
+      document,
+      collectionId: collection.id,
+    });
+
+    expect(response.collections.length).toEqual(1);
+    expect(response.documents.length).toEqual(1);
+  });
+
+  it('should move with children', async () => {
+    const { document, collection } = await seed();
+    const newDocument = await buildDocument({
+      parentDocumentId: document.id,
+      collectionId: collection.id,
+      teamId: collection.teamId,
+      userId: collection.creatorId,
+      title: 'Child document',
+      text: 'content',
+    });
+    await collection.addDocumentToStructure(newDocument);
+
+    const response = await documentMover({
+      document,
+      collectionId: collection.id,
+      parentDocumentId: undefined,
+      index: 0,
+    });
+
+    expect(response.collections[0].documentStructure[0].children[0].id).toBe(
+      newDocument.id
+    );
+    expect(response.collections.length).toEqual(1);
+    expect(response.documents.length).toEqual(1);
+  });
+
+  it('should move with children to another collection', async () => {
+    const { document, collection } = await seed();
+    const newCollection = await buildCollection({
+      teamId: collection.teamId,
+    });
+    const newDocument = await buildDocument({
+      parentDocumentId: document.id,
+      collectionId: collection.id,
+      teamId: collection.teamId,
+      userId: collection.creatorId,
+      title: 'Child document',
+      text: 'content',
+    });
+    await collection.addDocumentToStructure(newDocument);
+
+    const response = await documentMover({
+      document,
+      collectionId: newCollection.id,
+      parentDocumentId: undefined,
+      index: 0,
+    });
+
+    // check document ids where updated
+    await newDocument.reload();
+    expect(newDocument.collectionId).toBe(newCollection.id);
+
+    await document.reload();
+    expect(document.collectionId).toBe(newCollection.id);
+
+    // check collection structure updated
+    expect(response.collections[0].id).toBe(collection.id);
+    expect(response.collections[1].id).toBe(newCollection.id);
+    expect(response.collections[1].documentStructure[0].children[0].id).toBe(
+      newDocument.id
+    );
+    expect(response.collections.length).toEqual(2);
+    expect(response.documents.length).toEqual(2);
+  });
+});

server/models/Collection.js

@@ -156,8 +156,12 @@ Collection.prototype.addDocumentToStructure = async function(
 ) {
   if (!this.documentStructure) return;
 
+  let unlock;
+
   // documentStructure can only be updated by one request at a time
-  const unlock = await asyncLock(`collection-${this.id}`);
+  if (options.save !== false) {
+    unlock = await asyncLock(`collection-${this.id}`);
+  }
 
   // If moving existing document with children, use existing structure
   const documentJson = {
@@ -195,8 +199,11 @@ Collection.prototype.addDocumentToStructure = async function(
 
   // Sequelize doesn't seem to set the value with splice on JSONB field
   this.documentStructure = this.documentStructure;
-  await this.save(options);
-  unlock();
+
+  if (options.save !== false) {
+    await this.save(options);
+    if (unlock) unlock();
+  }
 
   return this;
 };
@@ -234,34 +241,21 @@ Collection.prototype.updateDocument = async function(
   return this;
 };
 
-/**
- * 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 documentJson = await this.removeDocumentInStructure(document);
-  await this.addDocumentToStructure(document, index, { documentJson });
-};
-
 Collection.prototype.deleteDocument = async function(document) {
-  await this.removeDocumentInStructure(document, { save: true });
+  await this.removeDocumentInStructure(document);
   await document.deleteWithChildren();
 };
 
 Collection.prototype.removeDocumentInStructure = async function(
   document,
-  options?: { save?: boolean }
+  options
 ) {
   if (!this.documentStructure) return;
   let returnValue;
   let unlock;
 
-  if (options && options.save) {
-    // documentStructure can only be updated by one request at the time
-    unlock = await asyncLock(`collection-${this.id}`);
-  }
+  // documentStructure can only be updated by one request at the time
+  unlock = await asyncLock(`collection-${this.id}`);
 
   const removeFromChildren = async (children, id) => {
     children = await Promise.all(
@@ -287,10 +281,8 @@ Collection.prototype.removeDocumentInStructure = async function(
     document.id
   );
 
-  if (options && options.save) {
-    await this.save(options);
-    if (unlock) await unlock();
-  }
+  await this.save(options);
+  if (unlock) await unlock();
 
   return returnValue;
 };

server/models/Collection.test.js

@@ -145,37 +145,7 @@ describe('#updateDocument', () => {
   });
 });
 
-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();
-    const newDocument = await Document.create({
-      parentDocumentId: document.id,
-      collectionId: 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');
@@ -260,18 +230,4 @@ describe('#removeDocument', () => {
     });
     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.removeDocumentInStructure(document);
-      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([]);
-    });
-  });
 });

server/models/Document.js

@@ -362,7 +362,7 @@ Document.prototype.publish = async function() {
 Document.prototype.archive = async function(userId) {
   // archive any children and remove from the document structure
   const collection = await this.getCollection();
-  await collection.removeDocumentInStructure(this, { save: true });
+  await collection.removeDocumentInStructure(this);
   this.collection = collection;
 
   await this.archiveWithChildren(userId);

server/pages/developers/Api.js

@@ -352,8 +352,7 @@ export default function Pricing() {
 
           <Method method="documents.move" label="Move document in a collection">
             <Description>
-              Move a document into a new location inside the collection. This is
-              easily done by defining the parent document ID. If no parent
+              Move a document to a new location or collection. If no parent
               document is provided, the document will be moved to the collection
               root.
             </Description>
@@ -364,8 +363,13 @@ export default function Pricing() {
                 required
               />
               <Argument
-                id="parentDocument"
-                description="ID of the new parent document (if any)"
+                id="collectionId"
+                description="ID of the collection"
+                required
+              />
+              <Argument
+                id="parentDocumentId"
+                description="ID of the new parent document"
               />
             </Arguments>
           </Method>

server/policies/document.js

@@ -14,7 +14,7 @@ allow(User, ['read', 'delete'], Document, (user, document) => {
   return user.teamId === document.teamId;
 });
 
-allow(User, ['update', 'share'], Document, (user, document) => {
+allow(User, ['update', 'move', 'share'], Document, (user, document) => {
   if (document.collection) {
     if (cannot(user, 'read', document.collection)) return false;
   }