chore: Get all notification API improvments (#8549)

Co-authored-by: Sojan Jose <sojan@chatwoot.com>

app/builders/notification_builder.rb

@@ -2,8 +2,6 @@ class NotificationBuilder
   pattr_initialize [:notification_type!, :user!, :account!, :primary_actor!, :secondary_actor]
 
   def perform
-    return unless user_subscribed_to_notification?
-
     build_notification
   end
 
@@ -16,7 +14,7 @@ class NotificationBuilder
   def user_subscribed_to_notification?
     notification_setting = user.notification_settings.find_by(account_id: account.id)
     # added for the case where an assignee might be removed from the account but remains in conversation
-    return if notification_setting.blank?
+    return false if notification_setting.blank?
 
     return true if notification_setting.public_send("email_#{notification_type}?")
     return true if notification_setting.public_send("push_#{notification_type}?")
@@ -25,6 +23,9 @@ class NotificationBuilder
   end
 
   def build_notification
+    # Create conversation_creation notification only if user is subscribed to it
+    return if notification_type == 'conversation_creation' && !user_subscribed_to_notification?
+
     user.notifications.create!(
       notification_type: notification_type,
       account: account,

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

@@ -7,9 +7,9 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
   before_action :set_current_page, only: [:index]
 
   def index
-    @unread_count = current_user.notifications.where(account_id: current_account.id, read_at: nil).count
+    @unread_count = notification_finder.unread_count
-    @count = notifications.count
+    @notifications = notification_finder.perform
-    @notifications = notifications.page(@current_page).per(RESULTS_PER_PAGE)
+    @count = @notifications.count
   end
 
   def read_all
@@ -35,7 +35,7 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
   end
 
   def unread_count
-    @unread_count = current_user.notifications.where(account_id: current_account.id, read_at: nil).count
+    @unread_count = notification_finder.unread_count
     render json: @unread_count
   end
 
@@ -61,7 +61,7 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
     @current_page = params[:page] || 1
   end
 
-  def notifications
+  def notification_finder
-    @notifications ||= current_user.notifications.where(account_id: current_account.id)
+    @notification_finder ||= NotificationFinder.new(Current.user, Current.account, params)
   end
 end

app/finders/notification_finder.rb

@@ -0,0 +1,47 @@
+class NotificationFinder
+  attr_reader :current_user, :current_account, :params
+
+  RESULTS_PER_PAGE = 15
+
+  def initialize(current_user, current_account, params = {})
+    @current_user = current_user
+    @current_account = current_account
+    @params = params
+    set_up
+  end
+
+  def perform
+    notifications
+  end
+
+  def unread_count
+    @notifications.where(read_at: nil).count
+  end
+
+  def count
+    @notifications.count
+  end
+
+  private
+
+  def set_up
+    find_all_notifications
+    filter_by_status
+  end
+
+  def find_all_notifications
+    @notifications = current_user.notifications.where(account_id: @current_account.id)
+  end
+
+  def filter_by_status
+    @notifications = @notifications.where('snoozed_until > ?', DateTime.now.utc) if params[:status] == 'snoozed'
+  end
+
+  def current_page
+    params[:page] || 1
+  end
+
+  def notifications
+    @notifications.page(current_page).per(RESULTS_PER_PAGE).order(last_activity_at: :desc)
+  end
+end

app/jobs/notification/remove_duplicate_notification_job.rb

@@ -7,10 +7,11 @@ class Notification::RemoveDuplicateNotificationJob < ApplicationJob
     user_id = notification.user_id
     primary_actor_id = notification.primary_actor_id
 
-    # Find older notifications with the same user and primary_actor_id, excluding the new one
+    # Find older notifications with the same user and primary_actor_id
     duplicate_notifications = Notification.where(user_id: user_id, primary_actor_id: primary_actor_id)
-                                          .where.not(id: notification.id)
+                                          .order(created_at: :desc)
 
-    duplicate_notifications.each(&:destroy)
+    # Skip the first one (the latest notification) and destroy the rest
+    duplicate_notifications.offset(1).each(&:destroy)
   end
 end

app/listeners/base_listener.rb

@@ -8,9 +8,9 @@ class BaseListener
 
   def extract_notification_and_account(event)
     notification = event.data[:notification]
-    notifications_meta = notification.user.notifications_meta(notification.account_id)
+    notification_finder = NotificationFinder.new(notification.user, notification.account)
-    unread_count = notifications_meta[:unread_count]
+    unread_count = notification_finder.unread_count
-    count = notifications_meta[:count]
+    count = notification_finder.count
     [notification, notification.account, unread_count, count]
   end
 

app/models/notification.rb

@@ -47,10 +47,6 @@ class Notification < ApplicationRecord
   after_create_commit :process_notification_delivery, :dispatch_create_event
   after_destroy_commit :dispatch_destroy_event
 
-  # TODO: Get rid of default scope
-  # https://stackoverflow.com/a/1834250/939299
-  default_scope { order(id: :desc) }
-
   PRIMARY_ACTORS = ['Conversation'].freeze
 
   def push_event_data
@@ -122,17 +118,24 @@ class Notification < ApplicationRecord
   private
 
   def process_notification_delivery
-    Notification::PushNotificationJob.perform_later(self)
+    Notification::PushNotificationJob.perform_later(self) if user_subscribed_to_notification?('push')
 
     # Should we do something about the case where user subscribed to both push and email ?
     # In future, we could probably add condition here to enqueue the job for 30 seconds later
     # when push enabled and then check in email job whether notification has been read already.
-    Notification::EmailNotificationJob.perform_later(self)
+    Notification::EmailNotificationJob.perform_later(self) if user_subscribed_to_notification?('email')
 
-    # Remove duplicate notifications
     Notification::RemoveDuplicateNotificationJob.perform_later(self)
   end
 
+  def user_subscribed_to_notification?(delivery_type)
+    notification_setting = user.notification_settings.find_by(account_id: account.id)
+    return false if notification_setting.blank?
+
+    # Check if the user has subscribed to the specified type of notification
+    notification_setting.public_send("#{delivery_type}_#{notification_type}?")
+  end
+
   def dispatch_create_event
     Rails.configuration.dispatcher.dispatch(NOTIFICATION_CREATED, Time.zone.now, notification: self)
   end

app/models/user.rb

@@ -153,13 +153,6 @@ class User < ApplicationRecord
     mutations_from_database.changed?('email')
   end
 
-  def notifications_meta(account_id)
-    {
-      unread_count: notifications.where(account_id: account_id, read_at: nil).count,
-      count: notifications.where(account_id: account_id).count
-    }
-  end
-
   private
 
   def remove_macros

spec/builders/notification_builder_spec.rb

@@ -11,6 +11,7 @@ describe NotificationBuilder do
     before do
       notification_setting = user.notification_settings.find_by(account_id: account.id)
       notification_setting.selected_email_flags = [:email_conversation_creation]
+      notification_setting.selected_push_flags = [:push_conversation_creation]
       notification_setting.save!
     end
 
@@ -38,5 +39,37 @@ describe NotificationBuilder do
         ).perform
       ).to be_nil
     end
+
+    it 'will not create a conversation_creation notification if user is not subscribed to it' do
+      notification_setting = user.notification_settings.find_by(account_id: account.id)
+      notification_setting.selected_email_flags = []
+      notification_setting.selected_push_flags = []
+      notification_setting.save!
+
+      expect(
+        described_class.new(
+          notification_type: 'conversation_creation',
+          user: user,
+          account: account,
+          primary_actor: primary_actor
+        ).perform
+      ).to be_nil
+    end
+
+    it 'will create a conversation_mention notification eventhough user is not subscribed to it' do
+      notification_setting = user.notification_settings.find_by(account_id: account.id)
+      notification_setting.selected_email_flags = []
+      notification_setting.selected_push_flags = []
+      notification_setting.save!
+
+      expect do
+        described_class.new(
+          notification_type: 'conversation_mention',
+          user: user,
+          account: account,
+          primary_actor: primary_actor
+        ).perform
+      end.to change { user.notifications.count }.by(1)
+    end
   end
 end

spec/finders/notification_finder_spec.rb

@@ -0,0 +1,67 @@
+require 'rails_helper'
+
+describe NotificationFinder do
+  subject(:notification_finder) { described_class.new(user, account, params) }
+
+  let!(:account) { create(:account) }
+  let!(:user) { create(:user, account: account) }
+
+  before do
+    create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 1.day, last_activity_at: DateTime.now.utc + 1.day,
+                          read_at: DateTime.now.utc)
+    create(:notification, account: account, user: user, snoozed_until: DateTime.now.utc + 3.days, updated_at: DateTime.now.utc,
+                          last_activity_at: DateTime.now.utc)
+    create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 2.days, last_activity_at: DateTime.now.utc + 2.days)
+    create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 4.days, last_activity_at: DateTime.now.utc + 4.days,
+                          notification_type: :conversation_creation, read_at: DateTime.now.utc)
+    create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 5.days, last_activity_at: DateTime.now.utc + 5.days,
+                          notification_type: :conversation_mention)
+    create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 6.days, last_activity_at: DateTime.now.utc + 6.days,
+                          notification_type: :participating_conversation_new_message)
+  end
+
+  describe '#perform' do
+    context 'when params are empty' do
+      let(:params) { {} }
+
+      it 'returns all the notifications' do
+        result = notification_finder.perform
+        expect(result.length).to be 6
+      end
+
+      it 'orders notifications by last activity at' do
+        result = notification_finder.perform
+        expect(result.first.last_activity_at).to be > result.last.last_activity_at
+      end
+
+      it 'returns unread count' do
+        result = notification_finder.unread_count
+        expect(result).to be 4
+      end
+
+      it 'returns count' do
+        result = notification_finder.count
+        expect(result).to be 6
+      end
+    end
+
+    context 'when snoozed param is passed' do
+      let(:params) { { status: 'snoozed' } }
+
+      it 'returns only snoozed notifications' do
+        result = notification_finder.perform
+        expect(result.length).to be 1
+      end
+
+      it 'returns unread count' do
+        result = notification_finder.unread_count
+        expect(result).to be 1
+      end
+
+      it 'returns count' do
+        result = notification_finder.count
+        expect(result).to be 1
+      end
+    end
+  end
+end

spec/models/notification_spec.rb

@@ -16,8 +16,8 @@ RSpec.describe Notification do
       create(:notification)
       notification3 = create(:notification)
 
-      expect(described_class.all.first).to eq notification3
+      expect(described_class.all.first).to eq notification1
-      expect(described_class.all.last).to eq notification1
+      expect(described_class.all.last).to eq notification3
     end
   end