DEV: Refactor some services from chat

Extracted from https://github.com/discourse/discourse/pull/29129.

This patch makes the code more compliant with the upcoming service docs best practices.
This commit is contained in:
Loïc Guitaut 2024-10-18 17:09:23 +02:00 committed by Loïc Guitaut
parent 43a0ea876a
commit 08e9364573
24 changed files with 223 additions and 224 deletions

View File

@ -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]

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -39,7 +39,6 @@ module Chat
},
allow_nil: true
end
model :channel
policy :can_view_channel
model :membership, optional: true

View File

@ -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,
) || []

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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 {

View File

@ -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 {

View File

@ -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

View File

@ -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) }

View File

@ -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 }

View File

@ -1,15 +1,14 @@
# 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
describe ".call" do
subject(:result) { described_class.call(**params, **dependencies) }
fab!(:current_user) { Fabricate(:admin) }
fab!(:user_1) { Fabricate(:user) }
fab!(:user_2) { Fabricate(:user) }
@ -19,9 +18,8 @@ RSpec.describe Chat::InviteUsersToChannel do
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
let(:params) { { channel_id: channel_1.id, message_id:, user_ids: } }
let(:dependencies) { { guardian: } }
before do
group_1.add(user_1)
@ -73,6 +71,12 @@ RSpec.describe Chat::InviteUsersToChannel do
end
end
context "when contract is invalid" do
let(:user_ids) { nil }
it { is_expected.to fail_a_contract }
end
context "when channel model is not found" do
before { params[:channel_id] = -1 }
@ -86,3 +90,4 @@ RSpec.describe Chat::InviteUsersToChannel do
it { is_expected.to fail_a_policy(:can_view_channel) }
end
end
end

View File

@ -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

View File

@ -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) }

View File

@ -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

View File

@ -1,26 +1,30 @@
# frozen_string_literal: true
RSpec.describe Chat::TrashMessages do
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, **dependencies) }
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 ".call" do
subject(:result) { described_class.call(params) }
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) }

View File

@ -1,37 +1,41 @@
# 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
describe ".call" do
subject(:result) { described_class.call(**params, **dependencies) }
fab!(:channel) { Fabricate(:chat_channel) }
fab!(:current_user) { Fabricate(:admin) }
let(:params) { { channel_id:, status: } }
let(:dependencies) { { guardian: } }
let(:guardian) { Guardian.new(current_user) }
let(:status) { "open" }
let(:channel_id) { channel.id }
context "when no channel_id is given" do
subject(:result) { described_class.call(guardian: guardian, status: status) }
let(:channel_id) { nil }
it { is_expected.to fail_to_find_a_model(:channel) }
end
context "when user is not allowed to change channel status" do
fab!(:current_user) { Fabricate(:user) }
let!(:current_user) { Fabricate(:user) }
it { is_expected.to fail_a_policy(:check_channel_permission) }
end
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 }
context "when contract is invalid" do
let(:status) { :invalid_status }
it { is_expected.to fail_a_contract }
end
end
end
context "when new status is the same than the existing one" do
let(:status) { channel.status }
@ -39,7 +43,7 @@ RSpec.describe(Chat::UpdateChannelStatus) do
it { is_expected.to fail_a_policy(:check_channel_permission) }
end
context "when status is allowed" do
context "when everything's ok" do
let(:status) { "closed" }
it { is_expected.to run_successfully }
@ -50,3 +54,4 @@ RSpec.describe(Chat::UpdateChannelStatus) do
end
end
end
end

View File

@ -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

View File

@ -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! }

View File

@ -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