Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions app/mailers/user_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -178,32 +178,30 @@ def invite_request_declined(user_id, total, reason)
end
end

# TODO: This may be sent to multiple users simultaneously. We need to ensure
# each user gets the email for their preferred locale.
def collection_notification(collection_id, subject, message)
def collection_notification(collection_id, subject, message, email)
@message = message
@collection = Collection.find(collection_id)
mail(
to: @collection.get_maintainers_email,
to: email,
subject: "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] #{subject}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

)
end

def invalid_signup_notification(collection_id, invalid_signup_ids)
def invalid_signup_notification(collection_id, invalid_signup_ids, email)
@collection = Collection.find(collection_id)
@invalid_signups = invalid_signup_ids
mail(
to: @collection.get_maintainers_email,
to: email,
subject: "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] Invalid sign-ups found"
)
end

# This is sent at the end of matching, i.e., after assignments are generated.
# It is also sent when assignments are regenerated.
def potential_match_generation_notification(collection_id)
def potential_match_generation_notification(collection_id, email)
@collection = Collection.find(collection_id)
mail(
to: @collection.get_maintainers_email,
to: email,
subject: "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] Potential assignment generation complete"
)
end
Expand Down
5 changes: 4 additions & 1 deletion app/models/challenge_assignment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maintainers.each do |i|
UserMailer.potential_match_generation_notification(collection.id, i.email).deliver_later
end
end

# go through the request's potential matches in order from best to worst and try and assign
Expand Down
13 changes: 7 additions & 6 deletions app/models/collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -334,15 +334,16 @@ def prompt_meme?
return self.challenge_type == "PromptMeme"
end

def get_maintainers_email
return self.email if !self.email.blank?
return parent.email if parent && !parent.email.blank?
Comment on lines -338 to -339
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"#{self.maintainers.collect(&:user).flatten.uniq.collect(&:email).join(',')}"
def maintainers_list
@maintainers_list = self.maintainers.collect(&:user).flatten.uniq
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

def notify_maintainers(subject, message)
Copy link
Contributor

@Bilka2 Bilka2 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# send maintainers a notice via email
UserMailer.collection_notification(self.id, subject, message).deliver_later
@maintainers = self.maintainers_list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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
Copy link
Contributor

@Bilka2 Bilka2 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end

include AsyncWithResque
Expand Down
7 changes: 5 additions & 2 deletions app/models/potential_match.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,12 @@ def self.generate_in_background(collection_id)
# check for invalid signups
PotentialMatch.clear_invalid_signups(collection)
invalid_signup_ids = collection.signups.select {|s| !s.valid?}.collect(&:id)
unless invalid_signup_ids.empty?
if invalid_signup_ids.present?
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maintainers.each do |i|
UserMailer.invalid_signup_notification(collection.id, invalid_signup_ids, i.email).deliver_later
end
PotentialMatch.cancel_generation(collection)
else

Expand Down
6 changes: 3 additions & 3 deletions spec/mailers/user_mailer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,7 @@
end

describe "potential_match_generation_notification" do
subject(:email) { UserMailer.potential_match_generation_notification(collection.id) }
subject(:email) { UserMailer.potential_match_generation_notification(collection.id, "[email protected]") }

let(:collection) { create(:collection) }

Expand Down Expand Up @@ -966,7 +966,7 @@
end

describe "invalid_signup_notification" do
subject(:email) { UserMailer.invalid_signup_notification(collection.id, [signup.id]) }
subject(:email) { UserMailer.invalid_signup_notification(collection.id, [signup.id], "[email protected]") }

let(:collection) { create(:collection) }
let(:signup) { create(:challenge_signup) }
Expand Down Expand Up @@ -998,7 +998,7 @@
end

describe "collection_notification" do
subject(:email) { UserMailer.collection_notification(collection.id, subject_text, message_text) }
subject(:email) { UserMailer.collection_notification(collection.id, subject_text, message_text, "[email protected]") }

let(:collection) { create(:collection) }
let(:subject_text) { Faker::Hipster.sentence }
Expand Down
Loading