chore: Change primary actor to Conversation for all the notification types. (#8435)

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

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

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.push_event_data[:id],
-        id: primary_actor.conversation.push_event_data[:id],
+      meta: primary_actor.push_event_data[:meta]
-        meta: primary_actor.conversation.push_event_data[:meta]
+    }
-      }
-    else
-      {
-        id: primary_actor.push_event_data[:id],
-        meta: primary_actor.push_event_data[:meta]
-      }
-    end
   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

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

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
 

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[
+      json.primary_actor_type notification.primary_actor_type
-        assigned_conversation_new_message
+      json.primary_actor_id notification.primary_actor_id
-        participating_conversation_new_message
+      json.primary_actor notification.primary_actor.push_event_data
-        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.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

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

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"

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

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,
+      notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message.conversation,
-                                           secondary_actor: message.sender)
+                                           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,
+      notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message.conversation,
-                                           secondary_actor: message.sender)
+                                           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,
+      notification = create(:notification, notification_type: 'participating_conversation_new_message', primary_actor: message.conversation,
-                                           secondary_actor: message.sender)
+                                           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,
+                                                                 'id' => notification.primary_actor.display_id
-                                                                 'conversation_id' => notification.primary_actor.conversation.display_id
                                                                })
     end
   end

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

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