FIX: correctly makes dm creator to follow channel (#22470)

In previous changes we prevented creating a channel to also make users follow the channel. We were forcing recipients to follow the channel on message sent but this was not including the creator of the message itself.

This commit fixes it and also write an end-to-end system spec to cover these cases. The message creator service is currently being rewritten and should correctly test and ensure this logic is present.

This commit also makes changes on the frontend to instantly follow a DM when you open it, this change prevents a green dot to appear for a split second when you send a message in a channel you were previously not following. Only recipients will see the green dot.

plugins/chat/app/controllers/chat/chat_controller.rb

@@ -118,22 +118,21 @@ module Chat
         # If any of the channel users is ignoring, muting, or preventing DMs from
         # the current user then we should not auto-follow the channel once again or
         # publish the new channel.
-        user_ids_allowing_communication =
+        allowed_user_ids =
           UserCommScreener.new(
             acting_user: current_user,
             target_user_ids:
               @chat_channel.user_chat_channel_memberships.where(following: false).pluck(:user_id),
           ).allowing_actor_communication
 
-        if user_ids_allowing_communication.any?
+        allowed_user_ids << current_user.id if !@user_chat_channel_membership.following
-          Chat::Publisher.publish_new_channel(
+
-            @chat_channel,
+        if allowed_user_ids.any?
-            User.where(id: user_ids_allowing_communication),
+          Chat::Publisher.publish_new_channel(@chat_channel, User.where(id: allowed_user_ids))
-          )
 
           @chat_channel
             .user_chat_channel_memberships
-            .where(user_id: user_ids_allowing_communication)
+            .where(user_id: allowed_user_ids)
             .update_all(following: true)
         end
       end

plugins/chat/app/services/chat/publisher.rb

@@ -372,7 +372,7 @@ module Chat
             serialized_channel =
               Chat::ChannelSerializer.new(
                 chat_channel,
-                scope: Guardian.new(membership.user), # We need a guardian here for direct messages
+                scope: membership.user.guardian, # We need a guardian here for direct messages
                 root: :channel,
                 membership: membership,
               ).as_json

plugins/chat/assets/javascripts/discourse/components/chat-channel.js

@@ -113,6 +113,13 @@ export default class ChatLivePane extends Component {
       return;
     }
 
+    if (
+      this.args.channel.isDirectMessageChannel &&
+      !this.args.channel.isFollowing
+    ) {
+      this.chatChannelsManager.follow(this.args.channel);
+    }
+
     // Technically we could keep messages to avoid re-fetching them, but
     // it's not worth the complexity for now
     this.args.channel.clearMessages();

plugins/chat/assets/javascripts/discourse/models/chat-channel.js

@@ -56,7 +56,6 @@ export default class ChatChannel {
     return new ChatChannel(args);
   }
 
-  @tracked currentUserMembership = null;
   @tracked title;
   @tracked slug;
   @tracked description;
@@ -82,6 +81,7 @@ export default class ChatChannel {
   messagesManager = new ChatMessagesManager(getOwner(this));
 
   @tracked _unreadThreadIds = new TrackedSet();
+  @tracked _currentUserMembership;
 
   constructor(args = {}) {
     this.id = args.id;
@@ -109,9 +109,7 @@ export default class ChatChannel {
           users: args.chatable?.users,
         })
       : Category.create(args.chatable);
-    this.currentUserMembership = UserChatChannelMembership.create(
+    this.currentUserMembership = args.current_user_membership;
-      args.current_user_membership
-    );
 
     if (args.archive_completed || args.archive_failed) {
       this.archive = ChatChannelArchive.create(args);
@@ -301,15 +299,17 @@ export default class ChatChannel {
     return !READONLY_STATUSES.includes(this.status);
   }
 
-  updateMembership(membership) {
+  get currentUserMembership() {
-    this.currentUserMembership.following = membership.following;
+    return this._currentUserMembership;
-    this.currentUserMembership.lastReadMessage_id =
+  }
-      membership.last_read_message_id;
+
-    this.currentUserMembership.desktopNotificationLevel =
+  set currentUserMembership(membership) {
-      membership.desktop_notification_level;
+    if (membership instanceof UserChatChannelMembership) {
-    this.currentUserMembership.mobileNotificationLevel =
+      this._currentUserMembership = membership;
-      membership.mobile_notification_level;
+    } else {
-    this.currentUserMembership.muted = membership.muted;
+      this._currentUserMembership =
+        UserChatChannelMembership.create(membership);
+    }
   }
 
   clearSelectedMessages() {

plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js

@@ -67,16 +67,11 @@ export default class ChatChannelsManager extends Service {
 
     if (!model.currentUserMembership.following) {
       return this.chatApi.followChannel(model.id).then((membership) => {
-        model.currentUserMembership.following = membership.following;
+        model.currentUserMembership = membership;
-        model.currentUserMembership.muted = membership.muted;
-        model.currentUserMembership.desktopNotificationLevel =
-          membership.desktopNotificationLevel;
-        model.currentUserMembership.mobileNotificationLevel =
-          membership.mobileNotificationLevel;
         return model;
       });
     } else {
-      return Promise.resolve(model);
+      return model;
     }
   }
 

plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js

@@ -354,7 +354,7 @@ export default class ChatSubscriptionsManager extends Service {
     this.chatChannelsManager.find(data.channel.id).then((channel) => {
       // we need to refresh here to have correct last message ids
       channel.meta = data.channel.meta;
-      channel.updateMembership(data.channel.current_user_membership);
+      channel.currentUserMembership = data.channel.current_user_membership;
 
       if (
         channel.isDirectMessageChannel &&

plugins/chat/assets/javascripts/discourse/services/chat.js

@@ -143,7 +143,7 @@ export default class Chat extends Service {
               channel.tracking.unreadCount = state.unread_count;
               channel.tracking.mentionCount = state.mention_count;
 
-              channel.updateMembership(channelObject.current_user_membership);
+              channel.updateMembership = channelObject.current_user_membership;
 
               this.chatSubscriptionsManager.startChannelSubscription(channel);
             });

plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb

@@ -238,45 +238,6 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
           end
         end
       end
-
-      context "with dm and public channel" do
-        fab!(:current_user) { Fabricate(:admin) }
-        fab!(:user_1) { Fabricate(:user) }
-        fab!(:channel_1) { Fabricate(:category_channel) }
-        fab!(:dm_channel_1) { Fabricate(:direct_message_channel, users: [current_user, user_1]) }
-
-        before do
-          channel_1.add(user_1)
-          channel_1.add(current_user)
-        end
-
-        context "when messages are created" do
-          xit "correctly renders notifications" do
-            using_session(:current_user) { visit("/") }
-
-            using_session(:user_1) { create_message(channel: channel_1, creator: user_1) }
-
-            using_session(:current_user) do
-              expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "")
-              expect(page).to have_css(".sidebar-row.channel-#{channel_1.id} .unread")
-            end
-
-            using_session(:user_1) do |session|
-              create_message(channel: dm_channel_1, creator: user_1)
-              session.quit
-            end
-
-            using_session(:current_user) do |session|
-              expect(page).to have_css(".sidebar-row.channel-#{dm_channel_1.id} .icon.urgent")
-              expect(page).to have_css(
-                ".chat-header-icon .chat-channel-unread-indicator",
-                text: "1",
-              )
-              session.quit
-            end
-          end
-        end
-      end
     end
   end
 end

plugins/chat/spec/system/page_objects/chat/chat.rb

@@ -7,6 +7,10 @@ module PageObjects
         @message_creator ||= PageObjects::Components::Chat::MessageCreator.new
       end
 
+      def sidebar
+        @sidebar ||= PageObjects::Components::Chat::Sidebar.new
+      end
+
       def prefers_full_page
         page.execute_script(
           "window.localStorage.setItem('discourse_chat_preferred_mode', '\"FULL_PAGE_CHAT\"');",

plugins/chat/spec/system/page_objects/chat/components/composer.rb

@@ -49,26 +49,32 @@ module PageObjects
         end
 
         def reply_to_last_message_shortcut
+          input.click
           input.send_keys(%i[shift arrow_up])
         end
 
         def edit_last_message_shortcut
+          input.click
           input.send_keys(%i[arrow_up])
         end
 
         def emphasized_text_shortcut
+          input.click
           input.send_keys([PLATFORM_KEY_MODIFIER, "i"])
         end
 
         def cancel_shortcut
+          input.click
           input.send_keys(:escape)
         end
 
         def indented_text_shortcut
+          input.click
           input.send_keys([PLATFORM_KEY_MODIFIER, "e"])
         end
 
         def bold_text_shortcut
+          input.click
           input.send_keys([PLATFORM_KEY_MODIFIER, "b"])
         end
 

plugins/chat/spec/system/page_objects/chat/components/sidebar.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Components
+    module Chat
+      class Sidebar < PageObjects::Components::Base
+        attr_reader :context
+
+        SELECTOR = "#d-sidebar"
+
+        def component
+          page.find(SELECTOR)
+        end
+
+        def has_direct_message_channel?(channel, **args)
+          channel_selector = direct_message_channel_selector(channel, **args)
+          predicate = component.has_css?(channel_selector)
+
+          if args[:unread]
+            predicate &&
+              component.has_css?("#{channel_selector} .sidebar-section-link-suffix.icon.unread")
+          elsif args[:mention]
+            predicate &&
+              component.has_css?("#{channel_selector} .sidebar-section-link-suffix.icon.urgent")
+          else
+            predicate &&
+              component.has_no_css?("#{channel_selector} .sidebar-section-link-suffix.icon")
+          end
+        end
+
+        def has_no_direct_message_channel?(channel, **args)
+          component.has_no_css?(direct_message_channel_selector(channel, **args))
+        end
+
+        def direct_message_channel_selector(channel, **args)
+          selector = "#sidebar-section-content-chat-dms"
+          selector += " .sidebar-section-link.channel-#{channel.id}"
+          selector
+        end
+      end
+    end
+  end
+end

plugins/chat/spec/system/send_message_spec.rb

@@ -0,0 +1,115 @@
+# frozen_string_literal: true
+
+RSpec.describe "Send message", type: :system do
+  fab!(:user_1) { Fabricate(:admin) }
+  fab!(:user_2) { Fabricate(:admin) }
+
+  let(:chat_page) { PageObjects::Pages::Chat.new }
+  let(:channel_page) { PageObjects::Pages::ChatChannel.new }
+
+  before do
+    # simpler user search without having to worry about user search data
+    SiteSetting.enable_names = false
+
+    chat_system_bootstrap
+  end
+
+  context "with direct message channels" do
+    context "when users are not following the channel" do
+      fab!(:channel_1) { Fabricate(:direct_message_channel, users: [user_1, user_2]) }
+
+      before do
+        channel_1.remove(user_1)
+        channel_1.remove(user_2)
+      end
+
+      it "shows correct state" do
+        using_session(:user_1) do
+          sign_in(user_1)
+          visit("/")
+
+          expect(chat_page.sidebar).to have_no_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_2) do
+          sign_in(user_2)
+          visit("/")
+
+          expect(chat_page.sidebar).to have_no_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_1) do
+          chat_page.open_new_message
+          chat_page.message_creator.filter(user_2.username)
+          chat_page.message_creator.click_row(user_2)
+
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_2) do
+          expect(chat_page.sidebar).to have_no_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_1) do |session|
+          channel_page.send_message
+
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1)
+
+          session.quit
+        end
+
+        using_session(:user_2) do |session|
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1, mention: true)
+
+          session.quit
+        end
+      end
+    end
+
+    context "when users are following the channel" do
+      fab!(:channel_1) { Fabricate(:direct_message_channel, users: [user_1, user_2]) }
+
+      it "shows correct state" do
+        using_session(:user_1) do
+          sign_in(user_1)
+          visit("/")
+
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_2) do
+          sign_in(user_2)
+          visit("/")
+
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_1) do
+          chat_page.open_new_message
+          chat_page.message_creator.filter(user_2.username)
+          chat_page.message_creator.click_row(user_2)
+
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_2) do
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1)
+        end
+
+        using_session(:user_1) do |session|
+          channel_page.send_message
+
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1)
+
+          session.quit
+        end
+
+        using_session(:user_2) do |session|
+          expect(chat_page.sidebar).to have_direct_message_channel(channel_1, mention: true)
+
+          session.quit
+        end
+      end
+    end
+  end
+end