Refactoring notification service to find subscriptions per project

2 files changed, 92 insertions, 50 deletions

diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 3060babaf6b..ecdcbf08ee1 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -75,7 +75,7 @@ class NotificationService
   #  * watchers of the issue's labels
   #
   def relabeled_issue(issue, added_labels, current_user)
-    relabeled_resource_email(issue, added_labels, current_user, :relabeled_issue_email)
+    relabeled_resource_email(issue, issue.project, added_labels, current_user, :relabeled_issue_email)
   end
 
   # When create a merge request we should send an email to:
@@ -118,7 +118,7 @@ class NotificationService
   #  * watchers of the mr's labels
   #
   def relabeled_merge_request(merge_request, added_labels, current_user)
-    relabeled_resource_email(merge_request, added_labels, current_user, :relabeled_merge_request_email)
+    relabeled_resource_email(merge_request, merge_request.target_project, added_labels, current_user, :relabeled_merge_request_email)
   end
 
   def close_mr(merge_request, current_user)
@@ -205,7 +205,7 @@ class NotificationService
 
     recipients = reject_muted_users(recipients, note.project)
 
-    recipients = add_subscribed_users(recipients, note.noteable)
+    recipients = add_subscribed_users(recipients, note.project, note.noteable)
     recipients = reject_unsubscribed_users(recipients, note.noteable)
     recipients = reject_users_without_access(recipients, note.noteable)
 
@@ -505,17 +505,17 @@ class NotificationService
     end
   end
 
-  def add_subscribed_users(recipients, target)
+  def add_subscribed_users(recipients, project, target)
     return recipients unless target.respond_to? :subscribers
 
-    recipients + target.subscribers
+    recipients + target.subscribers(project)
   end
 
-  def add_labels_subscribers(recipients, target, labels: nil)
+  def add_labels_subscribers(recipients, project, target, labels: nil)
     return recipients unless target.respond_to? :labels
 
     (labels || target.labels).each do |label|
-      recipients += label.subscribers
+      recipients += label.subscribers(project)
     end
 
     recipients
@@ -571,8 +571,8 @@ class NotificationService
     end
   end
 
-  def relabeled_resource_email(target, labels, current_user, method)
-    recipients = build_relabeled_recipients(target, current_user, labels: labels)
+  def relabeled_resource_email(target, project, labels, current_user, method)
+    recipients = build_relabeled_recipients(target, project, current_user, labels: labels)
     label_names = labels.map(&:name)
 
     recipients.each do |recipient|
@@ -608,10 +608,10 @@ class NotificationService
     end
 
     recipients = reject_muted_users(recipients, project)
-    recipients = add_subscribed_users(recipients, target)
+    recipients = add_subscribed_users(recipients, project, target)
 
     if [:new_issue, :new_merge_request].include?(custom_action)
-      recipients = add_labels_subscribers(recipients, target)
+      recipients = add_labels_subscribers(recipients, project, target)
     end
 
     recipients = reject_unsubscribed_users(recipients, target)
@@ -622,8 +622,8 @@ class NotificationService
     recipients.uniq
   end
 
-  def build_relabeled_recipients(target, current_user, labels:)
-    recipients = add_labels_subscribers([], target, labels: labels)
+  def build_relabeled_recipients(target, project, current_user, labels:)
+    recipients = add_labels_subscribers([], project, target, labels: labels)
     recipients = reject_unsubscribed_users(recipients, target)
     recipients = reject_users_without_access(recipients, target)
     recipients.delete(current_user)
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 8ce35354c22..3a5bd4a4084 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -342,7 +342,9 @@ describe NotificationService, services: true do
   end
 
   describe 'Issues' do
-    let(:project) { create(:empty_project, :public) }
+    let(:group) { create(:group) }
+    let(:project) { create(:empty_project, :public, namespace: group) }
+    let(:another_project) { create(:empty_project, :public, namespace: group) }
     let(:issue) { create :issue, project: project, assignee: create(:user), description: 'cc @participant' }
 
     before do
@@ -377,13 +379,21 @@ describe NotificationService, services: true do
       end
 
       it "emails subscribers of the issue's labels" do
-        subscriber = create(:user)
-        label = create(:label, issues: [issue])
+        user_1 = create(:user)
+        user_2 = create(:user)
+        user_3 = create(:user)
+        label = create(:label, project: project, issues: [issue])
+        group_label = create(:group_label, group: group, issues: [issue])
         issue.reload
-        label.toggle_subscription(subscriber)
+        label.toggle_subscription(user_1)
+        group_label.toggle_subscription(user_2, project)
+        group_label.toggle_subscription(user_3, another_project)
+
         notification.new_issue(issue, @u_disabled)
 
-        should_email(subscriber)
+        should_email(user_1)
+        should_email(user_2)
+        should_not_email(user_3)
       end
 
       context 'confidential issues' do
@@ -399,7 +409,7 @@ describe NotificationService, services: true do
           project.team << [member, :developer]
           project.team << [guest, :guest]
 
-          label = create(:label, issues: [confidential_issue])
+          label = create(:label, project: project, issues: [confidential_issue])
           confidential_issue.reload
           label.toggle_subscription(non_member)
           label.toggle_subscription(author)
@@ -554,20 +564,28 @@ describe NotificationService, services: true do
     end
 
     describe '#relabeled_issue' do
-      let(:label) { create(:label, issues: [issue]) }
-      let(:label2) { create(:label) }
-      let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } }
-      let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } }
+      let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', issues: [issue]) }
+      let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }
+      let(:label_1) { create(:label, project: project, title: 'Label 1', issues: [issue]) }
+      let(:label_2) { create(:label, project: project, title: 'Label 2') }
+      let!(:subscriber_to_group_label_1) { create(:user).tap { |u| group_label_1.toggle_subscription(u, project) } }
+      let!(:subscriber_to_group_label_2) { create(:user).tap { |u| group_label_2.toggle_subscription(u, project) } }
+      let!(:subscriber_to_group_label_2_on_another_project) { create(:user).tap { |u| group_label_2.toggle_subscription(u, another_project) } }
+      let!(:subscriber_to_label_1) { create(:user).tap { |u| label_1.toggle_subscription(u) } }
+      let!(:subscriber_to_label_2) { create(:user).tap { |u| label_2.toggle_subscription(u) } }
 
       it "emails subscribers of the issue's added labels only" do
-        notification.relabeled_issue(issue, [label2], @u_disabled)
+        notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled)
 
-        should_not_email(subscriber_to_label)
-        should_email(subscriber_to_label2)
+        should_not_email(subscriber_to_label_1)
+        should_not_email(subscriber_to_group_label_1)
+        should_not_email(subscriber_to_group_label_2_on_another_project)
+        should_email(subscriber_to_group_label_2)
+        should_email(subscriber_to_label_2)
       end
 
       it "doesn't send email to anyone but subscribers of the given labels" do
-        notification.relabeled_issue(issue, [label2], @u_disabled)
+        notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled)
 
         should_not_email(issue.assignee)
         should_not_email(issue.author)
@@ -578,8 +596,11 @@ describe NotificationService, services: true do
         should_not_email(@watcher_and_subscriber)
         should_not_email(@unsubscriber)
         should_not_email(@u_participating)
-        should_not_email(subscriber_to_label)
-        should_email(subscriber_to_label2)
+        should_not_email(subscriber_to_label_1)
+        should_not_email(subscriber_to_group_label_1)
+        should_not_email(subscriber_to_group_label_2_on_another_project)
+        should_email(subscriber_to_group_label_2)
+        should_email(subscriber_to_label_2)
       end
 
       context 'confidential issues' do
@@ -590,8 +611,8 @@ describe NotificationService, services: true do
         let(:guest) { create(:user) }
         let(:admin) { create(:admin) }
         let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) }
-        let!(:label_1) { create(:label, issues: [confidential_issue]) }
-        let!(:label_2) { create(:label) }
+        let!(:label_1) { create(:label, project: project, issues: [confidential_issue]) }
+        let!(:label_2) { create(:label, project: project) }
 
         it "emails subscribers of the issue's labels that can read the issue" do
           project.team << [member, :developer]
@@ -725,7 +746,9 @@ describe NotificationService, services: true do
   end
 
   describe 'Merge Requests' do
-    let(:project) { create(:project, :public) }
+    let(:group) { create(:group) }
+    let(:project) { create(:project, :public, namespace: group) }
+    let(:another_project) { create(:empty_project, :public, namespace: group) }
     let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' }
 
     before do
@@ -758,12 +781,20 @@ describe NotificationService, services: true do
       end
 
       it "emails subscribers of the merge request's labels" do
-        subscriber = create(:user)
-        label = create(:label, merge_requests: [merge_request])
-        label.toggle_subscription(subscriber)
+        user_1 = create(:user)
+        user_2 = create(:user)
+        user_3 = create(:user)
+        label = create(:label, project: project, merge_requests: [merge_request])
+        group_label = create(:group_label, group: group, merge_requests: [merge_request])
+        label.toggle_subscription(user_1)
+        group_label.toggle_subscription(user_2, project)
+        group_label.toggle_subscription(user_3, another_project)
+
         notification.new_merge_request(merge_request, @u_disabled)
 
-        should_email(subscriber)
+        should_email(user_1)
+        should_email(user_2)
+        should_not_email(user_3)
       end
 
       context 'participating' do
@@ -857,20 +888,28 @@ describe NotificationService, services: true do
     end
 
     describe '#relabel_merge_request' do
-      let(:label) { create(:label, merge_requests: [merge_request]) }
-      let(:label2) { create(:label) }
-      let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } }
-      let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } }
+      let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', merge_requests: [merge_request]) }
+      let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }
+      let(:label_1) { create(:label, project: project, title: 'Label 1', merge_requests: [merge_request]) }
+      let(:label_2) { create(:label, project: project, title: 'Label 2') }
+      let!(:subscriber_to_group_label_1) { create(:user).tap { |u| group_label_1.toggle_subscription(u, project) } }
+      let!(:subscriber_to_group_label_2) { create(:user).tap { |u| group_label_2.toggle_subscription(u, project) } }
+      let!(:subscriber_to_group_label_2_on_another_project) { create(:user).tap { |u| group_label_2.toggle_subscription(u, another_project) } }
+      let!(:subscriber_to_label_1) { create(:user).tap { |u| label_1.toggle_subscription(u) } }
+      let!(:subscriber_to_label_2) { create(:user).tap { |u| label_2.toggle_subscription(u) } }
 
       it "emails subscribers of the merge request's added labels only" do
-        notification.relabeled_merge_request(merge_request, [label2], @u_disabled)
+        notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled)
 
-        should_not_email(subscriber_to_label)
-        should_email(subscriber_to_label2)
+        should_not_email(subscriber_to_label_1)
+        should_not_email(subscriber_to_group_label_1)
+        should_not_email(subscriber_to_group_label_2_on_another_project)
+        should_email(subscriber_to_group_label_2)
+        should_email(subscriber_to_label_2)
       end
 
       it "doesn't send email to anyone but subscribers of the given labels" do
-        notification.relabeled_merge_request(merge_request, [label2], @u_disabled)
+        notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled)
 
         should_not_email(merge_request.assignee)
         should_not_email(merge_request.author)
@@ -881,8 +920,11 @@ describe NotificationService, services: true do
         should_not_email(@unsubscriber)
         should_not_email(@u_participating)
         should_not_email(@u_lazy_participant)
-        should_not_email(subscriber_to_label)
-        should_email(subscriber_to_label2)
+        should_not_email(subscriber_to_label_1)
+        should_not_email(subscriber_to_group_label_1)
+        should_not_email(subscriber_to_group_label_2_on_another_project)
+        should_email(subscriber_to_group_label_2)
+        should_email(subscriber_to_label_2)
       end
     end
 
@@ -1290,10 +1332,10 @@ describe NotificationService, services: true do
     project.team << [@unsubscriber, :master]
     project.team << [@watcher_and_subscriber, :master]
 
-    issuable.subscriptions.create(user: @subscriber, subscribed: true)
-    issuable.subscriptions.create(user: @subscribed_participant, subscribed: true)
-    issuable.subscriptions.create(user: @unsubscriber, subscribed: false)
+    issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true)
+    issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true)
+    issuable.subscriptions.create(user: @unsubscriber, project: project, subscribed: false)
     # Make the watcher a subscriber to detect dupes
-    issuable.subscriptions.create(user: @watcher_and_subscriber, subscribed: true)
+    issuable.subscriptions.create(user: @watcher_and_subscriber, project: project, subscribed: true)
   end
 end