Skip to content

Commit 4115014

Browse files
committed
deliver messages to all recipients in one job, closes instructure#1
If any individual message delivery fails, we'll reschedule it as its own individual job. Change-Id: I51ae5941fd001c61e6c6b708185ff12585d0a49f Reviewed-on: https://gerrit.instructure.com/2390 Tested-by: Hudson <hudson@instructure.com> Reviewed-by: Bracken Mosbacker <bracken@instructure.com>
1 parent b86fb7f commit 4115014

5 files changed

Lines changed: 46 additions & 12 deletions

File tree

app/models/message.rb

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,12 +140,12 @@ def self.old_dashboard
140140
{ :conditions => {:workflow_state => state.to_s } }
141141
end
142142
}
143-
143+
144144
workflow do
145145
state :created do
146146
event :stage, :transitions_to => :staged do
147147
self.dispatch_at = Time.now.utc + self.delay_for
148-
if self.to != 'dashboard'
148+
if self.to != 'dashboard' && !@stage_without_dispatch
149149
MessageDispatcher.dispatch(self)
150150
end
151151
end
@@ -199,6 +199,12 @@ def self.old_dashboard
199199
end
200200

201201
end
202+
203+
# skip dispatching the message during the stage transition, useful when batch
204+
# dispatching.
205+
def stage_without_dispatch!
206+
@stage_without_dispatch = true
207+
end
202208

203209
# Sets a few defaults and gets it on its way to be dispatched.
204210
# The path: created -> staged -> sending -> sent

app/models/notification.rb

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -213,18 +213,20 @@ def create_message(asset, *tos)
213213
end
214214
end
215215
@delayed_messages_to_save.each{|m| m.save! }
216-
217216

218-
messages.each{|m|
219-
if m.to != 'dashboard'
220-
m.save!
221-
elsif Notification.types_to_show_in_feed.include?(self.name)
217+
dashboard_messages, dispatch_messages = messages.partition { |m| m.to == 'dashboard' }
218+
219+
dashboard_messages.each do |m|
220+
if Notification.types_to_show_in_feed.include?(self.name)
222221
m.set_asset_context_code
223222
m.infer_defaults
224223
m.create_stream_items
225224
end
226-
}
227-
225+
end
226+
227+
dispatch_messages.each { |m| m.stage_without_dispatch!; m.save! }
228+
MessageDispatcher.batch_dispatch(dispatch_messages)
229+
228230
# re-set cached values
229231
@user_counts.each{|user_id, cnt| recent_messages_for_user(user_id, cnt) }
230232

lib/message_dispatcher.rb

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,36 @@ def self.dispatch(message)
2323
:run_at => message.dispatch_at)
2424
end
2525

26+
def self.batch_dispatch(messages)
27+
return if messages.empty?
28+
29+
if messages.size == 1
30+
self.dispatch(messages.first)
31+
return
32+
end
33+
34+
dispatch_at = messages.first.dispatch_at
35+
36+
Delayed::Job.enqueue(self.new(self, :deliver_batch, [messages]),
37+
:run_at => messages.first.dispatch_at)
38+
end
39+
2640
# Called by delayed_job when a job fails to reschedule it.
2741
def reschedule_at(now, num_attempts)
2842
live_object.dispatch_at
2943
end
3044

45+
protected
46+
47+
def self.deliver_batch(messages)
48+
messages.each do |message|
49+
begin
50+
message.deliver
51+
rescue
52+
# this delivery failed, we'll have to make an individual job to retry
53+
self.dispatch(message)
54+
end
55+
end
56+
end
57+
3158
end

spec/models/wiki_page_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
p.messages_sent.should_not be_empty
4646
p.messages_sent["Updated Wiki Page"].should_not be_nil
4747
p.messages_sent["Updated Wiki Page"].should_not be_empty
48-
p.messages_sent["Updated Wiki Page"].map(&:user).should be_include(@user) #[0].user.should eql(@user)
48+
p.messages_sent["Updated Wiki Page"].map(&:user).should be_include(@user)
4949
end
5050

5151
context "atom" do

vendor/plugins/broadcast_policy/lib/broadcast_policy.rb

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,7 @@ def broadcast(record)
108108
to_list = Array[to_list].flatten
109109
n = DelayedNotification.send_later_if_production(:process, record, notification, (to_list || []).compact.map(&:asset_string))
110110
n ||= DelayedNotification.new(:asset => record, :notification => notification, :recipient_keys => (to_list || []).compact.map(&:asset_string))
111-
record.messages_sent[self.dispatch] = n
112-
if ENV['RAILS_ENV'] == 'test'
111+
if Rails.env.test?
113112
record.messages_sent[self.dispatch] = n.is_a?(DelayedNotification) ? n.process : n
114113
end
115114
n

0 commit comments

Comments
 (0)