diff --git a/lib/assigner.rb b/lib/assigner.rb index e4d083c..8e37978 100644 --- a/lib/assigner.rb +++ b/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) - 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 + assigned_to_type == "Group" ? invite_group(assign_to) : invite_user(assign_to) 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, diff --git a/spec/lib/assigner_spec.rb b/spec/lib/assigner_spec.rb index a26dc77..87e4f8b 100644 --- a/spec/lib/assigner_spec.rb +++ b/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