Merge pull request #4479 from evanrolfe/revert/1event_per_email_job

Revert "[webui][api] Change SendEventEmails to 1 event per job."

src/api/app/jobs/send_event_emails_job.rb

@@ -1,23 +1,25 @@
 class SendEventEmailsJob < ApplicationJob
   queue_as :mailers
 
-  def perform(event_id)
-    event = Event::Base.find(event_id)
-    subscribers = event.subscribers
+  def perform
+    Event::Base.where(mails_sent: false).order(created_at: :asc).limit(1000).each do |event|
+      subscribers = event.subscribers
 
-    if subscribers.empty?
-      event.update_attributes(mails_sent: true)
-      return
-    end
+      if subscribers.empty?
+        event.update_attributes(mails_sent: true)
+        next
+      end
 
-    begin
-      create_rss_notifications(event)
-      EventMailer.event(subscribers, event).deliver_now
-    rescue StandardError => e
-      Airbrake.notify(e, event_id: event.id)
-    ensure
-      event.update_attributes(mails_sent: true)
+      begin
+        create_rss_notifications(event)
+        EventMailer.event(subscribers, event).deliver_now
+      rescue StandardError => e
+        Airbrake.notify(e, event_id: event.id)
+      ensure
+        event.update_attributes(mails_sent: true)
+      end
     end
+    true
   end
 
   private

src/api/app/models/event/base.rb

@@ -8,8 +8,6 @@ class Base < ApplicationRecord
     self.table_name = 'events'
 
     after_create :create_project_log_entry_job, if: -> { (PROJECT_CLASSES | PACKAGE_CLASSES).include?(self.class.name) }
-    after_create :perform_create_jobs
-    after_create :send_event_emails_job
 
     EXPLANATION_FOR_NOTIFICATIONS =  {
       'Event::BuildFail'          => 'Receive notifications of build failures for packages for which you are...',
@@ -153,6 +151,8 @@ def create_project_log_entry_job
       CreateProjectLogEntryJob.perform_later(payload, created_at.to_s, self.class.name)
     end
 
+    after_create :perform_create_jobs
+
     def perform_create_jobs
       self.undone_jobs = 0
       save
@@ -170,10 +170,6 @@ def perform_create_jobs
       save if self.undone_jobs > 0
     end
 
-    def send_event_emails_job
-      SendEventEmailsJob.perform_later(id) if id.present?
-    end
-
     # to be overwritten in subclasses
     def subject
       'Build Service Notification'

src/api/config/clock.rb

@@ -16,6 +16,10 @@ module Clockwork
     WorkerStatus.new.update_workerstatus_cache
   end
 
+  every(30.seconds, 'send notifications') do
+    SendEventEmailsJob.perform_later
+  end
+
   every(49.minutes, 'rescale history') do
     StatusHistoryRescalerJob.perform_later
   end

src/api/spec/jobs/send_event_emails_job_spec.rb

@@ -1,6 +1,8 @@
 require 'rails_helper'
 
 RSpec.describe SendEventEmailsJob, type: :job do
+  include ActiveJob::TestHelper
+
   describe '#perform' do
     before do
       ActionMailer::Base.deliveries = []
@@ -13,18 +15,16 @@
     let!(:project) { create(:project, name: 'comment_project', maintainer: [user, group]) }
 
     let!(:comment_author) { create(:confirmed_user) }
-    let!(:user_maintainer) { create(:group) }
 
-    let(:comment) do
-      create(:comment_project, commentable: project, body: "Hey @#{user.login} how are things?", user: comment_author)
-    end
+    let!(:comment) { create(:comment_project, commentable: project, body: "Hey @#{user.login} how are things?", user: comment_author) }
+    let!(:user_maintainer) { create(:group) }
 
     context 'with no errors being raised' do
       let!(:subscription1) { create(:event_subscription_comment_for_project, receiver_role: 'maintainer', user: user) }
       let!(:subscription2) { create(:event_subscription_comment_for_project, receiver_role: 'maintainer', user: nil, group: group) }
       let!(:subscription3) { create(:event_subscription_comment_for_project, receiver_role: 'commenter', user: comment_author) }
 
-      subject! { comment }
+      subject! { SendEventEmailsJob.new.perform }
 
       it 'sends an email to the subscribers' do
         email = ActionMailer::Base.deliveries.first
@@ -76,7 +76,7 @@
         allow(Airbrake).to receive(:notify)
       end
 
-      subject! { comment }
+      subject! { SendEventEmailsJob.new.perform }
 
       it 'updates the event mails_sent = true' do
         event = Event::CommentForProject.first
@@ -89,7 +89,7 @@
     end
 
     context 'with no subscriptions for the event' do
-      subject! { comment }
+      subject! { SendEventEmailsJob.new.perform }
 
       it 'updates the event mails_sent = true' do
         event = Event::CommentForProject.first

src/api/test/functional/comments_controller_test.rb

@@ -87,9 +87,11 @@ def test_create_request_comment
     # body can't be empty
     assert_xml_tag tag: 'status', attributes: { code: 'invalid_record' }
 
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_request_comment_path(request_number: 2), params: 'Hallo'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -101,9 +103,11 @@ def test_create_request_comment
 
     # just check if adrian gets the mail too - he's a commenter now
     login_dmayr
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_request_comment_path(request_number: 2), params: 'Hallo'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -113,6 +117,7 @@ def test_create_request_comment
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_request_comment_path(request_number: 2), params: 'Hallo @fred'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -122,6 +127,7 @@ def test_create_request_comment
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_request_comment_path(request_number: 2), params: 'Is Fred listening now?'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -138,9 +144,11 @@ def test_create_project_comment
     # body can't be empty
     assert_xml_tag tag: 'status', attributes: { code: 'invalid_record' }
 
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_project_comment_path(project: 'Apache'), params: 'Beautiful project'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -162,9 +170,11 @@ def test_create_package_comment
     # body can't be empty
     assert_xml_tag tag: 'status', attributes: { code: 'invalid_record' }
 
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_package_comment_path(project: 'kde4', package: 'kdebase'), params: 'Hola, estoy aprendiendo español'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -177,10 +187,12 @@ def test_create_package_comment
 
   def test_create_a_comment_that_only_mentioned_people_will_notice
     login_tom
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       # Trolling
       post create_package_comment_path(project: 'BaseDistro', package: 'pack1'), params: "I preffer Apache1, don't you? @fred"
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last

src/api/test/functional/maintenance_test.rb

@@ -1399,12 +1399,14 @@ def test_create_maintenance_project_and_release_packages
     assert_response :success
     assert_xml_tag(parent: { tag: 'state' }, tag: 'comment', content: 'blahfasel')
 
+    SendEventEmailsJob.new.perform
     ActionMailer::Base.deliveries.clear
 
     # leave a comment
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_request_comment_path(request_number: reqid), params: 'Release it now!'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -1417,6 +1419,7 @@ def test_create_maintenance_project_and_release_packages
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post create_request_comment_path(request_number: reqid), params: 'Slave, can you release it? The master is gone'
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last

src/api/test/functional/request_events_test.rb

@@ -22,11 +22,13 @@ def test_request_event
 
     Timecop.travel(2013, 8, 20, 12, 0, 0)
     myid = 0
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post '/request?cmd=create',
            params: "<request><action type='add_role'><target project='home:tom'/><person name='Iggy' role='reviewer'/></action></request>"
       assert_response :success
       myid = Xmlhash.parse(@response.body)['id']
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -41,6 +43,7 @@ def test_very_large_request_event
 
     Timecop.travel(2013, 8, 20, 12, 0, 0)
     myid = 0
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       body = "<request>\n"
       actions = 1000
@@ -53,6 +56,7 @@ def test_very_large_request_event
       req = Xmlhash.parse(@response.body)
       assert_equal actions, req['action'].count
       myid = req['id']
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -66,10 +70,12 @@ def test_set_bugowner_event
 
     Timecop.travel(2013, 8, 20, 12, 0, 0)
     myid = 0
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post '/request?cmd=create', params: "<request><action type='set_bugowner'><target project='home:tom'/><person name='Iggy'/></action></request>"
       assert_response :success
       myid = Xmlhash.parse(@response.body)['id']
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -86,6 +92,7 @@ def test_set_bugowner_event
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post "/request/#{myid}?cmd=changestate&newstate=declined", params: ''
       assert_response :success
+      SendEventEmailsJob.new.perform
     end
     email = nil
     ActionMailer::Base.deliveries.each do |m|
@@ -105,12 +112,14 @@ def test_devel_package_event
 
     Timecop.travel(2013, 8, 20, 12, 0, 0)
     myid = ''
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post '/request?cmd=create',
            params: "<request><action type='add_role'><target project='kde4' package='kdelibs'/><person name='Iggy' role='reviewer'/></action>"\
                    '</request>'
       assert_response :success
       myid = Xmlhash.parse(@response.body)['id']
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last
@@ -123,10 +132,12 @@ def test_repository_delete_request
 
     Timecop.travel(2013, 8, 20, 12, 0, 0)
     myid = ''
+    SendEventEmailsJob.new.perform
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       post '/request?cmd=create', params: "<request><action type='delete'><target project='home:coolo' repository='standard'/></action></request>"
       assert_response :success
       myid = Xmlhash.parse(@response.body)['id']
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last

src/api/test/models/event_test.rb

@@ -72,8 +72,10 @@ def groups_for_event(e)
     User.current = users(:Iggy)
     req = bs_requests(:submit_from_home_project)
     myid = req.number
+    SendEventEmailsJob.new.perform # empty queue
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       req.addreview by_user: 'tom', comment: 'Can you check that?'
+      SendEventEmailsJob.new.perform
     end
     email = ActionMailer::Base.deliveries.last
 
@@ -87,6 +89,17 @@ def groups_for_event(e)
     assert_equal should, email.encoded.lines.map(&:chomp).reject { |l| l =~ %r{^Date:} }.join("\n")
   end
 
+  test 'cleanup job' do
+    firstcount = Event::Base.count
+    CleanupEvents.new.perform
+    assert Event::Base.count == firstcount, 'all our fixtures are fresh, mail must be sent first'
+    f = Event::Base.first
+    f.mails_sent = true
+    f.save
+    CleanupEvents.new.perform
+    assert Event::Base.count != firstcount, 'now its gone'
+  end
+
   test 'maintainer mails for build failure' do
     # for this test we don't want fixtures to interfere
     EventSubscription.delete_all
@@ -122,8 +135,10 @@ def groups_for_event(e)
     User.current = users(:Iggy)
     req = bs_requests(:submit_from_home_project)
     myid = req.number
+    SendEventEmailsJob.new.perform # empty queue
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       req.addreview by_project: 'home:Iggy', by_package: 'TestPack', comment: 'Can you check that?'
+      SendEventEmailsJob.new.perform
     end
     email = ActionMailer::Base.deliveries.last
 
@@ -136,6 +151,7 @@ def groups_for_event(e)
     ActionMailer::Base.deliveries.clear
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
       req.addreview by_project: 'Apache', by_package: 'apache2', comment: 'Can you check that?'
+      SendEventEmailsJob.new.perform
     end
     email = ActionMailer::Base.deliveries.last
 

src/api/test/unit/event_mailer_test.rb

@@ -57,10 +57,11 @@ class EventMailerTest < ActionMailer::TestCase
     req = bs_requests(:submit_from_home_project)
     Timecop.travel(2013, 8, 20, 12, 0, 0)
     myid = req.number
-
+    SendEventEmailsJob.new.perform # empty queue
     assert_difference 'ActionMailer::Base.deliveries.size', +1 do
-      event = req.addreview(by_group: 'test_group', comment: 'does it look ok?')
-      SendEventEmailsJob.perform_now(event.id)
+      req.addreview(by_group: 'test_group', comment: 'does it look ok?')
+      # trigger the send job
+      SendEventEmailsJob.new.perform
     end
 
     email = ActionMailer::Base.deliveries.last