DEV: handle all notification consolidations in new 'NotificationConsolidator' class.

481c8314f0b79253578c0f7facbe91f792301411

app/models/notification.rb

@@ -5,7 +5,6 @@ class Notification < ActiveRecord::Base
   belongs_to :topic
 
   MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS = 24
-  MEMBERSHIP_REQUEST_CONSOLIDATION_THRESHOLD = 3
 
   validates_presence_of :data
   validates_presence_of :notification_type
@@ -15,10 +14,25 @@ class Notification < ActiveRecord::Base
   scope :visible , lambda { joins('LEFT JOIN topics ON notifications.topic_id = topics.id')
     .where('topics.id IS NULL OR topics.deleted_at IS NULL') }
 
-  scope :filter_by_display_username_and_type, ->(username, notification_type) {
+  scope :filter_by_consolidation_data, ->(notification_type, data) {
-    where("data::json ->> 'display_username' = ?", username)
+    notifications = where(notification_type: notification_type)
-      .where(notification_type: notification_type)
+
-      .order(created_at: :desc)
+    case notification_type
+    when types[:liked], types[:liked_consolidated]
+      key = "display_username"
+      consolidation_window = SiteSetting.likes_notification_consolidation_window_mins.minutes.ago
+    when types[:private_message]
+      key = "topic_title"
+      consolidation_window = MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours.ago
+    when types[:membership_request_consolidated]
+      key = "group_name"
+      consolidation_window = MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours.ago
+    end
+
+    notifications = notifications.where("created_at > ? AND data::json ->> '#{key}' = ?", consolidation_window, data[key.to_sym]) if data[key&.to_sym].present?
+    notifications = notifications.where("data::json ->> 'username2' IS NULL") if notification_type == types[:liked]
+
+    notifications
   }
 
   attr_accessor :skip_send_email
@@ -27,7 +41,7 @@ class Notification < ActiveRecord::Base
 
   after_commit(on: :create) do
     DiscourseEvent.trigger(:notification_created, self)
-    send_email unless consolidate_membership_requests
+    send_email unless NotificationConsolidator.new(self).consolidate!
   end
 
   def self.ensure_consistency!
@@ -230,70 +244,6 @@ class Notification < ActiveRecord::Base
     NotificationEmailer.process_notification(self) if !skip_send_email
   end
 
-  private
-
-  def consolidate_membership_requests
-    return unless unread_pm?
-
-    post_id = data_hash[:original_post_id]
-    return if post_id.blank?
-
-    custom_field = PostCustomField.select(:value).find_by(post_id: post_id, name: "requested_group_id")
-    return if custom_field.blank?
-
-    group_id = custom_field.value.to_i
-    group_name = Group.select(:name).find_by(id: group_id)&.name
-    return if group_name.blank?
-
-    consolidation_window = MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours.ago
-    timestamp = Time.zone.now
-    unread = user.notifications.unread
-
-    consolidated_notification = unread
-      .where("created_at > ? AND data::json ->> 'group_name' = ?", consolidation_window, group_name)
-      .find_by(notification_type: Notification.types[:membership_request_consolidated])
-
-    if consolidated_notification.present?
-      data = consolidated_notification.data_hash
-      data["count"] += 1
-
-      Notification.transaction do
-        consolidated_notification.update!(
-          data: data.to_json,
-          read: false,
-          updated_at: timestamp
-        )
-
-        destroy!
-      end
-
-      return true
-    end
-
-    notifications = unread
-      .where(notification_type: Notification.types[:private_message])
-      .where("created_at > ? AND data::json ->> 'topic_title' = ?", consolidation_window, data_hash[:topic_title])
-
-    return if notifications.count < MEMBERSHIP_REQUEST_CONSOLIDATION_THRESHOLD
-
-    Notification.transaction do
-      Notification.create!(
-        notification_type: Notification.types[:membership_request_consolidated],
-        user_id: user_id,
-        data: {
-          group_name: group_name,
-          count: notifications.count
-        }.to_json,
-        updated_at: timestamp,
-        created_at: timestamp
-      )
-
-      notifications.destroy_all
-    end
-
-    true
-  end
-
 end
 
 # == Schema Information

app/services/notification_consolidator.rb

@@ -0,0 +1,88 @@
+# frozen_string_literal: true
+
+class NotificationConsolidator
+  attr_reader :notification, :notification_type, :consolidation_type, :data
+
+  def initialize(notification)
+    @notification = notification
+    @notification_type = notification.notification_type
+    @data = notification.data_hash
+
+    if notification_type == Notification.types[:liked]
+      @consolidation_type = Notification.types[:liked_consolidated]
+      @data[:username] = @data[:display_username]
+    elsif notification_type == Notification.types[:private_message]
+      post_id = @data[:original_post_id]
+      return if post_id.blank?
+
+      custom_field = PostCustomField.select(:value).find_by(post_id: post_id, name: "requested_group_id")
+      return if custom_field.blank?
+
+      group_id = custom_field.value.to_i
+      group_name = Group.select(:name).find_by(id: group_id)&.name
+      return if group_name.blank?
+
+      @consolidation_type = Notification.types[:membership_request_consolidated]
+      @data[:group_name] = group_name
+    end
+  end
+
+  def consolidate!
+    return if SiteSetting.notification_consolidation_threshold.zero? || consolidation_type.blank?
+
+    update_consolidated_notification! || create_consolidated_notification!
+  end
+
+  def update_consolidated_notification!
+    consolidated_notification = user_notifications.filter_by_consolidation_data(consolidation_type, data).first
+    return if consolidated_notification.blank?
+
+    data_hash = consolidated_notification.data_hash
+    data_hash["count"] += 1
+
+    Notification.transaction do
+      consolidated_notification.update!(
+          data: data_hash.to_json,
+          read: false,
+          updated_at: timestamp
+      )
+      notification.destroy!
+    end
+
+    consolidated_notification
+  end
+
+  def create_consolidated_notification!
+    notifications = user_notifications.unread.filter_by_consolidation_data(notification_type, data)
+    return if notifications.count <= SiteSetting.notification_consolidation_threshold
+
+    consolidated_notification = nil
+
+    Notification.transaction do
+      timestamp = notifications.last.created_at
+      data[:count] = notifications.count
+
+      consolidated_notification = Notification.create!(
+        notification_type: consolidation_type,
+        user_id: notification.user_id,
+        data: data.to_json,
+        updated_at: timestamp,
+        created_at: timestamp
+      )
+
+      notifications.destroy_all
+    end
+
+    consolidated_notification
+  end
+
+  private
+
+  def user_notifications
+    notification.user.notifications
+  end
+
+  def timestamp
+    @timestamp ||= Time.zone.now
+  end
+end

app/services/post_alerter.rb

@@ -331,28 +331,19 @@ class PostAlerter
 
     notification_data = {}
 
-    if is_liked
+    if is_liked &&
-      if existing_notification_of_same_type &&
+      existing_notification_of_same_type &&
-        existing_notification_of_same_type.created_at > 1.day.ago &&
+      existing_notification_of_same_type.created_at > 1.day.ago &&
-        (
+      (
-          user.user_option.like_notification_frequency ==
+        user.user_option.like_notification_frequency ==
-          UserOption.like_notification_frequency_type[:always]
+        UserOption.like_notification_frequency_type[:always]
-        )
+      )
 
-        data = existing_notification_of_same_type.data_hash
+      data = existing_notification_of_same_type.data_hash
-        notification_data["username2"] = data["display_username"]
+      notification_data["username2"] = data["display_username"]
-        notification_data["count"] = (data["count"] || 1).to_i + 1
+      notification_data["count"] = (data["count"] || 1).to_i + 1
-        # don't use destroy so we don't trigger a notification count refresh
+      # don't use destroy so we don't trigger a notification count refresh
-        Notification.where(id: existing_notification_of_same_type.id).destroy_all
+      Notification.where(id: existing_notification_of_same_type.id).destroy_all
-      elsif !SiteSetting.likes_notification_consolidation_threshold.zero?
-        notification = consolidate_liked_notifications(
-          user,
-          post,
-          opts[:display_username]
-        )
-
-        return notification if notification
-      end
     end
 
     collapsed = false
@@ -625,82 +616,4 @@ class PostAlerter
   def warn_if_not_sidekiq
     Rails.logger.warn("PostAlerter.#{caller_locations(1, 1)[0].label} was called outside of sidekiq") unless Sidekiq.server?
   end
-
-  private
-
-  def consolidate_liked_notifications(user, post, username)
-    user_notifications = user.notifications
-
-    consolidation_window =
-      SiteSetting.likes_notification_consolidation_window_mins.minutes.ago
-
-    liked_by_user_notifications =
-      user_notifications
-        .filter_by_display_username_and_type(
-          username, Notification.types[:liked]
-        )
-        .where(
-          "created_at > ? AND data::json ->> 'username2' IS NULL",
-          consolidation_window
-        )
-
-    user_liked_consolidated_notification =
-      user_notifications
-        .filter_by_display_username_and_type(
-          username, Notification.types[:liked_consolidated]
-        )
-        .where("created_at > ?", consolidation_window)
-        .first
-
-    if user_liked_consolidated_notification
-      return update_consolidated_liked_notification_count!(
-        user_liked_consolidated_notification
-      )
-    elsif (
-      liked_by_user_notifications.count >=
-      SiteSetting.likes_notification_consolidation_threshold
-    )
-      return create_consolidated_liked_notification!(
-        liked_by_user_notifications,
-        post,
-        username
-      )
-    end
-  end
-
-  def update_consolidated_liked_notification_count!(notification)
-    data = notification.data_hash
-    data["count"] += 1
-
-    notification.update!(
-      data: data.to_json,
-      read: false
-    )
-
-    notification
-  end
-
-  def create_consolidated_liked_notification!(notifications, post, username)
-    notification = nil
-
-    Notification.transaction do
-      timestamp = notifications.last.created_at
-
-      notification = Notification.create!(
-        notification_type: Notification.types[:liked_consolidated],
-        user_id: post.user_id,
-        data: {
-          username: username,
-          display_username: username,
-          count: notifications.count + 1
-        }.to_json,
-        updated_at: timestamp,
-        created_at: timestamp
-      )
-
-      notifications.each(&:destroy!)
-    end
-
-    notification
-  end
 end

config/locales/server.en.yml

@@ -1988,9 +1988,9 @@ en:
 
     disable_system_edit_notifications: "Disables edit notifications by the system user when 'download_remote_images_to_local' is active."
 
-    likes_notification_consolidation_threshold: "Number of liked notifications received before the notifications are consolidated into a single one. Set to 0 to disable. The window can be configured via `SiteSetting.likes_notification_consolidation_window_mins`."
+    notification_consolidation_threshold: "Number of liked or membership request notifications received before the notifications are consolidated into a single one. Set to 0 to disable."
 
-    likes_notification_consolidation_window_mins: "Duration in minutes where liked notifications are consolidated into a single notification once the threshold has been reached. The threshold can be configured via `SiteSetting.likes_notification_consolidation_threshold`."
+    likes_notification_consolidation_window_mins: "Duration in minutes where liked notifications are consolidated into a single notification once the threshold has been reached. The threshold can be configured via `SiteSetting.notification_consolidation_threshold`."
 
     automatically_unpin_topics: "Automatically unpin topics when the user reaches the bottom."
 

config/site_settings.yml

@@ -1857,7 +1857,7 @@ uncategorized:
 
   disable_system_edit_notifications: true
 
-  likes_notification_consolidation_threshold:
+  notification_consolidation_threshold:
     default: 3
     min: 0
 

spec/models/notification_spec.rb

@@ -258,7 +258,7 @@ describe Notification do
     end
   end
 
-  describe '.filter_by_display_username_and_type' do
+  describe '.filter_by_consolidation_data' do
     let(:post) { Fabricate(:post) }
     fab!(:user) { Fabricate(:user) }
 
@@ -267,8 +267,8 @@ describe Notification do
     end
 
     it 'should return the right notifications' do
-      expect(Notification.filter_by_display_username_and_type(
+      expect(Notification.filter_by_consolidation_data(
-        user.username_lower, Notification.types[:liked]
+        Notification.types[:liked], display_username: user.username_lower
       )).to eq([])
 
       expect do
@@ -280,8 +280,8 @@ describe Notification do
         PostActionCreator.like(user, post)
       end.to change { Notification.count }.by(2)
 
-      expect(Notification.filter_by_display_username_and_type(
+      expect(Notification.filter_by_consolidation_data(
-        user.username_lower, Notification.types[:liked]
+        Notification.types[:liked], display_username: user.username_lower
       )).to contain_exactly(
         Notification.find_by(notification_type: Notification.types[:liked])
       )
@@ -376,7 +376,7 @@ describe Notification do
 
       before do
         PostCustomField.create!(post_id: post.id, name: "requested_group_id", value: group.id)
-        create_membership_request_notification
+        2.times { create_membership_request_notification }
       end
 
       it 'should consolidate membership requests to a new notification' do
@@ -391,12 +391,12 @@ describe Notification do
 
         data = notification.data_hash
         expect(data[:group_name]).to eq(group.name)
-        expect(data[:count]).to eq(3)
+        expect(data[:count]).to eq(4)
 
         notification = create_membership_request_notification
         expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound)
 
-        expect(Notification.last.data_hash[:count]).to eq(4)
+        expect(Notification.last.data_hash[:count]).to eq(5)
       end
     end
   end

spec/models/post_action_spec.rb

@@ -263,13 +263,13 @@ describe PostAction do
       fab!(:likee) { Fabricate(:user) }
 
       it "can be disabled" do
-        SiteSetting.likes_notification_consolidation_threshold = 0
+        SiteSetting.notification_consolidation_threshold = 0
 
         expect do
           PostActionCreator.like(liker, Fabricate(:post, user: likee))
         end.to change { likee.reload.notifications.count }.by(1)
 
-        SiteSetting.likes_notification_consolidation_threshold = 1
+        SiteSetting.notification_consolidation_threshold = 1
 
         expect do
           PostActionCreator.like(liker, Fabricate(:post, user: likee))
@@ -285,7 +285,7 @@ describe PostAction do
         end
 
         it 'should consolidate likes notification when the threshold is reached' do
-          SiteSetting.likes_notification_consolidation_threshold = 2
+          SiteSetting.notification_consolidation_threshold = 2
 
           expect do
             3.times do
@@ -353,7 +353,7 @@ describe PostAction do
         end
 
         it 'should consolidate liked notifications when threshold is reached' do
-          SiteSetting.likes_notification_consolidation_threshold = 2
+          SiteSetting.notification_consolidation_threshold = 2
 
           post = Fabricate(:post, user: likee)