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 fb74b1810f7e8..78aad30eb9f89 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 6f9113383ff2c..642c6a969bcf8 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 134a1b0a5b73a..5dacb0721cab5 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 9442b66d2894d..6206b76c19de5 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 656fdd3217463..eda45e67ede9a 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 6d29113823e85..38be61a3f06a7 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 39db22d53a202..0000000000000 --- 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 0aad8be80e30c..2e429015ffdea 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 6e21a3e76c59e..b3a12d99094cf 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 @@ def remove_internal_hostname 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 993aa92f6f212..fb7ab90cd9b55 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 027877dd7295e..09de746528985 100644 --- a/spec/system/custom_sidebar_sections_spec.rb +++ b/spec/system/custom_sidebar_sections_spec.rb @@ -55,7 +55,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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")