Merge branch 'ashmckenzie/centralise-skipping-notifications' into 'master'

Centralise if we should skip sending notifications [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56197

app/models/note.rb

@@ -552,7 +552,7 @@ class Note < ApplicationRecord
   end
 
   def skip_notification?
-    review.present? || author.blocked? || author.ghost?
+    review.present? || !author.can_trigger_notifications?
   end
 
   private

app/models/user.rb

@@ -1862,6 +1862,10 @@ class User < ApplicationRecord
     callouts.find_or_initialize_by(feature_name: ::UserCallout.feature_names[feature_name])
   end
 
+  def can_trigger_notifications?
+    confirmed? && !blocked? && !ghost?
+  end
+
   protected
 
   # override, from Devise::Validatable

app/services/notification_service.rb

@@ -95,7 +95,7 @@ class NotificationService
   #  * users with custom level checked with "new issue"
   #
   def new_issue(issue, current_user)
-    new_resource_email(issue, :new_issue_email)
+    new_resource_email(issue, current_user, :new_issue_email)
   end
 
   # When issue text is updated, we should send an email to:
@@ -176,7 +176,7 @@ class NotificationService
   #
   # In EE, approvers of the merge request are also included
   def new_merge_request(merge_request, current_user)
-    new_resource_email(merge_request, :new_merge_request_email)
+    new_resource_email(merge_request, current_user, :new_merge_request_email)
   end
 
   def push_to_merge_request(merge_request, current_user, new_commits: [], existing_commits: [])
@@ -385,6 +385,11 @@ class NotificationService
 
   # Notify users when a new release is created
   def send_new_release_notifications(release)
+    unless release.author&.can_trigger_notifications?
+      warn_skipping_notifications(release.author, release)
+      return false
+    end
+
     recipients = NotificationRecipients::BuildService.build_new_release_recipients(release)
 
     recipients.each do |recipient|
@@ -697,7 +702,12 @@ class NotificationService
 
   protected
 
-  def new_resource_email(target, method)
+  def new_resource_email(target, current_user, method)
+    unless current_user&.can_trigger_notifications?
+      warn_skipping_notifications(current_user, target)
+      return false
+    end
+
     recipients = NotificationRecipients::BuildService.build_recipients(target, target.author, action: "new")
 
     recipients.each do |recipient|
@@ -706,6 +716,11 @@ class NotificationService
   end
 
   def new_mentions_in_resource_email(target, new_mentioned_users, current_user, method)
+    unless current_user&.can_trigger_notifications?
+      warn_skipping_notifications(current_user, target)
+      return false
+    end
+
     recipients = NotificationRecipients::BuildService.build_recipients(target, current_user, action: "new")
     recipients = recipients.select {|r| new_mentioned_users.include?(r.user) }
 
@@ -839,6 +854,10 @@ class NotificationService
 
     source.respond_to?(:group) && source.group
   end
+
+  def warn_skipping_notifications(user, object)
+    Gitlab::AppLogger.warn(message: "Skipping sending notifications", user: user.id, klass: object.class, object_id: object.id)
+  end
 end
 
 NotificationService.prepend_if_ee('EE::NotificationService')

ee/app/services/ee/notification_service.rb

@@ -36,8 +36,8 @@ module EE
       end
     end
 
-    def new_epic(epic)
-      new_resource_email(epic, :new_epic_email)
+    def new_epic(epic, current_user)
+      new_resource_email(epic, current_user, :new_epic_email)
     end
 
     def close_epic(epic, current_user)

ee/app/workers/new_epic_worker.rb

@@ -12,7 +12,7 @@ class NewEpicWorker # rubocop:disable Scalability/IdempotentWorker
     return unless objects_found?(epic_id, user_id)
 
     EventCreateService.new.open_epic(issuable, user)
-    NotificationService.new.new_epic(issuable)
+    NotificationService.new.new_epic(issuable, user)
     issuable.create_cross_references!(user)
   end
 

ee/spec/services/ee/notification_service_spec.rb

@@ -648,9 +648,40 @@ RSpec.describe EE::NotificationService, :mailer do
     end
 
     context 'new epic' do
-      let(:execute) { subject.new_epic(epic) }
+      let(:current_user) { epic.author }
+      let(:execute) { subject.new_epic(epic, current_user) }
 
       include_examples 'epic notifications'
+
+      shared_examples 'is not able to send notifications' do
+        it 'does not send any notification' do
+          expect(Gitlab::AppLogger).to receive(:warn).with(message: 'Skipping sending notifications', user: current_user.id, klass: epic.class, object_id: epic.id)
+
+          execute
+
+          should_not_email(watcher)
+          should_not_email(participating)
+          should_not_email(other_user)
+        end
+      end
+
+      context 'when author is not confirmed' do
+        let(:current_user) { create(:user, :unconfirmed) }
+
+        include_examples 'is not able to send notifications'
+      end
+
+      context 'when author is blocked' do
+        let(:current_user) { create(:user, :blocked) }
+
+        include_examples 'is not able to send notifications'
+      end
+
+      context 'when author is a ghost' do
+        let(:current_user) { create(:user, :ghost) }
+
+        include_examples 'is not able to send notifications'
+      end
     end
   end
 

ee/spec/workers/new_epic_worker_spec.rb

@@ -34,14 +34,44 @@ RSpec.describe NewEpicWorker do
       end
     end
 
-    context 'when everything is ok' do
-      let(:user) { create(:user) }
+    context 'with a user' do
       let(:epic) { create(:epic) }
 
       before do
         stub_licensed_features(epics: true)
       end
 
+      shared_examples 'a new epic where the current user cannot trigger notifications' do
+        it 'does not create a notification for the mentioned user' do
+          expect(Notify).not_to receive(:new_epic_email).with(user.id, epic.id, nil)
+
+          expect(Gitlab::AppLogger).to receive(:warn).with(message: 'Skipping sending notifications', user: user.id, klass: epic.class, object_id: epic.id)
+
+          worker.perform(epic.id, user.id)
+        end
+      end
+
+      context 'when the new epic author is not confirmed' do
+        let_it_be(:user) { create(:user, :unconfirmed) }
+
+        it_behaves_like 'a new epic where the current user cannot trigger notifications'
+      end
+
+      context 'when the new epic author is blocked' do
+        let_it_be(:user) { create(:user, :blocked) }
+
+        it_behaves_like 'a new epic where the current user cannot trigger notifications'
+      end
+
+      context 'when the new epic author is a ghost' do
+        let_it_be(:user) { create(:user, :ghost) }
+
+        it_behaves_like 'a new epic where the current user cannot trigger notifications'
+      end
+
+      context 'when everything is ok' do
+        let(:user) { create(:user) }
+
         it 'creates an event' do
           expect { worker.perform(epic.id, user.id) }.to change { Event.count }.from(0).to(1)
         end
@@ -76,4 +106,5 @@ RSpec.describe NewEpicWorker do
         end
       end
     end
+  end
 end

spec/models/user_spec.rb

@@ -5425,6 +5425,40 @@ RSpec.describe User do
     end
   end
 
+  describe 'can_trigger_notifications?' do
+    context 'when user is not confirmed' do
+      let_it_be(:user) { create(:user, :unconfirmed) }
+
+      it 'returns false' do
+        expect(user.can_trigger_notifications?).to be(false)
+      end
+    end
+
+    context 'when user is blocked' do
+      let_it_be(:user) { create(:user, :blocked) }
+
+      it 'returns false' do
+        expect(user.can_trigger_notifications?).to be(false)
+      end
+    end
+
+    context 'when user is a ghost' do
+      let_it_be(:user) { create(:user, :ghost) }
+
+      it 'returns false' do
+        expect(user.can_trigger_notifications?).to be(false)
+      end
+    end
+
+    context 'when user is confirmed and neither blocked or a ghost' do
+      let_it_be(:user) { create(:user) }
+
+      it 'returns true' do
+        expect(user.can_trigger_notifications?).to be(true)
+      end
+    end
+  end
+
   context 'bot users' do
     shared_examples 'bot users' do |bot_type|
       it 'creates the user if it does not exist' do

spec/services/notification_service_spec.rb

@@ -99,6 +99,23 @@ RSpec.describe NotificationService, :mailer do
     end
   end
 
+  shared_examples 'is not able to send notifications' do
+    it 'does not send any notification' do
+      user_1 = create(:user)
+      recipient_1 = NotificationRecipient.new(user_1, :custom, custom_action: :new_release)
+      allow(NotificationRecipients::BuildService).to receive(:build_new_release_recipients).and_return([recipient_1])
+
+      expect(Gitlab::AppLogger).to receive(:warn).with(message: 'Skipping sending notifications', user: current_user.id, klass: object.class, object_id: object.id)
+
+      action
+
+      should_not_email(@u_mentioned)
+      should_not_email(@u_guest_watcher)
+      should_not_email(user_1)
+      should_not_email(current_user)
+    end
+  end
+
   # Next shared examples are intended to test notifications of "participants"
   #
   # they take the following parameters:
@@ -882,8 +899,24 @@ RSpec.describe NotificationService, :mailer do
   end
 
   describe '#send_new_release_notifications', :deliver_mails_inline do
+    let(:release) { create(:release, author: current_user) }
+    let(:object) { release }
+    let(:action) { notification.send_new_release_notifications(release) }
+
+    context 'when release author is blocked' do
+      let(:current_user) { create(:user, :blocked) }
+
+      include_examples 'is not able to send notifications'
+    end
+
+    context 'when release author is a ghost' do
+      let(:current_user) { create(:user, :ghost) }
+
+      include_examples 'is not able to send notifications'
+    end
+
     context 'when recipients for a new release exist' do
-      let(:release) { create(:release) }
+      let(:current_user) { create(:user) }
 
       it 'calls new_release_email for each relevant recipient' do
         user_1 = create(:user)
@@ -1128,11 +1161,31 @@ RSpec.describe NotificationService, :mailer do
           should_email(admin)
         end
       end
+
+      context 'when the author is not allowed to trigger notifications' do
+        let(:current_user) { nil }
+        let(:object) { issue }
+        let(:action) { notification.new_issue(issue, current_user) }
+
+        context 'because they are blocked' do
+          let(:current_user) { create(:user, :blocked) }
+
+          include_examples 'is not able to send notifications'
+        end
+
+        context 'because they are a ghost' do
+          let(:current_user) { create(:user, :ghost) }
+
+          include_examples 'is not able to send notifications'
+        end
+      end
     end
 
     describe '#new_mentions_in_issue' do
       let(:notification_method) { :new_mentions_in_issue }
       let(:mentionable) { issue }
+      let(:object) { mentionable }
+      let(:action) { send_notifications(@u_mentioned, current_user: current_user) }
 
       include_examples 'notifications for new mentions'
 
@@ -1140,6 +1193,18 @@ RSpec.describe NotificationService, :mailer do
         let(:notification_target)  { issue }
         let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) }
       end
+
+      context 'where current_user is blocked' do
+        let(:current_user) { create(:user, :blocked) }
+
+        include_examples 'is not able to send notifications'
+      end
+
+      context 'where current_user is a ghost' do
+        let(:current_user) { create(:user, :ghost) }
+
+        include_examples 'is not able to send notifications'
+      end
     end
 
     describe '#reassigned_issue' do
@@ -1752,11 +1817,31 @@ RSpec.describe NotificationService, :mailer do
           it { should_not_email(participant) }
         end
       end
+
+      context 'when the author is not allowed to trigger notifications' do
+        let(:current_user) { nil }
+        let(:object) { merge_request }
+        let(:action) { notification.new_merge_request(merge_request, current_user) }
+
+        context 'because they are blocked' do
+          let(:current_user) { create(:user, :blocked) }
+
+          it_behaves_like 'is not able to send notifications'
+        end
+
+        context 'because they are a ghost' do
+          let(:current_user) { create(:user, :ghost) }
+
+          it_behaves_like 'is not able to send notifications'
+        end
+      end
     end
 
     describe '#new_mentions_in_merge_request' do
       let(:notification_method) { :new_mentions_in_merge_request }
       let(:mentionable) { merge_request }
+      let(:object) { mentionable }
+      let(:action) { send_notifications(@u_mentioned, current_user: current_user) }
 
       include_examples 'notifications for new mentions'
 
@@ -1764,6 +1849,18 @@ RSpec.describe NotificationService, :mailer do
         let(:notification_target)  { merge_request }
         let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) }
       end
+
+      context 'where current_user is blocked' do
+        let(:current_user) { create(:user, :blocked) }
+
+        include_examples 'is not able to send notifications'
+      end
+
+      context 'where current_user is a ghost' do
+        let(:current_user) { create(:user, :ghost) }
+
+        include_examples 'is not able to send notifications'
+      end
     end
 
     describe '#reassigned_merge_request' do
@@ -2761,7 +2858,7 @@ RSpec.describe NotificationService, :mailer do
     end
 
     it 'filters out guests when new merge request is created' do
-      notification.new_merge_request(merge_request1, @u_disabled)
+      notification.new_merge_request(merge_request1, developer)
 
       should_not_email(guest)
       should_email(assignee)

spec/support/helpers/notification_helpers.rb

@@ -3,10 +3,10 @@
 module NotificationHelpers
   extend self
 
-  def send_notifications(*new_mentions)
+  def send_notifications(*new_mentions, current_user: @u_disabled)
     mentionable.description = new_mentions.map(&:to_reference).join(' ')
 
-    notification.send(notification_method, mentionable, new_mentions, @u_disabled)
+    notification.send(notification_method, mentionable, new_mentions, current_user)
   end
 
   def create_global_setting_for(user, level)

spec/workers/new_issue_worker_spec.rb

@@ -38,12 +38,38 @@ RSpec.describe NewIssueWorker do
       end
     end
 
-    context 'when everything is ok' do
-      let_it_be(:user) { create_default(:user) }
+    context 'with a user' do
       let_it_be(:project) { create(:project, :public) }
       let_it_be(:mentioned) { create(:user) }
+      let_it_be(:user) { nil }
       let_it_be(:issue) { create(:issue, project: project, description: "issue for #{mentioned.to_reference}") }
 
+      shared_examples 'a new issue where the current_user cannot trigger notifications' do
+        it 'does not create a notification for the mentioned user' do
+          expect(Notify).not_to receive(:new_issue_email)
+            .with(mentioned.id, issue.id, NotificationReason::MENTIONED)
+
+          expect(Gitlab::AppLogger).to receive(:warn).with(message: 'Skipping sending notifications', user: user.id, klass: issue.class, object_id: issue.id)
+
+          worker.perform(issue.id, user.id)
+        end
+      end
+
+      context 'when the new issue author is blocked' do
+        let_it_be(:user) { create_default(:user, :blocked) }
+
+        it_behaves_like 'a new issue where the current_user cannot trigger notifications'
+      end
+
+      context 'when the new issue author is a ghost' do
+        let_it_be(:user) { create_default(:user, :ghost) }
+
+        it_behaves_like 'a new issue where the current_user cannot trigger notifications'
+      end
+
+      context 'when everything is ok' do
+        let_it_be(:user) { create_default(:user) }
+
         it 'creates a new event record' do
           expect { worker.perform(issue.id, user.id) }.to change { Event.count }.from(0).to(1)
         end
@@ -56,4 +82,5 @@ RSpec.describe NewIssueWorker do
         end
       end
     end
+  end
 end

spec/workers/new_merge_request_worker_spec.rb

@@ -40,14 +40,40 @@ RSpec.describe NewMergeRequestWorker do
       end
     end
 
-    context 'when everything is ok' do
+    context 'with a user' do
       let(:project) { create(:project, :public) }
       let(:mentioned) { create(:user) }
-      let(:user) { create(:user) }
+      let(:user) { nil }
       let(:merge_request) do
         create(:merge_request, source_project: project, description: "mr for #{mentioned.to_reference}")
       end
 
+      shared_examples 'a new merge request where the author cannot trigger notifications' do
+        it 'does not create a notification for the mentioned user' do
+          expect(Notify).not_to receive(:new_merge_request_email)
+            .with(mentioned.id, merge_request.id, NotificationReason::MENTIONED)
+
+          expect(Gitlab::AppLogger).to receive(:warn).with(message: 'Skipping sending notifications', user: user.id, klass: merge_request.class, object_id: merge_request.id)
+
+          worker.perform(merge_request.id, user.id)
+        end
+      end
+
+      context 'when the merge request author is blocked' do
+        let(:user) { create(:user, :blocked) }
+
+        it_behaves_like 'a new merge request where the author cannot trigger notifications'
+      end
+
+      context 'when the merge request author is a ghost' do
+        let(:user) { create(:user, :ghost) }
+
+        it_behaves_like 'a new merge request where the author cannot trigger notifications'
+      end
+
+      context 'when everything is ok' do
+        let(:user) { create(:user) }
+
         it 'creates a new event record' do
           expect { worker.perform(merge_request.id, user.id) }.to change { Event.count }.from(0).to(1)
         end
@@ -61,4 +87,5 @@ RSpec.describe NewMergeRequestWorker do
         end
       end
     end
+  end
 end