diff --git a/plugins/chat/app/controllers/chat/api/channels_controller.rb b/plugins/chat/app/controllers/chat/api/channels_controller.rb index 0cd2c03f55d..37a32cfcc78 100644 --- a/plugins/chat/app/controllers/chat/api/channels_controller.rb +++ b/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! - if channel_params[:name].length > SiteSetting.max_topic_title_length - raise Discourse::InvalidParameters.new(:name) + # NOTE: We don't allow creating channels for anything but category chatable types + # at the moment. This may change in future, at which point we will need to pass in + # 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 diff --git a/plugins/chat/app/models/concerns/chat/chatable.rb b/plugins/chat/app/models/concerns/chat/chatable.rb index eedfbab7e19..02614235428 100644 --- a/plugins/chat/app/models/concerns/chat/chatable.rb +++ b/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 diff --git a/plugins/chat/app/services/chat/create_category_channel.rb b/plugins/chat/app/services/chat/create_category_channel.rb new file mode 100644 index 00000000000..0d25cd3c749 --- /dev/null +++ b/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 diff --git a/plugins/chat/app/services/service/base.rb b/plugins/chat/app/services/service/base.rb index fff8b0e414a..8efe51837d8 100644 --- a/plugins/chat/app/services/service/base.rb +++ b/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! diff --git a/plugins/chat/assets/javascripts/discourse/controllers/create-channel.js b/plugins/chat/assets/javascripts/discourse/controllers/create-channel.js index 31bc13b5514..48de049f1f5 100644 --- a/plugins/chat/assets/javascripts/discourse/controllers/create-channel.js +++ b/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; diff --git a/plugins/chat/lib/chat/steps_inspector.rb b/plugins/chat/lib/chat/steps_inspector.rb index 0f9b56263e7..f8cddfe595a 100644 --- a/plugins/chat/lib/chat/steps_inspector.rb +++ b/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 diff --git a/plugins/chat/lib/service_runner.rb b/plugins/chat/lib/service_runner.rb index 62db6ed2fdb..e362c743bcd 100644 --- a/plugins/chat/lib/service_runner.rb +++ b/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 diff --git a/plugins/chat/spec/lib/chat/steps_inspector_spec.rb b/plugins/chat/spec/lib/chat/steps_inspector_spec.rb index c452d8433d1..6ebba92001b 100644 --- a/plugins/chat/spec/lib/chat/steps_inspector_spec.rb +++ b/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 - class DummyService - def fetch_model - false + context "when the model is missing" do + before do + class DummyService + 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 - expect(error).to match(/Model not found/) + context "when the model has errors" do + 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 diff --git a/plugins/chat/spec/lib/service_runner_spec.rb b/plugins/chat/spec/lib/service_runner_spec.rb index 82ca9d87c62..bd9a8973f03 100644 --- a/plugins/chat/spec/lib/service_runner_spec.rb +++ b/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 } diff --git a/plugins/chat/spec/requests/chat/api/channels_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_controller_spec.rb index 041788c46d5..f22349422ab 100644 --- a/plugins/chat/spec/requests/chat/api/channels_controller_spec.rb +++ b/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 diff --git a/plugins/chat/spec/services/chat/create_category_channel_spec.rb b/plugins/chat/spec/services/chat/create_category_channel_spec.rb new file mode 100644 index 00000000000..4b4ae4e8b0b --- /dev/null +++ b/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 diff --git a/plugins/chat/spec/support/chat_service_matcher.rb b/plugins/chat/spec/support/chat_service_matcher.rb index 1cbfd64df84..b4df23678ac 100644 --- a/plugins/chat/spec/support/chat_service_matcher.rb +++ b/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:"