From 3deabb00d4eac414954c67306541b2faff9ef4ba Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 12 Aug 2022 11:26:56 +0800 Subject: [PATCH] DEV: Route PM only tags to PM tags show route (#17870) Previously, PM only tags were being routed to the public topic list with the tag added as a filter. However, the public topic list does not fetch PMs and hence PM only tags did not provide any value when added to the Sidebar. This commit changes that by allowing the client to differentiate PM only tag and thus routes the link to the PM tags show route. Counts for PM only tags section links are not supported as of this commit and will be added in a follow up commit. --- .../app/components/sidebar/tags-section.js | 31 +++++++--- .../app/controllers/preferences/sidebar.js | 15 +++-- .../tags-section/pm-tag-section-link.js | 22 ++++++++ .../sidebar/tags-section/tag-section-link.js | 32 +++++------ .../javascripts/discourse/app/models/user.js | 19 +++++-- .../components/sidebar/tags-section.hbs | 2 +- .../sidebar-categories-section-test.js | 2 +- .../acceptance/sidebar-tags-section-test.js | 56 +++++++++++++++++-- .../user-preferences-sidebar-test.js | 13 ++++- .../concerns/user_sidebar_tags_mixin.rb | 11 ++++ app/serializers/current_user_serializer.rb | 11 +--- app/serializers/sidebar/tag_serializer.rb | 11 ++++ app/serializers/user_card_serializer.rb | 6 +- app/serializers/user_serializer.rb | 4 +- .../current_user_serializer_spec.rb | 42 ++++++-------- spec/serializers/user_serializer_spec.rb | 48 ++++++++++++++++ 16 files changed, 244 insertions(+), 81 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/lib/sidebar/tags-section/pm-tag-section-link.js create mode 100644 app/serializers/concerns/user_sidebar_tags_mixin.rb create mode 100644 app/serializers/sidebar/tag_serializer.rb diff --git a/app/assets/javascripts/discourse/app/components/sidebar/tags-section.js b/app/assets/javascripts/discourse/app/components/sidebar/tags-section.js index f2722244c4..55dc9a376e 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/tags-section.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/tags-section.js @@ -1,15 +1,17 @@ import I18n from "I18n"; import { cached } from "@glimmer/tracking"; +import Component from "@glimmer/component"; import { inject as service } from "@ember/service"; import { action } from "@ember/object"; -import Component from "@glimmer/component"; import TagSectionLink from "discourse/lib/sidebar/tags-section/tag-section-link"; +import PMTagSectionLink from "discourse/lib/sidebar/tags-section/pm-tag-section-link"; export default class SidebarTagsSection extends Component { @service router; @service topicTrackingState; + @service pmTopicTrackingState; @service currentUser; constructor() { @@ -17,7 +19,9 @@ export default class SidebarTagsSection extends Component { this.callbackId = this.topicTrackingState.onStateChange(() => { this.sectionLinks.forEach((sectionLink) => { - sectionLink.refreshCounts(); + if (sectionLink.refreshCounts) { + sectionLink.refreshCounts(); + } }); }); } @@ -30,13 +34,22 @@ export default class SidebarTagsSection extends Component { get sectionLinks() { const links = []; - for (const tagName of this.currentUser.sidebarTagNames) { - links.push( - new TagSectionLink({ - tagName, - topicTrackingState: this.topicTrackingState, - }) - ); + for (const tag of this.currentUser.sidebarTags) { + if (tag.pm_only) { + links.push( + new PMTagSectionLink({ + tag, + currentUser: this.currentUser, + }) + ); + } else { + links.push( + new TagSectionLink({ + tag, + topicTrackingState: this.topicTrackingState, + }) + ); + } } return links; diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/sidebar.js b/app/assets/javascripts/discourse/app/controllers/preferences/sidebar.js index 73c840b008..2b712f0290 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/sidebar.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/sidebar.js @@ -12,24 +12,29 @@ export default class extends Controller { @action save() { const initialSidebarCategoryIds = this.model.sidebarCategoryIds; - const initialSidebarTagNames = this.model.sidebarTagNames; - - this.model.set("sidebar_tag_names", this.selectedSidebarTagNames); this.model.set( "sidebarCategoryIds", this.selectedSidebarCategories.mapBy("id") ); + this.model.set("sidebar_tag_names", this.selectedSidebarTagNames); + this.model .save() - .then(() => { + .then((result) => { + if (result.user.sidebar_tags) { + this.model.set("sidebar_tags", result.user.sidebar_tags); + } + this.saved = true; }) .catch((error) => { this.model.set("sidebarCategoryIds", initialSidebarCategoryIds); - this.model.set("sidebar_tag_names", initialSidebarTagNames); popupAjaxError(error); + }) + .finally(() => { + this.model.set("sidebar_tag_names", []); }); } } diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/tags-section/pm-tag-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/tags-section/pm-tag-section-link.js new file mode 100644 index 0000000000..aec926b601 --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/sidebar/tags-section/pm-tag-section-link.js @@ -0,0 +1,22 @@ +export default class PMTagSectionLink { + constructor({ tag, currentUser }) { + this.tag = tag; + this.currentUser = currentUser; + } + + get name() { + return this.tag.name; + } + + get models() { + return [this.currentUser, this.tag.name]; + } + + get route() { + return "userPrivateMessages.tagsShow"; + } + + get text() { + return this.tag.name; + } +} diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/tags-section/tag-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/tags-section/tag-section-link.js index fda4b8fa29..dd24165a49 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/tags-section/tag-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/tags-section/tag-section-link.js @@ -8,8 +8,8 @@ export default class TagSectionLink { @tracked totalUnread = 0; @tracked totalNew = 0; - constructor({ tagName, topicTrackingState }) { - this.tagName = tagName; + constructor({ tag, topicTrackingState }) { + this.tagName = tag.name; this.topicTrackingState = topicTrackingState; this.refreshCounts(); } @@ -31,18 +31,24 @@ export default class TagSectionLink { return this.tagName; } - get model() { - return this.tagName; + get models() { + return [this.tagName]; + } + + get route() { + if (this.totalUnread > 0) { + return "tag.showUnread"; + } else if (this.totalNew > 0) { + return "tag.showNew"; + } else { + return "tag.show"; + } } get currentWhen() { return "tag.show tag.showNew tag.showUnread tag.showTop"; } - get route() { - return "tag.show"; - } - get text() { return this.tagName; } @@ -58,14 +64,4 @@ export default class TagSectionLink { }); } } - - get route() { - if (this.totalUnread > 0) { - return "tag.showUnread"; - } else if (this.totalNew > 0) { - return "tag.showNew"; - } else { - return "tag.show"; - } - } } diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index 99e9eb4d8c..76c4d82869 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -1,7 +1,7 @@ import EmberObject, { computed, get, getProperties } from "@ember/object"; import cookie, { removeCookie } from "discourse/lib/cookie"; import { defaultHomepage, escapeExpression } from "discourse/lib/utilities"; -import { alias, equal, filterBy, gt, or } from "@ember/object/computed"; +import { alias, equal, filterBy, gt, mapBy, or } from "@ember/object/computed"; import getURL, { getURLWithCDN } from "discourse-common/lib/get-url"; import { A } from "@ember/array"; import Badge from "discourse/models/badge"; @@ -314,15 +314,23 @@ const User = RestModel.extend({ sidebarCategoryIds: alias("sidebar_category_ids"), - @discourseComputed("sidebar_tag_names.[]") - sidebarTagNames(sidebarTagNames) { - if (!sidebarTagNames || sidebarTagNames.length === 0) { + @discourseComputed("sidebar_tags.[]") + sidebarTags(sidebarTags) { + if (!sidebarTags || sidebarTags.length === 0) { return []; } - return sidebarTagNames; + if (this.siteSettings.tags_sort_alphabetically) { + return sidebarTags.sort((a, b) => { + return a.name.localeCompare(b); + }); + } else { + return sidebarTags; + } }, + sidebarTagNames: mapBy("sidebarTags", "name"), + @discourseComputed("sidebar_category_ids.[]") sidebarCategories(sidebarCategoryIds) { if (!sidebarCategoryIds || sidebarCategoryIds.length === 0) { @@ -446,6 +454,7 @@ const User = RestModel.extend({ ); User.current().setProperties(userProps); this.setProperties(updatedState); + return result; }) .finally(() => { this.set("isSaving", false); diff --git a/app/assets/javascripts/discourse/app/templates/components/sidebar/tags-section.hbs b/app/assets/javascripts/discourse/app/templates/components/sidebar/tags-section.hbs index 1f94d16fc8..5e9e419389 100644 --- a/app/assets/javascripts/discourse/app/templates/components/sidebar/tags-section.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/sidebar/tags-section.hbs @@ -16,7 +16,7 @@ @content={{sectionLink.text}} @currentWhen={{sectionLink.currentWhen}} @badgeText={{sectionLink.badgeText}} - @model={{sectionLink.model}}> + @models={{sectionLink.models}} > {{/each}} {{else}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js index 7abc849dbb..9c7c2390b1 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js @@ -57,7 +57,7 @@ acceptance( acceptance("Sidebar - Categories Section", function (needs) { needs.user({ sidebar_category_ids: [], - sidebar_tag_names: [], + sidebar_tags: [], }); needs.settings({ diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js index 4d370ea55a..c70e71b0f4 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js @@ -42,7 +42,18 @@ acceptance("Sidebar - Tags section", function (needs) { tracked_tags: ["tag1"], watched_tags: ["tag2", "tag3"], watching_first_post_tags: [], - sidebar_tag_names: ["tag1", "tag2", "tag3"], + sidebar_tags: [ + { name: "tag1", pm_only: false }, + { name: "tag2", pm_only: false }, + { + name: "tag3", + pm_only: false, + }, + { + name: "tag4", + pm_only: true, + }, + ], }); needs.pretender((server, helper) => { @@ -52,6 +63,20 @@ acceptance("Sidebar - Tags section", function (needs) { }); }); + server.get("/topics/private-messages-tags/:username/:tagId", () => { + const topics = [ + { id: 1, posters: [] }, + { id: 2, posters: [] }, + { id: 3, posters: [] }, + ]; + + return helper.response({ + topic_list: { + topics, + }, + }); + }); + ["latest", "top", "new", "unread"].forEach((type) => { server.get(`/tag/:tagId/l/${type}.json`, () => { return helper.response( @@ -85,7 +110,7 @@ acceptance("Sidebar - Tags section", function (needs) { test("section content when user has not added any tags", async function (assert) { updateCurrentUser({ - sidebar_tag_names: [], + sidebar_tags: [], }); await visit("/"); @@ -106,8 +131,8 @@ acceptance("Sidebar - Tags section", function (needs) { assert.strictEqual( count(".sidebar-section-tags .sidebar-section-link"), - 3, - "3 section links under the section" + 4, + "4 section links under the section" ); assert.strictEqual( @@ -167,6 +192,29 @@ acceptance("Sidebar - Tags section", function (needs) { ); }); + test("private message tag section links for user", async function (assert) { + await visit("/"); + + await click(".sidebar-section-link-tag4"); + + assert.strictEqual( + currentURL(), + "/u/eviltrout/messages/tags/tag4", + "it should transition to user's private message tag4 tag page" + ); + + assert.strictEqual( + count(".sidebar-section-tags .sidebar-section-link.active"), + 1, + "only one link is marked as active" + ); + + assert.ok( + exists(`.sidebar-section-link-tag4.active`), + "the tag4 section link is marked as active" + ); + }); + test("visiting tag discovery top route", async function (assert) { await visit(`/tag/tag1/l/top`); diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js index 6a2d3d429e..c5c2b3aeb4 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js @@ -12,7 +12,7 @@ import selectKit from "discourse/tests/helpers/select-kit-helper"; acceptance("User Preferences - Sidebar", function (needs) { needs.user({ sidebar_category_ids: [], - sidebar_tag_names: [], + sidebar_tags: [], }); needs.settings({ @@ -39,7 +39,14 @@ acceptance("User Preferences - Sidebar", function (needs) { // This request format will cause an error return helper.response(400, {}); } else { - return helper.response({ user: {} }); + return helper.response({ + user: { + sidebar_tags: [ + { name: "monkey", pm_only: false }, + { name: "gazelle", pm_only: false }, + ], + }, + }); } }); }); @@ -121,7 +128,7 @@ acceptance("User Preferences - Sidebar", function (needs) { }); test("user encountering error when adding tags to sidebar", async function (assert) { - updateCurrentUser({ sidebar_tag_names: ["monkey"] }); + updateCurrentUser({ sidebar_tags: [{ name: "monkey", pm_only: false }] }); await visit("/"); diff --git a/app/serializers/concerns/user_sidebar_tags_mixin.rb b/app/serializers/concerns/user_sidebar_tags_mixin.rb new file mode 100644 index 0000000000..f9ec79fbee --- /dev/null +++ b/app/serializers/concerns/user_sidebar_tags_mixin.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module UserSidebarTagsMixin + def self.included(base) + base.has_many :sidebar_tags, serializer: Sidebar::TagSerializer, embed: :objects + end + + def include_sidebar_tags? + SiteSetting.enable_experimental_sidebar_hamburger && SiteSetting.tagging_enabled + end +end diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index a922e4fcaa..cea7bda185 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -2,6 +2,7 @@ class CurrentUserSerializer < BasicUserSerializer include UserTagNotificationsMixin + include UserSidebarTagsMixin attributes :name, :unread_notifications, @@ -75,7 +76,7 @@ class CurrentUserSerializer < BasicUserSerializer :pending_posts_count, :status, :sidebar_category_ids, - :sidebar_tag_names, + :sidebar_tags, :likes_notifications_disabled, :grouped_unread_high_priority_notifications, :redesigned_user_menu_enabled @@ -315,14 +316,6 @@ class CurrentUserSerializer < BasicUserSerializer SiteSetting.enable_experimental_sidebar_hamburger end - def sidebar_tag_names - object.sidebar_tags.pluck(:name) - end - - def include_sidebar_tag_names? - include_sidebar_category_ids? && SiteSetting.tagging_enabled - end - def include_status? SiteSetting.enable_user_status && object.has_status? end diff --git a/app/serializers/sidebar/tag_serializer.rb b/app/serializers/sidebar/tag_serializer.rb new file mode 100644 index 0000000000..3e576cd247 --- /dev/null +++ b/app/serializers/sidebar/tag_serializer.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Sidebar + class TagSerializer < ::ApplicationSerializer + attributes :name, :pm_only + + def pm_only + object.topic_count == 0 && object.pm_topic_count > 0 + end + end +end diff --git a/app/serializers/user_card_serializer.rb b/app/serializers/user_card_serializer.rb index 3170936174..a109e9ebbf 100644 --- a/app/serializers/user_card_serializer.rb +++ b/app/serializers/user_card_serializer.rb @@ -16,7 +16,11 @@ class UserCardSerializer < BasicUserSerializer attributes(*attrs) attrs.each do |attr| define_method "include_#{attr}?" do - can_edit + if defined?(super) + super() && can_edit + else + can_edit + end end end end diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 7c0a97378c..0e9f975032 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -2,6 +2,7 @@ class UserSerializer < UserCardSerializer include UserTagNotificationsMixin + include UserSidebarTagsMixin attributes :bio_raw, :bio_cooked, @@ -62,7 +63,8 @@ class UserSerializer < UserCardSerializer :user_api_keys, :user_auth_tokens, :user_notification_schedule, - :use_logo_small_as_avatar + :use_logo_small_as_avatar, + :sidebar_tags untrusted_attributes :bio_raw, :bio_cooked, diff --git a/spec/serializers/current_user_serializer_spec.rb b/spec/serializers/current_user_serializer_spec.rb index 7063db3fdc..85ede7e134 100644 --- a/spec/serializers/current_user_serializer_spec.rb +++ b/spec/serializers/current_user_serializer_spec.rb @@ -220,45 +220,39 @@ RSpec.describe CurrentUserSerializer do end end - describe '#sidebar_tag_names' do + describe '#sidebar_tags' do fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user) } fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user) } - it "is not included when SiteSeting.enable_experimental_sidebar_hamburger is false" do - SiteSetting.enable_experimental_sidebar_hamburger = false - - json = serializer.as_json - - expect(json[:sidebar_tag_names]).to eq(nil) - end - - it "is not included when SiteSeting.tagging_enabled is false" do - SiteSetting.enable_experimental_sidebar_hamburger = true - SiteSetting.tagging_enabled = false - - json = serializer.as_json - - expect(json[:sidebar_tag_names]).to eq(nil) - end - it "is not included when experimental sidebar has not been enabled" do SiteSetting.enable_experimental_sidebar_hamburger = false SiteSetting.tagging_enabled = true json = serializer.as_json - expect(json[:sidebar_tag_names]).to eq(nil) + expect(json[:sidebar_tags]).to eq(nil) end - it "is present when experimental sidebar has been enabled" do + it "is not included when tagging has not been enabled" do SiteSetting.enable_experimental_sidebar_hamburger = true - SiteSetting.tagging_enabled = true + SiteSetting.tagging_enabled = false json = serializer.as_json - expect(json[:sidebar_tag_names]).to contain_exactly( - tag_sidebar_section_link.linkable.name, - tag_sidebar_section_link_2.linkable.name + expect(json[:sidebar_tags]).to eq(nil) + end + + it "is present when experimental sidebar and tagging has been enabled" do + SiteSetting.enable_experimental_sidebar_hamburger = true + SiteSetting.tagging_enabled = true + + tag_sidebar_section_link_2.linkable.update!(pm_topic_count: 5, topic_count: 0) + + json = serializer.as_json + + expect(json[:sidebar_tags]).to contain_exactly( + { name: tag_sidebar_section_link.linkable.name, pm_only: false }, + { name: tag_sidebar_section_link_2.linkable.name, pm_only: true } ) end end diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index bfe37e15bf..966d5f225f 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -376,4 +376,52 @@ RSpec.describe UserSerializer do expect(json[:user_api_keys][2][:id]).to eq(user_api_key_2.id) end end + + describe '#sidebar_tags' do + fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user) } + fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user) } + + context 'when viewing self' do + subject(:json) { UserSerializer.new(user, scope: Guardian.new(user), root: false).as_json } + + it "is not included when SiteSeting.enable_experimental_sidebar_hamburger is false" do + SiteSetting.enable_experimental_sidebar_hamburger = false + SiteSetting.tagging_enabled = true + + expect(json[:sidebar_tags]).to eq(nil) + end + + it "is not included when SiteSeting.tagging_enabled is false" do + SiteSetting.enable_experimental_sidebar_hamburger = true + SiteSetting.tagging_enabled = false + + expect(json[:sidebar_tags]).to eq(nil) + end + + it "is present when experimental sidebar and tagging has been enabled" do + SiteSetting.enable_experimental_sidebar_hamburger = true + SiteSetting.tagging_enabled = true + + tag_sidebar_section_link_2.linkable.update!(pm_topic_count: 5, topic_count: 0) + + expect(json[:sidebar_tags]).to contain_exactly( + { name: tag_sidebar_section_link.linkable.name, pm_only: false }, + { name: tag_sidebar_section_link_2.linkable.name, pm_only: true } + ) + end + end + + context 'when viewing another user' do + fab!(:user2) { Fabricate(:user) } + + subject(:json) { UserSerializer.new(user, scope: Guardian.new(user2), root: false).as_json } + + it "is not present even when experimental sidebar and tagging has been enabled" do + SiteSetting.enable_experimental_sidebar_hamburger = true + SiteSetting.tagging_enabled = true + + expect(json[:sidebar_tags]).to eq(nil) + end + end + end end