DEV: Refactor the Chat::CreateThread service a bit

Follow best practices from our docs.

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

@@ -71,24 +71,24 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController
 
   def create
     ::Chat::CreateThread.call(service_params) do
-      on_success do |thread:, membership:|
+      on_success do |thread:|
         render_serialized(
           thread,
           ::Chat::ThreadSerializer,
           root: false,
           include_thread_original_message: true,
-          membership:,
         )
       end
-      on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound }
-      on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess }
-      on_failed_step(:create_thread) do |step|
-        render json: failed_json.merge(errors: [step.error]), status: 422
-      end
-      on_failure { render(json: failed_json, status: 422) }
       on_failed_contract do |contract|
         render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
       end
+      on_model_not_found(:channel) { raise Discourse::NotFound }
+      on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess }
+      on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound }
+      on_model_errors(:thread) do |model|
+        render json: failed_json.merge(errors: [model.errors.full_messages.join(", ")]), status: 422
+      end
+      on_failure { render(json: failed_json, status: 422) }
     end
   end
 end

plugins/chat/app/services/chat/create_thread.rb

@@ -30,12 +30,11 @@ module Chat
     policy :threading_enabled_for_channel
     model :original_message
     transaction do
-      step :find_or_create_thread
+      model :thread, :find_or_create_thread
       step :associate_thread_to_message
-      step :fetch_membership
-      step :publish_new_thread
-      step :trigger_chat_thread_created_event
     end
+    step :publish_new_thread
+    step :trigger_chat_thread_created_event
 
     private
 
@@ -43,39 +42,32 @@ module Chat
       ::Chat::Channel.find_by(id: params.channel_id)
     end
 
-    def fetch_original_message(channel:, params:)
-      ::Chat::Message.find_by(id: params.original_message_id, chat_channel_id: params.channel_id)
-    end
-
     def can_view_channel(guardian:, channel:)
       guardian.can_preview_chat_channel?(channel)
     end
 
     def threading_enabled_for_channel(channel:)
-      channel.threading_enabled
+      channel.threading_enabled?
+    end
+
+    def fetch_original_message(channel:, params:)
+      ::Chat::Message.find_by(id: params.original_message_id, chat_channel_id: params.channel_id)
     end
 
     def find_or_create_thread(channel:, original_message:, params:)
-      if original_message.thread_id.present?
-        return context[:thread] = ::Chat::Thread.find_by(id: original_message.thread_id)
-      end
+      return original_message.thread if original_message.thread
 
-      context[:thread] = channel.threads.create(
+      channel.threads.create(
         title: params.title,
         original_message: original_message,
         original_message_user: original_message.user,
       )
-      fail!(context.thread.errors.full_messages.join(", ")) if context.thread.invalid?
     end
 
     def associate_thread_to_message(original_message:, thread:)
       original_message.update(thread:)
     end
 
-    def fetch_membership(guardian:, thread:)
-      context[:membership] = thread.membership_for(guardian.user)
-    end
-
     def publish_new_thread(channel:, original_message:, thread:)
       ::Chat::Publisher.publish_thread_created!(channel, original_message, thread.id)
     end

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

@@ -260,7 +260,7 @@ RSpec.describe Chat::Api::ChannelThreadsController do
     context "when channel does not exist" do
       it "returns 404" do
         channel_1.destroy!
-        post "/chat/api/channels/#{channel_id}", params: params
+        post "/chat/api/channels/#{channel_id}/threads", params: params
 
         expect(response.status).to eq(404)
       end

plugins/chat/spec/services/chat/create_thread_spec.rb

@@ -34,12 +34,7 @@ RSpec.describe Chat::CreateThread do
         expect {
           result
           message_1.reload
-        }.to change { message_1.thread_id }.from(nil).to(result.thread.id)
-      end
-
-      it "fetches the membership" do
-        result
-        expect(result.membership).to eq(result.thread.membership_for(current_user))
+        }.to change { message_1.thread }.from(nil).to(result.thread)
       end
 
       it "publishes a `thread_created` MessageBus event for public channels" do
@@ -67,6 +62,22 @@ RSpec.describe Chat::CreateThread do
         result
         expect(result.thread.title).to eq(params[:title])
       end
+
+      context "when a thread is already present" do
+        before do
+          Chat::CreateThread.call(
+            guardian: current_user.guardian,
+            params: {
+              original_message_id: message_1.id,
+              channel_id: channel_1.id,
+            },
+          )
+        end
+
+        it "uses the existing thread" do
+          expect { result }.not_to change { Chat::Thread.count }
+        end
+      end
     end
 
     context "when params are not valid" do
@@ -75,18 +86,10 @@ RSpec.describe Chat::CreateThread do
       it { is_expected.to fail_a_contract }
     end
 
-    context "when original message is not found" do
-      fab!(:channel_2) { Fabricate(:chat_channel, threading_enabled: true) }
+    context "when channel does not exist" do
+      before { params[:channel_id] = 0 }
 
-      before { params[:channel_id] = channel_2.id }
-
-      it { is_expected.to fail_to_find_a_model(:original_message) }
-    end
-
-    context "when original message is not found" do
-      before { message_1.destroy! }
-
-      it { is_expected.to fail_to_find_a_model(:original_message) }
+      it { is_expected.to fail_to_find_a_model(:channel) }
     end
 
     context "when user cannot see channel" do
@@ -103,20 +106,18 @@ RSpec.describe Chat::CreateThread do
       it { is_expected.to fail_a_policy(:threading_enabled_for_channel) }
     end
 
-    context "when a thread is already present" do
-      before do
-        Chat::CreateThread.call(
-          guardian: current_user.guardian,
-          params: {
-            original_message_id: message_1.id,
-            channel_id: channel_1.id,
-          },
-        )
-      end
+    context "when original message is not found" do
+      fab!(:channel_2) { Fabricate(:chat_channel, threading_enabled: true) }
 
-      it "uses the existing thread" do
-        expect { result }.not_to change { Chat::Thread.count }
-      end
+      before { params[:channel_id] = channel_2.id }
+
+      it { is_expected.to fail_to_find_a_model(:original_message) }
+    end
+
+    context "when original message is not found" do
+      before { message_1.destroy! }
+
+      it { is_expected.to fail_to_find_a_model(:original_message) }
     end
   end
 end