diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index 222a5d39392..d47a75051ef 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/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 diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 8ca231a5f30..69299b7a5f7 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -552,19 +552,7 @@ class UserNotifications < ActionMailer::Base I18n.t('subject_pm') end - participants = "" - 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(", ") + participants = self.class.participants(post, user) 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) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 7e3442be530..7125477c1ab 100644 --- a/config/locales/server.en.yml +++ b/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 diff --git a/config/site_settings.yml b/config/site_settings.yml index 875320552e4..04daca202a2 100644 --- a/config/site_settings.yml +++ b/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: diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb index a314948ce3d..d06186e265b 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -28,18 +28,18 @@ describe GroupSmtpMailer do let(:email) do <<~EMAIL - Delivered-To: bugs@gmail.com - MIME-Version: 1.0 - From: John Doe - Date: Tue, 01 Jan 2019 12:00:00 +0200 - Message-ID: - Subject: Hello from John - To: "bugs@gmail.com" - Content-Type: text/plain; charset="UTF-8" + Delivered-To: bugs@gmail.com + MIME-Version: 1.0 + From: John Doe + Date: Tue, 01 Jan 2019 12:00:00 +0200 + Message-ID: + Subject: Hello from John + To: "bugs@gmail.com" + 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, diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 0f1518cd7a2..1cff65d290d 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/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