renz45 / mandrill_mailer

A small gem for sending Mandrill template emails
260 stars 84 forks source link

Delay throwing warning in test_setup_for blocks if email option is missing #122

Closed chrisbloom7 closed 8 years ago

chrisbloom7 commented 8 years ago

I have some mailers that will always go to an internal email address, and so I don't pass in any recipient info. In my test_setup_for block for one such mailer I was attempting to call it without any email option:

# app/mailers/application_mailer.rb
test_setup_for :register_hold do |mailer, options|
  trial_form = mock(company_name: 'Veridian Dynamics', company_subdomain: 'veridian', email: 'admin@veridian.com',
                    first_name: 'Veridian', last_name: 'Dynamics')
  mailer.register_hold(trial_form).deliver
end

def register_hold(trial_form)
  # ...
  params[:to] = 'registrations@youearnedit.com'
  mandrill_mail(params)
end

# Console
> ApplicationMailer.test(:register_hold)
MandrillMailer::CoreMailer::InvalidEmail: Please specify a :email option(email to send the test to)
    from /data/app/shared/bundled_gems/ruby/2.3.0/gems/mandrill_mailer-1.3.0/lib/mandrill_mailer/core_mailer.rb:204:in `test'
    from (irb):1
    from /data/app/shared/bundled_gems/ruby/2.3.0/gems/railties-4.2.5.2/lib/rails/commands/console.rb:110:in `start'
    from /data/app/shared/bundled_gems/ruby/2.3.0/gems/railties-4.2.5.2/lib/rails/commands/console.rb:9:in `start'
    from /data/app/shared/bundled_gems/ruby/2.3.0/gems/railties-4.2.5.2/lib/rails/commands/commands_tasks.rb:68:in `console'
    from /data/app/shared/bundled_gems/ruby/2.3.0/gems/railties-4.2.5.2/lib/rails/commands/commands_tasks.rb:39:in `run_command!'
    from /data/app/shared/bundled_gems/ruby/2.3.0/gems/railties-4.2.5.2/lib/rails/commands.rb:17:in `<top (required)>'
    from bin/rails:8:in `require'
    from bin/rails:8:in `<main>'

This error happens whether config.mandrill_mailer.delivery_method = :test is set or not. Ideally it would be better to delay a warning about a missing recipient until after the mailer is constructed and about to be sent. The only way around this is to call the test method with an email option, even if only an empty string, which is unintuitive and leads to additional documentation about why a user needs to include it even if it will be ignored.

chrisbloom7 commented 8 years ago

ApplicationMailer.test(:register_hold, email: '') is valid, but ApplicationMailer.test(:register_hold, email: nil) is not.

renz45 commented 8 years ago

Hi Chris, I think the reason I originally had that error in there was that in our use case at Code School, we had front end devs/designers triggering these emails, so they needed the additional feedback to make sure they changed the email address. The idea behind the testing code is to send a test email to a test/dummy account, instead of the production ready email address. This was because if the app isn't in production, you wouldn't want to send an email there.

I might suggest hard coding that email (or pulling it from a config) in your registrations model and using it in the mailer call. The mailer then retains the functionality of a mailer, where an email is taken in, rather then hard coding it at the mailer and changing it's functionality compared to other mailers.

I'm not sure I want to change the way this error works, considering it's more of a production email endpoint guard then anything 😅

chrisbloom7 commented 8 years ago

I can see that being useful, although if I'm sending a test from a console on my production or staging environment I'm not sure I would expect anything different to happen, or if I did that's where an interceptor would come into play. Thanks for the quick response. I'll just add a note to my documentation.

renz45 commented 8 years ago

Sorry for the confusion, yea the interceptors were added after most of the original functionality. I can see how those would affect some architectural decisions in a situations like this. Feel free to reopen this if you have any other questions.