From e3cfb1967d531a09b475bba45b33605eabd5a0d8 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 20 Mar 2024 12:55:40 +0000 Subject: [PATCH] FIX: Simplify sidebar custom link implementation (#26201) All our link validation, and conversion from url -> route/model/query is expensive and prone to bugs. Instead, if people enter a link, we can just use it as-is. Originally all this extra logic was added to handle unusual situations like `/safe-mode`, `/my/...`, etc. However, all of these are now handled correctly by our Ember router, so there is no need for it. Now, we just pass the user-supplied `href` directly to the SectionLink component, and let Ember handle routing to it when clicked. The only functional change here is that we no longer validate internal links by parsing them with the Ember router. But I'd argue this is fine, because the previous logic would cause both false positives (e.g. `/t/123` would be valid, even if topic 123 doesn't exist), and false negatives (for routes which are server-side only, like the new AI share pages). --- .../components/modal/sidebar-section-form.js | 25 ++------- .../sidebar/common/custom-section.hbs | 51 +++++++------------ .../components/sidebar/more-section-link.hbs | 48 +++++++---------- .../app/components/sidebar/section-link.js | 14 +++-- .../sidebar/base-community-section-link.js | 6 +-- .../sidebar/custom-community-section-links.js | 37 +++----------- .../app/lib/sidebar/route-info-helper.js | 36 ------------- .../discourse/app/lib/sidebar/section-link.js | 26 +--------- app/models/sidebar_url.rb | 5 -- app/serializers/sidebar_url_serializer.rb | 6 +-- spec/system/custom_sidebar_sections_spec.rb | 48 ++++++++++++++--- 11 files changed, 103 insertions(+), 199 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/lib/sidebar/route-info-helper.js diff --git a/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.js b/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.js index fb74b1810f7..78aad30eb9f 100644 --- a/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.js +++ b/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.js @@ -7,17 +7,10 @@ import { isEmpty } from "@ember/utils"; import { ajax } from "discourse/lib/ajax"; import { extractError } from "discourse/lib/ajax-error"; import { SIDEBAR_SECTION, SIDEBAR_URL } from "discourse/lib/constants"; -import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper"; import { sanitize } from "discourse/lib/text"; import { afterRender, bind } from "discourse-common/utils/decorators"; import I18n from "discourse-i18n"; -const FULL_RELOAD_LINKS_REGEX = [ - /^\/my\/[a-z_\-\/]+$/, - /^\/pub\/[a-z_\-\/]+$/, - /^\/safe-mode$/, -]; - class Section { @tracked title; @tracked links; @@ -213,28 +206,16 @@ class SectionLink { } get #invalidValue() { - return ( - this.path && - (this.external ? !this.#validExternal() : !this.#validInternal()) - ); + return this.path && !this.#validLink(); } - #validExternal() { + #validLink() { try { - return new URL(this.value); + return new URL(this.value, document.location.origin); } catch { return false; } } - - #validInternal() { - const routeInfoHelper = new RouteInfoHelper(this.router, this.path); - - return ( - routeInfoHelper.route !== "unknown" || - FULL_RELOAD_LINKS_REGEX.some((regex) => this.path.match(regex)) - ); - } } export default class SidebarSectionForm extends Component { diff --git a/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs index 6f9113383ff..642c6a969bc 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs @@ -9,38 +9,25 @@ class={{this.section.dragCss}} > {{#each this.section.links as |link|}} - {{#if link.externalOrFullReload}} - - {{else}} - - {{/if}} + {{/each}} {{#if this.section.moreLinks}} diff --git a/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs b/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs index 134a1b0a5b7..5dacb0721ca 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs @@ -1,30 +1,18 @@ -{{#if @sectionLink.externalOrFullReload}} - -{{else}} - -{{/if}} \ No newline at end of file + \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/sidebar/section-link.js b/app/assets/javascripts/discourse/app/components/sidebar/section-link.js index 9442b66d289..6206b76c19d 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/section-link.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/section-link.js @@ -58,14 +58,20 @@ export default class SectionLink extends Component { } get target() { - if (this.args.fullReload) { - return "_self"; - } - return this.currentUser?.user_option?.external_links_in_new_tab + return this.currentUser?.user_option?.external_links_in_new_tab && + this.isExternal ? "_blank" : "_self"; } + get isExternal() { + return ( + this.args.href && + new URL(this.args.href, window.location.href).origin !== + window.location.origin + ); + } + get models() { if (this.args.model) { return [this.args.model]; diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js index 656fdd32174..eda45e67ede 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js @@ -47,11 +47,9 @@ export default class BaseCommunitySectionLink { } /** - * @returns {string} Ember route + * @returns {string|undefined} Ember route */ - get route() { - this._notImplemented(); - } + get route() {} /** * @returns {string} href attribute for the link. This property will take precedence over the `route` property when set. diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/custom-community-section-links.js b/app/assets/javascripts/discourse/app/lib/sidebar/custom-community-section-links.js index 6d29113823e..38be61a3f06 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/custom-community-section-links.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/custom-community-section-links.js @@ -1,5 +1,4 @@ import BaseCommunitySectionLink from "discourse/lib/sidebar/base-community-section-link"; -import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper"; export let customSectionLinks = []; export let secondaryCustomSectionLinks = []; @@ -27,38 +26,10 @@ export function addSectionLink(args, secondary) { links.push(args.call(this, BaseCommunitySectionLink)); } else { const klass = class extends BaseCommunitySectionLink { - constructor() { - super(...arguments); - - if (args.href) { - this.routeInfoHelper = new RouteInfoHelper(this.router, args.href); - } - } - get name() { return args.name; } - get route() { - if (args.href) { - return this.routeInfoHelper.route; - } else { - return args.route; - } - } - - get models() { - if (args.href) { - return this.routeInfoHelper.models; - } - } - - get query() { - if (args.href) { - return this.routeInfoHelper.query; - } - } - get text() { return args.text; } @@ -67,6 +38,14 @@ export function addSectionLink(args, secondary) { return args.title; } + get href() { + return args.href; + } + + get route() { + return args.route; + } + get prefixValue() { return args.icon || super.prefixValue; } diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/route-info-helper.js b/app/assets/javascripts/discourse/app/lib/sidebar/route-info-helper.js deleted file mode 100644 index 39db22d53a2..00000000000 --- a/app/assets/javascripts/discourse/app/lib/sidebar/route-info-helper.js +++ /dev/null @@ -1,36 +0,0 @@ -export default class RouteInfoHelper { - constructor(router, url) { - this.routeInfo = router.recognize(router.rootURL.replace(/\/$/, "") + url); - } - - get route() { - return this.routeInfo.name; - } - - get models() { - return this.#getParameters; - } - - get query() { - return this.routeInfo.queryParams; - } - - /** - * Extracted from https://github.com/emberjs/rfcs/issues/658 - * Retrieves all parameters for a `RouteInfo` object and its parents in - * correct oder, so that you can pass them to e.g. - * `transitionTo(routeName, ...params)`. - */ - get #getParameters() { - let allParameters = []; - let current = this.routeInfo; - - do { - const { params, paramNames } = current; - const currentParameters = paramNames.map((n) => params[n]); - allParameters = [...currentParameters, ...allParameters]; - } while ((current = current.parent)); - - return allParameters; - } -} diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js index 0aad8be80e3..2e429015ffd 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js @@ -1,17 +1,10 @@ import { tracked } from "@glimmer/tracking"; -import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper"; -import { defaultHomepage } from "discourse/lib/utilities"; export default class SectionLink { @tracked linkDragCss; - constructor( - { external, full_reload, icon, id, name, value }, - section, - router - ) { + constructor({ external, icon, id, name, value }, section) { this.external = external; - this.fullReload = full_reload; this.prefixValue = icon; this.id = id; this.name = name; @@ -19,26 +12,9 @@ export default class SectionLink { this.value = value; this.section = section; this.withAnchor = value.match(/#\w+$/gi); - - if (!this.externalOrFullReload) { - const routeInfoHelper = new RouteInfoHelper(router, value); - - if (routeInfoHelper.route === "discovery.index") { - this.route = `discovery.${defaultHomepage()}`; - } else { - this.route = routeInfoHelper.route; - } - - this.models = routeInfoHelper.models; - this.query = routeInfoHelper.query; - } } get shouldDisplay() { return true; } - - get externalOrFullReload() { - return this.external || this.fullReload || this.withAnchor; - } } diff --git a/app/models/sidebar_url.rb b/app/models/sidebar_url.rb index 6e21a3e76c5..b3a12d99094 100644 --- a/app/models/sidebar_url.rb +++ b/app/models/sidebar_url.rb @@ -3,7 +3,6 @@ class SidebarUrl < ActiveRecord::Base enum :segment, { primary: 0, secondary: 1 }, scopes: false, suffix: true - FULL_RELOAD_LINKS_REGEX = [%r{\A/my/[a-z_\-/]+\z}, %r{\A/pub/[a-z_\-/]+\z}, %r{\A/safe-mode\z}] MAX_ICON_LENGTH = 40 MAX_NAME_LENGTH = 80 MAX_VALUE_LENGTH = 1000 @@ -69,10 +68,6 @@ class SidebarUrl < ActiveRecord::Base def set_external self.external = value.start_with?("http://", "https://") end - - def full_reload? - FULL_RELOAD_LINKS_REGEX.any? { |regex| value =~ regex } - end end # == Schema Information diff --git a/app/serializers/sidebar_url_serializer.rb b/app/serializers/sidebar_url_serializer.rb index 993aa92f6f2..fb7ab90cd9b 100644 --- a/app/serializers/sidebar_url_serializer.rb +++ b/app/serializers/sidebar_url_serializer.rb @@ -1,13 +1,9 @@ # frozen_string_literal: true class SidebarUrlSerializer < ApplicationSerializer - attributes :id, :name, :value, :icon, :external, :full_reload, :segment + attributes :id, :name, :value, :icon, :external, :segment def external object.external? end - - def full_reload - object.full_reload? - end end diff --git a/spec/system/custom_sidebar_sections_spec.rb b/spec/system/custom_sidebar_sections_spec.rb index 027877dd729..09de7465289 100644 --- a/spec/system/custom_sidebar_sections_spec.rb +++ b/spec/system/custom_sidebar_sections_spec.rb @@ -55,7 +55,7 @@ describe "Custom sidebar sections", type: :system do expect(sidebar).to have_section_link("My preferences", target: "_self") end - it "allows the user to create custom section with `/` path which generates a link based on the first item in the `top_menu` site settings" do + it "allows the user to create custom section with `/` path" do SiteSetting.top_menu = "read|posted|latest" sign_in user @@ -67,7 +67,10 @@ describe "Custom sidebar sections", type: :system do section_modal.save expect(sidebar).to have_section("My section") - expect(sidebar).to have_section_link("Home", href: "/read") + expect(sidebar).to have_section_link("Home", href: "/") + + sidebar.click_section_link("Home") + expect(page).to have_css("#navigation-bar .active a[href='/read']") end it "allows the user to create custom section with /pub link" do @@ -94,9 +97,6 @@ describe "Custom sidebar sections", type: :system do section_modal.fill_name("My section") - section_modal.fill_link("Discourse Homepage", "htt") - expect(section_modal).to have_disabled_save - section_modal.fill_link("Discourse Homepage", "https://discourse.org") expect(section_modal).to have_enabled_save @@ -124,7 +124,41 @@ describe "Custom sidebar sections", type: :system do section_modal.save expect(sidebar).to have_section("My section") - expect(sidebar).to have_section_link("Faq", target: "_blank") + expect(sidebar).to have_section_link("Faq", target: "_self", href: "/faq#anchor") + end + + it "allows the user to create custom section with query param" do + sign_in user + visit("/latest") + sidebar.click_add_section_button + + expect(section_modal).to be_visible + expect(section_modal).to have_disabled_save + expect(sidebar.custom_section_modal_title).to have_content("Add custom section") + + section_modal.fill_name("My section") + section_modal.fill_link("Faq", "/faq?a=b") + section_modal.save + + expect(sidebar).to have_section("My section") + expect(sidebar).to have_section_link("Faq", target: "_self", href: "/faq?a=b") + end + + it "allows the user to create custom section with anchor link" do + sign_in user + visit("/latest") + sidebar.click_add_section_button + + expect(section_modal).to be_visible + expect(section_modal).to have_disabled_save + expect(sidebar.custom_section_modal_title).to have_content("Add custom section") + + section_modal.fill_name("My section") + section_modal.fill_link("Faq", "/faq#someheading") + section_modal.save + + expect(sidebar).to have_section("My section") + expect(sidebar).to have_section_link("Faq", target: "_self", href: "/faq#someheading") end it "accessibility - when new row is added in custom section, first new input is focused" do @@ -309,7 +343,7 @@ describe "Custom sidebar sections", type: :system do sidebar.click_add_section_button section_modal.fill_name("A" * (SidebarSection::MAX_TITLE_LENGTH + 1)) - section_modal.fill_link("B" * (SidebarUrl::MAX_NAME_LENGTH + 1), "/wrong-url") + section_modal.fill_link("B" * (SidebarUrl::MAX_NAME_LENGTH + 1), "https:") expect(page.find(".title.warning")).to have_content("Title must be shorter than 30 characters") expect(page.find(".name.warning")).to have_content("Name must be shorter than 80 characters")