Merge branch 'bugfix.silence-on-disabled-notifications' into 'master'

Bugfix.silence on disabled notifications

Closes #23714

See merge request !13325

app/models/member.rb

@@ -276,6 +276,13 @@ class Member < ActiveRecord::Base
     @notification_setting ||= user.notification_settings_for(source)
   end
 
+  def notifiable?(type, opts = {})
+    # always notify when there isn't a user yet
+    return true if user.blank?
+
+    NotificationRecipientService.notifiable?(user, type, notifiable_options.merge(opts))
+  end
+
   private
 
   def send_invite
@@ -332,4 +339,8 @@ class Member < ActiveRecord::Base
   def notification_service
     NotificationService.new
   end
+
+  def notifiable_options
+    {}
+  end
 end

app/models/members/group_member.rb

@@ -30,6 +30,10 @@ class GroupMember < Member
     'Group'
   end
 
+  def notifiable_options
+    { group: group }
+  end
+
   private
 
   def send_invite

app/models/members/project_member.rb

@@ -87,6 +87,10 @@ class ProjectMember < Member
     project.owner == user
   end
 
+  def notifiable_options
+    { project: project }
+  end
+
   private
 
   def delete_member_todos

app/models/notification_recipient.rb

@@ -5,14 +5,22 @@ class NotificationRecipient
     custom_action: nil,
     target: nil,
     acting_user: nil,
-    project: nil
+    project: nil,
+    group: nil,
+    skip_read_ability: false
   )
+    unless NotificationSetting.levels.key?(type) || type == :subscription
+      raise ArgumentError, "invalid type: #{type.inspect}"
+    end
+
     @custom_action = custom_action
     @acting_user = acting_user
     @target = target
-    @project = project || @target&.project
+    @project = project || default_project
+    @group = group || @project&.group
     @user = user
     @type = type
+    @skip_read_ability = skip_read_ability
   end
 
   def notification_setting
@@ -77,6 +85,8 @@ class NotificationRecipient
   def has_access?
     DeclarativePolicy.subject_scope do
       return false unless user.can?(:receive_notifications)
+      return true if @skip_read_ability
+
       return false if @project && !user.can?(:read_project, @project)
 
       return true unless read_ability
@@ -96,6 +106,7 @@ class NotificationRecipient
   private
 
   def read_ability
+    return nil if @skip_read_ability
     return @read_ability if instance_variable_defined?(:@read_ability)
 
     @read_ability =
@@ -111,12 +122,18 @@ class NotificationRecipient
       end
   end
 
+  def default_project
+    return nil if @target.nil?
+    return @target if @target.is_a?(Project)
+    return @target.project if @target.respond_to?(:project)
+  end
+
   def find_notification_setting
     project_setting = @project && user.notification_settings_for(@project)
 
     return project_setting unless project_setting.nil? || project_setting.global?
 
-    group_setting = @project&.group && user.notification_settings_for(@project.group)
+    group_setting = @group && user.notification_settings_for(@group)
 
     return group_setting unless group_setting.nil? || group_setting.global?
 

app/models/user.rb

@@ -1069,6 +1069,7 @@ class User < ActiveRecord::Base
 
   # Added according to https://github.com/plataformatec/devise/blob/7df57d5081f9884849ca15e4fde179ef164a575f/README.md#activejob-integration
   def send_devise_notification(notification, *args)
+    return true unless can?(:receive_notifications)
     devise_mailer.send(notification, self, *args).deliver_later
   end
 

app/services/notification_service.rb

@@ -10,9 +10,11 @@ class NotificationService
   # only if ssh key is not deploy key
   #
   # This is security email so it will be sent
-  # even if user disabled notifications
+  # even if user disabled notifications. However,
+  # it won't be sent to internal users like the
+  # ghost user or the EE support bot.
   def new_key(key)
-    if key.user
+    if key.user&.can?(:receive_notifications)
       mailer.new_ssh_key_email(key.id).deliver_later
     end
   end
@@ -22,14 +24,14 @@ class NotificationService
   # This is a security email so it will be sent even if the user user disabled
   # notifications
   def new_gpg_key(gpg_key)
-    if gpg_key.user
+    if gpg_key.user&.can?(:receive_notifications)
       mailer.new_gpg_key_email(gpg_key.id).deliver_later
     end
   end
 
   # Always notify user about email added to profile
   def new_email(email)
-    if email.user
+    if email.user&.can?(:receive_notifications)
       mailer.new_email_email(email.id).deliver_later
     end
   end
@@ -185,6 +187,8 @@ class NotificationService
 
   # Notify new user with email after creation
   def new_user(user, token = nil)
+    return true unless notifiable?(user, :mention)
+
     # Don't email omniauth created users
     mailer.new_user_email(user.id, token).deliver_later unless user.identities.any?
   end
@@ -206,19 +210,27 @@ class NotificationService
 
   # Members
   def new_access_request(member)
+    return true unless member.notifiable?(:subscription)
+
     mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later
   end
 
   def decline_access_request(member)
+    return true unless member.notifiable?(:subscription)
+
     mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later
   end
 
   # Project invite
   def invite_project_member(project_member, token)
+    return true unless project_member.notifiable?(:subscription)
+
     mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later
   end
 
   def accept_project_invite(project_member)
+    return true unless project_member.notifiable?(:subscription)
+
     mailer.member_invite_accepted_email(project_member.real_source_type, project_member.id).deliver_later
   end
 
@@ -232,10 +244,14 @@ class NotificationService
   end
 
   def new_project_member(project_member)
+    return true unless project_member.notifiable?(:mention, skip_read_ability: true)
+
     mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later
   end
 
   def update_project_member(project_member)
+    return true unless project_member.notifiable?(:mention)
+
     mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later
   end
 
@@ -249,6 +265,9 @@ class NotificationService
   end
 
   def decline_group_invite(group_member)
+    # always send this one, since it's a response to the user's own
+    # action
+
     mailer.member_invite_declined_email(
       group_member.real_source_type,
       group_member.group.id,
@@ -258,15 +277,19 @@ class NotificationService
   end
 
   def new_group_member(group_member)
+    return true unless group_member.notifiable?(:mention)
+
     mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later
   end
 
   def update_group_member(group_member)
+    return true unless group_member.notifiable?(:mention)
+
     mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later
   end
 
   def project_was_moved(project, old_path_with_namespace)
-    recipients = NotificationRecipientService.notifiable_users(project.team.members, :mention, project: project)
+    recipients = notifiable_users(project.team.members, :mention, project: project)
 
     recipients.each do |recipient|
       mailer.project_was_moved_email(
@@ -288,10 +311,14 @@ class NotificationService
   end
 
   def project_exported(project, current_user)
+    return true unless notifiable?(current_user, :mention, project: project)
+
     mailer.project_was_exported_email(current_user, project).deliver_later
   end
 
   def project_not_exported(project, current_user, errors)
+    return true unless notifiable?(current_user, :mention, project: project)
+
     mailer.project_was_not_exported_email(current_user, project, errors).deliver_later
   end
 
@@ -300,7 +327,7 @@ class NotificationService
 
     return unless mailer.respond_to?(email_template)
 
-    recipients ||= NotificationRecipientService.notifiable_users(
+    recipients ||= notifiable_users(
       [pipeline.user], :watch,
       custom_action: :"#{pipeline.status}_pipeline",
       target: pipeline
@@ -369,7 +396,7 @@ class NotificationService
 
   def relabeled_resource_email(target, labels, current_user, method)
     recipients = labels.flat_map { |l| l.subscribers(target.project) }
-    recipients = NotificationRecipientService.notifiable_users(
+    recipients = notifiable_users(
       recipients, :subscription,
       target: target,
       acting_user: current_user
@@ -401,4 +428,14 @@ class NotificationService
       object.previous_changes[attribute].first
     end
   end
+
+  private
+
+  def notifiable?(*args)
+    NotificationRecipientService.notifiable?(*args)
+  end
+
+  def notifiable_users(*args)
+    NotificationRecipientService.notifiable_users(*args)
+  end
 end

changelogs/unreleased/13325-bugfix-silence-on-disabled-notifications.yml

+---
+title: disabling notifications globally now properly turns off group/project added
+  emails
+merge_request: 13325
+author: @jneen
+type: fixed

spec/services/notification_service_spec.rb

@@ -80,12 +80,16 @@ describe NotificationService, :mailer do
 
   describe 'Keys' do
     describe '#new_key' do
-      let!(:key) { create(:personal_key) }
+      let(:key_options) { {} }
+      let!(:key) { create(:personal_key, key_options) }
 
       it { expect(notification.new_key(key)).to be_truthy }
+      it { should_email(key.user) }
 
-      it 'sends email to key owner' do
-        expect { notification.new_key(key) }.to change { ActionMailer::Base.deliveries.size }.by(1)
+      describe 'never emails the ghost user' do
+        let(:key_options) { { user: User.ghost } }
+
+        it { should_not_email_anyone }
       end
     end
   end
@@ -1173,11 +1177,12 @@ describe NotificationService, :mailer do
       end
     end
 
+    context 'user with notifications disabled' do
       describe '#project_exported' do
         it do
           notification.project_exported(project, @u_disabled)
 
-        should_only_email(@u_disabled)
+          should_not_email_anyone
         end
       end
 
@@ -1185,7 +1190,26 @@ describe NotificationService, :mailer do
         it do
           notification.project_not_exported(project, @u_disabled, ['error'])
 
-        should_only_email(@u_disabled)
+          should_not_email_anyone
+        end
+      end
+    end
+
+    context 'user with notifications enabled' do
+      describe '#project_exported' do
+        it do
+          notification.project_exported(project, @u_participating)
+
+          should_only_email(@u_participating)
+        end
+      end
+
+      describe '#project_not_exported' do
+        it do
+          notification.project_not_exported(project, @u_participating, ['error'])
+
+          should_only_email(@u_participating)
+        end
       end
     end
   end
@@ -1209,6 +1233,35 @@ describe NotificationService, :mailer do
         end.to change { ActionMailer::Base.deliveries.size }.by(1)
       end
     end
+
+    describe '#new_group_member' do
+      let(:group) { create(:group) }
+      let(:added_user) { create(:user) }
+
+      def create_member!
+        GroupMember.create(
+          group: group,
+          user: added_user,
+          access_level: Gitlab::Access::GUEST
+        )
+      end
+
+      it 'sends a notification' do
+        create_member!
+        should_only_email(added_user)
+      end
+
+      describe 'when notifications are disabled' do
+        before do
+          create_global_setting_for(added_user, :disabled)
+        end
+
+        it 'does not send a notification' do
+          create_member!
+          should_not_email_anyone
+        end
+      end
+    end
   end
 
   describe 'ProjectMember' do
@@ -1228,6 +1281,31 @@ describe NotificationService, :mailer do
         end.to change { ActionMailer::Base.deliveries.size }.by(1)
       end
     end
+
+    describe '#new_project_member' do
+      let(:project) { create(:project) }
+      let(:added_user) { create(:user) }
+
+      def create_member!
+        create(:project_member, user: added_user, project: project)
+      end
+
+      it do
+        create_member!
+        should_only_email(added_user)
+      end
+
+      describe 'when notifications are disabled' do
+        before do
+          create_global_setting_for(added_user, :disabled)
+        end
+
+        it do
+          create_member!
+          should_not_email_anyone
+        end
+      end
+    end
   end
 
   context 'guest user in private project' do