DEV: Introduce a Service::ActionBase class for service actions

This will help to enforce a consistent pattern for creating service
actions.

This patch also namespaces actions and policies, making everything
related to a service available directly in
`app/services/<concept-name>`, making things more consistent at that
level too.
This commit is contained in:
Loïc Guitaut 2024-09-12 15:09:10 +02:00 committed by Loïc Guitaut
parent aa9c59a24b
commit 05b8ff436c
36 changed files with 269 additions and 246 deletions

View File

@ -45,6 +45,7 @@ reviewed:
- concurrent-ruby # MIT
- css_parser # MIT
- drb # BSD-2-Clause
- dry-initializer # MIT
- excon # MIT
- faraday-em_http # MIT
- faraday-em_synchrony # MIT

View File

@ -287,3 +287,5 @@ group :migrations, optional: true do
# CLI
gem "ruby-progressbar"
end
gem "dry-initializer", "~> 3.1"

View File

@ -132,6 +132,7 @@ GEM
literate_randomizer
docile (1.4.1)
drb (2.2.1)
dry-initializer (3.1.1)
email_reply_trimmer (0.1.13)
erubi (1.13.0)
excon (0.111.0)
@ -623,6 +624,7 @@ DEPENDENCIES
discourse-fonts
discourse-seed-fu
discourse_dev_assets
dry-initializer (~> 3.1)
email_reply_trimmer
excon
execjs

View File

@ -1,51 +0,0 @@
# frozen_string_literal: true
module Action
module User
class SilenceAll
attr_reader :users, :actor, :contract
delegate :message, :post_id, :silenced_till, :reason, to: :contract, private: true
def initialize(users:, actor:, contract:)
@users, @actor, @contract = users, actor, contract
end
def self.call(...)
new(...).call
end
def call
silenced_users.first.try(:user_history).try(:details)
end
private
def silenced_users
users.map do |user|
UserSilencer
.new(
user,
actor,
message_body: message,
keep_posts: true,
silenced_till:,
reason:,
post_id:,
)
.tap do |silencer|
next unless silencer.silence
Jobs.enqueue(
:critical_user_email,
type: "account_silenced",
user_id: user.id,
user_history_id: silencer.user_history.id,
)
end
rescue => err
Discourse.warn_exception(err, message: "failed to silence user with ID #{user.id}")
end
end
end
end
end

View File

@ -1,40 +0,0 @@
# frozen_string_literal: true
module Action
module User
class SuspendAll
attr_reader :users, :actor, :contract
delegate :message, :post_id, :suspend_until, :reason, to: :contract, private: true
def initialize(users:, actor:, contract:)
@users, @actor, @contract = users, actor, contract
end
def self.call(...)
new(...).call
end
def call
suspended_users.first.try(:user_history).try(:details)
end
private
def suspended_users
users.map do |user|
UserSuspender.new(
user,
suspended_till: suspend_until,
reason: reason,
by_user: actor,
message: message,
post_id: post_id,
).tap(&:suspend)
rescue => err
Discourse.warn_exception(err, message: "failed to suspend user with ID #{user.id}")
end
end
end
end
end

View File

@ -1,48 +0,0 @@
# frozen_string_literal: true
module Action
module User
class TriggerPostAction
attr_reader :guardian, :post, :contract
delegate :post_action, to: :contract, private: true
delegate :user, to: :guardian, private: true
def initialize(guardian:, post:, contract:)
@guardian, @post, @contract = guardian, post, contract
end
def self.call(...)
new(...).call
end
def call
return if post.blank? || post_action.blank?
send(post_action)
rescue NoMethodError
end
private
def delete
return unless guardian.can_delete_post_or_topic?(post)
PostDestroyer.new(user, post).destroy
end
def delete_replies
return unless guardian.can_delete_post_or_topic?(post)
PostDestroyer.delete_with_replies(user, post)
end
def edit
# Take what the moderator edited in as gospel
PostRevisor.new(post).revise!(
user,
{ raw: contract.post_edit },
skip_validations: true,
skip_revision: true,
)
end
end
end
end

View File

@ -0,0 +1,41 @@
# frozen_string_literal: true
class User::Action::SilenceAll < Service::ActionBase
option :users, []
option :actor
option :contract
delegate :message, :post_id, :silenced_till, :reason, to: :contract, private: true
def call
silenced_users.first.try(:user_history).try(:details)
end
private
def silenced_users
users.map do |user|
UserSilencer
.new(
user,
actor,
message_body: message,
keep_posts: true,
silenced_till:,
reason:,
post_id:,
)
.tap do |silencer|
next unless silencer.silence
Jobs.enqueue(
:critical_user_email,
type: "account_silenced",
user_id: user.id,
user_history_id: silencer.user_history.id,
)
end
rescue => err
Discourse.warn_exception(err, message: "failed to silence user with ID #{user.id}")
end
end
end

View File

@ -0,0 +1,30 @@
# frozen_string_literal: true
class User::Action::SuspendAll < Service::ActionBase
option :users, []
option :actor
option :contract
delegate :message, :post_id, :suspend_until, :reason, to: :contract, private: true
def call
suspended_users.first.try(:user_history).try(:details)
end
private
def suspended_users
users.map do |user|
UserSuspender.new(
user,
suspended_till: suspend_until,
reason: reason,
by_user: actor,
message: message,
post_id: post_id,
).tap(&:suspend)
rescue => err
Discourse.warn_exception(err, message: "failed to suspend user with ID #{user.id}")
end
end
end

View File

@ -0,0 +1,38 @@
# frozen_string_literal: true
class User::Action::TriggerPostAction < Service::ActionBase
option :guardian
option :post
option :contract
delegate :post_action, to: :contract, private: true
delegate :user, to: :guardian, private: true
def call
return if post.blank? || post_action.blank?
send(post_action)
rescue NoMethodError
end
private
def delete
return unless guardian.can_delete_post_or_topic?(post)
PostDestroyer.new(user, post).destroy
end
def delete_replies
return unless guardian.can_delete_post_or_topic?(post)
PostDestroyer.delete_with_replies(user, post)
end
def edit
# Take what the moderator edited in as gospel
PostRevisor.new(post).revise!(
user,
{ raw: contract.post_edit },
skip_validations: true,
skip_revision: true,
)
end
end

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
class User::NotAlreadySilencedPolicy < PolicyBase
class User::Policy::NotAlreadySilenced < Service::PolicyBase
delegate :user, to: :context, private: true
delegate :silenced_record, to: :user, private: true

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
class User::NotAlreadySuspendedPolicy < PolicyBase
class User::Policy::NotAlreadySuspended < Service::PolicyBase
delegate :user, to: :context, private: true
delegate :suspend_record, to: :user, private: true

View File

@ -5,7 +5,7 @@ class User::Silence
contract
model :user
policy :not_silenced_already, class_name: User::NotAlreadySilencedPolicy
policy :not_silenced_already, class_name: User::Policy::NotAlreadySilenced
model :users
policy :can_silence_all_users
step :silence
@ -44,7 +44,7 @@ class User::Silence
end
def silence(guardian:, users:, contract:)
context[:full_reason] = Action::User::SilenceAll.call(users:, actor: guardian.user, contract:)
context[:full_reason] = User::Action::SilenceAll.call(users:, actor: guardian.user, contract:)
end
def fetch_post(contract:)
@ -52,6 +52,6 @@ class User::Silence
end
def perform_post_action(guardian:, post:, contract:)
Action::User::TriggerPostAction.call(guardian:, post:, contract:)
User::Action::TriggerPostAction.call(guardian:, post:, contract:)
end
end

View File

@ -5,7 +5,7 @@ class User::Suspend
contract
model :user
policy :not_suspended_already, class_name: User::NotAlreadySuspendedPolicy
policy :not_suspended_already, class_name: User::Policy::NotAlreadySuspended
model :users
policy :can_suspend_all_users
step :suspend
@ -44,7 +44,7 @@ class User::Suspend
end
def suspend(guardian:, users:, contract:)
context[:full_reason] = Action::User::SuspendAll.call(users:, actor: guardian.user, contract:)
context[:full_reason] = User::Action::SuspendAll.call(users:, actor: guardian.user, contract:)
end
def fetch_post(contract:)
@ -52,6 +52,6 @@ class User::Suspend
end
def perform_post_action(guardian:, post:, contract:)
Action::User::TriggerPostAction.call(guardian:, post:, contract:)
User::Action::TriggerPostAction.call(guardian:, post:, contract:)
end
end

View File

@ -0,0 +1,13 @@
# frozen_string_literal: true
class Service::ActionBase
extend Dry::Initializer
def self.call(...)
new(...).call
end
def call
raise "Not implemented"
end
end

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
class PolicyBase
class Service::PolicyBase
attr_reader :context
delegate :guardian, to: :context

View File

@ -18,52 +18,12 @@ module Chat
# Here, we can efficiently query the channel category permissions and figure
# out which of the users provided should have their [Chat::UserChatChannelMembership]
# records removed based on those security cases.
class CalculateMembershipsForRemoval
def self.call(scoped_users_query:, channel_ids: nil)
channel_permissions_map =
DB.query(<<~SQL, readonly: CategoryGroup.permission_types[:readonly])
WITH category_group_channel_map AS (
SELECT category_groups.group_id,
category_groups.permission_type,
chat_channels.id AS channel_id
FROM category_groups
INNER JOIN categories ON categories.id = category_groups.category_id
INNER JOIN chat_channels ON categories.id = chat_channels.chatable_id
AND chat_channels.chatable_type = 'Category'
)
SELECT chat_channels.id AS channel_id,
chat_channels.chatable_id AS category_id,
(
SELECT string_agg(category_group_channel_map.group_id::varchar, ',')
FROM category_group_channel_map
WHERE category_group_channel_map.permission_type < :readonly AND
category_group_channel_map.channel_id = chat_channels.id
) AS groups_with_write_permissions,
(
SELECT string_agg(category_group_channel_map.group_id::varchar, ',')
FROM category_group_channel_map
WHERE category_group_channel_map.permission_type = :readonly AND
category_group_channel_map.channel_id = chat_channels.id
) AS groups_with_readonly_permissions,
categories.read_restricted
FROM category_group_channel_map
INNER JOIN chat_channels ON chat_channels.id = category_group_channel_map.channel_id
INNER JOIN categories ON categories.id = chat_channels.chatable_id
WHERE chat_channels.chatable_type = 'Category'
#{channel_ids.present? ? "AND chat_channels.id IN (#{channel_ids.join(",")})" : ""}
GROUP BY chat_channels.id, chat_channels.chatable_id, categories.read_restricted
ORDER BY channel_id
SQL
scoped_memberships =
Chat::UserChatChannelMembership
.joins(:chat_channel)
.where(user_id: scoped_users_query.select(:id))
.where(chat_channel_id: channel_permissions_map.map(&:channel_id))
class CalculateMembershipsForRemoval < Service::ActionBase
option :scoped_users_query
option :channel_ids, [], optional: true
def call
memberships_to_remove = []
scoped_memberships.find_each do |membership|
channel_permission =
channel_permissions_map.find { |cpm| cpm.channel_id == membership.chat_channel_id }
@ -102,6 +62,54 @@ module Chat
memberships_to_remove
end
private
def channel_permissions_map
@channel_permissions_map ||=
DB.query(<<~SQL, readonly: CategoryGroup.permission_types[:readonly])
WITH category_group_channel_map AS (
SELECT category_groups.group_id,
category_groups.permission_type,
chat_channels.id AS channel_id
FROM category_groups
INNER JOIN categories ON categories.id = category_groups.category_id
INNER JOIN chat_channels ON categories.id = chat_channels.chatable_id
AND chat_channels.chatable_type = 'Category'
)
SELECT chat_channels.id AS channel_id,
chat_channels.chatable_id AS category_id,
(
SELECT string_agg(category_group_channel_map.group_id::varchar, ',')
FROM category_group_channel_map
WHERE category_group_channel_map.permission_type < :readonly AND
category_group_channel_map.channel_id = chat_channels.id
) AS groups_with_write_permissions,
(
SELECT string_agg(category_group_channel_map.group_id::varchar, ',')
FROM category_group_channel_map
WHERE category_group_channel_map.permission_type = :readonly AND
category_group_channel_map.channel_id = chat_channels.id
) AS groups_with_readonly_permissions,
categories.read_restricted
FROM category_group_channel_map
INNER JOIN chat_channels ON chat_channels.id = category_group_channel_map.channel_id
INNER JOIN categories ON categories.id = chat_channels.chatable_id
WHERE chat_channels.chatable_type = 'Category'
#{channel_ids.present? ? "AND chat_channels.id IN (#{channel_ids.join(",")})" : ""}
GROUP BY chat_channels.id, chat_channels.chatable_id, categories.read_restricted
ORDER BY channel_id
SQL
end
def scoped_memberships
@scoped_memberships ||=
Chat::UserChatChannelMembership
.joins(:chat_channel)
.where(user_id: scoped_users_query.select(:id))
.where(chat_channel_id: channel_permissions_map.map(&:channel_id))
end
end
end
end

View File

@ -2,19 +2,11 @@
module Chat
module Action
class CreateMembershipsForAutoJoin
def self.call(channel:, contract:)
query_args = {
chat_channel_id: channel.id,
start: contract.start_user_id,
end: contract.end_user_id,
suspended_until: Time.zone.now,
last_seen_at: 3.months.ago,
channel_category: channel.category.id,
permission_type: CategoryGroup.permission_types[:create_post],
everyone: Group::AUTO_GROUPS[:everyone],
mode: ::Chat::UserChatChannelMembership.join_modes[:automatic],
}
class CreateMembershipsForAutoJoin < Service::ActionBase
option :channel
option :contract
def call
::DB.query_single(<<~SQL, query_args)
INSERT INTO user_chat_channel_memberships (user_id, chat_channel_id, following, created_at, updated_at, join_mode)
SELECT DISTINCT(users.id), :chat_channel_id, TRUE, NOW(), NOW(), :mode
@ -44,6 +36,22 @@ module Chat
RETURNING user_chat_channel_memberships.user_id
SQL
end
private
def query_args
{
chat_channel_id: channel.id,
start: contract.start_user_id,
end: contract.end_user_id,
suspended_until: Time.zone.now,
last_seen_at: 3.months.ago,
channel_category: channel.category.id,
permission_type: CategoryGroup.permission_types[:create_post],
everyone: Group::AUTO_GROUPS[:everyone],
mode: ::Chat::UserChatChannelMembership.join_modes[:automatic],
}
end
end
end
end

View File

@ -4,7 +4,7 @@ module Chat
module Action
# When updating the read state of chat channel memberships, we also need
# to be sure to mark any mention-based notifications read at the same time.
class MarkMentionsRead
class MarkMentionsRead < Service::ActionBase
# @param [User] user The user that we are marking notifications read for.
# @param [Array] channel_ids The chat channels that are having their notifications
# marked as read.
@ -12,7 +12,12 @@ module Chat
# mentions read for in the channel.
# @param [Integer] thread_id Optional, if provided then all notifications related
# to messages in the thread will be marked as read.
def self.call(user, channel_ids:, message_id: nil, thread_id: nil)
param :user
option :channel_ids, []
option :message_id, optional: true
option :thread_id, optional: true
def call
::Notification
.where(notification_type: Notification.types[:chat_mention])
.where(user: user)

View File

@ -2,19 +2,11 @@
module Chat
module Action
class PublishAndFollowDirectMessageChannel
attr_reader :channel_membership
class PublishAndFollowDirectMessageChannel < Service::ActionBase
option :channel_membership
delegate :chat_channel, :user, to: :channel_membership
def self.call(...)
new(...).call
end
def initialize(channel_membership:)
@channel_membership = channel_membership
end
def call
return unless chat_channel.direct_message_channel?
return if users_allowing_communication.none?

View File

@ -7,12 +7,15 @@ module Chat
# were removed and from which channel, as well as logging
# this in staff actions so it's obvious why these users were
# removed.
class PublishAutoRemovedUser
class PublishAutoRemovedUser < Service::ActionBase
# @param [Symbol] event_type What caused the users to be removed,
# each handler will define this, e.g. category_updated, user_removed_from_group
# @param [Hash] users_removed_map A hash with channel_id as its keys and an
# array of user_ids who were removed from the channel.
def self.call(event_type:, users_removed_map:)
option :event_type
option :users_removed_map
def call
return if users_removed_map.empty?
users_removed_map.each do |channel_id, user_ids|

View File

@ -2,8 +2,10 @@
module Chat
module Action
class RemoveMemberships
def self.call(memberships:)
class RemoveMemberships < Service::ActionBase
option :memberships
def call
memberships
.destroy_all
.each_with_object(Hash.new { |h, k| h[k] = [] }) do |obj, hash|

View File

@ -2,14 +2,17 @@
module Chat
module Action
class ResetChannelsLastMessageIds
class ResetChannelsLastMessageIds < Service::ActionBase
# @param [Array] last_message_ids The message IDs to match with the
# last_message_id in Chat::Channel which will be reset
# to NULL or the most recent non-deleted message in the channel to
# update read state.
# @param [Integer] channel_ids The channel IDs to update. This is used
# to scope the queries better.
def self.call(last_message_ids, channel_ids)
param :last_message_ids, []
param :channel_ids, []
def call
Chat::Channel
.where(id: channel_ids)
.where("last_message_id IN (?)", last_message_ids)

View File

@ -2,15 +2,24 @@
module Chat
module Action
class ResetUserLastReadChannelMessage
class ResetUserLastReadChannelMessage < Service::ActionBase
# @param [Array] last_read_message_ids The message IDs to match with the
# last_read_message_ids in UserChatChannelMembership which will be reset
# to NULL or the most recent non-deleted message in the channel to
# update read state.
# @param [Integer] channel_ids The channel IDs of the memberships to update,
# this is used to find the latest non-deleted message in the channel.
def self.call(last_read_message_ids, channel_ids)
sql = <<~SQL
param :last_read_message_ids, []
param :channel_ids, []
def call
DB.exec(sql_query, last_read_message_ids:, channel_ids:)
end
private
def sql_query
<<~SQL
-- update the last_read_message_id to the most recent
-- non-deleted message in the channel so unread counts are correct.
-- the cte row_number is necessary to only return a single row
@ -33,8 +42,6 @@ module Chat
SET last_read_message_id = NULL
WHERE last_read_message_id IN (:last_read_message_ids);
SQL
DB.exec(sql, last_read_message_ids: last_read_message_ids, channel_ids: channel_ids)
end
end
end

View File

@ -2,15 +2,24 @@
module Chat
module Action
class ResetUserLastReadThreadMessage
class ResetUserLastReadThreadMessage < Service::ActionBase
# @param [Array] last_read_message_ids The message IDs to match with the
# last_read_message_ids in UserChatThreadMembership which will be reset
# to NULL or the most recent non-deleted message in the thread to
# update read state.
# @param [Integer] thread_ids The thread IDs of the memberships to update,
# this is used to find the latest non-deleted message in the thread.
def self.call(last_read_message_ids, thread_ids)
sql = <<~SQL
param :last_read_message_ids, []
param :thread_ids, []
def call
DB.exec(sql_query, last_read_message_ids:, thread_ids:)
end
private
def sql_query
<<~SQL
-- update the last_read_message_id to the most recent
-- non-deleted message in the thread so unread counts are correct.
-- the cte row_number is necessary to only return a single row
@ -40,8 +49,6 @@ module Chat
SET last_read_message_id = NULL
WHERE last_read_message_id IN (:last_read_message_ids);
SQL
DB.exec(sql, last_read_message_ids: last_read_message_ids, thread_ids: thread_ids)
end
end
end

View File

@ -27,7 +27,7 @@ module Chat
policy :can_add_users_to_channel
model :target_users, optional: true
policy :satisfies_dms_max_users_limit,
class_name: Chat::DirectMessageChannel::MaxUsersExcessPolicy
class_name: Chat::DirectMessageChannel::Policy::MaxUsersExcess
transaction do
step :upsert_memberships

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
class Chat::Channel::MessageCreationPolicy < PolicyBase
class Chat::Channel::Policy::MessageCreation < Service::PolicyBase
class DirectMessageStrategy
class << self
def call(guardian, channel)

View File

@ -27,11 +27,11 @@ module Chat
model :target_users
policy :can_create_direct_message
policy :satisfies_dms_max_users_limit,
class_name: Chat::DirectMessageChannel::MaxUsersExcessPolicy
class_name: Chat::DirectMessageChannel::Policy::MaxUsersExcess
model :user_comm_screener
policy :actor_allows_dms
policy :targets_allow_dms_from_user,
class_name: Chat::DirectMessageChannel::CanCommunicateAllPartiesPolicy
class_name: Chat::DirectMessageChannel::Policy::CanCommunicateAllParties
model :direct_message, :fetch_or_create_direct_message
model :channel, :fetch_or_create_channel
step :set_optional_name

View File

@ -26,7 +26,7 @@ module Chat
model :channel
step :enforce_membership
model :membership
policy :allowed_to_create_message_in_channel, class_name: Chat::Channel::MessageCreationPolicy
policy :allowed_to_create_message_in_channel, class_name: Chat::Channel::Policy::MessageCreation
model :reply, optional: true
policy :ensure_reply_consistency
model :thread, optional: true

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
class Chat::DirectMessageChannel::CanCommunicateAllPartiesPolicy < PolicyBase
class Chat::DirectMessageChannel::Policy::CanCommunicateAllParties < Service::PolicyBase
delegate :target_users, :user_comm_screener, to: :context
def call

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
class Chat::DirectMessageChannel::MaxUsersExcessPolicy < PolicyBase
class Chat::DirectMessageChannel::Policy::MaxUsersExcess < Service::PolicyBase
delegate :target_users, to: :context
def call

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
RSpec.describe Chat::Channel::MessageCreationPolicy do
RSpec.describe Chat::Channel::Policy::MessageCreation do
subject(:policy) { described_class.new(context) }
fab!(:user)

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
RSpec.describe Action::User::TriggerPostAction do
RSpec.describe User::Action::TriggerPostAction do
describe ".call" do
subject(:action) { described_class.call(guardian:, post:, contract:) }

View File

@ -63,7 +63,7 @@ RSpec.describe User::Silence do
end
context "when all users can be silenced" do
before { allow(Action::User::TriggerPostAction).to receive(:call) }
before { allow(User::Action::TriggerPostAction).to receive(:call) }
it "silences all provided users" do
result
@ -80,7 +80,7 @@ RSpec.describe User::Silence do
it "triggers a post action" do
result
expect(Action::User::TriggerPostAction).to have_received(:call).with(
expect(User::Action::TriggerPostAction).to have_received(:call).with(
guardian:,
post: nil,
contract: result[:contract],

View File

@ -65,7 +65,7 @@ RSpec.describe User::Suspend do
end
context "when all users can be suspended" do
before { allow(Action::User::TriggerPostAction).to receive(:call) }
before { allow(User::Action::TriggerPostAction).to receive(:call) }
it "suspends all provided users" do
result
@ -74,7 +74,7 @@ RSpec.describe User::Suspend do
it "triggers a post action" do
result
expect(Action::User::TriggerPostAction).to have_received(:call).with(
expect(User::Action::TriggerPostAction).to have_received(:call).with(
guardian:,
post: nil,
contract: result[:contract],