Skip to content

ActionMailer renders views even when mail() isn't called #7761

Closed
KieranP opened this Issue Sep 25, 2012 · 17 comments

8 participants

@KieranP
KieranP commented Sep 25, 2012

There currently isn't a graceful way of aborting a mailer. When you try to wrap the mail() call in a conditional, while the email isn't sent if the conditional fails, the view for it is still rendered, which may causes exceptions if certain data isn't found.

The fact that it continues to render the template doesn't seem right, and it can cause issues. I propose that Rails not render the mailer view template unless the mail call is made. Here's a simple use case:

def new_comment(comment_id)
  @comment = Comment.find_by_id(comment_id)
  if @comment
    mail(:to => comment.author, :subject => '...')
  end
end

Currently, the mailer will throw an exception if @comment is used in the mailer view template. Ideally it wouldn't try render the mailers view in this case.

@steveklabnik
Ruby on Rails member

Thanks, but we don't take feature requests on the issues tracker. If you'd like to make this change in Rails, please submit a pull request. If you'd like some feedback before working on the idea, please post to rails-core. Thanks.

@steveklabnik
Ruby on Rails member
@KieranP
KieranP commented Sep 26, 2012

Ok, well, I consider this a bug: If mail() isn't called, it still tries to render the view, which is wrong. I want to be able to wrap mail() in a conditional, and if mail() isn't called, mailer stops.

@rafaelfranca rafaelfranca reopened this Sep 26, 2012
@rafaelfranca
Ruby on Rails member

Reopen to discussion.

cc/ @spastorino @jeremy

@jeremy
Ruby on Rails member
jeremy commented Sep 26, 2012

Makes sense to do nothing if mail isn't called.

@spastorino
Ruby on Rails member

I think it shouldn't be rendering the view if mail is not called

@KieranP
KieranP commented Sep 26, 2012

I've adjusted the original description and title to reflect an issue, rather than a request.

@senny
Ruby on Rails member
senny commented Sep 29, 2012

I tried to reproduce this one with a test case but the test passes on master and I can't reproduce the issue. Can someone look at the test and point out if I am missing the point? I tested against master and 3-2-stable.

Thanks for the feedback.

@jeremy
Ruby on Rails member
jeremy commented Oct 5, 2012

@senny Try calling #deliver on the resulting message, though.

We need to introduce a NullMessage whose #deliver method is just a no-op.

@senny
Ruby on Rails member
senny commented Oct 6, 2012

I added the deliver call. The test now fails with:

test_0055_the view is not rendered when mail was never called(BaseTest):
ArgumentError: A sender (Return-Path, Sender or From) required to send a message
    /Users/senny/.rvm/gems/ruby-1.9.3-p194@rails/gems/mail-2.4.4/lib/mail/network/delivery_methods/smtp.rb:99:in `deliver!'
    /Users/senny/.rvm/gems/ruby-1.9.3-p194@rails/gems/mail-2.4.4/lib/mail/message.rb:2034:in `do_delivery'
    /Users/senny/.rvm/gems/ruby-1.9.3-p194@rails/gems/mail-2.4.4/lib/mail/message.rb:231:in `deliver'
    /Users/senny/Projects/rails/actionmailer/lib/action_mailer/queued_message.rb:33:in `run'
    /Users/senny/Projects/rails/activesupport/lib/active_support/queueing.rb:123:in `run'
    /Users/senny/Projects/rails/activesupport/lib/active_support/queueing.rb:113:in `drain'
    /Users/senny/Projects/rails/activesupport/lib/active_support/queueing.rb:25:in `drain'
    /Users/senny/Projects/rails/activesupport/lib/active_support/queueing.rb:31:in `block in push'
    /Users/senny/Projects/rails/activesupport/lib/active_support/queueing.rb:31:in `tap'
    /Users/senny/Projects/rails/activesupport/lib/active_support/queueing.rb:31:in `push'
    /Users/senny/Projects/rails/actionmailer/lib/action_mailer/queued_message.rb:18:in `block in deliver'
    /Users/senny/Projects/rails/actionmailer/lib/action_mailer/queued_message.rb:18:in `tap'
    /Users/senny/Projects/rails/actionmailer/lib/action_mailer/queued_message.rb:18:in `deliver'
    test/base_test.rb:505:in `block in <class:BaseTest>'

Is this this the problem described in this ticket?

@jeremy Is already someone on the NullMessage refactoring? if not I try to come up with an implementation.

@rafaelfranca
Ruby on Rails member

Closed by a273b6b

@sekrett
sekrett commented Nov 29, 2012

In my application it does not send the message in production env but it sends in development and test.

I call mail() inside another function. When I refactored send_sms_message to return a hash and call mail() inside phone_confirmation, it resolved.

So, this does not work:

class SmsMailer < SmsActionMailer
  helper :application, :sms_mailer

  default from: 'sms@example.com', to: 'sms@example.com'

  def phone_confirmation subscriber
    send_sms_message 'phone_confirmation', subscriber
  end

  protected
  def send_sms_message sms_message_name, subscriber
    @subscriber = subscriber
    @message = view_context.get_message(sms_message_name, subscriber)
    mail(subject: view_context.build_subject_str_for_message(sms_message_name, subscriber), template_name: :sms_message)
  end
end

And this works fine:

class SmsMailer < SmsActionMailer
  helper :application, :sms_mailer

  default from: 'sms@example.com', to: 'sms@example.com'

  def phone_confirmation subscriber
    mail(send_sms_message('phone_confirmation', subscriber))
  end

  protected
  def send_sms_message sms_message_name, subscriber
    @subscriber = subscriber
    @message = view_context.get_message(sms_message_name, subscriber)
    {subject: view_context.build_subject_str_for_message(sms_message_name, subscriber), template_name: :sms_message}
  end
end
@gabceb
gabceb commented Feb 15, 2013

For some reason this still happens for me using Rails 3.2.11.

Code:

def personal_welcome_email(user_id)
    sendgrid_category("Personal welcome email")

    @user = User.where(:id => user_id).first

    if @user && @user.email.present?
      show_email = "#{@user.full_name} <#{@user.email}>"

      mail(:from => "\"XX XX\" <xx@xx.com>", :reply_to => "xx@xx.com", :to => show_email, :subject => "Hey #{@user.first} - let me know if I can do anything to help with xx")
    else
      logger.info "User #{user_id} not found or doesnt exist anymore. Not sending welcome email"
    end

  end

I am using DelayedJob to send this email 1 hour later. I am using actionmailer (= 3.2.11)

image

You can take a look at my logs below and notice how the logic is correct since it says:

User 27243 not found or doesnt exist anymore. Not sending welcome email

image

I will keep digging and try to get it to repro without DJ

@senny
Ruby on Rails member
senny commented Feb 15, 2013

@sekrett is your problem still happening? what version of rails are you using?

@gabceb this is strange. I have to take a look at it and report back. If you happen to have an example app that illustrates the issue that would be wonderful to debug.

@senny
Ruby on Rails member
senny commented Feb 19, 2013

@gabceb I am not able to reproduce your issue. If you are still experiencing the problem, please open a new issue with a description how to reproduce the problem. A small sample application would also do the trick.

@sekrett
sekrett commented Feb 19, 2013

@senny I will not test this now, don't have time. It requires production to reproduce and in production real mail are being sent. I found a workaround solution and I'm happy with it. I have given the code to reproduce, anyone can use it to test if he wants to.

@gabceb
gabceb commented Feb 19, 2013

@senny I will make an sample app and try it out this week. Sorry I didn't get back to you earlier. Thanks for taking a look into it anyways.

@Ricardonacif Ricardonacif referenced this issue in sendwithus/sendwithus_ruby_action_mailer Feb 25, 2016
Merged

Adding perform_delivery option so we can cancel the email sending #32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.