FIX: Liked notification consolidation has to account for user like frequency setting.

FIX: Liked notification consolidation has to account for user like frequency setting.

diff --git a/app/models/notification.rb b/app/models/notification.rb
index bb69227..ba49fbb 100644
--- a/app/models/notification.rb
+++ b/app/models/notification.rb
@@ -13,9 +13,9 @@ 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 :get_liked_by, ->(user) {
-    where("data::json ->> 'original_username' = ?", user.username_lower)
-      .where(notification_type: Notification.types[:liked])
+  scope :filter_by_display_username_and_type, ->(username, notification_type) {
+    where("data::json ->> 'display_username' = ?", username)
+      .where(notification_type: notification_type)
       .order(created_at: :desc)
   }
 
diff --git a/app/services/post_action_notifier.rb b/app/services/post_action_notifier.rb
index c392d9f..c4d988f 100644
--- a/app/services/post_action_notifier.rb
+++ b/app/services/post_action_notifier.rb
@@ -76,53 +76,6 @@ class PostActionNotifier
     post = post_action.post
     return if post_action.user.blank?
 
-    if SiteSetting.likes_notification_consolidation_threshold.zero?
-      return create_liked_notification(alerter, post, post_action)
-    end
-
-    user_notifications = post.user.notifications
-
-    consolidation_window =
-      SiteSetting.likes_notification_consolidation_window_mins.minutes.ago
-
-    liked_by_user_notifications =
-      user_notifications
-        .get_liked_by(post_action.user)
-        .where("created_at > ?", consolidation_window)
-
-    user_liked_consolidated_notification =
-      user_notifications
-        .where(
-          "
-            created_at > ? AND
-            notification_type = ? AND
-            data::json ->> 'display_username' = ?
-          ",
-          consolidation_window,
-          Notification.types[:liked_consolidated],
-          post_action.user.username_lower
-        )
-        .first
-
-    if user_liked_consolidated_notification
-      update_consolidated_liked_notification_count!(
-        user_liked_consolidated_notification
-      )
-    elsif (
-      liked_by_user_notifications.count >=
-      SiteSetting.likes_notification_consolidation_threshold
-    )
-      create_consolidated_liked_notification!(
-        liked_by_user_notifications,
-        post,
-        post_action
-      )
-    else
-      create_liked_notification(alerter, post, post_action)
-    end
-  end
-
-  def self.create_liked_notification(alerter, post, post_action)
     alerter.create_notification(
       post.user,
       Notification.types[:liked],
@@ -132,42 +85,6 @@ class PostActionNotifier
       user_id: post_action.user_id
     )
   end
-  private_class_method :create_liked_notification
-
-  def self.update_consolidated_liked_notification_count!(notification)
-    data = JSON.parse(notification.data)
-    data["count"] += 1
-
-    notification.update!(
-      data: data.to_json,
-      read: false
-    )
-  end
-  private_class_method :update_consolidated_liked_notification_count!
-
-  def self.create_consolidated_liked_notification!(notifications,
-                                                  post,
-                                                  post_action)
-
-    Notification.transaction do
-      timestamp = notifications.last.created_at
-
-      Notification.create!(
-        notification_type: Notification.types[:liked_consolidated],
-        user_id: post.user_id,
-        data: {
-          username: post_action.user.username,
-          display_username: post_action.user.username,
-          count: notifications.count + 1
-        }.to_json,
-        updated_at: timestamp,
-        created_at: timestamp
-      )
-
-      notifications.delete_all
-    end
-  end
-  private_class_method :create_consolidated_liked_notification!
 
   def self.after_create_post_revision(post_revision)
     return if @disabled
diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index ecd1af3..632029f 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -282,7 +282,8 @@ class PostAlerter
     return if user.blank?
     return if user.id < 0
 
-    return if type == Notification.types[:liked] && user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:never]
+    is_liked = type == Notification.types[:liked]
+    return if is_liked && user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:never]
 
     # Make sure the user can see the post
     return unless Guardian.new(user).can_see?(post)
@@ -316,23 +317,38 @@ class PostAlerter
     # Don't notify the same user about the same notification on the same post
     existing_notification = user.notifications
       .order("notifications.id DESC")
-      .find_by(topic_id: post.topic_id,
-               post_number: post.post_number,
-               notification_type: type)
+      .find_by(
+        topic_id: post.topic_id,
+        post_number: post.post_number,
+        notification_type: type
+      )
 
     return if existing_notification && !should_notify_previous?(user, existing_notification, opts)
 
     notification_data = {}
 
-    if  existing_notification &&
+    if is_liked
+      if existing_notification &&
         existing_notification.created_at > 1.day.ago &&
-        user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:always]
-
-      data = existing_notification.data_hash
-      notification_data["username2"] = data["display_username"]
-      notification_data["count"] = (data["count"] || 1).to_i + 1
-      # don't use destroy so we don't trigger a notification count refresh
-      Notification.where(id: existing_notification.id).destroy_all
+        (
+          user.user_option.like_notification_frequency ==
+          UserOption.like_notification_frequency_type[:always]
+        )
+
+        data = existing_notification.data_hash
+        notification_data["username2"] = data["display_username"]
+        notification_data["count"] = (data["count"] || 1).to_i + 1
+        # don't use destroy so we don't trigger a notification count refresh
+        Notification.where(id: existing_notification.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
@@ -601,4 +617,81 @@ class PostAlerter
     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

[... diff too long, it was truncated ...]

GitHub sha: aa2cc4ab