From 0fb54edda879b26a32d89379d9aaa3cea93a6093 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 25 Apr 2024 10:39:21 +0800 Subject: [PATCH] FIX: Make `links` and `icons` setting migration more resilient 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 7e304e0a12115c6596fc64ff173368cba1a28e7b 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. --- .../settings/0001-migrate-links-setting.js | 32 ++++++++----------- .../settings/0002-migrate-icons-setting.js | 32 ++++++++----------- .../0001-migrate-links-setting-test.js | 21 ++++++++++++ .../0002-migrate-icons-setting-test.js | 26 ++++++++++++++- 4 files changed, 74 insertions(+), 37 deletions(-) diff --git a/migrations/settings/0001-migrate-links-setting.js b/migrations/settings/0001-migrate-links-setting.js index 11d17ac..462afac 100644 --- a/migrations/settings/0001-migrate-links-setting.js +++ b/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()); - - if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) { - target = "_blank"; - } + const newLinks = []; - const newLink = { - name, - url, - target - } + oldSetting.split("|").forEach((link) => { + let [name, url, target] = link.split(",").map((s) => s.trim()); - Object.keys(newLink).forEach((key) => { - if (newLink[key] === undefined) { - delete newLink[key]; + if (name && url) { + if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) { + target = "_blank"; } - }); - return newLink; - }) + newLinks.push({ + name, + url, + target, + }); + } + }); - settings.set("links", newSetting); + settings.set("links", newLinks); } return settings; diff --git a/migrations/settings/0002-migrate-icons-setting.js b/migrations/settings/0002-migrate-icons-setting.js index 9693e9b..580cf97 100644 --- a/migrations/settings/0002-migrate-icons-setting.js +++ b/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; diff --git a/test/unit/migrations/settings/0001-migrate-links-setting-test.js b/test/unit/migrations/settings/0001-migrate-links-setting-test.js index e4b27fc..c78a36f 100644 --- a/test/unit/migrations/settings/0001-migrate-links-setting-test.js +++ b/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({ diff --git a/test/unit/migrations/settings/0002-migrate-icons-setting-test.js b/test/unit/migrations/settings/0002-migrate-icons-setting-test.js index 1ec7cc9..cad7538 100644 --- a/test/unit/migrations/settings/0002-migrate-icons-setting-test.js +++ b/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()) + ); }); } );