From c0808b2537927aa01702a269cf1676d99843bf32 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 6 Jul 2023 21:42:19 +0200 Subject: [PATCH] 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. --- .../app/controllers/chat/chat_controller.rb | 13 +- plugins/chat/app/services/chat/publisher.rb | 2 +- .../discourse/components/chat-channel.js | 7 ++ .../discourse/models/chat-channel.js | 26 ++-- .../services/chat-channels-manager.js | 9 +- .../services/chat-subscriptions-manager.js | 2 +- .../javascripts/discourse/services/chat.js | 2 +- ...message_notifications_with_sidebar_spec.rb | 39 ------ .../spec/system/page_objects/chat/chat.rb | 4 + .../page_objects/chat/components/composer.rb | 6 + .../page_objects/chat/components/sidebar.rb | 43 +++++++ plugins/chat/spec/system/send_message_spec.rb | 115 ++++++++++++++++++ 12 files changed, 199 insertions(+), 69 deletions(-) create mode 100644 plugins/chat/spec/system/page_objects/chat/components/sidebar.rb create mode 100644 plugins/chat/spec/system/send_message_spec.rb diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index e0d9d3a9a79..66fa311a390 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/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? - Chat::Publisher.publish_new_channel( - @chat_channel, - User.where(id: user_ids_allowing_communication), - ) + allowed_user_ids << current_user.id if !@user_chat_channel_membership.following + + if allowed_user_ids.any? + 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 diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index 5f3c8b58b72..33da0ba24ed 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/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 diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index ccbefd36d57..04c9f32de3c 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/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(); diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index 135291c449b..e1e2a51dc71 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/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( - args.current_user_membership - ); + this.currentUserMembership = 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) { - this.currentUserMembership.following = membership.following; - this.currentUserMembership.lastReadMessage_id = - membership.last_read_message_id; - this.currentUserMembership.desktopNotificationLevel = - membership.desktop_notification_level; - this.currentUserMembership.mobileNotificationLevel = - membership.mobile_notification_level; - this.currentUserMembership.muted = membership.muted; + get currentUserMembership() { + return this._currentUserMembership; + } + + set currentUserMembership(membership) { + if (membership instanceof UserChatChannelMembership) { + this._currentUserMembership = membership; + } else { + this._currentUserMembership = + UserChatChannelMembership.create(membership); + } } clearSelectedMessages() { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js index 03aa219bb91..f42ad19f551 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js +++ b/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.muted = membership.muted; - model.currentUserMembership.desktopNotificationLevel = - membership.desktopNotificationLevel; - model.currentUserMembership.mobileNotificationLevel = - membership.mobileNotificationLevel; + model.currentUserMembership = membership; return model; }); } else { - return Promise.resolve(model); + return model; } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js index ee172ac0eca..e65a97b1775 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js +++ b/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 && diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index 1b12c8580d2..decadb20f61 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/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); }); diff --git a/plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb b/plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb index f10aa455ef9..656d16ab3f2 100644 --- a/plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb +++ b/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 diff --git a/plugins/chat/spec/system/page_objects/chat/chat.rb b/plugins/chat/spec/system/page_objects/chat/chat.rb index 590b2f2f2d8..a79d717b4a7 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat.rb +++ b/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\"');", diff --git a/plugins/chat/spec/system/page_objects/chat/components/composer.rb b/plugins/chat/spec/system/page_objects/chat/components/composer.rb index cdb69716222..07695148542 100644 --- a/plugins/chat/spec/system/page_objects/chat/components/composer.rb +++ b/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 diff --git a/plugins/chat/spec/system/page_objects/chat/components/sidebar.rb b/plugins/chat/spec/system/page_objects/chat/components/sidebar.rb new file mode 100644 index 00000000000..95984bfbbe4 --- /dev/null +++ b/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 diff --git a/plugins/chat/spec/system/send_message_spec.rb b/plugins/chat/spec/system/send_message_spec.rb new file mode 100644 index 00000000000..05e49104346 --- /dev/null +++ b/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