REFACTOR: Make chat summary email notifications easier to translate (#19354)

plugins/chat/config/locales/server.en.yml

@@ -174,14 +174,17 @@ en:
         other: "You have new chat messages"
       from: "%{site_name}"
       subject:
-        direct_message:
+        direct_message_from_1: "[%{email_prefix}] New message from %{username}"
-          one: "[%{email_prefix}] New message from %{message_title}"
+        direct_message_from_2: "[%{email_prefix}] New message from %{username1} and %{username2}"
-          other: "[%{email_prefix}] New messages from %{message_title} and %{others}"
+        direct_message_from_more:
-        chat_channel:
+          one: "[%{email_prefix}] New message from %{username} and %{count} other"
-          one: "[%{email_prefix}] New message in %{message_title}"
+          other: "[%{email_prefix}] New message from %{username} and %{count} others"
-          other: "[%{email_prefix}] New messages in %{message_title} and %{others}"
+        chat_channel_1: "[%{email_prefix}] New message in %{channel}"
-        other_direct_message: "from %{dm_title}"
+        chat_channel_2: "[%{email_prefix}] New message in %{channel1} and %{channel2}"
-        others: "%{count} others"
+        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:

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
+    all_channels = grouped_messages.keys
-    grouped_channels = channels.partition { |c| !c.direct_message_channel? }
+    grouped_channels = all_channels.partition { |c| !c.direct_message_channel? }
-    non_dm_channels = grouped_channels.first
+    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 over direct messages
-
+    if channels.any?
-    # Prioritize messages from regular channels.
+      channel_notification_text(channels, dm_users)
-    first_message_from = non_dm_channels.pop
-    if first_message_from
-      first_message_title = first_message_from.title(user)
-      subject_key = "chat_channel"
     else
-      subject_key = "direct_message"
+      direct_message_notification_text(dm_users)
-      first_message_from = dm_users.pop
+    end
-      first_message_title = first_message_from.username
   end
 
-    subject_opts = {
+  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,
-      count: total_count_for_subject,
+        channel: channels.first.title,
-      message_title: first_message_title,
+        count: total_count - 1
-      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}"
-  end
-
-  def other_channels_text(
-    user,
-    total_count,
-    first_message_from,
-    other_non_dm_channels,
-    other_dm_users
       )
-    return if total_count <= 1
+    elsif channels.size == 1 && dm_users.size == 0
-    return I18n.t(with_subject_prefix("others"), count: total_count - 1) if total_count > 2
+      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
 
-    # The summary contains exactly two messages.
+  def direct_message_notification_text(dm_users)
-    if other_non_dm_channels.empty?
+    case dm_users.size
-      second_message_from = other_dm_users.first
+    when 1
-      second_message_title = second_message_from.username
+      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
+      I18n.t(
-      second_message_title = second_message_from.title(user)
+        "user_notifications.chat_summary.subject.direct_message_from_more",
-    end
+        email_prefix: @email_prefix,
-
+        username: dm_users.first.username,
-    second_message_is_from_channel = first_message_from.class == second_message_from.class
+        count: dm_users.size - 1
-    return second_message_title if second_message_is_from_channel
+      )
-
+    end
-    I18n.t(with_subject_prefix("other_direct_message"), dm_title: second_message_title)
   end
 end

plugins/chat/spec/mailers/user_notifications_spec.rb

@@ -27,12 +27,10 @@ describe UserNotifications do
 
       describe "email subject" do
         it "includes the sender username in the subject" do
-          expected_subject =
+          expected_subject = I18n.t(
-            I18n.t(
+            "user_notifications.chat_summary.subject.direct_message_from_1",
-              "user_notifications.chat_summary.subject.direct_message",
-              count: 1,
             email_prefix: SiteSetting.title,
-              message_title: sender.username,
+            username: sender.username
           )
           Fabricate(:chat_message, user: sender, chat_channel: channel)
           email = described_class.chat_summary(user, {})
@@ -49,12 +47,10 @@ describe UserNotifications do
             chat_channel: channel,
           )
           DirectMessageUser.create!(direct_message: channel.chatable, user: another_participant)
-          expected_subject =
+          expected_subject = I18n.t(
-            I18n.t(
+            "user_notifications.chat_summary.subject.direct_message_from_1",
-              "user_notifications.chat_summary.subject.direct_message",
-              count: 1,
             email_prefix: SiteSetting.title,
-              message_title: sender.username,
+            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,12 +155,10 @@ describe UserNotifications do
           before { Fabricate(:chat_mention, user: user, chat_message: chat_message) }
 
           it "includes the sender username in the subject" do
-            expected_subject =
+            expected_subject = I18n.t(
-              I18n.t(
+              "user_notifications.chat_summary.subject.chat_channel_1",
-                "user_notifications.chat_summary.subject.chat_channel",
-                count: 1,
               email_prefix: SiteSetting.title,
-                message_title: channel.title(user),
+              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 =
+            expected_subject = I18n.t(
-              I18n.t(
+              "user_notifications.chat_summary.subject.chat_channel_and_direct_message",
-                "user_notifications.chat_summary.subject.other_direct_message",
+              email_prefix: SiteSetting.title,
-                dm_title: sender.username,
+              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