Fix message subscription rules (#803)

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
 

spec/models/concerns/message_subscription_spec.rb

@@ -102,17 +102,20 @@
 
       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 }
 
         it do
           # 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