feat: docs managers can action docs & create subdocs (#7077)

* feat: docs managers can action docs & create subdocs * tests --------- Co-authored-by: Tom Moor <tom.moor@gmail.com>
2024-06-19 19:22:33 -07:00
parent 2333602f25
commit 95b9453269
11 changed files with 271 additions and 136 deletions
--- a/app/actions/definitions/documents.tsx
+++ b/app/actions/definitions/documents.tsx
@@ -51,6 +51,7 @@ import {
  documentHistoryPath,
  homePath,
  newDocumentPath,
+  newNestedDocumentPath,
  searchPath,
  documentPath,
  urlify,
@@ -141,15 +142,10 @@ export const createNestedDocument = createAction({
    !!activeDocumentId &&
    stores.policies.abilities(currentTeamId).createDocument &&
    stores.policies.abilities(activeDocumentId).createChildDocument,
-  perform: ({ activeCollectionId, activeDocumentId, inStarredSection }) =>
-    history.push(
-      newDocumentPath(activeCollectionId, {
-        parentDocumentId: activeDocumentId,
-      }),
-      {
-        starred: inStarredSection,
-      }
-    ),
+  perform: ({ activeDocumentId, inStarredSection }) =>
+    history.push(newNestedDocumentPath(activeDocumentId), {
+      starred: inStarredSection,
+    }),
 });

 export const starDocument = createAction({
--- a/app/components/Sidebar/components/DocumentLink.tsx
+++ b/app/components/Sidebar/components/DocumentLink.tsx
@@ -20,7 +20,7 @@ import useBoolean from "~/hooks/useBoolean";
 import usePolicy from "~/hooks/usePolicy";
 import useStores from "~/hooks/useStores";
 import DocumentMenu from "~/menus/DocumentMenu";
-import { newDocumentPath } from "~/utils/routeHelpers";
+import { newNestedDocumentPath } from "~/utils/routeHelpers";
 import DropCursor from "./DropCursor";
 import DropToImport from "./DropToImport";
 import EditableTitle, { RefHandle } from "./EditableTitle";
@@ -359,9 +359,7 @@ function InnerDocumentLink(
                            type={undefined}
                            aria-label={t("New nested document")}
                            as={Link}
-                            to={newDocumentPath(document.collectionId, {
-                              parentDocumentId: document.id,
-                            })}
+                            to={newNestedDocumentPath(document.id)}
                          >
                            <PlusIcon />
                          </NudeButton>
--- a/app/menus/NewChildDocumentMenu.tsx
+++ b/app/menus/NewChildDocumentMenu.tsx
@@ -5,8 +5,10 @@ import { useMenuState, MenuButton, MenuButtonHTMLProps } from "reakit/Menu";
 import Document from "~/models/Document";
 import ContextMenu from "~/components/ContextMenu";
 import Template from "~/components/ContextMenu/Template";
+import usePolicy from "~/hooks/usePolicy";
 import useStores from "~/hooks/useStores";
-import { newDocumentPath } from "~/utils/routeHelpers";
+import { MenuItem } from "~/types";
+import { newDocumentPath, newNestedDocumentPath } from "~/utils/routeHelpers";

 type Props = {
  label?: (props: MenuButtonHTMLProps) => React.ReactNode;
@@ -17,58 +19,59 @@ function NewChildDocumentMenu({ document, label }: Props) {
  const menu = useMenuState({
    modal: true,
  });
-  const { collections } = useStores();
  const { t } = useTranslation();
-  const collection = document.collectionId
-    ? collections.get(document.collectionId)
-    : undefined;
-  const collectionName = collection ? collection.name : t("collection");
+  const canCollection = usePolicy(document.collectionId);
+  const { collections } = useStores();
+
+  const items: MenuItem[] = [];
+
+  if (canCollection.createDocument) {
+    const collection = document.collectionId
+      ? collections.get(document.collectionId)
+      : undefined;
+    const collectionName = collection ? collection.name : t("collection");
+    items.push({
+      type: "route",
+      title: (
+        <span>
+          <Trans
+            defaults="New document in <em>{{ collectionName }}</em>"
+            values={{
+              collectionName,
+            }}
+            components={{
+              em: <strong />,
+            }}
+          />
+        </span>
+      ),
+      to: newDocumentPath(document.collectionId),
+    });
+  }
+
+  items.push({
+    type: "route",
+    title: (
+      <span>
+        <Trans
+          defaults="New document in <em>{{ collectionName }}</em>"
+          values={{
+            collectionName: document.title,
+          }}
+          components={{
+            em: <strong />,
+          }}
+        />
+      </span>
+    ),
+    to: newNestedDocumentPath(document.id),
+  });

  return (
    <>
      <MenuButton {...menu}>{label}</MenuButton>
      <ContextMenu {...menu} aria-label={t("New child document")}>
-        <Template
-          {...menu}
-          items={[
-            {
-              type: "route",
-              title: (
-                <span>
-                  <Trans
-                    defaults="New document in <em>{{ collectionName }}</em>"
-                    values={{
-                      collectionName,
-                    }}
-                    components={{
-                      em: <strong />,
-                    }}
-                  />
-                </span>
-              ),
-              to: newDocumentPath(document.collectionId),
-            },
-            {
-              type: "route",
-              title: (
-                <span>
-                  <Trans
-                    defaults="New document in <em>{{ collectionName }}</em>"
-                    values={{
-                      collectionName: document.title,
-                    }}
-                    components={{
-                      em: <strong />,
-                    }}
-                  />
-                </span>
-              ),
-              to: newDocumentPath(document.collectionId, {
-                parentDocumentId: document.id,
-              }),
-            },
-          ]}
-        />
+        <Template {...menu} items={items} />
      </ContextMenu>
    </>
  );
--- a/app/utils/routeHelpers.ts
+++ b/app/utils/routeHelpers.ts
@@ -88,13 +88,16 @@ export function newTemplatePath(collectionId: string) {
 export function newDocumentPath(
  collectionId?: string | null,
  params: {
-    parentDocumentId?: string;
    templateId?: string;
  } = {}
 ): string {
  return collectionId
    ? `/collection/${collectionId}/new?${queryString.stringify(params)}`
-    : `/doc/new`;
+    : `/doc/new?${queryString.stringify(params)}`;
+}
+
+export function newNestedDocumentPath(parentDocumentId?: string): string {
+  return `/doc/new?${queryString.stringify({ parentDocumentId })}`;
 }

 export function searchPath(
--- a/server/commands/documentUpdater.ts
+++ b/server/commands/documentUpdater.ts
@@ -136,5 +136,9 @@ export default async function documentUpdater({
    });
  }

-  return document;
+  return await Document.findByPk(document.id, {
+    userId: user.id,
+    rejectOnEmpty: true,
+    transaction,
+  });
 }
--- a/server/policies/document.test.ts
+++ b/server/policies/document.test.ts
@@ -1,5 +1,9 @@
-import { CollectionPermission, UserRole } from "@shared/types";
-import { Document } from "@server/models";
+import {
+  CollectionPermission,
+  DocumentPermission,
+  UserRole,
+} from "@shared/types";
+import { Document, UserMembership } from "@server/models";
 import {
  buildUser,
  buildTeam,
@@ -28,6 +32,7 @@ describe("read_write collection", () => {
    expect(abilities.download).toEqual(true);
    expect(abilities.update).toEqual(true);
    expect(abilities.createChildDocument).toEqual(true);
+    expect(abilities.manageUsers).toEqual(true);
    expect(abilities.archive).toEqual(true);
    expect(abilities.delete).toEqual(true);
    expect(abilities.share).toEqual(true);
@@ -56,6 +61,7 @@ describe("read_write collection", () => {
    expect(abilities.download).toEqual(true);
    expect(abilities.update).toEqual(false);
    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
    expect(abilities.archive).toEqual(false);
    expect(abilities.delete).toEqual(false);
    expect(abilities.share).toEqual(false);
@@ -86,6 +92,7 @@ describe("read_write collection", () => {
    expect(abilities.download).toEqual(false);
    expect(abilities.update).toEqual(false);
    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
    expect(abilities.archive).toEqual(false);
    expect(abilities.delete).toEqual(false);
    expect(abilities.share).toEqual(false);
@@ -145,6 +152,7 @@ describe("read collection", () => {
    expect(abilities.download).toEqual(false);
    expect(abilities.update).toEqual(false);
    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
    expect(abilities.archive).toEqual(false);
    expect(abilities.delete).toEqual(false);
    expect(abilities.share).toEqual(false);
@@ -172,6 +180,7 @@ describe("private collection", () => {
    expect(abilities.download).toEqual(false);
    expect(abilities.update).toEqual(false);
    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
    expect(abilities.archive).toEqual(false);
    expect(abilities.delete).toEqual(false);
    expect(abilities.share).toEqual(false);
@@ -200,6 +209,7 @@ describe("private collection", () => {
    expect(abilities.download).toEqual(false);
    expect(abilities.update).toEqual(false);
    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
    expect(abilities.archive).toEqual(false);
    expect(abilities.delete).toEqual(false);
    expect(abilities.share).toEqual(false);
@@ -297,9 +307,121 @@ describe("archived document", () => {
    expect(abilities.unarchive).toEqual(true);
    expect(abilities.update).toEqual(false);
    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
    expect(abilities.archive).toEqual(false);
    expect(abilities.share).toEqual(false);
    expect(abilities.move).toEqual(false);
    expect(abilities.comment).toEqual(false);
  });
 });
+
+describe("read document", () => {
+  it("should allow read permissions for team member", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const doc = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    await UserMembership.create({
+      userId: user.id,
+      documentId: doc.id,
+      permission: DocumentPermission.Read,
+      createdById: user.id,
+    });
+
+    // reload to get membership
+    const document = await Document.findByPk(doc.id, { userId: user.id });
+    const abilities = serialize(user, document);
+    expect(abilities.read).toEqual(true);
+    expect(abilities.download).toEqual(true);
+    expect(abilities.subscribe).toEqual(true);
+    expect(abilities.unsubscribe).toEqual(true);
+    expect(abilities.comment).toEqual(true);
+    expect(abilities.update).toEqual(false);
+    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
+    expect(abilities.archive).toEqual(false);
+    expect(abilities.delete).toEqual(false);
+    expect(abilities.share).toEqual(false);
+    expect(abilities.move).toEqual(false);
+  });
+});
+
+describe("read_write document", () => {
+  it("should allow write permissions for team member", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const doc = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    await UserMembership.create({
+      userId: user.id,
+      documentId: doc.id,
+      permission: DocumentPermission.ReadWrite,
+      createdById: user.id,
+    });
+
+    // reload to get membership
+    const document = await Document.findByPk(doc.id, { userId: user.id });
+    const abilities = serialize(user, document);
+    expect(abilities.read).toEqual(true);
+    expect(abilities.download).toEqual(true);
+    expect(abilities.update).toEqual(true);
+    expect(abilities.delete).toEqual(true);
+    expect(abilities.subscribe).toEqual(true);
+    expect(abilities.unsubscribe).toEqual(true);
+    expect(abilities.comment).toEqual(true);
+    expect(abilities.createChildDocument).toEqual(false);
+    expect(abilities.manageUsers).toEqual(false);
+    expect(abilities.archive).toEqual(false);
+    expect(abilities.share).toEqual(false);
+    expect(abilities.move).toEqual(false);
+  });
+});
+
+describe("manage document", () => {
+  it("should allow write permissions, user management, and sub-document creation", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const doc = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    await UserMembership.create({
+      userId: user.id,
+      documentId: doc.id,
+      permission: DocumentPermission.Admin,
+      createdById: user.id,
+    });
+
+    // reload to get membership
+    const document = await Document.findByPk(doc.id, { userId: user.id });
+    const abilities = serialize(user, document);
+    expect(abilities.read).toEqual(true);
+    expect(abilities.download).toEqual(true);
+    expect(abilities.update).toEqual(true);
+    expect(abilities.delete).toEqual(true);
+    expect(abilities.subscribe).toEqual(true);
+    expect(abilities.unsubscribe).toEqual(true);
+    expect(abilities.comment).toEqual(true);
+    expect(abilities.createChildDocument).toEqual(true);
+    expect(abilities.manageUsers).toEqual(true);
+    expect(abilities.archive).toEqual(true);
+    expect(abilities.move).toEqual(true);
+    expect(abilities.share).toEqual(false);
+  });
+});
--- a/server/policies/document.ts
+++ b/server/policies/document.ts
@@ -129,7 +129,10 @@ allow(User, ["move", "duplicate", "manageUsers"], Document, (actor, document) =>
 allow(User, "createChildDocument", Document, (actor, document) =>
  and(
    can(actor, "update", document),
-    can(actor, "read", document?.collection),
+    or(
+      includesMembership(document, [DocumentPermission.Admin]),
+      can(actor, "read", document?.collection)
+    ),
    !document?.isDraft,
    !document?.template,
    !actor.isGuest
@@ -181,10 +184,8 @@ allow(User, ["restore", "permanentDelete"], Document, (actor, document) =>
        DocumentPermission.ReadWrite,
        DocumentPermission.Admin,
      ]),
-      or(
-        can(actor, "updateDocument", document?.collection),
-        and(!!document?.isDraft && actor.id === document?.createdById)
-      ),
+      can(actor, "updateDocument", document?.collection),
+      and(!!document?.isDraft && actor.id === document?.createdById),
      !document?.collection
    )
  )
@@ -197,7 +198,10 @@ allow(User, "archive", Document, (actor, document) =>
    !document?.isDraft,
    !!document?.isActive,
    can(actor, "update", document),
-    can(actor, "updateDocument", document?.collection)
+    or(
+      includesMembership(document, [DocumentPermission.Admin]),
+      can(actor, "updateDocument", document?.collection)
+    )
  )
 );

@@ -208,20 +212,15 @@ allow(User, "unarchive", Document, (actor, document) =>
    !document?.isDraft,
    !document?.isDeleted,
    !!document?.archivedAt,
-    and(
-      can(actor, "read", document),
-      or(
-        includesMembership(document, [
-          DocumentPermission.ReadWrite,
-          DocumentPermission.Admin,
-        ]),
-        or(
-          can(actor, "updateDocument", document?.collection),
-          and(!!document?.isDraft && actor.id === document?.createdById)
-        )
-      )
-    ),
-    can(actor, "updateDocument", document?.collection)
+    can(actor, "read", document),
+    or(
+      includesMembership(document, [
+        DocumentPermission.ReadWrite,
+        DocumentPermission.Admin,
+      ]),
+      can(actor, "updateDocument", document?.collection),
+      and(!!document?.isDraft && actor.id === document?.createdById)
+    )
  )
 );

--- a/server/routes/api/documents/documents.test.ts
+++ b/server/routes/api/documents/documents.test.ts
@@ -2537,11 +2537,12 @@ describe("#documents.restore", () => {

  it("should not allow restore of trashed documents to collection user cannot access", async () => {
    const user = await buildUser();
+    const collection = await buildCollection();
    const document = await buildDocument({
      userId: user.id,
      teamId: user.teamId,
+      collectionId: collection.id,
    });
-    const collection = await buildCollection();
    await document.destroy();
    const res = await server.post("/api/documents.restore", {
      body: {
@@ -2772,14 +2773,9 @@ describe("#documents.create", () => {
  it("should fail for invalid parentDocumentId", async () => {
    const team = await buildTeam();
    const user = await buildUser({ teamId: team.id });
-    const collection = await buildCollection({
-      userId: user.id,
-      teamId: team.id,
-    });
    const res = await server.post("/api/documents.create", {
      body: {
        token: user.getJwtToken(),
-        collectionId: collection.id,
        parentDocumentId: "invalid",
        title: "new document",
        text: "hello",
@@ -2834,7 +2830,7 @@ describe("#documents.create", () => {
    expect(body.data.collectionId).toBeNull();
  });

-  it("should not allow creating a template without a collection", async () => {
+  it("should not allow creating a template with a collection", async () => {
    const team = await buildTeam();
    const user = await buildUser({ teamId: team.id });
    const res = await server.post("/api/documents.create", {
@@ -2867,15 +2863,18 @@ describe("#documents.create", () => {
    const body = await res.json();

    expect(res.status).toEqual(400);
-    expect(body.message).toBe("collectionId is required to publish");
+    expect(body.message).toBe(
+      "collectionId or parentDocumentId is required to publish"
+    );
  });

-  it("should not allow creating a nested doc without a collection", async () => {
+  it("should not allow creating a nested doc with 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(),
+        collectionId: "d7a4eb73-fac1-4028-af45-d7e34d54db8e",
        parentDocumentId: "d7a4eb73-fac1-4028-af45-d7e34d54db8e",
        title: "nested doc",
        text: "nested doc without collection",
@@ -2885,7 +2884,7 @@ describe("#documents.create", () => {

    expect(res.status).toEqual(400);
    expect(body.message).toBe(
-      "collectionId is required to create a nested document"
+      "collectionId is inferred when creating a nested document"
    );
  });

@@ -2947,7 +2946,6 @@ describe("#documents.create", () => {
    const res = await server.post("/api/documents.create", {
      body: {
        token: user.getJwtToken(),
-        collectionId: collection.id,
        parentDocumentId: document.id,
        title: "new document",
        text: "hello",
@@ -2963,14 +2961,9 @@ describe("#documents.create", () => {
  it("should error with invalid parentDocument", async () => {
    const team = await buildTeam();
    const user = await buildUser({ teamId: team.id });
-    const collection = await buildCollection({
-      userId: user.id,
-      teamId: team.id,
-    });
    const res = await server.post("/api/documents.create", {
      body: {
        token: user.getJwtToken(),
-        collectionId: collection.id,
        parentDocumentId: "d7a4eb73-fac1-4028-af45-d7e34d54db8e",
        title: "new document",
        text: "hello",
@@ -2996,7 +2989,6 @@ describe("#documents.create", () => {
    const res = await server.post("/api/documents.create", {
      body: {
        token: user.getJwtToken(),
-        collectionId: collection.id,
        parentDocumentId: document.id,
        title: "new document",
        text: "hello",
--- a/server/routes/api/documents/documents.ts
+++ b/server/routes/api/documents/documents.ts
@@ -675,10 +675,6 @@ router.post(
      );
    }

-    if (document.collection) {
-      authorize(user, "updateDocument", collection);
-    }
-
    if (document.deletedAt) {
      authorize(user, "restore", document);
      // restore a previously deleted document
@@ -1023,7 +1019,19 @@ router.post(
          method: ["withMembership", user.id],
        }).findByPk(collectionId!, { transaction });
      }
-      authorize(user, "createDocument", collection);
+
+      if (document.parentDocumentId) {
+        const parentDocument = await Document.findByPk(
+          document.parentDocumentId,
+          {
+            userId: user.id,
+            transaction,
+          }
+        );
+        authorize(user, "createChildDocument", parentDocument, { collection });
+      } else {
+        authorize(user, "createDocument", collection);
+      }
    }

    await documentUpdater({
@@ -1038,18 +1046,9 @@ router.post(
      ip: ctx.request.ip,
    });

-    collection = document.collectionId
-      ? await Collection.scope({
-          method: ["withMembership", user.id],
-        }).findByPk(document.collectionId, { transaction })
-      : null;
-
-    document.updatedBy = user;
-    document.collection = collection;
-
    ctx.body = {
      data: await presentDocument(ctx, document),
-      policies: presentPolicies(user, [document, collection]),
+      policies: presentPolicies(user, [document]),
    };
  }
 );
@@ -1395,7 +1394,29 @@ router.post(

    let collection;

-    if (collectionId) {
+    let parentDocument;
+
+    if (parentDocumentId) {
+      parentDocument = await Document.findByPk(parentDocumentId, {
+        userId: user.id,
+      });
+
+      if (parentDocument?.collectionId) {
+        collection = await Collection.scope({
+          method: ["withMembership", user.id],
+        }).findOne({
+          where: {
+            id: parentDocument.collectionId,
+            teamId: user.teamId,
+          },
+          transaction,
+        });
+      }
+
+      authorize(user, "createChildDocument", parentDocument, {
+        collection,
+      });
+    } else if (collectionId) {
      collection = await Collection.scope({
        method: ["withMembership", user.id],
      }).findOne({
@@ -1408,17 +1429,6 @@ router.post(
      authorize(user, "createDocument", collection);
    }

-    let parentDocument;
-
-    if (parentDocumentId) {
-      parentDocument = await Document.findByPk(parentDocumentId, {
-        userId: user.id,
-      });
-      authorize(user, "read", parentDocument, {
-        collection,
-      });
-    }
-
    let templateDocument: Document | null | undefined;

    if (templateId) {
@@ -1435,7 +1445,7 @@ router.post(
      emoji,
      createdAt,
      publish,
-      collectionId,
+      collectionId: collection?.id,
      parentDocumentId,
      templateDocument,
      template,
--- a/server/routes/api/documents/schema.ts
+++ b/server/routes/api/documents/schema.ts
@@ -348,15 +348,23 @@ export const DocumentsCreateSchema = BaseSchema.extend({
    template: z.boolean().optional(),
  }),
 })
-  .refine((req) => !(req.body.parentDocumentId && !req.body.collectionId), {
-    message: "collectionId is required to create a nested document",
+  .refine((req) => !req.body.parentDocumentId || !req.body.collectionId, {
+    message: "collectionId is inferred when creating a nested document",
  })
  .refine((req) => !(req.body.template && !req.body.collectionId), {
    message: "collectionId is required to create a template document",
  })
-  .refine((req) => !(req.body.publish && !req.body.collectionId), {
-    message: "collectionId is required to publish",
-  });
+  .refine(
+    (req) =>
+      !(
+        req.body.publish &&
+        !req.body.parentDocumentId &&
+        !req.body.collectionId
+      ),
+    {
+      message: "collectionId or parentDocumentId is required to publish",
+    }
+  );

 export type DocumentsCreateReq = z.infer<typeof DocumentsCreateSchema>;

--- a/shared/i18n/locales/en_US/translation.json
+++ b/shared/i18n/locales/en_US/translation.json
@@ -466,8 +466,8 @@
  "Delete group": "Delete group",
  "Group options": "Group options",
  "Member options": "Member options",
-  "New child document": "New child document",
  "New document in <em>{{ collectionName }}</em>": "New document in <em>{{ collectionName }}</em>",
+  "New child document": "New child document",
  "Notification settings": "Notification settings",
  "Revision options": "Revision options",
  "Share link revoked": "Share link revoked",