Add latest changes from gitlab-org/gitlab@master

9 files changed, 277 insertions, 241 deletions

diff --git a/app/services/notification_recipients/build_service.rb b/app/services/notification_recipients/build_service.rb
new file mode 100644
index 00000000000..67f9849aece
--- /dev/null
+++ b/app/services/notification_recipients/build_service.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+#
+# Used by NotificationService to determine who should receive notification
+#
+module NotificationRecipients
+  module BuildService
+    def self.notifiable_users(users, *args)
+      users.compact.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
+    end
+
+    def self.notifiable?(user, *args)
+      NotificationRecipient.new(user, *args).notifiable?
+    end
+
+    def self.build_recipients(*args)
+      Builder::Default.new(*args).notification_recipients
+    end
+
+    def self.build_new_note_recipients(*args)
+      Builder::NewNote.new(*args).notification_recipients
+    end
+
+    def self.build_merge_request_unmergeable_recipients(*args)
+      Builder::MergeRequestUnmergeable.new(*args).notification_recipients
+    end
+
+    def self.build_project_maintainers_recipients(*args)
+      Builder::ProjectMaintainers.new(*args).notification_recipients
+    end
+
+    def self.build_new_release_recipients(*args)
+      Builder::NewRelease.new(*args).notification_recipients
+    end
+  end
+end
+
+NotificationRecipients::BuildService.prepend_if_ee('EE::NotificationRecipients::BuildService')
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipients/builder/base.rb
index 0bdf6a0e6bc..3aa00c09ba2 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipients/builder/base.rb
@@ -1,37 +1,6 @@
 # frozen_string_literal: true
 
-#
-# Used by NotificationService to determine who should receive notification
-#
-module NotificationRecipientService
-  def self.notifiable_users(users, *args)
-    users.compact.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
-  end
-
-  def self.notifiable?(user, *args)
-    NotificationRecipient.new(user, *args).notifiable?
-  end
-
-  def self.build_recipients(*args)
-    Builder::Default.new(*args).notification_recipients
-  end
-
-  def self.build_new_note_recipients(*args)
-    Builder::NewNote.new(*args).notification_recipients
-  end
-
-  def self.build_merge_request_unmergeable_recipients(*args)
-    Builder::MergeRequestUnmergeable.new(*args).notification_recipients
-  end
-
-  def self.build_project_maintainers_recipients(*args)
-    Builder::ProjectMaintainers.new(*args).notification_recipients
-  end
-
-  def self.build_new_release_recipients(*args)
-    Builder::NewRelease.new(*args).notification_recipients
-  end
-
+module NotificationRecipients
   module Builder
     class Base
       def initialize(*)
@@ -244,186 +213,5 @@ module NotificationRecipientService
         end
       end
     end
-
-    class Default < Base
-      MENTION_TYPE_ACTIONS = [:new_issue, :new_merge_request].freeze
-
-      attr_reader :target
-      attr_reader :current_user
-      attr_reader :action
-      attr_reader :previous_assignees
-      attr_reader :skip_current_user
-
-      def initialize(target, current_user, action:, custom_action: nil, previous_assignees: nil, skip_current_user: true)
-        @target = target
-        @current_user = current_user
-        @action = action
-        @custom_action = custom_action
-        @previous_assignees = previous_assignees
-        @skip_current_user = skip_current_user
-      end
-
-      def add_watchers
-        add_project_watchers
-      end
-
-      def build!
-        add_participants(current_user)
-        add_watchers
-        add_custom_notifications
-
-        # Re-assign is considered as a mention of the new assignee
-        case custom_action
-        when :reassign_merge_request, :reassign_issue
-          add_recipients(previous_assignees, :mention, nil)
-          add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED)
-        end
-
-        add_subscribed_users
-
-        if self.class.mention_type_actions.include?(custom_action)
-          # These will all be participants as well, but adding with the :mention
-          # type ensures that users with the mention notification level will
-          # receive them, too.
-          add_mentions(current_user, target: target)
-
-          # We use the `:participating` notification level in order to match existing legacy behavior as captured
-          # in existing specs (notification_service_spec.rb ~ line 507)
-          if target.is_a?(Issuable)
-            add_recipients(target.assignees, :participating, NotificationReason::ASSIGNED)
-          end
-
-          add_labels_subscribers
-        end
-      end
-
-      def acting_user
-        current_user if skip_current_user
-      end
-
-      # Build event key to search on custom notification level
-      # Check NotificationSetting.email_events
-      def custom_action
-        @custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym
-      end
-
-      def self.mention_type_actions
-        MENTION_TYPE_ACTIONS.dup
-      end
-    end
-
-    class NewNote < Base
-      attr_reader :note
-      def initialize(note)
-        @note = note
-      end
-
-      def target
-        note.noteable
-      end
-
-      # NOTE: may be nil, in the case of a PersonalSnippet
-      #
-      # (this is okay because NotificationRecipient is written
-      # to handle nil projects)
-      def project
-        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)
-        add_mentions(note.author, target: note)
-
-        if note.for_project_noteable?
-          # Merge project watchers
-          add_project_watchers
-        else
-          add_group_watchers
-        end
-
-        add_custom_notifications
-        add_subscribed_users
-      end
-
-      def custom_action
-        :new_note
-      end
-
-      def acting_user
-        note.author
-      end
-    end
-
-    class NewRelease < Base
-      attr_reader :target
-
-      def initialize(target)
-        @target = target
-      end
-
-      def build!
-        add_recipients(target.project.authorized_users, :custom, nil)
-      end
-
-      def custom_action
-        :new_release
-      end
-
-      def acting_user
-        target.author
-      end
-    end
-
-    class MergeRequestUnmergeable < Base
-      attr_reader :target
-      def initialize(merge_request)
-        @target = merge_request
-      end
-
-      def build!
-        target.merge_participants.each do |user|
-          add_recipients(user, :participating, nil)
-        end
-      end
-
-      def custom_action
-        :unmergeable_merge_request
-      end
-
-      def acting_user
-        nil
-      end
-    end
-
-    class ProjectMaintainers < Base
-      attr_reader :target
-
-      def initialize(target, action:)
-        @target = target
-        @action = action
-      end
-
-      def build!
-        return [] unless project
-
-        add_recipients(project.team.maintainers, :mention, nil)
-      end
-
-      def acting_user
-        nil
-      end
-    end
   end
 end
-
-NotificationRecipientService::Builder::Default.prepend_if_ee('EE::NotificationRecipientBuilders::Default') # rubocop: disable Cop/InjectEnterpriseEditionModule
-NotificationRecipientService.prepend_if_ee('EE::NotificationRecipientService')
diff --git a/app/services/notification_recipients/builder/default.rb b/app/services/notification_recipients/builder/default.rb
new file mode 100644
index 00000000000..790ce57452c
--- /dev/null
+++ b/app/services/notification_recipients/builder/default.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+module NotificationRecipients
+  module Builder
+    class Default < Base
+      MENTION_TYPE_ACTIONS = [:new_issue, :new_merge_request].freeze
+
+      attr_reader :target
+      attr_reader :current_user
+      attr_reader :action
+      attr_reader :previous_assignees
+      attr_reader :skip_current_user
+
+      def initialize(target, current_user, action:, custom_action: nil, previous_assignees: nil, skip_current_user: true)
+        @target = target
+        @current_user = current_user
+        @action = action
+        @custom_action = custom_action
+        @previous_assignees = previous_assignees
+        @skip_current_user = skip_current_user
+      end
+
+      def add_watchers
+        add_project_watchers
+      end
+
+      def build!
+        add_participants(current_user)
+        add_watchers
+        add_custom_notifications
+
+        # Re-assign is considered as a mention of the new assignee
+        case custom_action
+        when :reassign_merge_request, :reassign_issue
+          add_recipients(previous_assignees, :mention, nil)
+          add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED)
+        end
+
+        add_subscribed_users
+
+        if self.class.mention_type_actions.include?(custom_action)
+          # These will all be participants as well, but adding with the :mention
+          # type ensures that users with the mention notification level will
+          # receive them, too.
+          add_mentions(current_user, target: target)
+
+          # We use the `:participating` notification level in order to match existing legacy behavior as captured
+          # in existing specs (notification_service_spec.rb ~ line 507)
+          if target.is_a?(Issuable)
+            add_recipients(target.assignees, :participating, NotificationReason::ASSIGNED)
+          end
+
+          add_labels_subscribers
+        end
+      end
+
+      def acting_user
+        current_user if skip_current_user
+      end
+
+      # Build event key to search on custom notification level
+      # Check NotificationSetting.email_events
+      def custom_action
+        @custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym
+      end
+
+      def self.mention_type_actions
+        MENTION_TYPE_ACTIONS.dup
+      end
+    end
+  end
+end
+
+NotificationRecipients::Builder::Default.prepend_if_ee('EE::NotificationRecipients::Builder::Default')
diff --git a/app/services/notification_recipients/builder/merge_request_unmergeable.rb b/app/services/notification_recipients/builder/merge_request_unmergeable.rb
new file mode 100644
index 00000000000..24d96b98002
--- /dev/null
+++ b/app/services/notification_recipients/builder/merge_request_unmergeable.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module NotificationRecipients
+  module Builder
+    class MergeRequestUnmergeable < Base
+      attr_reader :target
+      def initialize(merge_request)
+        @target = merge_request
+      end
+
+      def build!
+        target.merge_participants.each do |user|
+          add_recipients(user, :participating, nil)
+        end
+      end
+
+      def custom_action
+        :unmergeable_merge_request
+      end
+
+      def acting_user
+        nil
+      end
+    end
+  end
+end
diff --git a/app/services/notification_recipients/builder/new_note.rb b/app/services/notification_recipients/builder/new_note.rb
new file mode 100644
index 00000000000..27699a0d9cc
--- /dev/null
+++ b/app/services/notification_recipients/builder/new_note.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+module NotificationRecipients
+  module Builder
+    class NewNote < Base
+      attr_reader :note
+      def initialize(note)
+        @note = note
+      end
+
+      def target
+        note.noteable
+      end
+
+      # NOTE: may be nil, in the case of a PersonalSnippet
+      #
+      # (this is okay because NotificationRecipient is written
+      # to handle nil projects)
+      def project
+        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)
+        add_mentions(note.author, target: note)
+
+        if note.for_project_noteable?
+          # Merge project watchers
+          add_project_watchers
+        else
+          add_group_watchers
+        end
+
+        add_custom_notifications
+        add_subscribed_users
+      end
+
+      def custom_action
+        :new_note
+      end
+
+      def acting_user
+        note.author
+      end
+    end
+  end
+end
diff --git a/app/services/notification_recipients/builder/new_release.rb b/app/services/notification_recipients/builder/new_release.rb
new file mode 100644
index 00000000000..67676b6eec8
--- /dev/null
+++ b/app/services/notification_recipients/builder/new_release.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module NotificationRecipients
+  module Builder
+    class NewRelease < Base
+      attr_reader :target
+
+      def initialize(target)
+        @target = target
+      end
+
+      def build!
+        add_recipients(target.project.authorized_users, :custom, nil)
+      end
+
+      def custom_action
+        :new_release
+      end
+
+      def acting_user
+        target.author
+      end
+    end
+  end
+end
diff --git a/app/services/notification_recipients/builder/project_maintainers.rb b/app/services/notification_recipients/builder/project_maintainers.rb
new file mode 100644
index 00000000000..e8f22c00a83
--- /dev/null
+++ b/app/services/notification_recipients/builder/project_maintainers.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module NotificationRecipients
+  module Builder
+    class ProjectMaintainers < Base
+      attr_reader :target
+
+      def initialize(target, action:)
+        @target = target
+        @action = action
+      end
+
+      def build!
+        return [] unless project
+
+        add_recipients(project.team.maintainers, :mention, nil)
+      end
+
+      def acting_user
+        nil
+      end
+    end
+  end
+end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 6f2bfa8169b..6b92e5a5625 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -108,7 +108,7 @@ class NotificationService
   #  * users with custom level checked with "reassign issue"
   #
   def reassigned_issue(issue, current_user, previous_assignees = [])
-    recipients = NotificationRecipientService.build_recipients(
+    recipients = NotificationRecipients::BuildService.build_recipients(
       issue,
       current_user,
       action: "reassign",
@@ -161,7 +161,7 @@ class NotificationService
   def push_to_merge_request(merge_request, current_user, new_commits: [], existing_commits: [])
     new_commits = new_commits.map { |c| { short_id: c.short_id, title: c.title } }
     existing_commits = existing_commits.map { |c| { short_id: c.short_id, title: c.title } }
-    recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: "push_to")
+    recipients = NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: "push_to")
 
     recipients.each do |recipient|
       mailer.send(:push_to_merge_request_email, recipient.user.id, merge_request.id, current_user.id, recipient.reason, new_commits: new_commits, existing_commits: existing_commits).deliver_later
@@ -197,7 +197,7 @@ class NotificationService
   #  * users with custom level checked with "reassign merge request"
   #
   def reassigned_merge_request(merge_request, current_user, previous_assignees = [])
-    recipients = NotificationRecipientService.build_recipients(
+    recipients = NotificationRecipients::BuildService.build_recipients(
       merge_request,
       current_user,
       action: "reassign",
@@ -260,7 +260,7 @@ class NotificationService
   end
 
   def resolve_all_discussions(merge_request, current_user)
-    recipients = NotificationRecipientService.build_recipients(
+    recipients = NotificationRecipients::BuildService.build_recipients(
       merge_request,
       current_user,
       action: "resolve_all_discussions")
@@ -291,7 +291,7 @@ class NotificationService
   def send_new_note_notifications(note)
     notify_method = "note_#{note.noteable_ability_name}_email".to_sym
 
-    recipients = NotificationRecipientService.build_new_note_recipients(note)
+    recipients = NotificationRecipients::BuildService.build_new_note_recipients(note)
     recipients.each do |recipient|
       mailer.send(notify_method, recipient.user.id, note.id, recipient.reason).deliver_later
     end
@@ -299,7 +299,7 @@ class NotificationService
 
   # Notify users when a new release is created
   def send_new_release_notifications(release)
-    recipients = NotificationRecipientService.build_new_release_recipients(release)
+    recipients = NotificationRecipients::BuildService.build_new_release_recipients(release)
 
     recipients.each do |recipient|
       mailer.new_release_email(recipient.user.id, release, recipient.reason).deliver_later
@@ -413,7 +413,7 @@ class NotificationService
   end
 
   def issue_moved(issue, new_issue, current_user)
-    recipients = NotificationRecipientService.build_recipients(issue, current_user, action: 'moved')
+    recipients = NotificationRecipients::BuildService.build_recipients(issue, current_user, action: 'moved')
 
     recipients.map do |recipient|
       email = mailer.issue_moved_email(recipient.user, issue, new_issue, current_user, recipient.reason)
@@ -490,7 +490,7 @@ class NotificationService
   end
 
   def issue_due(issue)
-    recipients = NotificationRecipientService.build_recipients(
+    recipients = NotificationRecipients::BuildService.build_recipients(
       issue,
       issue.author,
       action: 'due',
@@ -526,7 +526,7 @@ class NotificationService
   protected
 
   def new_resource_email(target, method)
-    recipients = NotificationRecipientService.build_recipients(target, target.author, action: "new")
+    recipients = NotificationRecipients::BuildService.build_recipients(target, target.author, action: "new")
 
     recipients.each do |recipient|
       mailer.send(method, recipient.user.id, target.id, recipient.reason).deliver_later
@@ -534,7 +534,7 @@ class NotificationService
   end
 
   def new_mentions_in_resource_email(target, new_mentioned_users, current_user, method)
-    recipients = NotificationRecipientService.build_recipients(target, current_user, action: "new")
+    recipients = NotificationRecipients::BuildService.build_recipients(target, current_user, action: "new")
     recipients = recipients.select {|r| new_mentioned_users.include?(r.user) }
 
     recipients.each do |recipient|
@@ -545,7 +545,7 @@ class NotificationService
   def close_resource_email(target, current_user, method, skip_current_user: true, closed_via: nil)
     action = method == :merged_merge_request_email ? "merge" : "close"
 
-    recipients = NotificationRecipientService.build_recipients(
+    recipients = NotificationRecipients::BuildService.build_recipients(
       target,
       current_user,
       action: action,
@@ -573,7 +573,7 @@ class NotificationService
   end
 
   def removed_milestone_resource_email(target, current_user, method)
-    recipients = NotificationRecipientService.build_recipients(
+    recipients = NotificationRecipients::BuildService.build_recipients(
       target,
       current_user,
       action: 'removed_milestone'
@@ -585,7 +585,7 @@ class NotificationService
   end
 
   def changed_milestone_resource_email(target, milestone, current_user, method)
-    recipients = NotificationRecipientService.build_recipients(
+    recipients = NotificationRecipients::BuildService.build_recipients(
       target,
       current_user,
       action: 'changed_milestone'
@@ -597,7 +597,7 @@ class NotificationService
   end
 
   def reopen_resource_email(target, current_user, method, status)
-    recipients = NotificationRecipientService.build_recipients(target, current_user, action: "reopen")
+    recipients = NotificationRecipients::BuildService.build_recipients(target, current_user, action: "reopen")
 
     recipients.each do |recipient|
       mailer.send(method, recipient.user.id, target.id, status, current_user.id, recipient.reason).deliver_later
@@ -605,7 +605,7 @@ class NotificationService
   end
 
   def merge_request_unmergeable_email(merge_request)
-    recipients = NotificationRecipientService.build_merge_request_unmergeable_recipients(merge_request)
+    recipients = NotificationRecipients::BuildService.build_merge_request_unmergeable_recipients(merge_request)
 
     recipients.each do |recipient|
       mailer.merge_request_unmergeable_email(recipient.user.id, merge_request.id).deliver_later
@@ -619,15 +619,15 @@ class NotificationService
   private
 
   def project_maintainers_recipients(target, action:)
-    NotificationRecipientService.build_project_maintainers_recipients(target, action: action)
+    NotificationRecipients::BuildService.build_project_maintainers_recipients(target, action: action)
   end
 
   def notifiable?(*args)
-    NotificationRecipientService.notifiable?(*args)
+    NotificationRecipients::BuildService.notifiable?(*args)
   end
 
   def notifiable_users(*args)
-    NotificationRecipientService.notifiable_users(*args)
+    NotificationRecipients::BuildService.notifiable_users(*args)
   end
 
   def deliver_access_request_email(recipient, member)
diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb
index cc645c514b7..2998208f50b 100644
--- a/app/services/snippets/create_service.rb
+++ b/app/services/snippets/create_service.rb
@@ -38,25 +38,30 @@ module Snippets
     private
 
     def save_and_commit(snippet)
-      snippet.with_transaction_returning_status do
+      result = snippet.with_transaction_returning_status do
         (snippet.save && snippet.store_mentions!).tap do |saved|
           break false unless saved
 
           if Feature.enabled?(:version_snippets, current_user)
             create_repository_for(snippet)
-            create_commit(snippet)
           end
         end
-      rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ...
-        snippet.errors.add(:base, e.message)
+      end
 
-        # If the commit action failed we need to remove the repository if exists
-        if snippet.repository_exists?
-          Repositories::DestroyService.new(snippet.repository).execute
-        end
+      create_commit(snippet) if result && snippet.repository_exists?
 
-        false
-      end
+      result
+    rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ...
+      snippet.errors.add(:base, e.message)
+
+      # If the commit action failed we need to remove the repository if exists
+      snippet.repository.remove if snippet.repository_exists?
+
+      # If the snippet was created, we need to remove it as we
+      # would do like if it had had any validation error
+      snippet.delete if snippet.persisted?
+
+      false
     end
 
     def create_repository_for(snippet)