FIX: method extraction caused push notifications to include incorrect post

FIX: method extraction caused push notifications to include incorrect post

Previously the push notification code path was not tested for notification
collapsing. This happens if you get multiple replies to a topic you are
watching.

From 82e45f5485be0deaa606b3536426666f2bf88412 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Wed, 5 Dec 2018 16:39:17 +1100
Subject: [PATCH] FIX: method extraction caused push notifications to include
 incorrect post

Previously the push notification code path was not tested for notification
collapsing. This happens if you get multiple replies to a topic you are
watching.

diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index a1a1c93..a702efd 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -399,7 +399,7 @@ class PostAlerter
     )
 
     if created.id && !existing_notification && NOTIFIABLE_TYPES.include?(type) && !user.suspended?
-      create_notification_alert(user: user, post: post, notification_type: type, username: original_username)
+      create_notification_alert(user: user, post: original_post, notification_type: type, username: original_username)
     end
 
     created.id ? created : nil
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index 90c948c..85bd3ff 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@@ -705,7 +705,7 @@ describe PostAlerter do
       Excon.expects(:post).with { |_req, _body|
         headers = _body[:headers]
         body = _body[:body]
-      }.returns("OK")
+      }.times(3).returns("OK")
 
       payload = {
         "secret_key" => SiteSetting.push_api_secret_key,
@@ -736,10 +736,46 @@ describe PostAlerter do
         ]
       }
 
-      mention_post
+      post = mention_post
 
       expect(JSON.parse(body)).to eq(payload)
       expect(headers["Content-Type"]).to eq('application/json')
+
+      TopicUser.change(evil_trout.id, topic.id, notification_level: TopicUser.notification_levels[:watching])
+
+      post = Fabricate(:post, topic: post.topic, user_id: evil_trout.id)
+      user2 = Fabricate(:user)
+
+      # if we collapse a reply notification we should get notified on the correct post
+      new_post = create_post_with_alerts(topic: post.topic, user_id: user.id, reply_to_post_number: post.post_number, raw: 'this is my first reply')
+
+      changes = {
+        "notification_type" => Notification.types[:posted],
+        "post_number" => new_post.post_number,
+        "username" => new_post.user.username,
+        "excerpt" => new_post.raw,
+        "url" => UrlHelper.absolute(new_post.url)
+      }
+
+      payload["notifications"][0].merge! changes
+      payload["notifications"][1].merge! changes
+
+      expect(JSON.parse(body)).to eq(payload)
+
+      new_post = create_post_with_alerts(topic: post.topic, user_id: user2.id, reply_to_post_number: post.post_number, raw: 'this is my second reply')
+
+      changes = {
+        "post_number" => new_post.post_number,
+        "username" => new_post.user.username,
+        "excerpt" => new_post.raw,
+        "url" => UrlHelper.absolute(new_post.url)
+      }
+
+      payload["notifications"][0].merge! changes
+      payload["notifications"][1].merge! changes
+
+      expect(JSON.parse(body)).to eq(payload)
+
     end
   end
 
@@ -880,7 +916,7 @@ describe PostAlerter do
         Fabricate.build(:topic_allowed_user, user: dave),
         Fabricate.build(:topic_allowed_user, user: erin)
       ])
-      post = Fabricate(:post, user: alice, topic: topic)
+      _post = Fabricate(:post, user: alice, topic: topic)
 
       TopicUser.change(alice.id, topic.id, notification_level: TopicUser.notification_levels[:watching])
       TopicUser.change(bob.id, topic.id, notification_level: TopicUser.notification_levels[:watching])
@@ -925,7 +961,7 @@ describe PostAlerter do
       category = Fabricate(:mailinglist_mirror_category)
       topic = Fabricate(:topic, category: category)
       user = Fabricate(:staged)
-      post = Fabricate(:post, user: user, topic: topic)
+      _post = Fabricate(:post, user: user, topic: topic)
       reply = Fabricate(:post, topic: topic, reply_to_post_number: 1)
 
       NotificationEmailer.expects(:process_notification).never

GitHub

2 Likes