FIX: Make links and icons setting migration more resilient (#39)

This commit updates the migrations to assume that the old setting value
may be invalid so that we don't end up with a new setting which fails
the objects schema validation.

This is a follow up to 7e304e0 which
added migrations for the `links` and `icons` settings. We have
gotten reports of errors happening with the migrations because the value of the old
setting before migration did not follow the "informal" format required
previously. Since there was nothing enforcing the format for the value
previously, we need to assume the worst possible cases in our
migrations.

migrations/settings/0001-migrate-links-setting.js

@@ -2,29 +2,25 @@ export default function migrate(settings) {
   const oldSetting = settings.get("links");
 
   if (oldSetting) {
-    const newSetting = oldSetting.split("|").map((link) => {
-      let [name, url, target] = link.split(",").map((s) => s.trim());
+    const newLinks = [];
 
-      if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) {
-        target = "_blank";
-      }
-
-      const newLink = {
-        name,
-        url,
-        target
-      }
+    oldSetting.split("|").forEach((link) => {
+      let [text, url, target] = link.split(",").map((s) => s.trim());
 
-      Object.keys(newLink).forEach((key) => {
-        if (newLink[key] === undefined) {
-          delete newLink[key];
+      if (text && url) {
+        if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) {
+          target = "_blank";
         }
-      });
 
-      return newLink;
-    })
+        newLinks.push({
+          text,
+          url,
+          target,
+        });
+      }
+    });
 
-    settings.set("links", newSetting);
+    settings.set("links", newLinks);
   }
 
   return settings;

migrations/settings/0002-migrate-icons-setting.js

@@ -2,29 +2,25 @@ export default function migrate(settings) {
   const oldSetting = settings.get("icons");
 
   if (oldSetting) {
-    const newSetting = oldSetting.split("|").map((link) => {
-      let [iconName, url, target] = link.split(",").map((s) => s.trim());
-
-      if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) {
-        target = "_blank";
-      }
+    const newIcons = [];
 
-      const newLink = {
-        icon_name: iconName,
-        url,
-        target
-      }
+    oldSetting.split("|").map((link) => {
+      let [iconName, url, target] = link.split(",").map((s) => s.trim());
 
-      Object.keys(newLink).forEach((key) => {
-        if (newLink[key] === undefined) {
-          delete newLink[key];
+      if (iconName && url) {
+        if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) {
+          target = "_blank";
         }
-      });
 
-      return newLink;
-    })
+        newIcons.push({
+          icon_name: iconName,
+          url,
+          target,
+        });
+      }
+    });
 
-    settings.set("icons", newSetting);
+    settings.set("icons", newIcons);
   }
 
   return settings;

test/unit/migrations/settings/0001-migrate-links-setting-test.js

@@ -4,6 +4,27 @@ import migrate from "../../../../migrations/settings/0001-migrate-links-setting"
 module(
   "Unit | Migrations | Settings | 0001-migrate-links-setting",
   function () {
+    test("migrate when old setting is of an invalid format", function (assert) {
+      const settings = new Map(
+        Object.entries({
+          links: "some||another",
+        })
+      );
+
+      const result = migrate(settings);
+
+      const expectedResult = new Map(
+        Object.entries({
+          links: [],
+        })
+      );
+
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
+    });
+
     test("migrate", function (assert) {
       const settings = new Map(
         Object.entries({
@@ -18,17 +39,17 @@ module(
         Object.entries({
           links: [
             {
-              name: "some",
+              text: "some",
               url: "links",
               target: "_blank",
             },
             {
-              name: "another",
+              text: "another",
               url: "link",
               target: "_blank",
             },
             {
-              name: "link2",
+              text: "link2",
               url: "/some/path",
               target: "_blank",
             },

test/unit/migrations/settings/0002-migrate-icons-setting-test.js

@@ -4,6 +4,27 @@ import migrate from "../../../../migrations/settings/0002-migrate-icons-setting"
 module(
   "Unit | Migrations | Settings | 0002-migrate-icons-setting",
   function () {
+    test("migrate when old setting value is of an invalid format", function (assert) {
+      const settings = new Map(
+        Object.entries({
+          icons: "some||another",
+        })
+      );
+
+      const result = migrate(settings);
+
+      const expectedResult = new Map(
+        Object.entries({
+          icons: [],
+        })
+      );
+
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
+    });
+
     test("migrate", function (assert) {
       const settings = new Map(
         Object.entries({
@@ -36,7 +57,10 @@ module(
         })
       );
 
-      assert.deepEqual(result, expectedResult);
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
     });
   }
 );