Re-add dynamic delivery_method for ApplicationMailer #13994
Conversation
pr-triage
bot
added
the
PR: draft
| @@ -9,7 +9,7 @@ def perform(follow_id, mailer = NotifyMailer.name) | |||
|
|
|||
| return if EmailMessage.where(user_id: follow.followable_id) | |||
| .where("sent_at > ?", rand(15..35).hours.ago) | |||
| .exists?(["subject LIKE ?", "%#{NotifyMailer.subjects[:new_follower_email]}"]) | |||
| .exists?(["subject LIKE ?", "%#{NotifyMailer.new.subjects[:new_follower_email]}"]) | |||
There was a problem hiding this comment.
I unintentionally found out that this is a syntax error and this code may have been broken the whole time.
There was a problem hiding this comment.
@maestromac A syntax error? Not a NoMethodError? 😕
There was a problem hiding this comment.
@citizen428 ah yup I meant NoMethodError
| @@ -48,6 +46,6 @@ def set_perform_deliveries | |||
| protected | |||
|
|
|||
| def set_delivery_options | |||
| self.smtp_settings = Settings::SMTP.settings | |||
There was a problem hiding this comment.
Turns out this is the incorrect way to update SMTP config. In spec, checking ActionMailer::Base.deliveries.last.deliver_methods after any email is sent out would return {}.
|
Keeping it draft, for now, to test it on Canary first. |
| @@ -0,0 +1,16 @@ | |||
| module Deliverable | |||
There was a problem hiding this comment.
This is more like a placeholder. Happy to take any naming suggestion here
|
Deployed this to canary1 |
pr-triage
bot
added
PR: unreviewed
pr-triage
bot
added
PR: partially-approved
| end | ||
|
|
||
| def set_delivery_options | ||
| mail.delivery_method.settings.merge!(Settings::SMTP.settings) |
There was a problem hiding this comment.
How does this incorporate SENDGRID_API_KEY?
There was a problem hiding this comment.
Settings::SMTP.settings returns a hardcoded config if SENDGRID_API_KEY exists.
See https://github.com/forem/forem/blob/main/app/models/settings/smtp.rb#L20
| @@ -4,7 +4,7 @@ | |||
| let(:user) { create(:user) } | |||
| let(:email) { VerificationMailer.with(user_id: user.id).account_ownership_verification_email } | |||
|
|
|||
| xdescribe "#set_perform_deliveries" do | |||
| describe "#set_perform_deliveries" do | |||
There was a problem hiding this comment.
nitpick, non-blocking: I think this is a Deliverable test, not specifically an ApplicationMailer test.
pr-triage
bot
added
PR: reviewed-approved
pr-triage
bot
removed
the
PR: reviewed-approved
What type of PR is this? (check all applicable)
Description
The reason my previous attempt in #13943 did not work is most likely due setting
smtp_settingsimproperly in the callback. This PR should fix with by alteringdelivery_method.settingsinstead.Related Tickets & Documents
#13943
QA Instructions, Screenshots, Recordings
n/a
Added/updated tests?
[Forem core team only] How will this change be communicated?
[optional] Are there any post deployment tasks we need to perform?
QA on Canary first.