From aa2cc4ab3178a57aeaa9d9b1a104e64040c6eaef Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Thu, 17 Jan 2019 14:31:07 +0800
Subject: [PATCH] FIX: Liked notification consolidation has to account for user
 like frequency setting.

---
 app/models/notification.rb           |   6 +-
 app/services/post_action_notifier.rb |  83 ------------
 app/services/post_alerter.rb         | 115 +++++++++++++++--
 spec/models/notification_spec.rb     |  10 +-
 spec/models/post_action_spec.rb      | 185 +++++++++++++++++++--------
 5 files changed, 247 insertions(+), 152 deletions(-)

diff --git a/app/models/notification.rb b/app/models/notification.rb
index bb692273ce3..ba49fbb624a 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 c392d9ff623..c4d988f25dc 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 ecd1af37a91..632029fbddb 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]
+        (
+          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
+        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
+    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
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 3a7969fedca..3bca8db0ac6 100644
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -250,7 +250,7 @@ describe Notification do
     end
   end
 
-  describe '.get_liked_by' do
+  describe '.filter_by_display_username_and_type' do
     let(:post) { Fabricate(:post) }
     let(:user) { Fabricate(:user) }
 
@@ -259,7 +259,9 @@ describe Notification do
     end
 
     it 'should return the right notifications' do
-      expect(Notification.get_liked_by(user)).to eq([])
+      expect(Notification.filter_by_display_username_and_type(
+        user.username_lower, Notification.types[:liked]
+      )).to eq([])
 
       expect do
         PostAlerter.post_created(Fabricate(:basic_reply,
@@ -270,7 +272,9 @@ describe Notification do
         PostAction.act(user, post, PostActionType.types[:like])
       end.to change { Notification.count }.by(2)
 
-      expect(Notification.get_liked_by(user)).to contain_exactly(
+      expect(Notification.filter_by_display_username_and_type(
+        user.username_lower, Notification.types[:liked]
+      )).to contain_exactly(
         Notification.find_by(notification_type: Notification.types[:liked])
       )
     end
diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb
index 296fdbfe29b..8038c7865c2 100644
--- a/spec/models/post_action_spec.rb
+++ b/spec/models/post_action_spec.rb
@@ -381,6 +381,7 @@ describe PostAction do
 
     describe 'likes consolidation' do
       let(:liker) { Fabricate(:user) }
+      let(:liker2) { Fabricate(:user) }
       let(:likee) { Fabricate(:user) }
 
       it "can be disabled" do
@@ -405,80 +406,146 @@ describe PostAction do
         end.to_not change { likee.reload.notifications.count }
       end
 
-      it 'should consolidate likes notification when the threshold is reached' do
-        SiteSetting.likes_notification_consolidation_threshold = 2
+      describe 'frequency first_time_and_daily' do
+        before do
+          likee.user_option.update!(
+            like_notification_frequency:
+              UserOption.like_notification_frequency_type[:first_time_and_daily]
+          )
+        end
 
-        freeze_time
+        it 'should consolidate likes notification when the threshold is reached' do
+          SiteSetting.likes_notification_consolidation_threshold = 2
 
-        expect do
-          3.times do
+          expect do
+            3.times do
+              PostAction.act(
+                liker,
+                Fabricate(:post, user: likee),
+                PostActionType.types[:like]
+              )
+            end
+          end.to change { likee.reload.notifications.count }.by(1)
+
+          notification = likee.notifications.last
+
+          expect(notification.notification_type).to eq(
+            Notification.types[:liked_consolidated]
+          )
+
+          data = JSON.parse(notification.data)
+
+          expect(data["username"]).to eq(liker.username)
+          expect(data["display_username"]).to eq(liker.username)
+          expect(data["count"]).to eq(3)
+
+          notification.update!(read: true)
+
+          expect do
+            2.times do
+              PostAction.act(
+                liker,
+                Fabricate(:post, user: likee),
+                PostActionType.types[:like]
+              )
+            end
+          end.to_not change { likee.reload.notifications.count }
+
+          data = JSON.parse(notification.reload.data)
+
+          expect(notification.read).to eq(false)
+          expect(data["count"]).to eq(5)
+
+          # Like from a different user shouldn't be consolidated
+          expect do
+            PostAction.act(
+              Fabricate(:user),
+              Fabricate(:post, user: likee),
+              PostActionType.types[:like]
+            )
+          end.to change { likee.reload.notifications.count }.by(1)
+
+          notification = likee.notifications.last
+
+          expect(notification.notification_type).to eq(
+            Notification.types[:liked]
+          )
+
+          freeze_time((
+            SiteSetting.likes_notification_consolidation_window_mins.minutes +
+            1
+          ).since)
+
+          expect do
             PostAction.act(
               liker,
               Fabricate(:post, user: likee),
               PostActionType.types[:like]
             )
-          end
-        end.to change { likee.reload.notifications.count }.by(1)
+          end.to change { likee.reload.notifications.count }.by(1)
 
-        notification = likee.notifications.last
+          notification = likee.notifications.last
 
-        expect(notification.notification_type).to eq(
-          Notification.types[:liked_consolidated]
-        )
+          expect(notification.notification_type).to eq(Notification.types[:liked])
+        end
+      end
 
-        data = JSON.parse(notification.data)
+      describe 'frequency always' do
+        before do
+          likee.user_option.update!(
+            like_notification_frequency:
+              UserOption.like_notification_frequency_type[:always]
+          )
+        end
 
-        expect(data["username"]).to eq(liker.username)
-        expect(data["display_username"]).to eq(liker.username)
-        expect(data["count"]).to eq(3)
+        it 'should consolidate liked notifications when threshold is reached' do
+          SiteSetting.likes_notification_consolidation_threshold = 2
 
-        notification.update!(read: true)
+          post = Fabricate(:post, user: likee)
 
-        expect do
-          2.times do
+          expect do
+            [liker2, liker].each do |user|
+              PostAction.act(user, post, PostActionType.types[:like])
+            end
+          end.to change { likee.reload.notifications.count }.by(1)
+
+          notification = likee.notifications.last
+          data_hash = notification.data_hash
+
+          expect(data_hash["original_username"]).to eq(liker.username)
+          expect(data_hash["username2"]).to eq(liker2.username)
+          expect(data_hash["count"].to_i).to eq(2)
+
+          expect do
+            2.times do
+              PostAction.act(
+                liker,
+                Fabricate(:post, user: likee),
+                PostActionType.types[:like]
+              )
+            end
+          end.to change { likee.reload.notifications.count }.by(2)
+
+          expect(likee.notifications.pluck(:notification_type).uniq)
+            .to contain_exactly(Notification.types[:liked])
+
+          expect do
             PostAction.act(
               liker,
               Fabricate(:post, user: likee),
               PostActionType.types[:like]
             )
-          end
-        end.to_not change { likee.reload.notifications.count }
+          end.to change { likee.reload.notifications.count }.by(-1)
 
-        data = JSON.parse(notification.reload.data)
+          notification = likee.notifications.last
 
-        expect(notification.read).to eq(false)
-        expect(data["count"]).to eq(5)
-
-        # Like from a different user shouldn't be consolidated
-        expect do
-          PostAction.act(
-            Fabricate(:user),
-            Fabricate(:post, user: likee),
-            PostActionType.types[:like]
+          expect(notification.notification_type).to eq(
+            Notification.types[:liked_consolidated]
           )
-        end.to change { likee.reload.notifications.count }.by(1)
 
-        notification = likee.notifications.last
-
-        expect(notification.notification_type).to eq(
-          Notification.types[:liked]
-        )
-
-        freeze_time(
-          SiteSetting.likes_notification_consolidation_window_mins.minutes.since
-        )
-
-        expect do
-          PostAction.act(
-            liker,
-            Fabricate(:post, user: likee),
-            PostActionType.types[:like]
-          )
-        end.to change { likee.reload.notifications.count }.by(1)
-
-        notification = likee.notifications.last
-
-        expect(notification.notification_type).to eq(Notification.types[:liked])
+          expect(notification.data_hash["count"].to_i).to eq(3)
+          expect(notification.data_hash["username"]).to eq(liker.username)
+        end
       end
     end
 
@@ -491,6 +558,20 @@ describe PostAction do
       end.to_not change { Notification.count }
     end
 
+    it 'should not generate a notification if liker has the topic muted' do
+      post = Fabricate(:post, user: eviltrout)
+
+      TopicUser.create!(
+        topic: post.topic,
+        user: eviltrout,
+        notification_level: TopicUser.notification_levels[:muted]
+      )
+
+      expect do
+        PostAction.act(codinghorror, post, PostActionType.types[:like])
+      end.to_not change { Notification.count }
+    end
+
     it "should generate a notification if liker is an admin irregardles of \
       muting" do