From e92e05b22e94e7ad7b43b584216f4ab499fdbeaf Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 27 Feb 2025 14:26:07 +1000 Subject: [PATCH] FIX: Error when leaving group DM channel (#31537) Followup b5147a4634f0fd5c98262f949a8c766bfd73d290 When we aliased `leave` to `remove` and renamed the method in `DirectMessageChannel` in the previous commit, this inadvertantly caused an error when unfollowing group channels in the channel list. When clicking the X in the channel list, we hit ChannelsCurrentUserMembershipFollowsController for the current user and the channel, which is supposed to only unfollow the channel for all channel types including DMs. Group DMs have a different Leave behaviour vs Unfollow. Leaving the channel altogether is done from the channel settings page, the "Leave channel" button, and that deletes the user's membership and DM user record from that channel. So, we were trying to do the leave channel behaviour in the unfollow channel controller, which was returning the wrong record for the serializer (a User not a Membership) This fixes the issue and removes a bit of delegate/alias indirection which was making the code a bit harder to fllow and search, even though it was more succinct. Also adds missing specs that would have caught this regression. --- .../app/jobs/regular/chat/notify_watching.rb | 4 +- plugins/chat/app/models/chat/channel.rb | 5 +- .../app/models/chat/direct_message_channel.rb | 12 +-- .../chat/app/services/chat/leave_channel.rb | 9 ++- .../services/chat/remove_user_from_channel.rb | 6 +- .../app/services/chat/unfollow_channel.rb | 4 +- .../chat/spec/fabricators/chat_fabricator.rb | 5 +- .../jobs/regular/chat/notify_watching_spec.rb | 80 +++++++++++++------ .../spec/models/chat/category_channel_spec.rb | 12 +-- .../chat/direct_message_channel_spec.rb | 12 ++- ...current_user_membership_controller_spec.rb | 2 +- ...user_membership_follows_controller_spec.rb | 41 ++++++++-- 12 files changed, 142 insertions(+), 50 deletions(-) diff --git a/plugins/chat/app/jobs/regular/chat/notify_watching.rb b/plugins/chat/app/jobs/regular/chat/notify_watching.rb index de88fcf349e..063e8360f22 100644 --- a/plugins/chat/app/jobs/regular/chat/notify_watching.rb +++ b/plugins/chat/app/jobs/regular/chat/notify_watching.rb @@ -51,7 +51,7 @@ module Jobs translation_key = ( if @is_direct_message_channel - if @chat_channel.chatable.group + if @chat_channel.direct_message_group? "discourse_push_notifications.popup.new_chat_message" else "discourse_push_notifications.popup.new_direct_chat_message" @@ -63,7 +63,7 @@ module Jobs translation_args = { username: @creator.username } translation_args[:channel] = @chat_channel.title(user) unless @is_direct_message_channel && - !@chat_channel.chatable.group + !@chat_channel.direct_message_group? translation_args = DiscoursePluginRegistry.apply_modifier( :chat_notification_translation_args, diff --git a/plugins/chat/app/models/chat/channel.rb b/plugins/chat/app/models/chat/channel.rb index 8133eade078..29b8a0f5a5c 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/plugins/chat/app/models/chat/channel.rb @@ -144,7 +144,10 @@ module Chat def remove(user) Chat::ChannelMembershipManager.new(self).unfollow(user) end - alias leave remove + + def leave(user) + self.remove(user) + end def url "#{Discourse.base_url}/chat/c/#{self.slug || "-"}/#{self.id}" diff --git a/plugins/chat/app/models/chat/direct_message_channel.rb b/plugins/chat/app/models/chat/direct_message_channel.rb index f8bc4627533..3e134725a80 100644 --- a/plugins/chat/app/models/chat/direct_message_channel.rb +++ b/plugins/chat/app/models/chat/direct_message_channel.rb @@ -4,8 +4,6 @@ module Chat class DirectMessageChannel < Channel alias_method :direct_message, :chatable - delegate :group?, to: :direct_message, prefix: true, allow_nil: true - before_validation(on: :create) { self.threading_enabled = true } def direct_message_channel? @@ -28,13 +26,17 @@ module Chat self.slug.blank? end - def remove(user) - return super unless direct_message_group? + # Group DMs are DMs with > 2 users + def direct_message_group? + direct_message.group? + end + + def leave(user) + return super if !direct_message_group? transaction do membership_for(user)&.destroy! direct_message.users.delete(user) end end - alias leave remove end end diff --git a/plugins/chat/app/services/chat/leave_channel.rb b/plugins/chat/app/services/chat/leave_channel.rb index 908d5e528cb..f855142d4ac 100644 --- a/plugins/chat/app/services/chat/leave_channel.rb +++ b/plugins/chat/app/services/chat/leave_channel.rb @@ -1,7 +1,14 @@ # frozen_string_literal: true module Chat - # Service responsible to flag a message. + # Service responsible for completely leaving a channel, + # which does something different depending on the channel: + # + # Category channels - Unfollows the channel similar to + # behaviour of Chat::UnfollowChannel + # DM channels with 2 users - Same as category channel + # DM channels with > 2 users (group DM) - Deletes the user's + # membership and removes them from the channel's user list. # # @example # ::Chat::LeaveChannel.call( diff --git a/plugins/chat/app/services/chat/remove_user_from_channel.rb b/plugins/chat/app/services/chat/remove_user_from_channel.rb index 3a2146a8948..530c2fa0b0c 100644 --- a/plugins/chat/app/services/chat/remove_user_from_channel.rb +++ b/plugins/chat/app/services/chat/remove_user_from_channel.rb @@ -57,7 +57,11 @@ module Chat end def remove(channel:, target_user:) - channel.remove(target_user) + if channel.direct_message_channel? + channel.leave(target_user) + else + channel.remove(target_user) + end end def recompute_users_count(channel:) diff --git a/plugins/chat/app/services/chat/unfollow_channel.rb b/plugins/chat/app/services/chat/unfollow_channel.rb index eb979e98a98..f15cfb17822 100644 --- a/plugins/chat/app/services/chat/unfollow_channel.rb +++ b/plugins/chat/app/services/chat/unfollow_channel.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true module Chat - # Service responsible to flag a message. + # Service responsible to unfollow a chat channel, + # which means you no longer receive notifications or + # see it in the channel list. # # @example # ::Chat::UnfollowChannel.call( diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index f3c1ac69597..7d8d95a9419 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -35,10 +35,11 @@ end Fabricator(:direct_message_channel, from: :chat_channel) do transient :users, :group, following: true, with_membership: true chatable do |attrs| + users = attrs[:users] Fabricate( :direct_message, - users: attrs[:users] || [Fabricate(:user), Fabricate(:user)], - group: attrs[:group] || false, + users: users || [Fabricate(:user), Fabricate(:user)], + group: attrs[:group] || (users ? users.length > 2 : false), ) end status { :open } diff --git a/plugins/chat/spec/jobs/regular/chat/notify_watching_spec.rb b/plugins/chat/spec/jobs/regular/chat/notify_watching_spec.rb index 4696296ffd6..450dd3afe16 100644 --- a/plugins/chat/spec/jobs/regular/chat/notify_watching_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/notify_watching_spec.rb @@ -271,7 +271,7 @@ RSpec.describe Jobs::Chat::NotifyWatching do context "for a direct message channel" do fab!(:channel) do - Fabricate(:direct_message_channel, users: [user1, user2, user3], with_membership: false) + Fabricate(:direct_message_channel, users: [user1, user2], with_membership: false) end fab!(:membership1) do Fabricate(:user_chat_channel_membership, user: user1, chat_channel: channel) @@ -279,34 +279,66 @@ RSpec.describe Jobs::Chat::NotifyWatching do fab!(:membership2) do Fabricate(:user_chat_channel_membership, user: user2, chat_channel: channel) end - fab!(:membership3) do - Fabricate(:user_chat_channel_membership, user: user3, chat_channel: channel) - end fab!(:message) { Fabricate(:chat_message, chat_channel: channel, user: user1) } - before do - membership2.update!( - notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always], - ) + context "when the channel is a group DM" do + fab!(:membership3) do + Fabricate(:user_chat_channel_membership, user: user3, chat_channel: channel) + end + + before do + channel.add(user3) + channel.chatable.update!(group: true) + membership2.update!( + notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always], + ) + end + + it "sends a desktop notification" do + messages = notification_messages_for(user2) + + expect(messages.first.data).to include( + { + username: user1.username, + notification_type: Notification.types[:chat_message], + post_url: message.url, + translated_title: + I18n.t( + "discourse_push_notifications.popup.new_chat_message", + { username: user1.username, channel: channel.title(user2) }, + ), + tag: Chat::Notifier.push_notification_tag(:message, channel.id), + excerpt: message.push_notification_excerpt, + }, + ) + end end - it "sends a desktop notification" do - messages = notification_messages_for(user2) + context "when the channel is a regular DM" do + before do + membership2.update!( + notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always], + ) + end - expect(messages.first.data).to include( - { - username: user1.username, - notification_type: Notification.types[:chat_message], - post_url: message.url, - translated_title: - I18n.t( - "discourse_push_notifications.popup.new_direct_chat_message", - { username: user1.username, channel: channel.title(user2) }, - ), - tag: Chat::Notifier.push_notification_tag(:message, channel.id), - excerpt: message.push_notification_excerpt, - }, - ) + it "sends a desktop notification" do + messages = notification_messages_for(user2) + + expect(messages.first.data).to include( + { + username: user1.username, + notification_type: Notification.types[:chat_message], + post_url: message.url, + translated_title: + I18n.t( + "discourse_push_notifications.popup.new_direct_chat_message", + { username: user1.username }, + ), + tag: Chat::Notifier.push_notification_tag(:message, channel.id), + excerpt: message.push_notification_excerpt, + }, + ) + end end context "when the channel is muted via membership preferences" do diff --git a/plugins/chat/spec/models/chat/category_channel_spec.rb b/plugins/chat/spec/models/chat/category_channel_spec.rb index a31e322a067..19184248c38 100644 --- a/plugins/chat/spec/models/chat/category_channel_spec.rb +++ b/plugins/chat/spec/models/chat/category_channel_spec.rb @@ -89,12 +89,14 @@ RSpec.describe Chat::CategoryChannel do end describe "#leave" do - let(:original_method) { channel.method(:remove) } - let(:aliased_method) { channel.method(:leave) } + let(:public_category) { Fabricate(:category, read_restricted: false) } + let(:channel) { Fabricate(:category_channel, chatable: public_category) } - it "is an alias to '#remove'" do - expect(original_method.original_name).to eq(aliased_method.original_name) - expect(original_method.source_location).to eq(aliased_method.source_location) + it "calls #remove" do + Fabricate(:user_chat_channel_membership, chat_channel: channel) + user = channel.user_chat_channel_memberships.first.user + channel.expects(:remove).with(user) + channel.leave(user) end end diff --git a/plugins/chat/spec/models/chat/direct_message_channel_spec.rb b/plugins/chat/spec/models/chat/direct_message_channel_spec.rb index f33c9d4e7a0..645eb14015b 100644 --- a/plugins/chat/spec/models/chat/direct_message_channel_spec.rb +++ b/plugins/chat/spec/models/chat/direct_message_channel_spec.rb @@ -6,7 +6,6 @@ RSpec.describe Chat::DirectMessageChannel do it_behaves_like "a chat channel model" it { is_expected.to delegate_method(:allowed_user_ids).to(:direct_message).as(:user_ids) } - it { is_expected.to delegate_method(:group?).to(:direct_message).with_prefix.allow_nil } it { is_expected.to validate_length_of(:description).is_at_most(500) } it { is_expected.to validate_length_of(:chatable_type).is_at_most(100) } it { is_expected.to validate_length_of(:type).is_at_most(100) } @@ -110,4 +109,15 @@ RSpec.describe Chat::DirectMessageChannel do expect(channel.slug).to eq(nil) end end + + describe "#direct_message_group?" do + it "returns false if the DirectMessage chatable is not for a group DM" do + channel.chatable.update!(group: false) + expect(channel).not_to be_direct_message_group + end + it "returns true if the DirectMessage chatable is for a group DM" do + channel.chatable.update!(group: true) + expect(channel).to be_direct_message_group + end + end end diff --git a/plugins/chat/spec/requests/chat/api/channels_current_user_membership_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_current_user_membership_controller_spec.rb index da6b2f1f288..070e7237d99 100644 --- a/plugins/chat/spec/requests/chat/api/channels_current_user_membership_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channels_current_user_membership_controller_spec.rb @@ -34,7 +34,7 @@ describe Chat::Api::ChannelsCurrentUserMembershipController do end end - context "when channel is channel" do + context "when channel is a category channel" do context "when current user can't write in channel" do fab!(:private_category_1) { Fabricate(:private_category, group: Fabricate(:group)) } fab!(:readonly_group_1) { Fabricate(:group) } diff --git a/plugins/chat/spec/requests/chat/api/channels_current_user_membership_follows_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_current_user_membership_follows_controller_spec.rb index b581fb6986e..caecce0ee9e 100644 --- a/plugins/chat/spec/requests/chat/api/channels_current_user_membership_follows_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channels_current_user_membership_follows_controller_spec.rb @@ -2,7 +2,6 @@ describe Chat::Api::ChannelsCurrentUserMembershipController do fab!(:current_user) { Fabricate(:user) } - fab!(:channel_1) { Fabricate(:category_channel) } before do channel_1.add(current_user) @@ -12,7 +11,33 @@ describe Chat::Api::ChannelsCurrentUserMembershipController do end describe "#destroy" do - describe "success" do + describe "for a category channel" do + fab!(:channel_1) { Fabricate(:category_channel) } + + it "works" do + delete "/chat/api/channels/#{channel_1.id}/memberships/me/follows" + + expect(response.status).to eq(200) + expect(channel_1.membership_for(current_user).following).to eq(false) + end + + context "when channel is not found" do + before { channel_1.destroy! } + it "returns a 404" do + delete "/chat/api/channels/-999/memberships/me/follows" + + expect(response.status).to eq(404) + end + end + end + + describe "for a group direct message channel" do + fab!(:other_user_1) { Fabricate(:user) } + fab!(:other_user_2) { Fabricate(:user) } + fab!(:channel_1) do + Fabricate(:direct_message_channel, users: [current_user, other_user_1, other_user_2]) + end + it "works" do delete "/chat/api/channels/#{channel_1.id}/memberships/me/follows" @@ -21,11 +46,15 @@ describe Chat::Api::ChannelsCurrentUserMembershipController do end end - context "when channel is not found" do - it "returns a 404" do - delete "/chat/api/channels/-999/memberships/me/follows" + describe "for a direct message channel" do + fab!(:other_user_1) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:direct_message_channel, users: [current_user, other_user_1]) } - expect(response.status).to eq(404) + it "works" do + delete "/chat/api/channels/#{channel_1.id}/memberships/me/follows" + + expect(response.status).to eq(200) + expect(channel_1.membership_for(current_user).following).to eq(false) end end end