Chore: FCM Notification Improvements (#957)

Co-authored-by: Pranav Raj Sreepuram <pranavrajs@gmail.com>

app/controllers/api/v1/accounts/notifications_controller.rb

@@ -2,6 +2,7 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
   protect_from_forgery with: :null_session
 
   before_action :fetch_notification, only: [:update]
+  before_action :set_primary_actor, only: [:read_all]
 
   def index
     @unread_count = current_user.notifications.where(account_id: current_account.id, read_at: nil).count
@@ -9,7 +10,13 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
   end
 
   def read_all
-    current_user.notifications.where(account_id: current_account.id, read_at: nil).update(read_at: DateTime.now.utc)
+    if @primary_actor
+      current_user.notifications.where(account_id: current_account.id, primary_actor: @primary_actor, read_at: nil)
+                  .update(read_at: DateTime.now.utc)
+    else
+      current_user.notifications.where(account_id: current_account.id, read_at: nil).update(read_at: DateTime.now.utc)
+    end
+
     head :ok
   end
 
@@ -20,6 +27,13 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
 
   private
 
+  def set_primary_actor
+    return unless params[:primary_actor_type]
+    return unless Notification::PRIMARY_ACTORS.include?(params[:primary_actor_type])
+
+    @primary_actor = params[:primary_actor_type].safe_constantize.find_by(id: params[:primary_actor_id])
+  end
+
   def fetch_notification
     @notification = current_user.notifications.find(params[:id])
   end

app/models/notification.rb

@@ -37,11 +37,16 @@ class Notification < ApplicationRecord
   enum notification_type: NOTIFICATION_TYPES
 
   after_create_commit :process_notification_delivery
+  default_scope { order(id: :desc) }
+
+  PRIMARY_ACTORS = ['Conversation'].freeze
 
   def push_event_data
     {
       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,
       read_at: read_at,
       secondary_actor: secondary_actor&.push_event_data,

app/services/notification/push_notification_service.rb

@@ -66,15 +66,18 @@ class Notification::PushNotificationService
     return unless subscription.fcm?
 
     fcm = FCM.new(ENV['FCM_SERVER_KEY'])
-    options = {
+    response = fcm.send([subscription.subscription_attributes['push_token']], fcm_options)
+    subscription.destroy! if JSON.parse(response[:body])['results']&.first&.keys&.include?('error')
+  end
+
+  def fcm_options
+    {
       "notification": {
         "title": notification.notification_type.titleize,
         "body": notification.push_message_title
       },
-      "data": { notification: notification.push_event_data.to_json }
+      "data": { notification: notification.push_event_data.to_json },
+      "collapse_key": "chatwoot_#{notification.primary_actor_type.downcase}_#{notification.primary_actor_id}"
     }
-
-    response = fcm.send([subscription.subscription_attributes['push_token']], options)
-    subscription.destroy! if JSON.parse(response[:body])['results']&.first&.keys&.include?('error')
   end
 end

app/views/api/v1/accounts/notifications/index.json.jbuilder

@@ -8,6 +8,8 @@ json.data do
       json.id notification.id
       json.notification_type notification.notification_type
       json.push_message_title notification.push_message_title
+      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
       json.secondary_actor notification.secondary_actor&.push_event_data

spec/controllers/api/v1/accounts/notifications_controller_spec.rb

@@ -14,7 +14,8 @@ RSpec.describe 'Notifications API', type: :request do
 
     context 'when it is an authenticated user' do
       let(:admin) { create(:user, account: account, role: :administrator) }
-      let!(:notification) { create(:notification, account: account, user: admin) }
+      let!(:notification1) { create(:notification, account: account, user: admin) }
+      let!(:notification2) { create(:notification, account: account, user: admin) }
 
       it 'returns all notifications' do
         get "/api/v1/accounts/#{account.id}/notifications",
@@ -23,8 +24,10 @@ RSpec.describe 'Notifications API', type: :request do
 
         response_json = JSON.parse(response.body)
         expect(response).to have_http_status(:success)
-        expect(response.body).to include(notification.notification_type)
-        expect(response_json['data']['meta']['unread_count']).to eq 1
+        expect(response.body).to include(notification1.notification_type)
+        expect(response_json['data']['meta']['unread_count']).to eq 2
+        # notification appear in descending order
+        expect(response_json['data']['payload'].first['id']).to eq notification2.id
       end
     end
   end
@@ -54,6 +57,20 @@ RSpec.describe 'Notifications API', type: :request do
         expect(notification1.reload.read_at).not_to eq('')
         expect(notification2.reload.read_at).not_to eq('')
       end
+
+      it 'updates only the notifications read at for primary actor when param is passed' do
+        post "/api/v1/accounts/#{account.id}/notifications/read_all",
+             headers: admin.create_new_auth_token,
+             params: {
+               primary_actor_id: notification1.primary_actor_id,
+               primary_actor_type: notification1.primary_actor_type
+             },
+             as: :json
+
+        expect(response).to have_http_status(:success)
+        expect(notification1.reload.read_at).not_to eq('')
+        expect(notification2.reload.read_at).to eq nil
+      end
     end
   end
 

spec/models/notification_spec.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Notification do
+  context 'with associations' do
+    it { is_expected.to belong_to(:account) }
+    it { is_expected.to belong_to(:user) }
+  end
+
+  context 'with default order by' do
+    it 'sort by primary id desc' do
+      notification1 = create(:notification)
+      create(:notification)
+      notification3 = create(:notification)
+
+      expect(described_class.all.first).to eq notification3
+      expect(described_class.all.last).to eq notification1
+    end
+  end
+end