diff --git a/app/services/messages/new_message_notification_service.rb b/app/services/messages/new_message_notification_service.rb index 49d0d76b7..49f36a072 100644 --- a/app/services/messages/new_message_notification_service.rb +++ b/app/services/messages/new_message_notification_service.rb @@ -4,32 +4,17 @@ class Messages::NewMessageNotificationService def perform return unless message.notifiable? - notify_participating_users notify_conversation_assignee + notify_participating_users end private delegate :conversation, :sender, :account, to: :message - def notify_participating_users - participating_users = conversation.conversation_participants.map(&:user) - participating_users -= [sender] if sender.is_a?(User) - - participating_users.uniq.each do |participant| - NotificationBuilder.new( - notification_type: 'participating_conversation_new_message', - user: participant, - account: account, - primary_actor: message.conversation, - secondary_actor: message - ).perform - end - end - def notify_conversation_assignee return if conversation.assignee.blank? - return if assignee_already_notified_via_participation? + return if already_notified?(conversation.assignee) return if conversation.assignee == sender NotificationBuilder.new( @@ -41,12 +26,26 @@ class Messages::NewMessageNotificationService ).perform end - def assignee_already_notified_via_participation? - return unless conversation.conversation_participants.map(&:user).include?(conversation.assignee) + def notify_participating_users + participating_users = conversation.conversation_participants.map(&:user) + participating_users -= [sender] if sender.is_a?(User) - # check whether participation notifcation is disabled for assignee - notification_setting = conversation.assignee.notification_settings.find_by(account_id: account.id) - notification_setting.public_send(:email_participating_conversation_new_message?) || notification_setting - .public_send(:push_participating_conversation_new_message?) + participating_users.uniq.each do |participant| + next if already_notified?(participant) + + NotificationBuilder.new( + notification_type: 'participating_conversation_new_message', + user: participant, + account: account, + primary_actor: message.conversation, + secondary_actor: message + ).perform + end + end + + # The user could already have been notified via a mention or via assignment + # So we don't need to notify them again + def already_notified?(user) + conversation.notifications.exists?(user: user, secondary_actor: message) end end diff --git a/spec/listeners/notification_listener_spec.rb b/spec/listeners/notification_listener_spec.rb index 1505b0f57..2291c401a 100644 --- a/spec/listeners/notification_listener_spec.rb +++ b/spec/listeners/notification_listener_spec.rb @@ -120,6 +120,51 @@ describe NotificationListener do end end + # integration tests to ensure that the order mention service and new message notification service are called in the correct order + describe 'message_created - mentions, participation & assignment integration' do + let(:event_name) { :'message.created' } + + it 'will not create duplicate new message notification for the same user for mentions participation & assignment' do + create(:inbox_member, user: first_agent, inbox: inbox) + conversation.update(assignee: first_agent) + + message = build( + :message, + conversation: conversation, + account: account, + content: "hi [#{first_agent.name}](mention://user/#{first_agent.id}/#{first_agent.name})", + private: true + ) + event = Events::Base.new(event_name, Time.zone.now, message: message) + listener.message_created(event) + + expect(first_agent.notifications.count).to eq(1) + expect(first_agent.notifications.first.notification_type).to eq('conversation_mention') + end + + it 'will not create duplicate new message notifications for assignment & participation' do + create(:inbox_member, user: first_agent, inbox: inbox) + conversation.update(assignee: first_agent) + # participants is created by async job. so creating it directly for testcase + conversation.conversation_participants.first_or_create(user: first_agent) + + message = build( + :message, + conversation: conversation, + account: account, + content: 'hi', + private: true + ) + + event = Events::Base.new(event_name, Time.zone.now, message: message) + listener.message_created(event) + + expect(conversation.conversation_participants.map(&:user)).to include(first_agent) + expect(first_agent.notifications.count).to eq(1) + expect(first_agent.notifications.first.notification_type).to eq('assigned_conversation_new_message') + end + end + describe 'conversation_bot_handoff' do let(:event_name) { :'conversation.bot_handoff' } diff --git a/spec/services/messages/new_message_notification_service_spec.rb b/spec/services/messages/new_message_notification_service_spec.rb index c76453174..51da1ad5a 100644 --- a/spec/services/messages/new_message_notification_service_spec.rb +++ b/spec/services/messages/new_message_notification_service_spec.rb @@ -16,7 +16,6 @@ describe Messages::NewMessageNotificationService do let(:participating_agent_2) { create(:user, account: account) } let(:inbox) { create(:inbox, account: account) } let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: assignee) } - let(:builder) { double } before do create(:inbox_member, inbox: inbox, user: participating_agent_1) @@ -25,8 +24,6 @@ describe Messages::NewMessageNotificationService do create(:conversation_participant, conversation: conversation, user: participating_agent_1) create(:conversation_participant, conversation: conversation, user: participating_agent_2) create(:conversation_participant, conversation: conversation, user: assignee) - allow(NotificationBuilder).to receive(:new).and_return(builder) - allow(builder).to receive(:perform) end context 'when message is created by a participant' do @@ -37,27 +34,19 @@ describe Messages::NewMessageNotificationService do end it 'creates notifications for other participating users' do - expect(NotificationBuilder).to have_received(:new).with(notification_type: 'participating_conversation_new_message', - user: participating_agent_2, - account: account, - primary_actor: message.conversation, - secondary_actor: message) + expect(participating_agent_2.notifications.where(notification_type: 'participating_conversation_new_message', account: account, + primary_actor: message.conversation, secondary_actor: message)).to exist end it 'creates notifications for assignee' do - expect(NotificationBuilder).to have_received(:new).with(notification_type: 'assigned_conversation_new_message', - user: assignee, - account: account, - primary_actor: message.conversation, - secondary_actor: message) + expect(assignee.notifications.where(notification_type: 'assigned_conversation_new_message', account: account, + primary_actor: message.conversation, secondary_actor: message)).to exist end it 'will not create notifications for the user who created the message' do - expect(NotificationBuilder).not_to have_received(:new).with(notification_type: 'participating_conversation_new_message', - user: participating_agent_1, - account: account, - primary_actor: message.conversation, - secondary_actor: message) + expect(participating_agent_1.notifications.where(notification_type: 'participating_conversation_new_message', + account: account, primary_actor: message.conversation, + secondary_actor: message)).not_to exist end end @@ -69,42 +58,34 @@ describe Messages::NewMessageNotificationService do end it 'creates notifications for assignee' do - expect(NotificationBuilder).to have_received(:new).with(notification_type: 'assigned_conversation_new_message', - user: assignee, - account: account, - primary_actor: message.conversation, - secondary_actor: message) + expect(assignee.notifications.where(notification_type: 'assigned_conversation_new_message', account: account, + primary_actor: message.conversation, secondary_actor: message)).to exist end it 'creates notifications for all participating users' do - expect(NotificationBuilder).to have_received(:new).with(notification_type: 'participating_conversation_new_message', - user: participating_agent_1, - account: account, - primary_actor: message.conversation, - secondary_actor: message) - expect(NotificationBuilder).to have_received(:new).with(notification_type: 'participating_conversation_new_message', - user: participating_agent_2, - account: account, - primary_actor: message.conversation, - secondary_actor: message) + expect(participating_agent_1.notifications.where(notification_type: 'participating_conversation_new_message', + account: account, primary_actor: message.conversation, + secondary_actor: message)).to exist + expect(participating_agent_2.notifications.where(notification_type: 'participating_conversation_new_message', + account: account, primary_actor: message.conversation, + secondary_actor: message)).to exist end end - context 'with multiple notifications are subscribed' do + context 'when multiple notification conditions are met' do let(:message) { create(:message, conversation: conversation, account: account) } before do - assignee.notification_settings.find_by(account_id: account.id).update(selected_email_flags: %w[email_assigned_conversation_new_message - email_participating_conversation_new_message]) described_class.new(message: message).perform end - it 'will not create assignee notifications for the assignee if participating notification was send' do - expect(NotificationBuilder).not_to have_received(:new).with(notification_type: 'assigned_conversation_new_message', - user: assignee, - account: account, - primary_actor: message.conversation, - secondary_actor: message) + it 'will not create participating notifications for the assignee if assignee notification was send' do + expect(assignee.notifications.where(notification_type: 'assigned_conversation_new_message', + account: account, primary_actor: message.conversation, + secondary_actor: message)).to exist + expect(assignee.notifications.where(notification_type: 'participating_conversation_new_message', + account: account, primary_actor: message.conversation, + secondary_actor: message)).not_to exist end end @@ -116,16 +97,12 @@ describe Messages::NewMessageNotificationService do end it 'will not create notifications for the user who created the message' do - expect(NotificationBuilder).not_to have_received(:new).with(notification_type: 'participating_conversation_new_message', - user: assignee, - account: account, - primary_actor: message.conversation, - secondary_actor: message) - expect(NotificationBuilder).not_to have_received(:new).with(notification_type: 'assigned_conversation_new_message', - user: assignee, - account: account, - primary_actor: message.conversation, - secondary_actor: message) + expect(assignee.notifications.where(notification_type: 'participating_conversation_new_message', + account: account, primary_actor: message.conversation, + secondary_actor: message)).not_to exist + expect(assignee.notifications.where(notification_type: 'assigned_conversation_new_message', + account: account, primary_actor: message.conversation, + secondary_actor: message)).not_to exist end end end