Merge branch '5480-epic-notifications-ce' into 'master'

Backport 5480-epic-notifications from EE

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

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

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

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.
   #

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.

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

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
 

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")

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!(

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:)

spec/lib/gitlab/email/handler/create_issue_handler_spec.rb

 require 'spec_helper'
-require_relative '../email_shared_blocks'
 
 describe Gitlab::Email::Handler::CreateIssueHandler do
   include_context :email_shared_context

spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb

 require 'spec_helper'
-require_relative '../email_shared_blocks'
 
 describe Gitlab::Email::Handler::CreateMergeRequestHandler do
   include_context :email_shared_context

spec/lib/gitlab/email/handler/create_note_handler_spec.rb

 require 'spec_helper'
-require_relative '../email_shared_blocks'
 
 describe Gitlab::Email::Handler::CreateNoteHandler do
   include_context :email_shared_context

spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb

 require 'spec_helper'
-require_relative '../email_shared_blocks'
 
 describe Gitlab::Email::Handler::UnsubscribeHandler do
   include_context :email_shared_context

spec/lib/gitlab/email/receiver_spec.rb

 require 'spec_helper'
-require_relative 'email_shared_blocks'
 
 describe Gitlab::Email::Receiver do
   include_context :email_shared_context

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 }
 

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

spec/support/helpers/notification_helpers.rb

+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

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