FEATURE: skip group invite if all members can see topic already. (#474)

We don't want to invite a group when all of its members are part of another group that is already allowed to the topic.

lib/assigner.rb

@@ -259,18 +259,7 @@ class ::Assigner
     assigned_to_type = assign_to.is_a?(User) ? "User" : "Group"
 
     if topic.private_message? && SiteSetting.invite_on_assign
-      guardian = Guardian.new(@assigned_by)
+      assigned_to_type == "Group" ? invite_group(assign_to) : invite_user(assign_to)
-      if assigned_to_type == "Group"
-        unless topic.topic_allowed_groups.exists?(group_id: assign_to.id)
-          guardian.ensure_can_invite_group_to_private_message!(assign_to, topic)
-          topic.invite_group(@assigned_by, assign_to)
-        end
-      else
-        unless topic.all_allowed_users.exists?(id: assign_to.id)
-          guardian.ensure_can_invite_to!(topic)
-          topic.invite(@assigned_by, assign_to.username)
-        end
-      end
     end
 
     forbidden_reason =
@@ -463,6 +452,31 @@ class ::Assigner
 
   private
 
+  def invite_user(user)
+    return if topic.all_allowed_users.exists?(id: user.id)
+
+    guardian.ensure_can_invite_to!(topic)
+    topic.invite(@assigned_by, user.username)
+  end
+
+  def invite_group(group)
+    return if topic.topic_allowed_groups.exists?(group_id: group.id)
+    if topic
+         .all_allowed_users
+         .joins("RIGHT JOIN group_users ON group_users.user_id = users.id")
+         .where("group_users.group_id = ? AND users.id IS NULL", group.id)
+         .empty?
+      return # all group members can already see the topic
+    end
+
+    guardian.ensure_can_invite_group_to_private_message!(group, topic)
+    topic.invite_group(@assigned_by, group)
+  end
+
+  def guardian
+    @guardian ||= Guardian.new(@assigned_by)
+  end
+
   def queue_notification(assign_to, skip_small_action_post, assignment)
     Jobs.enqueue(
       :assign_notification,

spec/lib/assigner_spec.rb

@@ -740,10 +740,34 @@ RSpec.describe Assigner do
           assignable_level: Group::ALIAS_LEVELS[:only_admins],
           messageable_level: Group::ALIAS_LEVELS[:only_admins],
         )
+      group.add(Fabricate(:user))
       assigner.assign(group)
       expect(topic.allowed_groups).to include(group)
     end
 
+    it "doesn't invite group if all members have access to the PM already" do
+      user1, user2, user3 = 3.times.collect { Fabricate(:user) }
+      group1, group2, group3 =
+        3.times.collect do
+          Fabricate(
+            :group,
+            assignable_level: Group::ALIAS_LEVELS[:only_admins],
+            messageable_level: Group::ALIAS_LEVELS[:only_admins],
+          )
+        end
+      group1.add(user1)
+      group1.add(user3)
+      group2.add(user2)
+      group2.add(user3)
+      group3.add(user3)
+      topic.allowed_groups << group1
+
+      assigner.assign(group2)
+      assigner.assign(group3)
+
+      expect(topic.allowed_groups).to eq([group1, group2])
+    end
+
     it "doesn't invite group to the PM if it's not messageable" do
       group =
         Fabricate(
@@ -751,6 +775,7 @@ RSpec.describe Assigner do
           assignable_level: Group::ALIAS_LEVELS[:only_admins],
           messageable_level: Group::ALIAS_LEVELS[:nobody],
         )
+      group.add(Fabricate(:user))
       expect { assigner.assign(group) }.to raise_error(Discourse::InvalidAccess)
     end
   end