diff --git a/server/commands/documentPermanentDeleter.test.ts b/server/commands/documentPermanentDeleter.test.ts index 0a37d8480..001150a0b 100644 --- a/server/commands/documentPermanentDeleter.test.ts +++ b/server/commands/documentPermanentDeleter.test.ts @@ -1,9 +1,12 @@ import { subDays } from "date-fns"; import { Attachment, Document } from "@server/models"; +import DeleteAttachmentTask from "@server/queues/tasks/DeleteAttachmentTask"; import { buildAttachment, buildDocument } from "@server/test/factories"; import { setupTestDatabase } from "@server/test/support"; import documentPermanentDeleter from "./documentPermanentDeleter"; +jest.mock("@server/queues/tasks/DeleteAttachmentTask"); + setupTestDatabase(); describe("documentPermanentDeleter", () => { @@ -47,11 +50,15 @@ describe("documentPermanentDeleter", () => { teamId: document.teamId, documentId: document.id, }); + await buildAttachment({ + teamId: document.teamId, + documentId: document.id, + }); document.text = `![text](${attachment.redirectUrl})`; await document.save(); const countDeletedDoc = await documentPermanentDeleter([document]); expect(countDeletedDoc).toEqual(1); - expect(await Attachment.count()).toEqual(0); + expect(DeleteAttachmentTask.schedule).toHaveBeenCalledTimes(2); expect( await Document.unscoped().count({ paranoid: false, diff --git a/server/commands/documentPermanentDeleter.ts b/server/commands/documentPermanentDeleter.ts index f712a1a10..15475a047 100644 --- a/server/commands/documentPermanentDeleter.ts +++ b/server/commands/documentPermanentDeleter.ts @@ -1,7 +1,9 @@ +import { uniq } from "lodash"; import { QueryTypes } from "sequelize"; import { sequelize } from "@server/database/sequelize"; import Logger from "@server/logging/Logger"; import { Document, Attachment } from "@server/models"; +import DeleteAttachmentTask from "@server/queues/tasks/DeleteAttachmentTask"; import parseAttachmentIds from "@server/utils/parseAttachmentIds"; export default async function documentPermanentDeleter(documents: Document[]) { @@ -22,34 +24,53 @@ export default async function documentPermanentDeleter(documents: Document[]) { `; for (const document of documents) { - const attachmentIds = parseAttachmentIds(document.text); + // Find any attachments that are referenced in the text content + const attachmentIdsInText = parseAttachmentIds(document.text); - for (const attachmentId of attachmentIds) { - const [{ count }] = await sequelize.query(query, { - type: QueryTypes.SELECT, - replacements: { - documentId: document.id, + // Find any attachments that were originally uploaded to this document + const attachmentIdsForDocument = ( + await Attachment.findAll({ + attributes: ["id"], + where: { teamId: document.teamId, - query: attachmentId, + documentId: document.id, }, - }); + }) + ).map((attachment) => attachment.id); - if (parseInt(count) === 0) { - const attachment = await Attachment.findOne({ - where: { + const attachmentIds = uniq([ + ...attachmentIdsInText, + ...attachmentIdsForDocument, + ]); + + await Promise.all( + attachmentIds.map(async (attachmentId) => { + // Check if the attachment is referenced in any other documents – this + // is needed as it's easy to copy and paste content between documents. + // An uploaded attachment may end up referenced in multiple documents. + const [{ count }] = await sequelize.query<{ count: string }>(query, { + type: QueryTypes.SELECT, + replacements: { + documentId: document.id, teamId: document.teamId, - id: attachmentId, + query: attachmentId, }, }); - if (attachment) { - await attachment.destroy(); - Logger.info("commands", `Attachment ${attachmentId} deleted`); - } else { - Logger.info("commands", `Unknown attachment ${attachmentId} ignored`); + // If the attachment is not referenced in any other documents then + // delete it from the database and the storage provider. + if (parseInt(count) === 0) { + Logger.info( + "commands", + `Attachment ${attachmentId} scheduled for deletion` + ); + await DeleteAttachmentTask.schedule({ + attachmentId, + teamId: document.teamId, + }); } - } - } + }) + ); } return Document.scope("withDrafts").destroy({ diff --git a/server/models/Team.ts b/server/models/Team.ts index b2676bd87..b182d4cad 100644 --- a/server/models/Team.ts +++ b/server/models/Team.ts @@ -333,6 +333,7 @@ class Team extends ParanoidModel { if (attachment) { await DeleteAttachmentTask.schedule({ attachmentId: attachment.id, + teamId: model.id, }); } } diff --git a/server/models/User.ts b/server/models/User.ts index 0754d904e..654856b82 100644 --- a/server/models/User.ts +++ b/server/models/User.ts @@ -604,6 +604,7 @@ class User extends ParanoidModel { if (attachment) { await DeleteAttachmentTask.schedule({ attachmentId: attachment.id, + teamId: model.id, }); } } diff --git a/server/queues/tasks/DeleteAttachmentTask.ts b/server/queues/tasks/DeleteAttachmentTask.ts index e080f9f6d..d770d671a 100644 --- a/server/queues/tasks/DeleteAttachmentTask.ts +++ b/server/queues/tasks/DeleteAttachmentTask.ts @@ -2,12 +2,19 @@ import { Attachment } from "@server/models"; import BaseTask from "./BaseTask"; type Props = { + teamId: string; attachmentId: string; }; export default class DeleteAttachmentTask extends BaseTask { - public async perform({ attachmentId }: Props) { - const attachment = await Attachment.findByPk(attachmentId); + public async perform({ attachmentId, teamId }: Props) { + const attachment = await Attachment.findOne({ + where: { + teamId, + id: attachmentId, + }, + }); + if (!attachment) { return; }