diff --git a/README.md b/README.md index cff4ae5a..631a849c 100644 --- a/README.md +++ b/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 diff --git a/app/components/PathToDocument.js b/app/components/PathToDocument.js index 8ac9d8c4..7f8c3cf5 100644 --- a/app/components/PathToDocument.js +++ b/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 { 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
; return ( + {collection && + (collection.private ? ( + + ) : ( + + ))} {result.path .map(doc => {doc.title}) .reduce((prev, curr) => [prev, , 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; diff --git a/app/models/Collection.js b/app/models/Collection.js index fecc37d1..96df4648 100644 --- a/app/models/Collection.js +++ b/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; diff --git a/app/models/Document.js b/app/models/Document.js index 74395839..554362c9 100644 --- a/app/models/Document.js +++ b/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 = () => { diff --git a/app/scenes/Document/components/DocumentMove.js b/app/scenes/Document/components/DocumentMove.js index 309a3d44..ebd73712 100644 --- a/app/scenes/Document/components/DocumentMove.js +++ b/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 { @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 { } 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 { const result = collections.getPathForDocument(document.id); if (result) { - return ; + return ( + + ); } } @@ -141,7 +140,7 @@ class DocumentMove extends React.Component { { key={result.id} result={result} document={document} + collection={collections.get(result.collectionId)} ref={ref => index === 0 && this.setFirstDocumentRef(ref) } diff --git a/app/stores/BaseStore.js b/app/stores/BaseStore.js index 95044842..dd217397 100644 --- a/app/stores/BaseStore.js +++ b/app/stores/BaseStore.js @@ -65,6 +65,10 @@ export default class BaseStore { return this.create(params); } + get(id: string): ?T { + return this.data.get(id); + } + @action async create(params: Object) { if (!this.actions.includes('create')) { diff --git a/app/stores/CollectionsStore.js b/app/stores/CollectionsStore.js index ebd047bf..0c893b13 100644 --- a/app/stores/CollectionsStore.js +++ b/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 { @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]); }); } diff --git a/app/stores/DocumentsStore.js b/app/stores/DocumentsStore.js index 59ca1867..bcddcafa 100644 --- a/app/stores/DocumentsStore.js +++ b/app/stores/DocumentsStore.js @@ -294,17 +294,20 @@ export default class DocumentsStore extends BaseStore { }; @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 diff --git a/server/api/documents.js b/server/api/documents.js index 40863e81..0643d984 100644 --- a/server/api/documents.js +++ b/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)) + ), + }, }; }); diff --git a/server/commands/documentMover.js b/server/commands/documentMover.js new file mode 100644 index 00000000..ebfa39ea --- /dev/null +++ b/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; +} diff --git a/server/commands/documentMover.test.js b/server/commands/documentMover.test.js new file mode 100644 index 00000000..0134c6e0 --- /dev/null +++ b/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); + }); +}); diff --git a/server/models/Collection.js b/server/models/Collection.js index 0cbef17f..01bc33e7 100644 --- a/server/models/Collection.js +++ b/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; }; diff --git a/server/models/Collection.test.js b/server/models/Collection.test.js index da81c480..c7121d2e 100644 --- a/server/models/Collection.test.js +++ b/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([]); - }); - }); }); diff --git a/server/models/Document.js b/server/models/Document.js index b043b797..8ee1e1cc 100644 --- a/server/models/Document.js +++ b/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); diff --git a/server/pages/developers/Api.js b/server/pages/developers/Api.js index 01c7e097..5717a436 100644 --- a/server/pages/developers/Api.js +++ b/server/pages/developers/Api.js @@ -352,8 +352,7 @@ export default function Pricing() { - 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. @@ -364,8 +363,13 @@ export default function Pricing() { required /> + diff --git a/server/policies/document.js b/server/policies/document.js index eb0e3b0b..12f324d0 100644 --- a/server/policies/document.js +++ b/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; }