FIX: Remove superfluous topic allowed users on group invite (#14656)

When inviting a group to a topic, there may be members of
the group already in the topic as topic allowed users. These
can be safely removed from the topic, because they are implicitly
allowed in the topic based on their group membership.

Also, this prevents issues with group SMTP emails, which rely
on the topic_allowed_users of the topic to send to and cc's
for emails, and if there are members of the group as topic_allowed_users
then that complicates things and causes odd behaviour.

We also ensure that the OP of the topic is not removed from
the topic_allowed_users when a group they belong to is added,
as it will make it harder to add them back later.

app/assets/javascripts/discourse/app/components/invite-panel.js

@@ -351,12 +351,11 @@ export default Component.extend({
     if (this.isInviteeGroup) {
       return this.inviteModel
         .createGroupInvite(this.invitee.trim())
-        .then((data) => {
+        .then(() => {
           model.setProperties({ saving: false, finished: true });
-          this.get("inviteModel.details.allowed_groups").pushObject(
-            EmberObject.create(data.group)
-          );
-          this.appEvents.trigger("post-stream:refresh");
+          this.inviteModel.reload().then(() => {
+            this.appEvents.trigger("post-stream:refresh");
+          });
         })
         .catch(onerror);
     } else {

app/models/topic.rb

@@ -971,16 +971,38 @@ class Topic < ActiveRecord::Base
   end
 
   def invite_group(user, group)
-    TopicAllowedGroup.create!(topic_id: id, group_id: group.id)
-    allowed_groups.reload
+    TopicAllowedGroup.create!(topic_id: self.id, group_id: group.id)
+    self.allowed_groups.reload
 
-    last_post = posts.order('post_number desc').where('not hidden AND posts.deleted_at IS NULL').first
+    last_post = self.posts.order('post_number desc').where('not hidden AND posts.deleted_at IS NULL').first
     if last_post
       Jobs.enqueue(:post_alert, post_id: last_post.id)
       add_small_action(user, "invited_group", group.name)
       Jobs.enqueue(:group_pm_alert, user_id: user.id, group_id: group.id, post_id: last_post.id)
     end
 
+    # If the group invited includes the OP of the topic as one of is members,
+    # we cannot strip the topic_allowed_user record since it will be more
+    # complicated to recover the topic_allowed_user record for the OP if the
+    # group is removed.
+    allowed_user_where_clause = <<~SQL
+      users.id IN (
+        SELECT topic_allowed_users.user_id
+        FROM topic_allowed_users
+        INNER JOIN group_users ON group_users.user_id = topic_allowed_users.user_id
+        INNER JOIN topic_allowed_groups ON topic_allowed_groups.group_id = group_users.group_id
+        WHERE topic_allowed_groups.group_id = :group_id AND
+              topic_allowed_users.topic_id = :topic_id AND
+              topic_allowed_users.user_id != :op_user_id
+      )
+    SQL
+    User.where([
+      allowed_user_where_clause,
+      { group_id: group.id, topic_id: self.id, op_user_id: self.user_id }
+    ]).find_each do |allowed_user|
+      remove_allowed_user(Discourse.system_user, allowed_user)
+    end
+
     true
   end
 

spec/models/topic_spec.rb

@@ -1021,6 +1021,51 @@ describe Topic do
               .to eq(Notification.types[:group_message_summary])
 
           end
+
+          it "removes users in topic_allowed_users who are part of the added group" do
+            admins = Group[:admins]
+            admins.update!(messageable_level: Group::ALIAS_LEVELS[:everyone])
+
+            # clear up the state so we can be more explicit with the test
+            TopicAllowedUser.where(topic: topic).delete_all
+            user0 = topic.user
+            user1 = Fabricate(:user)
+            user2 = Fabricate(:user)
+            user3 = Fabricate(:user)
+            Fabricate(:topic_allowed_user, topic: topic, user: user0)
+            Fabricate(:topic_allowed_user, topic: topic, user: user1)
+            Fabricate(:topic_allowed_user, topic: topic, user: user2)
+            Fabricate(:topic_allowed_user, topic: topic, user: user3)
+
+            admins.add(user1)
+            admins.add(user2)
+
+            other_topic = Fabricate(:topic)
+            Fabricate(:topic_allowed_user, user: user1, topic: other_topic)
+
+            expect(topic.invite_group(topic.user, admins)).to eq(true)
+            expect(topic.posts.last.action_code).to eq("removed_user")
+            expect(topic.allowed_users).to match_array([user0, user3, Discourse.system_user])
+            expect(other_topic.allowed_users).to match_array([user1])
+          end
+
+          it "does not remove the OP from topic_allowed_users if they are part of an added group" do
+            admins = Group[:admins]
+            admins.update!(messageable_level: Group::ALIAS_LEVELS[:everyone])
+
+            # clear up the state so we can be more explicit with the test
+            TopicAllowedUser.where(topic: topic).delete_all
+            user0 = topic.user
+            user1 = Fabricate(:user)
+            Fabricate(:topic_allowed_user, topic: topic, user: user0)
+            Fabricate(:topic_allowed_user, topic: topic, user: user1)
+
+            admins.add(topic.user)
+            admins.add(user1)
+
+            expect(topic.invite_group(topic.user, admins)).to eq(true)
+            expect(topic.allowed_users).to match_array([topic.user, Discourse.system_user])
+          end
         end
       end
     end