diff --git a/app/builders/notification_builder.rb b/app/builders/notification_builder.rb index 6d2096233..8efe44fd6 100644 --- a/app/builders/notification_builder.rb +++ b/app/builders/notification_builder.rb @@ -1,5 +1,5 @@ class NotificationBuilder - pattr_initialize [:notification_type!, :user!, :account!, :primary_actor!] + pattr_initialize [:notification_type!, :user!, :account!, :primary_actor!, :secondary_actor] def perform return unless user_subscribed_to_notification? @@ -9,7 +9,7 @@ class NotificationBuilder private - def secondary_actor + def current_user Current.user end @@ -29,7 +29,8 @@ class NotificationBuilder notification_type: notification_type, account: account, primary_actor: primary_actor, - secondary_actor: secondary_actor + # secondary_actor is secondary_actor if present, else current_user + secondary_actor: secondary_actor || current_user ) end end diff --git a/app/jobs/migration/remove_message_notifications.rb b/app/jobs/migration/remove_message_notifications.rb new file mode 100644 index 000000000..df4de54c5 --- /dev/null +++ b/app/jobs/migration/remove_message_notifications.rb @@ -0,0 +1,8 @@ +# Delete migration and spec after 2 consecutive releases. +class Migration::RemoveMessageNotifications < ApplicationJob + queue_as :scheduled_jobs + + def perform + Notification.where(primary_actor_type: 'Message').in_batches(of: 100).each_record(&:destroy) + end +end diff --git a/app/models/notification.rb b/app/models/notification.rb index de6d6790c..1c5c42780 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -68,17 +68,10 @@ class Notification < ApplicationRecord end def primary_actor_data - if %w[assigned_conversation_new_message conversation_mention].include? notification_type - { - id: primary_actor.conversation.push_event_data[:id], - meta: primary_actor.conversation.push_event_data[:meta] - } - else - { - id: primary_actor.push_event_data[:id], - meta: primary_actor.push_event_data[:meta] - } - end + { + id: primary_actor.push_event_data[:id], + meta: primary_actor.push_event_data[:meta] + } end def fcm_push_data @@ -92,37 +85,33 @@ class Notification < ApplicationRecord end # TODO: move to a data presenter - # rubocop:disable Metrics/CyclomaticComplexity def push_message_title case notification_type when 'conversation_creation' - I18n.t('notifications.notification_title.conversation_creation', display_id: primary_actor.display_id, inbox_name: primary_actor.inbox.name) + I18n.t('notifications.notification_title.conversation_creation', display_id: conversation.display_id, inbox_name: primary_actor.inbox.name) when 'conversation_assignment' - I18n.t('notifications.notification_title.conversation_assignment', display_id: primary_actor.display_id) + I18n.t('notifications.notification_title.conversation_assignment', display_id: conversation.display_id) when 'assigned_conversation_new_message', 'participating_conversation_new_message' I18n.t( 'notifications.notification_title.assigned_conversation_new_message', display_id: conversation.display_id, - content: transform_user_mention_content(primary_actor&.content&.truncate_words(10)) + content: content ) when 'conversation_mention' - "[##{conversation&.display_id}] #{transform_user_mention_content primary_actor&.content}" + "[##{conversation&.display_id}] #{transform_user_mention_content content}" else '' end end - # rubocop:enable Metrics/CyclomaticComplexity def conversation - return primary_actor.conversation if %w[ - assigned_conversation_new_message - participating_conversation_new_message - conversation_mention - ].include? notification_type - primary_actor end + def content + transform_user_mention_content(secondary_actor&.content&.truncate_words(10) || '') + end + private def process_notification_delivery diff --git a/app/services/messages/mention_service.rb b/app/services/messages/mention_service.rb index ec7709ae1..3f4392ec1 100644 --- a/app/services/messages/mention_service.rb +++ b/app/services/messages/mention_service.rb @@ -35,7 +35,8 @@ class Messages::MentionService notification_type: 'conversation_mention', user: User.find(user_id), account: message.account, - primary_actor: message + primary_actor: message.conversation, + secondary_actor: message ).perform end end diff --git a/app/services/messages/new_message_notification_service.rb b/app/services/messages/new_message_notification_service.rb index f292ef19a..49d0d76b7 100644 --- a/app/services/messages/new_message_notification_service.rb +++ b/app/services/messages/new_message_notification_service.rb @@ -21,7 +21,8 @@ class Messages::NewMessageNotificationService notification_type: 'participating_conversation_new_message', user: participant, account: account, - primary_actor: message + primary_actor: message.conversation, + secondary_actor: message ).perform end end @@ -35,7 +36,8 @@ class Messages::NewMessageNotificationService notification_type: 'assigned_conversation_new_message', user: conversation.assignee, account: account, - primary_actor: message + primary_actor: message.conversation, + secondary_actor: message ).perform end diff --git a/app/views/api/v1/accounts/notifications/index.json.jbuilder b/app/views/api/v1/accounts/notifications/index.json.jbuilder index 4e881fe1a..e04ce22cf 100644 --- a/app/views/api/v1/accounts/notifications/index.json.jbuilder +++ b/app/views/api/v1/accounts/notifications/index.json.jbuilder @@ -11,19 +11,9 @@ json.data do json.notification_type notification.notification_type json.push_message_title notification.push_message_title # TODO: front end assumes primary actor to be conversation. should fix in future - if %w[ - assigned_conversation_new_message - participating_conversation_new_message - conversation_mention - ].include? notification.notification_type - json.primary_actor_type 'Conversation' - json.primary_actor_id notification.conversation.id - json.primary_actor notification.conversation.push_event_data - else - json.primary_actor_type notification.primary_actor_type - json.primary_actor_id notification.primary_actor_id - json.primary_actor notification.primary_actor.push_event_data - end + json.primary_actor_type notification.primary_actor_type + json.primary_actor_id notification.primary_actor_id + json.primary_actor notification.primary_actor.push_event_data json.read_at notification.read_at # Secondary actor could be nil for cases like system assigning conversation json.secondary_actor notification.secondary_actor&.push_event_data diff --git a/db/migrate/20231201014644_remove_notifications_with_message_primary_actor.rb b/db/migrate/20231201014644_remove_notifications_with_message_primary_actor.rb new file mode 100644 index 000000000..d0698a8cb --- /dev/null +++ b/db/migrate/20231201014644_remove_notifications_with_message_primary_actor.rb @@ -0,0 +1,5 @@ +class RemoveNotificationsWithMessagePrimaryActor < ActiveRecord::Migration[7.0] + def change + Migration::RemoveMessageNotifications.perform_later + end +end diff --git a/db/schema.rb b/db/schema.rb index 0e4ba980c..fe58aba72 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_11_29_091149) do +ActiveRecord::Schema[7.0].define(version: 2023_12_01_014644) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "pg_trgm" diff --git a/spec/jobs/migration/remove_message_notifications_spec.rb b/spec/jobs/migration/remove_message_notifications_spec.rb new file mode 100644 index 000000000..9c90e1160 --- /dev/null +++ b/spec/jobs/migration/remove_message_notifications_spec.rb @@ -0,0 +1,10 @@ +require 'rails_helper' + +RSpec.describe Migration::RemoveMessageNotifications do + subject(:job) { described_class.perform_later } + + it 'enqueues the job' do + expect { job }.to have_enqueued_job(described_class) + .on_queue('scheduled_jobs') + end +end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 00de30cfb..5c81be123 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -24,7 +24,6 @@ RSpec.describe Notification do context 'when push_title is called' do it 'returns appropriate title suited for the notification type conversation_creation' do notification = create(:notification, notification_type: 'conversation_creation') - expect(notification.push_message_title).to eq "[New conversation] - ##{notification.primary_actor.display_id} has\ been created in #{notification.primary_actor.inbox.name}" end @@ -37,7 +36,8 @@ RSpec.describe Notification do it 'returns appropriate title suited for the notification type assigned_conversation_new_message' do message = create(:message, sender: create(:user), content: Faker::Lorem.paragraphs(number: 2)) - notification = create(:notification, notification_type: 'assigned_conversation_new_message', primary_actor: message) + notification = create(:notification, notification_type: 'assigned_conversation_new_message', primary_actor: message.conversation, + secondary_actor: message) expect(notification.push_message_title).to eq "[New message] - ##{notification.conversation.display_id} \ #{message.content.truncate_words(10)}" @@ -46,14 +46,16 @@ RSpec.describe Notification do it 'returns appropriate title suited for the notification type assigned_conversation_new_message when attachment message' do # checking content nil should be suffice for attachments message = create(:message, sender: create(:user), content: nil) - notification = create(:notification, notification_type: 'assigned_conversation_new_message', primary_actor: message) + notification = create(:notification, notification_type: 'assigned_conversation_new_message', primary_actor: message.conversation, + secondary_actor: message) expect(notification.push_message_title).to eq "[New message] - ##{notification.conversation.display_id} " end it 'returns appropriate title suited for the notification type participating_conversation_new_message' do message = create(:message, sender: create(:user), content: Faker::Lorem.paragraphs(number: 2)) - notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message) + notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message.conversation, + secondary_actor: message) expect(notification.push_message_title).to eq "[New message] - ##{notification.conversation.display_id} \ #{message.content.truncate_words(10)}" @@ -61,17 +63,16 @@ RSpec.describe Notification do it 'returns appropriate title suited for the notification type participating_conversation_new_message having mention' do message = create(:message, sender: create(:user), content: 'Hey [@John](mention://user/1/john), can you check this ticket?') - notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message, - secondary_actor: message.sender) - + notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message.conversation, + secondary_actor: message) expect(notification.push_message_title).to eq "[New message] - ##{notification.conversation.display_id} Hey @John, can you check this ticket?" end it 'returns appropriate title suited for the notification type participating_conversation_new_message having multple mention' do message = create(:message, sender: create(:user), content: 'Hey [@John](mention://user/1/john), [@Alisha Peter](mention://user/2/alisha) can you check this ticket?') - notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message, - secondary_actor: message.sender) + notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message.conversation, + secondary_actor: message) expect(notification.push_message_title).to eq "[New message] - ##{notification.conversation.display_id} \ Hey @John, @Alisha Peter can you check this ticket?" @@ -79,15 +80,15 @@ Hey @John, @Alisha Peter can you check this ticket?" it 'returns appropriate title suited for the notification type participating_conversation_new_message if username contains white space' do message = create(:message, sender: create(:user), content: 'Hey [@John Peter](mention://user/1/john%20K) please check this?') - notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message, - secondary_actor: message.sender) + notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message.conversation, + secondary_actor: message) expect(notification.push_message_title).to eq "[New message] - ##{notification.conversation.display_id} Hey @John Peter please check this?" end it 'returns appropriate title suited for the notification type conversation_mention' do message = create(:message, sender: create(:user), content: 'Hey [@John](mention://user/1/john), can you check this ticket?') - notification = create(:notification, notification_type: 'conversation_mention', primary_actor: message, secondary_actor: message.sender) + notification = create(:notification, notification_type: 'conversation_mention', primary_actor: message.conversation, secondary_actor: message) expect(notification.push_message_title).to eq "[##{message.conversation.display_id}] Hey @John, can you check this ticket?" end @@ -98,7 +99,7 @@ Hey @John, @Alisha Peter can you check this ticket?" create(:user), content: 'Hey [@John](mention://user/1/john), [@Alisha Peter](mention://user/2/alisha) can you check this ticket?' ) - notification = create(:notification, notification_type: 'conversation_mention', primary_actor: message, secondary_actor: message.sender) + notification = create(:notification, notification_type: 'conversation_mention', primary_actor: message.conversation, secondary_actor: message) expect(notification.push_message_title).to eq "[##{message.conversation.display_id}] Hey @John, @Alisha Peter can you check this ticket?" end @@ -109,8 +110,7 @@ Hey @John, @Alisha Peter can you check this ticket?" create(:user), content: 'Hey [@John Peter](mention://user/1/john%20K) please check this?' ) - notification = create(:notification, notification_type: 'conversation_mention', primary_actor: message, secondary_actor: message.sender) - + notification = create(:notification, notification_type: 'conversation_mention', primary_actor: message.conversation, secondary_actor: message) expect(notification.push_message_title).to eq "[##{message.conversation.display_id}] Hey @John Peter please check this?" end end @@ -125,11 +125,10 @@ Hey @John, @Alisha Peter can you check this ticket?" it 'returns correct data for primary actor message' do message = create(:message, sender: create(:user), content: Faker::Lorem.paragraphs(number: 2)) - notification = create(:notification, notification_type: 'assigned_conversation_new_message', primary_actor: message) - + notification = create(:notification, notification_type: 'assigned_conversation_new_message', primary_actor: message.conversation, + secondary_actor: message) expect(notification.fcm_push_data[:primary_actor]).to eq({ - 'id' => notification.primary_actor.id, - 'conversation_id' => notification.primary_actor.conversation.display_id + 'id' => notification.primary_actor.display_id }) end end diff --git a/spec/services/messages/mention_service_spec.rb b/spec/services/messages/mention_service_spec.rb index 6090a013c..94277570b 100644 --- a/spec/services/messages/mention_service_spec.rb +++ b/spec/services/messages/mention_service_spec.rb @@ -32,7 +32,8 @@ describe Messages::MentionService do expect(NotificationBuilder).to have_received(:new).with(notification_type: 'conversation_mention', user: first_agent, account: account, - primary_actor: message) + primary_actor: message.conversation, + secondary_actor: message) end end @@ -55,11 +56,13 @@ describe Messages::MentionService do expect(NotificationBuilder).to have_received(:new).with(notification_type: 'conversation_mention', user: second_agent, account: account, - primary_actor: message) + primary_actor: message.conversation, + secondary_actor: message) expect(NotificationBuilder).to have_received(:new).with(notification_type: 'conversation_mention', user: first_agent, account: account, - primary_actor: message) + primary_actor: message.conversation, + secondary_actor: message) end it 'add the users to the participants list' do diff --git a/spec/services/messages/new_message_notification_service_spec.rb b/spec/services/messages/new_message_notification_service_spec.rb index bb35ec48a..c76453174 100644 --- a/spec/services/messages/new_message_notification_service_spec.rb +++ b/spec/services/messages/new_message_notification_service_spec.rb @@ -40,21 +40,24 @@ describe Messages::NewMessageNotificationService do expect(NotificationBuilder).to have_received(:new).with(notification_type: 'participating_conversation_new_message', user: participating_agent_2, account: account, - primary_actor: message) + primary_actor: message.conversation, + secondary_actor: message) 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) + primary_actor: message.conversation, + secondary_actor: message) 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) + primary_actor: message.conversation, + secondary_actor: message) end end @@ -69,18 +72,21 @@ describe Messages::NewMessageNotificationService do expect(NotificationBuilder).to have_received(:new).with(notification_type: 'assigned_conversation_new_message', user: assignee, account: account, - primary_actor: message) + primary_actor: message.conversation, + secondary_actor: message) 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) + 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) + primary_actor: message.conversation, + secondary_actor: message) end end @@ -97,7 +103,8 @@ describe Messages::NewMessageNotificationService do expect(NotificationBuilder).not_to have_received(:new).with(notification_type: 'assigned_conversation_new_message', user: assignee, account: account, - primary_actor: message) + primary_actor: message.conversation, + secondary_actor: message) end end @@ -112,11 +119,13 @@ describe Messages::NewMessageNotificationService do expect(NotificationBuilder).not_to have_received(:new).with(notification_type: 'participating_conversation_new_message', user: assignee, account: account, - primary_actor: message) + 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) + primary_actor: message.conversation, + secondary_actor: message) end end end