From a89d30c7359df16fc5ea0ef070dbb41192246be5 Mon Sep 17 00:00:00 2001 From: Apoorv Mishra Date: Tue, 25 Oct 2022 18:01:57 +0530 Subject: [PATCH] Allow drafts to be created without requiring a collection (#4175) * feat(server): allow document to be created without collectionId * fix(server): policies for a draft doc without collection * fix(app): hide share button for drafts * feat(server): permissions around publishing a draft * fix(server): return drafts without collection * fix(server): handle draft deletion * fix(server): show drafts in deleted docs * fix(server): allow drafts without collection to be restored * feat(server): return drafts in search results * fix: use buildDraftDocument for drafts * fix: remove isDraftWithoutCollection * fix: do not return drafts for team * fix: put invariants * fix: query clause * fix: check only for undefined * fix: restore includeDrafts clause as it was before --- app/scenes/Document/components/Header.tsx | 3 +- server/commands/documentLoader.ts | 2 +- server/commands/documentMover.test.ts | 8 +- server/commands/documentUpdater.ts | 7 +- server/models/Document.test.ts | 80 ++++++ server/models/Document.ts | 23 +- server/policies/document.test.ts | 33 +++ server/policies/document.ts | 43 +++- server/presenters/slackAttachment.ts | 6 +- server/queues/processors/SlackProcessor.ts | 2 +- server/routes/api/documents.test.ts | 285 ++++++++++++++++++++- server/routes/api/documents.ts | 122 ++++++--- server/routes/api/hooks.ts | 6 +- server/test/factories.ts | 21 +- 14 files changed, 557 insertions(+), 84 deletions(-) diff --git a/app/scenes/Document/components/Header.tsx b/app/scenes/Document/components/Header.tsx index 4a2f26e98..3d43d312b 100644 --- a/app/scenes/Document/components/Header.tsx +++ b/app/scenes/Document/components/Header.tsx @@ -237,7 +237,8 @@ function DocumentHeader({ {!isEditing && !isDeleted && !isRevision && - (!isMobile || !isTemplate) && ( + (!isMobile || !isTemplate) && + document.collectionId && ( diff --git a/server/commands/documentLoader.ts b/server/commands/documentLoader.ts index 5618a3cc1..5788d29fd 100644 --- a/server/commands/documentLoader.ts +++ b/server/commands/documentLoader.ts @@ -19,7 +19,7 @@ type Props = { type Result = { document: Document; share?: Share; - collection: Collection; + collection?: Collection | null; }; export default async function loadDocument({ diff --git a/server/commands/documentMover.test.ts b/server/commands/documentMover.test.ts index 39ea6c4f8..5c80613a2 100644 --- a/server/commands/documentMover.test.ts +++ b/server/commands/documentMover.test.ts @@ -73,7 +73,7 @@ describe("documentMover", () => { ); expect(response.collections.length).toEqual(1); expect(response.documents.length).toEqual(1); - expect(response.documents[0].collection.id).toEqual(collection.id); + expect(response.documents[0].collection?.id).toEqual(collection.id); expect(response.documents[0].updatedBy.id).toEqual(user.id); }); @@ -112,9 +112,9 @@ describe("documentMover", () => { expect(response.collections.length).toEqual(2); expect(response.documents.length).toEqual(2); - expect(response.documents[0].collection.id).toEqual(newCollection.id); + expect(response.documents[0].collection?.id).toEqual(newCollection.id); expect(response.documents[0].updatedBy.id).toEqual(user.id); - expect(response.documents[1].collection.id).toEqual(newCollection.id); + expect(response.documents[1].collection?.id).toEqual(newCollection.id); expect(response.documents[1].updatedBy.id).toEqual(user.id); }); @@ -151,7 +151,7 @@ describe("documentMover", () => { expect(response.collections.length).toEqual(2); expect(response.documents.length).toEqual(1); - expect(response.documents[0].collection.id).toEqual(newCollection.id); + expect(response.documents[0].collection?.id).toEqual(newCollection.id); expect(response.documents[0].updatedBy.id).toEqual(user.id); }); }); diff --git a/server/commands/documentUpdater.ts b/server/commands/documentUpdater.ts index 8567d79cf..5c3f47fc0 100644 --- a/server/commands/documentUpdater.ts +++ b/server/commands/documentUpdater.ts @@ -21,6 +21,8 @@ type Props = { append?: boolean; /** Whether the document should be published to the collection */ publish?: boolean; + /** The ID of the collection to publish the document to */ + collectionId?: string; /** The IP address of the user creating the document */ ip: string; /** The database transaction to run within */ @@ -44,6 +46,7 @@ export default async function documentUpdater({ fullWidth, append, publish, + collectionId, transaction, ip, }: Props): Promise { @@ -74,7 +77,9 @@ export default async function documentUpdater({ const changed = document.changed(); if (publish) { - document.lastModifiedById = user.id; + if (!document.collectionId) { + document.collectionId = collectionId as string; + } await document.publish(user.id, { transaction }); await Event.create( diff --git a/server/models/Document.test.ts b/server/models/Document.test.ts index 2cd41f271..55960c2f5 100644 --- a/server/models/Document.test.ts +++ b/server/models/Document.test.ts @@ -1,6 +1,7 @@ import Document from "@server/models/Document"; import { buildDocument, + buildDraftDocument, buildCollection, buildTeam, buildUser, @@ -336,6 +337,74 @@ describe("#searchForUser", () => { expect(results.length).toBe(0); }); + test("should search only drafts created by user", async () => { + const user = await buildUser(); + await buildDraftDocument({ + teamId: user.teamId, + userId: user.id, + createdById: user.id, + title: "test", + }); + const { results } = await Document.searchForUser(user, "test", { + includeDrafts: true, + }); + expect(results.length).toBe(1); + }); + + test("should not include drafts", async () => { + const user = await buildUser(); + await buildDraftDocument({ + teamId: user.teamId, + userId: user.id, + createdById: user.id, + title: "test", + }); + const { results } = await Document.searchForUser(user, "test", { + includeDrafts: false, + }); + expect(results.length).toBe(0); + }); + + test("should include results from drafts as well", async () => { + const user = await buildUser(); + await buildDocument({ + userId: user.id, + teamId: user.teamId, + createdById: user.id, + title: "not draft", + }); + await buildDraftDocument({ + teamId: user.teamId, + userId: user.id, + createdById: user.id, + title: "draft", + }); + const { results } = await Document.searchForUser(user, "draft", { + includeDrafts: true, + }); + expect(results.length).toBe(2); + }); + + test("should not include results from drafts", async () => { + const user = await buildUser(); + await buildDocument({ + userId: user.id, + teamId: user.teamId, + createdById: user.id, + title: "not draft", + }); + await buildDraftDocument({ + teamId: user.teamId, + userId: user.id, + createdById: user.id, + title: "draft", + }); + const { results } = await Document.searchForUser(user, "draft", { + includeDrafts: false, + }); + expect(results.length).toBe(1); + }); + test("should return the total count of search results", async () => { const team = await buildTeam(); const user = await buildUser({ @@ -445,6 +514,17 @@ describe("#delete", () => { expect(newDocument?.lastModifiedById).toBe(user.id); expect(newDocument?.deletedAt).toBeTruthy(); }); + + it("should delete draft without collection", async () => { + const user = await buildUser(); + const document = await buildDraftDocument(); + await document.delete(user.id); + const deletedDocument = await Document.findByPk(document.id, { + paranoid: false, + }); + expect(deletedDocument?.lastModifiedById).toBe(user.id); + expect(deletedDocument?.deletedAt).toBeTruthy(); + }); }); describe("#save", () => { diff --git a/server/models/Document.ts b/server/models/Document.ts index 8d5208722..2c4a079d1 100644 --- a/server/models/Document.ts +++ b/server/models/Document.ts @@ -404,7 +404,7 @@ class Document extends ParanoidModel { teamId: string; @BelongsTo(() => Collection, "collectionId") - collection: Collection; + collection: Collection | null | undefined; @ForeignKey(() => Collection) @Column(DataType.UUID) @@ -620,14 +620,6 @@ class Document extends ParanoidModel { collectionIds = await user.collectionIds(); } - // If the user has access to no collections then shortcircuit the rest of this - if (!collectionIds.length) { - return { - results: [], - totalCount: 0, - }; - } - let dateFilter; if (options.dateFilter) { @@ -636,9 +628,16 @@ class Document extends ParanoidModel { // Build the SQL query to get documentIds, ranking, and search term context const whereClause = ` - "searchVector" @@ to_tsquery('english', :query) AND + "searchVector" @@ to_tsquery('english', :query) AND "teamId" = :teamId AND - "collectionId" IN(:collectionIds) AND + ${ + collectionIds.length + ? `( + "collectionId" IN(:collectionIds) OR + ("collectionId" IS NULL AND "createdById" = :userId) + ) AND` + : '"collectionId" IS NULL AND "createdById" = :userId AND' + } ${ options.dateFilter ? '"updatedAt" > now() - interval :dateFilter AND' : "" } @@ -962,7 +961,7 @@ class Document extends ParanoidModel { // Delete a document, archived or otherwise. delete = (userId: string) => { return this.sequelize.transaction(async (transaction: Transaction) => { - if (!this.archivedAt && !this.template) { + if (!this.archivedAt && !this.template && this.collectionId) { // delete any children and remove from the document structure const collection = await Collection.findByPk(this.collectionId, { transaction, diff --git a/server/policies/document.test.ts b/server/policies/document.test.ts index 015e3b703..627837556 100644 --- a/server/policies/document.test.ts +++ b/server/policies/document.test.ts @@ -3,6 +3,7 @@ import { buildUser, buildTeam, buildDocument, + buildDraftDocument, buildCollection, } from "@server/test/factories"; import { setupTestDatabase } from "@server/test/support"; @@ -112,3 +113,35 @@ describe("private collection", () => { expect(abilities.move).toEqual(false); }); }); + +describe("no collection", () => { + it("should grant same permissions as that on a draft document except the share permission", async () => { + const team = await buildTeam(); + const user = await buildUser({ + teamId: team.id, + }); + const document = await buildDraftDocument({ + teamId: team.id, + }); + const abilities = serialize(user, document); + expect(abilities.archive).toEqual(false); + expect(abilities.createChildDocument).toEqual(false); + expect(abilities.delete).toEqual(true); + expect(abilities.download).toEqual(true); + expect(abilities.move).toEqual(false); + expect(abilities.permanentDelete).toEqual(false); + expect(abilities.pin).toEqual(false); + expect(abilities.pinToHome).toEqual(false); + expect(abilities.read).toEqual(true); + expect(abilities.restore).toEqual(false); + expect(abilities.share).toEqual(false); + expect(abilities.star).toEqual(true); + expect(abilities.subscribe).toEqual(false); + expect(abilities.unarchive).toEqual(false); + expect(abilities.unpin).toEqual(false); + expect(abilities.unpublish).toEqual(false); + expect(abilities.unstar).toEqual(true); + expect(abilities.unsubscribe).toEqual(false); + expect(abilities.update).toEqual(true); + }); +}); diff --git a/server/policies/document.ts b/server/policies/document.ts index 41806ee3e..4489f4eb6 100644 --- a/server/policies/document.ts +++ b/server/policies/document.ts @@ -35,13 +35,17 @@ allow(User, "star", Document, (user, document) => { if (document.template) { return false; } - invariant( - document.collection, - "collection is missing, did you forget to include in the query scope?" - ); - if (cannot(user, "read", document.collection)) { - return false; + + if (document.collectionId) { + invariant( + document.collection, + "collection is missing, did you forget to include in the query scope?" + ); + if (cannot(user, "read", document.collection)) { + return false; + } } + return user.teamId === document.teamId; }); @@ -52,13 +56,17 @@ allow(User, "unstar", Document, (user, document) => { if (document.template) { return false; } - invariant( - document.collection, - "collection is missing, did you forget to include in the query scope?" - ); - if (cannot(user, "read", document.collection)) { - return false; + + if (document.collectionId) { + invariant( + document.collection, + "collection is missing, did you forget to include in the query scope?" + ); + if (cannot(user, "read", document.collection)) { + return false; + } } + return user.teamId === document.teamId; }); @@ -95,8 +103,15 @@ allow(User, "update", Document, (user, document) => { return false; } - if (cannot(user, "update", document.collection)) { - return false; + if (document.collectionId) { + invariant( + document.collection, + "collection is missing, did you forget to include in the query scope?" + ); + + if (cannot(user, "update", document.collection)) { + return false; + } } return user.teamId === document.teamId; diff --git a/server/presenters/slackAttachment.ts b/server/presenters/slackAttachment.ts index 77543492f..586b8526f 100644 --- a/server/presenters/slackAttachment.ts +++ b/server/presenters/slackAttachment.ts @@ -10,8 +10,8 @@ type Action = { function present( document: Document, - collection: Collection, team: Team, + collection?: Collection | null, context?: string, actions?: Action[] ) { @@ -22,10 +22,10 @@ function present( : document.getSummary(); return { - color: collection.color, + color: collection?.color, title: document.title, title_link: `${team.url}${document.url}`, - footer: collection.name, + footer: collection?.name, callback_id: document.id, text, ts: document.getTimestamp(), diff --git a/server/queues/processors/SlackProcessor.ts b/server/queues/processors/SlackProcessor.ts index 6537333e0..4b0f0f61f 100644 --- a/server/queues/processors/SlackProcessor.ts +++ b/server/queues/processors/SlackProcessor.ts @@ -124,7 +124,7 @@ export default class SlackProcessor extends BaseProcessor { body: JSON.stringify({ text, attachments: [ - presentSlackAttachment(document, document.collection, team), + presentSlackAttachment(document, team, document.collection), ], }), }); diff --git a/server/routes/api/documents.test.ts b/server/routes/api/documents.test.ts index 29770437d..5968f5b0c 100644 --- a/server/routes/api/documents.test.ts +++ b/server/routes/api/documents.test.ts @@ -14,7 +14,9 @@ import { buildCollection, buildUser, buildDocument, + buildDraftDocument, buildViewer, + buildTeam, } from "@server/test/factories"; import { seed, getTestServer } from "@server/test/support"; @@ -865,6 +867,30 @@ describe("#documents.drafts", () => { expect(body.data.length).toEqual(1); }); + it("should return drafts, including ones without collectionIds", async () => { + const drafts = []; + const { user, document } = await seed(); + document.publishedAt = null; + await document.save(); + drafts.push(document); + const draftDocument = await buildDraftDocument({ + title: "draft title", + text: "draft text", + createdById: user.id, + teamId: user.teamId, + }); + drafts.push(draftDocument); + + const res = await server.post("/api/documents.drafts", { + body: { + token: user.getJwtToken(), + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.length).toEqual(drafts.length); + }); + it("should not return documents in private collections not a member of", async () => { const { user, document, collection } = await seed(); document.publishedAt = null; @@ -980,6 +1006,29 @@ describe("#documents.search", () => { expect(body.data[0].document.id).toEqual(share.documentId); }); + it("should not return drafts using shareId", async () => { + const { user, document } = await seed(); + document.publishedAt = null; + await document.save(); + const share = await buildShare({ + documentId: document.id, + includeChildDocuments: true, + teamId: user.teamId, + userId: user.id, + }); + const res = await server.post("/api/documents.search", { + body: { + token: user.getJwtToken(), + shareId: share.id, + includeDrafts: true, + query: "test", + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.length).toEqual(0); + }); + it("should not allow search if child documents are not included", async () => { const findableDocument = await buildDocument({ title: "search term", @@ -1140,7 +1189,7 @@ describe("#documents.search", () => { body: { token: user.getJwtToken(), query: "search term", - includeDrafts: "true", + includeDrafts: true, }, }); const body = await res.json(); @@ -1149,6 +1198,27 @@ describe("#documents.search", () => { expect(body.data[0].document.id).toEqual(document.id); }); + it("should return drafts without collection too", async () => { + const user = await buildUser(); + await buildDraftDocument({ + title: "some title", + text: "some text", + createdById: user.id, + userId: user.id, + teamId: user.teamId, + }); + const res = await server.post("/api/documents.search", { + body: { + token: user.getJwtToken(), + includeDrafts: true, + query: "text", + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.length).toEqual(1); + }); + it("should not return draft documents created by other users", async () => { const user = await buildUser(); await buildDocument({ @@ -1161,7 +1231,7 @@ describe("#documents.search", () => { body: { token: user.getJwtToken(), query: "search term", - includeDrafts: "true", + includeDrafts: true, }, }); const body = await res.json(); @@ -1200,7 +1270,7 @@ describe("#documents.search", () => { body: { token: user.getJwtToken(), query: "search term", - includeArchived: "true", + includeArchived: true, }, }); const body = await res.json(); @@ -1470,6 +1540,30 @@ describe("#documents.deleted", () => { expect(body.data.length).toEqual(1); }); + it("should return deleted documents, including drafts without collection", async () => { + const { user } = await seed(); + const document = await buildDocument({ + userId: user.id, + teamId: user.teamId, + }); + const draftDocument = await buildDraftDocument({ + userId: user.id, + teamId: user.teamId, + }); + await Promise.all([ + document.delete(user.id), + draftDocument.delete(user.id), + ]); + const res = await server.post("/api/documents.deleted", { + body: { + token: user.getJwtToken(), + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.length).toEqual(2); + }); + it("should not return documents in private collections not a member of", async () => { const { user } = await seed(); const collection = await buildCollection({ @@ -1646,6 +1740,24 @@ describe("#documents.restore", () => { expect(body.data.deletedAt).toEqual(null); }); + it("should allow restore of trashed drafts without collection", async () => { + const { user } = await seed(); + const document = await buildDraftDocument({ + userId: user.id, + teamId: user.teamId, + }); + await document.delete(user.id); + const res = await server.post("/api/documents.restore", { + body: { + token: user.getJwtToken(), + id: document.id, + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.deletedAt).toEqual(null); + }); + it("should allow restore of trashed documents with collectionId", async () => { const { user, document } = await seed(); const collection = await buildCollection({ @@ -1864,6 +1976,81 @@ describe("#documents.create", () => { expect(body.policies[0].abilities.update).toEqual(true); }); + it("should create a draft document not belonging to any collection", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const res = await server.post("/api/documents.create", { + body: { + token: user.getJwtToken(), + title: "draft document", + text: "draft document without collection", + }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.data.title).toBe("draft document"); + expect(body.data.text).toBe("draft document without collection"); + expect(body.data.collectionId).toBeNull(); + }); + + it("should not allow creating a template without a collection", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const res = await server.post("/api/documents.create", { + body: { + template: true, + token: user.getJwtToken(), + title: "template", + text: "template without collection", + }, + }); + const body = await res.json(); + + expect(body.message).toBe( + "collectionId is required to create a nested doc or a template" + ); + expect(res.status).toEqual(400); + }); + + it("should not allow publishing without specifying the collection", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const res = await server.post("/api/documents.create", { + body: { + token: user.getJwtToken(), + title: "title", + text: "text", + publish: true, + }, + }); + const body = await res.json(); + + expect(body.message).toBe( + "collectionId is required to publish a draft without collection" + ); + expect(res.status).toEqual(400); + }); + + it("should not allow creating a nested doc without a collection", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const res = await server.post("/api/documents.create", { + body: { + token: user.getJwtToken(), + parentDocumentId: "d7a4eb73-fac1-4028-af45-d7e34d54db8e", + title: "nested doc", + text: "nested doc without collection", + }, + }); + const body = await res.json(); + + expect(body.message).toBe( + "collectionId is required to create a nested doc or a template" + ); + expect(res.status).toEqual(400); + }); + it("should not allow very long titles", async () => { const { user, collection } = await seed(); const res = await server.post("/api/documents.create", { @@ -1969,6 +2156,73 @@ describe("#documents.update", () => { expect(events.length).toEqual(1); }); + it("should not allow publishing a draft without specifying the collection", async () => { + const { user, team } = await seed(); + const document = await buildDraftDocument({ + teamId: team.id, + }); + const res = await server.post("/api/documents.update", { + body: { + token: user.getJwtToken(), + id: document.id, + title: "Updated title", + text: "Updated text", + lastRevision: document.revisionCount, + publish: true, + }, + }); + const body = await res.json(); + expect(res.status).toEqual(400); + expect(body.message).toBe( + "collectionId is required to publish a draft without collection" + ); + }); + + it("should successfully publish a draft", async () => { + const { user, team, collection } = await seed(); + const document = await buildDraftDocument({ + title: "title", + text: "text", + teamId: team.id, + }); + + const res = await server.post("/api/documents.update", { + body: { + token: user.getJwtToken(), + id: document.id, + title: "Updated title", + text: "Updated text", + lastRevision: document.revisionCount, + collectionId: collection.id, + publish: true, + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.collectionId).toBe(collection.id); + expect(body.data.title).toBe("Updated title"); + expect(body.data.text).toBe("Updated text"); + }); + + it("should not allow publishing by another collection's user", async () => { + const { user, document, collection } = await seed(); + const anotherTeam = await buildTeam(); + user.teamId = anotherTeam.id; + await user.save(); + const res = await server.post("/api/documents.update", { + body: { + token: user.getJwtToken(), + id: document.id, + title: "Updated title", + text: "Updated text", + lastRevision: document.revisionCount, + collectionId: collection.id, + publish: true, + }, + }); + expect(res.status).toEqual(403); + }); + it("should not add template to collection structure when publishing", async () => { const user = await buildUser(); const collection = await buildCollection({ @@ -2282,6 +2536,31 @@ describe("#documents.delete", () => { expect(body.success).toEqual(true); }); + it("should delete a draft without collection", async () => { + const { user } = await seed(); + const document = await buildDraftDocument({ + teamId: user.teamId, + deletedAt: null, + }); + const res = await server.post("/api/documents.delete", { + body: { + token: user.getJwtToken(), + id: document.id, + }, + }); + + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.success).toEqual(true); + + const deletedDoc = await Document.findByPk(document.id, { + paranoid: false, + }); + expect(deletedDoc).toBeDefined(); + expect(deletedDoc).not.toBeNull(); + expect(deletedDoc?.deletedAt).toBeTruthy(); + }); + it("should allow permanently deleting a document", async () => { const user = await buildUser(); const document = await buildDocument({ diff --git a/server/routes/api/documents.ts b/server/routes/api/documents.ts index e1b3c2732..ab1e389da 100644 --- a/server/routes/api/documents.ts +++ b/server/routes/api/documents.ts @@ -44,6 +44,7 @@ import { assertPresent, assertPositiveInteger, assertNotEmpty, + assertBoolean, } from "@server/validation"; import env from "../../env"; import pagination from "./middlewares/pagination"; @@ -244,7 +245,9 @@ router.post( ]).findAll({ where: { teamId: user.teamId, - collectionId: collectionIds, + collectionId: { + [Op.or]: [{ [Op.in]: collectionIds }, { [Op.is]: null }], + }, deletedAt: { [Op.ne]: null, }, @@ -353,9 +356,11 @@ router.post("documents.drafts", auth(), pagination(), async (ctx) => { const collectionIds = collectionId ? [collectionId] : await user.collectionIds(); - const where: WhereOptions = { + const where: WhereOptions = { createdById: user.id, - collectionId: collectionIds, + collectionId: { + [Op.or]: [{ [Op.in]: collectionIds }, { [Op.is]: null }], + }, publishedAt: { [Op.is]: null, }, @@ -425,7 +430,7 @@ router.post( document: serializedDocument, sharedTree: share && share.includeChildDocuments - ? collection.getDocumentTree(share.documentId) + ? collection?.getDocumentTree(share.documentId) : undefined, } : serializedDocument; @@ -516,13 +521,15 @@ router.post("documents.restore", auth({ member: true }), async (ctx) => { // be caught as a 403 on the authorize call below. Otherwise we're checking here // that the original collection still exists and advising to pass collectionId // if not. - if (!collectionId && !collection) { + if (document.collection && !collectionId && !collection) { throw ValidationError( "Unable to restore to original collection, it may have been deleted" ); } - authorize(user, "update", collection); + if (document.collection) { + authorize(user, "update", collection); + } if (document.deletedAt) { authorize(user, "restore", document); @@ -655,6 +662,14 @@ router.post( } = ctx.request.body; assertNotEmpty(query, "query is required"); + if (includeDrafts) { + assertBoolean(includeDrafts); + } + + if (includeArchived) { + assertBoolean(includeArchived); + } + const { offset, limit } = ctx.state.pagination; const snippetMinWords = parseInt( ctx.request.body.snippetMinWords || 20, @@ -687,8 +702,8 @@ router.post( invariant(team, "Share must belong to a team"); response = await Document.searchForTeam(team, query, { - includeArchived: includeArchived === "true", - includeDrafts: includeDrafts === "true", + includeArchived, + includeDrafts, collectionId: document.collectionId, share, dateFilter, @@ -728,8 +743,8 @@ router.post( } response = await Document.searchForUser(user, query, { - includeArchived: includeArchived === "true", - includeDrafts: includeDrafts === "true", + includeArchived, + includeDrafts, collaboratorIds, collectionId, dateFilter, @@ -827,6 +842,7 @@ router.post("documents.update", auth(), async (ctx) => { publish, lastRevision, templateId, + collectionId, append, } = ctx.request.body; const editorVersion = ctx.headers["x-editor-version"] as string | undefined; @@ -834,24 +850,39 @@ router.post("documents.update", auth(), async (ctx) => { if (append) { assertPresent(text, "Text is required while appending"); } + + if (collectionId) { + assertUuid(collectionId, "collectionId must be an uuid"); + } + const { user } = ctx.state; let collection: Collection | null | undefined; - const document = await sequelize.transaction(async (transaction) => { - const document = await Document.findByPk(id, { - userId: user.id, - includeState: true, - transaction, - }); - authorize(user, "update", document); + const document = await Document.findByPk(id, { + userId: user.id, + includeState: true, + }); + authorize(user, "update", document); - collection = document.collection; - - if (lastRevision && lastRevision !== document.revisionCount) { - throw InvalidRequestError("Document has changed since last revision"); + if (publish) { + if (!document.collectionId) { + assertPresent( + collectionId, + "collectionId is required to publish a draft without collection" + ); + collection = await Collection.findByPk(collectionId); + } else { + collection = document.collection; } + authorize(user, "publish", collection); + } + if (lastRevision && lastRevision !== document.revisionCount) { + throw InvalidRequestError("Document has changed since last revision"); + } + + const updatedDocument = await sequelize.transaction(async (transaction) => { return documentUpdater({ document, user, @@ -859,6 +890,7 @@ router.post("documents.update", auth(), async (ctx) => { text, fullWidth, publish, + collectionId, append, templateId, editorVersion, @@ -867,14 +899,12 @@ router.post("documents.update", auth(), async (ctx) => { }); }); - invariant(collection, "collection not found"); - - document.updatedBy = user; - document.collection = collection; + updatedDocument.updatedBy = user; + updatedDocument.collection = collection; ctx.body = { - data: await presentDocument(document), - policies: presentPolicies(user, [document]), + data: await presentDocument(updatedDocument), + policies: presentPolicies(user, [updatedDocument]), }; }); @@ -1169,7 +1199,19 @@ router.post("documents.create", auth(), async (ctx) => { index, } = ctx.request.body; const editorVersion = ctx.headers["x-editor-version"] as string | undefined; - assertUuid(collectionId, "collectionId must be an uuid"); + + if (parentDocumentId || template || publish) { + assertPresent( + collectionId, + publish + ? "collectionId is required to publish a draft without collection" + : "collectionId is required to create a nested doc or a template" + ); + } + + if (collectionId) { + assertUuid(collectionId, "collectionId must be an uuid"); + } if (parentDocumentId) { assertUuid(parentDocumentId, "parentDocumentId must be an uuid"); @@ -1180,15 +1222,19 @@ router.post("documents.create", auth(), async (ctx) => { } const { user } = ctx.state; - const collection = await Collection.scope({ - method: ["withMembership", user.id], - }).findOne({ - where: { - id: collectionId, - teamId: user.teamId, - }, - }); - authorize(user, "publish", collection); + let collection; + + if (collectionId) { + collection = await Collection.scope({ + method: ["withMembership", user.id], + }).findOne({ + where: { + id: collectionId, + teamId: user.teamId, + }, + }); + authorize(user, "publish", collection); + } let parentDocument; @@ -1196,7 +1242,7 @@ router.post("documents.create", auth(), async (ctx) => { parentDocument = await Document.findOne({ where: { id: parentDocumentId, - collectionId: collection.id, + collectionId: collection?.id, }, }); authorize(user, "read", parentDocument, { diff --git a/server/routes/api/hooks.ts b/server/routes/api/hooks.ts index e1854a68e..773e19d7d 100644 --- a/server/routes/api/hooks.ts +++ b/server/routes/api/hooks.ts @@ -85,7 +85,7 @@ router.post("hooks.unfurl", async (ctx) => { unfurls[link.url] = { title: doc.title, text: doc.getSummary(), - color: doc.collection.color, + color: doc.collection?.color, }; } @@ -131,8 +131,8 @@ router.post("hooks.interactive", async (ctx) => { attachments: [ presentSlackAttachment( document, - document.collection, team, + document.collection, document.getSummary() ), ], @@ -303,8 +303,8 @@ router.post("hooks.slack", async (ctx) => { attachments.push( presentSlackAttachment( result.document, - result.document.collection, team, + result.document.collection, queryIsInTitle ? undefined : result.context, env.SLACK_MESSAGE_ACTIONS ? [ diff --git a/server/test/factories.ts b/server/test/factories.ts index 28ea63fee..3505d12bb 100644 --- a/server/test/factories.ts +++ b/server/test/factories.ts @@ -1,3 +1,4 @@ +import { isNull } from "lodash"; import { v4 as uuidv4 } from "uuid"; import { CollectionPermission } from "@shared/types"; import { @@ -323,8 +324,22 @@ export async function buildGroupUser( }); } -export async function buildDocument( +export async function buildDraftDocument( overrides: Partial & { userId?: string } = {} +) { + return buildDocument({ ...overrides, collectionId: null }); +} + +export async function buildDocument( + // Omission first, addition later? + // This is actually a workaround to allow + // passing collectionId as null. Ideally, it + // should be updated in the Document model itself + // but that'd cascade and require further changes + // beyond the scope of what's required now + overrides: Omit, "collectionId"> & { userId?: string } & { + collectionId?: string | null; + } = {} ) { if (!overrides.teamId) { const team = await buildTeam(); @@ -336,7 +351,7 @@ export async function buildDocument( overrides.userId = user.id; } - if (!overrides.collectionId) { + if (overrides.collectionId === undefined) { const collection = await buildCollection({ teamId: overrides.teamId, userId: overrides.userId, @@ -348,7 +363,7 @@ export async function buildDocument( return Document.create({ title: `Document ${count}`, text: "This is the text in an example document", - publishedAt: new Date(), + publishedAt: isNull(overrides.collectionId) ? null : new Date(), lastModifiedById: overrides.userId, createdById: overrides.userId, ...overrides,