Commit e6532ca2 authored by Sean McGivern's avatar Sean McGivern Committed by Douwe Maan

Fix notes email with group-level notification email

A Noteable doesn't have a group directly, unless it's an epic - we need
to look for the project's group to find the right email address.
parent 6177bc1d
......@@ -60,7 +60,7 @@ module Emails
# `note_id` is a `Note` when originating in `NotifyPreview`
@note = note_id.is_a?(Note) ? note_id : Note.find(note_id)
@project = @note.project
@group = @note.noteable.try(:group)
@group = @project.try(:group) || @note.noteable.try(:group)
if (@project || @group) && @note.persisted?
@sent_notification = SentNotification.record_note(@note, recipient_id, reply_key)
......
---
title: Fix comment emails not respecting group-level notification email
merge_request:
author:
type: fixed
......@@ -9,7 +9,7 @@ describe Emails::PagesDomains do
set(:user) { project.creator }
shared_examples 'a pages domain email' do
let(:test_recipient) { user }
let(:recipient) { user }
it_behaves_like 'an email sent to a user'
it_behaves_like 'an email sent from GitLab'
......
......@@ -45,7 +45,7 @@ describe Notify do
context 'for a project' do
shared_examples 'an assignee email' do
let(:test_recipient) { assignee }
let(:recipient) { assignee }
it_behaves_like 'an email sent to a user'
......@@ -55,7 +55,7 @@ describe Notify do
aggregate_failures do
expect(sender.display_name).to eq(current_user.name)
expect(sender.address).to eq(gitlab_sender)
expect(subject).to deliver_to(assignee.email)
expect(subject).to deliver_to(recipient.notification_email)
end
end
end
......@@ -547,12 +547,13 @@ describe Notify do
let(:host) { Gitlab.config.gitlab.host }
context 'in discussion' do
set(:first_note) { create(:discussion_note_on_issue) }
set(:second_note) { create(:discussion_note_on_issue, in_reply_to: first_note) }
set(:third_note) { create(:discussion_note_on_issue, in_reply_to: second_note) }
set(:first_note) { create(:discussion_note_on_issue, project: project) }
set(:second_note) { create(:discussion_note_on_issue, in_reply_to: first_note, project: project) }
set(:third_note) { create(:discussion_note_on_issue, in_reply_to: second_note, project: project) }
subject { described_class.note_issue_email(recipient.id, third_note.id) }
it_behaves_like 'an email sent to a user'
it_behaves_like 'appearance header and footer enabled'
it_behaves_like 'appearance header and footer not enabled'
......@@ -572,10 +573,11 @@ describe Notify do
end
context 'individual issue comments' do
set(:note) { create(:note_on_issue) }
set(:note) { create(:note_on_issue, project: project) }
subject { described_class.note_issue_email(recipient.id, note.id) }
it_behaves_like 'an email sent to a user'
it_behaves_like 'appearance header and footer enabled'
it_behaves_like 'appearance header and footer not enabled'
......@@ -604,13 +606,13 @@ describe Notify do
it_behaves_like 'a user cannot unsubscribe through footer link'
it 'has the correct subject and body' do
is_expected.to have_referable_subject(project_snippet, reply: true)
is_expected.to have_referable_subject(project_snippet, include_group: true, reply: true)
is_expected.to have_body_text project_snippet_note.note
end
end
describe 'project was moved' do
let(:test_recipient) { user }
let(:recipient) { user }
subject { described_class.project_was_moved_email(project.id, user.id, "gitlab/gitlab") }
it_behaves_like 'an email sent to a user'
......@@ -811,7 +813,7 @@ describe Notify do
it 'has the correct subject and body' do
aggregate_failures do
is_expected.to have_subject("Re: #{project.name} | #{commit.title} (#{commit.short_id})")
is_expected.to have_subject("Re: #{project.name} | #{project.group.name} | #{commit.title} (#{commit.short_id})")
is_expected.to have_body_text(commit.short_id)
end
end
......@@ -837,7 +839,7 @@ describe Notify do
it 'has the correct subject and body' do
aggregate_failures do
is_expected.to have_referable_subject(merge_request, reply: true)
is_expected.to have_referable_subject(merge_request, include_group: true, reply: true)
is_expected.to have_body_text note_on_merge_request_path
end
end
......@@ -863,7 +865,7 @@ describe Notify do
it 'has the correct subject and body' do
aggregate_failures do
is_expected.to have_referable_subject(issue, reply: true)
is_expected.to have_referable_subject(issue, include_group: true, reply: true)
is_expected.to have_body_text(note_on_issue_path)
end
end
......@@ -929,7 +931,7 @@ describe Notify do
it_behaves_like 'appearance header and footer not enabled'
it 'has the correct subject' do
is_expected.to have_subject "Re: #{project.name} | #{commit.title} (#{commit.short_id})"
is_expected.to have_subject "Re: #{project.name} | #{project.group.name} | #{commit.title} (#{commit.short_id})"
end
it 'contains a link to the commit' do
......@@ -957,7 +959,7 @@ describe Notify do
it_behaves_like 'appearance header and footer not enabled'
it 'has the correct subject' do
is_expected.to have_referable_subject(merge_request, reply: true)
is_expected.to have_referable_subject(merge_request, include_group: true, reply: true)
end
it 'contains a link to the merge request note' do
......@@ -985,7 +987,7 @@ describe Notify do
it_behaves_like 'appearance header and footer not enabled'
it 'has the correct subject' do
is_expected.to have_referable_subject(issue, reply: true)
is_expected.to have_referable_subject(issue, include_group: true, reply: true)
end
it 'contains a link to the issue note' do
......
......@@ -37,8 +37,19 @@ module EmailHelpers
ActionMailer::Base.deliveries.find { |d| d.to.include?(user.notification_email) }
end
def have_referable_subject(referable, include_project: true, reply: false)
prefix = (include_project && referable.project ? "#{referable.project.name} | " : '').freeze
def have_referable_subject(referable, include_project: true, include_group: false, reply: false)
context = []
context << referable.project.name if include_project && referable.project
context << referable.project.group.name if include_group && referable.project.group
prefix =
if context.any?
context.join(' | ') + ' | '
else
''
end
prefix = "Re: #{prefix}" if reply
suffix = "#{referable.title} (#{referable.to_reference})"
......
......@@ -45,18 +45,18 @@ 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(test_recipient.notification_email)
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: test_recipient, source: project.group, notification_email: group_notification_email)
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: test_recipient, source: project.group, notification_email: '')
expect(subject).to deliver_to(test_recipient.notification_email)
create(:notification_setting, user: recipient, source: project.group, notification_email: '')
expect(subject).to deliver_to(recipient.notification_email)
end
end
......@@ -67,17 +67,17 @@ shared_examples 'an email sent to a user' do
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: test_recipient, source: group, notification_email: group_notification_email)
create(:notification_setting, user: recipient, source: group, notification_email: group_notification_email)
subgroup_notification_email = 'user+subgroup@example.com'
create(:notification_setting, user: test_recipient, source: subgroup, notification_email: subgroup_notification_email)
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: test_recipient, source: subgroup, notification_email: '')
expect(subject).to deliver_to(test_recipient.notification_email)
create(:notification_setting, user: recipient, source: subgroup, notification_email: '')
expect(subject).to deliver_to(recipient.notification_email)
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment