From aaf47c02bcf86be6c9749342e7b38589a122c067 Mon Sep 17 00:00:00 2001 From: Jan Cernik <66427541+jancernik@users.noreply.github.com> Date: Mon, 4 Sep 2023 11:55:02 -0300 Subject: [PATCH] DEV: Refactor chat oneboxes (#23031) - moves the onebox logic away from `plugin.rb` to a new `onebox_handler` lib - splits the `discourse_chat_message` template into two: one for channels, and one for messages - refactors the logic code slightly to send only the necessary arguments to each template This commit shouldn't change end-user behavior. --- plugins/chat/lib/chat/engine.rb | 14 +- plugins/chat/lib/chat/onebox_handler.rb | 86 +++++++++++ .../templates/discourse_chat_channel.mustache | 26 ++++ ...stache => discourse_chat_message.mustache} | 31 ---- plugins/chat/plugin.rb | 57 +------- .../chat/spec/lib/chat/onebox_handler_spec.rb | 136 ++++++++++++++++++ plugins/chat/spec/plugin_spec.rb | 68 +-------- 7 files changed, 262 insertions(+), 156 deletions(-) create mode 100644 plugins/chat/lib/chat/onebox_handler.rb create mode 100644 plugins/chat/lib/onebox/templates/discourse_chat_channel.mustache rename plugins/chat/lib/onebox/templates/{discourse_chat.mustache => discourse_chat_message.mustache} (55%) create mode 100644 plugins/chat/spec/lib/chat/onebox_handler_spec.rb diff --git a/plugins/chat/lib/chat/engine.rb b/plugins/chat/lib/chat/engine.rb index 38cca8ab7ba..7f38ccd7c43 100644 --- a/plugins/chat/lib/chat/engine.rb +++ b/plugins/chat/lib/chat/engine.rb @@ -13,10 +13,18 @@ module ::Chat SiteSetting.chat_allowed_groups_map end - def self.onebox_template - @onebox_template ||= + def self.message_onebox_template + @message_onebox_template ||= begin - path = "#{Rails.root}/plugins/chat/lib/onebox/templates/discourse_chat.mustache" + path = "#{Rails.root}/plugins/chat/lib/onebox/templates/discourse_chat_message.mustache" + File.read(path) + end + end + + def self.channel_onebox_template + @channel_onebox_template ||= + begin + path = "#{Rails.root}/plugins/chat/lib/onebox/templates/discourse_chat_channel.mustache" File.read(path) end end diff --git a/plugins/chat/lib/chat/onebox_handler.rb b/plugins/chat/lib/chat/onebox_handler.rb new file mode 100644 index 00000000000..ca5462491a9 --- /dev/null +++ b/plugins/chat/lib/chat/onebox_handler.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +module Chat + class OneboxHandler + 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 + else + chat_channel = Chat::Channel.find_by(id: route[:channel_id]) + return if !chat_channel + end + + return if !Guardian.new.can_preview_chat_channel?(chat_channel) + + args = build_args(url, chat_channel) + + if message.present? + render_message_onebox(args, message) + else + render_channel_onebox(args, chat_channel) + end + end + + private + + def self.build_args(url, chat_channel) + args = { + url: url, + channel_id: chat_channel.id, + channel_name: chat_channel.name, + is_category: chat_channel.category_channel?, + color: chat_channel.category_channel? ? chat_channel.chatable.color : nil, + } + end + + def self.render_message_onebox(args, message) + args.merge!( + message_id: message.id, + username: message.user.username, + avatar_url: message.user.avatar_template_url.gsub("{size}", "20"), + cooked: message.cooked, + created_at: message.created_at, + created_at_str: message.created_at.iso8601, + ) + + Mustache.render(Chat.message_onebox_template, args) + end + + def self.render_channel_onebox(args, chat_channel) + users = build_users_list(chat_channel) + + remaining_user_count_str = build_remaining_user_count_str(chat_channel, users) + + args.merge!( + users: users, + user_count_str: I18n.t("chat.onebox.x_members", count: chat_channel.user_count), + remaining_user_count_str: remaining_user_count_str, + description: chat_channel.description, + ) + + Mustache.render(Chat.channel_onebox_template, args) + end + + def self.build_users_list(chat_channel) + Chat::ChannelMembershipsQuery + .call(channel: chat_channel, limit: 10) + .map do |membership| + { + username: membership.user.username, + avatar_url: membership.user.avatar_template_url.gsub("{size}", "60"), + } + end + end + + def self.build_remaining_user_count_str(chat_channel, users) + if chat_channel.user_count > users.size + I18n.t("chat.onebox.and_x_others", count: chat_channel.user_count - users.size) + end + end + end +end diff --git a/plugins/chat/lib/onebox/templates/discourse_chat_channel.mustache b/plugins/chat/lib/onebox/templates/discourse_chat_channel.mustache new file mode 100644 index 00000000000..57b0805d16a --- /dev/null +++ b/plugins/chat/lib/onebox/templates/discourse_chat_channel.mustache @@ -0,0 +1,26 @@ + diff --git a/plugins/chat/lib/onebox/templates/discourse_chat.mustache b/plugins/chat/lib/onebox/templates/discourse_chat_message.mustache similarity index 55% rename from plugins/chat/lib/onebox/templates/discourse_chat.mustache rename to plugins/chat/lib/onebox/templates/discourse_chat_message.mustache index c0fdf1ff9d4..6646aa8ed93 100644 --- a/plugins/chat/lib/onebox/templates/discourse_chat.mustache +++ b/plugins/chat/lib/onebox/templates/discourse_chat_message.mustache @@ -1,33 +1,3 @@ -{{^cooked}} - -{{/cooked}} - -{{#cooked}}
@@ -55,4 +25,3 @@
{{{cooked}}}
-{{/cooked}} diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 7beb2d8f812..335630c795c 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -71,62 +71,7 @@ after_initialize do if Oneboxer.respond_to?(:register_local_handler) Oneboxer.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 - else - chat_channel = Chat::Channel.find_by(id: route[:channel_id]) - next if !chat_channel - end - - next if !Guardian.new.can_preview_chat_channel?(chat_channel) - - name = (chat_channel.name if chat_channel.name.present?) - - users = - chat_channel - .user_chat_channel_memberships - .includes(:user) - .where(user: User.activated.not_suspended.not_staged) - .limit(10) - .map do |membership| - { - username: membership.user.username, - avatar_url: membership.user.avatar_template_url.gsub("{size}", "60"), - } - end - - remaining_user_count_str = - if chat_channel.user_count > users.size - I18n.t("chat.onebox.and_x_others", count: chat_channel.user_count - users.size) - end - - args = { - url: url, - channel_id: chat_channel.id, - channel_name: name, - description: chat_channel.description, - user_count_str: I18n.t("chat.onebox.x_members", count: chat_channel.user_count), - users: users, - remaining_user_count_str: remaining_user_count_str, - is_category: chat_channel.chatable_type == "Category", - color: chat_channel.chatable_type == "Category" ? chat_channel.chatable.color : nil, - } - - if message.present? - args[:message_id] = message.id - args[:username] = message.user.username - args[:avatar_url] = message.user.avatar_template_url.gsub("{size}", "20") - args[:cooked] = message.cooked - args[:created_at] = message.created_at - args[:created_at_str] = message.created_at.iso8601 - end - - Mustache.render(Chat.onebox_template, args) + Chat::OneboxHandler.handle(url, route) end end diff --git a/plugins/chat/spec/lib/chat/onebox_handler_spec.rb b/plugins/chat/spec/lib/chat/onebox_handler_spec.rb new file mode 100644 index 00000000000..275be14d849 --- /dev/null +++ b/plugins/chat/spec/lib/chat/onebox_handler_spec.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe Chat::OneboxHandler do + fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) } + fab!(:private_channel) { Fabricate(:category_channel, chatable: private_category) } + fab!(:public_channel) { Fabricate(:category_channel) } + fab!(:user) { Fabricate(: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" } + + describe "chat channel" do + context "when valid" do + it "renders channel onebox, excluding inactive, staged, and suspended users" do + public_channel.add(user) + public_channel.add(user_2) + public_channel.add(user_3) + public_channel.add(user_4) + Chat::Channel.ensure_consistency! + + onebox_html = Chat::OneboxHandler.handle(public_chat_url, { channel_id: public_channel.id }) + + expect(onebox_html).to match_html <<~HTML + + + HTML + end + end + + context "when channel is private" do + it "does not create a onebox" do + onebox_html = + Chat::OneboxHandler.handle(private_chat_url, { channel_id: private_channel.id }) + + expect(onebox_html).to be_nil + end + end + + context "when channel does not exists" do + it "does not raise an error" do + onebox_html = Chat::OneboxHandler.handle(invalid_chat_url, { channel_id: 999 }) + + expect(onebox_html).to be_nil + end + end + end + + describe "chat message" do + fab!(:public_message) do + Fabricate(:chat_message, chat_channel: public_channel, user: user, message: "Hello world!") + end + fab!(:private_message) do + Fabricate(:chat_message, chat_channel: private_channel, user: user, message: "Hello world!") + end + + context "when valid" do + it "renders message onebox" do + onebox_html = + Chat::OneboxHandler.handle( + "#{public_chat_url}/#{public_message.id}", + { channel_id: public_channel.id, message_id: public_message.id }, + ) + + expect(onebox_html).to match_html <<~HTML +
+ +

Hello world!

+
+ HTML + end + end + + context "when channel is private" do + it "does not create a onebox" do + onebox_html = + Chat::OneboxHandler.handle( + "#{private_chat_url}/#{private_message.id}", + { channel_id: private_channel.id, message_id: private_message.id }, + ) + + expect(onebox_html).to be_nil + end + end + + context "when message does not exists" do + it "does not raise an error" do + onebox_html = + Chat::OneboxHandler.handle( + "#{public_chat_url}/999", + { channel_id: public_channel.id, message_id: 999 }, + ) + + expect(onebox_html).to be_nil + end + end + end +end diff --git a/plugins/chat/spec/plugin_spec.rb b/plugins/chat/spec/plugin_spec.rb index 84be69df448..d03c60e7d74 100644 --- a/plugins/chat/spec/plugin_spec.rb +++ b/plugins/chat/spec/plugin_spec.rb @@ -127,22 +127,14 @@ describe Chat do describe "chat oneboxes" do fab!(:chat_channel) { Fabricate(:category_channel) } - fab!(:user) { Fabricate(:user, active: true) } - 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) } + fab!(:user) { Fabricate(:user) } - let!(:chat_message) do + 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}" } - before do - chat_channel.update!(last_message: chat_message) - chat_channel.add(user) - end - context "when inline" do it "renders channel" do results = InlineOneboxer.new([chat_url], skip_cache: true).process @@ -160,62 +152,6 @@ describe Chat do ) end end - - context "when regular" do - it "renders channel, excluding inactive, staged, and suspended users" do - user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true) - user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true) - user_4.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true) - Chat::Channel.ensure_consistency! - - expect(Oneboxer.preview(chat_url)).to match_html <<~HTML - - - HTML - end - - it "renders messages" do - expect(Oneboxer.preview("#{chat_url}/#{chat_message.id}")).to match_html <<~HTML -
- -

Hello world!

-
- HTML - end - end end describe "auto-joining users to a channel" do