From 86062f396dc441d4c977c173312635d07341c9d3 Mon Sep 17 00:00:00 2001 From: Apoorv Mishra Date: Mon, 24 Apr 2023 00:50:44 +0530 Subject: [PATCH] Deleting a collection should detach associated drafts from it (#5082) Co-authored-by: Tom Moor --- app/components/CollectionDeleteDialog.tsx | 2 +- app/components/DocumentBreadcrumb.tsx | 4 +- app/components/DocumentCard.tsx | 4 +- app/components/DocumentMeta.tsx | 4 +- .../Sidebar/components/StarredLink.tsx | 4 +- app/components/WebsocketProvider.tsx | 10 +- app/hooks/useImportDocument.ts | 2 +- app/hooks/usePolicy.ts | 2 +- app/menus/DocumentMenu.tsx | 4 +- app/menus/NewChildDocumentMenu.tsx | 4 +- app/models/Document.ts | 4 +- app/scenes/Document/components/References.tsx | 4 +- app/scenes/DocumentDelete.tsx | 4 +- app/stores/DocumentsStore.ts | 10 +- app/stores/SharesStore.ts | 4 +- app/stores/UiStore.ts | 2 +- app/types.ts | 2 +- app/utils/routeHelpers.ts | 2 +- server/commands/documentCreator.ts | 2 +- server/commands/documentLoader.ts | 8 +- server/commands/documentMover.test.ts | 23 +++++ server/commands/documentMover.ts | 97 ++++++++++++------- server/commands/documentUpdater.ts | 2 +- server/commands/pinCreator.ts | 2 +- server/models/Document.ts | 55 +++++++---- server/models/Team.ts | 4 +- server/models/helpers/NotificationHelper.ts | 3 +- server/models/helpers/SearchHelper.ts | 2 +- .../processors/BacklinksProcessor.test.ts | 18 ++-- .../queues/processors/CollectionsProcessor.ts | 23 +++++ .../processors/RevisionsProcessor.test.ts | 4 +- .../DetachDraftsFromCollectionTask.test.ts | 33 +++++++ .../tasks/DetachDraftsFromCollectionTask.ts | 48 +++++++++ ...DocumentPublishedNotificationsTask.test.ts | 8 +- .../RevisionCreatedNotificationsTask.test.ts | 24 ++--- server/routes/api/documents/documents.test.ts | 29 ++++++ server/routes/api/documents/documents.ts | 12 ++- server/test/support.ts | 4 +- shared/i18n/locales/en_US/translation.json | 2 +- 39 files changed, 363 insertions(+), 112 deletions(-) create mode 100644 server/queues/processors/CollectionsProcessor.ts create mode 100644 server/queues/tasks/DetachDraftsFromCollectionTask.test.ts create mode 100644 server/queues/tasks/DetachDraftsFromCollectionTask.ts diff --git a/app/components/CollectionDeleteDialog.tsx b/app/components/CollectionDeleteDialog.tsx index 27a82ec63..f51da8ed2 100644 --- a/app/components/CollectionDeleteDialog.tsx +++ b/app/components/CollectionDeleteDialog.tsx @@ -39,7 +39,7 @@ function CollectionDeleteDialog({ collection, onSubmit }: Props) { <> = ({ const { collections } = useStores(); const { t } = useTranslation(); const category = useCategory(document); - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; let collectionNode: MenuInternalLink | undefined; diff --git a/app/components/DocumentCard.tsx b/app/components/DocumentCard.tsx index 81a74aa22..4e05e057c 100644 --- a/app/components/DocumentCard.tsx +++ b/app/components/DocumentCard.tsx @@ -36,7 +36,9 @@ function DocumentCard(props: Props) { const { collections } = useStores(); const theme = useTheme(); const { document, pin, canUpdatePin, isDraggable } = props; - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; const { attributes, listeners, diff --git a/app/components/DocumentMeta.tsx b/app/components/DocumentMeta.tsx index 92e8c28e1..787d629d4 100644 --- a/app/components/DocumentMeta.tsx +++ b/app/components/DocumentMeta.tsx @@ -57,7 +57,9 @@ const DocumentMeta: React.FC = ({ return null; } - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; const lastUpdatedByCurrentUser = user.id === updatedBy.id; const userName = updatedBy.name; let content; diff --git a/app/components/Sidebar/components/StarredLink.tsx b/app/components/Sidebar/components/StarredLink.tsx index 30560b34d..250f15592 100644 --- a/app/components/Sidebar/components/StarredLink.tsx +++ b/app/components/Sidebar/components/StarredLink.tsx @@ -152,7 +152,9 @@ function StarredLink({ star }: Props) { return null; } - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; const childDocuments = collection ? collection.getDocumentChildren(documentId) : []; diff --git a/app/components/WebsocketProvider.tsx b/app/components/WebsocketProvider.tsx index a62274b02..00057e587 100644 --- a/app/components/WebsocketProvider.tsx +++ b/app/components/WebsocketProvider.tsx @@ -175,7 +175,7 @@ class WebsocketProvider extends React.Component { id: document.collectionId, }); - if (!existing) { + if (!existing && document.collectionId) { event.collectionIds.push({ id: document.collectionId, }); @@ -298,10 +298,14 @@ class WebsocketProvider extends React.Component { action((event: WebsocketEntityDeletedEvent) => { const collectionId = event.modelId; const deletedAt = new Date().toISOString(); - const deletedDocuments = documents.inCollection(collectionId); deletedDocuments.forEach((doc) => { - doc.deletedAt = deletedAt; + if (!doc.publishedAt) { + // draft is to be detached from collection, not deleted + doc.collectionId = null; + } else { + doc.deletedAt = deletedAt; + } policies.remove(doc.id); }); documents.removeCollectionDocuments(collectionId); diff --git a/app/hooks/useImportDocument.ts b/app/hooks/useImportDocument.ts index 9e668e77d..ddd1d4d8e 100644 --- a/app/hooks/useImportDocument.ts +++ b/app/hooks/useImportDocument.ts @@ -8,7 +8,7 @@ import useToasts from "~/hooks/useToasts"; let importingLock = false; export default function useImportDocument( - collectionId?: string, + collectionId?: string | null, documentId?: string ): { handleFiles: (files: File[]) => Promise; diff --git a/app/hooks/usePolicy.ts b/app/hooks/usePolicy.ts index 5e455ab57..996e4ea2c 100644 --- a/app/hooks/usePolicy.ts +++ b/app/hooks/usePolicy.ts @@ -9,7 +9,7 @@ import useStores from "./useStores"; * @param entity The model or model id * @returns The policy for the model */ -export default function usePolicy(entity: string | BaseModel | undefined) { +export default function usePolicy(entity?: string | BaseModel | null) { const { policies } = useStores(); const triggered = React.useRef(false); const entityId = entity diff --git a/app/menus/DocumentMenu.tsx b/app/menus/DocumentMenu.tsx index 2af249b69..67d8d86da 100644 --- a/app/menus/DocumentMenu.tsx +++ b/app/menus/DocumentMenu.tsx @@ -123,7 +123,9 @@ function DocumentMenu({ [showToast, t, document] ); - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; const can = usePolicy(document); const restoreItems = React.useMemo( () => [ diff --git a/app/menus/NewChildDocumentMenu.tsx b/app/menus/NewChildDocumentMenu.tsx index 7f7378794..2bbb7b046 100644 --- a/app/menus/NewChildDocumentMenu.tsx +++ b/app/menus/NewChildDocumentMenu.tsx @@ -19,7 +19,9 @@ function NewChildDocumentMenu({ document, label }: Props) { }); const { collections } = useStores(); const { t } = useTranslation(); - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; const collectionName = collection ? collection.name : t("collection"); return ( diff --git a/app/models/Document.ts b/app/models/Document.ts index d4b8c68c2..3745ec01b 100644 --- a/app/models/Document.ts +++ b/app/models/Document.ts @@ -46,7 +46,7 @@ export default class Document extends ParanoidModel { @Field @observable - collectionId: string; + collectionId?: string | null; @Field @observable @@ -261,7 +261,7 @@ export default class Document extends ParanoidModel { }; @action - pin = (collectionId?: string) => + pin = (collectionId?: string | null) => this.store.rootStore.pins.create({ documentId: this.id, ...(collectionId ? { collectionId } : {}), diff --git a/app/scenes/Document/components/References.tsx b/app/scenes/Document/components/References.tsx index 38c913054..6c7fee58a 100644 --- a/app/scenes/Document/components/References.tsx +++ b/app/scenes/Document/components/References.tsx @@ -23,7 +23,9 @@ function References({ document }: Props) { }, [documents, document.id]); const backlinks = documents.getBacklinkedDocuments(document.id); - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; const children = collection ? collection.getDocumentChildren(document.id) : []; diff --git a/app/scenes/DocumentDelete.tsx b/app/scenes/DocumentDelete.tsx index 8fc8408db..1a74b678f 100644 --- a/app/scenes/DocumentDelete.tsx +++ b/app/scenes/DocumentDelete.tsx @@ -23,7 +23,9 @@ function DocumentDelete({ document, onSubmit }: Props) { const [isArchiving, setArchiving] = React.useState(false); const { showToast } = useToasts(); const canArchive = !document.isDraft && !document.isArchived; - const collection = collections.get(document.collectionId); + const collection = document.collectionId + ? collections.get(document.collectionId) + : undefined; const nestedDocumentsCount = collection ? collection.getDocumentChildren(document.id).length : 0; diff --git a/app/stores/DocumentsStore.ts b/app/stores/DocumentsStore.ts index c6b68fa2a..cab0281c5 100644 --- a/app/stores/DocumentsStore.ts +++ b/app/stores/DocumentsStore.ts @@ -645,7 +645,11 @@ export default class DocumentsStore extends BaseStore { @action removeCollectionDocuments(collectionId: string) { - const documents = this.inCollection(collectionId); + // drafts are to be detached from collection rather than deleted, hence excluded here + const documents = filter( + this.inCollection(collectionId), + (d) => !!d.publishedAt + ); const documentIds = documents.map((doc) => doc.id); documentIds.forEach((id) => this.remove(id)); } @@ -795,6 +799,8 @@ export default class DocumentsStore extends BaseStore { find(this.orderedData, (doc) => url.endsWith(doc.urlId)); getCollectionForDocument(document: Document) { - return this.rootStore.collections.data.get(document.collectionId); + return document.collectionId + ? this.rootStore.collections.get(document.collectionId) + : undefined; } } diff --git a/app/stores/SharesStore.ts b/app/stores/SharesStore.ts index cf09314c6..c244ed283 100644 --- a/app/stores/SharesStore.ts +++ b/app/stores/SharesStore.ts @@ -78,7 +78,9 @@ export default class SharesStore extends BaseStore { return; } - const collection = this.rootStore.collections.get(document.collectionId); + const collection = document.collectionId + ? this.rootStore.collections.get(document.collectionId) + : undefined; if (!collection) { return; } diff --git a/app/stores/UiStore.ts b/app/stores/UiStore.ts index 67c4667e1..a9f5b966f 100644 --- a/app/stores/UiStore.ts +++ b/app/stores/UiStore.ts @@ -37,7 +37,7 @@ class UiStore { activeDocumentId: string | undefined; @observable - activeCollectionId: string | undefined; + activeCollectionId?: string | null; @observable observingUserId: string | undefined; diff --git a/app/types.ts b/app/types.ts index 01261a47b..7d4c489da 100644 --- a/app/types.ts +++ b/app/types.ts @@ -76,7 +76,7 @@ export type ActionContext = { isCommandBar: boolean; isButton: boolean; inStarredSection?: boolean; - activeCollectionId: string | undefined; + activeCollectionId?: string | null; activeDocumentId: string | undefined; currentUserId: string | undefined; currentTeamId: string | undefined; diff --git a/app/utils/routeHelpers.ts b/app/utils/routeHelpers.ts index bf275e696..3cb5c1dc8 100644 --- a/app/utils/routeHelpers.ts +++ b/app/utils/routeHelpers.ts @@ -101,7 +101,7 @@ export function updateDocumentPath(oldUrl: string, document: Document): string { } export function newDocumentPath( - collectionId?: string, + collectionId?: string | null, params: { parentDocumentId?: string; templateId?: string; diff --git a/server/commands/documentCreator.ts b/server/commands/documentCreator.ts index 5bdd2ae9b..323dc1e2f 100644 --- a/server/commands/documentCreator.ts +++ b/server/commands/documentCreator.ts @@ -109,7 +109,7 @@ export default async function documentCreator({ ); if (publish) { - await document.publish(user.id, { transaction }); + await document.publish(user.id, collectionId!, { transaction }); await Event.create( { name: "documents.publish", diff --git a/server/commands/documentLoader.ts b/server/commands/documentLoader.ts index 617c7f7a5..024d637dc 100644 --- a/server/commands/documentLoader.ts +++ b/server/commands/documentLoader.ts @@ -126,7 +126,9 @@ export default async function loadDocument({ if (canReadDocument) { // Cannot use document.collection here as it does not include the // documentStructure by default through the relationship. - collection = await Collection.findByPk(document.collectionId); + if (document.collectionId) { + collection = await Collection.findByPk(document.collectionId); + } if (!collection) { throw NotFoundError("Collection could not be found for document"); } @@ -146,7 +148,9 @@ export default async function loadDocument({ } // It is possible to disable sharing at the collection so we must check - collection = await Collection.findByPk(document.collectionId); + if (document.collectionId) { + collection = await Collection.findByPk(document.collectionId); + } invariant(collection, "collection not found"); if (!collection.sharing) { diff --git a/server/commands/documentMover.test.ts b/server/commands/documentMover.test.ts index 8627aaded..2395a1c7b 100644 --- a/server/commands/documentMover.test.ts +++ b/server/commands/documentMover.test.ts @@ -153,4 +153,27 @@ describe("documentMover", () => { expect(response.documents[0].collection?.id).toEqual(newCollection.id); expect(response.documents[0].updatedBy.id).toEqual(user.id); }); + + it("should detach document from collection and move it to drafts", async () => { + const { document, user, collection } = await seed(); + + const response = await sequelize.transaction(async (transaction) => + documentMover({ + user, + document, + collectionId: null, + index: 0, + ip, + transaction, + }) + ); + + expect(response.collections[0].id).toBe(collection.id); + expect(response.collections.length).toEqual(1); + expect(response.documents.length).toEqual(1); + + expect(response.documents[0].collection).toBeNull(); + expect(response.documents[0].updatedBy.id).toEqual(user.id); + expect(response.documents[0].publishedAt).toBeNull(); + }); }); diff --git a/server/commands/documentMover.ts b/server/commands/documentMover.ts index dc6502d33..a689fee0d 100644 --- a/server/commands/documentMover.ts +++ b/server/commands/documentMover.ts @@ -5,12 +5,19 @@ import { User, Document, Collection, Pin, Event } from "@server/models"; import pinDestroyer from "./pinDestroyer"; type Props = { + /** User attempting to move the document */ user: User; + /** Document which is being moved */ document: Document; - collectionId: string; + /** Destination collection to which the document is moved */ + collectionId: string | null; + /** ID of parent under which the document is moved */ parentDocumentId?: string | null; + /** Position of moved document within document structure */ index?: number; + /** The IP address of the user moving the document */ ip: string; + /** The database transaction to run within */ transaction?: Transaction; }; @@ -23,7 +30,7 @@ type Result = { async function documentMover({ user, document, - collectionId, + collectionId = null, parentDocumentId = null, // convert undefined to null so parentId comparison treats them as equal index, @@ -43,6 +50,7 @@ async function documentMover({ } if (document.template) { + invariant(collectionId, "collectionId should exist"); document.collectionId = collectionId; document.parentDocumentId = null; document.lastModifiedById = user.id; @@ -51,20 +59,23 @@ async function documentMover({ result.documents.push(document); } else { // Load the current and the next collection upfront and lock them - const collection = await Collection.findByPk(document.collectionId, { + const collection = await Collection.findByPk(document.collectionId!, { transaction, lock: Transaction.LOCK.UPDATE, paranoid: false, }); - let newCollection = collectionChanged - ? await Collection.findByPk(collectionId, { + let newCollection = collection; + if (collectionChanged) { + if (collectionId) { + newCollection = await Collection.findByPk(collectionId, { transaction, lock: Transaction.LOCK.UPDATE, - }) - : collection; - - invariant(newCollection, "collection should exist"); + }); + } else { + newCollection = null; + } + } if (document.publishedAt) { // Remove the document from the current collection @@ -99,11 +110,13 @@ async function documentMover({ document.lastModifiedById = user.id; document.updatedBy = user; - // Add the document and it's tree to the new collection - await newCollection.addDocumentToStructure(document, toIndex, { - documentJson, - transaction, - }); + if (newCollection) { + // Add the document and it's tree to the new collection + await newCollection.addDocumentToStructure(document, toIndex, { + documentJson, + transaction, + }); + } } else { document.collectionId = collectionId; document.parentDocumentId = parentDocumentId; @@ -118,30 +131,48 @@ async function documentMover({ // If the collection has changed then we also need to update the properties // on all of the documents children to reflect the new collectionId if (collectionChanged) { - // Reload the collection to get relationship data - newCollection = await Collection.scope({ - method: ["withMembership", user.id], - }).findByPk(collectionId, { - transaction, - }); - invariant(newCollection, "collection should exist"); - - result.collections.push(newCollection); - // Efficiently find the ID's of all the documents that are children of // the moved document and update in one query const childDocumentIds = await document.getChildDocumentIds(); - await Document.update( - { - collectionId: newCollection.id, - }, - { + + if (collectionId) { + // Reload the collection to get relationship data + newCollection = await Collection.scope({ + method: ["withMembership", user.id], + }).findByPk(collectionId, { transaction, - where: { - id: childDocumentIds, + }); + invariant(newCollection, "collection should exist"); + result.collections.push(newCollection); + + await Document.update( + { + collectionId: newCollection.id, }, - } - ); + { + transaction, + where: { + id: childDocumentIds, + }, + } + ); + } else { + // document will be moved to drafts + document.publishedAt = null; + + // point children's parent to moved document's parent + await Document.update( + { + parentDocumentId: document.parentDocumentId, + }, + { + transaction, + where: { + id: childDocumentIds, + }, + } + ); + } // We must reload from the database to get the relationship data const documents = await Document.findAll({ diff --git a/server/commands/documentUpdater.ts b/server/commands/documentUpdater.ts index 2b12abd1f..d13ac7e60 100644 --- a/server/commands/documentUpdater.ts +++ b/server/commands/documentUpdater.ts @@ -74,7 +74,7 @@ export default async function documentUpdater({ if (!document.collectionId) { document.collectionId = collectionId as string; } - await document.publish(user.id, { transaction }); + await document.publish(user.id, collectionId!, { transaction }); await Event.create( { diff --git a/server/commands/pinCreator.ts b/server/commands/pinCreator.ts index 12b957ea1..d7f684592 100644 --- a/server/commands/pinCreator.ts +++ b/server/commands/pinCreator.ts @@ -11,7 +11,7 @@ type Props = { /** The document to pin */ documentId: string; /** The collection to pin the document in. If no collection is provided then it will be pinned to home */ - collectionId?: string | undefined; + collectionId?: string | null; /** The index to pin the document at. If no index is provided then it will be pinned to the end of the collection */ index?: string; /** The IP address of the user creating the pin */ diff --git a/server/models/Document.ts b/server/models/Document.ts index 20695496f..29aa9d5ad 100644 --- a/server/models/Document.ts +++ b/server/models/Document.ts @@ -267,7 +267,8 @@ class Document extends ParanoidModel { model.archivedAt || model.template || !model.publishedAt || - !model.changed("title") + !model.changed("title") || + !model.collectionId ) { return; } @@ -286,12 +287,17 @@ class Document extends ParanoidModel { @AfterCreate static async addDocumentToCollectionStructure(model: Document) { - if (model.archivedAt || model.template || !model.publishedAt) { + if ( + model.archivedAt || + model.template || + !model.publishedAt || + !model.collectionId + ) { return; } return this.sequelize!.transaction(async (transaction: Transaction) => { - const collection = await Collection.findByPk(model.collectionId, { + const collection = await Collection.findByPk(model.collectionId!, { transaction, lock: transaction.LOCK.UPDATE, }); @@ -399,7 +405,7 @@ class Document extends ParanoidModel { @ForeignKey(() => Collection) @Column(DataType.UUID) - collectionId: string; + collectionId?: string | null; @HasMany(() => Revision) revisions: Revision[]; @@ -555,13 +561,21 @@ class Document extends ParanoidModel { return this.save(options); }; - publish = async (userId: string, { transaction }: SaveOptions) => { + publish = async ( + userId: string, + collectionId: string, + { transaction }: SaveOptions + ) => { // If the document is already published then calling publish should act like // a regular save if (this.publishedAt) { return this.save({ transaction }); } + if (!this.collectionId) { + this.collectionId = collectionId; + } + if (!this.template) { const collection = await Collection.findByPk(this.collectionId, { transaction, @@ -587,10 +601,12 @@ class Document extends ParanoidModel { } await this.sequelize.transaction(async (transaction: Transaction) => { - const collection = await Collection.findByPk(this.collectionId, { - transaction, - lock: transaction.LOCK.UPDATE, - }); + const collection = this.collectionId + ? await Collection.findByPk(this.collectionId, { + transaction, + lock: transaction.LOCK.UPDATE, + }) + : undefined; if (collection) { await collection.removeDocumentInStructure(this, { transaction }); @@ -610,10 +626,12 @@ class Document extends ParanoidModel { // to the archived area, where it can be subsequently restored. archive = async (userId: string) => { await this.sequelize.transaction(async (transaction: Transaction) => { - const collection = await Collection.findByPk(this.collectionId, { - transaction, - lock: transaction.LOCK.UPDATE, - }); + const collection = this.collectionId + ? await Collection.findByPk(this.collectionId, { + transaction, + lock: transaction.LOCK.UPDATE, + }) + : undefined; if (collection) { await collection.removeDocumentInStructure(this, { transaction }); @@ -628,10 +646,12 @@ class Document extends ParanoidModel { // Restore an archived document back to being visible to the team unarchive = async (userId: string) => { await this.sequelize.transaction(async (transaction: Transaction) => { - const collection = await Collection.findByPk(this.collectionId, { - transaction, - lock: transaction.LOCK.UPDATE, - }); + const collection = this.collectionId + ? await Collection.findByPk(this.collectionId, { + transaction, + lock: transaction.LOCK.UPDATE, + }) + : undefined; // check to see if the documents parent hasn't been archived also // If it has then restore the document to the collection root. @@ -675,6 +695,7 @@ class Document extends ParanoidModel { const collection = await Collection.findByPk(this.collectionId, { transaction, lock: transaction.LOCK.UPDATE, + paranoid: false, }); await collection?.deleteDocument(this, { transaction }); } else { diff --git a/server/models/Team.ts b/server/models/Team.ts index 8ebaa9e33..50fb254a9 100644 --- a/server/models/Team.ts +++ b/server/models/Team.ts @@ -252,7 +252,9 @@ class Team extends ParanoidModel { }, { transaction } ); - await document.publish(collection.createdById, { transaction }); + await document.publish(collection.createdById, collection.id, { + transaction, + }); } }); }; diff --git a/server/models/helpers/NotificationHelper.ts b/server/models/helpers/NotificationHelper.ts index 5a82b2227..88475fe04 100644 --- a/server/models/helpers/NotificationHelper.ts +++ b/server/models/helpers/NotificationHelper.ts @@ -164,11 +164,12 @@ export default class NotificationHelper { const collectionIds = await recipient.collectionIds(); // Check the recipient has access to the collection this document is in. Just - // because they are subscribed doesn't meant they "still have access to read + // because they are subscribed doesn't mean they still have access to read // the document. if ( recipient.email && !recipient.isSuspended && + document.collectionId && collectionIds.includes(document.collectionId) ) { filtered.push(recipient); diff --git a/server/models/helpers/SearchHelper.ts b/server/models/helpers/SearchHelper.ts index 18c62c312..1ad8415fe 100644 --- a/server/models/helpers/SearchHelper.ts +++ b/server/models/helpers/SearchHelper.ts @@ -31,7 +31,7 @@ type SearchOptions = { /** The query offset for pagination */ offset?: number; /** Limit results to a collection. Authorization is presumed to have been done before passing to this helper. */ - collectionId?: string; + collectionId?: string | null; /** Limit results to a shared document. */ share?: Share; /** Limit results to a date range. */ diff --git a/server/queues/processors/BacklinksProcessor.test.ts b/server/queues/processors/BacklinksProcessor.test.ts index bde73a555..24fd18da7 100644 --- a/server/queues/processors/BacklinksProcessor.test.ts +++ b/server/queues/processors/BacklinksProcessor.test.ts @@ -18,7 +18,7 @@ describe("documents.publish", () => { await processor.perform({ name: "documents.publish", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { title: document.title }, @@ -46,7 +46,7 @@ describe("documents.publish", () => { await processor.perform({ name: "documents.publish", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { title: document.title }, @@ -72,7 +72,7 @@ describe("documents.update", () => { await processor.perform({ name: "documents.update", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, createdAt: new Date().toISOString(), @@ -100,7 +100,7 @@ describe("documents.update", () => { await processor.perform({ name: "documents.update", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, createdAt: new Date().toISOString(), @@ -125,7 +125,7 @@ describe("documents.update", () => { await processor.perform({ name: "documents.update", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, createdAt: new Date().toISOString(), @@ -153,7 +153,7 @@ describe("documents.update", () => { await processor.perform({ name: "documents.publish", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { title: document.title }, @@ -167,7 +167,7 @@ describe("documents.update", () => { await processor.perform({ name: "documents.update", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, createdAt: new Date().toISOString(), @@ -195,7 +195,7 @@ describe("documents.delete", () => { await processor.perform({ name: "documents.update", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, createdAt: new Date().toISOString(), @@ -206,7 +206,7 @@ describe("documents.delete", () => { await processor.perform({ name: "documents.delete", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { title: document.title }, diff --git a/server/queues/processors/CollectionsProcessor.ts b/server/queues/processors/CollectionsProcessor.ts new file mode 100644 index 000000000..121d96336 --- /dev/null +++ b/server/queues/processors/CollectionsProcessor.ts @@ -0,0 +1,23 @@ +import { CollectionEvent, Event } from "@server/types"; +import DetachDraftsFromCollectionTask from "../tasks/DetachDraftsFromCollectionTask"; +import BaseProcessor from "./BaseProcessor"; + +export default class CollectionsProcessor extends BaseProcessor { + static applicableEvents: Event["name"][] = ["collections.delete"]; + + async perform(event: CollectionEvent) { + switch (event.name) { + case "collections.delete": + return this.collectionDeleted(event); + default: + } + } + + async collectionDeleted(event: CollectionEvent) { + await DetachDraftsFromCollectionTask.schedule({ + collectionId: event.collectionId, + actorId: event.actorId, + ip: event.ip, + }); + } +} diff --git a/server/queues/processors/RevisionsProcessor.test.ts b/server/queues/processors/RevisionsProcessor.test.ts index bc0d7666e..749fa5ea7 100644 --- a/server/queues/processors/RevisionsProcessor.test.ts +++ b/server/queues/processors/RevisionsProcessor.test.ts @@ -15,7 +15,7 @@ describe("documents.update.debounced", () => { await processor.perform({ name: "documents.update.debounced", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, createdAt: new Date().toISOString(), @@ -38,7 +38,7 @@ describe("documents.update.debounced", () => { await processor.perform({ name: "documents.update.debounced", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, createdAt: new Date().toISOString(), diff --git a/server/queues/tasks/DetachDraftsFromCollectionTask.test.ts b/server/queues/tasks/DetachDraftsFromCollectionTask.test.ts new file mode 100644 index 000000000..3582fd996 --- /dev/null +++ b/server/queues/tasks/DetachDraftsFromCollectionTask.test.ts @@ -0,0 +1,33 @@ +import { Document } from "@server/models"; +import { buildCollection, buildDocument } from "@server/test/factories"; +import { setupTestDatabase } from "@server/test/support"; +import DetachDraftsFromCollectionTask from "./DetachDraftsFromCollectionTask"; + +setupTestDatabase(); + +describe("DetachDraftsFromCollectionTask", () => { + const ip = "127.0.0.1"; + it("should detach drafts from deleted collection", async () => { + const collection = await buildCollection(); + const document = await buildDocument({ + title: "test", + collectionId: collection.id, + publishedAt: null, + createdById: collection.createdById, + teamId: collection.teamId, + }); + await collection.destroy(); + + const task = new DetachDraftsFromCollectionTask(); + await task.perform({ + collectionId: collection.id, + ip, + actorId: collection.createdById, + }); + + const draft = await Document.findByPk(document.id); + expect(draft).not.toBe(null); + expect(draft?.deletedAt).toBe(null); + expect(draft?.collectionId).toBe(null); + }); +}); diff --git a/server/queues/tasks/DetachDraftsFromCollectionTask.ts b/server/queues/tasks/DetachDraftsFromCollectionTask.ts new file mode 100644 index 000000000..9320ff78b --- /dev/null +++ b/server/queues/tasks/DetachDraftsFromCollectionTask.ts @@ -0,0 +1,48 @@ +import { Op } from "sequelize"; +import documentMover from "@server/commands/documentMover"; +import { sequelize } from "@server/database/sequelize"; +import { Collection, Document, User } from "@server/models"; +import BaseTask from "./BaseTask"; + +type Props = { + collectionId: string; + actorId: string; + ip: string; +}; + +export default class DetachDraftsFromCollectionTask extends BaseTask { + async perform(props: Props) { + const [collection, actor] = await Promise.all([ + Collection.findByPk(props.collectionId, { + paranoid: false, + }), + User.findByPk(props.actorId), + ]); + + if (!actor || !collection || !collection.deletedAt) { + return; + } + + const documents = await Document.scope("withDrafts").findAll({ + where: { + collectionId: props.collectionId, + publishedAt: { + [Op.is]: null, + }, + }, + paranoid: false, + }); + + return sequelize.transaction(async (transaction) => { + for (const document of documents) { + await documentMover({ + document, + user: actor, + ip: props.ip, + collectionId: null, + transaction, + }); + } + }); + } +} diff --git a/server/queues/tasks/DocumentPublishedNotificationsTask.test.ts b/server/queues/tasks/DocumentPublishedNotificationsTask.test.ts index c455fcfda..4b31e7711 100644 --- a/server/queues/tasks/DocumentPublishedNotificationsTask.test.ts +++ b/server/queues/tasks/DocumentPublishedNotificationsTask.test.ts @@ -31,7 +31,7 @@ describe("documents.publish", () => { await processor.perform({ name: "documents.publish", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { @@ -55,7 +55,7 @@ describe("documents.publish", () => { await processor.perform({ name: "documents.publish", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { @@ -95,7 +95,7 @@ describe("documents.publish", () => { await processor.perform({ name: "documents.publish", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { @@ -124,7 +124,7 @@ describe("documents.publish", () => { await processor.perform({ name: "documents.publish", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: document.createdById, data: { diff --git a/server/queues/tasks/RevisionCreatedNotificationsTask.test.ts b/server/queues/tasks/RevisionCreatedNotificationsTask.test.ts index 8e1a96d83..7a0823c05 100644 --- a/server/queues/tasks/RevisionCreatedNotificationsTask.test.ts +++ b/server/queues/tasks/RevisionCreatedNotificationsTask.test.ts @@ -34,7 +34,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator.id, modelId: revision.id, @@ -63,7 +63,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator.id, modelId: revision.id, @@ -88,7 +88,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: user.id, modelId: revision.id, @@ -123,7 +123,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator.id, modelId: revision.id, @@ -152,7 +152,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator0.id, modelId: revision.id, @@ -202,7 +202,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator0.id, modelId: revision.id, @@ -247,7 +247,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator0.id, modelId: revision.id, @@ -303,7 +303,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator.id, modelId: revision.id, @@ -345,7 +345,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator.id, modelId: revision.id, @@ -391,7 +391,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator.id, modelId: revision.id, @@ -421,7 +421,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: collaborator.id, modelId: revision.id, @@ -444,7 +444,7 @@ describe("revisions.create", () => { await task.perform({ name: "revisions.create", documentId: document.id, - collectionId: document.collectionId, + collectionId: document.collectionId!, teamId: document.teamId, actorId: user.id, modelId: revision.id, diff --git a/server/routes/api/documents/documents.test.ts b/server/routes/api/documents/documents.test.ts index 53b7ce601..94f4e71d1 100644 --- a/server/routes/api/documents/documents.test.ts +++ b/server/routes/api/documents/documents.test.ts @@ -2928,6 +2928,35 @@ describe("#documents.delete", () => { expect(deletedDoc?.deletedAt).toBeTruthy(); }); + it("should delete a draft under deleted collection", async () => { + const user = await buildUser(); + const collection = await buildCollection({ + createdById: user.id, + teamId: user.teamId, + deletedAt: new Date(), + }); + const document = await buildDocument({ + createdById: user.id, + teamId: user.teamId, + collectionId: collection.id, + publishedAt: null, + }); + + const res = await server.post("/api/documents.delete", { + body: { + token: user.getJwtToken(), + id: document.id, + }, + }); + + expect(res.status).toBe(200); + const deletedDoc = await Document.findByPk(document.id, { + paranoid: false, + }); + expect(deletedDoc).not.toBe(null); + expect(deletedDoc?.deletedAt).not.toBe(null); + }); + it("should allow permanently deleting a document", async () => { const user = await buildUser(); const document = await buildDocument({ diff --git a/server/routes/api/documents/documents.ts b/server/routes/api/documents/documents.ts index b5819f4f9..b89254351 100644 --- a/server/routes/api/documents/documents.ts +++ b/server/routes/api/documents/documents.ts @@ -582,9 +582,11 @@ router.post( document.collectionId = collectionId; } - const collection = await Collection.scope({ - method: ["withMembership", user.id], - }).findByPk(document.collectionId); + const collection = document.collectionId + ? await Collection.scope({ + method: ["withMembership", user.id], + }).findByPk(document.collectionId) + : undefined; // if the collectionId was provided in the request and isn't valid then it will // be caught as a 403 on the authorize call below. Otherwise we're checking here @@ -938,6 +940,10 @@ router.post( ip: ctx.request.ip, }); + if (!document.collectionId) { + return null; + } + return await Collection.scope({ method: ["withMembership", user.id], }).findByPk(document.collectionId, { transaction }); diff --git a/server/test/support.ts b/server/test/support.ts index b813be3cc..08c8a282e 100644 --- a/server/test/support.ts +++ b/server/test/support.ts @@ -87,7 +87,9 @@ export const seed = async () => }, { transaction } ); - await document.publish(collection.createdById, { transaction }); + await document.publish(collection.createdById, collection.id, { + transaction, + }); await collection.reload({ transaction }); return { user, diff --git a/shared/i18n/locales/en_US/translation.json b/shared/i18n/locales/en_US/translation.json index 80e7166d3..ba6b0ab45 100644 --- a/shared/i18n/locales/en_US/translation.json +++ b/shared/i18n/locales/en_US/translation.json @@ -98,7 +98,7 @@ "Viewers": "Viewers", "I’m sure – Delete": "I’m sure – Delete", "Deleting": "Deleting", - "Are you sure about that? Deleting the {{collectionName}} collection is permanent and cannot be restored, however documents within will be moved to the trash.": "Are you sure about that? Deleting the {{collectionName}} collection is permanent and cannot be restored, however documents within will be moved to the trash.", + "Are you sure about that? Deleting the {{collectionName}} collection is permanent and cannot be restored, however all published documents within will be moved to the trash.": "Are you sure about that? Deleting the {{collectionName}} collection is permanent and cannot be restored, however all published documents within will be moved to the trash.", "Also, {{collectionName}} is being used as the start view – deleting it will reset the start view to the Home page.": "Also, {{collectionName}} is being used as the start view – deleting it will reset the start view to the Home page.", "Sorry, an error occurred saving the collection": "Sorry, an error occurred saving the collection", "Add a description": "Add a description",