Merge branch '63485-fix-pipeline-emails-to-use-group-setting' into 'master'

Make pipeline emails respect group email setting

Closes #63485

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

app/mailers/notify.rb

@@ -78,17 +78,7 @@ class Notify < BaseMailer
   #
   # Returns a String containing the User's email address.
   def recipient(recipient_id, notification_group = nil)
-    @current_user = User.find(recipient_id)
-    group_notification_email = nil
-
-    if notification_group
-      notification_settings = notification_group.notification_settings_for(@current_user, hierarchy_order: :asc)
-      group_notification_email = notification_settings.find { |n| n.notification_email.present? }&.notification_email
-    end
-
-    # Return group-specific email address if present, otherwise return global
-    # email address
-    group_notification_email || @current_user.notification_email
+    User.find(recipient_id).notification_email_for(notification_group)
   end
 
   # Formats arguments into a String suitable for use as an email subject

app/models/group.rb

@@ -144,6 +144,12 @@ class Group < Namespace
     notification_settings(hierarchy_order: hierarchy_order).where(user: user)
   end
 
+  def notification_email_for(user)
+    # Finds the closest notification_setting with a `notification_email`
+    notification_settings = notification_settings_for(user, hierarchy_order: :asc)
+    notification_settings.find { |n| n.notification_email.present? }&.notification_email
+  end
+
   def to_reference(_from = nil, full: nil)
     "#{self.class.reference_prefix}#{full_path}"
   end

app/models/user.rb

@@ -1259,6 +1259,11 @@ class User < ApplicationRecord
     end
   end
 
+  def notification_email_for(notification_group)
+    # Return group-specific email address if present, otherwise return global notification email address
+    notification_group&.notification_email_for(self) || notification_email
+  end
+
   def notification_settings_for(source)
     if notification_settings.loaded?
       notification_settings.find do |notification|

app/services/notification_service.rb

@@ -418,7 +418,9 @@ class NotificationService
       [pipeline.user], :watch,
       custom_action: :"#{pipeline.status}_pipeline",
       target: pipeline
-    ).map(&:notification_email)
+    ).map do |user|
+      user.notification_email_for(pipeline.project.group)
+    end
 
     if recipients.any?
       mailer.public_send(email_template, pipeline, recipients).deliver_later

changelogs/unreleased/63485-fix-pipeline-emails-to-use-group-setting.yml

+---
+title: Fix pipeline emails not respecting group notification email setting
+merge_request: 30907
+author:
+type: fixed

spec/models/group_spec.rb

@@ -95,6 +95,43 @@ describe Group do
       end
     end
 
+    describe '#notification_email_for' do
+      let(:user) { create(:user) }
+      let(:group) { create(:group) }
+      let(:subgroup) { create(:group, parent: group) }
+
+      let(:group_notification_email) { 'user+group@example.com' }
+      let(:subgroup_notification_email) { 'user+subgroup@example.com' }
+
+      subject { subgroup.notification_email_for(user) }
+
+      context 'when both group notification emails are set' do
+        it 'returns subgroup notification email' do
+          create(:notification_setting, user: user, source: group, notification_email: group_notification_email)
+          create(:notification_setting, user: user, source: subgroup, notification_email: subgroup_notification_email)
+
+          is_expected.to eq(subgroup_notification_email)
+        end
+      end
+
+      context 'when subgroup notification email is blank' do
+        it 'returns parent group notification email' do
+          create(:notification_setting, user: user, source: group, notification_email: group_notification_email)
+          create(:notification_setting, user: user, source: subgroup, notification_email: '')
+
+          is_expected.to eq(group_notification_email)
+        end
+      end
+
+      context 'when only the parent group notification email is set' do
+        it 'returns parent group notification email' do
+          create(:notification_setting, user: user, source: group, notification_email: group_notification_email)
+
+          is_expected.to eq(group_notification_email)
+        end
+      end
+    end
+
     describe '#visibility_level_allowed_by_parent' do
       let(:parent) { create(:group, :internal) }
       let(:sub_group) { build(:group, parent_id: parent.id) }

spec/models/user_spec.rb

@@ -3504,4 +3504,37 @@ describe User do
       expect(described_class.reorder_by_name).to eq([user1, user2])
     end
   end
+
+  describe '#notification_email_for' do
+    let(:user) { create(:user) }
+    let(:group) { create(:group) }
+
+    subject { user.notification_email_for(group) }
+
+    context 'when group is nil' do
+      let(:group) { nil }
+
+      it 'returns global notification email' do
+        is_expected.to eq(user.notification_email)
+      end
+    end
+
+    context 'when group has no notification email set' do
+      it 'returns global notification email' do
+        create(:notification_setting, user: user, source: group, notification_email: '')
+
+        is_expected.to eq(user.notification_email)
+      end
+    end
+
+    context 'when group has notification email set' do
+      it 'returns group notification email' do
+        group_notification_email = 'user+group@example.com'
+
+        create(:notification_setting, user: user, source: group, notification_email: group_notification_email)
+
+        is_expected.to eq(group_notification_email)
+      end
+    end
+  end
 end

spec/services/notification_service_spec.rb

@@ -2063,27 +2063,59 @@ describe NotificationService, :mailer do
         end
 
         context 'when the creator has custom notifications enabled' do
-          before do
-            pipeline = create_pipeline(u_custom_notification_enabled, :success)
-            notification.pipeline_finished(pipeline)
-          end
+          let(:pipeline) { create_pipeline(u_custom_notification_enabled, :success) }
 
           it 'emails only the creator' do
+            notification.pipeline_finished(pipeline)
+
             should_only_email(u_custom_notification_enabled, kind: :bcc)
           end
+
+          context 'when the creator has group notification email set' do
+            let(:group_notification_email) { 'user+group@example.com' }
+
+            before do
+              group = create(:group)
+
+              project.update(group: group)
+              create(:notification_setting, user: u_custom_notification_enabled, source: group, notification_email: group_notification_email)
+            end
+
+            it 'sends to group notification email' do
+              notification.pipeline_finished(pipeline)
+
+              expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
+            end
+          end
         end
       end
 
       context 'with a failed pipeline' do
         context 'when the creator has no custom notification set' do
-          before do
-            pipeline = create_pipeline(u_member, :failed)
-            notification.pipeline_finished(pipeline)
-          end
+          let(:pipeline) { create_pipeline(u_member, :failed) }
 
           it 'emails only the creator' do
+            notification.pipeline_finished(pipeline)
+
             should_only_email(u_member, kind: :bcc)
           end
+
+          context 'when the creator has group notification email set' do
+            let(:group_notification_email) { 'user+group@example.com' }
+
+            before do
+              group = create(:group)
+
+              project.update(group: group)
+              create(:notification_setting, user: u_member, source: group, notification_email: group_notification_email)
+            end
+
+            it 'sends to group notification email' do
+              notification.pipeline_finished(pipeline)
+
+              expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
+            end
+          end
         end
 
         context 'when the creator has watch set' do

spec/support/shared_examples/notify_shared_examples.rb

@@ -42,42 +42,17 @@ shared_examples 'an email sent from GitLab' do
 end
 
 shared_examples 'an email sent to a user' do
-  let(:group_notification_email) { 'user+group@example.com' }
-
   it 'is sent to user\'s global notification email address' do
     expect(subject).to deliver_to(recipient.notification_email)
   end
 
-  context 'that is part of a project\'s group' do
-    it 'is sent to user\'s group notification email address when set' do
-      create(:notification_setting, user: recipient, source: project.group, notification_email: group_notification_email)
-      expect(subject).to deliver_to(group_notification_email)
-    end
+  context 'with group notification email' do
+    it 'is sent to user\'s group notification email' do
+      group_notification_email = 'user+group@example.com'
 
-    it 'is sent to user\'s global notification email address when no group email set' do
-      create(:notification_setting, user: recipient, source: project.group, notification_email: '')
-      expect(subject).to deliver_to(recipient.notification_email)
-    end
-  end
-
-  context 'when project is in a sub-group', :nested_groups do
-    before do
-      project.update!(group: subgroup)
-    end
-
-    it 'is sent to user\'s subgroup notification email address when set' do
-      # Set top-level group notification email address to make sure it doesn't get selected
-      create(:notification_setting, user: recipient, source: group, notification_email: group_notification_email)
-
-      subgroup_notification_email = 'user+subgroup@example.com'
-      create(:notification_setting, user: recipient, source: subgroup, notification_email: subgroup_notification_email)
-
-      expect(subject).to deliver_to(subgroup_notification_email)
-    end
+      create(:notification_setting, user: recipient, source: project.group, notification_email: group_notification_email)
 
-    it 'is sent to user\'s group notification email address when set and subgroup email address not set' do
-      create(:notification_setting, user: recipient, source: subgroup, notification_email: '')
-      expect(subject).to deliver_to(recipient.notification_email)
+      expect(subject).to deliver_to(group_notification_email)
     end
   end
 end