From 7a790a5f4cda4d9a98e5fb59cb3d6710297dcac5 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Mon, 24 Jul 2023 08:07:37 +0800 Subject: [PATCH] UX: Display tag's description as title in navigation menu (#22710) Why this change? We're already displaying a category's description as the title attribute on the category section link. We should do the same for tags as well. --- .../sidebar/anonymous/tags-section.hbs | 1 + .../sidebar/anonymous/tags-section.js | 13 +-- .../components/sidebar/common/tags-section.js | 20 ----- .../components/sidebar/user/tags-section.js | 16 ++-- .../tags-section/base-tag-section-link.js | 11 ++- .../sidebar-user-tags-section-test.js | 90 ------------------- .../concerns/navigation_menu_tags_mixin.rb | 12 +++ .../concerns/user_sidebar_mixin.rb | 11 +-- app/serializers/sidebar_tag_serializer.rb | 10 +++ app/serializers/site_serializer.rb | 31 ++++++- .../api/schemas/json/site_response.json | 3 + spec/serializers/site_serializer_spec.rb | 69 +++++++++++++- .../user_sidebar_serializer_attributes.rb | 12 +-- .../components/navigation_menu/base.rb | 10 +++ .../viewing_sidebar_as_anonymous_user_spec.rb | 55 +++++++----- spec/system/viewing_sidebar_spec.rb | 76 +++++++++++++++- 16 files changed, 269 insertions(+), 171 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/components/sidebar/common/tags-section.js create mode 100644 app/serializers/concerns/navigation_menu_tags_mixin.rb create mode 100644 app/serializers/sidebar_tag_serializer.rb diff --git a/app/assets/javascripts/discourse/app/components/sidebar/anonymous/tags-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/anonymous/tags-section.hbs index 644fb57316e..95fb8b2d5d4 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/anonymous/tags-section.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar/anonymous/tags-section.hbs @@ -9,6 +9,7 @@ 0 || - this.topSiteTags?.length > 0 + this.site.navigation_menu_site_top_tags?.length > 0 ); } @cached get sectionLinks() { return ( - this.site.anonymous_default_navigation_menu_tags || this.topSiteTags - ).map((tagName) => { + this.site.anonymous_default_navigation_menu_tags || + this.site.navigation_menu_site_top_tags + ).map((tag) => { return new TagSectionLink({ - tagName, + tag, topicTrackingState: this.topicTrackingState, }); }); diff --git a/app/assets/javascripts/discourse/app/components/sidebar/common/tags-section.js b/app/assets/javascripts/discourse/app/components/sidebar/common/tags-section.js deleted file mode 100644 index 700808ccbba..00000000000 --- a/app/assets/javascripts/discourse/app/components/sidebar/common/tags-section.js +++ /dev/null @@ -1,20 +0,0 @@ -import Component from "@glimmer/component"; -import { inject as service } from "@ember/service"; - -export const TOP_SITE_TAGS_TO_SHOW = 5; - -export default class SidebarCommonTagsSection extends Component { - @service site; - - topSiteTags = []; - - constructor() { - super(...arguments); - - if (this.site.top_tags?.length > 0) { - this.site.top_tags.splice(0, TOP_SITE_TAGS_TO_SHOW).forEach((tagName) => { - this.topSiteTags.push(tagName); - }); - } - } -} diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js b/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js index f6cedc53594..3a52c2cb576 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js @@ -1,18 +1,19 @@ +import Component from "@glimmer/component"; import { action } from "@ember/object"; import { cached } from "@glimmer/tracking"; import { inject as service } from "@ember/service"; -import SidebarCommonTagsSection from "discourse/components/sidebar/common/tags-section"; import TagSectionLink from "discourse/lib/sidebar/user/tags-section/tag-section-link"; import PMTagSectionLink from "discourse/lib/sidebar/user/tags-section/pm-tag-section-link"; import { hasDefaultSidebarTags } from "discourse/lib/sidebar/helpers"; import SidebarEditNavigationMenuTagsModal from "discourse/components/sidebar/edit-navigation-menu/tags-modal"; -export default class SidebarUserTagsSection extends SidebarCommonTagsSection { +export default class SidebarUserTagsSection extends Component { @service currentUser; @service modal; @service pmTopicTrackingState; @service router; + @service site; @service siteSettings; @service topicTrackingState; @@ -41,26 +42,21 @@ export default class SidebarUserTagsSection extends SidebarCommonTagsSection { if (this.currentUser.sidebarTags.length > 0) { tags = this.currentUser.sidebarTags; } else { - tags = this.topSiteTags.map((tagName) => { - return { - name: tagName, - pm_only: false, - }; - }); + tags = this.site.navigation_menu_site_top_tags || []; } for (const tag of tags) { if (tag.pm_only) { links.push( new PMTagSectionLink({ - tagName: tag.name, + tag, currentUser: this.currentUser, }) ); } else { links.push( new TagSectionLink({ - tagName: tag.name, + tag, topicTrackingState: this.topicTrackingState, currentUser: this.currentUser, }) diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/user/tags-section/base-tag-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/user/tags-section/base-tag-section-link.js index 2c2c9967478..4f8cac18897 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/user/tags-section/base-tag-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/user/tags-section/base-tag-section-link.js @@ -1,3 +1,5 @@ +import { escape } from "pretty-text/sanitizer"; + let customTagSectionLinkPrefixIcons = {}; export function registerCustomTagSectionLinkPrefixIcon({ @@ -20,8 +22,9 @@ export function resetCustomTagSectionLinkPrefixIcons() { } export default class BaseTagSectionLink { - constructor({ tagName, currentUser }) { - this.tagName = tagName; + constructor({ tag, currentUser }) { + this.tag = tag; + this.tagName = tag.name; this.currentUser = currentUser; } @@ -33,6 +36,10 @@ export default class BaseTagSectionLink { return this.tagName; } + get title() { + return escape(this.tag.description); + } + get prefixType() { return "icon"; } diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js index 80193b5ca06..d441ca48968 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js @@ -7,35 +7,11 @@ import { exists, publishToMessageBus, query, - queryAll, updateCurrentUser, } from "discourse/tests/helpers/qunit-helpers"; import discoveryFixture from "discourse/tests/fixtures/discovery-fixtures"; import { cloneJSON } from "discourse-common/lib/object"; import { NotificationLevels } from "discourse/lib/notification-levels"; -import Site from "discourse/models/site"; -import { TOP_SITE_TAGS_TO_SHOW } from "discourse/components/sidebar/common/tags-section"; - -acceptance( - "Sidebar - Logged on user - Tags section - tagging disabled", - function (needs) { - needs.settings({ - tagging_enabled: false, - navigation_menu: "sidebar", - }); - - needs.user(); - - test("tags section is not shown", async function (assert) { - await visit("/"); - - assert.ok( - !exists(".sidebar-section[data-section-name='tags']"), - "does not display the tags section" - ); - }); - } -); acceptance("Sidebar - Logged on user - Tags section", function (needs) { needs.settings({ @@ -93,72 +69,6 @@ acceptance("Sidebar - Logged on user - Tags section", function (needs) { }); }); - test("section is not displayed when display_sidebar_tags property is false", async function (assert) { - updateCurrentUser({ display_sidebar_tags: false }); - - await visit("/"); - - assert.notOk( - exists(".sidebar-section[data-section-name='tags']"), - "tags section is not displayed" - ); - }); - - test("tags section is displayed with site's top tags when user has not added any tags and there are no default tags configured", async function (assert) { - updateCurrentUser({ - sidebar_tags: [], - }); - - Site.current().top_tags = [ - "test1", - "test2", - "test3", - "test4", - "test5", - "test6", - ]; - - await visit("/"); - - assert.ok( - exists(".sidebar-section[data-section-name='tags']"), - "tags section is displayed" - ); - - assert.strictEqual( - count( - ".sidebar-section[data-section-name='tags'] .sidebar-section-link-wrapper[data-tag-name]" - ), - TOP_SITE_TAGS_TO_SHOW, - "right number of tag section links are displayed" - ); - - ["test1", "test2", "test3", "test4", "test5"].forEach((tagName) => { - assert.ok( - exists(`.sidebar-section-link-wrapper[data-tag-name=${tagName}]`), - `${tagName} tag section link is displayed` - ); - }); - }); - - test("tag section links are sorted alphabetically by tag's name", async function (assert) { - await visit("/"); - - const tagSectionLinks = queryAll( - ".sidebar-section[data-section-name='tags'] .sidebar-section-link:not(.sidebar-section-link[data-link-name='all-tags'])" - ); - - const tagNames = [...tagSectionLinks].map((tagSectionLink) => - tagSectionLink.textContent.trim() - ); - - assert.deepEqual( - tagNames, - ["tag1", "tag2", "tag3", "tag4"], - "tag section links are displayed in the right order" - ); - }); - test("tag section links for user", async function (assert) { await visit("/"); diff --git a/app/serializers/concerns/navigation_menu_tags_mixin.rb b/app/serializers/concerns/navigation_menu_tags_mixin.rb new file mode 100644 index 00000000000..55adb1be0ae --- /dev/null +++ b/app/serializers/concerns/navigation_menu_tags_mixin.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module NavigationMenuTagsMixin + def serialize_tags(tags) + topic_count_column = Tag.topic_count_column(scope) + + tags + .select(:name, topic_count_column, :pm_topic_count, :description) + .order(topic_count_column => :desc) + .map { |tag| SidebarTagSerializer.new(tag, scope: scope, root: false).as_json } + end +end diff --git a/app/serializers/concerns/user_sidebar_mixin.rb b/app/serializers/concerns/user_sidebar_mixin.rb index a606de5ff59..bfeddacee46 100644 --- a/app/serializers/concerns/user_sidebar_mixin.rb +++ b/app/serializers/concerns/user_sidebar_mixin.rb @@ -1,15 +1,10 @@ # frozen_string_literal: true module UserSidebarMixin - def sidebar_tags - topic_count_column = Tag.topic_count_column(scope) + include NavigationMenuTagsMixin - object - .visible_sidebar_tags(scope) - .pluck(:name, topic_count_column, :pm_topic_count) - .reduce([]) do |tags, sidebar_tag| - tags.push(name: sidebar_tag[0], pm_only: sidebar_tag[1] == 0 && sidebar_tag[2] > 0) - end + def sidebar_tags + serialize_tags(object.visible_sidebar_tags(scope)) end def display_sidebar_tags diff --git a/app/serializers/sidebar_tag_serializer.rb b/app/serializers/sidebar_tag_serializer.rb new file mode 100644 index 00000000000..7433950c423 --- /dev/null +++ b/app/serializers/sidebar_tag_serializer.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class SidebarTagSerializer < ApplicationSerializer + attributes :name, :description, :pm_only + + def pm_only + topic_count_column = Tag.topic_count_column(scope) + object.public_send(topic_count_column) == 0 && object.pm_topic_count > 0 + end +end diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 394e087298c..ee8b027ebc9 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class SiteSerializer < ApplicationSerializer + include NavigationMenuTagsMixin + attributes( :default_archetype, :notification_types, @@ -21,6 +23,7 @@ class SiteSerializer < ApplicationSerializer :can_tag_pms, :tags_filter_regexp, :top_tags, + :navigation_menu_site_top_tags, :can_associate_groups, :wizard_required, :topic_featured_link_allowed_category_ids, @@ -187,7 +190,7 @@ class SiteSerializer < ApplicationSerializer end def top_tags - Tag.top_tags(guardian: scope) + @top_tags ||= Tag.top_tags(guardian: scope) end def wizard_required @@ -250,9 +253,33 @@ class SiteSerializer < ApplicationSerializer About.displayed_plugin_stat_groups end + SIDEBAR_TOP_TAGS_TO_SHOW = 5 + + def navigation_menu_site_top_tags + if top_tags.present? + tag_names = top_tags[0...SIDEBAR_TOP_TAGS_TO_SHOW] + serialized = serialize_tags(Tag.where(name: tag_names)) + + # Ensures order of top tags is preserved + serialized.sort_by { |tag| tag_names.index(tag[:name]) } + else + [] + end + end + + def include_navigation_menu_site_top_tags? + !SiteSetting.legacy_navigation_menu? && SiteSetting.tagging_enabled + end + def anonymous_default_navigation_menu_tags @anonymous_default_navigation_menu_tags ||= - SiteSetting.default_navigation_menu_tags.split("|") - DiscourseTagging.hidden_tag_names(scope) + begin + tag_names = + SiteSetting.default_navigation_menu_tags.split("|") - + DiscourseTagging.hidden_tag_names(scope) + + serialize_tags(Tag.where(name: tag_names)) + end end def include_anonymous_default_navigation_menu_tags? diff --git a/spec/requests/api/schemas/json/site_response.json b/spec/requests/api/schemas/json/site_response.json index fc1a3af5cba..29194ce1c97 100644 --- a/spec/requests/api/schemas/json/site_response.json +++ b/spec/requests/api/schemas/json/site_response.json @@ -800,6 +800,9 @@ }, "denied_emojis" : { "type": "array" + }, + "navigation_menu_site_top_tags": { + "type": "array" } }, "required": [ diff --git a/spec/serializers/site_serializer_spec.rb b/spec/serializers/site_serializer_spec.rb index c1337e8a74f..54a1a035c43 100644 --- a/spec/serializers/site_serializer_spec.rb +++ b/spec/serializers/site_serializer_spec.rb @@ -133,9 +133,10 @@ RSpec.describe SiteSerializer do describe "#anonymous_default_navigation_menu_tags" do fab!(:user) { Fabricate(:user) } - fab!(:tag) { Fabricate(:tag, name: "dev") } + fab!(:tag) { Fabricate(:tag, name: "dev", description: "some description") } fab!(:tag2) { Fabricate(:tag, name: "random") } fab!(:hidden_tag) { Fabricate(:tag, name: "secret") } + fab!(:staff_tag_group) do Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) end @@ -177,7 +178,13 @@ RSpec.describe SiteSerializer do it "includes only tags user can see in the serialised object when user is anonymous" do serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json - expect(serialized[:anonymous_default_navigation_menu_tags]).to eq(%w[dev random]) + + expect(serialized[:anonymous_default_navigation_menu_tags]).to eq( + [ + { name: "dev", description: "some description", pm_only: false }, + { name: "random", description: tag2.description, pm_only: false }, + ], + ) end end @@ -299,6 +306,64 @@ RSpec.describe SiteSerializer do end end + describe "#navigation_menu_site_top_tags" do + fab!(:tag1) do + Fabricate(:tag, name: "tag 1").tap { |tag| Fabricate.times(2, :topic, tags: [tag]) } + end + + fab!(:tag2) do + Fabricate(:tag, name: "tag 2").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) } + end + + fab!(:tag3) do + Fabricate(:tag, name: "tag 3").tap { |tag| Fabricate.times(3, :topic, tags: [tag]) } + end + + fab!(:hidden_tag) do + Fabricate(:tag, name: "tag 4").tap { |tag| Fabricate.times(4, :topic, tags: [tag]) } + end + + fab!(:staff_tag_group) do + Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) + end + + it "should return the site's top tags as the default tags for sidebar" do + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + + expect(serialized[:navigation_menu_site_top_tags]).to eq( + [ + { name: tag3.name, description: tag2.description, pm_only: false }, + { name: tag1.name, description: tag1.description, pm_only: false }, + { name: tag2.name, description: tag3.description, pm_only: false }, + ], + ) + end + + it "should not be serialized if `navigation_menu` site setting is set to `legacy`" do + SiteSetting.set(:navigation_menu, "legacy") + + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + + expect(serialized[:navigation_menu_site_top_tags]).to eq(nil) + end + + it "should not be serialized if `tagging_enabled` site setting is set to false" do + SiteSetting.set(:tagging_enabled, false) + + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + + expect(serialized[:navigation_menu_site_top_tags]).to eq(nil) + end + + it "should return an empty array if site has no top tags" do + Tag.delete_all + + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + + expect(serialized[:navigation_menu_site_top_tags]).to eq([]) + end + end + describe "#whispers_allowed_groups_names" do fab!(:admin) { Fabricate(:admin) } fab!(:allowed_user) { Fabricate(:user) } diff --git a/spec/support/user_sidebar_serializer_attributes.rb b/spec/support/user_sidebar_serializer_attributes.rb index 5e8f908e3ea..6424947d8cf 100644 --- a/spec/support/user_sidebar_serializer_attributes.rb +++ b/spec/support/user_sidebar_serializer_attributes.rb @@ -46,7 +46,7 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass| end describe "#sidebar_tags" do - fab!(:tag) { Fabricate(:tag, name: "foo") } + fab!(:tag) { Fabricate(:tag, name: "foo", description: "foo tag") } fab!(:pm_tag) do Fabricate(:tag, name: "bar", pm_topic_count: 5, staff_topic_count: 0, public_topic_count: 0) end @@ -89,8 +89,8 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass| json = serializer.as_json expect(json[:sidebar_tags]).to contain_exactly( - { name: tag.name, pm_only: false }, - { name: pm_tag.name, pm_only: true }, + { name: tag.name, pm_only: false, description: tag.description }, + { name: pm_tag.name, pm_only: true, description: nil }, ) user.update!(admin: true) @@ -98,9 +98,9 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass| json = serializer.as_json expect(json[:sidebar_tags]).to contain_exactly( - { name: tag.name, pm_only: false }, - { name: pm_tag.name, pm_only: true }, - { name: hidden_tag.name, pm_only: false }, + { name: tag.name, pm_only: false, description: tag.description }, + { name: pm_tag.name, pm_only: true, description: nil }, + { name: hidden_tag.name, pm_only: false, description: nil }, ) end end diff --git a/spec/system/page_objects/components/navigation_menu/base.rb b/spec/system/page_objects/components/navigation_menu/base.rb index beb3a51ad33..a7b1ab7edcd 100644 --- a/spec/system/page_objects/components/navigation_menu/base.rb +++ b/spec/system/page_objects/components/navigation_menu/base.rb @@ -25,6 +25,7 @@ module PageObjects def has_no_section_link?(name, href: nil, active: false) section_link_present?(name, href: href, active: active, present: false) end + def has_section?(name) has_css?(".sidebar-sections [data-section-name='#{name.parameterize}']") end @@ -61,6 +62,15 @@ module PageObjects expect(tag_section_links.map(&:text)).to eq(tag_names) end + def has_tag_section_link_with_title?(tag, title) + section_link = + find( + ".sidebar-section[data-section-name='tags'] .sidebar-section-link-wrapper[data-tag-name='#{tag.name}'] .sidebar-section-link", + ) + + expect(section_link["title"]).to eq(title) + end + def primary_section_links(slug) all("[data-section-name='#{slug}'] .sidebar-section-link-wrapper").map(&:text) end diff --git a/spec/system/viewing_sidebar_as_anonymous_user_spec.rb b/spec/system/viewing_sidebar_as_anonymous_user_spec.rb index 1d1b75a6a55..bac2805e04a 100644 --- a/spec/system/viewing_sidebar_as_anonymous_user_spec.rb +++ b/spec/system/viewing_sidebar_as_anonymous_user_spec.rb @@ -1,33 +1,37 @@ # frozen_string_literal: true describe "Viewing sidebar as anonymous user", type: :system do - fab!(:tag1) do - Fabricate(:tag, name: "tag 1").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) } - end - - fab!(:tag2) do - Fabricate(:tag, name: "tag 2").tap { |tag| Fabricate.times(2, :topic, tags: [tag]) } - end - - fab!(:tag3) do - Fabricate(:tag, name: "tag 3").tap { |tag| Fabricate.times(3, :topic, tags: [tag]) } - end - - fab!(:tag4) do - Fabricate(:tag, name: "tag 4").tap { |tag| Fabricate.times(2, :topic, tags: [tag]) } - end - - fab!(:tag5) do - Fabricate(:tag, name: "tag 5").tap { |tag| Fabricate.times(2, :topic, tags: [tag]) } - end - - fab!(:tag6) do - Fabricate(:tag, name: "tag 6").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) } - end - let(:sidebar) { PageObjects::Components::NavigationMenu::Sidebar.new } describe "when viewing the tags section" do + fab!(:tag1) do + Fabricate(:tag, name: "tag 1", description: "tag 1 description