FIX: Ensure that custom header links migration do not fail validation

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 [text, title, url, view, target, hide_on_scroll, locale] = link
+    const newLinks = [];
+
+    oldSetting.split("|").forEach((link) => {
+      const [text, title, url, view, target, hideOnScroll, locale] = link
         .split(",")
         .map((s) => s.trim());
 
-      const newLink = {
-        text,
-        title,
-        url,
-        view,
-        target,
-        hide_on_scroll,
-        locale,
-      };
-
-      Object.keys(newLink).forEach((key) => {
-        if (newLink[key] === undefined) {
-          delete newLink[key];
+      if (text && title && url) {
+        const newLink = {
+          text,
+          title,
+          url,
+        };
+
+        if (["vdm", "vdo", "vmo"].includes(view)) {
+          newLink.view = view;
+        } else {
+          newLink.view = "vdm";
+        }
+
+        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;
         }
-      });
 
-      return newLink;
+        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:
-    - 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"
+  default: []
   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 @@
   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({