From e86c8a0e40f7195de00bba870d19d08f97aa0ada Mon Sep 17 00:00:00 2001 From: Cesium Date: Mon, 23 Sep 2024 17:37:05 -0700 Subject: [PATCH] internationalize default email, minor refactoring, add more UTs --- .../challenge_assignments_controller.rb | 5 +-- app/models/challenge_assignment.rb | 10 ++--- app/models/collection.rb | 30 ++++++------- app/models/potential_match.rb | 6 +-- config/locales/models/en.yml | 7 ++++ .../notification_emails.feature | 42 ++++++++++++++++++- .../step_definitions/email_custom_steps.rb | 28 +++++++++++++ 7 files changed, 99 insertions(+), 29 deletions(-) diff --git a/app/controllers/challenge_assignments_controller.rb b/app/controllers/challenge_assignments_controller.rb index 3f959ec830..1e90132add 100644 --- a/app/controllers/challenge_assignments_controller.rb +++ b/app/controllers/challenge_assignments_controller.rb @@ -219,11 +219,10 @@ def default_all def default @challenge_assignment.defaulted_at = Time.now @challenge_assignment.save - offer_byline = @challenge_assignment.offer_byline - request_byline = @challenge_assignment.request_byline + assignments_page_url = collection_assignments_url(@challenge_assignment.collection) - @challenge_assignment.collection.notify_maintainers_challenge_default(offer_byline, request_byline, assignments_page_url) + @challenge_assignment.collection.notify_maintainers_challenge_default(@challenge_assignment, assignments_page_url) flash[:notice] = "We have notified the collection maintainers that you had to default on your assignment." redirect_to user_assignments_path(current_user) diff --git a/app/models/challenge_assignment.rb b/app/models/challenge_assignment.rb index ab4dee0ff4..80b3773845 100755 --- a/app/models/challenge_assignment.rb +++ b/app/models/challenge_assignment.rb @@ -214,7 +214,7 @@ def offer_byline if offer_signup && offer_signup.pseud offer_signup.pseud.byline else - (pinch_hitter ? (pinch_hitter.byline + "* (pinch hitter)") : "- none -") + (pinch_hitter ? I18n.t("challenge_assignment.offer_byline.pinch_hitter", pinch_hitter_byline: pinch_hitter.byline) : I18n.t("challenge_assignment.offer_byline.none")) end end @@ -222,7 +222,7 @@ def request_byline if request_signup && request_signup.pseud request_signup.pseud.byline else - (pinch_request_signup ? (pinch_request_byline + "* (pinch recipient)") : "- None -") + (pinch_request_signup ? I18n.t("challenge_assignment.request_byline.pinch_recipient", pinch_request_byline: pinch_request_byline) : I18n.t("challenge_assignment.request_byline.none")) end end @@ -387,10 +387,8 @@ def self.delayed_generate(collection_id) end REDIS_GENERAL.del(progress_key(collection)) - if collection.email.present? - UserMailer.potential_match_generation_notification(collection.id, collection.email).deliver_later - elsif collection.parent && collection.parent.email.present? - UserMailer.potential_match_generation_notification(collection.id, collection.parent.email).deliver_later + if collection.collection_email.present? + UserMailer.potential_match_generation_notification(collection.id, collection.collection_email).deliver_later else collection.maintainers_list.each do |user| I18n.with_locale(user.preference.locale.iso) do diff --git a/app/models/collection.rb b/app/models/collection.rb index c36f80ac8e..46732943b4 100755 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -147,7 +147,7 @@ def title too_long: ts("must be less than %{max} characters long.", max: ArchiveConfig.SUMMARY_MAX) } validates :header_image_url, format: { allow_blank: true, with: URI::DEFAULT_PARSER.make_regexp(%w[http https]), message: ts("is not a valid URL.") } - validates :header_image_url, format: { allow_blank: true, with: /\A\S+\.(png|gif|jpg)\z/, message: ts("can only point to a gif, jpg, or png file."), multiline: true } + validates :header_image_url, format: { allow_blank: true, with: /\A\S+\.(png|gif|jpg)\z/, message: ts("can only point to a gif, jpg, or png file.") } validates :tags_after_saving, length: { maximum: ArchiveConfig.COLLECTION_TAGS_MAX, @@ -337,13 +337,16 @@ def maintainers_list self.maintainers.collect(&:user).flatten.uniq end + def collection_email + return self.email if self.email.present? + return parent.email if parent && parent.email.present? + end + def notify_maintainers_assignments_sent subject = I18n.t("user_mailer.collection_notification.assignments_sent.subject") message = I18n.t("user_mailer.collection_notification.assignments_sent.complete") - if self.email.present? - UserMailer.collection_notification(self.id, subject, message, self.email).deliver_later - elsif self.parent && self.parent.email.present? - UserMailer.collection_notification(self.id, subject, message, self.parent.email).deliver_later + if self.collection_email.present? + UserMailer.collection_notification(self.id, subject, message, self.collection_email).deliver_later else # if collection email is not set and collection parent email is not set, loop through maintainers and send each a notice via email self.maintainers_list.each do |user| @@ -356,20 +359,17 @@ def notify_maintainers_assignments_sent end end - def notify_maintainers_challenge_default(offer_byline, request_byline, assignments_page_url) - subject = I18n.t("user_mailer.collection_notification.challenge_default.subject", offer_byline: offer_byline) - message = I18n.t("user_mailer.collection_notification.challenge_default.complete", offer_byline: offer_byline, request_byline: request_byline, assignments_page_url: assignments_page_url) - - if self.email.present? - UserMailer.collection_notification(self.id, subject, message, self.email).deliver_later - elsif self.parent && self.parent.email.present? - UserMailer.collection_notification(self.id, subject, message, self.parent.email).deliver_later + def notify_maintainers_challenge_default(challenge_assignment, assignments_page_url) + if self.collection_email.present? + subject = I18n.t("user_mailer.collection_notification.challenge_default.subject", offer_byline: challenge_assignment.offer_byline) + message = I18n.t("user_mailer.collection_notification.challenge_default.complete", offer_byline: challenge_assignment.offer_byline, request_byline: challenge_assignment.request_byline, assignments_page_url: assignments_page_url) + UserMailer.collection_notification(self.id, subject, message, self.collection_email).deliver_later else # if collection email is not set and collection parent email is not set, loop through maintainers and send each a notice via email self.maintainers_list.each do |user| I18n.with_locale(user.preference.locale.iso) do - translated_subject = I18n.t("user_mailer.collection_notification.challenge_default.subject", offer_byline: offer_byline) - translated_message = I18n.t("user_mailer.collection_notification.challenge_default.complete", offer_byline: offer_byline, request_byline: request_byline, assignments_page_url: assignments_page_url) + translated_subject = I18n.t("user_mailer.collection_notification.challenge_default.subject", offer_byline: challenge_assignment.offer_byline) + translated_message = I18n.t("user_mailer.collection_notification.challenge_default.complete", offer_byline: challenge_assignment.offer_byline, request_byline: challenge_assignment.request_byline, assignments_page_url: assignments_page_url) UserMailer.collection_notification(self.id, translated_subject, translated_message, user.email).deliver_later end end diff --git a/app/models/potential_match.rb b/app/models/potential_match.rb index 4ee894dc5a..0927aeb660 100644 --- a/app/models/potential_match.rb +++ b/app/models/potential_match.rb @@ -90,10 +90,8 @@ def self.generate_in_background(collection_id) if invalid_signup_ids.present? invalid_signup_ids.each { |sid| REDIS_GENERAL.sadd invalid_signup_key(collection), sid } - if collection.email.present? - UserMailer.invalid_signup_notification(collection.id, invalid_signup_ids, collection.email).deliver_later - elsif collection.parent && collection.parent.email.present? - UserMailer.invalid_signup_notification(collection.id, invalid_signup_ids, collection.parent.email).deliver_later + if collection.collection_email.present? + UserMailer.invalid_signup_notification(collection.id, invalid_signup_ids, collection.collection_email).deliver_later else collection.maintainers_list.each do |user| I18n.with_locale(user.preference.locale.iso) do diff --git a/config/locales/models/en.yml b/config/locales/models/en.yml index 83d9a8a6b4..0424c928c2 100644 --- a/config/locales/models/en.yml +++ b/config/locales/models/en.yml @@ -235,6 +235,13 @@ en: other: Works attributes: ticket_number: Ticket ID + challenge_assignment: + offer_byline: + none: "- none -" + pinch_hitter: "%{pinch_hitter_byline}* (pinch hitter)" + request_byline: + none: "- None -" + pinch_recipient: "%{pinch_request_byline}* (pinch recipient)" errors: attributes: ticket_number: diff --git a/features/gift_exchanges/notification_emails.feature b/features/gift_exchanges/notification_emails.feature index edbcc197fb..c36449ba17 100644 --- a/features/gift_exchanges/notification_emails.feature +++ b/features/gift_exchanges/notification_emails.feature @@ -1,7 +1,7 @@ Feature: Gift Exchange Notification Emails Make sure that gift exchange notification emails are formatted properly - Scenario: Assignment notification emails should be sent to two owners in their respective locales + Scenario: Assignment notification emails should be sent to two owners in their respective locales when assignments are generated Given I have created the tagless gift exchange "Holiday Swap" And I open signups for "Holiday Swap" @@ -32,6 +32,46 @@ Feature: Gift Exchange Notification Emails And "participant1" should receive 1 email And "participant2" should receive 1 email + Scenario: If collection email is set, use the collection email instead of moderator emails + Given I have created the tagless gift exchange "Holiday Swap" + And I open signups for "Holiday Swap" + And I am logged in as "participant1" + And I start signing up for "Holiday Swap" + And I press "Submit" + And I am logged in as "participant2" + And I start signing up for "Holiday Swap" + And I press "Submit" + And I have added a co-moderator "mod2" to collection "Holiday Swap" + And I go to "Holiday Swap" collection's page + And I follow "Collection Settings" + And I fill in "Collection email" with "test@archiveofourown.org" + And I press "Update" + And I close signups for "Holiday Swap" + And I have generated matches for "Holiday Swap" + And I have sent assignments for "Holiday Swap" + Then 3 emails should be delivered + And 1 email should be delivered to test@archiveofourown.org + And the email should contain "You have received a message about your collection" + + Scenario: Default notification emails should be sent to two owners in their respective locales when a user defaults on an assignment + + Given everyone has their assignments for "Holiday Swap" + And I have added a co-moderator "mod2" to collection "Holiday Swap" + And a locale with translated emails + And the user "mod1" enables translated emails + + When I am logged in as "myname1" + And I go to my assignments page + And I follow "Default" + Then I should see "We have notified the collection maintainers that you had to default on your assignment." + And 7 emails should be delivered + And "mod1" should receive 2 emails + And the last email to "mod1" should be translated + And the last email should contain "defaulted on their assignment" + And "mod2" should receive 1 email + And the email to "mod2" should be non-translated + And the email should contain "defaulted on their assignment" + Scenario: Assignment notifications with linebreaks. Given I have created the tagless gift exchange "Holiday Swap" And I open signups for "Holiday Swap" diff --git a/features/step_definitions/email_custom_steps.rb b/features/step_definitions/email_custom_steps.rb index b9c3b2932a..e03db12e2d 100644 --- a/features/step_definitions/email_custom_steps.rb +++ b/features/step_definitions/email_custom_steps.rb @@ -27,6 +27,12 @@ step(%{the email to "#{user}" should not contain "translation missing"}) # missing translations in the target language fall back to English end +Then "the last email to {string} should be translated" do |user| + step(%{the last email to "#{user}" should contain "Translated footer"}) + step(%{the last email to "#{user}" should not contain "fan-run and fan-supported archive"}) # untranslated English text + step(%{the last 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"}) @@ -54,6 +60,17 @@ end end +Then "the last email to {string} should contain {string}" do |user, text| + @user = User.find_by(login: user) + email = emails("to: \"#{email_for(@user.email)}\"").last + if email.multipart? + expect(email.text_part.body).to match(text) + expect(email.html_part.body).to match(text) + else + expect(email.body).to match(text) + end +end + 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 @@ -65,6 +82,17 @@ end end +Then "the last email to {string} should not contain {string}" do |user, text| + @user = User.find_by(login: user) + email = emails("to: \"#{email_for(@user.email)}\"").last + if email.multipart? + expect(email.text_part.body).not_to match(text) + expect(email.html_part.body).not_to match(text) + else + expect(email.body).not_to match(text) + end +end + Then "{string} should receive {int} email(s)" do |user, count| @user = User.find_by(login: user) expect(emails("to: \"#{email_for(@user.email)}\"").size).to eq(count.to_i)