From 40d6d5e2d0123f1417bb5d3d1ead47bd525f8dac Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Fri, 19 Jul 2019 01:04:43 +0800 Subject: [PATCH] Make pipeline emails respect group email setting When a user's notification email is set for a group, we should use that for pipeline emails --- app/mailers/notify.rb | 12 +---- app/models/group.rb | 6 +++ app/models/user.rb | 5 ++ app/services/notification_service.rb | 4 +- ...x-pipeline-emails-to-use-group-setting.yml | 5 ++ spec/models/group_spec.rb | 37 ++++++++++++++ spec/models/user_spec.rb | 33 +++++++++++++ spec/services/notification_service_spec.rb | 48 +++++++++++++++---- .../shared_examples/notify_shared_examples.rb | 35 ++------------ 9 files changed, 135 insertions(+), 50 deletions(-) create mode 100644 changelogs/unreleased/63485-fix-pipeline-emails-to-use-group-setting.yml diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 576caea4c10..8ef20a03541 100644 --- a/app/mailers/notify.rb +++ b/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 diff --git a/app/models/group.rb b/app/models/group.rb index 37f30552b39..26ce2957e9b 100644 --- a/app/models/group.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 0fd3daa3383..b439d1c0c16 100644 --- a/app/models/user.rb +++ b/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| diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5aa804666f0..a55771ed538 100644 --- a/app/services/notification_service.rb +++ b/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 diff --git a/changelogs/unreleased/63485-fix-pipeline-emails-to-use-group-setting.yml b/changelogs/unreleased/63485-fix-pipeline-emails-to-use-group-setting.yml new file mode 100644 index 00000000000..c3ee3108216 --- /dev/null +++ b/changelogs/unreleased/63485-fix-pipeline-emails-to-use-group-setting.yml @@ -0,0 +1,5 @@ +--- +title: Fix pipeline emails not respecting group notification email setting +merge_request: 30907 +author: +type: fixed diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index c7fb0f51075..90e0900445e 100644 --- a/spec/models/group_spec.rb +++ b/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) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5cfa64fd764..2d20f8c78cc 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 3e3de051732..c20de1fd079 100644 --- a/spec/services/notification_service_spec.rb +++ b/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 diff --git a/spec/support/shared_examples/notify_shared_examples.rb b/spec/support/shared_examples/notify_shared_examples.rb index e64c7e37a0c..4452b1c82cb 100644 --- a/spec/support/shared_examples/notify_shared_examples.rb +++ b/spec/support/shared_examples/notify_shared_examples.rb @@ -42,43 +42,18 @@ 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 + context 'with group notification email' do + it 'is sent to user\'s group notification email' do + group_notification_email = 'user+group@example.com' + create(:notification_setting, user: recipient, source: project.group, notification_email: group_notification_email) + expect(subject).to deliver_to(group_notification_email) end - - 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 - - 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) - end end end