From b0bbd757b519a915a83edf0177cf86ec6f3137e3 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Mon, 15 Jun 2020 13:36:56 +0530 Subject: [PATCH] Chore: FCM Notification Improvements (#957) Co-authored-by: Pranav Raj Sreepuram --- .../v1/accounts/notifications_controller.rb | 16 ++++++++++++- app/models/notification.rb | 5 ++++ .../notification/push_notification_service.rb | 13 +++++++---- .../notifications/index.json.jbuilder | 2 ++ .../accounts/notifications_controller_spec.rb | 23 ++++++++++++++++--- spec/models/notification_spec.rb | 21 +++++++++++++++++ 6 files changed, 71 insertions(+), 9 deletions(-) create mode 100644 spec/models/notification_spec.rb diff --git a/app/controllers/api/v1/accounts/notifications_controller.rb b/app/controllers/api/v1/accounts/notifications_controller.rb index 1ec27ba29..8ae1629ca 100644 --- a/app/controllers/api/v1/accounts/notifications_controller.rb +++ b/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 diff --git a/app/models/notification.rb b/app/models/notification.rb index 660bcc33c..33c04dda2 100644 --- a/app/models/notification.rb +++ b/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, diff --git a/app/services/notification/push_notification_service.rb b/app/services/notification/push_notification_service.rb index fadfddf53..275e10c2b 100644 --- a/app/services/notification/push_notification_service.rb +++ b/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 diff --git a/app/views/api/v1/accounts/notifications/index.json.jbuilder b/app/views/api/v1/accounts/notifications/index.json.jbuilder index 048c3d85c..b69d869e6 100644 --- a/app/views/api/v1/accounts/notifications/index.json.jbuilder +++ b/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 diff --git a/spec/controllers/api/v1/accounts/notifications_controller_spec.rb b/spec/controllers/api/v1/accounts/notifications_controller_spec.rb index 594a198b6..8f61cd803 100644 --- a/spec/controllers/api/v1/accounts/notifications_controller_spec.rb +++ b/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 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb new file mode 100644 index 000000000..7920df919 --- /dev/null +++ b/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