From 1377186d3875847b961f4e21ae38e1178677d0db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Tue, 25 Jul 2023 15:50:37 +0200 Subject: [PATCH] DEV: Refactor chat channel fetching This is extracted from #22390. This patch introduces a scope to avoid duplication and a new method, `Chat::Channel.find_by_id_or_slug` to allow finding a channel either by its id or by its slug (or its category slug). --- plugins/chat/app/models/chat/channel.rb | 18 +- plugins/chat/lib/chat/channel_fetcher.rb | 30 +--- plugins/chat/spec/models/chat/channel_spec.rb | 156 ++++++++++++------ 3 files changed, 121 insertions(+), 83 deletions(-) diff --git a/plugins/chat/app/models/chat/channel.rb b/plugins/chat/app/models/chat/channel.rb index 8c108a05534..077a7d7f79e 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/plugins/chat/app/models/chat/channel.rb @@ -39,12 +39,16 @@ module Chat validate :ensure_slug_ok, if: :slug_changed? before_validation :generate_auto_slug + scope :with_categories, + -> { + joins( + "LEFT JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'", + ) + } scope :public_channels, -> { - where(chatable_type: public_channel_chatable_types).where( + with_categories.where(chatable_type: public_channel_chatable_types).where( "categories.id IS NOT NULL", - ).joins( - "LEFT JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'", ) } @@ -74,6 +78,14 @@ module Chat def chatable_types public_channel_chatable_types + direct_channel_chatable_types end + + def find_by_id_or_slug(id) + with_categories.find_by( + "chat_channels.id = :id OR categories.slug = :slug OR chat_channels.slug = :slug", + id: Integer(id, exception: false), + slug: id.to_s.downcase, + ) + end end statuses.keys.each do |status| diff --git a/plugins/chat/lib/chat/channel_fetcher.rb b/plugins/chat/lib/chat/channel_fetcher.rb index 3f640637561..61112b5e3e3 100644 --- a/plugins/chat/lib/chat/channel_fetcher.rb +++ b/plugins/chat/lib/chat/channel_fetcher.rb @@ -76,9 +76,7 @@ module Chat allowed_channel_ids = generate_allowed_channel_ids_sql(guardian, exclude_dm_channels: true) Chat::Channel - .joins( - "LEFT JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'", - ) + .with_categories .where(chatable_type: Chat::Channel.public_channel_chatable_types) .where("chat_channels.id IN (#{allowed_channel_ids})") .where("chat_channels.slug IN (:slugs)", slugs: slugs) @@ -95,9 +93,7 @@ module Chat channels = channels - .joins( - "LEFT JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'", - ) + .with_categories .where(chatable_type: Chat::Channel.public_channel_chatable_types) .where("chat_channels.id IN (#{allowed_channel_ids})") @@ -249,30 +245,14 @@ module Chat ).report end - def self.find_with_access_check(channel_id_or_name, guardian) - begin - channel_id_or_name = Integer(channel_id_or_name) - rescue ArgumentError - end - - base_channel_relation = - Chat::Channel.includes(:chatable).joins( - "LEFT JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'", - ) + def self.find_with_access_check(channel_id_or_slug, guardian) + base_channel_relation = Chat::Channel.includes(:chatable) if guardian.user.staff? base_channel_relation = base_channel_relation.includes(:chat_channel_archive) end - if channel_id_or_name.is_a? Integer - chat_channel = base_channel_relation.find_by(id: channel_id_or_name) - else - chat_channel = - base_channel_relation.find_by( - "LOWER(categories.name) = :name OR LOWER(chat_channels.name) = :name", - name: channel_id_or_name.downcase, - ) - end + chat_channel = base_channel_relation.find_by_id_or_slug(channel_id_or_slug) raise Discourse::NotFound if chat_channel.blank? raise Discourse::InvalidAccess if !guardian.can_join_chat_channel?(chat_channel) diff --git a/plugins/chat/spec/models/chat/channel_spec.rb b/plugins/chat/spec/models/chat/channel_spec.rb index a634588f7ad..6d8cd244db9 100644 --- a/plugins/chat/spec/models/chat/channel_spec.rb +++ b/plugins/chat/spec/models/chat/channel_spec.rb @@ -1,74 +1,120 @@ # frozen_string_literal: true RSpec.describe Chat::Channel do - fab!(:category_channel1) { Fabricate(:category_channel) } - fab!(:dm_channel1) { Fabricate(:direct_message_channel) } + fab!(:category_channel_1) { Fabricate(:category_channel) } + fab!(:dm_channel_1) { Fabricate(:direct_message_channel) } + + describe ".find_by_id_or_slug" do + subject(:find_channel) { described_class.find_by_id_or_slug(channel_id) } + + context "when the channel is a direct message one" do + let(:channel_id) { dm_channel_1.id } + + it "finds it" do + expect(find_channel).to eq dm_channel_1 + end + end + + context "when the channel is a category one" do + context "when providing its id" do + let(:channel_id) { category_channel_1.id } + + it "finds it" do + expect(find_channel).to eq category_channel_1 + end + end + + context "when providing its slug" do + let(:channel_id) { category_channel_1.slug } + + it "finds it" do + expect(find_channel).to eq category_channel_1 + end + end + + context "when providing its category slug" do + let(:channel_id) { category_channel_1.category.slug } + + it "finds it" do + expect(find_channel).to eq category_channel_1 + end + end + end + + context "when providing a non existent id" do + let(:channel_id) { -1 } + + it "returns nothing" do + expect(find_channel).to be_blank + end + end + end describe "#relative_url" do context "when the slug is nil" do it "uses a - instead" do - category_channel1.slug = nil - expect(category_channel1.relative_url).to eq("/chat/c/-/#{category_channel1.id}") + category_channel_1.slug = nil + expect(category_channel_1.relative_url).to eq("/chat/c/-/#{category_channel_1.id}") end end context "when the slug is not nil" do - before { category_channel1.update!(slug: "some-cool-channel") } + before { category_channel_1.update!(slug: "some-cool-channel") } it "includes the slug for the channel" do - expect(category_channel1.relative_url).to eq( - "/chat/c/some-cool-channel/#{category_channel1.id}", + expect(category_channel_1.relative_url).to eq( + "/chat/c/some-cool-channel/#{category_channel_1.id}", ) end end end describe ".ensure_consistency!" do - fab!(:category_channel2) { Fabricate(:category_channel) } + fab!(:category_channel_2) { Fabricate(:category_channel) } describe "updating messages_count for all channels" do - fab!(:category_channel3) { Fabricate(:category_channel) } - fab!(:category_channel4) { Fabricate(:category_channel) } - fab!(:dm_channel2) { Fabricate(:direct_message_channel) } + fab!(:category_channel_3) { Fabricate(:category_channel) } + fab!(:category_channel_4) { Fabricate(:category_channel) } + fab!(:dm_channel_2) { Fabricate(:direct_message_channel) } before do - Fabricate(:chat_message, chat_channel: category_channel1) - Fabricate(:chat_message, chat_channel: category_channel1) - Fabricate(:chat_message, chat_channel: category_channel1) + Fabricate(:chat_message, chat_channel: category_channel_1) + Fabricate(:chat_message, chat_channel: category_channel_1) + Fabricate(:chat_message, chat_channel: category_channel_1) - Fabricate(:chat_message, chat_channel: category_channel2) - Fabricate(:chat_message, chat_channel: category_channel2) - Fabricate(:chat_message, chat_channel: category_channel2) - Fabricate(:chat_message, chat_channel: category_channel2) + Fabricate(:chat_message, chat_channel: category_channel_2) + Fabricate(:chat_message, chat_channel: category_channel_2) + Fabricate(:chat_message, chat_channel: category_channel_2) + Fabricate(:chat_message, chat_channel: category_channel_2) - Fabricate(:chat_message, chat_channel: category_channel3) + Fabricate(:chat_message, chat_channel: category_channel_3) - Fabricate(:chat_message, chat_channel: dm_channel2) - Fabricate(:chat_message, chat_channel: dm_channel2) + Fabricate(:chat_message, chat_channel: dm_channel_2) + Fabricate(:chat_message, chat_channel: dm_channel_2) end it "counts correctly" do described_class.ensure_consistency! - expect(category_channel1.reload.messages_count).to eq(3) - expect(category_channel2.reload.messages_count).to eq(4) - expect(category_channel3.reload.messages_count).to eq(1) - expect(category_channel4.reload.messages_count).to eq(0) - expect(dm_channel1.reload.messages_count).to eq(0) - expect(dm_channel2.reload.messages_count).to eq(2) + expect(category_channel_1.reload.messages_count).to eq(3) + expect(category_channel_2.reload.messages_count).to eq(4) + expect(category_channel_3.reload.messages_count).to eq(1) + expect(category_channel_4.reload.messages_count).to eq(0) + expect(dm_channel_1.reload.messages_count).to eq(0) + expect(dm_channel_2.reload.messages_count).to eq(2) end it "does not count deleted messages" do - category_channel3.chat_messages.last.trash! + category_channel_3.chat_messages.last.trash! described_class.ensure_consistency! - expect(category_channel3.reload.messages_count).to eq(0) + expect(category_channel_3.reload.messages_count).to eq(0) end it "does not update deleted channels" do described_class.ensure_consistency! - category_channel3.chat_messages.last.trash! - category_channel3.trash! + category_channel_3.chat_messages.last.trash! + category_channel_3.trash! described_class.ensure_consistency! - expect(category_channel3.reload.messages_count).to eq(1) + expect(category_channel_3.reload.messages_count).to eq(1) end end @@ -80,29 +126,29 @@ RSpec.describe Chat::Channel do def create_memberships user_1.user_chat_channel_memberships.create!( - chat_channel: category_channel1, + chat_channel: category_channel_1, following: true, ) user_1.user_chat_channel_memberships.create!( - chat_channel: category_channel2, + chat_channel: category_channel_2, following: true, ) user_2.user_chat_channel_memberships.create!( - chat_channel: category_channel1, + chat_channel: category_channel_1, following: true, ) user_2.user_chat_channel_memberships.create!( - chat_channel: category_channel2, + chat_channel: category_channel_2, following: true, ) user_3.user_chat_channel_memberships.create!( - chat_channel: category_channel1, + chat_channel: category_channel_1, following: false, ) user_3.user_chat_channel_memberships.create!( - chat_channel: category_channel2, + chat_channel: category_channel_2, following: true, ) end @@ -112,25 +158,25 @@ RSpec.describe Chat::Channel do described_class.ensure_consistency! - expect(category_channel1.reload.user_count).to eq(2) - expect(category_channel2.reload.user_count).to eq(3) + expect(category_channel_1.reload.user_count).to eq(2) + expect(category_channel_2.reload.user_count).to eq(3) end it "does not count suspended, non-activated, nor staged users" do user_1.user_chat_channel_memberships.create!( - chat_channel: category_channel1, + chat_channel: category_channel_1, following: true, ) user_2.user_chat_channel_memberships.create!( - chat_channel: category_channel2, + chat_channel: category_channel_2, following: true, ) user_3.user_chat_channel_memberships.create!( - chat_channel: category_channel2, + chat_channel: category_channel_2, following: true, ) user_4.user_chat_channel_memberships.create!( - chat_channel: category_channel2, + chat_channel: category_channel_2, following: true, ) user_2.update(suspended_till: 3.weeks.from_now) @@ -139,20 +185,20 @@ RSpec.describe Chat::Channel do described_class.ensure_consistency! - expect(category_channel1.reload.user_count).to eq(1) - expect(category_channel2.reload.user_count).to eq(0) + expect(category_channel_1.reload.user_count).to eq(1) + expect(category_channel_2.reload.user_count).to eq(0) end it "does not count archived, or read_only channels" do create_memberships - category_channel1.update!(status: :archived) + category_channel_1.update!(status: :archived) described_class.ensure_consistency! - expect(category_channel1.reload.user_count).to eq(0) + expect(category_channel_1.reload.user_count).to eq(0) - category_channel1.update!(status: :read_only) + category_channel_1.update!(status: :read_only) described_class.ensure_consistency! - expect(category_channel1.reload.user_count).to eq(0) + expect(category_channel_1.reload.user_count).to eq(0) end it "publishes all the updated channels" do @@ -163,9 +209,9 @@ RSpec.describe Chat::Channel do expect(messages.length).to eq(3) expect(messages.map(&:data)).to match_array( [ - { chat_channel_id: category_channel1.id, memberships_count: 2 }, - { chat_channel_id: category_channel2.id, memberships_count: 3 }, - { chat_channel_id: dm_channel1.id, memberships_count: 2 }, + { chat_channel_id: category_channel_1.id, memberships_count: 2 }, + { chat_channel_id: category_channel_2.id, memberships_count: 3 }, + { chat_channel_id: dm_channel_1.id, memberships_count: 2 }, ], ) @@ -177,11 +223,11 @@ RSpec.describe Chat::Channel do describe "#allow_channel_wide_mentions" do it "defaults to true" do - expect(category_channel1.allow_channel_wide_mentions).to be(true) + expect(category_channel_1.allow_channel_wide_mentions).to be(true) end it "cant be nullified" do - expect { category_channel1.update!(allow_channel_wide_mentions: nil) }.to raise_error( + expect { category_channel_1.update!(allow_channel_wide_mentions: nil) }.to raise_error( ActiveRecord::NotNullViolation, ) end