From a26f2242a15cf861e01d6269e5c316cb0ff862f3 Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Wed, 2 Jun 2021 14:50:10 +0300
Subject: [PATCH] Fix incorrect slugs for collections (#16296)

---
 frontend/src/metabase/collections/utils.js   |  1 +
 frontend/src/metabase/lib/urls.js            | 36 +++++++++++---
 frontend/test/metabase/lib/urls.unit.spec.js | 51 +++++++++++++++++---
 3 files changed, 75 insertions(+), 13 deletions(-)

diff --git a/frontend/src/metabase/collections/utils.js b/frontend/src/metabase/collections/utils.js
index 8ccaa58c857..7735f6d31c2 100644
--- a/frontend/src/metabase/collections/utils.js
+++ b/frontend/src/metabase/collections/utils.js
@@ -11,6 +11,7 @@ function preparePersonalCollection(c) {
   return {
     ...c,
     name: t`Your personal collection`,
+    originalName: c.name,
   };
 }
 
diff --git a/frontend/src/metabase/lib/urls.js b/frontend/src/metabase/lib/urls.js
index af13ada31c5..8a562280ffc 100644
--- a/frontend/src/metabase/lib/urls.js
+++ b/frontend/src/metabase/lib/urls.js
@@ -124,18 +124,40 @@ export function tableRowsQuery(databaseId, tableId, metricId, segmentId) {
   return question(null, query);
 }
 
+function slugifyPersonalCollection(collection) {
+  // Current user's personal collection name is replaced with "Your personal collection"
+  // `originalName` keeps the original name like "John Doe's Personal Collection"
+  const name = collection.originalName || collection.name;
+
+  // Will keep single quote characters,
+  // so "John's" can be converted to `john-s` instead of `johns`
+  let slug = slugg(name, {
+    toStrip: /["”“]/g,
+  });
+
+  // If can't build a slug out of user's name (e.g. if it contains only emojis)
+  // removes the "s-" part of a slug
+  if (slug === "s-personal-collection") {
+    slug = slug.replace("s-", "");
+  }
+
+  return slug;
+}
+
 export function collection(collection) {
-  if (
-    !collection ||
-    collection.id === null ||
-    typeof collection.id === "string"
-  ) {
+  const isSystemCollection =
+    !collection || collection.id === null || typeof collection.id === "string";
+
+  if (isSystemCollection) {
     const id = collection && collection.id ? collection.id : "root";
     return `/collection/${id}`;
   }
-  const slug = collection.slug
-    ? collection.slug.split("_").join("-")
+
+  const isPersonalCollection = typeof collection.personal_owner_id === "number";
+  const slug = isPersonalCollection
+    ? slugifyPersonalCollection(collection)
     : slugg(collection.name);
+
   return appendSlug(`/collection/${collection.id}`, slug);
 }
 
diff --git a/frontend/test/metabase/lib/urls.unit.spec.js b/frontend/test/metabase/lib/urls.unit.spec.js
index ecdd23a7c6a..c1e1fd797d4 100644
--- a/frontend/test/metabase/lib/urls.unit.spec.js
+++ b/frontend/test/metabase/lib/urls.unit.spec.js
@@ -78,10 +78,6 @@ describe("urls", () => {
     });
 
     it("returns correct url", () => {
-      expect(collection({ id: 1, slug: "first_collection" })).toBe(
-        "/collection/1-first-collection",
-      );
-
       expect(collection({ id: 1, name: "First collection" })).toBe(
         "/collection/1-first-collection",
       );
@@ -94,6 +90,37 @@ describe("urls", () => {
         }),
       ).toBe("/collection/1-first-collection");
     });
+
+    it("handles possessives correctly", () => {
+      expect(
+        collection({
+          id: 1,
+          name: "John Doe's Personal Collection",
+          personal_owner_id: 1,
+        }),
+      ).toBe("/collection/1-john-doe-s-personal-collection");
+    });
+
+    it("omits possessive form if name can't be turned into a slug", () => {
+      expect(
+        collection({
+          id: 1,
+          name: "🍎's Personal Collection",
+          personal_owner_id: 1,
+        }),
+      ).toBe("/collection/1-personal-collection");
+    });
+
+    it("uses originalName to build a slug if present", () => {
+      expect(
+        collection({
+          id: 1,
+          name: "Your personal collection",
+          originalName: "John Doe's Personal Collection",
+          personal_owner_id: 1,
+        }),
+      ).toBe("/collection/1-john-doe-s-personal-collection");
+    });
   });
 
   describe("extractEntityId", () => {
@@ -212,16 +239,28 @@ describe("urls", () => {
         return slug ? `${path}-${slug}` : path;
       }
 
-      it(`should handle ${caseName} correctly`, () => {
+      it(`should handle ${caseName} correctly for database browse URLs`, () => {
         expect(browseDatabase(entity)).toBe(
           expectedUrl("/browse/1", expectedString),
         );
-        expect(collection(entity)).toBe(
+      });
+
+      it(`should handle ${caseName} correctly for collection URLs`, () => {
+        // collection objects have not transliterated slugs separated by underscores
+        // this makes sure they don't affect the slug builder
+        const collectionOwnSlug = entity.name.split(" ").join("_");
+        expect(collection({ ...entity, slug: collectionOwnSlug })).toBe(
           expectedUrl("/collection/1", expectedString),
         );
+      });
+
+      it(`should handle ${caseName} correctly for dashboard URLs`, () => {
         expect(dashboard(entity)).toBe(
           expectedUrl("/dashboard/1", expectedString),
         );
+      });
+
+      it(`should handle ${caseName} correctly for question URLs`, () => {
         expect(question(entity)).toBe(
           expectedUrl("/question/1", expectedString),
         );
-- 
GitLab