brandonhilkert / sucker_punch

Sucker Punch is a Ruby asynchronous processing library using concurrent-ruby, heavily influenced by Sidekiq and girl_friday.
MIT License
2.64k stars 114 forks source link

Can't send email using v1.0.1 #29

Closed albertogg closed 11 years ago

albertogg commented 11 years ago

Hi, I don't know if I'm missing something but I'm getting a undefined method error every time I try to send an email.

This is the error: from the log (if you need the full stack let me know).

EmailJob crashed!
NoMethodError: undefined method `contact_form' for Celluloid::Notifications:Module
    app/jobs/email_job.rb:6:in `perform' 

Here is the mailer class from app/mailers/notifications.rb

class Notifications < ActionMailer::Base
  default from: "alberto@example.com"

  def contact_form(contact)
    @contact = contact

    mail to: "alberto@example.com",
           subject: contact.subject,
           from: contact.email
  end
end

This is the job from app/jobs/email_job.rb

class EmailJob
  include SuckerPunch::Job

  def perform(contact)
    @contact = contact
    Notifications.contact_form(@contact).deliver
  end

end

This is the controller method

def create
  @contact = Contact.new(params[:contact])

  if @contact.valid?
    EmailJob.new.async.perform(@contact)
    flash[:success] = t('controller.contact.flash.successful')
    redirect_to root_url
  else
    render :action => 'new'
  end
end

Also I tried loggin the contact object send from the controller to the job to see if it's receiving it right and it is totally fine.

jmazzi commented 11 years ago

In your EmailJob, change the line to

::Notifications.contact_form(@contact).deliver On Jul 11, 2013 11:47 PM, "Alberto Grespan" notifications@github.com wrote:

Hi, I don't know if I'm missing something but I'm getting a undefined method error every time I try to send an email.

This is the error: from the log (if you need the full stack let me know).

EmailJob crashed! NoMethodError: undefined method contact_form' for Celluloid::Notifications:Module app/jobs/email_job.rb:6:inperform'

Here is the mailer class from app/mailers/notifications.rb

class Notifications < ActionMailer::Base default from: "alberto@example.com"

def contact_form(contact) @contact = contact

mail to: "alberto@example.com",
       subject: contact.subject,
       from: contact.email

end end

This is the job from app/jobs/email_job.rb

class EmailJob include SuckerPunch::Job

def perform(contact) @contact = contact Notifications.contact_form(@contact).deliver end

end

This is the controller method

def create @contact = Contact.new(params[:contact])

if @contact.valid? EmailJob.new.async.perform(@contact) flash[:success] = t('controller.contact.flash.successful') redirect_to root_url else render :action => 'new' end end

Also I tried loggin the contact object send from the controller to the job to see if it's receiving it right and it is totally fine.

— Reply to this email directly or view it on GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/29 .

albertogg commented 11 years ago

@jmazzi thank you! It's woking... But is it the desired behavior?

jmazzi commented 11 years ago

@albertogg it's possible it's a bug, but I'll let @brandonhilkert comment on that.

In general, you should always prefix toplevel classes with :: or there is no guarantee which class you will get.

brandonhilkert commented 11 years ago

The desired behavior is a loaded topic ;).

I do agree that having to preface the class with :: seems non-obvious. However, including SuckerPunch::Job in your class instantiates it as a Celluloid Actor. That's awesome because Celluloid allows us to do all these awesome things, but also isn't the smallest library in the world and has libraries like Notifications. Had you named your mailer UserMailer or NotificationMailer, you wouldn't have encountered this issue since those classes don't exist in Celluloid and therefore Rails would go looking within your app for them.

albertogg commented 11 years ago

Ok, so both solutions worked out, but I'm staying with @brandonhilkert one, I had the Notifications class renamed to ContactMailer to pair the name with the controller/model and everything worked as expected. Thank you both @jmazzi @brandonhilkert for the help, much appreciate.

brandonhilkert commented 11 years ago

Glad to hear it worked out. I'll make a note in the troubleshooting section.

brandonhilkert commented 11 years ago

https://github.com/brandonhilkert/sucker_punch#class-naming

albertogg commented 11 years ago

@brandonhilkert excellent!