FEATURE: Site setting to cap the recipient list in notification emails

* Adds a hidden site setting: `max_participant_names`
* Replaces duplicate code in `GroupSmtpMailer` and `UserNotifications`
* Groups are sorted by the number of users (decreasing)
* Replaces the query to count users of each group with `Group#user_count`)
* Users are sorted by their last reply in the topic (most recent first)
* Adds lots of tests

app/mailers/group_smtp_mailer.rb

@@ -36,7 +36,7 @@ class GroupSmtpMailer < ActionMailer::Base
       only_reply_by_email: true,
       use_from_address_for_reply_to: SiteSetting.enable_smtp && from_group.smtp_enabled?,
       private_reply: post.topic.private_message?,
-      participants: participants(post, recipient_user),
+      participants: UserNotifications.participants(post, recipient_user, reveal_staged_email: true),
       include_respond_instructions: true,
       template: 'user_notifications.user_posted_pm',
       use_topic_title_subject: true,
@@ -68,24 +68,4 @@ class GroupSmtpMailer < ActionMailer::Base
       }
     )
   end
-
-  def participants(post, recipient_user)
-    list = []
-
-    post.topic.allowed_groups.each do |g|
-      list.push("[#{g.name_full_preferred}](#{g.full_url})")
-    end
-
-    post.topic.allowed_users.each do |u|
-      next if u.id == recipient_user.id
-
-      if u.staged?
-        list.push("#{u.email}")
-      else
-        list.push("[#{u.display_name}](#{u.full_url})")
-      end
-    end
-
-    list.join(', ')
-  end
 end

app/mailers/user_notifications.rb

@@ -552,19 +552,7 @@ class UserNotifications < ActionMailer::Base
           I18n.t('subject_pm')
         end
 
-      participants = ""
+      participants = self.class.participants(post, user)
-      participant_list = []
-
-      post.topic.allowed_groups.each do |g|
-        participant_list.push "[#{g.name} (#{g.users.count})](#{g.full_url})"
-      end
-
-      post.topic.allowed_users.each do |u|
-        next if u.id == user.id
-        participant_list.push "[#{u.display_name}](#{u.full_url})"
-      end
-
-      participants += participant_list.join(", ")
     end
 
     if SiteSetting.private_email?
@@ -697,6 +685,51 @@ class UserNotifications < ActionMailer::Base
     build_email(user.email, email_opts)
   end
 
+  def self.participants(post, recipient_user, reveal_staged_email: false)
+    list = []
+
+    allowed_groups = post.topic.allowed_groups.order("user_count DESC")
+
+    allowed_groups.each do |g|
+      list.push("[#{g.name_full_preferred} (#{g.user_count})](#{g.full_url})")
+      break if list.size >= SiteSetting.max_participant_names
+    end
+
+    recent_posts_query = post.topic.posts
+      .select("user_id, MAX(post_number) AS post_number")
+      .where(post_type: Post.types[:regular], post_number: ..post.post_number)
+      .where.not(user_id: recipient_user.id)
+      .group(:user_id)
+      .order("post_number DESC")
+      .limit(SiteSetting.max_participant_names)
+      .to_sql
+
+    allowed_users = post.topic.allowed_users
+      .joins("LEFT JOIN (#{recent_posts_query}) pu ON topic_allowed_users.user_id = pu.user_id")
+      .order("post_number DESC NULLS LAST", :id)
+      .where.not(id: recipient_user.id)
+      .human_users
+
+    allowed_users.each do |u|
+      break if list.size >= SiteSetting.max_participant_names
+
+      if reveal_staged_email && u.staged?
+        list.push("#{u.email}")
+      else
+        list.push("[#{u.display_name}](#{u.full_url})")
+      end
+    end
+
+    participants = list.join(I18n.t("word_connector.comma"))
+    others_count = allowed_groups.size + allowed_users.size - list.size
+
+    if others_count > 0
+      I18n.t("user_notifications.more_pm_participants", participants: participants, count: others_count)
+    else
+      participants
+    end
+  end
+
   private
 
   def build_user_email_token_by_template(template, user, email_token)

config/locales/server.en.yml

@@ -3520,6 +3520,9 @@ en:
     posted_by: "Posted by %{username} on %{post_date}"
 
     pm_participants: "Participants: %{participants}"
+    more_pm_participants:
+      one: "%{participants} and %{count} other"
+      other: "%{participants} and %{count} others"
 
     invited_group_to_private_message_body: |
       %{username} invited @%{group_name} to a message

config/site_settings.yml

@@ -1260,6 +1260,9 @@ email:
     client: true
     default: true
     hidden: true
+  max_participant_names:
+    default: 10
+    hidden: true
 
 files:
   max_image_size_kb:

spec/mailers/group_smtp_mailer_spec.rb

@@ -28,18 +28,18 @@ describe GroupSmtpMailer do
 
   let(:email) do
     <<~EMAIL
-    Delivered-To: bugs@gmail.com
+      Delivered-To: bugs@gmail.com
-    MIME-Version: 1.0
+      MIME-Version: 1.0
-    From: John Doe <john@doe.com>
+      From: John Doe <john@doe.com>
-    Date: Tue, 01 Jan 2019 12:00:00 +0200
+      Date: Tue, 01 Jan 2019 12:00:00 +0200
-    Message-ID: <a52f67a3d3560f2a35276cda8519b10b595623bcb66912bb92df6651ad5f75be@mail.gmail.com>
+      Message-ID: <a52f67a3d3560f2a35276cda8519b10b595623bcb66912bb92df6651ad5f75be@mail.gmail.com>
-    Subject: Hello from John
+      Subject: Hello from John
-    To: "bugs@gmail.com" <bugs@gmail.com>
+      To: "bugs@gmail.com" <bugs@gmail.com>
-    Content-Type: text/plain; charset="UTF-8"
+      Content-Type: text/plain; charset="UTF-8"
 
-    Hello,
+      Hello,
 
-    How are you doing?
+      How are you doing?
     EMAIL
   end
 
@@ -80,6 +80,19 @@ describe GroupSmtpMailer do
     expect(sent_mail.to_s).to include(raw)
   end
 
+  it "includes the participants list in the email" do
+    Fabricate(:staged, email: "james.bond@gmail.com")
+    topic = receiver.incoming_email.topic
+    topic.invite(Discourse.system_user, "james.bond@gmail.com")
+
+    PostCreator.create(user, topic_id: topic.id, raw: raw)
+
+    expect(ActionMailer::Base.deliveries.size).to eq(1)
+
+    sent_mail = ActionMailer::Base.deliveries[0]
+    expect(sent_mail.to_s).to include("[Testers Group (1)](http://test.localhost/g/Testers), james.bond@gmail.com")
+  end
+
   it "uses the OP incoming email subject for the subject over topic title" do
     receiver.incoming_email.topic.update(title: "blah")
     post = PostCreator.create(user,

spec/mailers/user_notifications_spec.rb

@@ -594,7 +594,7 @@ describe UserNotifications do
       group2 = Fabricate(:group, name: "group2")
 
       user1 = Fabricate(:user, username: "one", groups: [group1, group2])
-      user2 = Fabricate(:user, username: "two", groups: [group1])
+      user2 = Fabricate(:user, username: "two", groups: [group1], staged: true)
 
       topic.allowed_users = [user, user1, user2]
       topic.allowed_groups = [group1, group2]
@@ -1086,4 +1086,102 @@ describe UserNotifications do
     end
 
   end
+
+  describe "#participants" do
+    fab!(:group1) { Fabricate(:group, name: "group1") }
+    fab!(:group2) { Fabricate(:group, name: "group2") }
+    fab!(:group3) { Fabricate(:group, name: "group3") }
+    fab!(:user1) { Fabricate(:user, username: "one", name: nil, groups: [group1, group2]) }
+    fab!(:user2) { Fabricate(:user, username: "two", name: nil, groups: [group1]) }
+    fab!(:user3) { Fabricate(:user, username: "three", name: nil, groups: [group3]) }
+    fab!(:user4) { Fabricate(:user, username: "four", name: nil, groups: [group1, group3]) }
+    fab!(:admin) { Fabricate(:admin, username: "admin", name: nil) }
+
+    fab!(:topic) do
+      t = Fabricate(:private_message_topic, title: "Super cool topic")
+      t.allowed_users = [user1, user2, user3, user4, admin]
+      t.allowed_groups = [group1]
+      t
+    end
+    fab!(:posts) do
+      [
+        Fabricate(:post, topic: topic, post_number: 1, user: user2),
+        Fabricate(:post, topic: topic, post_number: 2, user: user1),
+        Fabricate(:post, topic: topic, post_number: 3, user: user2),
+        Fabricate(:small_action, topic: topic, post_number: 4, user: admin),
+        Fabricate(:post, topic: topic, post_number: 5, user: user4),
+        Fabricate(:post, topic: topic, post_number: 6, user: user3),
+        Fabricate(:post, topic: topic, post_number: 7, user: user4)
+      ]
+    end
+
+    it "returns a list of participants (except for the recipient), groups first, followed by users in order of their last reply" do
+      expect(UserNotifications.participants(posts.last, user3)).to eq("[group1 (3)](http://test.localhost/g/group1), " \
+        "[four](http://test.localhost/u/four), [two](http://test.localhost/u/two), [one](http://test.localhost/u/one), " \
+        "[admin](http://test.localhost/u/admin)")
+    end
+
+    it "caps the list according to site setting" do
+      SiteSetting.max_participant_names = 3
+      list = "[group1 (3)](http://test.localhost/g/group1), [four](http://test.localhost/u/four), [two](http://test.localhost/u/two)"
+      expect(UserNotifications.participants(posts.last, user3)).to eq(I18n.t("user_notifications.more_pm_participants", participants: list, count: 2))
+    end
+
+    it "orders groups by user count" do
+      SiteSetting.max_participant_names = 3
+      topic.allowed_groups = [group1, group2, group3]
+
+      list = "[group1 (3)](http://test.localhost/g/group1), [group3 (2)](http://test.localhost/g/group3), [group2 (1)](http://test.localhost/g/group2)"
+      expect(UserNotifications.participants(posts.last, user3)).to eq(I18n.t("user_notifications.more_pm_participants", participants: list, count: 4))
+    end
+
+    it "orders users by their last reply and user id" do
+      expect(UserNotifications.participants(posts[-3], user4)).to eq("[group1 (3)](http://test.localhost/g/group1), " \
+        "[two](http://test.localhost/u/two), [one](http://test.localhost/u/one), [three](http://test.localhost/u/three), " \
+        "[admin](http://test.localhost/u/admin)")
+    end
+
+    it "prefers full group names when available" do
+      SiteSetting.max_participant_names = 2
+      topic.allowed_groups = [group1, group2]
+
+      group2.update!(full_name: "Awesome Group")
+
+      list = "[group1 (3)](http://test.localhost/g/group1), [Awesome Group (1)](http://test.localhost/g/group2)"
+      expect(UserNotifications.participants(posts.last, user3)).to eq(I18n.t("user_notifications.more_pm_participants", participants: list, count: 4))
+    end
+
+    it "always uses usernames when prioritize_username_in_ux is enabled" do
+      user4.update!(name: "James Bond")
+      user1.update!(name: "Indiana Jones")
+
+      SiteSetting.prioritize_username_in_ux = true
+      expect(UserNotifications.participants(posts.last, user3)).to eq("[group1 (3)](http://test.localhost/g/group1), " \
+        "[four](http://test.localhost/u/four), [two](http://test.localhost/u/two), [one](http://test.localhost/u/one), " \
+        "[admin](http://test.localhost/u/admin)")
+
+      SiteSetting.prioritize_username_in_ux = false
+      expect(UserNotifications.participants(posts.last, user3)).to eq("[group1 (3)](http://test.localhost/g/group1), " \
+        "[James Bond](http://test.localhost/u/four), [two](http://test.localhost/u/two), [Indiana Jones](http://test.localhost/u/one), " \
+        "[admin](http://test.localhost/u/admin)")
+    end
+
+    it "reveals the email address of staged users if enabled" do
+      user4.update!(staged: true, email: "james.bond@mi6.invalid")
+      user1.update!(staged: true, email: "indiana.jones@example.com")
+
+      SiteSetting.prioritize_username_in_ux = true
+      expect(UserNotifications.participants(posts.last, user3, reveal_staged_email: true)).to eq( \
+        "[group1 (3)](http://test.localhost/g/group1), james.bond@mi6.invalid, [two](http://test.localhost/u/two), " \
+        "indiana.jones@example.com, [admin](http://test.localhost/u/admin)")
+    end
+
+    it "does only include human users" do
+      topic.allowed_users << Discourse.system_user
+
+      expect(UserNotifications.participants(posts.last, user3)).to eq("[group1 (3)](http://test.localhost/g/group1), " \
+        "[four](http://test.localhost/u/four), [two](http://test.localhost/u/two), [one](http://test.localhost/u/one), " \
+        "[admin](http://test.localhost/u/admin)")
+    end
+  end
 end