From 167bc8cee96ca7f7b140682ffca3aa5425846ca2 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 26 Apr 2024 12:57:46 +0800 Subject: [PATCH] FIX: Ensure that custom header links migration do not fail validation (#55) This commit is a follow up to 73747938bde3c2d3f6f68350c48a320364be1b04 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. --- .../0002-migrate-custom-header-links.js | 57 +++++-- settings.yml | 22 +-- .../viewing_custom_header_links_spec.rb | 34 ++++ .../0002-migrate-custom-header-links-test.js | 152 ++++++++++++++++++ 4 files changed, 228 insertions(+), 37 deletions(-) diff --git a/migrations/settings/0002-migrate-custom-header-links.js b/migrations/settings/0002-migrate-custom-header-links.js index ba7ecad..46648f0 100644 --- a/migrations/settings/0002-migrate-custom-header-links.js +++ b/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, - }; + if (text && title && url) { + const newLink = { + text, + title, + url, + }; - Object.keys(newLink).forEach((key) => { - if (newLink[key] === undefined) { - delete newLink[key]; + if (["vdm", "vdo", "vmo"].includes(view)) { + newLink.view = view; + } 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; diff --git a/settings.yml b/settings.yml index 71b6ddc..3bb8161 100644 --- a/settings.yml +++ b/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 diff --git a/spec/system/viewing_custom_header_links_spec.rb b/spec/system/viewing_custom_header_links_spec.rb index 857f3b7..a1ec1d8 100644 --- a/spec/system/viewing_custom_header_links_spec.rb +++ b/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) diff --git a/test/unit/migrations/settings/0002-migrate-custom-header-links-test.js b/test/unit/migrations/settings/0002-migrate-custom-header-links-test.js index 0a02b99..bde6ef1 100644 --- a/test/unit/migrations/settings/0002-migrate-custom-header-links-test.js +++ b/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({