FIX: Ensure that custom header links migration do not fail validation (#55)

This commit is a follow up to 73747938bd
where the migration will fail because the objects created by the
migration will end up failing to save because the objects will fail the
validation given the new schema. This commit updates the migration to
ensure that we do not end up with invalid objects.

migrations/settings/0002-migrate-custom-header-links.js

@@ -1,32 +1,55 @@
 export default function migrate(settings) {
   const oldSetting = settings.get("custom_header_links");
 
+  // Do nothing if setting is already an array which means user has saved the setting in the new format
+  // This is required because there was a bug in core where this migration would fail but the theme was still updated
+  // allowing the users to save the setting in the new format.
+  if (Array.isArray(oldSetting)) {
+    return settings;
+  }
+
   if (oldSetting) {
-    const newSetting = oldSetting.split("|").map((link) => {
+    const newLinks = [];
-      const [text, title, url, view, target, hide_on_scroll, locale] = link
+
+    oldSetting.split("|").forEach((link) => {
+      const [text, title, url, view, target, hideOnScroll, locale] = link
         .split(",")
         .map((s) => s.trim());
 
-      const newLink = {
+      if (text && title && url) {
-        text,
+        const newLink = {
-        title,
+          text,
-        url,
+          title,
-        view,
+          url,
-        target,
+        };
-        hide_on_scroll,
-        locale,
-      };
 
-      Object.keys(newLink).forEach((key) => {
+        if (["vdm", "vdo", "vmo"].includes(view)) {
-        if (newLink[key] === undefined) {
+          newLink.view = view;
-          delete newLink[key];
+        } else {
+          newLink.view = "vdm";
         }
-      });
 
-      return newLink;
+        if (["blank", "self"].includes(target)) {
+          newLink.target = target;
+        } else {
+          newLink.target = "blank";
+        }
+
+        if (["remove", "keep"].includes(hideOnScroll)) {
+          newLink.hide_on_scroll = hideOnScroll;
+        } else {
+          newLink.hide_on_scroll = "keep";
+        }
+
+        if (locale) {
+          newLink.locale = locale;
+        }
+
+        newLinks.push(newLink);
+      }
     });
 
-    settings.set("custom_header_links", newSetting);
+    settings.set("custom_header_links", newLinks);
   }
 
   return settings;

settings.yml

@@ -1,26 +1,9 @@
 custom_header_links:
   type: objects
-  default:
+  default: []
-    - text: "External link"
-      title: "This link will open in a new tab"
-      url: "https://meta.discourse.org"
-      view: "vdo"
-      target: "blank"
-      hide_on_scroll: "remove"
-    - text: "Most Liked"
-      title: "Posts with the most amount of likes"
-      url: "/latest/?order=op_likes"
-      view: "vdo"
-      target: "self"
-      hide_on_scroll: "keep"
-    - text: "Privacy"
-      title: "Our Privacy Policy"
-      url: "/privacy"
-      view: "vdm"
-      target: "self"
-      hide_on_scroll: "keep"
   schema:
     name: "link"
+    identifier: text
     properties:
       text:
         type: string
@@ -30,7 +13,6 @@ custom_header_links:
           max_length: 100
       title:
         type: string
-        required: true
         validations:
           min_length: 1
           max_length: 1000

spec/system/viewing_custom_header_links_spec.rb

@@ -6,6 +6,40 @@ RSpec.describe "Viewing Custom Header Links", system: true do
   fab!(:theme) { upload_theme_component }
   let!(:custom_header_link) { PageObjects::Components::CustomHeaderLink.new }
 
+  before do
+    theme.update_setting(
+      :custom_header_links,
+      [
+        {
+          text: "External link",
+          title: "This link will open in a new tab",
+          url: "https://meta.discourse.org",
+          view: "vdo",
+          target: "blank",
+          hide_on_scroll: "remove",
+        },
+        {
+          text: "Most Liked",
+          title: "Posts with the most amount of likes",
+          url: "/latest/?order=op_likes",
+          view: "vdo",
+          target: "self",
+          hide_on_scroll: "keep",
+        },
+        {
+          text: "Privacy",
+          title: "Our Privacy Policy",
+          url: "/privacy",
+          view: "vdm",
+          target: "self",
+          hide_on_scroll: "keep",
+        },
+      ],
+    )
+
+    theme.save!
+  end
+
   context "when glimmer headers are enabled" do
     before do
       if SiteSetting.respond_to?(:experimental_glimmer_header_groups)

test/unit/migrations/settings/0002-migrate-custom-header-links-test.js

@@ -4,6 +4,158 @@ import migrate from "../../../../migrations/settings/0002-migrate-custom-header-
 module(
   "Unit | Migrations | Settings | 0002-migrate-custom-header-links",
   function () {
+    test("migrate when value of setting is already an array", function (assert) {
+      const settings = new Map(
+        Object.entries({
+          custom_header_links: [
+            {
+              text: "Some link",
+              title: "Some link title",
+              url: "https://some.url.com",
+              view: "vdo",
+              target: "blank",
+              hide_on_scroll: "remove",
+            },
+          ],
+        })
+      );
+
+      const result = migrate(settings);
+
+      const expectedResult = new Map(
+        Object.entries({
+          custom_header_links: [
+            {
+              text: "Some link",
+              title: "Some link title",
+              url: "https://some.url.com",
+              view: "vdo",
+              target: "blank",
+              hide_on_scroll: "remove",
+            },
+          ],
+        })
+      );
+
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
+    });
+
+    test("migrate when old setting value is invalid", function (assert) {
+      const settings = new Map(
+        Object.entries({
+          custom_header_links:
+            "Invalid Link|Invalid Link 2, some title|Invalid Link 3, , ,",
+        })
+      );
+
+      const result = migrate(settings);
+
+      const expectedResult = new Map(
+        Object.entries({ custom_header_links: [] })
+      );
+
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
+    });
+
+    test("migrate when `hide_on_scroll` attribute is invalid", function (assert) {
+      const settings = new Map(
+        Object.entries({
+          custom_header_links:
+            "External link, this link will open in a new tab, https://meta.discourse.org, vdo, blank, invalid",
+        })
+      );
+
+      const result = migrate(settings);
+
+      const expectedResult = new Map(
+        Object.entries({
+          custom_header_links: [
+            {
+              text: "External link",
+              title: "this link will open in a new tab",
+              url: "https://meta.discourse.org",
+              view: "vdo",
+              target: "blank",
+              hide_on_scroll: "keep",
+            },
+          ],
+        })
+      );
+
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
+    });
+
+    test("migrate when `target` attribute is invalid", function (assert) {
+      const settings = new Map(
+        Object.entries({
+          custom_header_links:
+            "External link, this link will open in a new tab, https://meta.discourse.org, vdo, invalid, remove",
+        })
+      );
+
+      const result = migrate(settings);
+
+      const expectedResult = new Map(
+        Object.entries({
+          custom_header_links: [
+            {
+              text: "External link",
+              title: "this link will open in a new tab",
+              url: "https://meta.discourse.org",
+              view: "vdo",
+              target: "blank",
+              hide_on_scroll: "remove",
+            },
+          ],
+        })
+      );
+
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
+    });
+
+    test("migrate when `view` attribute is invalid", function (assert) {
+      const settings = new Map(
+        Object.entries({
+          custom_header_links:
+            "External link, this link will open in a new tab, https://meta.discourse.org, invalid, blank, remove",
+        })
+      );
+
+      const result = migrate(settings);
+
+      const expectedResult = new Map(
+        Object.entries({
+          custom_header_links: [
+            {
+              text: "External link",
+              title: "this link will open in a new tab",
+              url: "https://meta.discourse.org",
+              view: "vdm",
+              target: "blank",
+              hide_on_scroll: "remove",
+            },
+          ],
+        })
+      );
+
+      assert.deepEqual(
+        Object.fromEntries(result.entries()),
+        Object.fromEntries(expectedResult.entries())
+      );
+    });
+
     test("migrate", function (assert) {
       const settings = new Map(
         Object.entries({