diff --git a/plugins/chat/app/services/chat/add_users_to_channel.rb b/plugins/chat/app/services/chat/add_users_to_channel.rb index 6c0325b3bce..585f44dd372 100644 --- a/plugins/chat/app/services/chat/add_users_to_channel.rb +++ b/plugins/chat/app/services/chat/add_users_to_channel.rb @@ -47,6 +47,10 @@ module Chat private + def fetch_channel(contract:) + ::Chat::Channel.includes(:chatable).find_by(id: contract.channel_id) + end + def can_add_users_to_channel(guardian:, channel:) (guardian.user.admin? || channel.joined_by?(guardian.user)) && channel.direct_message_channel? && channel.chatable.group @@ -60,10 +64,6 @@ module Chat ) end - def fetch_channel(contract:) - ::Chat::Channel.includes(:chatable).find_by(id: contract.channel_id) - end - def upsert_memberships(channel:, target_users:) always_level = ::Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always] diff --git a/plugins/chat/app/services/chat/auto_remove/handle_category_updated.rb b/plugins/chat/app/services/chat/auto_remove/handle_category_updated.rb index 6da61967c1d..909c6855704 100644 --- a/plugins/chat/app/services/chat/auto_remove/handle_category_updated.rb +++ b/plugins/chat/app/services/chat/auto_remove/handle_category_updated.rb @@ -14,13 +14,13 @@ module Chat class HandleCategoryUpdated include Service::Base + policy :chat_enabled contract do attribute :category_id, :integer validates :category_id, presence: true end step :assign_defaults - policy :chat_enabled model :category model :category_channel_ids model :users diff --git a/plugins/chat/app/services/chat/auto_remove/handle_destroyed_group.rb b/plugins/chat/app/services/chat/auto_remove/handle_destroyed_group.rb index 36bada03d11..18e7d6a6dd0 100644 --- a/plugins/chat/app/services/chat/auto_remove/handle_destroyed_group.rb +++ b/plugins/chat/app/services/chat/auto_remove/handle_destroyed_group.rb @@ -21,13 +21,13 @@ module Chat class HandleDestroyedGroup include Service::Base + policy :chat_enabled contract do - attribute :destroyed_group_user_ids + attribute :destroyed_group_user_ids, :array validates :destroyed_group_user_ids, presence: true end step :assign_defaults - policy :chat_enabled policy :not_everyone_allowed model :scoped_users step :remove_users_outside_allowed_groups @@ -48,7 +48,7 @@ module Chat !SiteSetting.chat_allowed_groups_map.include?(Group::AUTO_GROUPS[:everyone]) end - def fetch_scoped_users(destroyed_group_user_ids:) + def fetch_scoped_users(contract:) User .real .activated @@ -56,7 +56,7 @@ module Chat .not_staged .includes(:group_users) .where("NOT admin AND NOT moderator") - .where(id: destroyed_group_user_ids) + .where(id: contract.destroyed_group_user_ids) .joins(:user_chat_channel_memberships) .distinct end diff --git a/plugins/chat/app/services/chat/auto_remove/handle_user_removed_from_group.rb b/plugins/chat/app/services/chat/auto_remove/handle_user_removed_from_group.rb index 9154ff133c8..5d497d5d850 100644 --- a/plugins/chat/app/services/chat/auto_remove/handle_user_removed_from_group.rb +++ b/plugins/chat/app/services/chat/auto_remove/handle_user_removed_from_group.rb @@ -20,13 +20,13 @@ module Chat class HandleUserRemovedFromGroup include Service::Base + policy :chat_enabled contract do attribute :user_id, :integer validates :user_id, presence: true end step :assign_defaults - policy :chat_enabled policy :not_everyone_allowed model :user policy :user_not_staff diff --git a/plugins/chat/app/services/chat/flag_message.rb b/plugins/chat/app/services/chat/flag_message.rb index ad610dddebe..dabb638f1ee 100644 --- a/plugins/chat/app/services/chat/flag_message.rb +++ b/plugins/chat/app/services/chat/flag_message.rb @@ -37,7 +37,7 @@ module Chat validates :message_id, presence: true validates :channel_id, presence: true - validates :flag_type_id, inclusion: { in: ->(_flag_type) { ::ReviewableScore.types.values } } + validates :flag_type_id, inclusion: { in: -> { ::ReviewableScore.types.values } } end model :message policy :can_flag_message_in_channel diff --git a/plugins/chat/app/services/chat/list_channel_messages.rb b/plugins/chat/app/services/chat/list_channel_messages.rb index 6632b4e1c3b..6f77b71e306 100644 --- a/plugins/chat/app/services/chat/list_channel_messages.rb +++ b/plugins/chat/app/services/chat/list_channel_messages.rb @@ -39,7 +39,6 @@ module Chat }, allow_nil: true end - model :channel policy :can_view_channel model :membership, optional: true diff --git a/plugins/chat/app/services/chat/search_chatable.rb b/plugins/chat/app/services/chat/search_chatable.rb index 829f9dc9841..dbe2e41e1c0 100644 --- a/plugins/chat/app/services/chat/search_chatable.rb +++ b/plugins/chat/app/services/chat/search_chatable.rb @@ -34,7 +34,7 @@ module Chat private def clean_term(contract:) - context[:term] = contract.term&.downcase&.strip&.gsub(/^[@#]+/, "") + contract.term = contract.term&.downcase&.strip&.gsub(/^[@#]+/, "") end def fetch_memberships(guardian:) @@ -44,31 +44,31 @@ module Chat def fetch_users(guardian:, contract:) return unless contract.include_users return unless guardian.can_create_direct_message? - search_users(context, guardian) + search_users(contract, guardian) end def fetch_groups(guardian:, contract:) return unless contract.include_groups return unless guardian.can_create_direct_message? - search_groups(context, guardian) + search_groups(contract, guardian) end def fetch_category_channels(guardian:, contract:) return unless contract.include_category_channels return unless SiteSetting.enable_public_channels - search_category_channels(context, guardian) + search_category_channels(contract, guardian) end def fetch_direct_message_channels(guardian:, contract:, users:) return unless contract.include_direct_message_channels return unless guardian.can_create_direct_message? - search_direct_message_channels(context, guardian, contract, users) + search_direct_message_channels(guardian, contract, users) end - def search_users(context, guardian) - user_search = ::UserSearch.new(context.term, limit: SEARCH_RESULT_LIMIT) + def search_users(contract, guardian) + user_search = ::UserSearch.new(contract.term, limit: SEARCH_RESULT_LIMIT) - if context.term.blank? + if contract.term.blank? user_search = user_search.scoped_users else user_search = user_search.search @@ -80,45 +80,45 @@ module Chat user_search = user_search.real(allowed_bot_user_ids: allowed_bot_user_ids) user_search = user_search.includes(:user_option) - if context.excluded_memberships_channel_id + if contract.excluded_memberships_channel_id user_search = user_search.where( "NOT EXISTS (SELECT 1 FROM user_chat_channel_memberships WHERE user_id = users.id AND chat_channel_id = ?)", - context.excluded_memberships_channel_id, + contract.excluded_memberships_channel_id, ) end user_search end - def search_groups(context, guardian) + def search_groups(contract, guardian) Group .visible_groups(guardian.user) .includes(users: :user_option) .where( "groups.name ILIKE :term_like OR groups.full_name ILIKE :term_like", - term_like: "%#{context.term}%", + term_like: "%#{contract.term}%", ) .limit(SEARCH_RESULT_LIMIT) end - def search_category_channels(context, guardian) + def search_category_channels(contract, guardian) ::Chat::ChannelFetcher.secured_public_channel_search( guardian, status: :open, - filter: context.term, + filter: contract.term, filter_on_category_name: false, match_filter_on_starts_with: false, limit: SEARCH_RESULT_LIMIT, ) end - def search_direct_message_channels(context, guardian, contract, users) + def search_direct_message_channels(guardian, contract, users) channels = ::Chat::ChannelFetcher.secured_direct_message_channels_search( guardian.user.id, guardian, - filter: context.term, + filter: contract.term, match_filter_on_starts_with: false, limit: SEARCH_RESULT_LIMIT, ) || [] diff --git a/plugins/chat/app/services/chat/update_channel.rb b/plugins/chat/app/services/chat/update_channel.rb index 9bb1248d1e2..1898a5eba6d 100644 --- a/plugins/chat/app/services/chat/update_channel.rb +++ b/plugins/chat/app/services/chat/update_channel.rb @@ -65,13 +65,11 @@ module Chat end def update_channel(channel:, contract:) - channel.assign_attributes(contract.attributes) - context[:threading_enabled_changed] = channel.threading_enabled_changed? - channel.save! + channel.update!(contract.attributes) end def mark_all_threads_as_read_if_needed(channel:) - return if !(context.threading_enabled_changed && channel.threading_enabled) + return unless channel.threading_enabled_previously_changed?(to: true) Jobs.enqueue(Jobs::Chat::MarkAllChannelThreadsRead, channel_id: channel.id) end diff --git a/plugins/chat/app/services/chat/update_channel_status.rb b/plugins/chat/app/services/chat/update_channel_status.rb index 1816599846c..cf3b2892ce5 100644 --- a/plugins/chat/app/services/chat/update_channel_status.rb +++ b/plugins/chat/app/services/chat/update_channel_status.rb @@ -17,7 +17,7 @@ module Chat model :channel, :fetch_channel contract do - attribute :status + attribute :status, :string validates :status, inclusion: { in: Chat::Channel.editable_statuses.keys } end @@ -30,13 +30,13 @@ module Chat Chat::Channel.find_by(id: channel_id) end - def check_channel_permission(guardian:, channel:, status:) + def check_channel_permission(guardian:, channel:, contract:) guardian.can_preview_chat_channel?(channel) && - guardian.can_change_channel_status?(channel, status.to_sym) + guardian.can_change_channel_status?(channel, contract.status.to_sym) end - def change_status(channel:, status:, guardian:) - channel.public_send("#{status}!", guardian.user) + def change_status(channel:, contract:, guardian:) + channel.public_send("#{contract.status}!", guardian.user) end end end diff --git a/plugins/chat/spec/requests/chat/api/channel_messages_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channel_messages_controller_spec.rb index 63c12100989..25e6b3846d1 100644 --- a/plugins/chat/spec/requests/chat/api/channel_messages_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channel_messages_controller_spec.rb @@ -76,19 +76,19 @@ RSpec.describe Chat::Api::ChannelMessagesController do describe "#create" do context "when force_thread param is given" do - it "removes it from params" do - sign_in(current_user) + let!(:message) { Fabricate(:chat_message, chat_channel: channel) } - message_1 = Fabricate(:chat_message, chat_channel: channel) + before { sign_in(current_user) } + it "ignores it" do expect { post "/chat/#{channel.id}.json", params: { - in_reply_to_id: message_1.id, + in_reply_to_id: message.id, message: "test", force_thread: true, } - }.to change { Chat::Thread.where(force: false).count }.by(1) + }.not_to change { Chat::Thread.where(force: true).count } end end end diff --git a/plugins/chat/spec/services/auto_remove/handle_category_updated_spec.rb b/plugins/chat/spec/services/auto_remove/handle_category_updated_spec.rb index 87ef5c75d92..76700891df5 100644 --- a/plugins/chat/spec/services/auto_remove/handle_category_updated_spec.rb +++ b/plugins/chat/spec/services/auto_remove/handle_category_updated_spec.rb @@ -51,9 +51,7 @@ RSpec.describe Chat::AutoRemove::HandleCategoryUpdated do updated_category.category_groups.delete_all end - it "sets the service result as successful" do - expect(result).to be_a_success - end + it { is_expected.to run_successfully } it "does not kick any users since the default permission is Everyone (full)" do expect { result }.not_to change { @@ -90,9 +88,7 @@ RSpec.describe Chat::AutoRemove::HandleCategoryUpdated do group_2.add(user_1) end - it "sets the service result as successful" do - expect(result).to be_a_success - end + it { is_expected.to run_successfully } it "kicks all regular users who are not in any groups with reply + see permissions" do expect { result }.to change { diff --git a/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb b/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb index 3a1d436d9e5..7b84ac10654 100644 --- a/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb +++ b/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do + describe described_class::Contract, type: :model do + it { is_expected.to validate_presence_of(:destroyed_group_user_ids) } + end + describe ".call" do subject(:result) { described_class.call(params) } @@ -45,9 +49,7 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do channel_1.add(admin_2) end - it "sets the service result as successful" do - expect(result).to be_a_success - end + it { is_expected.to run_successfully } it "removes the destroyed_group_user_ids from all public channels" do expect { result }.to change { @@ -132,9 +134,7 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do channel_1.add(admin_2) end - it "sets the service result as successful" do - expect(result).to be_a_success - end + it { is_expected.to run_successfully } it "does not remove any memberships" do expect { result }.not_to change { Chat::UserChatChannelMembership.count } @@ -196,9 +196,7 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do ) end - it "sets the service result as successful" do - expect(result).to be_a_success - end + it { is_expected.to run_successfully } it "removes the destroyed_group_user_ids from the channel" do expect { result }.to change { @@ -237,9 +235,7 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do context "when one of the users is not in any of the groups" do before { user_2.change_trust_level!(TrustLevel[3]) } - it "sets the service result as successful" do - expect(result).to be_a_success - end + it { is_expected.to run_successfully } it "removes the destroyed_group_user_ids from the channel" do expect { result }.to change { diff --git a/plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb b/plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb index fa234b6bf4d..7c57b9ea3fa 100644 --- a/plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb +++ b/plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe Chat::CreateDirectMessageChannel do - describe Chat::CreateDirectMessageChannel::Contract, type: :model do + describe described_class::Contract, type: :model do subject(:contract) { described_class.new(params) } let(:params) { { target_usernames: %w[lechuck elaine] } } @@ -45,10 +45,6 @@ RSpec.describe Chat::CreateDirectMessageChannel do context "when all steps pass" do it { is_expected.to run_successfully } - it "sets the service result as successful" do - expect(result).to be_a_success - end - it "updates user count" do expect(result.channel.user_count).to eq(3) # current user + user_1 + user_2 end diff --git a/plugins/chat/spec/services/chat/create_thread_spec.rb b/plugins/chat/spec/services/chat/create_thread_spec.rb index e2479837901..e76e1f69411 100644 --- a/plugins/chat/spec/services/chat/create_thread_spec.rb +++ b/plugins/chat/spec/services/chat/create_thread_spec.rb @@ -1,9 +1,10 @@ # frozen_string_literal: true RSpec.describe Chat::CreateThread do - describe Chat::CreateThread::Contract, type: :model do + describe described_class::Contract, type: :model do it { is_expected.to validate_presence_of :channel_id } it { is_expected.to validate_presence_of :original_message_id } + it { is_expected.to validate_length_of(:title).is_at_most(Chat::Thread::MAX_TITLE_LENGTH) } end describe ".call" do @@ -68,12 +69,6 @@ RSpec.describe Chat::CreateThread do it { is_expected.to fail_a_contract } end - context "when title is too long" do - let(:title) { "a" * Chat::Thread::MAX_TITLE_LENGTH + "a" } - - 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) } diff --git a/plugins/chat/spec/services/chat/flag_message_spec.rb b/plugins/chat/spec/services/chat/flag_message_spec.rb index 83b58bebaf1..f382984a2c6 100644 --- a/plugins/chat/spec/services/chat/flag_message_spec.rb +++ b/plugins/chat/spec/services/chat/flag_message_spec.rb @@ -2,8 +2,6 @@ RSpec.describe Chat::FlagMessage do describe described_class::Contract, type: :model do - subject(:contract) { described_class.new } - it { is_expected.to validate_presence_of(:channel_id) } it { is_expected.to validate_presence_of(:message_id) } @@ -26,9 +24,6 @@ RSpec.describe Chat::FlagMessage do let(:message) { nil } let(:is_warning) { nil } let(:take_action) { nil } - - before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:everyone] } - let(:params) do { guardian: guardian, @@ -41,23 +36,34 @@ RSpec.describe Chat::FlagMessage do } end + before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:everyone] } + context "when all steps pass" do fab!(:current_user) { Fabricate(:admin) } + let(:reviewable) { Reviewable.last } + it { is_expected.to run_successfully } it "flags the message" do expect { result }.to change { Reviewable.count }.by(1) - - reviewable = Reviewable.last - expect(reviewable.target_type).to eq("ChatMessage") - expect(reviewable.created_by_id).to eq(current_user.id) - expect(reviewable.target_created_by_id).to eq(message_1.user.id) - expect(reviewable.target_id).to eq(message_1.id) - expect(reviewable.payload).to eq("message_cooked" => message_1.cooked) + expect(reviewable).to have_attributes( + target: message_1, + created_by: current_user, + target_created_by: message_1.user, + payload: { + "message_cooked" => message_1.cooked, + }, + ) end end + context "when contract is invalid" do + let(:channel_id) { nil } + + it { is_expected.to fail_a_contract } + end + context "when channel is not found" do before { params[:channel_id] = -999 } diff --git a/plugins/chat/spec/services/chat/invite_users_to_channel_spec.rb b/plugins/chat/spec/services/chat/invite_users_to_channel_spec.rb index 6e9a8c7e03b..02682d14836 100644 --- a/plugins/chat/spec/services/chat/invite_users_to_channel_spec.rb +++ b/plugins/chat/spec/services/chat/invite_users_to_channel_spec.rb @@ -1,88 +1,93 @@ # frozen_string_literal: true RSpec.describe Chat::InviteUsersToChannel do - subject(:result) { described_class.call(params) } - describe described_class::Contract, type: :model do - subject(:contract) { described_class.new } - it { is_expected.to validate_presence_of :channel_id } it { is_expected.to validate_presence_of :user_ids } end - fab!(:current_user) { Fabricate(:admin) } - fab!(:user_1) { Fabricate(:user) } - fab!(:user_2) { Fabricate(:user) } - fab!(:channel_1) { Fabricate(:chat_channel) } - fab!(:group_1) { Fabricate(:group) } + describe ".call" do + subject(:result) { described_class.call(**params, **dependencies) } - let(:guardian) { current_user.guardian } - let(:user_ids) { [user_1.id, user_2.id] } - let(:message_id) { nil } - let(:params) do - { guardian: guardian, channel_id: channel_1.id, message_id: message_id, user_ids: user_ids } - end + fab!(:current_user) { Fabricate(:admin) } + fab!(:user_1) { Fabricate(:user) } + fab!(:user_2) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:chat_channel) } + fab!(:group_1) { Fabricate(:group) } - before do - group_1.add(user_1) - SiteSetting.chat_allowed_groups = group_1.id - end + let(:guardian) { current_user.guardian } + let(:user_ids) { [user_1.id, user_2.id] } + let(:message_id) { nil } + let(:params) { { channel_id: channel_1.id, message_id:, user_ids: } } + let(:dependencies) { { guardian: } } - context "when all steps pass" do - it { is_expected.to run_successfully } - - it "creates the notifications for allowed users" do - result - - notification = user_1.reload.notifications.last - expect(notification.notification_type).to eq(::Notification.types[:chat_invitation]) - - notification = user_2.reload.notifications.last - expect(notification).to be_nil + before do + group_1.add(user_1) + SiteSetting.chat_allowed_groups = group_1.id end - it "doesnt create notifications for suspended users" do - user_1.update!(suspended_till: 2.days.from_now, suspended_at: Time.now) + context "when all steps pass" do + it { is_expected.to run_successfully } - result - - notification = user_1.reload.notifications.last - expect(notification).to be_nil - end - - it "doesnt create notifications for users with disabled chat" do - user_1.user_option.update!(chat_enabled: false) - - result - - notification = user_1.reload.notifications.last - expect(notification).to be_nil - end - - context "when message id is provided" do - fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } - - let(:message_id) { message_1.id } - - it "sets the message id on the notification" do + it "creates the notifications for allowed users" do result - data = JSON.parse(user_1.reload.notifications.last.data) - expect(data["chat_message_id"]).to eq(message_id) + notification = user_1.reload.notifications.last + expect(notification.notification_type).to eq(::Notification.types[:chat_invitation]) + + notification = user_2.reload.notifications.last + expect(notification).to be_nil + end + + it "doesnt create notifications for suspended users" do + user_1.update!(suspended_till: 2.days.from_now, suspended_at: Time.now) + + result + + notification = user_1.reload.notifications.last + expect(notification).to be_nil + end + + it "doesnt create notifications for users with disabled chat" do + user_1.user_option.update!(chat_enabled: false) + + result + + notification = user_1.reload.notifications.last + expect(notification).to be_nil + end + + context "when message id is provided" do + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + + let(:message_id) { message_1.id } + + it "sets the message id on the notification" do + result + + data = JSON.parse(user_1.reload.notifications.last.data) + expect(data["chat_message_id"]).to eq(message_id) + end end end - end - context "when channel model is not found" do - before { params[:channel_id] = -1 } + context "when contract is invalid" do + let(:user_ids) { nil } - it { is_expected.to fail_to_find_a_model(:channel) } - end + it { is_expected.to fail_a_contract } + end - context "when current user can't view channel" do - fab!(:current_user) { Fabricate(:user) } - fab!(:channel_1) { Fabricate(:private_category_channel) } + context "when channel model is not found" do + before { params[:channel_id] = -1 } - it { is_expected.to fail_a_policy(:can_view_channel) } + it { is_expected.to fail_to_find_a_model(:channel) } + end + + context "when current user can't view channel" do + fab!(:current_user) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:private_category_channel) } + + it { is_expected.to fail_a_policy(:can_view_channel) } + end end end diff --git a/plugins/chat/spec/services/chat/search_chatable_spec.rb b/plugins/chat/spec/services/chat/search_chatable_spec.rb index 943f4723103..174d395c217 100644 --- a/plugins/chat/spec/services/chat/search_chatable_spec.rb +++ b/plugins/chat/spec/services/chat/search_chatable_spec.rb @@ -47,10 +47,10 @@ RSpec.describe Chat::SearchChatable do it "cleans the term" do params[:term] = "#bob" - expect(result.term).to eq("bob") + expect(result.contract.term).to eq("bob") params[:term] = "@bob" - expect(result.term).to eq("bob") + expect(result.contract.term).to eq("bob") end it "fetches user memberships" do diff --git a/plugins/chat/spec/services/chat/stop_message_streaming_spec.rb b/plugins/chat/spec/services/chat/stop_message_streaming_spec.rb index e4b6058907b..8e7b21a33fc 100644 --- a/plugins/chat/spec/services/chat/stop_message_streaming_spec.rb +++ b/plugins/chat/spec/services/chat/stop_message_streaming_spec.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true RSpec.describe Chat::StopMessageStreaming do + describe described_class::Contract, type: :model do + it { is_expected.to validate_presence_of(:message_id) } + end + describe ".call" do subject(:result) { described_class.call(params) } diff --git a/plugins/chat/spec/services/chat/trash_message_spec.rb b/plugins/chat/spec/services/chat/trash_message_spec.rb index b7bc327a458..d9dff95ec00 100644 --- a/plugins/chat/spec/services/chat/trash_message_spec.rb +++ b/plugins/chat/spec/services/chat/trash_message_spec.rb @@ -1,24 +1,29 @@ # frozen_string_literal: true RSpec.describe Chat::TrashMessage do - fab!(:current_user) { Fabricate(:user) } - let!(:guardian) { Guardian.new(current_user) } - fab!(:message) { Fabricate(:chat_message, user: current_user) } + describe described_class::Contract, type: :model do + it { is_expected.to validate_presence_of(:message_id) } + it { is_expected.to validate_presence_of(:channel_id) } + end describe ".call" do - subject(:result) { described_class.call(params) } + subject(:result) { described_class.call(**params, **dependencies) } + + fab!(:current_user) { Fabricate(:user) } + fab!(:message) { Fabricate(:chat_message, user: current_user) } + + let(:guardian) { Guardian.new(current_user) } + let(:params) { { message_id: message.id, channel_id: } } + let(:dependencies) { { guardian: } } + let(:channel_id) { message.chat_channel_id } context "when params are not valid" do - let(:params) { { guardian: guardian } } + let(:params) { {} } it { is_expected.to fail_a_contract } end context "when params are valid" do - let(:params) do - { guardian: guardian, message_id: message.id, channel_id: message.chat_channel_id } - end - context "when the user does not have permission to delete" do before { message.update!(user: Fabricate(:admin)) } @@ -26,9 +31,7 @@ RSpec.describe Chat::TrashMessage do end context "when the channel does not match the message" do - let(:params) do - { guardian: guardian, message_id: message.id, channel_id: Fabricate(:chat_channel).id } - end + let(:channel_id) { -1 } it { is_expected.to fail_to_find_a_model(:message) } end diff --git a/plugins/chat/spec/services/chat/trash_messages_spec.rb b/plugins/chat/spec/services/chat/trash_messages_spec.rb index 10ac0e19c9d..c0dae7e7aac 100644 --- a/plugins/chat/spec/services/chat/trash_messages_spec.rb +++ b/plugins/chat/spec/services/chat/trash_messages_spec.rb @@ -1,26 +1,30 @@ # frozen_string_literal: true RSpec.describe Chat::TrashMessages do - fab!(:current_user) { Fabricate(:user) } - let!(:guardian) { Guardian.new(current_user) } - fab!(:chat_channel) { Fabricate(:chat_channel) } - fab!(:message1) { Fabricate(:chat_message, user: current_user, chat_channel: chat_channel) } - fab!(:message2) { Fabricate(:chat_message, user: current_user, chat_channel: chat_channel) } + describe described_class::Contract, type: :model do + it { is_expected.to validate_presence_of(:channel_id) } + it { is_expected.to allow_values([1], (1..200).to_a).for(:message_ids) } + it { is_expected.not_to allow_values([], (1..201).to_a).for(:message_ids) } + end describe ".call" do - subject(:result) { described_class.call(params) } + subject(:result) { described_class.call(**params, **dependencies) } + + fab!(:current_user) { Fabricate(:user) } + fab!(:chat_channel) { Fabricate(:chat_channel) } + fab!(:message1) { Fabricate(:chat_message, user: current_user, chat_channel: chat_channel) } + fab!(:message2) { Fabricate(:chat_message, user: current_user, chat_channel: chat_channel) } + let(:guardian) { Guardian.new(current_user) } + let(:params) { { message_ids: [message1.id, message2.id], channel_id: chat_channel.id } } + let(:dependencies) { { guardian: } } context "when params are not valid" do - let(:params) { { guardian: guardian } } + let(:params) { {} } it { is_expected.to fail_a_contract } end context "when params are valid" do - let(:params) do - { guardian: guardian, message_ids: [message1.id, message2.id], channel_id: chat_channel.id } - end - context "when the user does not have permission to delete" do before { message1.update!(user: Fabricate(:admin)) } @@ -29,11 +33,7 @@ RSpec.describe Chat::TrashMessages do context "when the channel does not match the message" do let(:params) do - { - guardian: guardian, - message_ids: [message1.id, message2.id], - channel_id: Fabricate(:chat_channel).id, - } + { message_ids: [message1.id, message2.id], channel_id: Fabricate(:chat_channel).id } end it { is_expected.to fail_to_find_a_model(:messages) } diff --git a/plugins/chat/spec/services/chat/update_channel_status_spec.rb b/plugins/chat/spec/services/chat/update_channel_status_spec.rb index b3ccccec7d6..9df907e8d2f 100644 --- a/plugins/chat/spec/services/chat/update_channel_status_spec.rb +++ b/plugins/chat/spec/services/chat/update_channel_status_spec.rb @@ -1,52 +1,57 @@ # frozen_string_literal: true RSpec.describe(Chat::UpdateChannelStatus) do - subject(:result) do - described_class.call(guardian: guardian, channel_id: channel.id, status: status) + describe described_class::Contract, type: :model do + it do + is_expected.to validate_inclusion_of(:status).in_array(Chat::Channel.editable_statuses.keys) + end end - fab!(:channel) { Fabricate(:chat_channel) } - fab!(:current_user) { Fabricate(:admin) } + describe ".call" do + subject(:result) { described_class.call(**params, **dependencies) } - let(:guardian) { Guardian.new(current_user) } - let(:status) { "open" } + fab!(:channel) { Fabricate(:chat_channel) } + fab!(:current_user) { Fabricate(:admin) } - context "when no channel_id is given" do - subject(:result) { described_class.call(guardian: guardian, status: status) } + let(:params) { { channel_id:, status: } } + let(:dependencies) { { guardian: } } + let(:guardian) { Guardian.new(current_user) } + let(:status) { "open" } + let(:channel_id) { channel.id } - it { is_expected.to fail_to_find_a_model(:channel) } - end + context "when no channel_id is given" do + let(:channel_id) { nil } - context "when user is not allowed to change channel status" do - fab!(:current_user) { Fabricate(:user) } + it { is_expected.to fail_to_find_a_model(:channel) } + end - it { is_expected.to fail_a_policy(:check_channel_permission) } - end + context "when user is not allowed to change channel status" do + let!(:current_user) { Fabricate(:user) } - context "when status is not allowed" do - (Chat::Channel.statuses.keys - Chat::Channel.editable_statuses.keys).each do |na_status| - context "when status is '#{na_status}'" do - let(:status) { na_status } + it { is_expected.to fail_a_policy(:check_channel_permission) } + end - it { is_expected.to fail_a_contract } + context "when contract is invalid" do + let(:status) { :invalid_status } + + it { is_expected.to fail_a_contract } + end + + context "when new status is the same than the existing one" do + let(:status) { channel.status } + + it { is_expected.to fail_a_policy(:check_channel_permission) } + end + + context "when everything's ok" do + let(:status) { "closed" } + + it { is_expected.to run_successfully } + + it "changes the status" do + result + expect(channel.reload).to be_closed end end end - - context "when new status is the same than the existing one" do - let(:status) { channel.status } - - it { is_expected.to fail_a_policy(:check_channel_permission) } - end - - context "when status is allowed" do - let(:status) { "closed" } - - it { is_expected.to run_successfully } - - it "changes the status" do - result - expect(channel.reload).to be_closed - end - end end diff --git a/plugins/chat/spec/services/chat/update_thread_notification_settings_spec.rb b/plugins/chat/spec/services/chat/update_thread_notification_settings_spec.rb index c026504b399..ff72bfbfa94 100644 --- a/plugins/chat/spec/services/chat/update_thread_notification_settings_spec.rb +++ b/plugins/chat/spec/services/chat/update_thread_notification_settings_spec.rb @@ -1,10 +1,13 @@ # frozen_string_literal: true RSpec.describe Chat::UpdateThreadNotificationSettings do - describe Chat::UpdateThreadNotificationSettings::Contract, type: :model do + describe described_class::Contract, type: :model do + let(:notification_levels) { Chat::UserChatThreadMembership.notification_levels.values } + it { is_expected.to validate_presence_of :channel_id } it { is_expected.to validate_presence_of :thread_id } it { is_expected.to validate_presence_of :notification_level } + it { is_expected.to validate_inclusion_of(:notification_level).in_array(notification_levels) } end describe ".call" do diff --git a/plugins/chat/spec/services/chat/update_thread_spec.rb b/plugins/chat/spec/services/chat/update_thread_spec.rb index 5be66c740f2..00f8f48fbbd 100644 --- a/plugins/chat/spec/services/chat/update_thread_spec.rb +++ b/plugins/chat/spec/services/chat/update_thread_spec.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true RSpec.describe Chat::UpdateThread do - describe Chat::UpdateThread::Contract, type: :model do + describe described_class::Contract, type: :model do it { is_expected.to validate_presence_of :thread_id } + it { is_expected.to validate_length_of(:title).is_at_most(Chat::Thread::MAX_TITLE_LENGTH) } end describe ".call" do @@ -42,12 +43,6 @@ RSpec.describe Chat::UpdateThread do it { is_expected.to fail_a_contract } end - context "when title is too long" do - let(:title) { "a" * Chat::Thread::MAX_TITLE_LENGTH + "a" } - - it { is_expected.to fail_a_contract } - end - context "when thread is not found" do before { thread.destroy! } diff --git a/plugins/chat/spec/services/chat/upsert_draft_spec.rb b/plugins/chat/spec/services/chat/upsert_draft_spec.rb index 5727730c255..4dcc5004bd3 100644 --- a/plugins/chat/spec/services/chat/upsert_draft_spec.rb +++ b/plugins/chat/spec/services/chat/upsert_draft_spec.rb @@ -2,8 +2,6 @@ RSpec.describe Chat::UpsertDraft do describe described_class::Contract, type: :model do - subject(:contract) { described_class.new(data: nil, channel_id: nil, thread_id: nil) } - it { is_expected.to validate_presence_of :channel_id } end