From dcb1e2a341d889d7d1e82a0356d384d8e3fb7699 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 20 Jan 2025 10:08:38 +1000 Subject: [PATCH] FEATURE: Chat thread inline oneboxes (#30834) Previously if you linked to a chat thread inline, our oneboxer did not have special handling for this, so the link would end up with the text "Chat #channel-name" which is not ideal for a thread. This commit makes it so the thread onebox is in the format "Thread title - #channel-name" if the thread title exists, otherwise we show "Thread in #channel-name" --- plugins/chat/config/locales/server.en.yml | 2 + .../chat/lib/chat/inline_onebox_handler.rb | 52 ++++++ plugins/chat/plugin.rb | 29 +-- .../lib/chat/inline_onebox_handler_spec.rb | 172 ++++++++++++++++++ plugins/chat/spec/plugin_spec.rb | 29 --- 5 files changed, 227 insertions(+), 57 deletions(-) create mode 100644 plugins/chat/lib/chat/inline_onebox_handler.rb create mode 100644 plugins/chat/spec/lib/chat/inline_onebox_handler_spec.rb diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index de3c24610fa..eff1f3074d0 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -184,6 +184,8 @@ en: inline_to_message: "Message #%{message_id} by %{username} – #%{chat_channel}" inline_to_channel: "Chat #%{chat_channel}" inline_to_topic_channel: "Chat for Topic %{topic_title}" + inline_to_thread: "%{thread_title} - #%{chat_channel}" + inline_to_thread_no_title: "Thread in #%{chat_channel}" thread_title_connector: "in" x_members: diff --git a/plugins/chat/lib/chat/inline_onebox_handler.rb b/plugins/chat/lib/chat/inline_onebox_handler.rb new file mode 100644 index 00000000000..79c40c82d93 --- /dev/null +++ b/plugins/chat/lib/chat/inline_onebox_handler.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Chat + class InlineOneboxHandler + def self.handle(url, route) + if route[:message_id].present? + message = Chat::Message.find_by(id: route[:message_id]) + return if !message + + chat_channel = message.chat_channel + user = message.user + return if !chat_channel || !user + + title = + I18n.t( + "chat.onebox.inline_to_message", + message_id: message.id, + chat_channel: chat_channel.name, + username: user.username, + ) + else + chat_channel = Chat::Channel.find_by(id: route[:channel_id]) + return if !chat_channel + + if route[:thread_id].present? + thread = Chat::Thread.find_by(id: route[:thread_id]) + return if !thread + + title = + if thread.title.present? + I18n.t( + "chat.onebox.inline_to_thread", + chat_channel: chat_channel.name, + thread_title: thread.title, + ) + else + I18n.t("chat.onebox.inline_to_thread_no_title", chat_channel: chat_channel.name) + end + else + title = + if chat_channel.name.present? + I18n.t("chat.onebox.inline_to_channel", chat_channel: chat_channel.name) + end + end + end + + return if !Guardian.new.can_preview_chat_channel?(chat_channel) + + { url: url, title: title } + end + end +end diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 07c61261ecb..6f23633a789 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -89,34 +89,7 @@ after_initialize do if InlineOneboxer.respond_to?(:register_local_handler) InlineOneboxer.register_local_handler("chat/chat") do |url, route| - if route[:message_id].present? - message = Chat::Message.find_by(id: route[:message_id]) - next if !message - - chat_channel = message.chat_channel - user = message.user - next if !chat_channel || !user - - title = - I18n.t( - "chat.onebox.inline_to_message", - message_id: message.id, - chat_channel: chat_channel.name, - username: user.username, - ) - else - chat_channel = Chat::Channel.find_by(id: route[:channel_id]) - next if !chat_channel - - title = - if chat_channel.name.present? - I18n.t("chat.onebox.inline_to_channel", chat_channel: chat_channel.name) - end - end - - next if !Guardian.new.can_preview_chat_channel?(chat_channel) - - { url: url, title: title } + Chat::InlineOneboxHandler.handle(url, route) end end diff --git a/plugins/chat/spec/lib/chat/inline_onebox_handler_spec.rb b/plugins/chat/spec/lib/chat/inline_onebox_handler_spec.rb new file mode 100644 index 00000000000..1a8444980bd --- /dev/null +++ b/plugins/chat/spec/lib/chat/inline_onebox_handler_spec.rb @@ -0,0 +1,172 @@ +# frozen_string_literal: true + +describe Chat::InlineOneboxHandler do + fab!(:private_category_group) { Fabricate(:group) } + fab!(:private_category) { Fabricate(:private_category, group: private_category_group) } + fab!(:private_channel) { Fabricate(:category_channel, chatable: private_category) } + fab!(:public_channel) { Fabricate(:category_channel) } + fab!(:user) + fab!(:user_2) { Fabricate(:user, active: false) } + fab!(:user_3) { Fabricate(:user, staged: true) } + fab!(:user_4) { Fabricate(:user, suspended_till: 3.weeks.from_now) } + + let(:public_chat_url) { "#{Discourse.base_url}/chat/c/-/#{public_channel.id}" } + let(:private_chat_url) { "#{Discourse.base_url}/chat/c/-/#{private_channel.id}" } + let(:invalid_chat_url) { "#{Discourse.base_url}/chat/c/-/999" } + + context "when the link is to a public channel" do + describe "channel" do + it "renders an inline onebox for the channel" do + expect( + Chat::InlineOneboxHandler.handle(public_chat_url, { channel_id: public_channel.id }), + ).to eq( + { + url: public_chat_url, + title: I18n.t("chat.onebox.inline_to_channel", chat_channel: public_channel.name), + }, + ) + end + + it "does not render an inline onebox for a channel which does not exist" do + public_channel.trash! + expect( + Chat::InlineOneboxHandler.handle(public_chat_url, { channel_id: public_channel.id }), + ).to be_nil + end + end + + describe "message" do + fab!(:message) { Fabricate(:chat_message, chat_channel: public_channel) } + let(:public_chat_message_url) do + "#{Discourse.base_url}/chat/c/-/#{public_channel.id}/#{message.id}" + end + + it "renders an inline onebox for a message" do + expect( + Chat::InlineOneboxHandler.handle( + public_chat_message_url, + { channel_id: public_channel.id, message_id: message.id }, + ), + ).to eq( + { + url: public_chat_message_url, + title: + I18n.t( + "chat.onebox.inline_to_message", + chat_channel: public_channel.name, + message_id: message.id, + username: message.user.username, + ), + }, + ) + end + + it "does not render an inline onebox for a message which does not exist" do + message.trash! + expect( + Chat::InlineOneboxHandler.handle( + public_chat_message_url, + { channel_id: public_channel.id, message_id: message.id }, + ), + ).to be_nil + end + end + + describe "thread" do + fab!(:thread) do + Fabricate(:chat_thread, channel: public_channel, title: "Let's talk about some games") + end + let(:public_chat_thread_url) do + "#{Discourse.base_url}/chat/c/-/#{public_channel.id}/t/#{thread.id}" + end + + it "renders an inline onebox for a thread" do + expect( + Chat::InlineOneboxHandler.handle( + public_chat_thread_url, + { channel_id: public_channel.id, thread_id: thread.id }, + ), + ).to eq( + { + url: public_chat_thread_url, + title: + I18n.t( + "chat.onebox.inline_to_thread", + chat_channel: public_channel.name, + thread_id: thread.id, + thread_title: thread.title, + ), + }, + ) + end + + it "renders an inline onebox for a thread with no title" do + thread.update!(title: nil) + expect( + Chat::InlineOneboxHandler.handle( + public_chat_thread_url, + { channel_id: public_channel.id, thread_id: thread.id }, + ), + ).to eq( + { + url: public_chat_thread_url, + title: + I18n.t( + "chat.onebox.inline_to_thread_no_title", + chat_channel: public_channel.name, + thread_id: thread.id, + thread_title: thread.title, + ), + }, + ) + end + + it "does not render an inline onebox for a thread which does not exist" do + thread.destroy! + expect( + Chat::InlineOneboxHandler.handle( + public_chat_thread_url, + { channel_id: public_channel.id, thread_id: thread.id }, + ), + ).to be_nil + end + end + end + + context "when the link is to a private channel" do + fab!(:message) { Fabricate(:chat_message, chat_channel: private_channel) } + fab!(:thread) do + Fabricate(:chat_thread, channel: private_channel, title: "Let's talk about some games") + end + let(:private_chat_thread_url) do + "#{Discourse.base_url}/chat/c/-/#{private_channel.id}/t/#{thread.id}" + end + let(:private_chat_message_url) do + "#{Discourse.base_url}/chat/c/-/#{private_channel.id}/#{message.id}" + end + + it "does not render an inline onebox for the channel for any users" do + expect( + Chat::InlineOneboxHandler.handle(private_chat_url, { channel_id: private_channel.id }), + ).to be_nil + end + + it "does not render an inline onebox for the channel message for any users" do + expect( + Chat::InlineOneboxHandler.handle( + private_chat_message_url, + { channel_id: private_channel.id, message_id: message.id }, + ), + ).to be_nil + end + + it "does not render an inline onebox for the channel thread for any users" do + expect( + Chat::InlineOneboxHandler.handle( + private_chat_thread_url, + { channel_id: private_channel.id, thread_id: thread.id }, + ), + ).to be_nil + end + end +end diff --git a/plugins/chat/spec/plugin_spec.rb b/plugins/chat/spec/plugin_spec.rb index 296d023f0fd..c1b4aae9b5e 100644 --- a/plugins/chat/spec/plugin_spec.rb +++ b/plugins/chat/spec/plugin_spec.rb @@ -152,35 +152,6 @@ describe Chat do end end - describe "chat oneboxes" do - fab!(:chat_channel) { Fabricate(:category_channel) } - fab!(:user) - - fab!(:chat_message) do - Fabricate(:chat_message, chat_channel: chat_channel, user: user, message: "Hello world!") - end - - let(:chat_url) { "#{Discourse.base_url}/chat/c/-/#{chat_channel.id}" } - - context "when inline" do - it "renders channel" do - results = InlineOneboxer.new([chat_url], skip_cache: true).process - expect(results).to be_present - expect(results[0][:url]).to eq(chat_url) - expect(results[0][:title]).to eq("Chat ##{chat_channel.name}") - end - - it "renders messages" do - results = InlineOneboxer.new(["#{chat_url}/#{chat_message.id}"], skip_cache: true).process - expect(results).to be_present - expect(results[0][:url]).to eq("#{chat_url}/#{chat_message.id}") - expect(results[0][:title]).to eq( - "Message ##{chat_message.id} by #{chat_message.user.username} – ##{chat_channel.name}", - ) - end - end - end - describe "auto-joining users to a channel" do fab!(:chatters_group) { Fabricate(:group) } fab!(:user) { Fabricate(:user, last_seen_at: 15.minutes.ago, trust_level: 1) }