scottk/chatwoot
1
0
Fork 0
mirror of https://github.com/lingble/chatwoot.git synced 2025-11-01 19:48:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Prevent duplicate notifications for mentions (#10675)

Browse Source 
This PR addresses an issue where mention_notification was being
overwritten by assigned_conversation_new_message notifications.
Similarly, participating_conversation_new_message was taking priority
over assigned_conversation_new_message.

The fix ensures that additional notifications are not created in these
scenarios by establishing a priority order for notifications as follows:

1. mention_notification
2. assigned_conversation_new_message
3. participating_conversation_new_message

This change maintains consistency and prevents redundant notifications.

----

Note: In the older implementation of notifications in Chatwoot,
notification objects were created only if the user had subscribed to a
specific notification type. With the new inbox_view model, we are
changing this approach to create notification objects in all cases. This
PR updates the services to remove reliance on notification preferences,
simplifying the logic.


----

Caveat: Since we don’t create extra notifications for new messages if a
mention_notification already exists, a user subscribed to
new_message_notification push/email notifications but not to
mention_notification will not receive notifications in these cases.
This commit is contained in:
Sojan Jose
2025-01-13 11:20:31 +05:30
committed by GitHub
parent 97235a4365
commit c75ed1ef69
3 changed files with 96 additions and 75 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
app/services/messages/new_message_notification_service.rb
View File

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

45
spec/listeners/notification_listener_spec.rb
View File

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

81
spec/services/messages/new_message_notification_service_spec.rb
View File

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