From e7e23e8d9ce73ac61b970d5192f9f4af90b3a87c Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 14 Jun 2022 15:39:56 +0800 Subject: [PATCH] FIX: Remove tags from experimental sidebar on notification level changed (#17083) As part of this commit, a bug where updating a tag's notification level on the server side does not update the state of the user's tag notification levels on the client side is fixed too. --- .../discourse/app/controllers/tag-show.js | 17 +++--- .../app/models/topic-tracking-state.js | 15 +++-- .../acceptance/sidebar-tags-section-test.js | 33 +++++++++++ .../unit/models/topic-tracking-state-test.js | 4 +- app/controllers/tags_controller.rb | 2 +- app/serializers/basic_user_serializer.rb | 10 ---- .../concerns/user_tag_notifications_mixin.rb | 33 +++++++++++ app/serializers/current_user_serializer.rb | 28 +--------- app/serializers/user_serializer.rb | 17 +----- .../user_tag_notifications_serializer.rb | 15 +++++ spec/requests/tags_controller_spec.rb | 55 +++++++++++++++++++ .../current_user_serializer_spec.rb | 16 +++--- 12 files changed, 167 insertions(+), 78 deletions(-) create mode 100644 app/serializers/concerns/user_tag_notifications_mixin.rb create mode 100644 app/serializers/user_tag_notifications_serializer.rb diff --git a/app/assets/javascripts/discourse/app/controllers/tag-show.js b/app/assets/javascripts/discourse/app/controllers/tag-show.js index c3339debfcf..e0faae1e25f 100644 --- a/app/assets/javascripts/discourse/app/controllers/tag-show.js +++ b/app/assets/javascripts/discourse/app/controllers/tag-show.js @@ -181,14 +181,15 @@ export default DiscoverySortableController.extend( this.tagNotification .update({ notification_level: notificationLevel }) .then((response) => { - this.currentUser.set( - "muted_tag_ids", - this.currentUser.calculateMutedIds( - notificationLevel, - response.responseJson.tag_id, - "muted_tag_ids" - ) - ); + const payload = response.responseJson; + + this.currentUser.setProperties({ + watched_tags: payload.watched_tags, + watching_first_post_tags: payload.watching_first_post_tags, + tracked_tags: payload.tracked_tags, + muted_tags: payload.muted_tags, + regular_tags: payload.regular_tags, + }); }); }, } diff --git a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js index 109bd65707e..0a2a9dac3c8 100644 --- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js +++ b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js @@ -32,15 +32,15 @@ function isUnseen(topic) { return !topic.is_seen; } -function hasMutedTags(topicTagIds, mutedTagIds, siteSettings) { - if (!mutedTagIds || !topicTagIds) { +function hasMutedTags(topicTags, mutedTags, siteSettings) { + if (!mutedTags || !topicTags) { return false; } return ( (siteSettings.remove_muted_tags_from_latest === "always" && - topicTagIds.any((tagId) => mutedTagIds.includes(tagId))) || + topicTags.any((topicTag) => mutedTags.includes(topicTag))) || (siteSettings.remove_muted_tags_from_latest === "only_muted" && - topicTagIds.every((tagId) => mutedTagIds.includes(tagId))) + topicTags.every((topicTag) => mutedTags.includes(topicTag))) ); } @@ -876,10 +876,9 @@ const TopicTrackingState = EmberObject.extend({ } if (["new_topic", "latest"].includes(data.message_type)) { - const mutedTagIds = User.currentProp("muted_tag_ids"); - if ( - hasMutedTags(data.payload.topic_tag_ids, mutedTagIds, this.siteSettings) - ) { + const mutedTags = User.currentProp("muted_tags"); + + if (hasMutedTags(data.payload.tags, mutedTags, this.siteSettings)) { return; } } 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 03c26f09922..cf220996fb9 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 @@ -14,6 +14,8 @@ import { import { isLegacyEmber } from "discourse-common/config/environment"; import discoveryFixture from "discourse/tests/fixtures/discovery-fixtures"; import { cloneJSON } from "discourse-common/lib/object"; +import selectKit from "discourse/tests/helpers/select-kit-helper"; +import { NotificationLevels } from "discourse/lib/notification-levels"; acceptance("Sidebar - Tags section - tagging disabled", function (needs) { needs.settings({ @@ -62,6 +64,16 @@ acceptance("Sidebar - Tags section", function (needs) { ); }); }); + + server.put("/tag/:tagId/notifications", (request) => { + return helper.response({ + watched_tags: [], + watching_first_post_tags: [], + regular_tags: [request.params.tagId], + tracked_tags: [], + muted_tags: [], + }); + }); }); conditionalTest( @@ -370,4 +382,25 @@ acceptance("Sidebar - Tags section", function (needs) { ); } ); + + conditionalTest( + "updating tags notification levels", + !isLegacyEmber(), + async function (assert) { + await visit(`/tag/tag1/l/unread`); + + const notificationLevelsDropdown = selectKit(".notifications-button"); + + await notificationLevelsDropdown.expand(); + + await notificationLevelsDropdown.selectRowByValue( + NotificationLevels.REGULAR + ); + + assert.ok( + !exists(".sidebar-section-tags .sidebar-section-link-tag1"), + "tag1 section link is removed from sidebar" + ); + } + ); }); diff --git a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js b/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js index 5978f449e36..173035392b9 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js @@ -760,8 +760,10 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { }); test("topics in muted tags do not get added to the state", function (assert) { - trackingState.currentUser.set("muted_tag_ids", [44]); + trackingState.currentUser.set("muted_tags", ["pending"]); + publishToMessageBus("/new", newTopicPayload); + assert.strictEqual( trackingState.findState(222), undefined, diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 7d3012bb652..096c4d8c382 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -301,7 +301,7 @@ class TagsController < ::ApplicationController raise Discourse::NotFound unless tag level = params[:tag_notification][:notification_level].to_i TagUser.change(current_user.id, tag.id, level) - render json: { notification_level: level, tag_id: tag.id } + render_serialized(current_user, UserTagNotificationsSerializer, root: false) end def personal_messages diff --git a/app/serializers/basic_user_serializer.rb b/app/serializers/basic_user_serializer.rb index 26249bb4833..fc580931f60 100644 --- a/app/serializers/basic_user_serializer.rb +++ b/app/serializers/basic_user_serializer.rb @@ -36,14 +36,4 @@ class BasicUserSerializer < ApplicationSerializer def category_user_notification_levels @category_user_notification_levels ||= CategoryUser.notification_levels_for(user) end - - def tags_with_notification_level(lookup_level) - tag_user_notification_levels.select do |id, level| - level == TagUser.notification_levels[lookup_level] - end.keys - end - - def tag_user_notification_levels - @tag_user_notification_levels ||= TagUser.notification_levels_for(user) - end end diff --git a/app/serializers/concerns/user_tag_notifications_mixin.rb b/app/serializers/concerns/user_tag_notifications_mixin.rb new file mode 100644 index 00000000000..dfa367d6165 --- /dev/null +++ b/app/serializers/concerns/user_tag_notifications_mixin.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module UserTagNotificationsMixin + def muted_tags + tags_with_notification_level(:muted) + end + + def tracked_tags + tags_with_notification_level(:tracking) + end + + def watching_first_post_tags + tags_with_notification_level(:watching_first_post) + end + + def watched_tags + tags_with_notification_level(:watching) + end + + def regular_tags + tags_with_notification_level(:regular) + end + + def tags_with_notification_level(lookup_level) + tag_user_notification_levels.select do |id, level| + level == TagUser.notification_levels[lookup_level] + end.keys + end + + def tag_user_notification_levels + @tag_user_notification_levels ||= TagUser.notification_levels_for(user) + end +end diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 2a48b85a3ec..afbc3b75857 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class CurrentUserSerializer < BasicUserSerializer + include UserTagNotificationsMixin attributes :name, :unread_notifications, @@ -33,7 +34,6 @@ class CurrentUserSerializer < BasicUserSerializer :tracked_category_ids, :watched_first_post_category_ids, :watched_category_ids, - :muted_tag_ids, :watched_tags, :watching_first_post_tags, :tracked_tags, @@ -230,32 +230,6 @@ class CurrentUserSerializer < BasicUserSerializer categories_with_notification_level(:watching_first_post) end - # this is a weird outlier that is used for topic tracking state which - # needs the actual ids, which is why it is duplicated with muted_tags - def muted_tag_ids - TagUser.lookup(object, :muted).pluck(:tag_id) - end - - def muted_tags - tags_with_notification_level(:muted) - end - - def tracked_tags - tags_with_notification_level(:tracking) - end - - def watching_first_post_tags - tags_with_notification_level(:watching_first_post) - end - - def watched_tags - tags_with_notification_level(:watching) - end - - def regular_tags - tags_with_notification_level(:regular) - end - def ignored_users IgnoredUser.where(user: object.id).joins(:ignored_user).pluck(:username) end diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 01df2bddfe1..7c0a97378c5 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class UserSerializer < UserCardSerializer + include UserTagNotificationsMixin attributes :bio_raw, :bio_cooked, @@ -211,22 +212,6 @@ class UserSerializer < UserCardSerializer ### ### PRIVATE ATTRIBUTES ### - def muted_tags - tags_with_notification_level(:muted) - end - - def tracked_tags - tags_with_notification_level(:tracking) - end - - def watching_first_post_tags - tags_with_notification_level(:watching_first_post) - end - - def watched_tags - tags_with_notification_level(:watching) - end - def muted_category_ids categories_with_notification_level(:muted) end diff --git a/app/serializers/user_tag_notifications_serializer.rb b/app/serializers/user_tag_notifications_serializer.rb new file mode 100644 index 00000000000..d2d0766a3e3 --- /dev/null +++ b/app/serializers/user_tag_notifications_serializer.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class UserTagNotificationsSerializer < ApplicationSerializer + include UserTagNotificationsMixin + + attributes :watched_tags, + :watching_first_post_tags, + :tracked_tags, + :muted_tags, + :regular_tags + + def user + object + end +end diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb index 260f2d20026..5fe847eb7b4 100644 --- a/spec/requests/tags_controller_spec.rb +++ b/spec/requests/tags_controller_spec.rb @@ -1095,4 +1095,59 @@ describe TagsController do end end end + + describe '#update_notifications' do + fab!(:tag) { Fabricate(:tag) } + + before do + sign_in(user) + end + + it 'returns 404 when tag is not found' do + put "/tag/someinvalidtagname/notifications.json" + + expect(response.status).to eq(404) + end + + it 'updates the notification level of a tag for a user' do + tag_user = TagUser.change(user.id, tag.id, NotificationLevels.all[:muted]) + + put "/tag/#{tag.name}/notifications.json", params: { + tag_notification: { + notification_level: NotificationLevels.all[:tracking] + } + } + + expect(response.status).to eq(200) + expect(response.parsed_body["watched_tags"]).to eq([]) + expect(response.parsed_body["watching_first_post_tags"]).to eq([]) + expect(response.parsed_body["tracked_tags"]).to eq([tag.name]) + expect(response.parsed_body["muted_tags"]).to eq([]) + expect(response.parsed_body["regular_tags"]).to eq([]) + + expect(tag_user.reload.notification_level).to eq(NotificationLevels.all[:tracking]) + end + + it 'sets the notification level of a tag for a user' do + expect do + put "/tag/#{tag.name}/notifications.json", params: { + tag_notification: { + notification_level: NotificationLevels.all[:muted] + } + } + + expect(response.status).to eq(200) + + expect(response.parsed_body["watched_tags"]).to eq([]) + expect(response.parsed_body["watching_first_post_tags"]).to eq([]) + expect(response.parsed_body["tracked_tags"]).to eq([]) + expect(response.parsed_body["muted_tags"]).to eq([tag.name]) + expect(response.parsed_body["regular_tags"]).to eq([]) + end.to change { user.tag_users.count }.by(1) + + tag_user = user.tag_users.last + + expect(tag_user.notification_level).to eq(NotificationLevels.all[:muted]) + end + end end diff --git a/spec/serializers/current_user_serializer_spec.rb b/spec/serializers/current_user_serializer_spec.rb index 4047b40d656..bf2ec182ba5 100644 --- a/spec/serializers/current_user_serializer_spec.rb +++ b/spec/serializers/current_user_serializer_spec.rb @@ -60,19 +60,21 @@ RSpec.describe CurrentUserSerializer do end end - context "#muted_tag_ids" do + context "#muted_tag" do fab!(:user) { Fabricate(:user) } fab!(:tag) { Fabricate(:tag) } + let!(:tag_user) do - TagUser.create!(user_id: user.id, - notification_level: TagUser.notification_levels[:muted], - tag_id: tag.id - ) + TagUser.create!( + user_id: user.id, + notification_level: TagUser.notification_levels[:muted], + tag_id: tag.id + ) end - it 'include muted tag ids' do + it 'includes muted tag names' do payload = serializer.as_json - expect(payload[:muted_tag_ids]).to eq([tag.id]) + expect(payload[:muted_tags]).to eq([tag.name]) end end