New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuration of ActionMailer queue name #18587

Merged
merged 1 commit into from Jun 3, 2015

Conversation

Projects
None yet
10 participants
@chrismcg
Contributor

chrismcg commented Jan 18, 2015

Currently all mails sent with deliver_later are put on the mailers queue. This patch keeps that default but allows the queue name to be configured globally.

The motivation was trying to use https://github.com/chanks/que as the queuing backend. It's a good fit for a simple queue for a single dyno heroku app but by default only listens to one queue per application. As ActiveJob will use the default queue and ActionMailer the mailers queue jobs wouldn't get run.

The Que library might change to support more than one queue but in the meantime this will allow Rails to work with it. It will also be useful for users of other backends who might need to change where mails are queued.

require "#{app_path}/config/environment"
require "mail"
_ = ActionMailer::Base

This comment has been minimized.

@vipulnsward

vipulnsward Jan 20, 2015

Member

Whats this for?

@vipulnsward

vipulnsward Jan 20, 2015

Member

Whats this for?

This comment has been minimized.

@chrismcg

chrismcg Jan 28, 2015

Contributor

For testing I could set the configuration separately from using it.

@chrismcg

chrismcg Jan 28, 2015

Contributor

For testing I could set the configuration separately from using it.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jan 28, 2015

Member

We need to configure the default AM queue as well in Queue Classic.

Member

senny commented Jan 28, 2015

We need to configure the default AM queue as well in Queue Classic.

@mandrews

This comment has been minimized.

Show comment
Hide comment
@mandrews

mandrews commented Jan 31, 2015

+1

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 12, 2015

Member

I'm thinking of a cleaner approach. ActiveJob::Base accepts a Proc for the queue_as and that proc is executed when trying to enqueue the job so we can leverage that:

  class DeliveryJob < ActiveJob::Base # :nodoc:
    queue_as do
      ActionMailer::Base. deliver_later_queue_name || :mailers
    end
  end

what do you think @chrismcg?

Member

cristianbica commented Feb 12, 2015

I'm thinking of a cleaner approach. ActiveJob::Base accepts a Proc for the queue_as and that proc is executed when trying to enqueue the job so we can leverage that:

  class DeliveryJob < ActiveJob::Base # :nodoc:
    queue_as do
      ActionMailer::Base. deliver_later_queue_name || :mailers
    end
  end

what do you think @chrismcg?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 12, 2015

Member

@cristianbica I'm not sure I understand the benefit of that approach. Why not set :mailers as the default for that option? This way you can at least use introspection on deliver_later_queue_name to see where the jobs are going. Otherwise the default is hidden away.

Member

senny commented Feb 12, 2015

@cristianbica I'm not sure I understand the benefit of that approach. Why not set :mailers as the default for that option? This way you can at least use introspection on deliver_later_queue_name to see where the jobs are going. Otherwise the default is hidden away.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 12, 2015

Member

@senny yeah the || :mailers makes no sense here but my main suggesting was to keep defining the deliver_later_queue_name on ActionMailer base use use that in the DeliveryJob's queue_as. In this PR implementation the queue name is set in the MessageDelivery wrapper.

Member

cristianbica commented Feb 12, 2015

@senny yeah the || :mailers makes no sense here but my main suggesting was to keep defining the deliver_later_queue_name on ActionMailer base use use that in the DeliveryJob's queue_as. In this PR implementation the queue name is set in the MessageDelivery wrapper.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 12, 2015

Member

@cristianbica 👍 got it.

Member

senny commented Feb 12, 2015

@cristianbica 👍 got it.

@chrismcg

This comment has been minimized.

Show comment
Hide comment
@chrismcg

chrismcg Feb 23, 2015

Contributor

@cristianbica sorry, I've only just seen your comment. I didn't know you could use a proc there so I'll have a look at it later in the week when I've a bit more time.

Contributor

chrismcg commented Feb 23, 2015

@cristianbica sorry, I've only just seen your comment. I didn't know you could use a proc there so I'll have a look at it later in the week when I've a bit more time.

@jakegavin

This comment has been minimized.

Show comment
Hide comment
@jakegavin

jakegavin Mar 27, 2015

Contributor

+1
Just came across this while upgrading to 4.2. This would be nice to have. Thanks for working on it.

Contributor

jakegavin commented Mar 27, 2015

+1
Just came across this while upgrading to 4.2. This would be nice to have. Thanks for working on it.

@chrismcg

This comment has been minimized.

Show comment
Hide comment
@chrismcg

chrismcg May 19, 2015

Contributor

@cristianbica finally got around to doing that change. I think it's clearer this way.

Contributor

chrismcg commented May 19, 2015

@cristianbica finally got around to doing that change. I think it's clearer this way.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica May 20, 2015

Member

Please squash your commits. Otherwise seems good to me.

Member

cristianbica commented May 20, 2015

Please squash your commits. Otherwise seems good to me.

@chrismcg

This comment has been minimized.

Show comment
Hide comment
@chrismcg

chrismcg Jun 2, 2015

Contributor

Commits squashed, thanks!

Contributor

chrismcg commented Jun 2, 2015

Commits squashed, thanks!

@rafaelfranca rafaelfranca merged commit f5a131a into rails:master Jun 3, 2015

rafaelfranca added a commit that referenced this pull request Jun 3, 2015

Merge pull request #18587 from chrismcg/allow_deliver_later_queue_nam…
…e_to_be_configured

Allow configuration of ActionMailer queue name

rafaelfranca added a commit that referenced this pull request Jun 3, 2015

Document config.action_mailer.deliver_later_queue_name
Also add a CHANGELOG entry for #18587

[ci skip]
@michaelsauter

This comment has been minimized.

Show comment
Hide comment
@michaelsauter

michaelsauter Oct 21, 2015

Is there a specific reason why this didn't make it into 4.2.4?

Judging by this PR, I thought it would be included since this has been merged on June 3, but 4.2.4 wasn't released until August 24.

michaelsauter commented Oct 21, 2015

Is there a specific reason why this didn't make it into 4.2.4?

Judging by this PR, I thought it would be included since this has been merged on June 3, but 4.2.4 wasn't released until August 24.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Oct 21, 2015

Member

@michaelsauter new features aren't backported to bugfix releases.

Member

matthewd commented Oct 21, 2015

@michaelsauter new features aren't backported to bugfix releases.

@michaelsauter

This comment has been minimized.

Show comment
Hide comment
@michaelsauter

michaelsauter Oct 21, 2015

@matthewd Ah okay, thanks. Though I think this almost counts as a bugfix ;)

michaelsauter commented Oct 21, 2015

@matthewd Ah okay, thanks. Though I think this almost counts as a bugfix ;)

@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Dec 30, 2015

@rossinkiwi

This comment has been minimized.

Show comment
Hide comment
@rossinkiwi

rossinkiwi Aug 25, 2017

I'm trying to set up separate mailer queues for transaction mail processing and bulk mail processing in my Rails 5.1.3 application. I was hoping to be able to designate the queue by setting the deliver_later_queue_name for each of my inherited ActionMailer's:

class ApplicationMailer < ActionMailer::Base
end

class UserMailer < ApplicationMailer  self.deliver_later_queue_name = 'mail'
  self.deliver_later_queue_name = 'mail'
  self.smtp_settings = {
    address: "smtp.sendgrid.net",
    port: 587,
    ...
}

class BulkMailer < ApplicationMailer
  self.deliver_later_queue_name = 'bulkmail'
  self.smtp_settings = {
    address: "email-smtp.us-west-2.amazonaws.com",
    port: 587,
    ...
  }

This works fine for the smtp_settings, but the deliver_later_queue_name appears to be a class variable? and so when I set it in the BulkMailer or UserMailer, it changes it for all the ActionMailer's.

Is there a way for me to set separate deliver_later_queue_name's for my BulkMailer and UserMailer?

rossinkiwi commented Aug 25, 2017

I'm trying to set up separate mailer queues for transaction mail processing and bulk mail processing in my Rails 5.1.3 application. I was hoping to be able to designate the queue by setting the deliver_later_queue_name for each of my inherited ActionMailer's:

class ApplicationMailer < ActionMailer::Base
end

class UserMailer < ApplicationMailer  self.deliver_later_queue_name = 'mail'
  self.deliver_later_queue_name = 'mail'
  self.smtp_settings = {
    address: "smtp.sendgrid.net",
    port: 587,
    ...
}

class BulkMailer < ApplicationMailer
  self.deliver_later_queue_name = 'bulkmail'
  self.smtp_settings = {
    address: "email-smtp.us-west-2.amazonaws.com",
    port: 587,
    ...
  }

This works fine for the smtp_settings, but the deliver_later_queue_name appears to be a class variable? and so when I set it in the BulkMailer or UserMailer, it changes it for all the ActionMailer's.

Is there a way for me to set separate deliver_later_queue_name's for my BulkMailer and UserMailer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment