From 3c019b1c0f7db4b5482bf93f13bb8dfcdd5e808d Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Fri, 30 Jun 2023 12:25:43 +1000 Subject: [PATCH] FIX: consistent sidebar section external links (#22343) Before this change, links which required full reload because they are not in ember routes like `/my/preferences` or links to docs like `/pub/*` were treated as real external links. Therefore, they were opening in self window or new tab based on user `external_links_in_new_tab` setting. To be consistent with behavior when full reload links are in the post, they are treated as internal and always open in the same window. --- .../components/sidebar/common/custom-section.hbs | 3 ++- .../app/components/sidebar/more-section-link.hbs | 3 ++- .../app/components/sidebar/section-link.js | 3 +++ .../discourse/app/lib/sidebar/section-link.js | 13 +++++++++++-- app/serializers/sidebar_url_serializer.rb | 8 ++++++-- spec/system/custom_sidebar_sections_spec.rb | 12 +++++++++--- spec/system/page_objects/components/sidebar.rb | 7 ++++--- 7 files changed, 37 insertions(+), 12 deletions(-) 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 a433af1e15d..cde9643f48f 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 @@ -8,13 +8,14 @@ @class={{this.section.dragCss}} > {{#each this.section.links as |link|}} - {{#if link.external}} + {{#if link.externalOrFullReload}} {{else}} 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 00e497d64e7..0c50537e832 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/section-link.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/section-link.js @@ -50,6 +50,9 @@ export default class SectionLink extends Component { } get target() { + if (this.args.fullReload) { + return "_self"; + } return this.currentUser?.user_option?.external_links_in_new_tab ? "_blank" : "_self"; 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 5846f319c5a..f08aff2fab0 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js @@ -9,8 +9,13 @@ const MOUSE_DELAY = 250; export default class SectionLink { @tracked linkDragCss; - constructor({ external, icon, id, name, value }, section, router) { + constructor( + { external, full_reload, icon, id, name, value }, + section, + router + ) { this.external = external; + this.fullReload = full_reload; this.prefixValue = icon; this.id = id; this.name = name; @@ -18,7 +23,7 @@ export default class SectionLink { this.value = value; this.section = section; - if (!this.external) { + if (!this.externalOrFullReload) { const routeInfoHelper = new RouteInfoHelper(router, value); this.route = routeInfoHelper.route; this.models = routeInfoHelper.models; @@ -30,6 +35,10 @@ export default class SectionLink { return true; } + get externalOrFullReload() { + return this.external || this.fullReload; + } + @bind didStartDrag(event) { // 0 represents left button of the mouse diff --git a/app/serializers/sidebar_url_serializer.rb b/app/serializers/sidebar_url_serializer.rb index 2425d0dda81..993aa92f6f2 100644 --- a/app/serializers/sidebar_url_serializer.rb +++ b/app/serializers/sidebar_url_serializer.rb @@ -1,9 +1,13 @@ # frozen_string_literal: true class SidebarUrlSerializer < ApplicationSerializer - attributes :id, :name, :value, :icon, :external, :segment + attributes :id, :name, :value, :icon, :external, :full_reload, :segment def external - object.external? || object.full_reload? + 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 18c49d14040..c92eaea28bf 100644 --- a/spec/system/custom_sidebar_sections_spec.rb +++ b/spec/system/custom_sidebar_sections_spec.rb @@ -6,6 +6,8 @@ describe "Custom sidebar sections", type: :system do let(:section_modal) { PageObjects::Modals::SidebarSectionForm.new } let(:sidebar) { PageObjects::Components::Sidebar.new } + before { user.user_option.update!(external_links_in_new_tab: true) } + it "allows the user to create custom section" do visit("/latest") @@ -40,7 +42,7 @@ describe "Custom sidebar sections", type: :system do section_modal.save expect(sidebar).to have_section("My section") - expect(sidebar).to have_section_link("My preferences") + expect(sidebar).to have_section_link("My preferences", target: "_self") end it "allows the user to create custom section with /pub link" do @@ -53,7 +55,7 @@ describe "Custom sidebar sections", type: :system do section_modal.save expect(sidebar).to have_section("My section") - expect(sidebar).to have_section_link("Published Page") + expect(sidebar).to have_section_link("Published Page", target: "_self") end it "allows the user to create custom section with external link" do @@ -76,7 +78,11 @@ describe "Custom sidebar sections", type: :system do section_modal.save expect(sidebar).to have_section("My section") - expect(sidebar).to have_section_link("Discourse Homepage", href: "https://discourse.org") + expect(sidebar).to have_section_link( + "Discourse Homepage", + href: "https://discourse.org", + target: "_blank", + ) end it "allows the user to edit custom section" do diff --git a/spec/system/page_objects/components/sidebar.rb b/spec/system/page_objects/components/sidebar.rb index 7dbea8f9c07..ccd68a80f74 100644 --- a/spec/system/page_objects/components/sidebar.rb +++ b/spec/system/page_objects/components/sidebar.rb @@ -57,8 +57,8 @@ module PageObjects has_css?(".#{SIDEBAR_SECTION_LINK_SELECTOR}--active", count: 1) end - def has_section_link?(name, href: nil, active: false) - section_link_present?(name, href: href, active: active, present: true) + def has_section_link?(name, href: nil, active: false, target: nil) + section_link_present?(name, href: href, active: active, target: target, present: true) end def has_no_section_link?(name, href: nil, active: false) @@ -119,11 +119,12 @@ module PageObjects private - def section_link_present?(name, href: nil, active: false, present:) + def section_link_present?(name, href: nil, active: false, target: nil, present:) attributes = { exact_text: name } attributes[:href] = href if href attributes[:class] = SIDEBAR_SECTION_LINK_SELECTOR attributes[:class] += "--active" if active + attributes[:target] = target if target page.public_send(present ? :has_link? : :has_no_link?, **attributes) end