fix: Use friendly urls for collections (#2162)

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

server/models/Collection.js

@@ -1,13 +1,13 @@
 // @flow
 import { find, findIndex, concat, remove, uniq } from "lodash";
 import randomstring from "randomstring";
-import slug from "slug";
+import isUUID from "validator/lib/isUUID";
+import { SLUG_URL_REGEX } from "../../shared/utils/routeHelpers";
 import { Op, DataTypes, sequelize } from "../sequelize";
+import slugify from "../utils/slugify";
 import CollectionUser from "./CollectionUser";
 import Document from "./Document";
 
-slug.defaults.mode = "rfc3986";
-
 const Collection = sequelize.define(
   "collection",
   {
@@ -72,7 +72,9 @@ const Collection = sequelize.define(
     },
     getterMethods: {
       url() {
-        return `/collections/${this.id}`;
+        if (!this.name) return `/collection/untitled-${this.urlId}`;
+
+        return `/collection/${slugify(this.name)}-${this.urlId}`;
       },
     },
   }
@@ -223,6 +225,17 @@ Collection.addHook("afterCreate", (model: Collection, options) => {
 
 // Class methods
 
+Collection.findByPk = async function (id, options = {}) {
+  if (isUUID(id)) {
+    return this.findOne({ where: { id }, ...options });
+  } else if (id.match(SLUG_URL_REGEX)) {
+    return this.findOne({
+      where: { urlId: id.match(SLUG_URL_REGEX)[1] },
+      ...options,
+    });
+  }
+};
+
 // get all the membership relationshps a user could have with the collection
 Collection.membershipUserIds = async (collectionId: string) => {
   const collection = await Collection.scope("withAllMemberships").findByPk(

server/models/Collection.test.js

@@ -1,4 +1,5 @@
 /* eslint-disable flowtype/require-valid-file-annotation */
+import randomstring from "randomstring";
 import { v4 as uuidv4 } from "uuid";
 import { Collection, Document } from "../models";
 import {
@@ -9,6 +10,7 @@ import {
   buildDocument,
 } from "../test/factories";
 import { flushdb, seed } from "../test/support";
+import slugify from "../utils/slugify";
 
 beforeEach(() => flushdb());
 beforeEach(jest.resetAllMocks);
@@ -16,7 +18,7 @@ beforeEach(jest.resetAllMocks);
 describe("#url", () => {
   test("should return correct url for the collection", () => {
     const collection = new Collection({ id: "1234" });
-    expect(collection.url).toBe("/collections/1234");
+    expect(collection.url).toBe(`/collection/untitled-${collection.urlId}`);
   });
 });
 
@@ -416,3 +418,53 @@ describe("#membershipUserIds", () => {
     expect(membershipUserIds.length).toBe(6);
   });
 });
+
+describe("#findByPk", () => {
+  test("should return collection with collection Id", async () => {
+    const collection = await buildCollection();
+    const response = await Collection.findByPk(collection.id);
+
+    expect(response.id).toBe(collection.id);
+  });
+
+  test("should return collection when urlId is present", async () => {
+    const collection = await buildCollection();
+    const id = `${slugify(collection.name)}-${collection.urlId}`;
+
+    const response = await Collection.findByPk(id);
+
+    expect(response.id).toBe(collection.id);
+  });
+
+  test("should return undefined when incorrect uuid type", async () => {
+    const collection = await buildCollection();
+    const response = await Collection.findByPk(collection.id + "-incorrect");
+
+    expect(response).toBe(undefined);
+  });
+
+  test("should return undefined when incorrect urlId length", async () => {
+    const collection = await buildCollection();
+    const id = `${slugify(collection.name)}-${collection.urlId}incorrect`;
+
+    const response = await Collection.findByPk(id);
+
+    expect(response).toBe(undefined);
+  });
+
+  test("should return null when no collection is found with uuid", async () => {
+    const response = await Collection.findByPk(
+      "a9e71a81-7342-4ea3-9889-9b9cc8f667da"
+    );
+
+    expect(response).toBe(null);
+  });
+
+  test("should return null when no collection is found with urlId", async () => {
+    const id = `${slugify("test collection")}-${randomstring.generate(15)}`;
+
+    const response = await Collection.findByPk(id);
+
+    expect(response).toBe(null);
+  });
+});

server/models/Document.js

@@ -7,6 +7,7 @@ import MarkdownSerializer from "slate-md-serializer";
 import isUUID from "validator/lib/isUUID";
 import { MAX_TITLE_LENGTH } from "../../shared/constants";
 import parseTitle from "../../shared/utils/parseTitle";
+import { SLUG_URL_REGEX } from "../../shared/utils/routeHelpers";
 import unescape from "../../shared/utils/unescape";
 import { Collection, User } from "../models";
 import { DataTypes, sequelize } from "../sequelize";
@@ -14,7 +15,6 @@ import slugify from "../utils/slugify";
 import Revision from "./Revision";
 
 const Op = Sequelize.Op;
-const URL_REGEX = /^[0-9a-zA-Z-_~]*-([a-zA-Z0-9]{10,15})$/;
 const serializer = new MarkdownSerializer();
 
 export const DOCUMENT_VERSION = 2;
@@ -216,10 +216,10 @@ Document.findByPk = async function (id, options = {}) {
       where: { id },
       ...options,
     });
-  } else if (id.match(URL_REGEX)) {
+  } else if (id.match(SLUG_URL_REGEX)) {
     return scope.findOne({
       where: {
-        urlId: id.match(URL_REGEX)[1],
+        urlId: id.match(SLUG_URL_REGEX)[1],
       },
       ...options,
     });

server/models/Document.test.js

@@ -6,7 +6,8 @@ import {
   buildTeam,
   buildUser,
 } from "../test/factories";
-import { flushdb } from "../test/support";
+import { flushdb, seed } from "../test/support";
+import slugify from "../utils/slugify";
 
 beforeEach(() => flushdb());
 beforeEach(jest.resetAllMocks);
@@ -307,3 +308,14 @@ describe("#delete", () => {
     expect(document.deletedAt).toBeTruthy();
   });
 });
+
+describe("#findByPk", () => {
+  test("should return document when urlId is correct", async () => {
+    const { document } = await seed();
+    const id = `${slugify(document.title)}-${document.urlId}`;
+
+    const response = await Document.findByPk(id);
+
+    expect(response.id).toBe(document.id);
+  });
+});