From a6fe9743f65dee0123a6c080236717b626ea6cec Mon Sep 17 00:00:00 2001 From: Will Read Date: Tue, 16 Aug 2016 20:05:16 -0700 Subject: [PATCH] EmailDigestAgent relies on received events, rather in memory (#1624) * EmailDigestAgent relies on received events, rather than storing them in memory - Addressing a problem where 40,000+ events were stored in one agent's memory, and the agent never loaded from the database * Cleans up references to memory["queue"] - incorporates suggested style and safeguards * Adds clarification on event inclusion in email * Use `reorder` rather than stripping away all scoping --- app/models/agents/email_digest_agent.rb | 20 ++++---- ...ve_queue_from_email_digest_agent_memory.rb | 8 +++ spec/models/agents/email_digest_agent_spec.rb | 51 ++++++++++--------- 3 files changed, 45 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20160807000122_remove_queue_from_email_digest_agent_memory.rb diff --git a/app/models/agents/email_digest_agent.rb b/app/models/agents/email_digest_agent.rb index 559476e9..349da7fc 100644 --- a/app/models/agents/email_digest_agent.rb +++ b/app/models/agents/email_digest_agent.rb @@ -7,7 +7,9 @@ module Agents cannot_create_events! description <<-MD - The Email Digest Agent collects any Events sent to it and sends them all via email when scheduled. + The Email Digest Agent collects any Events sent to it and sends them all via email when scheduled. The number of + used events also relies on the `Keep events` option of the emitting Agent, meaning that if events expire before + this agent is scheduled to run, they will not appear in the email. By default, the will have a `subject` and an optional `headline` before listing the Events. If the Events' payloads contain a `message`, that will be highlighted, otherwise everything in @@ -37,18 +39,16 @@ module Agents end def receive(incoming_events) + self.memory['events'] ||= [] incoming_events.each do |event| - self.memory['queue'] ||= [] - self.memory['queue'] << event.payload - self.memory['events'] ||= [] self.memory['events'] << event.id end end def check - if self.memory['queue'] && self.memory['queue'].length > 0 - ids = self.memory['events'].join(",") - groups = self.memory['queue'].map { |payload| present(payload) } + if self.memory['events'] && self.memory['events'].length > 0 + payloads = received_events.reorder("events.id ASC").where(id: self.memory['events']).pluck(:payload).to_a + groups = payloads.map { |payload| present(payload) } recipients.each do |recipient| begin SystemMailer.send_message( @@ -59,13 +59,13 @@ module Agents content_type: interpolated['content_type'], groups: groups ).deliver_now - log "Sent digest mail to #{recipient} with events [#{ids}]" + + log "Sent digest mail to #{recipient}" rescue => e - error("Error sending digest mail to #{recipient} with events [#{ids}]: #{e.message}") + error("Error sending digest mail to #{recipient}: #{e.message}") raise end end - self.memory['queue'] = [] self.memory['events'] = [] end end diff --git a/db/migrate/20160807000122_remove_queue_from_email_digest_agent_memory.rb b/db/migrate/20160807000122_remove_queue_from_email_digest_agent_memory.rb new file mode 100644 index 00000000..b5db693f --- /dev/null +++ b/db/migrate/20160807000122_remove_queue_from_email_digest_agent_memory.rb @@ -0,0 +1,8 @@ +class RemoveQueueFromEmailDigestAgentMemory < ActiveRecord::Migration + def up + Agents::EmailDigestAgent.find_each do |agent| + agent.memory.delete("queue") + agent.save!(validate: false) + end + end +end diff --git a/spec/models/agents/email_digest_agent_spec.rb b/spec/models/agents/email_digest_agent_spec.rb index dc6a2250..b443f487 100644 --- a/spec/models/agents/email_digest_agent_spec.rb +++ b/spec/models/agents/email_digest_agent_spec.rb @@ -8,11 +8,11 @@ describe Agents::EmailDigestAgent do end before do - @checker = Agents::EmailDigestAgent.new(:name => "something", :options => { :expected_receive_period_in_days => "2", :subject => "something interesting" }) + @checker = Agents::EmailDigestAgent.new(:name => "something", :options => {:expected_receive_period_in_days => "2", :subject => "something interesting"}) @checker.user = users(:bob) @checker.save! - @checker1 = Agents::EmailDigestAgent.new(:name => "something", :options => { :expected_receive_period_in_days => "2", :subject => "something interesting", :content_type => "text/plain" }) + @checker1 = Agents::EmailDigestAgent.new(:name => "something", :options => {:expected_receive_period_in_days => "2", :subject => "something interesting", :content_type => "text/plain"}) @checker1.user = users(:bob) @checker1.save! end @@ -25,16 +25,16 @@ describe Agents::EmailDigestAgent do it "queues any payloads it receives" do event1 = Event.new event1.agent = agents(:bob_rain_notifier_agent) - event1.payload = { :data => "Something you should know about" } + event1.payload = {:data => "Something you should know about"} event1.save! event2 = Event.new event2.agent = agents(:bob_weather_agent) - event2.payload = { :data => "Something else you should know about" } + event2.payload = {:data => "Something else you should know about"} event2.save! Agents::EmailDigestAgent.async_receive(@checker.id, [event1.id, event2.id]) - expect(@checker.reload.memory[:queue]).to eq([{ 'data' => "Something you should know about" }, { 'data' => "Something else you should know about" }]) + expect(@checker.reload.memory['events']).to match([event1.id, event2.id]) end end @@ -44,25 +44,34 @@ describe Agents::EmailDigestAgent do Agents::EmailDigestAgent.async_check(@checker.id) expect(ActionMailer::Base.deliveries).to eq([]) - @checker.memory[:queue] = [{ :data => "Something you should know about" }, - { :title => "Foo", :url => "http://google.com", :bar => 2 }, - { "message" => "hi", :woah => "there" }, - { "test" => 2 }] - @checker.memory[:events] = [1,2,3,4] - @checker.save! + payloads = [ + {:data => "Something you should know about"}, + {:title => "Foo", :url => "http://google.com", :bar => 2}, + {"message" => "hi", :woah => "there"}, + {"test" => 2} + ] - Agents::EmailDigestAgent.async_check(@checker.id) + events = payloads.map do |payload| + Event.new.tap do |event| + event.agent = agents(:bob_weather_agent) + event.payload = payload + event.save! + end + end + + Agents::DigestAgent.async_receive(@checker.id, events.map(&:id)) + @checker.sources << agents(:bob_weather_agent) + Agents::DigestAgent.async_check(@checker.id) expect(ActionMailer::Base.deliveries.last.to).to eq(["bob@example.com"]) expect(ActionMailer::Base.deliveries.last.subject).to eq("something interesting") expect(get_message_part(ActionMailer::Base.deliveries.last, /plain/).strip).to eq("Event\n data: Something you should know about\n\nFoo\n bar: 2\n url: http://google.com\n\nhi\n woah: there\n\nEvent\n test: 2") - expect(@checker.reload.memory[:queue]).to be_empty + expect(@checker.reload.memory[:events]).to be_empty end it "logs and re-raises mailer errors" do mock(SystemMailer).send_message(anything) { raise Net::SMTPAuthenticationError.new("Wrong password") } - @checker.memory[:queue] = [{ :data => "Something you should know about" }] @checker.memory[:events] = [1] @checker.save! @@ -71,8 +80,6 @@ describe Agents::EmailDigestAgent do }.to raise_error(/Wrong password/) expect(@checker.reload.memory[:events]).not_to be_empty - expect(@checker.reload.memory[:queue]).not_to be_empty - expect(@checker.logs.last.message).to match(/Error sending digest mail .* Wrong password/) end @@ -84,7 +91,7 @@ describe Agents::EmailDigestAgent do Agent.async_check(agents(:bob_weather_agent).id) Agent.receive! - expect(@checker.reload.memory[:queue]).not_to be_empty + expect(@checker.reload.memory[:events]).not_to be_empty Agents::EmailDigestAgent.async_check(@checker.id) @@ -94,18 +101,14 @@ describe Agents::EmailDigestAgent do expect(plain_email_text).to match(/avehumidity/) expect(html_email_text).to match(/avehumidity/) - expect(@checker.reload.memory[:queue]).to be_empty + expect(@checker.reload.memory[:events]).to be_empty end - + it "should send email with correct content type" do Agents::EmailDigestAgent.async_check(@checker1.id) expect(ActionMailer::Base.deliveries).to eq([]) - @checker1.memory[:queue] = [{ :data => "Something you should know about" }, - { :title => "Foo", :url => "http://google.com", :bar => 2 }, - { "message" => "hi", :woah => "there" }, - { "test" => 2 }] - @checker1.memory[:events] = [1,2,3,4] + @checker1.memory[:events] = [1, 2, 3, 4] @checker1.save! Agents::EmailDigestAgent.async_check(@checker1.id)