From 9b9d162cc29ff41b042f842c8481c0722421325b Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Thu, 29 Dec 2022 12:55:06 -0300 Subject: [PATCH] Queue a different notify_mentioned job per mention type --- .../app/jobs/regular/chat_notify_mentioned.rb | 15 ++- plugins/chat/lib/chat_notifier.rb | 40 ++++--- .../regular/chat_notify_mentioned_spec.rb | 101 +++++++++++------- 3 files changed, 100 insertions(+), 56 deletions(-) diff --git a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb index d28c927603..d225931204 100644 --- a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb +++ b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb @@ -12,10 +12,21 @@ module Jobs return end + mention_type = args[:mention_type]&.to_sym + return if ( + mention_type.blank? || + ( + !Chat::ChatNotifier::STATIC_MENTION_TYPES.include?(mention_type) && + !Group.where("LOWER(name) = ?", mention_type).exists? + ) + ) + @creator = @chat_message.user @chat_channel = @chat_message.chat_channel - user_ids_to_notify = args[:to_notify_ids_map] || {} - user_ids_to_notify.each { |mention_type, ids| process_mentions(ids, mention_type.to_sym) } + + user_ids_to_notify = args[:user_ids].to_a + + process_mentions(user_ids_to_notify, mention_type) end private diff --git a/plugins/chat/lib/chat_notifier.rb b/plugins/chat/lib/chat_notifier.rb index 6e907888b7..df9e85d66a 100644 --- a/plugins/chat/lib/chat_notifier.rb +++ b/plugins/chat/lib/chat_notifier.rb @@ -27,6 +27,11 @@ # The ignore/mute filtering is also applied via the ChatNotifyWatching job, # which prevents desktop / push notifications being sent. class Chat::ChatNotifier + DIRECT_MENTIONS = :direct_mentions + HERE_MENTIONS = :here_mentions + GLOBAL_MENTIONS = :global_mentions + STATIC_MENTION_TYPES = [DIRECT_MENTIONS, HERE_MENTIONS, GLOBAL_MENTIONS] + class << self def push_notification_tag(type, chat_channel_id) "#{Discourse.current_hostname}-chat-#{type}-#{chat_channel_id}" @@ -64,14 +69,17 @@ class Chat::ChatNotifier to_notify = list_users_to_notify notify_creator_of_inaccessible_mentions(to_notify) - notify_mentioned_users(to_notify) + + to_notify.each do |mention_type, user_ids| + notify_mentioned_users(mention_type, user_ids) + end global_mentions = [] global_mentions << "all" if typed_global_mention? global_mentions << "here" if typed_here_mention? notify_watching_users( - to_notify[:direct_mentions], + to_notify[DIRECT_MENTIONS], global_mentions, to_notify[:mentioned_group_ids] ) @@ -84,7 +92,10 @@ class Chat::ChatNotifier purge_outdated_mentions(to_notify) notify_creator_of_inaccessible_mentions(to_notify) - notify_mentioned_users(to_notify) + + to_notify.each do |mention_type, user_ids| + notify_mentioned_users(mention_type, user_ids) + end to_notify end @@ -130,10 +141,8 @@ class Chat::ChatNotifier end def chat_users - users = - User.includes(:user_chat_channel_memberships, :group_users) - - users + User + .includes(:user_chat_channel_memberships, :group_users) .distinct .joins("LEFT OUTER JOIN user_chat_channel_memberships uccm ON uccm.user_id = users.id") .joins(:user_option) @@ -145,7 +154,7 @@ class Chat::ChatNotifier def channel_members chat_users.where( - user_chat_channel_memberships: { + uccm: { following: true, chat_channel_id: @chat_channel.id, }, @@ -189,19 +198,19 @@ class Chat::ChatNotifier .where("last_seen_at < ?", 5.minutes.ago) end - to_notify[:global_mentions] = global_mentions.pluck(:id) + to_notify[GLOBAL_MENTIONS] = global_mentions.pluck(:id) else - to_notify[:global_mentions] = [] + to_notify[GLOBAL_MENTIONS] = [] end end def expand_here_mention(to_notify, skip) if typed_here_mention? && @chat_channel.allow_channel_wide_mentions && !skip - to_notify[:here_mentions] = channel_wide_mentions(to_notify[:mentioned_group_ids]) + to_notify[HERE_MENTIONS] = channel_wide_mentions(to_notify[:mentioned_group_ids]) .where("last_seen_at > ?", 5.minutes.ago) .pluck(:id) else - to_notify[:here_mentions] = [] + to_notify[HERE_MENTIONS] = [] end end @@ -239,7 +248,7 @@ class Chat::ChatNotifier grouped = group_users_to_notify(direct_mentions) - to_notify[:direct_mentions] = grouped[:already_participating].map(&:id) + to_notify[DIRECT_MENTIONS] = grouped[:already_participating].map(&:id) to_notify[:welcome_to_join] = grouped[:welcome_to_join] to_notify[:unreachable] = grouped[:unreachable] end @@ -331,12 +340,13 @@ class Chat::ChatNotifier end end - def notify_mentioned_users(to_notify) + def notify_mentioned_users(mention_type, user_ids) Jobs.enqueue( :chat_notify_mentioned, { chat_message_id: @chat_message.id, - to_notify_ids_map: to_notify.as_json, + user_ids: user_ids, + mention_type: mention_type, timestamp: @timestamp.iso8601(6), }, ) diff --git a/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb b/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb index 5f5e4a248b..ac4c07c11b 100644 --- a/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb @@ -7,6 +7,8 @@ describe Jobs::ChatNotifyMentioned do fab!(:user_2) { Fabricate(:user) } fab!(:public_channel) { Fabricate(:category_channel) } + let(:user_ids) { [user_2.id] } + before do Group.refresh_automatic_groups! user_1.reload @@ -28,31 +30,34 @@ describe Jobs::ChatNotifyMentioned do def track_desktop_notification( user: user_2, message:, - to_notify_ids_map: + user_ids:, + mention_type: ) MessageBus .track_publish("/chat/notification-alert/#{user.id}") do subject.execute( chat_message_id: message.id, timestamp: message.created_at, - to_notify_ids_map: to_notify_ids_map + user_ids: user_ids, + mention_type: mention_type ) end .first end - def track_core_notification(user: user_2, message:, to_notify_ids_map:) + def track_core_notification(user: user_2, message:, user_ids:, mention_type:) subject.execute( chat_message_id: message.id, timestamp: message.created_at, - to_notify_ids_map: to_notify_ids_map, + user_ids: user_ids, + mention_type: mention_type ) Notification.where(user: user, notification_type: Notification.types[:chat_mention]).last end describe "scenarios where we should skip sending notifications" do - let(:to_notify_ids_map) { { here_mentions: [user_2.id] } } + let(:mention_type) { Chat::ChatNotifier::HERE_MENTIONS } it "does nothing if there is a newer version of the message" do message = create_chat_message @@ -61,7 +66,7 @@ describe Jobs::ChatNotifyMentioned do PostAlerter.expects(:push_notification).never desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil created_notification = @@ -79,7 +84,7 @@ describe Jobs::ChatNotifyMentioned do PostAlerter.expects(:push_notification).never desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil created_notification = @@ -95,7 +100,7 @@ describe Jobs::ChatNotifyMentioned do PostAlerter.expects(:push_notification).never desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil created_notification = @@ -110,7 +115,7 @@ describe Jobs::ChatNotifyMentioned do PostAlerter.expects(:push_notification).never desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil created_notification = @@ -127,7 +132,7 @@ describe Jobs::ChatNotifyMentioned do PostAlerter.expects(:push_notification).once desktop_notification = - track_desktop_notification(message: message_2, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message_2, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_present end @@ -135,14 +140,14 @@ describe Jobs::ChatNotifyMentioned do it "does nothing if user is not participating in a private channel" do user_3 = Fabricate(:user) @chat_group.add(user_3) - to_notify_map = { direct_mentions: [user_3.id] } - message = create_chat_message(channel: @personal_chat_channel) PostAlerter.expects(:push_notification).never desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_map) + track_desktop_notification( + message: message, user_ids: [user_3.id], mention_type: Chat::ChatNotifier::HERE_MENTIONS + ) expect(desktop_notification).to be_nil created_notification = @@ -157,7 +162,7 @@ describe Jobs::ChatNotifyMentioned do ) desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil end @@ -173,7 +178,8 @@ describe Jobs::ChatNotifyMentioned do subject.execute( chat_message_id: message.id, timestamp: message.created_at, - to_notify_ids_map: to_notify_ids_map, + user_ids: user_ids, + mention_type: mention_type ) end @@ -185,7 +191,7 @@ describe Jobs::ChatNotifyMentioned do ) desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil end @@ -202,10 +208,25 @@ describe Jobs::ChatNotifyMentioned do subject.execute( chat_message_id: message.id, timestamp: message.created_at, - to_notify_ids_map: to_notify_ids_map, + user_ids: user_ids, + mention_type: mention_type ) end + it "does nothing when the mention type is invalid" do + message = create_chat_message + + PostAlerter.expects(:push_notification).never + + desktop_notification = + track_desktop_notification(message: message, user_ids: user_ids, mention_type: "invalid") + expect(desktop_notification).to be_nil + + created_notification = + Notification.where(user: user_2, notification_type: Notification.types[:chat_mention]).last + expect(created_notification).to be_nil + end + context "when the user is muting the message sender" do it "does not send notifications to the user who is muting the acting user" do Fabricate(:muted_user, user: user_2, muted_user: user_1) @@ -214,7 +235,7 @@ describe Jobs::ChatNotifyMentioned do PostAlerter.expects(:push_notification).never desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil created_notification = @@ -229,7 +250,7 @@ describe Jobs::ChatNotifyMentioned do PostAlerter.expects(:push_notification).never desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_nil created_notification = @@ -246,7 +267,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification).to be_present expect(desktop_notification.data[:notification_type]).to eq(Notification.types[:chat_mention]) @@ -279,7 +300,8 @@ describe Jobs::ChatNotifyMentioned do subject.execute( chat_message_id: message.id, timestamp: message.created_at, - to_notify_ids_map: to_notify_ids_map, + user_ids: user_ids, + mention_type: mention_type ) end @@ -287,7 +309,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message created_notification = - track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_core_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(created_notification).to be_present expect(created_notification.high_priority).to eq(true) @@ -315,7 +337,8 @@ describe Jobs::ChatNotifyMentioned do subject.execute( chat_message_id: message.id, timestamp: message.created_at, - to_notify_ids_map: to_notify_ids_map, + user_ids: user_ids, + mention_type: mention_type ) end.first @@ -327,7 +350,7 @@ describe Jobs::ChatNotifyMentioned do describe "#execute" do describe "global mention notifications" do - let(:to_notify_ids_map) { { global_mentions: [user_2.id] } } + let(:mention_type) { Chat::ChatNotifier::GLOBAL_MENTIONS } let(:payload_translated_title) do I18n.t( @@ -344,7 +367,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message created_notification = - track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_core_notification(message: message, user_ids: user_ids, mention_type: mention_type) data_hash = created_notification.data_hash @@ -355,7 +378,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification.data[:translated_title]).to eq(payload_translated_title) end @@ -365,7 +388,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message(channel: @personal_chat_channel) desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expected_title = I18n.t( @@ -380,7 +403,7 @@ describe Jobs::ChatNotifyMentioned do end describe "here mention notifications" do - let(:to_notify_ids_map) { { here_mentions: [user_2.id] } } + let(:mention_type) { Chat::ChatNotifier::HERE_MENTIONS } let(:payload_translated_title) do I18n.t( @@ -397,7 +420,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message created_notification = - track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_core_notification(message: message, user_ids: user_ids, mention_type: mention_type) data_hash = created_notification.data_hash expect(data_hash[:identifier]).to eq("here") @@ -407,7 +430,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification.data[:translated_title]).to eq(payload_translated_title) end @@ -417,7 +440,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message(channel: @personal_chat_channel) desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expected_title = I18n.t( @@ -432,7 +455,7 @@ describe Jobs::ChatNotifyMentioned do end describe "direct mention notifications" do - let(:to_notify_ids_map) { { direct_mentions: [user_2.id] } } + let(:mention_type) { Chat::ChatNotifier::DIRECT_MENTIONS } let(:payload_translated_title) do I18n.t( @@ -449,7 +472,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message created_notification = - track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_core_notification(message: message, user_ids: user_ids, mention_type: mention_type) data_hash = created_notification.data_hash expect(data_hash[:identifier]).to be_nil @@ -459,7 +482,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification.data[:translated_title]).to eq(payload_translated_title) end @@ -469,7 +492,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message(channel: @personal_chat_channel) desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expected_title = I18n.t( @@ -484,7 +507,7 @@ describe Jobs::ChatNotifyMentioned do end describe "group mentions" do - let(:to_notify_ids_map) { { @chat_group.name.to_sym => [user_2.id] } } + let(:mention_type) { @chat_group.name.to_sym } let(:payload_translated_title) do I18n.t( @@ -501,7 +524,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message created_notification = - track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_core_notification(message: message, user_ids: user_ids, mention_type: mention_type) data_hash = created_notification.data_hash expect(data_hash[:identifier]).to eq(@chat_group.name) @@ -512,7 +535,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expect(desktop_notification.data[:translated_title]).to eq(payload_translated_title) end @@ -522,7 +545,7 @@ describe Jobs::ChatNotifyMentioned do message = create_chat_message(channel: @personal_chat_channel) desktop_notification = - track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) + track_desktop_notification(message: message, user_ids: user_ids, mention_type: mention_type) expected_title = I18n.t(