FIX: properly set notification levels on group invite

FIX: properly set notification levels on group invite

Previously we relied on side effects to set tracking state correctly when inviting groups to messages

Also has a minor optimisation in that we use pluck instead of pulling in full record

diff --git a/app/models/group.rb b/app/models/group.rb
index 7e560dc..fd76e8a 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -359,6 +359,25 @@ class Group < ActiveRecord::Base
     (10..19).to_a
   end
 
+  def set_message_default_notification_levels!(topic, ignore_existing: false)
+    group_users.pluck(:user_id, :notification_level).each do |user_id, notification_level|
+      next if user_id == -1
+      next if user_id == topic.user_id
+      next if ignore_existing && TopicUser.where(user_id: user_id, topic_id: topic.id).exists?
+
+      action =
+        case notification_level
+        when TopicUser.notification_levels[:tracking] then "track!"
+        when TopicUser.notification_levels[:regular]  then "regular!"
+        when TopicUser.notification_levels[:muted]    then "mute!"
+        when TopicUser.notification_levels[:watching] then "watch!"
+        else "track!"
+        end
+
+      topic.notifier.public_send(action, user_id)
+    end
+  end
+
   def self.refresh_automatic_group!(name)
     return unless id = AUTO_GROUPS[name]
 
diff --git a/app/models/topic.rb b/app/models/topic.rb
index a37ea05..b99b9c7 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -821,6 +821,8 @@ class Topic < ActiveRecord::Base
 
       group_id = group.id
 
+      group.set_message_default_notification_levels!(self, ignore_existing: true)
+
       group.users.where(
         "group_users.notification_level = :level",
         level: NotificationLevels.all[:tracking],
@@ -845,6 +847,7 @@ class Topic < ActiveRecord::Base
             group_id: group_id
           }.to_json
         )
+
       end
     end
 
diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb
index 5726d74..34e3848 100644
--- a/lib/topic_creator.rb
+++ b/lib/topic_creator.rb
@@ -84,21 +84,8 @@ class TopicCreator
       topic.notifier.watch!(tau.user_id)
     end
 
-    topic.reload.topic_allowed_groups.each do |tag|
-      tag.group.group_users.each do |gu|
-        next if gu.user_id == -1 || gu.user_id == topic.user_id
-
-        action =
-          case gu.notification_level
-          when TopicUser.notification_levels[:tracking] then "track!"
-          when TopicUser.notification_levels[:regular]  then "regular!"
-          when TopicUser.notification_levels[:muted]    then "mute!"
-          when TopicUser.notification_levels[:watching] then "watch!"
-          else "track!"
-          end
-
-        topic.notifier.public_send(action, gu.user_id)
-      end
+    topic.reload.topic_allowed_groups.each do |topic_allowed_group|
+      topic_allowed_group.group.set_message_default_notification_levels!(topic)
     end
   end
 
diff --git a/spec/models/topic_user_spec.rb b/spec/models/topic_user_spec.rb
index f0d36fb..5f2f16d 100644
--- a/spec/models/topic_user_spec.rb
+++ b/spec/models/topic_user_spec.rb
@@ -279,8 +279,8 @@ describe TopicUser do
         it "should use group's default notification level" do
           another_user = Fabricate(:user)
           group.add(another_user)
+
           topic.invite_group(target_user, group)
-          TopicUser.track_visit!(topic.id, another_user.id)
 
           expect(TopicUser.get(topic, another_user).notification_level)
             .to eq(TopicUser.notification_levels[:tracking])

GitHub sha: 9a9e31f9

1 Like

This might blow up on large groups?

Topic creation has had this for ages, might make sense to add a limit on large groups, on groups with more than n members do nothing

1 Like