Merge branch 'move-notification-service-calls-to-sidekiq' into 'master'

Move NotificationService calls to Sidekiq

Closes #43106

See merge request gitlab-org/gitlab-ce!18535

3 files changed, 85 insertions, 5 deletions

diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 55bbe954491..48ef5f3c115 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -96,6 +96,37 @@ describe NotificationService, :mailer do
     it_should_behave_like 'participating by assignee notification'
   end
 
+  describe '#async' do
+    let(:async) { notification.async }
+    set(:key) { create(:personal_key) }
+
+    it 'returns an Async object with the correct parent' do
+      expect(async).to be_a(described_class::Async)
+      expect(async.parent).to eq(notification)
+    end
+
+    context 'when receiving a public method' do
+      it 'schedules a MailScheduler::NotificationServiceWorker' do
+        expect(MailScheduler::NotificationServiceWorker)
+          .to receive(:perform_async).with('new_key', key)
+
+        async.new_key(key)
+      end
+    end
+
+    context 'when receiving a private method' do
+      it 'raises NoMethodError' do
+        expect { async.notifiable?(key) }.to raise_error(NoMethodError)
+      end
+    end
+
+    context 'when recieving a non-existent method' do
+      it 'raises NoMethodError' do
+        expect { async.foo(key) }.to raise_error(NoMethodError)
+      end
+    end
+  end
+
   describe 'Keys' do
     describe '#new_key' do
       let(:key_options) { {} }
@@ -982,6 +1013,8 @@ describe NotificationService, :mailer do
     let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' }
 
     before do
+      project.add_master(merge_request.author)
+      project.add_master(merge_request.assignee)
       build_team(merge_request.target_project)
       add_users_with_subscription(merge_request.target_project, merge_request)
       update_custom_notification(:new_merge_request, @u_guest_custom, resource: project)
@@ -1093,15 +1126,18 @@ describe NotificationService, :mailer do
     end
 
     describe '#reassigned_merge_request' do
+      let(:current_user) { create(:user) }
+
       before do
         update_custom_notification(:reassign_merge_request, @u_guest_custom, resource: project)
         update_custom_notification(:reassign_merge_request, @u_custom_global)
       end
 
       it do
-        notification.reassigned_merge_request(merge_request, merge_request.author)
+        notification.reassigned_merge_request(merge_request, current_user, merge_request.author)
 
         should_email(merge_request.assignee)
+        should_email(merge_request.author)
         should_email(@u_watcher)
         should_email(@u_participant_mentioned)
         should_email(@subscriber)
@@ -1116,7 +1152,7 @@ describe NotificationService, :mailer do
       end
 
       it 'adds "assigned" reason for new assignee' do
-        notification.reassigned_merge_request(merge_request, merge_request.author)
+        notification.reassigned_merge_request(merge_request, current_user, merge_request.author)
 
         email = find_email_for(merge_request.assignee)
 
@@ -1126,7 +1162,7 @@ describe NotificationService, :mailer do
       it_behaves_like 'participating notifications' do
         let(:participant) { create(:user, username: 'user-participant') }
         let(:issuable) { merge_request }
-        let(:notification_trigger) { notification.reassigned_merge_request(merge_request, @u_disabled) }
+        let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, merge_request.author) }
       end
     end
 
diff --git a/spec/workers/mail_scheduler/issue_due_worker_spec.rb b/spec/workers/mail_scheduler/issue_due_worker_spec.rb
index 48ac1b8a1a4..1026ae5b4bf 100644
--- a/spec/workers/mail_scheduler/issue_due_worker_spec.rb
+++ b/spec/workers/mail_scheduler/issue_due_worker_spec.rb
@@ -12,8 +12,8 @@ describe MailScheduler::IssueDueWorker do
       create(:issue, :opened, project: project, due_date: 2.days.from_now) # due on another day
       create(:issue, :opened, due_date: Date.tomorrow) # different project
 
-      expect_any_instance_of(NotificationService).to receive(:issue_due).with(issue1)
-      expect_any_instance_of(NotificationService).to receive(:issue_due).with(issue2)
+      expect(worker.notification_service).to receive(:issue_due).with(issue1)
+      expect(worker.notification_service).to receive(:issue_due).with(issue2)
 
       worker.perform(project.id)
     end
diff --git a/spec/workers/mail_scheduler/notification_service_worker_spec.rb b/spec/workers/mail_scheduler/notification_service_worker_spec.rb
new file mode 100644
index 00000000000..f725c8763a0
--- /dev/null
+++ b/spec/workers/mail_scheduler/notification_service_worker_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+
+describe MailScheduler::NotificationServiceWorker do
+  let(:worker) { described_class.new }
+  let(:method) { 'new_key' }
+  set(:key) { create(:personal_key) }
+
+  def serialize(*args)
+    ActiveJob::Arguments.serialize(args)
+  end
+
+  describe '#perform' do
+    it 'deserializes arguments from global IDs' do
+      expect(worker.notification_service).to receive(method).with(key)
+
+      worker.perform(method, *serialize(key))
+    end
+
+    context 'when the arguments cannot be deserialized' do
+      it 'does nothing' do
+        expect(worker.notification_service).not_to receive(method)
+
+        worker.perform(method, key.to_global_id.to_s.succ)
+      end
+    end
+
+    context 'when the method is not a public method' do
+      it 'raises NoMethodError' do
+        expect { worker.perform('notifiable?', *serialize(key)) }.to raise_error(NoMethodError)
+      end
+    end
+  end
+
+  describe '.perform_async' do
+    it 'serializes arguments as global IDs when scheduling' do
+      Sidekiq::Testing.fake! do
+        described_class.perform_async(method, key)
+
+        expect(described_class.jobs.count).to eq(1)
+        expect(described_class.jobs.first).to include('args' => [method, *serialize(key)])
+      end
+    end
+  end
+end