Make BroadcastMailer send individual emails rather than batching #162

Merged
merged 3 commits into from Aug 23, 2013
View
@@ -37,6 +37,8 @@ gem 'daemons', :require => false
gem 'whenever'
gem 'rails_setup'
+gem 'mustache'
+
gem 'mixpanel'
group :development do
View
@@ -146,6 +146,7 @@ GEM
multi_json (1.7.1)
multi_xml (0.5.3)
multipart-post (1.2.0)
+ mustache (0.99.4)
net-scp (1.0.4)
net-ssh (>= 1.99.1)
net-sftp (2.0.5)
@@ -296,6 +297,7 @@ DEPENDENCIES
mixpanel
mocha
multi_xml (>= 0.5.2)
+ mustache
nokogiri
omniauth-github (~> 1.0.1)
omniauth-oauth2 (~> 1.1.1)
@@ -12,10 +12,15 @@ def create
render(:new) && return
end
- BroadcastMailer.deliver_broadcast(params)
+ if params[:commit] == "Test"
+ Broadcaster.notify_testers(params)
+ else
+ Broadcaster.notify_subscribers(params)
+ end
+
flash[:notice] = "Message sent"
redirect_to :action => :new
end
end
-end
+end
@@ -1,24 +1,14 @@
class BroadcastMailer < ActionMailer::Base
- def deliver_broadcast(message={})
- @body = message[:body]
-
- user_batches(message) do |users|
- mail(
- :to => "gregory@practicingruby.com",
- :bcc => users,
- :subject => message[:subject]
- ).deliver
- end
+ def self.recipients
+ User.where(:notify_updates => true).to_notify.map(&:contact_email)
end
- private
+ def broadcast(message, email)
+ article_finder = ->(e) { article_url(Article[e]) }
- def user_batches(message)
- yield(message[:to]) && return if message[:commit] == "Test"
+ @body = Mustache.render(message[:body], :article => article_finder)
- User.where(:notify_updates => true).to_notify.
- find_in_batches(:batch_size => 25) do |group|
- yield group.map(&:contact_email)
- end
+ mail(:to => email,
+ :subject => message[:subject])
end
end
@@ -0,0 +1,11 @@
+class Broadcaster
+ def self.notify_subscribers(params)
+ BroadcastMailer.recipients.each do |email|
+ BroadcastMailer.broadcast(params, email).deliver
+ end
+ end
+
+ def self.notify_testers(params)
+ BroadcastMailer.broadcast(params, params[:to]).deliver
+ end
+end
@@ -61,7 +61,12 @@ class Application < Rails::Application
# Action Mailer Defaults
config.action_mailer.delivery_method = :mailhopper
- config.action_mailer.default_url_options = { :host => "practicingruby.com" }
+
+ url_options = { :host => "practicingruby.com" }
+
+ config.action_mailer.default_url_options = url_options
+ Rails.application.routes.default_url_options = url_options
+
ActionMailer::Base.default :from => "Practicing Ruby <gregory@practicingruby.com>"
config.generators do |g|
@@ -24,6 +24,7 @@
# The :test delivery method accumulates sent emails in the
# ActionMailer::Base.deliveries array.
config.action_mailer.delivery_method = :test
+ config.action_mailer.raise_delivery_errors = true
# Use SQL instead of Active Record's schema dumper when creating the test database.
# This is necessary if your schema can't be completely dumped by the schema dumper,
@@ -1,24 +1,31 @@
-require 'test_helper'
+require_relative '../test_helper'
class BroadcastMessagesTest < ActionDispatch::IntegrationTest
setup do
ActionMailer::Base.deliveries.clear
- @user = FactoryGirl.create(:user, :admin => true)
- FactoryGirl.create(:authorization, :user => @user)
+ @admin = FactoryGirl.create(:user, :admin => true)
+ FactoryGirl.create(:authorization, :user => @admin)
end
test "messages are broadcast to users" do
+ users = 3.times.map { FactoryGirl.create(:user) } + [@admin]
+
subject = "Weekly Update 1.5"
body = 100.times.map { "Long body content is long" }.join
- send_message :subject => subject, :body => body
- message = ActionMailer::Base.deliveries.first
+ send_message(:subject => subject, :body => body)
+ messages = ActionMailer::Base.deliveries
- assert_equal subject, message.subject
- assert message.bcc.include?(@user.contact_email),
- "User missing from broadcast message"
+ users.each do |user|
+ message = messages.find { |msg| msg.to == [user.contact_email] }
+
+ assert message, "unable to find message for #{user.contact_email}"
+
+ assert_equal subject, message.subject
+ assert message.body.to_s[body]
+ end
end
test "only sends messages to users who have opted in" do
@@ -27,20 +34,21 @@ class BroadcastMessagesTest < ActionDispatch::IntegrationTest
send_message
- message = ActionMailer::Base.deliveries.first
+ ActionMailer::Base.deliveries.each do |message|
+ refute message.to.include?(no_updates_user.contact_email),
+ "User was sent a broadcast message and they didn't want it"
- refute message.bcc.include?(no_updates_user.contact_email),
- "User was sent a broadcast message and they didn't want it"
- refute message.bcc.include?(nothing_user.contact_email),
- "User was sent a broadcast message and their notfications are disabled"
+ refute message.to.include?(nothing_user.contact_email),
+ "User was sent a broadcast message and their notifications are disabled"
+ end
end
test "sending test copies" do
send_message(:to => "support@elmcitycraftworks.com", :button => "Test")
message = ActionMailer::Base.deliveries.first
- assert_equal ["support@elmcitycraftworks.com"], message.bcc
+ assert_equal ["support@elmcitycraftworks.com"], message.to
end
private
@@ -54,6 +62,13 @@ def send_message(options = {})
fill_in 'Subject', :with => options[:subject] || "Subject"
fill_in 'Body', :with => options[:body] || "Body"
- click_button options[:button] || "Send"
+ case options[:button]
+ when "Test"
+ click_button "Test"
+ when nil
+ click_button "Send"
+ else
+ raise NotImplementedError, "Don't know how to click #{options[:button]}"
+ end
end
end
@@ -1,35 +1,42 @@
-require 'test_helper'
+require_relative '../../test_helper'
class BroadcastMailerTest < ActionMailer::TestCase
- test "emails should not be escaped" do
- assert ActionMailer::Base.deliveries.empty?
-
- BroadcastMailer.deliver_broadcast(:body => "It's working",
- :subject => "TEST",
- :to => "test@test.com",
- :commit => "Test")
-
- message = ActionMailer::Base.deliveries.first
-
- assert message.body.to_s[/\AIt's working\n/]
- end
-
test "users without confirmed emails are not notified" do
do_not_mail = %w{authorized pending_confirmation}.map do |status|
FactoryGirl.create(:user, :status => status)
end
5.times { FactoryGirl.create(:user) }
- BroadcastMailer.deliver_broadcast(:body => "Only you can see this",
- :subject => "TEST",
- :to => "test@test.com")
+ Broadcaster.notify_subscribers(:body => "Only you all can see this",
+ :subject => "TEST")
- message = ActionMailer::Base.deliveries.first
+ messages = ActionMailer::Base.deliveries
+
+ assert_equal 5, messages.count
do_not_mail.each do |user|
- refute message.bcc.include?(user.contact_email),
- "User with status '#{user.status}' was sent a broadcast"
+ refute messages.any? { |m| m.to.include?(user.contact_email) },
+ "User with status '#{user.status}' was sent a broadcast"
end
end
+
+
+ test "article links can be generated from template" do
+ FactoryGirl.create(:user)
+
+ article = FactoryGirl.create(:article, :slug => "a-fancy-article")
+
+ message_body = "Here's an amazing article\n{{#article}}a-fancy-article{{/article}}"
+
+ Broadcaster.notify_subscribers(:body => message_body, :subject => "Hi there!")
+
+ message = ActionMailer::Base.deliveries.first
+
+ url = Rails.application.routes.url_helpers.article_url(article)
+
+ expected_body = "Here's an amazing article\n#{url}"
+
+ assert message.body.to_s[expected_body], "Expected link to be expanded, instead got: #{message.body.to_s}"
+ end
end