From 260b3e7f96a95d5ba14d1270a47ff58c34a43ee0 Mon Sep 17 00:00:00 2001
From: Patrick R <patrick.rabier@gmail.com>
Date: Wed, 14 Aug 2019 16:22:04 +0800
Subject: [PATCH] Fix message subscription rules (#803)

---
 app/models/concerns/message_subscription.rb    | 18 ++++++++++--------
 .../concerns/message_subscription_spec.rb      |  3 +++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/app/models/concerns/message_subscription.rb b/app/models/concerns/message_subscription.rb
index 6adc8e3ef..0ce30b5e2 100755
--- a/app/models/concerns/message_subscription.rb
+++ b/app/models/concerns/message_subscription.rb
@@ -27,13 +27,15 @@ def subscribe_users_to_message
     user_ids -= [User.system_user.try(:id), User.stockit_user.try(:id)]
     user_ids -= [obj.try(:created_by_id)] if self.is_private or obj.try('cancelled?')
 
-    # For private messages, subscribe all supervisors ONLY for the first message
-    if self.is_private && is_first_message_for(klass, obj.id)
-      user_ids += User.supervisors.pluck(:id)
-    end
 
-    # If donor sends a message but no one else is listening, subscribe all reviewers.
-    user_ids += User.reviewers.pluck(:id) if [self.sender_id] == user_ids
+    # Cases where we subscribe every stafff member
+    #  - For private messages, subscribe all supervisors ONLY for the first message
+    #  - If donor sends a message but no one else is listening, subscribe all reviewers.
+    subscribe_all_staff = is_private ?
+      is_first_message_for(klass, obj.id) :
+      [self.sender_id] == user_ids
+
+    user_ids += User.staff.pluck(:id) if subscribe_all_staff
 
     user_ids.flatten.compact.uniq.each do |user_id|
       state = (user_id == self.sender_id) ? "read" : "unread" # mark as read for sender
@@ -56,9 +58,9 @@ def public_subscribers_to(klass, id)
   #   > A supervisor who has answered the private thread
   def private_subscribers_to(klass, id)
     User.supervisors
-        .joins(subscriptions: [:message])
-        .where(subscriptions: { "#{klass}_id": id })
+        .joins(messages: [:subscriptions])
         .where(messages: { is_private: true })
+        .where(subscriptions: { "#{klass}_id": id })
         .pluck(:id)
   end
 
diff --git a/spec/models/concerns/message_subscription_spec.rb b/spec/models/concerns/message_subscription_spec.rb
index f73f841d0..9b1777cdf 100755
--- a/spec/models/concerns/message_subscription_spec.rb
+++ b/spec/models/concerns/message_subscription_spec.rb
@@ -102,6 +102,7 @@
 
       context "should not subscribe other supervisors for subsequent messages" do
         let!(:supervisor) { create :user, :supervisor }
+        let!(:other_reviewer) { create :user, :reviewer }
 
         before { User.current_user = supervisor }
 
@@ -109,10 +110,12 @@
           # The unrelated supervisor receives the first message of the thread
           expect(message).to receive(:add_subscription).with('read', reviewer.id)
           expect(message).to receive(:add_subscription).with('unread', supervisor.id)
+          expect(message).to receive(:add_subscription).with('unread', other_reviewer.id)
           message.save
 
           # The unrelated supervisor doesn't receive subsequent message of the thread
           expect(message2).to receive(:add_subscription).with('read', reviewer.id) # sender
+          expect(message2).not_to receive(:add_subscription).with('unread', other_reviewer.id)
           expect(message2).not_to receive(:add_subscription).with('unread', supervisor.id)
           message2.save
         end