Fix incorrect slugs for collections (#16296)

frontend/src/metabase/collections/utils.js

@@ -11,6 +11,7 @@ function preparePersonalCollection(c) {
   return {
     ...c,
     name: t`Your personal collection`,
+    originalName: c.name,
   };
 }
 

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);
 }
 

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),
         );