From f20be4b092c1a4dfe927c74fe978900b91d0517e Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 11 May 2023 22:27:48 +0200 Subject: [PATCH] FIX: prevents subscribing with an old id (#21509) This issue was for example possibly causing the last visit indicator to be reset by `sent` messages events. The following was happening: - a user (bob) had a last message bus ID of 1 on a channel (id:1) subscription - bob then go to another channel (id:2), unsubscribing from updates of channel (id:1) - another user (laura) then send messages to channel (id:1) - bob goes back to channel (id:1) At this point we we doing in the same sequence: - loading channel with messages, getting a new last message bus id - subscribing to updates using the last known message bus id Most of the times we were lucky enough for this to work (no events while away, or just got the new id in time...) but it was also very likely to do a double fetch of messages as MessageBus would think we were late. --- .../discourse/components/chat-channel.js | 12 ++-- .../chat/spec/system/dates_separators_spec.rb | 66 +++++++++++++++++++ plugins/chat/spec/system/sticky_date_spec.rb | 32 --------- 3 files changed, 72 insertions(+), 38 deletions(-) create mode 100644 plugins/chat/spec/system/dates_separators_spec.rb delete mode 100644 plugins/chat/spec/system/sticky_date_spec.rb diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index f524241464a..c7b0d682252 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -85,7 +85,7 @@ export default class ChatLivePane extends Component { teardownListeners() { document.removeEventListener("scroll", this._forceBodyScroll); removeOnPresenceChange(this.onPresenceChangeCallback); - this._unsubscribeToUpdates(this._loadedChannelId); + this.unsubscribeToUpdates(this._loadedChannelId); this.requestedTargetMessageId = null; cancel(this._laterComputeHandler); } @@ -111,7 +111,7 @@ export default class ChatLivePane extends Component { this.args.channel?.clearMessages(); if (this._loadedChannelId !== this.args.channel?.id) { - this._unsubscribeToUpdates(this._loadedChannelId); + this.unsubscribeToUpdates(this._loadedChannelId); this.chatChannelPane.selectingMessages = false; this.chatChannelComposer.message = this.args.channel.draft || @@ -123,7 +123,6 @@ export default class ChatLivePane extends Component { } this.loadMessages(); - this._subscribeToUpdates(this.args.channel); } @action @@ -215,6 +214,7 @@ export default class ChatLivePane extends Component { this.loadingMorePast = false; this.fillPaneAttempt(); this.updateLastReadMessage(); + this.subscribeToUpdates(this.args.channel); }); } @@ -766,7 +766,7 @@ export default class ChatLivePane extends Component { }); } - _unsubscribeToUpdates(channelId) { + unsubscribeToUpdates(channelId) { if (!channelId) { return; } @@ -775,12 +775,12 @@ export default class ChatLivePane extends Component { this.messageBus.unsubscribe(`/chat/${channelId}`, this.onMessage); } - _subscribeToUpdates(channel) { + subscribeToUpdates(channel) { if (!channel) { return; } - this._unsubscribeToUpdates(channel.id); + this.unsubscribeToUpdates(channel.id); this.messageBus.subscribe( `/chat/${channel.id}`, this.onMessage, diff --git a/plugins/chat/spec/system/dates_separators_spec.rb b/plugins/chat/spec/system/dates_separators_spec.rb new file mode 100644 index 00000000000..520fa1eee31 --- /dev/null +++ b/plugins/chat/spec/system/dates_separators_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +RSpec.describe "Dates separators", type: :system, js: true do + fab!(:current_user) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:chat_channel) } + + let(:chat_page) { PageObjects::Pages::Chat.new } + let(:channel_page) { PageObjects::Pages::ChatChannel.new } + + before do + chat_system_bootstrap + channel_1.add(current_user) + sign_in(current_user) + end + + context "when today separator is out of screen" do + before do + 20.times { Fabricate(:chat_message, chat_channel: channel_1, created_at: 1.day.ago) } + 25.times { Fabricate(:chat_message, chat_channel: channel_1) } + end + + it "shows it as a sticky date" do + chat_page.visit_channel(channel_1) + + expect(page.find(".chat-message-separator__text-container.is-pinned")).to have_content( + I18n.t("js.chat.chat_message_separator.today"), + ) + expect(page).to have_css( + ".chat-message-separator__text-container:not(.is-pinned)", + visible: :hidden, + text: + "#{I18n.t("js.chat.chat_message_separator.yesterday")} - #{I18n.t("js.chat.last_visit")}", + ) + end + end + + context "when receiving messages on a different channel" do + fab!(:channel_2) { Fabricate(:chat_channel) } + fab!(:user_1) { Fabricate(:user) } + + before do + channel_2.add(current_user) + channel_1.add(user_1) + end + + it "doesn't impact the last visit separator" do + chat_page.visit_channel(channel_1) + channel_page.send_message("message1") + chat_page.visit_channel(channel_2) + + using_session(:user_1) do |session| + sign_in(user_1) + chat_page.visit_channel(channel_1) + channel_page.send_message("message2") + session.quit + end + + chat_page.visit_channel(channel_1) + + expect(page).to have_css( + ".chat-message-separator__text-container", + text: I18n.t("js.chat.last_visit"), + ) + end + end +end diff --git a/plugins/chat/spec/system/sticky_date_spec.rb b/plugins/chat/spec/system/sticky_date_spec.rb deleted file mode 100644 index 9f043afa9f9..00000000000 --- a/plugins/chat/spec/system/sticky_date_spec.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe "Sticky date", type: :system, js: true do - fab!(:current_user) { Fabricate(:user) } - fab!(:channel_1) { Fabricate(:category_channel) } - - let(:chat_page) { PageObjects::Pages::Chat.new } - - before do - chat_system_bootstrap - channel_1.add(current_user) - 20.times { Fabricate(:chat_message, chat_channel: channel_1, created_at: 1.day.ago) } - 25.times { Fabricate(:chat_message, chat_channel: channel_1) } - sign_in(current_user) - end - - context "when today separator is out of screen" do - it "shows it as a sticky date" do - chat_page.visit_channel(channel_1) - - expect(page.find(".chat-message-separator__text-container.is-pinned")).to have_content( - I18n.t("js.chat.chat_message_separator.today"), - ) - expect(page).to have_css( - ".chat-message-separator__text-container:not(.is-pinned)", - visible: :hidden, - text: - "#{I18n.t("js.chat.chat_message_separator.yesterday")} - #{I18n.t("js.chat.last_visit")}", - ) - end - end -end