AO3-6268 Fix All challenge owners/moderators get CCed in "Assignments sent" notification #4912

Cesium-Ice · 2024-09-16T07:10:03Z

Pull Request Checklist

Have you read "How to write the perfect pull request"?
Have you read the contributing guidelines?
Have you added tests for any changed functionality?
Have you added the Jira issue number
as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
Do you have fewer than 5 pull requests already open? If not, please wait
until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-6268

Purpose

What does this PR do?

Testing Instructions

How can the Archive's QA team verify that this is working as you intended?

If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.

References

Are there other relevant issues/pull requests/mailing list discussions?

Credit

What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?

Cesium-Ice, they/them
If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.

Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.

…lenge owners get cc-ed on the same email

Cesium-Ice · 2024-09-16T07:49:07Z

Running rubocop locally on the files changed in this PR brings up a lot of changes that the rubocop check doesn't seem to catch, should I commit the changes made by the rubocop auto-corrector as well?

Bilka2

I left some notes. Also, could you write a cucumber scenario where the email notification is sent to two maintainers and translated for one and untranslated for the other? The cucumber steps for translated emails defined in email_custom_steps.rb should be helpful for checking the translation status.

should I commit the changes made by the rubocop auto-corrector as well?

It makes reviewing a lot easier to not have style corrections for unrelated lines. But on the other hand, it would be nice to have a consistent style. If you're up for it, you could make it a separate commit from the rest of the changes.

Bilka2 · 2024-09-17T19:44:31Z

app/models/collection.rb

-    return parent.email if parent && !parent.email.blank?
-    "#{self.maintainers.collect(&:user).flatten.uniq.collect(&:email).join(',')}"
+  def maintainers_list
+    @maintainers_list = self.maintainers.collect(&:user).flatten.uniq


Couldn't this just be returned instead of setting an instance variable?

This both sets it as an instance variable and returns it. Rubocop complains about an unnecessary return when I return without setting the instance variable.

I think rubocop might complain about an extra return keyword, since Ruby returns the last statement by default. Hopefully it should be happy with this though:

Suggested change

@maintainers_list = self.maintainers.collect(&:user).flatten.uniq

self.maintainers.collect(&:user).flatten.uniq

Bilka2 · 2024-09-17T19:46:39Z

app/models/collection.rb

  end

  def notify_maintainers(subject, message)
-    # send maintainers a notice via email
-    UserMailer.collection_notification(self.id, subject, message).deliver_later
+    @maintainers = self.maintainers_list


Is this accessed somewhere later? If not, I think this shouldn't be assigned to an instance variable.

Bilka2 · 2024-09-17T19:49:18Z

app/models/collection.rb

+    @maintainers = self.maintainers_list
+    # loop through maintainers and send each a notice via email
+    @maintainers.each do |i|
+      UserMailer.collection_notification(self.id, subject, message, i.email).deliver_later


To fulfill the Jira issue, please wrap this in a I18n.with_locale call with the users locale, like the other mailers (e.g. the batch_kudo_notification). Also, it would be a bit nicer code style to use something more descriptive like user for the loop variable.

Bilka2 · 2024-09-17T19:52:24Z

app/mailers/user_mailer.rb

    @message = message
    @collection = Collection.find(collection_id)
    mail(
-      to: @collection.get_maintainers_email,
+      to: email,
      subject: "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] #{subject}"


(Regarding your comment on Jira, this is the related issue: The subject locale should include the whole string with the placeholders for the short_name and the collection title, not just the last part.

But not relevant for this PR.)

Bilka2 · 2024-09-17T19:58:38Z

app/models/collection.rb

-    return parent.email if parent && !parent.email.blank?
-    "#{self.maintainers.collect(&:user).flatten.uniq.collect(&:email).join(',')}"
+  def maintainers_list
+    @maintainers_list = self.maintainers.collect(&:user).flatten.uniq
  end

  def notify_maintainers(subject, message)


A bit of an annoying thing: The subject and message currently have their t() calls where this method is called and then the translation is passed in here. But that wont use the locale later when it is set per user with I18n.with_locale. So please move the subject and message translation for where this method is called into the (suggested) I18n.with_locale block.

Maybe they could even be moved to be directly in the UserMailer method.

Bilka2 · 2024-09-17T19:59:51Z

app/models/challenge_assignment.rb

@@ -374,7 +374,10 @@ def self.delayed_generate(collection_id)
      end
    end
    REDIS_GENERAL.del(progress_key(collection))
-    UserMailer.potential_match_generation_notification(collection.id).deliver_later
+    @maintainers = collection.maintainers_list


Is this accessed somewhere later? If not, I think this shouldn't be assigned to an instance variable.

Bilka2 · 2024-09-17T20:00:00Z

app/models/potential_match.rb

      invalid_signup_ids.each {|sid| REDIS_GENERAL.sadd invalid_signup_key(collection), sid}
-      UserMailer.invalid_signup_notification(collection.id, invalid_signup_ids).deliver_later
+      @maintainers = collection.maintainers_list


Is this accessed somewhere later? If not, I think this shouldn't be assigned to an instance variable.

Cesium-Ice · 2024-09-18T08:01:22Z

I left some notes. Also, could you write a cucumber scenario where the email notification is sent to two maintainers and translated for one and untranslated for the other? The cucumber steps for translated emails defined in email_custom_steps.rb should be helpful for checking the translation status.

Do you have suggestions on how to go about setting up a collection with two maintainers in cucumber?

When I am logged in as "mod1"
      And I go to "Holiday Swap" collection's page
      And I follow "Profile"
      And I follow "Membership"
      And I fill in "Add new members" with "participant1"
      And I press "Submit"

This gets as far as adding participant1 as a member but I don't know how to interact with the drop-down selector to change them into a collection owner

Bilka2 · 2024-09-18T08:23:18Z

Do you have suggestions on how to go about setting up a collection with two maintainers in cucumber?

There is a I have added a co-moderator "..." to collection "..." step that should hopefully do the job.

app/models/collection.rb

Cesium-Ice · 2024-09-18T20:59:32Z

I split off the notify_maintainers function because it was being used for different messages and I couldn't move the message translation into the locale block if I didn't know what message was actually going to be received, unsure if this is the best way to go about things

…ent function

Cesium-Ice · 2024-09-18T21:37:20Z

Does anyone know why rubocop seems to be inconsistent about the things it flags as errors vs the things it flags as warnings?

Bilka2 · 2024-09-20T10:20:23Z

Generally rubocop flags most things as warnings, but I have found that the error level doesn't matter much, usually all the things should be fixed. There might be some strangeness going on with the reporting of the warnings in the github actions due to some reviewdog changes, but rubocop itself should be working fine, especially locally.

Bilka2

The approach to split notify_maintainers is a good idea! I left comments regarding that and some other things, which includes some more annoyances with the target email addresses.

Bilka2 · 2024-09-20T10:29:54Z

app/mailers/user_mailer.rb

@@ -92,7 +92,7 @@ def invitation_to_claim(invitation_id, archivist_login)
  end

  # Notifies a writer that their imported works have been claimed
-  def claim_notification(creator_id, claimed_work_ids, is_user=false)
+  def claim_notification(creator_id, claimed_work_ids, is_user = false)


I guess this is an autocorrection from rubocop? The method is getting changed in #4921 so I think it'd be best to revert this change to avoid merge conflicts.

Bilka2 · 2024-09-20T10:57:02Z

app/mailers/user_mailer.rb

-         to: @user.email,
-         subject: "[#{ArchiveConfig.APP_SHORT_NAME}]#{'[' + @collection.title + ']'} Request to include work in a collection"
-    )
+      to: @user.email,
+      subject: "[#{ArchiveConfig.APP_SHORT_NAME}]#{'[' + @collection.title + ']'} Request to include work in a collection"
+       )


It might be easier to avoid this specific rabbithole of rubocop warnings from reviewdog by not changing this unrelated bit of code at all.

Bilka2 · 2024-09-20T10:59:44Z

app/mailers/user_mailer.rb

@@ -160,7 +160,7 @@ def invite_increase_notification(user_id, total)
    I18n.with_locale(@user.preference.locale.iso) do
      mail(
        to: @user.email,
-        subject: "#{t 'user_mailer.invite_increase_notification.subject', app_name: ArchiveConfig.APP_SHORT_NAME}"
+        subject: t("user_mailer.invite_increase_notification.subject", app_name: ArchiveConfig.APP_SHORT_NAME.to_s)


Let's get this to an even better code style:

Suggested change

subject: t("user_mailer.invite_increase_notification.subject", app_name: ArchiveConfig.APP_SHORT_NAME.to_s)

subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME)

Bilka2 · 2024-09-20T11:05:13Z

app/models/challenge_assignment.rb

+       self.request_signup.present? &&
+       self.offer_signup.present? &&
+       !self.request_signup.request_potential_matches.pluck(:offer_signup_id).include?(self.offer_signup_id)


It might be easier to avoid this specific rabbithole of rubocop warnings from reviewdog by not changing this unrelated bit of code at all.

Bilka2 · 2024-09-20T12:03:31Z

app/models/collection.rb

-    return self.email if !self.email.blank?
-    return parent.email if parent && !parent.email.blank?


More annoying things, sorry.

These two lines of code are also needed for all the changed collection email sending. So if the collection email or parent collection email are set, send an untranslated email there. If not, send individual emails to all maintainers translated to the language they have selected.

Bilka2 · 2024-09-20T12:36:29Z

app/controllers/challenge_assignments_controller.rb

+    subject = t("user_mailer.collection_notification.challenge_default.subject", offer_byline: @challenge_assignment.offer_byline )
+    message = t("user_mailer.collection_notification.challenge_default.complete", offer_byline: @challenge_assignment.offer_byline, request_byline: @challenge_assignment.request_byline, assignments_page_link: collection_assignments_url(@challenge_assignment.collection))
+    @challenge_assignment.collection.notify_maintainers(subject, message)


The t() calls here should also be pulled into I18n.with_locale block in the notify_maintainers method, which could maybe be renamed to notify_maintainers_challenge_default to signify that specialization/split of the method.

Bilka2 · 2024-09-20T12:42:29Z

app/controllers/challenge_assignments_controller.rb

-        "Signed-up participant #{@challenge_assignment.offer_byline} has defaulted on their assignment for #{@challenge_assignment.request_byline}. " +
-        "You may want to assign a pinch hitter on the collection assignments page: #{collection_assignments_url(@challenge_assignment.collection)}")
+    subject = t("user_mailer.collection_notification.challenge_default.subject", offer_byline: @challenge_assignment.offer_byline )
+    message = t("user_mailer.collection_notification.challenge_default.complete", offer_byline: @challenge_assignment.offer_byline, request_byline: @challenge_assignment.request_byline, assignments_page_link: collection_assignments_url(@challenge_assignment.collection))


Please use assignments_page_url as the interpolation variable name for collection_assignments_url.

Bilka2 · 2024-09-20T12:43:37Z

features/gift_exchanges/notification_emails.feature

+      And I press "Submit"
+    Then I should see "Sign-up was successfully created."
+
+    Given I have added a co-moderator "mod2" to collection "Holiday Swap"


Suggested change

Given I have added a co-moderator "mod2" to collection "Holiday Swap"

When I have added a co-moderator "mod2" to collection "Holiday Swap"

Bilka2 · 2024-09-20T12:43:46Z

features/gift_exchanges/notification_emails.feature

+    Given 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 close signups for "Holiday Swap"


Suggested change

When I close signups for "Holiday Swap"

And I close signups for "Holiday Swap"

Bilka2 · 2024-09-20T12:46:41Z

features/gift_exchanges/notification_emails.feature

+
+    Then 4 emails should be delivered
+      And "mod1" should receive 1 email
+      And the email to "mod1" should be translated


For the mods it would be nice to check that it's also the right email, e.g. by checking the subject. The subject will be in English for both the translated and non-translated email, because the fake locale used by mod1 doesn't have translations for the subject and falls back to English.

app/models/collection.rb

…ts()

Cesium-Ice · 2024-09-22T08:59:42Z

Sorry for all the commits but it seems like rubocop really wants me to internationalize collection.rb

Cesium-Ice · 2024-09-22T10:10:50Z

app/models/collection.rb

-  validates_length_of :icon_comment_text, allow_blank: true, maximum: ArchiveConfig.ICON_COMMENT_MAX,
-    too_long: ts("must be less than %{max} characters long.", max: ArchiveConfig.ICON_COMMENT_MAX)
+  validates :name, presence: { message: I18n.t("collection.validation.name.presence") }
+  validates :name, uniqueness: { message: I18n.t("collection.validation.name.uniqueness") }


I can add write a migration script to add a database constraint so rubocop stops complaining about this, but it feels kinda weird to do that just to please the linter... should I do that or suppress the warning in some way?

Let's not, that would go completely out of the scope of the original PR. Furthermore, please revert 450dbeb and ec338b0. While the dedication to i18n is appreciated, it would take a lot of extra work to correctly localise the errors and this PR is already quite large. We'd also risk a bug slipping through because manual QA wouldn't test this i18n.

Reviewdog/Rubocop will complain about the ts uses, but that's fine since we're aware that we wont be handling those in this PR.

Ok, I've reverted the requested commits

This reverts commit ec338b0.

…nces of ts()" This reverts commit 450dbeb.

Bilka2

Just a few more comments, thank you for your continued work on this.

Regarding tests, because this has quite a few changes to the "challenge default" email I think it'd be good to test that as well, with a setup similar to the other test you added.

Unfortunately I don't see a test for the fallback email behaviour (preferring the collection email, then the parent collection email) in any of the existing files. I'd appreciate if you could add a test for that as well, just to make sure the emails are getting sent to the right addresses. No need to check for translation behaviour there.

Bilka2 · 2024-09-23T21:46:17Z

app/models/collection.rb

-  validates_format_of :header_image_url, allow_blank: true, with: URI::regexp(%w(http https)), message: ts("is not a valid URL.")
-  validates_format_of :header_image_url, allow_blank: true, with: /\.(png|gif|jpg)$/, message: ts("can only point to a gif, jpg, or png file."), multiline: true
+  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 }


Thanks for the security fix, lets make it even better

Suggested change

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.")}

Bilka2 · 2024-09-23T21:48:14Z

app/controllers/challenge_assignments_controller.rb

+    offer_byline = @challenge_assignment.offer_byline
+    request_byline = @challenge_assignment.request_byline


More i18n fun times! This needs to be inside the with_locale block too, because it creates text that needs to be localised. More on that in the next comment ...

Bilka2 · 2024-09-23T21:53:05Z

app/models/challenge_assignment.rb

+    if offer_signup && offer_signup.pseud
+      offer_signup.pseud.byline
+    else
+      (pinch_hitter ? (pinch_hitter.byline + "* (pinch hitter)") : "- none -")


... for the i18n fun times, this is what needs to be localised. Please use an interpolation variable for the byline and include the formatting (the asterisk and parens and for the other string the dashes) in the localised text.

Same goes for request_byline.

Is there a particular reason why None is capitalized request byline but not offer byline?

Probably not, the previous code is quite old. From what I could find out from talking to people, this rarely shows up in emails, since the codepath is only taken if the user is deleted.

Bilka2 · 2024-09-23T21:56:20Z

app/models/collection.rb

+    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


This email condition (and the other instances of it) could be shortened by making a method that gets the collection email with the fallback to the parent collection email, so you only need one if.

Bilka2

Thank you very much for wrangling this annoying bit of code!

let each challenge owner receive their own email rather than all chal…

25e6ed1

…lenge owners get cc-ed on the same email

github-actions bot added the Awaiting Review label Sep 16, 2024

Cesium-Ice added 3 commits September 16, 2024 00:21

address rubocop

fdfca33

remove redundant return

c8d8fe2

another reviewdog fix

7fb0d0b

Bilka2 suggested changes Sep 17, 2024

View reviewed changes

Bilka2 added Reviewed: Action Needed and removed Awaiting Review labels Sep 17, 2024

rubocop autocorrections

7b51ffd

address Bilka comments

d4da2da

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Sep 18, 2024

github-advanced-security bot found potential problems Sep 18, 2024

View reviewed changes

app/models/collection.rb Fixed Show resolved Hide resolved

Cesium-Ice added 2 commits September 18, 2024 13:53

rubocop linter changes

1f0242e

fix unused translation

f9e91ce

Cesium-Ice added 3 commits September 18, 2024 14:19

yet more rubocop autocorrection changes

5e92d68

fix broken UTs by fixing defining t in notify_maintainers_challenge_s…

a4f6c63

…ent function

rubocop change keyword argument from default = true to default: true

85681cb

Bilka2 suggested changes Sep 20, 2024

View reviewed changes

Bilka2 added Reviewed: Action Needed and removed Coder Has Actioned Review labels Sep 20, 2024

address bilka comments

13bd956

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Sep 22, 2024

github-advanced-security bot found potential problems Sep 22, 2024

View reviewed changes

app/models/collection.rb Fixed Show fixed Hide fixed

Cesium-Ice added 4 commits September 22, 2024 00:48

address rubocop and fix broken cucumber test

cde3a74

will rubocop finally be satisfied?

047a875

see if rubocop will let me get away with not fixing all instances of …

450dbeb

…ts()

rubocop change not about ts

c28acc5

internationalize collection.rb

ec338b0

Cesium-Ice commented Sep 22, 2024

View reviewed changes

Cesium-Ice added 2 commits September 22, 2024 22:10

Revert "internationalize collection.rb"

e05c37b

This reverts commit ec338b0.

Revert "see if rubocop will let me get away with not fixing all insta…

ff9a9c0

…nces of ts()" This reverts commit 450dbeb.

Bilka2 reviewed Sep 23, 2024

View reviewed changes

Bilka2 added Reviewed: Action Needed and removed Coder Has Actioned Review labels Sep 23, 2024

internationalize default email, minor refactoring, add more UTs

e86c8a0

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Sep 24, 2024

Bilka2 approved these changes Sep 24, 2024

View reviewed changes

Bilka2 added Reviewed: Ready to Merge and removed Coder Has Actioned Review labels Sep 24, 2024

	@maintainers_list = self.maintainers.collect(&:user).flatten.uniq
	self.maintainers.collect(&:user).flatten.uniq

	subject: t("user_mailer.invite_increase_notification.subject", app_name: ArchiveConfig.APP_SHORT_NAME.to_s)
	subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME)

		return self.email if !self.email.blank?
		return parent.email if parent && !parent.email.blank?

	Given I have added a co-moderator "mod2" to collection "Holiday Swap"
	When I have added a co-moderator "mod2" to collection "Holiday Swap"

	When I close signups for "Holiday Swap"
	And I close signups for "Holiday Swap"

	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.")}

		offer_byline = @challenge_assignment.offer_byline
		request_byline = @challenge_assignment.request_byline

AO3-6268 Fix All challenge owners/moderators get CCed in "Assignments sent" notification #4912

Are you sure you want to change the base?

AO3-6268 Fix All challenge owners/moderators get CCed in "Assignments sent" notification #4912

Conversation

Cesium-Ice commented Sep 16, 2024 • edited Loading

Pull Request Checklist

Issue

Purpose

Testing Instructions

References

Credit

Cesium-Ice commented Sep 16, 2024

Bilka2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cesium-Ice commented Sep 18, 2024

Bilka2 commented Sep 18, 2024

Cesium-Ice commented Sep 18, 2024

Cesium-Ice commented Sep 18, 2024 • edited Loading

Bilka2 commented Sep 20, 2024

Bilka2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cesium-Ice commented Sep 22, 2024

Choose a reason for hiding this comment

Bilka2 Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 left a comment

Choose a reason for hiding this comment

Cesium-Ice commented Sep 16, 2024 •

edited

Loading

Bilka2 Sep 17, 2024 •

edited

Loading

Bilka2 Sep 17, 2024 •

edited

Loading

Cesium-Ice commented Sep 18, 2024 •

edited

Loading

Bilka2 Sep 20, 2024 •

edited

Loading

Bilka2 Sep 22, 2024 •

edited

Loading

Bilka2 left a comment •

edited

Loading

Bilka2 Sep 24, 2024 •

edited

Loading