Discourse/discourse
Discourse/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-10-03 17:21:20 +08:00
Code Packages Activity Actions 7

DEV: Refactor Chat::ListChannelMessages service a bit

Browse source 
- improve the contract a little
- use `model` where possible
- extract message existence logic to a dedicated policy, allowing easier
  testing.
- remove unused code
- refactor specs to follow current guidelines/best practices
This commit is contained in:
Loïc Guitaut 2025-05-23 15:24:56 +02:00 • committed by Loïc Guitaut
parent 8337b49fe8
commit 3fbb2954cb
8 changed files with 295 additions and 333 deletions
Download patch file Download diff file Expand all files Collapse all files

4
lib/service/base.rb
View file

@ -85,6 +85,10 @@ module Service
return super if args.present?
store[method_name]
end

def respond_to_missing?(name, include_all)
store.key?(name) || super
end
end

# Internal module to define available steps as DSL

8
plugins/chat/app/serializers/chat/messages_serializer.rb
View file

@ -4,6 +4,8 @@ module Chat
class MessagesSerializer < ::ApplicationSerializer
attributes :messages, :tracking, :meta

delegate :metadata, to: :object, private: true

def initialize(object, opts)
super(object, opts)
@opts = opts
@ -30,9 +32,9 @@ module Chat

def meta
{
target_message_id: object.target_message_id,
can_load_more_future: object.can_load_more_future,
can_load_more_past: object.can_load_more_past,
target_message_id: metadata[:target_message]&.id,
can_load_more_future: metadata[:can_load_more_future],
can_load_more_past: metadata[:can_load_more_past],
}
end
end

26
plugins/chat/app/services/chat/channel/policy/message_existence.rb Normal file
View file

@ -0,0 +1,26 @@
# frozen_string_literal: true

class Chat::Channel::Policy::MessageExistence < Service::PolicyBase
delegate :channel, :target_message_id, to: :context

def call
return true if target_message_id.blank?
return false if target_message.blank?
return true if !target_message.trashed?
if target_message.trashed? && target_message.user_id == guardian.user.id || guardian.is_staff?
return true
end
context[:target_message_id] = nil
true
end

def reason
end

private

def target_message
@target_message ||=
Chat::Message.with_deleted.find_by(id: target_message_id, chat_channel: channel)
end
end

136
plugins/chat/app/services/chat/list_channel_messages.rb
View file

@ -5,7 +5,7 @@ module Chat
# or fetching paginated messages from last read.
#
# @example
# Chat::ListChannelMessages.call(params: { channel_id: 2, **optional_params }, guardian: guardian)
# Chat::ListChannelMessages.call(params: { channel_id: 2, **optional_params }, guardian:)
#
class ListChannelMessages
include Service::Base
@ -30,8 +30,10 @@ module Chat
validates :channel_id, presence: true
validates :page_size,
numericality: {
less_than_or_equal_to: ::Chat::MessagesQuery::MAX_PAGE_SIZE,
less_than_or_equal_to: Chat::MessagesQuery::MAX_PAGE_SIZE,
greater_than_or_equal_to: 1,
only_integer: true,
only_numeric: true,
},
allow_nil: true
validates :direction,
@ -39,19 +41,21 @@ module Chat
in: Chat::MessagesQuery::VALID_DIRECTIONS,
},
allow_nil: true

after_validation { self.page_size ||= Chat::MessagesQuery::MAX_PAGE_SIZE }
end

model :channel
policy :can_view_channel
model :membership, optional: true
step :enabled_threads?
step :determine_target_message_id
policy :target_message_exists
step :fetch_messages
step :fetch_thread_ids
step :fetch_tracking
step :fetch_thread_participants
step :fetch_thread_memberships
model :target_message_id, optional: true
policy :target_message_exists, class_name: Chat::Channel::Policy::MessageExistence
model :metadata, optional: true
model :messages, optional: true
model :thread_ids, optional: true
model :tracking, optional: true
model :thread_participants, optional: true
model :thread_memberships, optional: true
step :update_membership_last_viewed_at
step :update_user_last_channel

@ -65,107 +69,57 @@ module Chat
channel.membership_for(guardian.user)
end

def enabled_threads?(channel:)
context[:enabled_threads] = channel.threading_enabled
end

def can_view_channel(guardian:, channel:)
guardian.can_preview_chat_channel?(channel)
end

def determine_target_message_id(params:, membership:)
if params.fetch_from_last_read
context[:target_message_id] = membership&.last_read_message_id
else
context[:target_message_id] = params.target_message_id
end
def fetch_target_message_id(params:, membership:)
return params.target_message_id unless params.fetch_from_last_read
membership&.last_read_message_id
end

def target_message_exists(channel:, guardian:)
return true if context.target_message_id.blank?

target_message =
Chat::Message.with_deleted.find_by(id: context.target_message_id, chat_channel: channel)
return false if target_message.blank?

return true if !target_message.trashed?
if target_message.trashed? && target_message.user_id == guardian.user.id || guardian.is_staff?
return true
end

context[:target_message_id] = nil
true
end

def fetch_messages(channel:, params:, guardian:, enabled_threads:, target_message_id:)
messages_data =
::Chat::MessagesQuery.call(
channel:,
guardian:,
target_message_id:,
include_thread_messages: !enabled_threads,
page_size: params.page_size || Chat::MessagesQuery::MAX_PAGE_SIZE,
direction: params.direction,
target_date: params.target_date,
)

context[:can_load_more_past] = messages_data[:can_load_more_past]
context[:can_load_more_future] = messages_data[:can_load_more_future]
context[:target_message_id] = messages_data[:target_message_id]

messages_data[:target_message] = (
if messages_data[:target_message]&.thread_reply? &&
(enabled_threads || messages_data[:target_message].thread&.force)
[]
else
[messages_data[:target_message]]
end
def fetch_metadata(channel:, guardian:, target_message_id:, params:)
::Chat::MessagesQuery.call(
channel:,
guardian:,
target_message_id:,
include_thread_messages: !channel.threading_enabled?,
**params.slice(:page_size, :direction, :target_date),
)
end

context[:messages] = [
messages_data[:messages],
messages_data[:past_messages]&.reverse,
messages_data[:target_message],
messages_data[:future_messages],
def fetch_messages(metadata:)
[
metadata[:messages],
metadata[:past_messages]&.reverse,
(metadata[:target_message] unless metadata[:target_message]&.thread_reply?),
metadata[:future_messages],
].flatten.compact
end

def fetch_tracking(guardian:)
context[:tracking] = {}

return if !context.thread_ids.present?

context[:tracking] = ::Chat::TrackingStateReportQuery.call(
guardian: guardian,
thread_ids: context.thread_ids,
include_threads: true,
)
end

def fetch_thread_ids(messages:)
context[:thread_ids] = messages.map(&:thread_id).compact.uniq
messages.filter_map(&:thread_id).uniq
end

def fetch_thread_participants(messages:)
return if context.thread_ids.empty?

context[:thread_participants] = ::Chat::ThreadParticipantQuery.call(
thread_ids: context.thread_ids,
)
def fetch_tracking(guardian:, thread_ids:)
::Chat::TrackingStateReportQuery.(guardian:, thread_ids:, include_threads: true)
end

def fetch_thread_memberships(guardian:)
return if context.thread_ids.empty?
def fetch_thread_participants(messages:, thread_ids:)
return if thread_ids.blank?

context[:thread_memberships] = ::Chat::UserChatThreadMembership.where(
thread_id: context.thread_ids,
user_id: guardian.user.id,
)
::Chat::ThreadParticipantQuery.(thread_ids:)
end

def update_membership_last_viewed_at(guardian:)
def fetch_thread_memberships(guardian:, thread_ids:)
return if thread_ids.blank?

::Chat::UserChatThreadMembership.where(thread_id: thread_ids, user_id: guardian.user.id)
end

def update_membership_last_viewed_at(guardian:, membership:)
Scheduler::Defer.later "Chat::ListChannelMessages - defer update_membership_last_viewed_at" do
context.membership&.update!(last_viewed_at: Time.zone.now)
membership&.update!(last_viewed_at: Time.zone.now)
end
end


39
plugins/chat/app/services/chat/list_channel_thread_messages.rb
View file

@ -42,6 +42,7 @@ module Chat
model :membership, optional: true
step :determine_target_message_id
policy :target_message_exists
model :metadata, optional: true
model :messages, optional: true

private
@ -82,28 +83,26 @@ module Chat
target_message.user_id == guardian.user.id || guardian.is_staff?
end

def fetch_messages(thread:, guardian:, params:)
messages_data =
::Chat::MessagesQuery.call(
channel: thread.channel,
guardian: guardian,
target_message_id: context.target_message_id,
thread_id: thread.id,
page_size: params.page_size,
direction: params.direction,
target_date: params.target_date,
include_target_message_id:
params.fetch_from_first_message || params.fetch_from_last_message,
)

context[:can_load_more_past] = messages_data[:can_load_more_past]
context[:can_load_more_future] = messages_data[:can_load_more_future]
def fetch_metadata(thread:, guardian:, params:)
::Chat::MessagesQuery.call(
guardian:,
channel: thread.channel,
target_message_id: context.target_message_id,
thread_id: thread.id,
page_size: params.page_size,
direction: params.direction,
target_date: params.target_date,
include_target_message_id:
params.fetch_from_first_message || params.fetch_from_last_message,
)
end

def fetch_messages(metadata:)
[
messages_data[:messages],
messages_data[:past_messages]&.reverse,
messages_data[:target_message],
messages_data[:future_messages],
metadata[:messages],
metadata[:past_messages]&.reverse,
metadata[:target_message],
metadata[:future_messages],
].flatten.compact
end
end

75
plugins/chat/spec/services/chat/channel/policy/message_existence_spec.rb Normal file
View file

@ -0,0 +1,75 @@
# frozen_string_literal: true

RSpec.describe Chat::Channel::Policy::MessageExistence do
subject(:policy) { described_class.new(context) }

fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:channel) { Fabricate(:chat_channel) }

let(:guardian) { user.guardian }
let(:context) { Service::Base::Context.build(channel:, guardian:, target_message_id:) }

describe "#call" do
subject(:result) { policy.call }

context "when 'target_message_id' is not provided" do
let(:target_message_id) { nil }

it { is_expected.to be true }
end

context "when 'target_message_id' is provided" do
context "when target message does not exist" do
let(:target_message_id) { -1 }

it { is_expected.to be false }
end

context "when target message exists" do
fab!(:message) { Fabricate(:chat_message, chat_channel: channel, user:) }

let(:target_message_id) { message.id }

context "when target message is not trashed" do
it { is_expected.to be true }
end

context "when target message is trashed" do
before { message.trash! }

context "when target message’s user is the same as the guardian" do
it { is_expected.to be true }

it "does not set 'target_message_id' to nil" do
expect { result }.not_to change { context.target_message_id }
end
end

context "when target message’s user is different than the guardian" do
fab!(:other_user) { Fabricate(:user, refresh_auto_groups: true) }

before { message.update!(user: other_user) }

context "when guardian is staff" do
let(:guardian) { Discourse.system_user.guardian }

it { is_expected.to be true }

it "does not set 'target_message_id' to nil" do
expect { result }.not_to change { context.target_message_id }
end
end

context "when guardian is not staff" do
it { is_expected.to be true }

it "sets 'target_message_id' to nil" do
expect { result }.to change { context.target_message_id }.to(nil)
end
end
end
end
end
end
end
end

333
plugins/chat/spec/services/chat/list_channel_messages_spec.rb
View file

@ -1,267 +1,168 @@
# frozen_string_literal: true

RSpec.describe Chat::ListChannelMessages do
subject(:result) { described_class.call(params:, **dependencies) }
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:channel_id) }
it { is_expected.to allow_values(1, Chat::MessagesQuery::MAX_PAGE_SIZE, nil).for(:page_size) }
it do
is_expected.not_to allow_values(0, Chat::MessagesQuery::MAX_PAGE_SIZE + 1).for(:page_size)
end
it do
is_expected.to validate_inclusion_of(:direction).in_array(
Chat::MessagesQuery::VALID_DIRECTIONS,
).allow_nil
end

fab!(:user)
fab!(:channel) { Fabricate(:chat_channel) }
describe "#page_size" do
let(:contract) { described_class.new }

let(:guardian) { Guardian.new(user) }
let(:channel_id) { channel.id }
let(:optional_params) { {} }
let(:params) { { channel_id: }.merge(optional_params) }
let(:dependencies) { { guardian: } }
context "when page_size is not set" do
it "defaults to MAX_PAGE_SIZE" do
contract.validate
expect(contract.page_size).to eq(Chat::MessagesQuery::MAX_PAGE_SIZE)
end
end

before { channel.add(user) }
context "when page_size is set to nil" do
before { contract.page_size = nil }

context "when contract" do
context "when channel_id is not present" do
it "defaults to MAX_PAGE_SIZE" do
contract.validate
expect(contract.page_size).to eq(Chat::MessagesQuery::MAX_PAGE_SIZE)
end
end

context "when page_size is set" do
before { contract.page_size = 5 }

it "does not change the value" do
contract.validate
expect(contract.page_size).to eq(5)
end
end
end
end

describe ".call" do
subject(:result) { described_class.call(params:, **dependencies) }

fab!(:user)
fab!(:channel) { Fabricate(:chat_channel, threading_enabled: true) }

let(:guardian) { Guardian.new(user) }
let(:channel_id) { channel.id }
let(:optional_params) { {} }
let(:params) { { channel_id:, **optional_params } }
let(:dependencies) { { guardian: } }

before { channel.add(user) }

context "when data is not valid" do
let(:channel_id) { nil }

it { is_expected.to fail_a_contract }
end
end

context "when fetch_channel" do
context "when channel doesn’t exist" do
let(:channel_id) { -1 }

it { is_expected.to fail_to_find_a_model(:channel) }
end

context "when channel exists" do
it { is_expected.to run_successfully }

it "finds the correct channel" do
expect(result.channel).to eq(channel)
end
end
end

context "when fetch_eventual_membership" do
context "when user has membership" do
it { is_expected.to run_successfully }

it "finds the correct membership" do
expect(result.membership).to eq(channel.membership_for(user))
end
end

context "when user has no membership" do
before { channel.membership_for(user).destroy! }

it { is_expected.to run_successfully }

it "finds no membership" do
expect(result.membership).to be_blank
end
end
end

context "when enabled_threads?" do
context "when channel threading is disabled" do
before { channel.update!(threading_enabled: false) }

it "marks threads as disabled" do
expect(result.enabled_threads).to eq(false)
end
end

context "when channel and site setting are enabling threading" do
before { channel.update!(threading_enabled: true) }

it "marks threads as enabled" do
expect(result.enabled_threads).to eq(true)
end
end
end

context "when determine_target_message_id" do
context "when fetch_from_last_read is true" do
let(:optional_params) { { fetch_from_last_read: true } }

before do
channel.add(user)
channel.membership_for(user).update!(last_read_message_id: 1)
end

it "sets target_message_id to last_read_message_id" do
expect(result.target_message_id).to eq(1)
end
end
end

context "when target_message_exists" do
context "when no target_message_id is given" do
it { is_expected.to run_successfully }
end

context "when target message is not found" do
let(:optional_params) { { target_message_id: -1 } }

it { is_expected.to fail_a_policy(:target_message_exists) }
end

context "when target message is found" do
fab!(:target_message) { Fabricate(:chat_message, chat_channel: channel) }
let(:optional_params) { { target_message_id: target_message.id } }
context "when everything is ok" do
fab!(:thread) { Fabricate(:chat_thread, channel:) }
fab!(:messages) do
[thread.original_message, *Fabricate.times(20, :chat_message, chat_channel: channel)]
end

let(:tracking) { double }
let(:thread_ids) { [thread.id] }

before do
thread.add(user)
allow(Chat::TrackingStateReportQuery).to receive(:call).with(
guardian:,
thread_ids:,
include_threads: true,
).and_return(tracking)
end

it { is_expected.to run_successfully }
end

context "when target message is trashed" do
fab!(:target_message) { Fabricate(:chat_message, chat_channel: channel) }
let(:optional_params) { { target_message_id: target_message.id } }
it "finds the correct channel" do
expect(result.channel).to eq(channel)
end

before { target_message.trash! }
it "finds the correct membership" do
expect(result.membership).to eq(channel.membership_for(user))
end

context "when user is regular" do
it "nullifies target_message_id" do
expect(result.target_message_id).to be_blank
context "when fetch_from_last_read is true" do
let(:optional_params) { { fetch_from_last_read: true } }

before do
channel.add(user)
channel.membership_for(user).update!(last_read_message: messages.second)
end

it "sets target_message to last_read_message_id" do
expect(result.metadata[:target_message]).to eq(messages.second)
end
end

context "when user is the message creator" do
fab!(:target_message) { Fabricate(:chat_message, chat_channel: channel, user: user) }

it { is_expected.to run_successfully }
end

context "when user is admin" do
fab!(:user) { Fabricate(:admin) }

it { is_expected.to run_successfully }
end
end
end

context "when fetch_messages" do
context "with no params" do
fab!(:messages) { Fabricate.times(20, :chat_message, chat_channel: channel) }

it { is_expected.to be_a_success }

it "returns messages" do
expect(result.can_load_more_past).to eq(false)
expect(result.can_load_more_future).to eq(false)
expect(result.messages).to contain_exactly(*messages)
end
end

context "when target_date is provided" do
fab!(:past_message) do
Fabricate(:chat_message, chat_channel: channel, created_at: 3.days.ago)
end
fab!(:future_message) do
Fabricate(:chat_message, chat_channel: channel, created_at: 1.days.ago)
end

let(:optional_params) { { target_date: 2.days.ago } }

it { is_expected.to be_a_success }

it "includes past and future messages" do
expect(result.messages).to eq([past_message, future_message])
end
end
end

context "when fetch_tracking" do
context "when threads are disabled" do
fab!(:thread_1) { Fabricate(:chat_thread, channel: channel) }

before do
channel.update!(threading_enabled: false)
thread_1.add(user)
end

it { is_expected.to be_a_success }

it "returns tracking" do
Fabricate(:chat_message, chat_channel: channel, thread: thread_1)

expect(result.tracking.thread_tracking).to eq(
{
thread_1.id => {
channel_id: channel.id,
mention_count: 0,
unread_count: 0,
watched_threads_unread_count: 0,
},
},
expect(result).to have_attributes(
metadata: a_hash_including(can_load_more_future: false, can_load_more_past: false),
messages:,
)
end

context "when thread is forced" do
before { thread_1.update!(force: true) }
context "when target_date is provided" do
fab!(:past_message) do
Fabricate(:chat_message, chat_channel: channel, created_at: 3.days.ago)
end
fab!(:future_message) do
Fabricate(:chat_message, chat_channel: channel, created_at: 1.days.ago)
end

it { is_expected.to be_a_success }
let(:optional_params) { { target_date: 2.days.ago } }

it "returns tracking" do
Fabricate(:chat_message, chat_channel: channel, thread: thread_1)

expect(result.tracking.thread_tracking).to eq(
{
thread_1.id => {
channel_id: channel.id,
mention_count: 0,
unread_count: 1,
watched_threads_unread_count: 0,
},
},
)
it "includes past and future messages" do
expect(result.messages).to include(past_message, future_message)
end
end
end

context "when threads are enabled" do
fab!(:thread_1) { Fabricate(:chat_thread, channel: channel) }

before do
channel.update!(threading_enabled: true)
thread_1.add(user)
end

it { is_expected.to be_a_success }

it "returns tracking" do
Fabricate(:chat_message, chat_channel: channel, thread: thread_1)
expect(result.tracking).to eq(tracking)
end

expect(result.tracking.channel_tracking).to eq({})
expect(result.tracking.thread_tracking).to eq(
{
thread_1.id => {
channel_id: channel.id,
mention_count: 0,
unread_count: 1,
watched_threads_unread_count: 0,
},
},
it "updates the last viewed at" do
expect { result }.to change { channel.membership_for(user).last_viewed_at }.to be_within(
1.second,
).of(Time.zone.now)
end

it "updates the custom field" do
expect { result }.to change { user.custom_fields[Chat::LAST_CHAT_CHANNEL_ID] }.from(nil).to(
channel.id,
)
end
end
end

context "when update_membership_last_viewed_at" do
it "updates the last viewed at" do
expect { result }.to change { channel.membership_for(user).last_viewed_at }.to be_within(
1.second,
).of(Time.zone.now)
end
end
context "when custom field was already set" do
let(:field) { UserCustomField.find_by(name: Chat::LAST_CHAT_CHANNEL_ID, user_id: user.id) }

context "when update_user_last_channel" do
it "updates the custom field" do
expect { result }.to change { user.custom_fields[Chat::LAST_CHAT_CHANNEL_ID] }.from(nil).to(
channel.id,
)
end
before { user.upsert_custom_fields(::Chat::LAST_CHAT_CHANNEL_ID => channel.id) }

it "doesn’t update the custom field when it was already set to this value" do
user.upsert_custom_fields(::Chat::LAST_CHAT_CHANNEL_ID => channel.id)
field = UserCustomField.find_by(name: Chat::LAST_CHAT_CHANNEL_ID, user_id: user.id)

expect { result }.to_not change { field.reload.updated_at }
it "doesn’t update the custom field" do
expect { result }.to_not change { field.reload.updated_at }
end
end
end
end
end

7
plugins/chat/spec/services/chat/list_channel_thread_messages_spec.rb
View file

@ -149,9 +149,10 @@ RSpec.describe Chat::ListChannelThreadMessages do
end

it "returns messages" do
expect(result.can_load_more_past).to eq(false)
expect(result.can_load_more_future).to eq(false)
expect(result.messages).to contain_exactly(thread.original_message, *messages)
expect(result).to have_attributes(
metadata: a_hash_including(can_load_more_future: false, can_load_more_past: false),
messages: [thread.original_message, *messages],
)
end
end