From 584d9a94389eaae16c5c4af8c038104bbf500c85 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 17 Feb 2023 04:17:26 +1000 Subject: [PATCH] DEV: Use service objects for chat thread lookup (#20276) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../api/chat_channel_threads_controller.rb | 21 ++---- plugins/chat/app/services/base.rb | 14 ++-- plugins/chat/app/services/lookup_thread.rb | 60 ++++++++++++++++ plugins/chat/plugin.rb | 1 + .../chat/spec/fabricators/chat_fabricator.rb | 4 +- .../chat_channel_threads_controller_spec.rb | 24 ++++++- .../chat/spec/services/lookup_thread_spec.rb | 71 +++++++++++++++++++ 7 files changed, 171 insertions(+), 24 deletions(-) create mode 100644 plugins/chat/app/services/lookup_thread.rb create mode 100644 plugins/chat/spec/services/lookup_thread_spec.rb diff --git a/plugins/chat/app/controllers/api/chat_channel_threads_controller.rb b/plugins/chat/app/controllers/api/chat_channel_threads_controller.rb index 920588dbda1..58baa9bb8a4 100644 --- a/plugins/chat/app/controllers/api/chat_channel_threads_controller.rb +++ b/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) - params.require(:thread_id) - - raise Discourse::NotFound if !SiteSetting.enable_experimental_chat_threaded_discussions - - thread = - 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") + with_service(Chat::Service::LookupThread) do + on_success { render_serialized(result.thread, ChatThreadSerializer, root: "thread") } + on_failed_policy(:threaded_discussions_enabled) { raise Discourse::NotFound } + on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound } + on_model_not_found(:thread) { raise Discourse::NotFound } + end end end diff --git a/plugins/chat/app/services/base.rb b/plugins/chat/app/services/base.rb index c42fe736f93..4123d1b3537 100644 --- a/plugins/chat/app/services/base.rb +++ b/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 diff --git a/plugins/chat/app/services/lookup_thread.rb b/plugins/chat/app/services/lookup_thread.rb new file mode 100644 index 00000000000..9e4c3ca26c7 --- /dev/null +++ b/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 diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 8a4792cb653..71cf26a3090 100644 --- a/plugins/chat/plugin.rb +++ b/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__) diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index 3602998622e..58953e9cd16 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/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 diff --git a/plugins/chat/spec/requests/api/chat_channel_threads_controller_spec.rb b/plugins/chat/spec/requests/api/chat_channel_threads_controller_spec.rb index 0a2e4f054aa..2bec6a415eb 100644 --- a/plugins/chat/spec/requests/api/chat_channel_threads_controller_spec.rb +++ b/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 } diff --git a/plugins/chat/spec/services/lookup_thread_spec.rb b/plugins/chat/spec/services/lookup_thread_spec.rb new file mode 100644 index 00000000000..6ecd42d1f06 --- /dev/null +++ b/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