diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index ce7ada209d8..62c2229208b 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -162,7 +162,7 @@ en: notify_moderators: chat_pm_title: 'A chat message in "%{channel_name}" requires staff attention' chat_pm_body: "%{link}\n\n%{message}" - + reviewables: reasons: chat_message_queued_by_staff: "A staff member thinks this chat message needs review." @@ -174,14 +174,17 @@ en: other: "You have new chat messages" from: "%{site_name}" subject: - direct_message: - one: "[%{email_prefix}] New message from %{message_title}" - other: "[%{email_prefix}] New messages from %{message_title} and %{others}" - chat_channel: - one: "[%{email_prefix}] New message in %{message_title}" - other: "[%{email_prefix}] New messages in %{message_title} and %{others}" - other_direct_message: "from %{dm_title}" - others: "%{count} others" + direct_message_from_1: "[%{email_prefix}] New message from %{username}" + direct_message_from_2: "[%{email_prefix}] New message from %{username1} and %{username2}" + direct_message_from_more: + one: "[%{email_prefix}] New message from %{username} and %{count} other" + other: "[%{email_prefix}] New message from %{username} and %{count} others" + chat_channel_1: "[%{email_prefix}] New message in %{channel}" + chat_channel_2: "[%{email_prefix}] New message in %{channel1} and %{channel2}" + chat_channel_more: + one: "[%{email_prefix}] New message in %{channel} and %{count} other" + other: "[%{email_prefix}] New message in %{channel} and %{count} others" + chat_channel_and_direct_message: "[%{email_prefix}] New message in %{channel} and from %{username}" unsubscribe: "This chat summary is sent from %{site_link} when you are away. Change your %{email_preferences_link}, or %{unsubscribe_link} to unsubscribe." unsubscribe_no_link: "This chat summary is sent from %{site_link} when you are away. Change your %{email_preferences_link}." view_messages: diff --git a/plugins/chat/lib/extensions/user_notifications_extension.rb b/plugins/chat/lib/extensions/user_notifications_extension.rb index e86d88f7d90..55fd73470f0 100644 --- a/plugins/chat/lib/extensions/user_notifications_extension.rb +++ b/plugins/chat/lib/extensions/user_notifications_extension.rb @@ -60,67 +60,76 @@ module Chat::UserNotificationsExtension end def summary_subject(user, grouped_messages) - channels = grouped_messages.keys - grouped_channels = channels.partition { |c| !c.direct_message_channel? } - non_dm_channels = grouped_channels.first + all_channels = grouped_messages.keys + grouped_channels = all_channels.partition { |c| !c.direct_message_channel? } + channels = grouped_channels.first dm_users = grouped_channels.last.flat_map { |c| grouped_messages[c].map(&:user) }.uniq - total_count_for_subject = non_dm_channels.size + dm_users.size - - # Prioritize messages from regular channels. - first_message_from = non_dm_channels.pop - if first_message_from - first_message_title = first_message_from.title(user) - subject_key = "chat_channel" + # Prioritize messages from regular channels over direct messages + if channels.any? + channel_notification_text(channels, dm_users) else - subject_key = "direct_message" - first_message_from = dm_users.pop - first_message_title = first_message_from.username + direct_message_notification_text(dm_users) end - - subject_opts = { - email_prefix: @email_prefix, - count: total_count_for_subject, - message_title: first_message_title, - others: - other_channels_text( - user, - total_count_for_subject, - first_message_from, - non_dm_channels, - dm_users, - ), - } - - I18n.t(with_subject_prefix(subject_key), **subject_opts) end - def with_subject_prefix(key) - "user_notifications.chat_summary.subject.#{key}" + private + + def channel_notification_text(channels, dm_users) + total_count = channels.size + dm_users.size + + if total_count > 2 + I18n.t( + "user_notifications.chat_summary.subject.chat_channel_more", + email_prefix: @email_prefix, + channel: channels.first.title, + count: total_count - 1 + ) + elsif channels.size == 1 && dm_users.size == 0 + I18n.t( + "user_notifications.chat_summary.subject.chat_channel_1", + email_prefix: @email_prefix, + channel: channels.first.title + ) + elsif channels.size == 1 && dm_users.size == 1 + I18n.t( + "user_notifications.chat_summary.subject.chat_channel_and_direct_message", + email_prefix: @email_prefix, + channel: channels.first.title, + username: dm_users.first.username + ) + elsif channels.size == 2 + I18n.t( + "user_notifications.chat_summary.subject.chat_channel_2", + email_prefix: @email_prefix, + channel1: channels.first.title, + channel2: channels.second.title + ) + end end - def other_channels_text( - user, - total_count, - first_message_from, - other_non_dm_channels, - other_dm_users - ) - return if total_count <= 1 - return I18n.t(with_subject_prefix("others"), count: total_count - 1) if total_count > 2 - - # The summary contains exactly two messages. - if other_non_dm_channels.empty? - second_message_from = other_dm_users.first - second_message_title = second_message_from.username + def direct_message_notification_text(dm_users) + case dm_users.size + when 1 + I18n.t( + "user_notifications.chat_summary.subject.direct_message_from_1", + email_prefix: @email_prefix, + username: dm_users.first.username + ) + when 2 + I18n.t( + "user_notifications.chat_summary.subject.direct_message_from_2", + email_prefix: @email_prefix, + username1: dm_users.first.username, + username2: dm_users.second.username + ) else - second_message_from = other_non_dm_channels.first - second_message_title = second_message_from.title(user) + I18n.t( + "user_notifications.chat_summary.subject.direct_message_from_more", + email_prefix: @email_prefix, + username: dm_users.first.username, + count: dm_users.size - 1 + ) end - - second_message_is_from_channel = first_message_from.class == second_message_from.class - return second_message_title if second_message_is_from_channel - - I18n.t(with_subject_prefix("other_direct_message"), dm_title: second_message_title) end end diff --git a/plugins/chat/spec/mailers/user_notifications_spec.rb b/plugins/chat/spec/mailers/user_notifications_spec.rb index 11d35065d52..f825c5400c3 100644 --- a/plugins/chat/spec/mailers/user_notifications_spec.rb +++ b/plugins/chat/spec/mailers/user_notifications_spec.rb @@ -27,13 +27,11 @@ describe UserNotifications do describe "email subject" do it "includes the sender username in the subject" do - expected_subject = - I18n.t( - "user_notifications.chat_summary.subject.direct_message", - count: 1, - email_prefix: SiteSetting.title, - message_title: sender.username, - ) + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.direct_message_from_1", + email_prefix: SiteSetting.title, + username: sender.username + ) Fabricate(:chat_message, user: sender, chat_channel: channel) email = described_class.chat_summary(user, {}) @@ -49,13 +47,11 @@ describe UserNotifications do chat_channel: channel, ) DirectMessageUser.create!(direct_message: channel.chatable, user: another_participant) - expected_subject = - I18n.t( - "user_notifications.chat_summary.subject.direct_message", - count: 1, - email_prefix: SiteSetting.title, - message_title: sender.username, - ) + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.direct_message_from_1", + email_prefix: SiteSetting.title, + username: sender.username + ) Fabricate(:chat_message, user: sender, chat_channel: channel) email = described_class.chat_summary(user, {}) @@ -64,7 +60,7 @@ describe UserNotifications do expect(email.subject).not_to include(another_participant.username) end - it "includes both channel titles when there are exactly two with unread messages" do + it "includes both usernames when there are exactly two DMs with unread messages" do another_dm_user = Fabricate(:user, group_ids: [chatters_group.id]) refresh_auto_groups another_dm_user.reload @@ -77,17 +73,27 @@ describe UserNotifications do Fabricate(:chat_message, user: sender, chat_channel: channel) email = described_class.chat_summary(user, {}) + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.direct_message_from_2", + email_prefix: SiteSetting.title, + username1: another_dm_user.username, + username2: sender.username + ) + + expect(email.subject).to eq(expected_subject) expect(email.subject).to include(sender.username) expect(email.subject).to include(another_dm_user.username) end it "displays a count when there are more than two DMs with unread messages" do user = Fabricate(:user, group_ids: [chatters_group.id]) + senders = [] 3.times do sender = Fabricate(:user, group_ids: [chatters_group.id]) refresh_auto_groups sender.reload + senders << sender channel = Chat::DirectMessageChannelCreator.create!( acting_user: sender, @@ -101,11 +107,16 @@ describe UserNotifications do Fabricate(:chat_message, user: sender, chat_channel: channel) end - expected_count_text = I18n.t("user_notifications.chat_summary.subject.others", count: 2) - email = described_class.chat_summary(user, {}) - expect(email.subject).to include(expected_count_text) + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.direct_message_from_more", + email_prefix: SiteSetting.title, + username: senders.first.username, + count: 2 + ) + + expect(email.subject).to eq(expected_subject) end it "returns an email if the user is not following the direct channel" do @@ -144,13 +155,11 @@ describe UserNotifications do before { Fabricate(:chat_mention, user: user, chat_message: chat_message) } it "includes the sender username in the subject" do - expected_subject = - I18n.t( - "user_notifications.chat_summary.subject.chat_channel", - count: 1, - email_prefix: SiteSetting.title, - message_title: channel.title(user), - ) + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.chat_channel_1", + email_prefix: SiteSetting.title, + channel: channel.title(user) + ) email = described_class.chat_summary(user, {}) @@ -177,6 +186,14 @@ describe UserNotifications do email = described_class.chat_summary(user, {}) + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.chat_channel_2", + email_prefix: SiteSetting.title, + channel1: channel.title(user), + channel2: another_chat_channel.title(user) + ) + + expect(email.subject).to eq(expected_subject) expect(email.subject).to include(channel.title(user)) expect(email.subject).to include(another_chat_channel.title(user)) end @@ -199,11 +216,17 @@ describe UserNotifications do ) Fabricate(:chat_mention, user: user, chat_message: another_chat_message) end - expected_count_text = I18n.t("user_notifications.chat_summary.subject.others", count: 2) + + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.chat_channel_more", + email_prefix: SiteSetting.title, + channel: channel.title(user), + count: 2 + ) email = described_class.chat_summary(user, {}) - expect(email.subject).to include(expected_count_text) + expect(email.subject).to eq(expected_subject) end end @@ -220,15 +243,16 @@ describe UserNotifications do end it "always includes the DM second" do - expected_other_text = - I18n.t( - "user_notifications.chat_summary.subject.other_direct_message", - dm_title: sender.username, - ) + expected_subject = I18n.t( + "user_notifications.chat_summary.subject.chat_channel_and_direct_message", + email_prefix: SiteSetting.title, + channel: channel.title(user), + username: sender.username + ) email = described_class.chat_summary(user, {}) - expect(email.subject).to include(expected_other_text) + expect(email.subject).to eq(expected_subject) end end end