Skip to content
This repository has been archived by the owner on Jul 24, 2020. It is now read-only.

[1127] Add an ENV variable to log rake emails #1316

Merged
merged 1 commit into from
Oct 20, 2015
Merged

Conversation

esoterik
Copy link
Collaborator

@esoterik esoterik commented Oct 2, 2015

Resolves #1127

this shouldn't be merged until #1203 is merged

@orenyk orenyk changed the title Add an ENV variable to log rake emails [1127] Add an ENV variable to log rake emails Oct 4, 2015
@@ -6,7 +6,8 @@ task email_checkin_reminder: :environment do
Rails.logger.info "Found #{upcoming_reservations.size} reservations due "\
'for check-in. Sending reminder emails...'
upcoming_reservations.each do |upcoming_reservation|
UserMailer.reservation_status_update(upcoming_reservation).deliver
m = UserMailer.reservation_status_update(upcoming_reservation).deliver
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 we can move this into the reservation_status_update method so it doesn't have to be duplicated in all of the rake tasks; the only exception is in the email_notes_to_admins task which uses a separate method.

@orenyk
Copy link
Contributor

orenyk commented Oct 4, 2015

This works fine, but I think we can come up with a DRY-er solution. Is it possible for us to add a method to either both of the Mailer classes (AdminMailer and UserMailer), still slightly duplicated, or even possibly to create an abstract Mailer that inherits from ActionMailer::Base and includes an optional logging method that we then use as the base class for our custom mailers.

@orenyk
Copy link
Contributor

orenyk commented Oct 4, 2015

Ooh, never mind! Rails supports Mailer Observers - that looks perfect to me. We'll basically set up an initializer that only creates and registers the Observer if the environment variable is set - if it's registered it will log the sending of each e-mail, which might be overkill but for debugging purposes seems acceptable to me. There's also an SO Thread on creating Observers. Let me know how that sounds!

@esoterik
Copy link
Collaborator Author

esoterik commented Oct 9, 2015

is it okay if it logs all emails, not just the ones sent by rake task?

@orenyk
Copy link
Contributor

orenyk commented Oct 11, 2015

Yea I think that's fine. This should only be activated for debugging purposes (e.g. we'd need console access to the application server to add it to the .env file), so if we're seeing a specific e-mail problem we can have ITS flip this on for us to see if we can identify what's going on (and turn it off when we're done). Does that sound reasonable to you?

@esoterik
Copy link
Collaborator Author

yup, just wanted to make sure because I'm not sure how to implement this only for specific emails

@esoterik
Copy link
Collaborator Author

this now has tests and is (actually) ready for review!

@@ -49,6 +50,19 @@
expect(ActionMailer::Base.deliveries.count).to eq(1)
end

it 'logs if the env is set' do
Copy link
Contributor

Choose a reason for hiding this comment

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

these are AWESOME, great work! I'll just test this locally and we should be good to go 😄

@orenyk
Copy link
Contributor

orenyk commented Oct 18, 2015

This works! Strangely, it's also logging that an e-mail was sent without the flag sent, even at logger level :info:

Sent mail to cindy_anderson@hahn.name (25.9ms)
  Rendered html template (0.0ms)
  Rendered user_mailer/_overdue.html.erb (3.9ms)
  Rendered user_mailer/reservation_status_update.html.erb (26.3ms)

whereas with the ENV variable set it looks like:

Sent mail to cindy_anderson@hahn.name (20.2ms)
Sent [Reservations] Troune gps auto mount 5447 Overdue to ["cindy_anderson@hahn.name"]
  Rendered html template (0.0ms)
  Rendered user_mailer/_overdue.html.erb (9.5ms)
  Rendered user_mailer/reservation_status_update.html.erb (27.5ms)

The latter is clearly better since it includes the subject, but I'm wondering if the former has something to do with the fact that I'm in the development environment and just simulating the log level of production. In any case, this does what we need and the code looks good, so let's go those commits squashed!

Resolves #1127
  - Emails are logged via an Observer
  - Log message contains subject and addressee
@esoterik
Copy link
Collaborator Author

should be good to go

@orenyk
Copy link
Contributor

orenyk commented Oct 20, 2015

Note to self: it is true that Rails already logs e-mails, but only logs the recipient. The original issue which inspired this (#1125) was a case where the instance was, in fact, not sending the e-mails we were looking for so I didn't realize that this logging was occurring. In any event, having some more details for debugging (the subject, essentially) is still useful. Merging!

orenyk added a commit that referenced this pull request Oct 20, 2015
[1127] Add an ENV variable to log rake emails
@orenyk orenyk merged commit 6099360 into master Oct 20, 2015
@orenyk orenyk deleted the 1127_log_emails branch January 13, 2016 21:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants