From 1612f515b0113904b1ab0fc3915d3e1451882005 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Fri, 19 Feb 2021 18:35:21 +0530 Subject: [PATCH] chore: Fix issues with notification actors (#1807) --- app/listeners/notification_listener.rb | 2 +- app/models/notification.rb | 13 ++++++------- .../v1/accounts/notifications/index.json.jbuilder | 11 ++++++----- .../20210219085719_remove_old_notifications.rb | 5 +++++ db/schema.rb | 2 +- spec/models/notification_spec.rb | 6 ++++-- 6 files changed, 23 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20210219085719_remove_old_notifications.rb diff --git a/app/listeners/notification_listener.rb b/app/listeners/notification_listener.rb index 0fe10b739..bfa2ec99f 100644 --- a/app/listeners/notification_listener.rb +++ b/app/listeners/notification_listener.rb @@ -41,7 +41,7 @@ class NotificationListener < BaseListener notification_type: 'assigned_conversation_new_message', user: conversation.assignee, account: account, - primary_actor: conversation + primary_actor: message ).perform end diff --git a/app/models/notification.rb b/app/models/notification.rb index 40b6ad765..5d559befe 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -44,12 +44,13 @@ class Notification < ApplicationRecord PRIMARY_ACTORS = ['Conversation'].freeze def push_event_data + # Secondary actor could be nil for cases like system assigning conversation { id: id, notification_type: notification_type, primary_actor_type: primary_actor_type, primary_actor_id: primary_actor_id, - primary_actor: primary_actor&.push_event_data, + primary_actor: primary_actor.push_event_data, read_at: read_at, secondary_actor: secondary_actor&.push_event_data, user: user&.push_event_data, @@ -59,7 +60,6 @@ class Notification < ApplicationRecord end # TODO: move to a data presenter - # rubocop:disable Metrics/CyclomaticComplexity def push_message_title case notification_type when 'conversation_creation' @@ -69,19 +69,18 @@ class Notification < ApplicationRecord when 'assigned_conversation_new_message' I18n.t( 'notifications.notification_title.assigned_conversation_new_message', - display_id: primary_actor.display_id, - content: primary_actor&.messages&.incoming&.last&.content + display_id: conversation.display_id, + content: primary_actor.content.truncate_words(10) ) when 'conversation_mention' - I18n.t('notifications.notification_title.conversation_mention', display_id: primary_actor.conversation.display_id, name: secondary_actor.name) + I18n.t('notifications.notification_title.conversation_mention', display_id: conversation.display_id, name: secondary_actor.name) else '' end end - # rubocop:enable Metrics/CyclomaticComplexity def conversation - return primary_actor.conversation if ['conversation_mention'].include? notification_type + return primary_actor.conversation if %w[assigned_conversation_new_message conversation_mention].include? notification_type primary_actor end diff --git a/app/views/api/v1/accounts/notifications/index.json.jbuilder b/app/views/api/v1/accounts/notifications/index.json.jbuilder index 9180fe512..e715d5ada 100644 --- a/app/views/api/v1/accounts/notifications/index.json.jbuilder +++ b/app/views/api/v1/accounts/notifications/index.json.jbuilder @@ -11,18 +11,19 @@ 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 notification.notification_type == 'conversation_mention' + if %w[assigned_conversation_new_message conversation_mention].include? notification.notification_type json.primary_actor_type 'Conversation' - json.primary_actor_id notification.primary_actor.conversation_id - json.primary_actor notification.primary_actor&.conversation&.push_event_data + 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 + 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 - json.user notification.user&.push_event_data + json.user notification.user.push_event_data json.created_at notification.created_at.to_i end end diff --git a/db/migrate/20210219085719_remove_old_notifications.rb b/db/migrate/20210219085719_remove_old_notifications.rb new file mode 100644 index 000000000..06021a341 --- /dev/null +++ b/db/migrate/20210219085719_remove_old_notifications.rb @@ -0,0 +1,5 @@ +class RemoveOldNotifications < ActiveRecord::Migration[6.0] + def change + Notification.where(notification_type: 'assigned_conversation_new_message').destroy_all + end +end diff --git a/db/schema.rb b/db/schema.rb index a9bf11747..bf0f57714 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.define(version: 2021_02_17_154129) do +ActiveRecord::Schema.define(version: 2021_02_19_085719) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 229cc12e0..0978bab4f 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -34,9 +34,11 @@ RSpec.describe Notification do end it 'returns appropriate title suited for the notification type assigned_conversation_new_message' do - notification = create(:notification, notification_type: 'assigned_conversation_new_message') + message = create(:message, sender: create(:user), content: Faker::Lorem.paragraphs(number: 2)) + notification = create(:notification, notification_type: 'assigned_conversation_new_message', primary_actor: message) - expect(notification.push_message_title).to eq "[New message] - ##{notification.primary_actor.display_id} " + expect(notification.push_message_title).to eq "[New message] - ##{notification.conversation.display_id} \ +#{message.content.truncate_words(10)}" end it 'returns appropriate title suited for the notification type conversation_mention' do