Commit 02741ca4 authored by Mario de la Ossa's avatar Mario de la Ossa

Backport 5480-epic-notifications from EE

parent 33e78f9e
...@@ -17,16 +17,20 @@ class SentNotificationsController < ApplicationController ...@@ -17,16 +17,20 @@ class SentNotificationsController < ApplicationController
flash[:notice] = "You have been unsubscribed from this thread." flash[:notice] = "You have been unsubscribed from this thread."
if current_user if current_user
redirect_to noteable_path(noteable)
else
redirect_to new_user_session_path
end
end
def noteable_path(noteable)
case noteable case noteable
when Issue when Issue
redirect_to issue_path(noteable) issue_path(noteable)
when MergeRequest when MergeRequest
redirect_to merge_request_path(noteable) merge_request_path(noteable)
else
redirect_to root_path
end
else else
redirect_to new_user_session_path root_path
end end
end end
end end
...@@ -43,7 +43,7 @@ module Emails ...@@ -43,7 +43,7 @@ module Emails
private private
def note_target_url_options def note_target_url_options
[@project, @note.noteable, anchor: "note_#{@note.id}"] [@project || @group, @note.noteable, anchor: "note_#{@note.id}"]
end end
def note_thread_options(recipient_id) def note_thread_options(recipient_id)
...@@ -58,8 +58,9 @@ module Emails ...@@ -58,8 +58,9 @@ module Emails
# `note_id` is a `Note` when originating in `NotifyPreview` # `note_id` is a `Note` when originating in `NotifyPreview`
@note = note_id.is_a?(Note) ? note_id : Note.find(note_id) @note = note_id.is_a?(Note) ? note_id : Note.find(note_id)
@project = @note.project @project = @note.project
@group = @note.noteable.try(:group)
if @project && @note.persisted? if (@project || @group) && @note.persisted?
@sent_notification = SentNotification.record_note(@note, recipient_id, reply_key) @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key)
end end
end end
......
...@@ -94,6 +94,7 @@ class Notify < BaseMailer ...@@ -94,6 +94,7 @@ class Notify < BaseMailer
def subject(*extra) def subject(*extra)
subject = "" subject = ""
subject << "#{@project.name} | " if @project subject << "#{@project.name} | " if @project
subject << "#{@group.name} | " if @group
subject << extra.join(' | ') if extra.present? subject << extra.join(' | ') if extra.present?
subject << " | #{Gitlab.config.gitlab.email_subject_suffix}" if Gitlab.config.gitlab.email_subject_suffix.present? subject << " | #{Gitlab.config.gitlab.email_subject_suffix}" if Gitlab.config.gitlab.email_subject_suffix.present?
subject subject
...@@ -117,10 +118,9 @@ class Notify < BaseMailer ...@@ -117,10 +118,9 @@ class Notify < BaseMailer
@reason = headers['X-GitLab-NotificationReason'] @reason = headers['X-GitLab-NotificationReason']
if Gitlab::IncomingEmail.enabled? && @sent_notification if Gitlab::IncomingEmail.enabled? && @sent_notification
address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) headers['Reply-To'] = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)).tap do |address|
address.display_name = @project.full_name address.display_name = reply_display_name(model)
end
headers['Reply-To'] = address
fallback_reply_message_id = "<reply-#{reply_key}@#{Gitlab.config.gitlab.host}>".freeze fallback_reply_message_id = "<reply-#{reply_key}@#{Gitlab.config.gitlab.host}>".freeze
headers['References'] ||= [] headers['References'] ||= []
...@@ -132,6 +132,11 @@ class Notify < BaseMailer ...@@ -132,6 +132,11 @@ class Notify < BaseMailer
mail(headers) mail(headers)
end end
# `model` is used on EE code
def reply_display_name(_model)
@project.full_name
end
# Send an email that starts a new conversation thread, # Send an email that starts a new conversation thread,
# with headers suitable for grouping by thread in email clients. # with headers suitable for grouping by thread in email clients.
# #
......
...@@ -317,10 +317,6 @@ class Note < ActiveRecord::Base ...@@ -317,10 +317,6 @@ class Note < ActiveRecord::Base
!system? && !for_snippet? !system? && !for_snippet?
end end
def can_create_notification?
true
end
def discussion_class(noteable = nil) def discussion_class(noteable = nil)
# When commit notes are rendered on an MR's Discussion page, they are # When commit notes are rendered on an MR's Discussion page, they are
# displayed in one discussion instead of individually. # displayed in one discussion instead of individually.
......
...@@ -5,14 +5,14 @@ class SentNotification < ActiveRecord::Base ...@@ -5,14 +5,14 @@ class SentNotification < ActiveRecord::Base
belongs_to :noteable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :noteable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :recipient, class_name: "User" belongs_to :recipient, class_name: "User"
validates :project, :recipient, presence: true validates :recipient, presence: true
validates :reply_key, presence: true, uniqueness: true validates :reply_key, presence: true, uniqueness: true
validates :noteable_id, presence: true, unless: :for_commit? validates :noteable_id, presence: true, unless: :for_commit?
validates :commit_id, presence: true, if: :for_commit? validates :commit_id, presence: true, if: :for_commit?
validates :in_reply_to_discussion_id, format: { with: /\A\h{40}\z/, allow_nil: true } validates :in_reply_to_discussion_id, format: { with: /\A\h{40}\z/, allow_nil: true }
validate :note_valid validate :note_valid
after_save :keep_around_commit after_save :keep_around_commit, if: :for_commit?
class << self class << self
def reply_key def reply_key
......
...@@ -45,6 +45,10 @@ module NotificationRecipientService ...@@ -45,6 +45,10 @@ module NotificationRecipientService
target.project target.project
end end
def group
project&.group || target.try(:group)
end
def recipients def recipients
@recipients ||= [] @recipients ||= []
end end
...@@ -67,6 +71,7 @@ module NotificationRecipientService ...@@ -67,6 +71,7 @@ module NotificationRecipientService
user, type, user, type,
reason: reason, reason: reason,
project: project, project: project,
group: group,
custom_action: custom_action, custom_action: custom_action,
target: target, target: target,
acting_user: acting_user acting_user: acting_user
...@@ -107,11 +112,11 @@ module NotificationRecipientService ...@@ -107,11 +112,11 @@ module NotificationRecipientService
# Users with a notification setting on group or project # Users with a notification setting on group or project
user_ids += user_ids_notifiable_on(project, :custom) user_ids += user_ids_notifiable_on(project, :custom)
user_ids += user_ids_notifiable_on(project.group, :custom) user_ids += user_ids_notifiable_on(group, :custom)
# Users with global level custom # Users with global level custom
user_ids_with_project_level_global = user_ids_notifiable_on(project, :global) user_ids_with_project_level_global = user_ids_notifiable_on(project, :global)
user_ids_with_group_level_global = user_ids_notifiable_on(project.group, :global) user_ids_with_group_level_global = user_ids_notifiable_on(group, :global)
global_users_ids = user_ids_with_project_level_global.concat(user_ids_with_group_level_global) global_users_ids = user_ids_with_project_level_global.concat(user_ids_with_group_level_global)
user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action) user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action)
...@@ -123,6 +128,10 @@ module NotificationRecipientService ...@@ -123,6 +128,10 @@ module NotificationRecipientService
add_recipients(project_watchers, :watch, nil) add_recipients(project_watchers, :watch, nil)
end end
def add_group_watchers
add_recipients(group_watchers, :watch, nil)
end
# Get project users with WATCH notification level # Get project users with WATCH notification level
def project_watchers def project_watchers
project_members_ids = user_ids_notifiable_on(project) project_members_ids = user_ids_notifiable_on(project)
...@@ -138,6 +147,14 @@ module NotificationRecipientService ...@@ -138,6 +147,14 @@ module NotificationRecipientService
user_scope.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq) user_scope.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq)
end end
def group_watchers
user_ids_with_group_global = user_ids_notifiable_on(group, :global)
user_ids = user_ids_with_global_level_watch(user_ids_with_group_global)
user_ids_with_group_setting = select_group_members_ids(group, [], user_ids_with_group_global, user_ids)
user_scope.where(id: user_ids_with_group_setting)
end
def add_subscribed_users def add_subscribed_users
return unless target.respond_to? :subscribers return unless target.respond_to? :subscribers
...@@ -281,6 +298,14 @@ module NotificationRecipientService ...@@ -281,6 +298,14 @@ module NotificationRecipientService
note.project note.project
end end
def group
if note.for_project_noteable?
project.group
else
target.try(:group)
end
end
def build! def build!
# Add all users participating in the thread (author, assignee, comment authors) # Add all users participating in the thread (author, assignee, comment authors)
add_participants(note.author) add_participants(note.author)
...@@ -289,11 +314,11 @@ module NotificationRecipientService ...@@ -289,11 +314,11 @@ module NotificationRecipientService
if note.for_project_noteable? if note.for_project_noteable?
# Merge project watchers # Merge project watchers
add_project_watchers add_project_watchers
else
# Merge project with custom notification add_group_watchers
add_custom_notifications
end end
add_custom_notifications
add_subscribed_users add_subscribed_users
end end
......
...@@ -5,7 +5,7 @@ class NewNoteWorker ...@@ -5,7 +5,7 @@ class NewNoteWorker
# old `NewNoteWorker` jobs (can remove later) # old `NewNoteWorker` jobs (can remove later)
def perform(note_id, _params = {}) def perform(note_id, _params = {})
if note = Note.find_by(id: note_id) if note = Note.find_by(id: note_id)
NotificationService.new.new_note(note) if note.can_create_notification? NotificationService.new.new_note(note)
Notes::PostProcessService.new(note).execute Notes::PostProcessService.new(note).execute
else else
Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job") Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")
......
...@@ -8,6 +8,7 @@ module Gitlab ...@@ -8,6 +8,7 @@ module Gitlab
include ReplyProcessing include ReplyProcessing
delegate :project, to: :sent_notification, allow_nil: true delegate :project, to: :sent_notification, allow_nil: true
delegate :noteable, to: :sent_notification
def can_handle? def can_handle?
mail_key =~ /\A\w+\z/ mail_key =~ /\A\w+\z/
...@@ -18,7 +19,7 @@ module Gitlab ...@@ -18,7 +19,7 @@ module Gitlab
validate_permission!(:create_note) validate_permission!(:create_note)
raise NoteableNotFoundError unless sent_notification.noteable raise NoteableNotFoundError unless noteable
raise EmptyEmailError if message.blank? raise EmptyEmailError if message.blank?
verify_record!( verify_record!(
......
...@@ -32,8 +32,12 @@ module Gitlab ...@@ -32,8 +32,12 @@ module Gitlab
def validate_permission!(permission) def validate_permission!(permission)
raise UserNotFoundError unless author raise UserNotFoundError unless author
raise UserBlockedError if author.blocked? raise UserBlockedError if author.blocked?
if project
raise ProjectNotFound unless author.can?(:read_project, project) raise ProjectNotFound unless author.can?(:read_project, project)
raise UserNotAuthorizedError unless author.can?(permission, project) end
raise UserNotAuthorizedError unless author.can?(permission, project || noteable)
end end
def verify_record!(record:, invalid_exception:, record_name:) def verify_record!(record:, invalid_exception:, record_name:)
......
require 'spec_helper' require 'spec_helper'
require_relative '../email_shared_blocks'
describe Gitlab::Email::Handler::CreateIssueHandler do describe Gitlab::Email::Handler::CreateIssueHandler do
include_context :email_shared_context include_context :email_shared_context
......
require 'spec_helper' require 'spec_helper'
require_relative '../email_shared_blocks'
describe Gitlab::Email::Handler::CreateMergeRequestHandler do describe Gitlab::Email::Handler::CreateMergeRequestHandler do
include_context :email_shared_context include_context :email_shared_context
......
require 'spec_helper' require 'spec_helper'
require_relative '../email_shared_blocks'
describe Gitlab::Email::Handler::CreateNoteHandler do describe Gitlab::Email::Handler::CreateNoteHandler do
include_context :email_shared_context include_context :email_shared_context
......
require 'spec_helper' require 'spec_helper'
require_relative '../email_shared_blocks'
describe Gitlab::Email::Handler::UnsubscribeHandler do describe Gitlab::Email::Handler::UnsubscribeHandler do
include_context :email_shared_context include_context :email_shared_context
......
require 'spec_helper' require 'spec_helper'
require_relative 'email_shared_blocks'
describe Gitlab::Email::Receiver do describe Gitlab::Email::Receiver do
include_context :email_shared_context include_context :email_shared_context
......
...@@ -654,38 +654,6 @@ describe Notify do ...@@ -654,38 +654,6 @@ describe Notify do
allow(Note).to receive(:find).with(note.id).and_return(note) allow(Note).to receive(:find).with(note.id).and_return(note)
end end
shared_examples 'a note email' do
it_behaves_like 'it should have Gmail Actions links'
it 'is sent to the given recipient as the author' do
sender = subject.header[:from].addrs[0]
aggregate_failures do
expect(sender.display_name).to eq(note_author.name)
expect(sender.address).to eq(gitlab_sender)
expect(subject).to deliver_to(recipient.notification_email)
end
end
it 'contains the message from the note' do
is_expected.to have_html_escaped_body_text note.note
end
it 'does not contain note author' do
is_expected.not_to have_body_text note.author_name
end
context 'when enabled email_author_in_body' do
before do
stub_application_setting(email_author_in_body: true)
end
it 'contains a link to note author' do
is_expected.to have_html_escaped_body_text note.author_name
end
end
end
describe 'on a commit' do describe 'on a commit' do
let(:commit) { project.commit } let(:commit) { project.commit }
......
...@@ -2,6 +2,7 @@ require 'spec_helper' ...@@ -2,6 +2,7 @@ require 'spec_helper'
describe NotificationService, :mailer do describe NotificationService, :mailer do
include EmailSpec::Matchers include EmailSpec::Matchers
include NotificationHelpers
let(:notification) { described_class.new } let(:notification) { described_class.new }
let(:assignee) { create(:user) } let(:assignee) { create(:user) }
...@@ -13,12 +14,6 @@ describe NotificationService, :mailer do ...@@ -13,12 +14,6 @@ describe NotificationService, :mailer do
end end
shared_examples 'notifications for new mentions' do shared_examples 'notifications for new mentions' do
def send_notifications(*new_mentions)
mentionable.description = new_mentions.map(&:to_reference).join(' ')
notification.send(notification_method, mentionable, new_mentions, @u_disabled)
end
it 'sends no emails when no new mentions are present' do it 'sends no emails when no new mentions are present' do
send_notifications send_notifications
should_not_email_anyone should_not_email_anyone
...@@ -1914,30 +1909,6 @@ describe NotificationService, :mailer do ...@@ -1914,30 +1909,6 @@ describe NotificationService, :mailer do
group group
end end
def create_global_setting_for(user, level)
setting = user.global_notification_setting
setting.level = level
setting.save
user
end
def create_user_with_notification(level, username, resource = project)
user = create(:user, username: username)
setting = user.notification_settings_for(resource)
setting.level = level
setting.save
user
end
# Create custom notifications
# When resource is nil it means global notification
def update_custom_notification(event, user, resource: nil, value: true)
setting = user.notification_settings_for(resource)
setting.update!(event => value)
end
def add_users_with_subscription(project, issuable) def add_users_with_subscription(project, issuable)
@subscriber = create :user @subscriber = create :user
@unsubscriber = create :user @unsubscriber = create :user
......
module NotificationHelpers
extend self
def send_notifications(*new_mentions)
mentionable.description = new_mentions.map(&:to_reference).join(' ')
notification.send(notification_method, mentionable, new_mentions, @u_disabled)
end
def create_global_setting_for(user, level)
setting = user.global_notification_setting
setting.level = level
setting.save
user
end
def create_user_with_notification(level, username, resource = project)
user = create(:user, username: username)
setting = user.notification_settings_for(resource)
setting.level = level
setting.save
user
end
# Create custom notifications
# When resource is nil it means global notification
def update_custom_notification(event, user, resource: nil, value: true)
setting = user.notification_settings_for(resource)
setting.update!(event => value)
end
end
...@@ -197,3 +197,35 @@ end ...@@ -197,3 +197,35 @@ end
shared_examples 'an email with a labels subscriptions link in its footer' do shared_examples 'an email with a labels subscriptions link in its footer' do
it { is_expected.to have_body_text('label subscriptions') } it { is_expected.to have_body_text('label subscriptions') }
end end
shared_examples 'a note email' do
it_behaves_like 'it should have Gmail Actions links'
it 'is sent to the given recipient as the author' do
sender = subject.header[:from].addrs[0]
aggregate_failures do
expect(sender.display_name).to eq(note_author.name)
expect(sender.address).to eq(gitlab_sender)
expect(subject).to deliver_to(recipient.notification_email)
end
end
it 'contains the message from the note' do
is_expected.to have_html_escaped_body_text note.note
end
it 'does not contain note author' do
is_expected.not_to have_body_text note.author_name
end
context 'when enabled email_author_in_body' do
before do
stub_application_setting(email_author_in_body: true)
end
it 'contains a link to note author' do
is_expected.to have_html_escaped_body_text note.author_name
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