DEV: Move channel creation for category into service (#21167)

This commit moves the category channel creation out
of the Chat::Api::Channel controller and into a
dedicated CreateCategoryChannel service. A follow up
commit will move the DM channel creation out of
the old DirectMessageChannelCreator service.

Also includes a new on_model_errors helper
for chat service class usage, that collects model
validation errors to present in a nice way.

---------

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

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

@@ -38,46 +38,33 @@ class Chat::Api::ChannelsController < Chat::ApiController
     channel_params =
       params.require(:channel).permit(:chatable_id, :name, :slug, :description, :auto_join_users)
 
-    guardian.ensure_can_create_chat_channel!
+    # NOTE: We don't allow creating channels for anything but category chatable types
-    if channel_params[:name].length > SiteSetting.max_topic_title_length
+    # at the moment. This may change in future, at which point we will need to pass in
-      raise Discourse::InvalidParameters.new(:name)
+    # a chatable_type param as well and switch to the correct service here.
+    with_service(
+      Chat::CreateCategoryChannel,
+      **channel_params.merge(category_id: channel_params[:chatable_id]),
+    ) do
+      on_success do
+        render_serialized(
+          result.channel,
+          Chat::ChannelSerializer,
+          root: "channel",
+          membership: result.membership,
+        )
+      end
+      on_model_not_found(:category) { raise ActiveRecord::RecordNotFound }
+      on_failed_policy(:can_create_channel) { raise Discourse::InvalidAccess }
+      on_failed_policy(:category_channel_does_not_exist) do
+        raise Discourse::InvalidParameters.new(I18n.t("chat.errors.channel_exists_for_category"))
+      end
+      on_model_errors(:channel) do
+        render_json_error(result.channel, type: :record_invalid, status: 422)
+      end
+      on_model_errors(:membership) do
+        render_json_error(result.membership, type: :record_invalid, status: 422)
+      end
     end
-
-    if Chat::Channel.exists?(
-         chatable_type: "Category",
-         chatable_id: channel_params[:chatable_id],
-         name: channel_params[:name],
-       )
-      raise Discourse::InvalidParameters.new(I18n.t("chat.errors.channel_exists_for_category"))
-    end
-
-    chatable = Category.find_by(id: channel_params[:chatable_id])
-    raise Discourse::NotFound unless chatable
-
-    auto_join_users =
-      ActiveRecord::Type::Boolean.new.deserialize(channel_params[:auto_join_users]) || false
-
-    channel =
-      chatable.create_chat_channel!(
-        name: channel_params[:name],
-        slug: channel_params[:slug],
-        description: channel_params[:description],
-        user_count: 1,
-        auto_join_users: auto_join_users,
-      )
-
-    channel.user_chat_channel_memberships.create!(user: current_user, following: true)
-
-    if channel.auto_join_users
-      Chat::ChannelMembershipManager.new(channel).enforce_automatic_channel_memberships
-    end
-
-    render_serialized(
-      channel,
-      Chat::ChannelSerializer,
-      membership: channel.membership_for(current_user),
-      root: "channel",
-    )
   end
 
   def show

plugins/chat/app/models/concerns/chat/chatable.rb

@@ -12,6 +12,10 @@ module Chat
       channel_class.create!(args.merge(chatable: self))
     end
 
+    def create_chat_channel(**args)
+      channel_class.create(args.merge(chatable: self))
+    end
+
     private
 
     def channel_class

plugins/chat/app/services/chat/create_category_channel.rb

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+module Chat
+  # Service responsible for creating a new category chat channel.
+  #
+  # @example
+  #  Service::Chat::CreateCategoryChannel.call(
+  #   guardian: guardian,
+  #   name: "SuperChannel",
+  #   description: "This is the best channel",
+  #   slug: "super-channel",
+  #   category_id: category.id,
+  #  )
+  #
+  class CreateCategoryChannel
+    include Service::Base
+
+    # @!method call(guardian:, **params_to_create)
+    #   @param [Guardian] guardian
+    #   @param [Hash] params_to_create
+    #   @option params_to_create [String] name
+    #   @option params_to_create [String] description
+    #   @option params_to_create [String] slug
+    #   @option params_to_create [Boolean] auto_join_users
+    #   @option params_to_create [Integer] category_id
+    #   @return [Service::Base::Context]
+
+    policy :can_create_channel
+    contract
+    model :category, :fetch_category
+    policy :category_channel_does_not_exist
+    transaction do
+      model :channel, :create_channel
+      model :membership, :create_membership
+    end
+    step :enforce_automatic_channel_memberships
+
+    # @!visibility private
+    class Contract
+      attribute :name, :string
+      attribute :description, :string
+      attribute :slug, :string
+      attribute :category_id, :integer
+      attribute :auto_join_users, :boolean, default: false
+
+      before_validation { self.auto_join_users = auto_join_users.presence || false }
+
+      validates :category_id, presence: true
+      validates :name, length: { maximum: SiteSetting.max_topic_title_length }
+    end
+
+    private
+
+    def can_create_channel(guardian:, **)
+      guardian.can_create_chat_channel?
+    end
+
+    def fetch_category(contract:, **)
+      Category.find_by(id: contract.category_id)
+    end
+
+    def category_channel_does_not_exist(category:, contract:, **)
+      !Chat::Channel.exists?(chatable: category, name: contract.name)
+    end
+
+    def create_channel(category:, contract:, **)
+      category.create_chat_channel(
+        name: contract.name,
+        slug: contract.slug,
+        description: contract.description,
+        user_count: 1,
+        auto_join_users: contract.auto_join_users,
+      )
+    end
+
+    def create_membership(channel:, guardian:, **)
+      channel.user_chat_channel_memberships.create(user: guardian.user, following: true)
+    end
+
+    def enforce_automatic_channel_memberships(channel:, **)
+      return if !channel.auto_join_users?
+      Chat::ChannelMembershipManager.new(channel).enforce_automatic_channel_memberships
+    end
+  end
+end

plugins/chat/app/services/service/base.rb

@@ -127,6 +127,10 @@ module Service
       def call(instance, context)
         context[name] = super
         raise ArgumentError, "Model not found" if context[name].blank?
+        if context[name].try(:invalid?)
+          context[result_key].fail(invalid: true)
+          context.fail!
+        end
       rescue ArgumentError => exception
         context[result_key].fail(exception: exception)
         context.fail!

plugins/chat/assets/javascripts/discourse/controllers/create-channel.js

@@ -34,7 +34,7 @@ export default class CreateChannelController extends Controller.extend(
   autoGeneratedSlug = "";
   description = "";
   categoryPermissionsHint = null;
-  autoJoinUsers = null;
+  autoJoinUsers = false;
   autoJoinWarning = "";
   loadingPermissionHint = false;
 

plugins/chat/lib/chat/steps_inspector.rb

@@ -66,6 +66,7 @@ module Chat
     # @!visibility private
     class Model < Step
       def error
+        return result[name].errors.inspect if step_result.invalid
         step_result.exception.full_message
       end
     end

plugins/chat/lib/service_runner.rb

@@ -55,9 +55,15 @@ class ServiceRunner
   AVAILABLE_ACTIONS = {
     on_success: -> { result.success? },
     on_failure: -> { result.failure? },
+    on_failed_step: ->(name) { failure_for?("result.step.#{name}") },
     on_failed_policy: ->(name = "default") { failure_for?("result.policy.#{name}") },
     on_failed_contract: ->(name = "default") { failure_for?("result.contract.#{name}") },
-    on_model_not_found: ->(name = "model") { failure_for?("result.model.#{name}") },
+    on_model_not_found: ->(name = "model") do
+      failure_for?("result.model.#{name}") && result[name].blank?
+    end,
+    on_model_errors: ->(name = "model") do
+      failure_for?("result.model.#{name}") && result["result.model.#{name}"].invalid
+    end,
   }.with_indifferent_access.freeze
 
   # @!visibility private

plugins/chat/spec/lib/chat/steps_inspector_spec.rb

@@ -184,16 +184,32 @@ RSpec.describe Chat::StepsInspector do
     end
 
     context "when the model step is failing" do
-      before do
+      context "when the model is missing" do
-        class DummyService
+        before do
-          def fetch_model
+          class DummyService
-            false
+            def fetch_model
+              false
+            end
           end
         end
+
+        it "returns an error related to the model" do
+          expect(error).to match(/Model not found/)
+        end
       end
 
-      it "returns an error related to the model" do
+      context "when the model has errors" do
-        expect(error).to match(/Model not found/)
+        before do
+          class DummyService
+            def fetch_model
+              OpenStruct.new(invalid?: true, errors: ActiveModel::Errors.new(nil))
+            end
+          end
+        end
+
+        it "returns an error related to the model" do
+          expect(error).to match(/ActiveModel::Errors \[\]/)
+        end
       end
     end
 

plugins/chat/spec/lib/service_runner_spec.rb

@@ -64,6 +64,18 @@ RSpec.describe ServiceRunner do
     end
   end
 
+  class FailureWithModelErrorsService
+    include Service::Base
+
+    model :fake_model, :fetch_fake_model
+
+    private
+
+    def fetch_fake_model
+      OpenStruct.new(invalid?: true)
+    end
+  end
+
   class SuccessWithModelService
     include Service::Base
 
@@ -76,6 +88,18 @@ RSpec.describe ServiceRunner do
     end
   end
 
+  class SuccessWithModelErrorsService
+    include Service::Base
+
+    model :fake_model, :fetch_fake_model
+
+    private
+
+    def fetch_fake_model
+      OpenStruct.new
+    end
+  end
+
   class FailureWithCollectionModelService
     include Service::Base
 
@@ -268,6 +292,30 @@ RSpec.describe ServiceRunner do
       end
     end
 
+    context "when using the on_model_errors action" do
+      let(:actions) { <<-BLOCK }
+          ->(*) do
+            on_model_errors(:fake_model) { :model_errors }
+          end
+        BLOCK
+
+      context "when the service fails with a model containing errors" do
+        let(:service) { FailureWithModelErrorsService }
+
+        it "runs the provided block" do
+          expect(runner).to eq :model_errors
+        end
+      end
+
+      context "when the service does not fail with a model containing errors" do
+        let(:service) { SuccessWithModelErrorsService }
+
+        it "does not run the provided block" do
+          expect(runner).not_to eq :model_errors
+        end
+      end
+    end
+
     context "when using several actions together" do
       let(:service) { FailureService }
       let(:actions) { <<-BLOCK }

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

@@ -155,7 +155,7 @@ RSpec.describe Chat::Api::ChannelsController do
           get "/chat/api/channels/#{channel_1.id}"
 
           expect(response.status).to eq(200)
-          expect(response.parsed_body["channel"]["id"]).to eq(channel_1.id)
+          expect(response.parsed_body.dig("channel", "id")).to eq(channel_1.id)
         end
       end
     end
@@ -248,8 +248,9 @@ RSpec.describe Chat::Api::ChannelsController do
 
     it "creates a channel associated to a category" do
       post "/chat/api/channels", params: params
+      expect(response.status).to eq(200)
 
-      new_channel = Chat::Channel.last
+      new_channel = Chat::Channel.find(response.parsed_body.dig("channel", "id"))
 
       expect(new_channel.name).to eq(params[:channel][:name])
       expect(new_channel.slug).to eq("channel-name")
@@ -262,16 +263,31 @@ RSpec.describe Chat::Api::ChannelsController do
       new_params = params.dup
       new_params[:channel][:slug] = "wow-so-cool"
       post "/chat/api/channels", params: new_params
+      expect(response.status).to eq(200)
 
-      new_channel = Chat::Channel.last
+      new_channel = Chat::Channel.find(response.parsed_body.dig("channel", "id"))
 
       expect(new_channel.slug).to eq("wow-so-cool")
     end
 
+    context "when the user-provided slug already exists for a channel" do
+      before do
+        params[:channel][:slug] = "wow-so-cool"
+        post "/chat/api/channels", params: params
+        params[:channel][:name] = "new name"
+      end
+
+      it "returns an error" do
+        post "/chat/api/channels", params: params
+        expect(response).to have_http_status :unprocessable_entity
+      end
+    end
+
     it "creates a channel sets auto_join_users to false by default" do
       post "/chat/api/channels", params: params
+      expect(response.status).to eq(200)
 
-      new_channel = Chat::Channel.last
+      new_channel = Chat::Channel.find(response.parsed_body.dig("channel", "id"))
 
       expect(new_channel.auto_join_users).to eq(false)
     end
@@ -279,8 +295,9 @@ RSpec.describe Chat::Api::ChannelsController do
     it "creates a channel with auto_join_users set to true" do
       params[:channel][:auto_join_users] = true
       post "/chat/api/channels", params: params
+      expect(response.status).to eq(200)
 
-      new_channel = Chat::Channel.last
+      new_channel = Chat::Channel.find(response.parsed_body.dig("channel", "id"))
 
       expect(new_channel.auto_join_users).to eq(true)
     end
@@ -298,6 +315,7 @@ RSpec.describe Chat::Api::ChannelsController do
       it "joins the user when auto_join_users is true" do
         params[:channel][:auto_join_users] = true
         post "/chat/api/channels", params: params
+        expect(response.status).to eq(200)
 
         created_channel_id = response.parsed_body.dig("channel", "id")
         membership_exists =
@@ -313,6 +331,7 @@ RSpec.describe Chat::Api::ChannelsController do
       it "doesn't join the user when auto_join_users is false" do
         params[:channel][:auto_join_users] = false
         post "/chat/api/channels", params: params
+        expect(response.status).to eq(200)
 
         created_channel_id = response.parsed_body.dig("channel", "id")
         membership_exists =
@@ -519,7 +538,7 @@ RSpec.describe Chat::Api::ChannelsController do
           it "joins the user when auto_join_users is true" do
             put "/chat/api/channels/#{channel.id}", params: { channel: { auto_join_users: true } }
 
-            created_channel_id = response.parsed_body["channel"]["id"]
+            created_channel_id = response.parsed_body.dig("channel", "id")
             membership_exists =
               Chat::UserChatChannelMembership.find_by(
                 user: another_user,
@@ -533,7 +552,7 @@ RSpec.describe Chat::Api::ChannelsController do
           it "doesn't join the user when auto_join_users is false" do
             put "/chat/api/channels/#{channel.id}", params: { channel: { auto_join_users: false } }
 
-            created_channel_id = response.parsed_body["channel"]["id"]
+            created_channel_id = response.parsed_body.dig("channel", "id")
 
             expect(created_channel_id).to be_present
 

plugins/chat/spec/services/chat/create_category_channel_spec.rb

@@ -0,0 +1,99 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::CreateCategoryChannel do
+  describe Chat::CreateCategoryChannel::Contract, type: :model do
+    it { is_expected.to validate_presence_of :category_id }
+    it { is_expected.to validate_length_of(:name).is_at_most(SiteSetting.max_topic_title_length) }
+  end
+
+  describe ".call" do
+    subject(:result) { described_class.call(params) }
+
+    fab!(:current_user) { Fabricate(:admin) }
+    fab!(:category) { Fabricate(:category) }
+    let(:category_id) { category.id }
+
+    let(:guardian) { Guardian.new(current_user) }
+    let(:params) { { guardian: guardian, category_id: category_id, name: "cool channel" } }
+
+    context "when the current user cannot make a channel" do
+      fab!(:current_user) { Fabricate(:user) }
+
+      it { is_expected.to fail_a_policy(:can_create_channel) }
+    end
+
+    context "when the current user can make a channel" do
+      context "when there is already a channel for the category with the same name" do
+        fab!(:old_channel) { Fabricate(:chat_channel, chatable: category, name: "old channel") }
+        let(:params) { { guardian: guardian, category_id: category_id, name: "old channel" } }
+
+        it { is_expected.to fail_a_policy(:category_channel_does_not_exist) }
+      end
+
+      context "when the category does not exist" do
+        before { category.destroy! }
+
+        it { is_expected.to fail_to_find_a_model(:category) }
+      end
+
+      context "when all steps pass" do
+        it "creates the channel" do
+          expect { result }.to change { Chat::Channel.count }.by(1)
+          expect(result.channel).to have_attributes(
+            chatable: category,
+            name: "cool channel",
+            slug: "cool-channel",
+          )
+        end
+
+        it "creates a membership for the user" do
+          expect { result }.to change { Chat::UserChatChannelMembership.count }.by(1)
+          expect(result.membership).to have_attributes(
+            user: current_user,
+            chat_channel: result.channel,
+            following: true,
+          )
+        end
+
+        it "does not enforce automatic memberships" do
+          Chat::ChannelMembershipManager
+            .any_instance
+            .expects(:enforce_automatic_channel_memberships)
+            .never
+          result
+        end
+
+        context "when the slug is already in use" do
+          fab!(:channel) { Fabricate(:chat_channel, chatable: category, slug: "in-use") }
+          let(:params) { { guardian: guardian, category_id: category_id, slug: "in-use" } }
+
+          it { is_expected.to fail_with_an_invalid_model(:channel) }
+        end
+
+        context "if auto_join_users is blank" do
+          let(:params) { { guardian: guardian, category_id: category_id, auto_join_users: "" } }
+
+          it "defaults to false" do
+            Chat::ChannelMembershipManager
+              .any_instance
+              .expects(:enforce_automatic_channel_memberships)
+              .never
+            result
+          end
+        end
+
+        context "if auto_join_users is true" do
+          let(:params) { { guardian: guardian, category_id: category_id, auto_join_users: "true" } }
+
+          it "enforces automatic memberships" do
+            Chat::ChannelMembershipManager
+              .any_instance
+              .expects(:enforce_automatic_channel_memberships)
+              .once
+            result
+          end
+        end
+      end
+    end
+  end
+end

plugins/chat/spec/support/chat_service_matcher.rb

@@ -84,6 +84,24 @@ module Chat
       def description
         "fail to find a model named '#{name}'"
       end
+
+      def step_failed?
+        super && result[name].blank?
+      end
+    end
+
+    class FailWithInvalidModel < FailStep
+      def type
+        "model"
+      end
+
+      def description
+        "fail to have a valid model named '#{name}'"
+      end
+
+      def step_failed?
+        super && result[step].invalid
+      end
     end
 
     def fail_a_policy(name)
@@ -98,6 +116,10 @@ module Chat
       FailToFindModel.new(name)
     end
 
+    def fail_with_an_invalid_model(name = "model")
+      FailWithInvalidModel.new(name)
+    end
+
     def inspect_steps(result)
       inspector = Chat::StepsInspector.new(result)
       puts "Steps:"