From a93d034091fdf7dbbdcb6484f07c03edf5449ea9 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Wed, 12 May 2021 22:38:24 -0700 Subject: [PATCH] fix: Moving documents between collections does not update attachment permissions (#2136) * fix: Copy attachments when neccessary and moving between collections * test: regression --- server/commands/documentMover.js | 52 ++++++++++++++++++++++++--- server/commands/documentMover.test.js | 51 +++++++++++++++++++++++++- server/test/factories.js | 5 --- 3 files changed, 97 insertions(+), 11 deletions(-) diff --git a/server/commands/documentMover.js b/server/commands/documentMover.js index 66572ccfd..eeebfeb75 100644 --- a/server/commands/documentMover.js +++ b/server/commands/documentMover.js @@ -1,6 +1,42 @@ // @flow -import { Document, Collection, User, Event } from "../models"; +import { Document, Attachment, Collection, User, Event } from "../models"; import { sequelize } from "../sequelize"; +import parseAttachmentIds from "../utils/parseAttachmentIds"; + +async function copyAttachments(document: Document, options) { + let text = document.text; + const documentId = document.id; + + // find any image attachments that are in this documents text + const attachmentIds = parseAttachmentIds(text); + + for (const id of attachmentIds) { + const existing = await Attachment.findOne({ + where: { + teamId: document.teamId, + id, + }, + }); + + // if the image attachment was originally uploaded to another document + // (this can happen in various ways, copy/paste, or duplicate for example) + // then create a new attachment pointed to this doc and update the reference + // in the text so that it gets the moved documents permissions + if (existing && existing.documentId !== documentId) { + const { id, ...rest } = existing.dataValues; + const attachment = await Attachment.create( + { + ...rest, + documentId, + }, + options + ); + text = text.replace(existing.redirectUrl, attachment.redirectUrl); + } + } + + return text; +} export default async function documentMover({ user, @@ -63,7 +99,11 @@ export default async function documentMover({ // 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 }); + if (collectionChanged) { + await collection.save({ transaction }); + + document.text = await copyAttachments(document, { transaction }); + } // add to new collection (may be the same) document.collectionId = collectionId; @@ -82,8 +122,8 @@ export default async function documentMover({ 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. + // collectionId and move any attachments they may have too. This includes + // archived children, otherwise their collection would be wrong once restored. if (collectionChanged) { result.collections.push(newCollection); @@ -95,7 +135,9 @@ export default async function documentMover({ await Promise.all( childDocuments.map(async (child) => { await loopChildren(child.id); - await child.update({ collectionId }, { transaction }); + child.text = await copyAttachments(child, { transaction }); + child.collectionId = collectionId; + await child.save(); child.collection = newCollection; result.documents.push(child); }) diff --git a/server/commands/documentMover.test.js b/server/commands/documentMover.test.js index dbd0b1e1c..5bd8cc445 100644 --- a/server/commands/documentMover.test.js +++ b/server/commands/documentMover.test.js @@ -1,6 +1,13 @@ // @flow -import { buildDocument, buildCollection, buildUser } from "../test/factories"; +import { Attachment } from "../models"; +import { + buildDocument, + buildAttachment, + buildCollection, + buildUser, +} from "../test/factories"; import { flushdb, seed } from "../test/support"; +import parseAttachmentIds from "../utils/parseAttachmentIds"; import documentMover from "./documentMover"; beforeEach(() => flushdb()); @@ -110,4 +117,46 @@ describe("documentMover", () => { expect(response.documents[0].collection.id).toEqual(newCollection.id); expect(response.documents[1].collection.id).toEqual(newCollection.id); }); + + it("should move attachments in children to another collection", async () => { + const { document, user, collection } = await seed(); + const newCollection = await buildCollection({ + teamId: collection.teamId, + }); + const attachment = await buildAttachment({ + teamId: user.teamId, + userId: user.id, + }); + const newDocument = await buildDocument({ + parentDocumentId: document.id, + collectionId: collection.id, + teamId: collection.teamId, + userId: collection.createdById, + title: "Child document", + text: `content ![attachment](${attachment.redirectUrl})`, + }); + await collection.addDocumentToStructure(newDocument); + + await documentMover({ + user, + document, + collectionId: newCollection.id, + parentDocumentId: undefined, + index: 0, + ip, + }); + + // check document ids where updated + await newDocument.reload(); + expect(newDocument.collectionId).toBe(newCollection.id); + + // check new attachment was created pointint to same key + const attachmentIds = parseAttachmentIds(newDocument.text); + const newAttachment = await Attachment.findByPk(attachmentIds[0]); + expect(newAttachment.documentId).toBe(newDocument.id); + expect(newAttachment.key).toBe(attachment.key); + + await document.reload(); + expect(document.collectionId).toBe(newCollection.id); + }); }); diff --git a/server/test/factories.js b/server/test/factories.js index 3a0ac0314..f4318d20a 100644 --- a/server/test/factories.js +++ b/server/test/factories.js @@ -242,11 +242,6 @@ export async function buildAttachment(overrides: Object = {}) { overrides.userId = user.id; } - if (!overrides.collectionId) { - const collection = await buildCollection(overrides); - overrides.collectionId = collection.id; - } - if (!overrides.documentId) { const document = await buildDocument(overrides); overrides.documentId = document.id;