feat: reordering documents in collection (#1722)

* tweaking effect details

* wrap work on this feature

* adds correct color to drop cursor

* simplify logic for early return

* much better comment so Tom doesn't fire me

* feat: Allow changing sort order of collections

* refactor: Move validation to model
feat: Make custom order the default (in prep for dnd)

* feat: Add sort choice to edit collection modal
fix: Improved styling of generic InputSelect

* fix: Vertical space left after removing previous collection description

* chore: Tweak language, menu contents, add auto-disclosure on sub menus

* only show drop-to-reorder cursor when sort is set to manual

Co-authored-by: Tom Moor <tom.moor@gmail.com>

server/api/collections.js

@@ -30,7 +30,13 @@ const { authorize } = policy;
 const router = new Router();
 
 router.post("collections.create", auth(), async (ctx) => {
-  const { name, color, description, icon } = ctx.body;
+  const {
+    name,
+    color,
+    description,
+    icon,
+    sort = Collection.DEFAULT_SORT,
+  } = ctx.body;
   const isPrivate = ctx.body.private;
   ctx.assertPresent(name, "name is required");
 
@@ -49,6 +55,7 @@ router.post("collections.create", auth(), async (ctx) => {
     teamId: user.teamId,
     creatorId: user.id,
     private: isPrivate,
+    sort,
   });
 
   await Event.create({
@@ -445,16 +452,14 @@ router.post("collections.export_all", auth(), async (ctx) => {
 });
 
 router.post("collections.update", auth(), async (ctx) => {
-  const { id, name, description, icon, color } = ctx.body;
+  let { id, name, description, icon, color, sort } = ctx.body;
   const isPrivate = ctx.body.private;
-  ctx.assertPresent(name, "name is required");
 
   if (color) {
     ctx.assertHexColor(color, "Invalid hex value (please use format #FFFFFF)");
   }
 
   const user = ctx.state.user;
-
   const collection = await Collection.scope({
     method: ["withMembership", user.id],
   }).findByPk(id);
@@ -478,11 +483,24 @@ router.post("collections.update", auth(), async (ctx) => {
 
   const isPrivacyChanged = isPrivate !== collection.private;
 
-  collection.name = name;
-  collection.description = description;
-  collection.icon = icon;
-  collection.color = color;
-  collection.private = isPrivate;
+  if (name !== undefined) {
+    collection.name = name;
+  }
+  if (description !== undefined) {
+    collection.description = description;
+  }
+  if (icon !== undefined) {
+    collection.icon = icon;
+  }
+  if (color !== undefined) {
+    collection.color = color;
+  }
+  if (isPrivate !== undefined) {
+    collection.private = isPrivate;
+  }
+  if (sort !== undefined) {
+    collection.sort = sort;
+  }
 
   await collection.save();
 

server/api/collections.test.js

@@ -864,6 +864,8 @@ describe("#collections.create", () => {
     expect(res.status).toEqual(200);
     expect(body.data.id).toBeTruthy();
     expect(body.data.name).toBe("Test");
+    expect(body.data.sort.field).toBe("index");
+    expect(body.data.sort.direction).toBe("asc");
     expect(body.policies.length).toBe(1);
     expect(body.policies[0].abilities.read).toBeTruthy();
     expect(body.policies[0].abilities.export).toBeTruthy();
@@ -916,6 +918,29 @@ describe("#collections.update", () => {
     expect(body.policies.length).toBe(1);
   });
 
+  it("allows editing sort", async () => {
+    const { user, collection } = await seed();
+    const sort = { field: "index", direction: "desc" };
+    const res = await server.post("/api/collections.update", {
+      body: { token: user.getJwtToken(), id: collection.id, sort },
+    });
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+    expect(body.data.sort.field).toBe("index");
+    expect(body.data.sort.direction).toBe("desc");
+  });
+
+  it("allows editing individual fields", async () => {
+    const { user, collection } = await seed();
+    const res = await server.post("/api/collections.update", {
+      body: { token: user.getJwtToken(), id: collection.id, private: true },
+    });
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+    expect(body.data.private).toBe(true);
+    expect(body.data.name).toBe(collection.name);
+  });
+
   it("allows editing from non-private to private collection", async () => {
     const { user, collection } = await seed();
     const res = await server.post("/api/collections.update", {
@@ -1027,6 +1052,24 @@ describe("#collections.update", () => {
     });
     expect(res.status).toEqual(403);
   });
+
+  it("does not allow setting unknown sort fields", async () => {
+    const { user, collection } = await seed();
+    const sort = { field: "blah", direction: "desc" };
+    const res = await server.post("/api/collections.update", {
+      body: { token: user.getJwtToken(), id: collection.id, sort },
+    });
+    expect(res.status).toEqual(400);
+  });
+
+  it("does not allow setting unknown sort directions", async () => {
+    const { user, collection } = await seed();
+    const sort = { field: "title", direction: "blah" };
+    const res = await server.post("/api/collections.update", {
+      body: { token: user.getJwtToken(), id: collection.id, sort },
+    });
+    expect(res.status).toEqual(400);
+  });
 });
 
 describe("#collections.delete", () => {

server/commands/documentMover.js

@@ -6,7 +6,7 @@ export default async function documentMover({
   user,
   document,
   collectionId,
-  parentDocumentId,
+  parentDocumentId = null, // convert undefined to null so parentId comparison treats them as equal
   index,
   ip,
 }: {
@@ -42,12 +42,24 @@ export default async function documentMover({
         transaction,
         paranoid: false,
       });
-      const documentJson = await collection.removeDocumentInStructure(
-        document,
-        {
-          save: false,
-        }
-      );
+      const [
+        documentJson,
+        fromIndex,
+      ] = await collection.removeDocumentInStructure(document, {
+        save: false,
+      });
+
+      // if we're reordering from within the same parent
+      // the original and destination collection are the same,
+      // so when the initial item is removed above, the list will reduce by 1.
+      // We need to compensate for this when reordering
+      const toIndex =
+        index !== undefined &&
+        document.parentDocumentId === parentDocumentId &&
+        document.collectionId === collectionId &&
+        fromIndex < index
+          ? index - 1
+          : index;
 
       // if the collection is the same then it will get saved below, this
       // line prevents a pointless intermediate save from occurring.
@@ -62,7 +74,7 @@ export default async function documentMover({
       const newCollection: Collection = collectionChanged
         ? await Collection.findByPk(collectionId, { transaction })
         : collection;
-      await newCollection.addDocumentToStructure(document, index, {
+      await newCollection.addDocumentToStructure(document, toIndex, {
         documentJson,
       });
       result.collections.push(collection);

server/middlewares/validation.js

@@ -37,7 +37,7 @@ export default function validation() {
     };
 
     ctx.assertPositiveInteger = (value, message) => {
-      if (!validator.isInt(value, { min: 0 })) {
+      if (!validator.isInt(String(value), { min: 0 })) {
         throw new ValidationError(message);
       }
     };

server/migrations/20201230031607-collection-sort.js

@@ -0,0 +1,14 @@
+'use strict';
+
+module.exports = {
+  up: async (queryInterface, Sequelize) => {
+    await queryInterface.addColumn('collections', 'sort', {
+      type: Sequelize.JSONB,
+      allowNull: true
+    });
+  },
+
+  down: async (queryInterface, Sequelize) => {
+    await queryInterface.removeColumn('collections', 'sort');
+  }
+};

server/models/Collection.js

@@ -1,5 +1,5 @@
 // @flow
-import { find, concat, remove, uniq } from "lodash";
+import { find, findIndex, concat, remove, uniq } from "lodash";
 import randomstring from "randomstring";
 import slug from "slug";
 import { DataTypes, sequelize } from "../sequelize";
@@ -24,6 +24,27 @@ const Collection = sequelize.define(
     private: DataTypes.BOOLEAN,
     maintainerApprovalRequired: DataTypes.BOOLEAN,
     documentStructure: DataTypes.JSONB,
+    sort: {
+      type: DataTypes.JSONB,
+      validate: {
+        isSort(value) {
+          if (
+            typeof value !== "object" ||
+            !value.direction ||
+            !value.field ||
+            Object.keys(value).length !== 2
+          ) {
+            throw new Error("Sort must be an object with field,direction");
+          }
+          if (!["asc", "desc"].includes(value.direction)) {
+            throw new Error("Sort direction must be one of asc,desc");
+          }
+          if (!["title", "index"].includes(value.field)) {
+            throw new Error("Sort field must be one of title,index");
+          }
+        },
+      },
+    },
   },
   {
     tableName: "collections",
@@ -41,6 +62,11 @@ const Collection = sequelize.define(
   }
 );
 
+Collection.DEFAULT_SORT = {
+  field: "index",
+  direction: "asc",
+};
+
 Collection.addHook("beforeSave", async (model) => {
   if (model.icon === "collection") {
     model.icon = null;
@@ -350,7 +376,7 @@ Collection.prototype.removeDocumentInStructure = async function (
 
       const match = find(children, { id });
       if (match) {
-        if (!returnValue) returnValue = match;
+        if (!returnValue) returnValue = [match, findIndex(children, { id })];
         remove(children, { id });
       }
 

server/models/Team.js

@@ -145,6 +145,7 @@ Team.prototype.provisionFirstCollection = async function (userId) {
       "This collection is a quick guide to what Outline is all about. Feel free to delete this collection once your team is up to speed with the basics!",
     teamId: this.id,
     creatorId: userId,
+    sort: Collection.DEFAULT_SORT,
   });
 
   // For the first collection we go ahead and create some intitial documents to get

server/presenters/collection.js

@@ -9,12 +9,14 @@ type Document = {
   url: string,
 };
 
-const sortDocuments = (documents: Document[]): Document[] => {
-  const orderedDocs = naturalSort(documents, "title");
+const sortDocuments = (documents: Document[], sort): Document[] => {
+  const orderedDocs = naturalSort(documents, sort.field, {
+    direction: sort.direction,
+  });
 
   return orderedDocs.map((document) => ({
     ...document,
-    children: sortDocuments(document.children),
+    children: sortDocuments(document.children, sort),
   }));
 };
 
@@ -24,17 +26,26 @@ export default function present(collection: Collection) {
     url: collection.url,
     name: collection.name,
     description: collection.description,
+    sort: collection.sort,
     icon: collection.icon,
     color: collection.color || "#4E5C6E",
     private: collection.private,
     createdAt: collection.createdAt,
     updatedAt: collection.updatedAt,
     deletedAt: collection.deletedAt,
-    documents: undefined,
+    documents: collection.documentStructure || [],
   };
 
-  // Force alphabetical sorting
-  data.documents = sortDocuments(collection.documentStructure);
+  // Handle the "sort" field being empty here for backwards compatability
+  if (!data.sort) {
+    data.sort = { field: "title", direction: "asc" };
+  }
+
+  // "index" field is manually sorted and is represented by the documentStructure
+  // already saved in the database, no further sort is needed
+  if (data.sort.field !== "index") {
+    data.documents = sortDocuments(collection.documentStructure, data.sort);
+  }
 
   return data;
 }