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.

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,

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}"

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)
+    # Group DMs are DMs with > 2 users
-      return super unless direct_message_group?
+    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

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(

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:)

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(

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)],
+      users: users || [Fabricate(:user), Fabricate(:user)],
-      group: attrs[:group] || false,
+      group: attrs[:group] || (users ? users.length > 2 : false),
     )
   end
   status { :open }

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
+    context "when the channel is a group DM" do
-      membership2.update!(
+      fab!(:membership3) do
-        notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always],
+        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
+    context "when the channel is a regular DM" do
-      messages = notification_messages_for(user2)
+      before do
+        membership2.update!(
+          notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always],
+        )
+      end
 
-      expect(messages.first.data).to include(
+      it "sends a desktop notification" do
-        {
+        messages = notification_messages_for(user2)
-          username: user1.username,
+
-          notification_type: Notification.types[:chat_message],
+        expect(messages.first.data).to include(
-          post_url: message.url,
+          {
-          translated_title:
+            username: user1.username,
-            I18n.t(
+            notification_type: Notification.types[:chat_message],
-              "discourse_push_notifications.popup.new_direct_chat_message",
+            post_url: message.url,
-              { username: user1.username, channel: channel.title(user2) },
+            translated_title:
-            ),
+              I18n.t(
-          tag: Chat::Notifier.push_notification_tag(:message, channel.id),
+                "discourse_push_notifications.popup.new_direct_chat_message",
-          excerpt: message.push_notification_excerpt,
+                { 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

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(:public_category) { Fabricate(:category, read_restricted: false) }
-    let(:aliased_method) { channel.method(:leave) }
+    let(:channel) { Fabricate(:category_channel, chatable: public_category) }
 
-    it "is an alias to '#remove'" do
+    it "calls #remove" do
-      expect(original_method.original_name).to eq(aliased_method.original_name)
+      Fabricate(:user_chat_channel_membership, chat_channel: channel)
-      expect(original_method.source_location).to eq(aliased_method.source_location)
+      user = channel.user_chat_channel_memberships.first.user
+      channel.expects(:remove).with(user)
+      channel.leave(user)
     end
   end
 

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

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) }

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
+    describe "for a direct message channel" do
-      it "returns a 404" do
+      fab!(:other_user_1) { Fabricate(:user) }
-        delete "/chat/api/channels/-999/memberships/me/follows"
+      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