diff --git a/app/builders/notification_builder.rb b/app/builders/notification_builder.rb index 8efe44fd6..3d8ce9674 100644 --- a/app/builders/notification_builder.rb +++ b/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, diff --git a/app/controllers/api/v1/accounts/notifications_controller.rb b/app/controllers/api/v1/accounts/notifications_controller.rb index 0d8cf6a47..9cea3fbb4 100644 --- a/app/controllers/api/v1/accounts/notifications_controller.rb +++ b/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 - @count = notifications.count - @notifications = notifications.page(@current_page).per(RESULTS_PER_PAGE) + @unread_count = notification_finder.unread_count + @notifications = notification_finder.perform + @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 - @notifications ||= current_user.notifications.where(account_id: current_account.id) + def notification_finder + @notification_finder ||= NotificationFinder.new(Current.user, Current.account, params) end end diff --git a/app/finders/notification_finder.rb b/app/finders/notification_finder.rb new file mode 100644 index 000000000..7559d6ef9 --- /dev/null +++ b/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 diff --git a/app/jobs/notification/remove_duplicate_notification_job.rb b/app/jobs/notification/remove_duplicate_notification_job.rb index 6baa87460..66c98393d 100644 --- a/app/jobs/notification/remove_duplicate_notification_job.rb +++ b/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 diff --git a/app/listeners/base_listener.rb b/app/listeners/base_listener.rb index acfbc4b18..c0cec45f5 100644 --- a/app/listeners/base_listener.rb +++ b/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) - unread_count = notifications_meta[:unread_count] - count = notifications_meta[:count] + notification_finder = NotificationFinder.new(notification.user, notification.account) + unread_count = notification_finder.unread_count + count = notification_finder.count [notification, notification.account, unread_count, count] end diff --git a/app/models/notification.rb b/app/models/notification.rb index fdc8e287c..db8fbb8ea 100644 --- a/app/models/notification.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 6ff4fb61d..25ae2826a 100644 --- a/app/models/user.rb +++ b/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 diff --git a/spec/builders/notification_builder_spec.rb b/spec/builders/notification_builder_spec.rb index 8386dbf9a..e86e01d17 100644 --- a/spec/builders/notification_builder_spec.rb +++ b/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 diff --git a/spec/finders/notification_finder_spec.rb b/spec/finders/notification_finder_spec.rb new file mode 100644 index 000000000..294cef7ff --- /dev/null +++ b/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 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 31d127844..cb3679290 100644 --- a/spec/models/notification_spec.rb +++ b/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.last).to eq notification1 + expect(described_class.all.first).to eq notification1 + expect(described_class.all.last).to eq notification3 end end