diff --git a/app/mailers/kudo_mailer.rb b/app/mailers/kudo_mailer.rb index f47d34fddb9..6c210c32027 100644 --- a/app/mailers/kudo_mailer.rb +++ b/app/mailers/kudo_mailer.rb @@ -10,20 +10,18 @@ def batch_kudo_notification(user_id, user_kudos) user = User.find(user_id) kudos_hash = JSON.parse(user_kudos) - I18n.with_locale(user.preference.locale.iso) do - kudos_hash.each_pair do |commentable_info, kudo_givers_hash| - # Parse the key to extract the type and id of the commentable so we can - # weed out any commentables that no longer exist. - commentable_type, commentable_id = commentable_info.split("_") - commentable = commentable_type.constantize.find_by(id: commentable_id) - next unless commentable + kudos_hash.each_pair do |commentable_info, _kudo_givers_hash| + # Parse the key to extract the type and id of the commentable so we can + # weed out any commentables that no longer exist. + commentable_type, commentable_id = commentable_info.split("_") + commentable = commentable_type.constantize.find_by(id: commentable_id) + next unless commentable - @commentables << commentable - end - mail( - to: user.email, - subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME) - ) + @commentables << commentable end + mail( + to: user.email, + subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME) + ) end end diff --git a/app/models/redis_mail_queue.rb b/app/models/redis_mail_queue.rb index cd4d0958325..63a23045480 100644 --- a/app/models/redis_mail_queue.rb +++ b/app/models/redis_mail_queue.rb @@ -37,7 +37,9 @@ def self.deliver_kudos # queue the notification for delivery begin # don't die if we hit one deleted user - KudoMailer.batch_kudo_notification(author_id, user_kudos.to_json).deliver_later + I18n.with_locale(User.find(author_id).preference.locale.iso) do + KudoMailer.batch_kudo_notification(author_id, user_kudos.to_json).deliver_later + end rescue end end diff --git a/app/views/kudo_mailer/batch_kudo_notification.html.erb b/app/views/kudo_mailer/batch_kudo_notification.html.erb index 97b1776b1bd..3dded0b680d 100644 --- a/app/views/kudo_mailer/batch_kudo_notification.html.erb +++ b/app/views/kudo_mailer/batch_kudo_notification.html.erb @@ -12,13 +12,13 @@ # kudos are present. givers = names.dup givers << t(".guest", count: guest_count) unless guest_count.zero? - givers_list = givers.map { |k| style_bold(k) }.to_sentence.html_safe + givers_list = to_sentence(givers.map { |k| style_bold(k) }) %> <% if kudo_count == 1 && guest_count == 1 %> - <%= t(".html.single_guest.left_kudos", giver_list: style_bold(t(".html.single_guest.giver_list")).html_safe, commentable_link: commentable_link.html_safe).html_safe %> + <%= t(".single_guest.html", giver: style_bold(t(".single_guest.giver")), commentable_link: commentable_link) %> <% else %> - <%= t(".html.left_kudos", givers_list: givers_list, commentable_link: commentable_link.html_safe, count: kudo_count).html_safe %> + <%= t(".left_kudos.html", givers_list: givers_list, commentable_link: commentable_link, count: kudo_count) %> <% end %> <% if (index < @commentables.length - 1) %> diff --git a/app/views/kudo_mailer/batch_kudo_notification.text.erb b/app/views/kudo_mailer/batch_kudo_notification.text.erb index dd56a2c9415..a79104b9649 100644 --- a/app/views/kudo_mailer/batch_kudo_notification.text.erb +++ b/app/views/kudo_mailer/batch_kudo_notification.text.erb @@ -12,13 +12,13 @@ # present. givers = names.dup givers << t(".guest", count: guest_count) unless guest_count.zero? - givers_list = givers.to_sentence + givers_list = to_sentence(givers) %> <% if kudo_count == 1 && guest_count == 1 %> -<%= t(".text.single_guest", commentable_title: commentable_title, commentable_url: commentable_url) %> +<%= t(".single_guest.text", commentable_title: commentable_title, commentable_url: commentable_url) %> <% else %> -<%= t(".text.left_kudos", givers_list: givers_list, commentable_title: commentable_title, commentable_url: commentable_url, count: kudo_count) %> +<%= t(".left_kudos.text", givers_list: givers_list, commentable_title: commentable_title, commentable_url: commentable_url, count: kudo_count) %> <% end %> <% if (index < @commentables.length - 1) %> diff --git a/config/locales/mailers/en.yml b/config/locales/mailers/en.yml index 84c9570101f..78269dd98d2 100644 --- a/config/locales/mailers/en.yml +++ b/config/locales/mailers/en.yml @@ -31,19 +31,18 @@ en: guest: one: a guest other: "%{count} guests" - html: - left_kudos: + left_kudos: + html: one: "%{givers_list} left kudos on %{commentable_link}." other: "%{givers_list} left kudos on %{commentable_link}." - single_guest: - giver_list: A guest - left_kudos: "%{giver_list} left kudos on %{commentable_link}." - subject: "[%{app_name}] You've got kudos!" - text: - left_kudos: + text: one: '%{givers_list} left kudos on "%{commentable_title}" (%{commentable_url}).' other: '%{givers_list} left kudos on "%{commentable_title}" (%{commentable_url}).' - single_guest: A guest left kudos on "%{commentable_title}" (%{commentable_url}). + single_guest: + giver: A guest + html: "%{giver} left kudos on %{commentable_link}." + text: A guest left kudos on "%{commentable_title}" (%{commentable_url}). + subject: "[%{app_name}] You've got kudos!" mailer: general: closing: diff --git a/features/comments_and_kudos/kudos.feature b/features/comments_and_kudos/kudos.feature index c6f85220ce1..6361c32603f 100644 --- a/features/comments_and_kudos/kudos.feature +++ b/features/comments_and_kudos/kudos.feature @@ -168,6 +168,32 @@ Feature: Kudos And the email should not contain "0 guests" And the email should not contain "translation missing" + Scenario: Translated kudos email + + Given a locale with translated emails + And the user "myname1" enables translated emails + And all emails have been delivered + And the kudos queue is cleared + And I am logged in as "myname2" + And I leave kudos on "Awesome Story" + When kudos are sent + Then 1 email should be delivered to "myname1@foo.com" + And the email should have "Translated subject" in the subject + And the email to "myname1" should contain "myname2" + And the email to "myname1" should contain "Awesome Story" + And the email to "myname1" should be translated + # AO3-6042: Emails should not obey user's locale preference when locales are deactivated + When the locale preference feature flag is disabled for user "myname1" + And all emails have been delivered + And I am logged in as "myname3" + And I leave kudos on "Awesome Story" + And kudos are sent + Then 1 email should be delivered to "myname1@foo.com" + And the email should have "You've got kudos!" in the subject + And the email to "myname1" should contain "myname3" + And the email to "myname1" should contain "Awesome Story" + And the email to "myname1" should be non-translated + Scenario: Blocked users should not see a kudos button on their blocker's works Given the work "Aftermath" by "creator" And the user "creator" has blocked the user "pest" diff --git a/features/step_definitions/email_custom_steps.rb b/features/step_definitions/email_custom_steps.rb index baec4b83c4b..3bc384a72ef 100644 --- a/features/step_definitions/email_custom_steps.rb +++ b/features/step_definitions/email_custom_steps.rb @@ -1,40 +1,70 @@ -Given /^(?:a clear email queue|no emails have been sent|the email queue is clear)$/ do +Given "the email queue is clear" do reset_mailer end -Then /^"([^\"]*)" should be emailed$/ do |user| +Given "a locale with translated emails" do + FactoryBot.create(:locale, iso: "new") + # The footer keys are used in all emails + I18n.backend.store_translations(:new, { mailer: { general: { footer: { general: { about: { html: "Translated footer", text: "Translated footer" } } } } } }) + I18n.backend.store_translations(:new, { kudo_mailer: { batch_kudo_notification: { subject: "Translated subject" } } }) +end + +Given "the user {string} enables translated emails" do |user| + user = User.find_by(login: user) + $rollout.activate_user(:set_locale_preference, user) + user.preference.update!(locale: Locale.find_by(iso: "new")) +end + +Given "the locale preference feature flag is disabled for user {string}" do |user| + user = User.find_by(login: user) + $rollout.deactivate_user(:set_locale_preference, user) +end + +Then "the email to {string} should be translated" do |user| + step(%{the email to "#{user}" should contain "Translated footer"}) + step(%{the email to "#{user}" should not contain "fan-run and fan-supported archive"}) # untranslated English text + step(%{the email to "#{user}" should not contain "translation missing"}) # missing translations in the target language fall back to English +end + +Then "the email to {string} should be non-translated" do |user| + step(%{the email to "#{user}" should not contain "Translated footer"}) + step(%{the email to "#{user}" should contain "fan-run and fan-supported archive"}) + step(%{the email to "#{user}" should not contain "translation missing"}) +end + +Then "{string} should be emailed" do |user| @user = User.find_by(login: user) - emails("to: \"#{email_for(@user.email)}\"").size.should > 0 + expect(emails("to: \"#{email_for(@user.email)}\"")).not_to be_empty end -Then /^"([^\"]*)" should not be emailed$/ do |user| +Then "{string} should not be emailed" do |user| @user = User.find_by(login: user) - emails("to: \"#{email_for(@user.email)}\"").size.should == 0 + expect(emails("to: \"#{email_for(@user.email)}\"")).to be_empty end -Then /^the email to "([^\"]*)" should contain "([^\"]*)"$/ do |user, text| +Then "the email to {string} should contain {string}" do |user, text| @user = User.find_by(login: user) email = emails("to: \"#{email_for(@user.email)}\"").first if email.multipart? - email.text_part.body.should =~ /#{text}/ - email.html_part.body.should =~ /#{text}/ + expect(email.text_part.body).to match(text) + expect(email.html_part.body).to match(text) else - email.body.should =~ /#{text}/ + expect(email.body).to match(text) end end -Then /^the email to "([^\"]*)" should not contain "([^\"]*)"$/ do |user, text| +Then "the email to {string} should not contain {string}" do |user, text| @user = User.find_by(login: user) email = emails("to: \"#{email_for(@user.email)}\"").first if email.multipart? - email.text_part.body.should_not =~ /#{text}/ - email.html_part.body.should_not =~ /#{text}/ + expect(email.text_part.body).not_to match(text) + expect(email.html_part.body).not_to match(text) else - email.body.should_not =~ /#{text}/ + expect(email.body).not_to match(text) end end -Then(/^"([^\"]*)" should receive (\d+) emails?$/) do |user, count| +Then "{string} should receive {int} email(s)" do |user, count| @user = User.find_by(login: user) - emails("to: \"#{email_for(@user.email)}\"").size.should == count.to_i + expect(emails("to: \"#{email_for(@user.email)}\"").size).to eq(count.to_i) end diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 13324a80cd2..970b96f678e 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -1204,54 +1204,4 @@ it_behaves_like "an email with a deleted work with draft chapters attached" end end - - describe "obeys the set locale preference feature flag" do - let(:user) { create(:user) } - let(:work) { create(:work, authors: [user.default_pseud]) } - let(:locale) { create(:locale) } - - context "when the set locale preference feature flag is on" do - before { $rollout.activate_user(:set_locale_preference, user) } - - context "and the user has non-default locale set" do - before { user.preference.update!(locale: locale) } - - it "sends a localised email" do - expect(I18n).to receive(:with_locale).with(locale.iso) - expect(UserMailer.admin_hidden_work_notification(work.id, user.id)).to be_truthy - end - end - - context "and the user has the default locale set" do - before { user.preference.update!(locale: Locale.default) } - - it "sends an English email" do - expect(I18n).to receive(:with_locale).with("en") - expect(UserMailer.admin_hidden_work_notification(work.id, user.id)).to be_truthy - end - end - end - - context "when the set locale preference feature flag is off" do - before { $rollout.deactivate_user(:set_locale_preference, user) } - - context "and the user has non-default locale set" do - before { user.preference.update!(locale: locale) } - - it "sends an English email" do - expect(I18n).to receive(:with_locale).with("en") - expect(UserMailer.admin_hidden_work_notification(work.id, user.id)).to be_truthy - end - end - - context "and the user has the default locale set" do - before { user.preference.update!(locale: Locale.default) } - - it "sends an English email" do - expect(I18n).to receive(:with_locale).with("en") - expect(UserMailer.admin_hidden_work_notification(work.id, user.id)).to be_truthy - end - end - end - end end diff --git a/test/mailers/previews/kudo_mailer_preview.rb b/test/mailers/previews/kudo_mailer_preview.rb new file mode 100644 index 00000000000..f22588f5b05 --- /dev/null +++ b/test/mailers/previews/kudo_mailer_preview.rb @@ -0,0 +1,12 @@ +class KudoMailerPreview < ApplicationMailerPreview + # Sends a kudos notification + def batch_kudo_notification + user = create(:user) + work = create(:work) + guest_count = params[:guest_count] || 1 + user_count = params[:user_count].to_i || 1 + names = Array.new(user_count) { "User#{Faker::Alphanumeric.alpha(number: 8)}" } + hash = { "Work_#{work.id}": { guest_count:, names: } } + KudoMailer.batch_kudo_notification(user.id, hash.to_json) + end +end