DEV: Use service objects for chat thread lookup (#20276)

Adds a new LookupThread class that handles finding the
thread based on thread + channel ID, checking permissions
and policy/contract checks.

Co-authored-by: Loïc Guitaut <loic@discourse.org>

plugins/chat/app/controllers/api/chat_channel_threads_controller.rb

@@ -2,20 +2,11 @@
 
 class Chat::Api::ChatChannelThreadsController < Chat::Api
   def show
-    params.require(:channel_id)
+    with_service(Chat::Service::LookupThread) do
-    params.require(:thread_id)
+      on_success { render_serialized(result.thread, ChatThreadSerializer, root: "thread") }
-
+      on_failed_policy(:threaded_discussions_enabled) { raise Discourse::NotFound }
-    raise Discourse::NotFound if !SiteSetting.enable_experimental_chat_threaded_discussions
+      on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound }
-
+      on_model_not_found(:thread) { raise Discourse::NotFound }
-    thread =
+    end
-      ChatThread
-        .includes(:channel)
-        .includes(original_message_user: :user_status)
-        .includes(original_message: :chat_webhook_event)
-        .find_by!(id: params[:thread_id], channel_id: params[:channel_id])
-
-    guardian.ensure_can_preview_chat_channel!(thread.channel)
-
-    render_serialized(thread, ChatThreadSerializer, root: "thread")
   end
 end

plugins/chat/app/services/base.rb

@@ -11,6 +11,13 @@ module Chat
     #
     # Currently, there are 5 types of steps:
     #
+    # * +contract(name = :default)+: used to validate the input parameters,
+    #   typically provided by a user calling an endpoint. A special embedded
+    #   +Contract+ class has to be defined to holds the validations. If the
+    #   validations fail, the step will fail. Otherwise, the resulting contract
+    #   will be available in +context[:contract]+. When calling +step(name)+ or
+    #   +model(name = :model)+ methods after validating a contract, the contract
+    #   should be used as an argument instead of context attributes.
     # * +model(name = :model)+: used to instantiate a model (either by building
     #   it or fetching it from the DB). If a falsy value is returned, then the
     #   step will fail. Otherwise the resulting object will be assigned in
@@ -18,11 +25,6 @@ module Chat
     # * +policy(name = :default)+: used to perform a check on the state of the
     #   system. Typically used to run guardians. If a falsy value is returned,
     #   the step will fail.
-    # * +contract(name = :default)+: used to validate the input parameters,
-    #   typically provided by a user calling an endpoint. A special embedded
-    #   +Contract+ class has to be defined to holds the validations. If the
-    #   validations fail, the step will fail. Otherwise, the resulting contract
-    #   will be available in +context[:contract]+.
     # * +step(name)+: used to run small snippets of arbitrary code. The step
     #   doesn’t care about its return value, so to mark the service as failed,
     #   {#fail!} has to be called explicitly.
@@ -34,7 +36,7 @@ module Chat
     # whatever reason a key isn’t found in the current context, then Ruby will
     # raise an exception when the method is called.
     #
-    # Regarding contract classes, they have automatically {ActiveModel} modules
+    # Regarding contract classes, they automatically have {ActiveModel} modules
     # included so all the {ActiveModel} API is available.
     #
     # @example An example from the {TrashChannel} service

plugins/chat/app/services/lookup_thread.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+module Chat
+  module Service
+    # Finds a thread within a channel. The thread_id and channel_id must
+    # match. For now we do not want to allow fetching threads if the
+    # enable_experimental_chat_threaded_discussions hidden site setting
+    # is not turned on, and the channel must specifically have threading
+    # enabled.
+    #
+    # @example
+    #  Chat::Service::LookupThread.call(thread_id: 88, channel_id: 2, guardian: guardian)
+    #
+    class LookupThread
+      include Base
+
+      # @!method call(thread_id:, channel_id:, guardian:)
+      #   @param [Integer] thread_id
+      #   @param [Integer] channel_id
+      #   @param [Guardian] guardian
+      #   @return [Chat::Service::Base::Context]
+
+      policy :threaded_discussions_enabled
+      contract
+      model :thread, :fetch_thread
+      policy :invalid_access
+      policy :threading_enabled_for_channel
+
+      # @!visibility private
+      class Contract
+        attribute :thread_id, :integer
+        attribute :channel_id, :integer
+
+        validates :thread_id, :channel_id, presence: true
+      end
+
+      private
+
+      def threaded_discussions_enabled
+        SiteSetting.enable_experimental_chat_threaded_discussions
+      end
+
+      def fetch_thread(contract:, **)
+        ChatThread.includes(
+          :channel,
+          original_message_user: :user_status,
+          original_message: :chat_webhook_event,
+        ).find_by(id: contract.thread_id, channel_id: contract.channel_id)
+      end
+
+      def invalid_access(guardian:, thread:, **)
+        guardian.can_preview_chat_channel?(thread.channel)
+      end
+
+      def threading_enabled_for_channel(thread:, **)
+        thread.channel.threading_enabled
+      end
+    end
+  end
+end

plugins/chat/plugin.rb

@@ -222,6 +222,7 @@ after_initialize do
   load File.expand_path("../app/services/update_channel_status.rb", __FILE__)
   load File.expand_path("../app/services/chat_message_destroyer.rb", __FILE__)
   load File.expand_path("../app/services/update_user_last_read.rb", __FILE__)
+  load File.expand_path("../app/services/lookup_thread.rb", __FILE__)
   load File.expand_path("../app/controllers/api_controller.rb", __FILE__)
   load File.expand_path("../app/controllers/api/chat_channels_controller.rb", __FILE__)
   load File.expand_path("../app/controllers/api/chat_current_user_channels_controller.rb", __FILE__)

plugins/chat/spec/fabricators/chat_fabricator.rb

@@ -189,5 +189,7 @@ Fabricator(:chat_thread) do
 
   transient :channel
 
-  original_message { |attrs| Fabricate(:chat_message, chat_channel: attrs[:channel]) }
+  original_message do |attrs|
+    Fabricate(:chat_message, chat_channel: attrs[:channel] || Fabricate(:chat_channel))
+  end
 end

plugins/chat/spec/requests/api/chat_channel_threads_controller_spec.rb

@@ -4,6 +4,7 @@ require "rails_helper"
 
 RSpec.describe Chat::Api::ChatChannelThreadsController do
   fab!(:current_user) { Fabricate(:user) }
+  fab!(:public_channel) { Fabricate(:chat_channel, threading_enabled: true) }
 
   before do
     SiteSetting.chat_enabled = true
@@ -15,7 +16,12 @@ RSpec.describe Chat::Api::ChatChannelThreadsController do
 
   describe "show" do
     context "when thread does not exist" do
-      fab!(:thread) { Fabricate(:chat_thread, original_message: Fabricate(:chat_message)) }
+      fab!(:thread) do
+        Fabricate(
+          :chat_thread,
+          original_message: Fabricate(:chat_message, chat_channel: public_channel),
+        )
+      end
 
       it "returns 404" do
         thread.destroy!
@@ -25,7 +31,12 @@ RSpec.describe Chat::Api::ChatChannelThreadsController do
     end
 
     context "when thread exists" do
-      fab!(:thread) { Fabricate(:chat_thread, original_message: Fabricate(:chat_message)) }
+      fab!(:thread) do
+        Fabricate(
+          :chat_thread,
+          original_message: Fabricate(:chat_message, chat_channel: public_channel),
+        )
+      end
 
       it "works" do
         get "/chat/api/channels/#{thread.channel_id}/threads/#{thread.id}"
@@ -42,6 +53,15 @@ RSpec.describe Chat::Api::ChatChannelThreadsController do
         end
       end
 
+      context "when channel does not have threading enabled" do
+        before { thread.channel.update!(threading_enabled: false) }
+
+        it "returns 404" do
+          get "/chat/api/channels/#{thread.channel_id}/threads/#{thread.id}"
+          expect(response.status).to eq(404)
+        end
+      end
+
       context "when enable_experimental_chat_threaded_discussions is disabled" do
         before { SiteSetting.enable_experimental_chat_threaded_discussions = false }
 

plugins/chat/spec/services/lookup_thread_spec.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::Service::LookupThread do
+  describe Chat::Service::LookupThread::Contract, type: :model do
+    it { is_expected.to validate_presence_of :channel_id }
+    it { is_expected.to validate_presence_of :thread_id }
+  end
+
+  describe ".call" do
+    subject(:result) { described_class.call(params) }
+
+    fab!(:current_user) { Fabricate(:user) }
+    fab!(:channel) { Fabricate(:chat_channel, threading_enabled: true) }
+    fab!(:private_channel) { Fabricate(:private_category_channel, group: Fabricate(:group)) }
+    fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+    fab!(:other_thread) { Fabricate(:chat_thread) }
+
+    let(:guardian) { Guardian.new(current_user) }
+    let(:params) { { guardian: guardian, thread_id: thread.id, channel_id: thread.channel_id } }
+
+    context "when enable_experimental_chat_threaded_discussions is disabled" do
+      before { SiteSetting.enable_experimental_chat_threaded_discussions = false }
+
+      it { is_expected.to fail_a_policy(:threaded_discussions_enabled) }
+    end
+
+    context "when enable_experimental_chat_threaded_discussions is enabled" do
+      before { SiteSetting.enable_experimental_chat_threaded_discussions = true }
+
+      context "when all steps pass" do
+        it "sets the service result as successful" do
+          expect(result).to be_a_success
+        end
+
+        it "fetches the thread" do
+          expect(result.thread).to eq(thread)
+        end
+      end
+
+      context "when params are not valid" do
+        before { params.delete(:thread_id) }
+
+        it { is_expected.to fail_a_contract }
+      end
+
+      context "when thread is not found because the channel ID differs" do
+        before { params[:thread_id] = other_thread.id }
+
+        it { is_expected.to fail_to_find_a_model(:thread) }
+      end
+
+      context "when thread is not found" do
+        before { thread.destroy! }
+
+        it { is_expected.to fail_to_find_a_model(:thread) }
+      end
+
+      context "when user cannot see channel" do
+        before { thread.update!(channel: private_channel) }
+
+        it { is_expected.to fail_a_policy(:invalid_access) }
+      end
+
+      context "when threading is not enabled for the channel" do
+        before { channel.update!(threading_enabled: false) }
+
+        it { is_expected.to fail_a_policy(:threading_enabled_for_channel) }
+      end
+    end
+  end
+end