diff options
19 files changed, 131 insertions, 96 deletions
diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb index 04c36b3ebfe..93a71103a09 100644 --- a/app/controllers/sent_notifications_controller.rb +++ b/app/controllers/sent_notifications_controller.rb @@ -17,16 +17,20 @@ class SentNotificationsController < ApplicationController flash[:notice] = "You have been unsubscribed from this thread." if current_user - case noteable - when Issue - redirect_to issue_path(noteable) - when MergeRequest - redirect_to merge_request_path(noteable) - else - redirect_to root_path - end + redirect_to noteable_path(noteable) else redirect_to new_user_session_path end end + + def noteable_path(noteable) + case noteable + when Issue + issue_path(noteable) + when MergeRequest + merge_request_path(noteable) + else + root_path + end + end end diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index 50e17fe7717..d9a6fe2a41e 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -43,7 +43,7 @@ module Emails private def note_target_url_options - [@project, @note.noteable, anchor: "note_#{@note.id}"] + [@project || @group, @note.noteable, anchor: "note_#{@note.id}"] end def note_thread_options(recipient_id) @@ -58,8 +58,9 @@ 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) - if @project && @note.persisted? + if (@project || @group) && @note.persisted? @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key) end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 3646e08a15f..1db1482d6b7 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -94,6 +94,7 @@ class Notify < BaseMailer def subject(*extra) subject = "" subject << "#{@project.name} | " if @project + subject << "#{@group.name} | " if @group subject << extra.join(' | ') if extra.present? subject << " | #{Gitlab.config.gitlab.email_subject_suffix}" if Gitlab.config.gitlab.email_subject_suffix.present? subject @@ -117,10 +118,9 @@ class Notify < BaseMailer @reason = headers['X-GitLab-NotificationReason'] if Gitlab::IncomingEmail.enabled? && @sent_notification - address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) - address.display_name = @project.full_name - - headers['Reply-To'] = address + headers['Reply-To'] = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)).tap do |address| + address.display_name = reply_display_name(model) + end fallback_reply_message_id = "<reply-#{reply_key}@#{Gitlab.config.gitlab.host}>".freeze headers['References'] ||= [] @@ -132,6 +132,11 @@ class Notify < BaseMailer mail(headers) 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, # with headers suitable for grouping by thread in email clients. # diff --git a/app/models/note.rb b/app/models/note.rb index e426f84832b..109405d3f17 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -317,10 +317,6 @@ class Note < ActiveRecord::Base !system? && !for_snippet? end - def can_create_notification? - true - end - def discussion_class(noteable = nil) # When commit notes are rendered on an MR's Discussion page, they are # displayed in one discussion instead of individually. diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index 6e311806be1..3da7c301d28 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -5,14 +5,14 @@ class SentNotification < ActiveRecord::Base belongs_to :noteable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :recipient, class_name: "User" - validates :project, :recipient, presence: true + validates :recipient, presence: true validates :reply_key, presence: true, uniqueness: true validates :noteable_id, presence: true, unless: :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 } validate :note_valid - after_save :keep_around_commit + after_save :keep_around_commit, if: :for_commit? class << self def reply_key diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 83e59a649b6..5658699664d 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -45,6 +45,10 @@ module NotificationRecipientService target.project end + def group + project&.group || target.try(:group) + end + def recipients @recipients ||= [] end @@ -67,6 +71,7 @@ module NotificationRecipientService user, type, reason: reason, project: project, + group: group, custom_action: custom_action, target: target, acting_user: acting_user @@ -107,11 +112,11 @@ module NotificationRecipientService # Users with a notification setting on group or project 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 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) user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action) @@ -123,6 +128,10 @@ module NotificationRecipientService add_recipients(project_watchers, :watch, nil) end + def add_group_watchers + add_recipients(group_watchers, :watch, nil) + end + # Get project users with WATCH notification level def project_watchers project_members_ids = user_ids_notifiable_on(project) @@ -138,6 +147,14 @@ module NotificationRecipientService user_scope.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq) 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 return unless target.respond_to? :subscribers @@ -281,6 +298,14 @@ module NotificationRecipientService note.project end + def group + if note.for_project_noteable? + project.group + else + target.try(:group) + end + end + def build! # Add all users participating in the thread (author, assignee, comment authors) add_participants(note.author) @@ -289,11 +314,11 @@ module NotificationRecipientService if note.for_project_noteable? # Merge project watchers add_project_watchers - - # Merge project with custom notification - add_custom_notifications + else + add_group_watchers end + add_custom_notifications add_subscribed_users end diff --git a/app/workers/new_note_worker.rb b/app/workers/new_note_worker.rb index b925741934a..67c54fbf10e 100644 --- a/app/workers/new_note_worker.rb +++ b/app/workers/new_note_worker.rb @@ -5,7 +5,7 @@ class NewNoteWorker # old `NewNoteWorker` jobs (can remove later) def perform(note_id, _params = {}) 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 else Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job") diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb index 8eea33b9ab5..5791dbd0484 100644 --- a/lib/gitlab/email/handler/create_note_handler.rb +++ b/lib/gitlab/email/handler/create_note_handler.rb @@ -8,6 +8,7 @@ module Gitlab include ReplyProcessing delegate :project, to: :sent_notification, allow_nil: true + delegate :noteable, to: :sent_notification def can_handle? mail_key =~ /\A\w+\z/ @@ -18,7 +19,7 @@ module Gitlab validate_permission!(:create_note) - raise NoteableNotFoundError unless sent_notification.noteable + raise NoteableNotFoundError unless noteable raise EmptyEmailError if message.blank? verify_record!( diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index 32c5caf93e8..da5ff350549 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -32,8 +32,12 @@ module Gitlab def validate_permission!(permission) raise UserNotFoundError unless author raise UserBlockedError if author.blocked? - raise ProjectNotFound unless author.can?(:read_project, project) - raise UserNotAuthorizedError unless author.can?(permission, project) + + if project + raise ProjectNotFound unless author.can?(:read_project, project) + end + + raise UserNotAuthorizedError unless author.can?(permission, project || noteable) end def verify_record!(record:, invalid_exception:, record_name:) diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb index 6568a0b1bb0..452249210b0 100644 --- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative '../email_shared_blocks' describe Gitlab::Email::Handler::CreateIssueHandler do include_context :email_shared_context diff --git a/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb b/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb index dc1a93367a4..43c6280f251 100644 --- a/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative '../email_shared_blocks' describe Gitlab::Email::Handler::CreateMergeRequestHandler do include_context :email_shared_context diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index 53899e00b53..950a7dd7d6c 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative '../email_shared_blocks' describe Gitlab::Email::Handler::CreateNoteHandler do include_context :email_shared_context diff --git a/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb index 21796694f26..ce160e11de2 100644 --- a/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative '../email_shared_blocks' describe Gitlab::Email::Handler::UnsubscribeHandler do include_context :email_shared_context diff --git a/spec/lib/gitlab/email/receiver_spec.rb b/spec/lib/gitlab/email/receiver_spec.rb index 59f43abf26d..0af978eced3 100644 --- a/spec/lib/gitlab/email/receiver_spec.rb +++ b/spec/lib/gitlab/email/receiver_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative 'email_shared_blocks' describe Gitlab::Email::Receiver do include_context :email_shared_context diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 43e419cd7de..84ddbbbf2ee 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -654,38 +654,6 @@ describe Notify do allow(Note).to receive(:find).with(note.id).and_return(note) 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 let(:commit) { project.commit } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 48ef5f3c115..5f28bc123f3 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' describe NotificationService, :mailer do include EmailSpec::Matchers + include NotificationHelpers let(:notification) { described_class.new } let(:assignee) { create(:user) } @@ -13,12 +14,6 @@ describe NotificationService, :mailer do end 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 send_notifications should_not_email_anyone @@ -1914,30 +1909,6 @@ describe NotificationService, :mailer do group 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) @subscriber = create :user @unsubscriber = create :user diff --git a/spec/support/helpers/notification_helpers.rb b/spec/support/helpers/notification_helpers.rb new file mode 100644 index 00000000000..8d84510fb73 --- /dev/null +++ b/spec/support/helpers/notification_helpers.rb @@ -0,0 +1,33 @@ +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 diff --git a/spec/lib/gitlab/email/email_shared_blocks.rb b/spec/support/shared_contexts/email_shared_blocks.rb index 9d806fc524d..9d806fc524d 100644 --- a/spec/lib/gitlab/email/email_shared_blocks.rb +++ b/spec/support/shared_contexts/email_shared_blocks.rb diff --git a/spec/support/shared_examples/notify_shared_examples.rb b/spec/support/shared_examples/notify_shared_examples.rb index e2c23607406..43fdaddf545 100644 --- a/spec/support/shared_examples/notify_shared_examples.rb +++ b/spec/support/shared_examples/notify_shared_examples.rb @@ -197,3 +197,35 @@ end shared_examples 'an email with a labels subscriptions link in its footer' do it { is_expected.to have_body_text('label subscriptions') } 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 |