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

@@ -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|

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(
+        .with_categories
-          "LEFT JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'",
-        )
         .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(
+          .with_categories
-            "LEFT JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'",
-          )
           .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)
+    def self.find_with_access_check(channel_id_or_slug, guardian)
-      begin
+      base_channel_relation = Chat::Channel.includes(:chatable)
-        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'",
-        )
 
       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_or_slug(channel_id_or_slug)
-        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
 
       raise Discourse::NotFound if chat_channel.blank?
       raise Discourse::InvalidAccess if !guardian.can_join_chat_channel?(chat_channel)

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!(:category_channel_1) { Fabricate(:category_channel) }
-  fab!(:dm_channel1) { Fabricate(:direct_message_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
+        category_channel_1.slug = nil
-        expect(category_channel1.relative_url).to eq("/chat/c/-/#{category_channel1.id}")
+        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(
+        expect(category_channel_1.relative_url).to eq(
-          "/chat/c/some-cool-channel/#{category_channel1.id}",
+          "/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_channel_3) { Fabricate(:category_channel) }
-      fab!(:category_channel4) { Fabricate(:category_channel) }
+      fab!(:category_channel_4) { Fabricate(:category_channel) }
-      fab!(:dm_channel2) { Fabricate(:direct_message_channel) }
+      fab!(:dm_channel_2) { Fabricate(:direct_message_channel) }
 
       before do
-        Fabricate(:chat_message, chat_channel: category_channel1)
+        Fabricate(:chat_message, chat_channel: category_channel_1)
-        Fabricate(:chat_message, chat_channel: category_channel1)
+        Fabricate(:chat_message, chat_channel: category_channel_1)
-        Fabricate(:chat_message, chat_channel: category_channel1)
+        Fabricate(:chat_message, chat_channel: category_channel_1)
 
-        Fabricate(:chat_message, chat_channel: category_channel2)
+        Fabricate(:chat_message, chat_channel: category_channel_2)
-        Fabricate(:chat_message, chat_channel: category_channel2)
+        Fabricate(:chat_message, chat_channel: category_channel_2)
-        Fabricate(:chat_message, chat_channel: category_channel2)
+        Fabricate(:chat_message, chat_channel: category_channel_2)
-        Fabricate(:chat_message, chat_channel: category_channel2)
+        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_channel_2)
-        Fabricate(:chat_message, chat_channel: dm_channel2)
+        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_channel_1.reload.messages_count).to eq(3)
-        expect(category_channel2.reload.messages_count).to eq(4)
+        expect(category_channel_2.reload.messages_count).to eq(4)
-        expect(category_channel3.reload.messages_count).to eq(1)
+        expect(category_channel_3.reload.messages_count).to eq(1)
-        expect(category_channel4.reload.messages_count).to eq(0)
+        expect(category_channel_4.reload.messages_count).to eq(0)
-        expect(dm_channel1.reload.messages_count).to eq(0)
+        expect(dm_channel_1.reload.messages_count).to eq(0)
-        expect(dm_channel2.reload.messages_count).to eq(2)
+        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_channel_3.chat_messages.last.trash!
-        category_channel3.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_channel_1.reload.user_count).to eq(2)
-        expect(category_channel2.reload.user_count).to eq(3)
+        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_channel_1.reload.user_count).to eq(1)
-        expect(category_channel2.reload.user_count).to eq(0)
+        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_channel_1.id, memberships_count: 2 },
-            { chat_channel_id: category_channel2.id, memberships_count: 3 },
+            { chat_channel_id: category_channel_2.id, memberships_count: 3 },
-            { chat_channel_id: dm_channel1.id, memberships_count: 2 },
+            { 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