Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ce into orderable-issues

24 files changed, 127 insertions, 166 deletions

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index ddaaed90e5b..b2a543daa00 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -1,10 +1,16 @@
-AccessTokenValidationService = Struct.new(:token) do
+class AccessTokenValidationService
   # Results:
   VALID = :valid
   EXPIRED = :expired
   REVOKED = :revoked
   INSUFFICIENT_SCOPE = :insufficient_scope
 
+  attr_reader :token
+
+  def initialize(token)
+    @token = token
+  end
+
   def validate(scopes: [])
     if token.expired?
       return EXPIRED
diff --git a/app/services/ci/image_for_build_service.rb b/app/services/ci/image_for_build_service.rb
deleted file mode 100644
index 240ddabec36..00000000000
--- a/app/services/ci/image_for_build_service.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-module Ci
-  class ImageForBuildService
-    def execute(project, opts)
-      ref = opts[:ref]
-      sha = opts[:sha] || ref_sha(project, ref)
-      pipelines = project.pipelines.where(sha: sha)
-
-      image_name = image_for_status(pipelines.latest_status(ref))
-      image_path = Rails.root.join('public/ci', image_name)
-
-      OpenStruct.new(path: image_path, name: image_name)
-    end
-
-    private
-
-    def ref_sha(project, ref)
-      project.commit(ref).try(:sha) if ref
-    end
-
-    def image_for_status(status)
-      status ||= 'unknown'
-      'build-' + status + ".svg"
-    end
-  end
-end
diff --git a/app/services/ci/register_build_service.rb b/app/services/ci/register_build_service.rb
index 6f03bf2be13..5b52a0425de 100644
--- a/app/services/ci/register_build_service.rb
+++ b/app/services/ci/register_build_service.rb
@@ -20,21 +20,33 @@ module Ci
           builds_for_specific_runner
         end
 
-      build = builds.find do |build|
-        runner.can_pick?(build)
-      end
+      valid = true
 
-      if build
-        # In case when 2 runners try to assign the same build, second runner will be declined
-        # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method.
-        build.runner_id = runner.id
-        build.run!
-      end
+      builds.find do |build|
+        next unless runner.can_pick?(build)
+
+        begin
+          # In case when 2 runners try to assign the same build, second runner will be declined
+          # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method.
+          build.runner_id = runner.id
+          build.run!
 
-      Result.new(build, true)
+          return Result.new(build, true)
+        rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
+          # We are looping to find another build that is not conflicting
+          # It also indicates that this build can be picked and passed to runner.
+          # If we don't do it, basically a bunch of runners would be competing for a build
+          # and thus we will generate a lot of 409. This will increase
+          # the number of generated requests, also will reduce significantly
+          # how many builds can be picked by runner in a unit of time.
+          # In case we hit the concurrency-access lock,
+          # we still have to return 409 in the end,
+          # to make sure that this is properly handled by runner.
+          valid = false
+        end
+      end
 
-    rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
-      Result.new(build, false)
+      Result.new(nil, valid)
     end
 
     private
diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb
index 38ef323f6e5..89da05b72bb 100644
--- a/app/services/ci/retry_build_service.rb
+++ b/app/services/ci/retry_build_service.rb
@@ -1,18 +1,9 @@
 module Ci
   class RetryBuildService < ::BaseService
-    CLONE_ATTRIBUTES = %i[pipeline project ref tag options commands name
-                          allow_failure stage stage_idx trigger_request
-                          yaml_variables when environment coverage_regex]
-                            .freeze
-
-    REJECT_ATTRIBUTES = %i[id status user token coverage trace runner
-                           artifacts_expire_at artifacts_file
-                           artifacts_metadata artifacts_size
-                           created_at updated_at started_at finished_at
-                           queued_at erased_by erased_at].freeze
-
-    IGNORE_ATTRIBUTES = %i[type lock_version gl_project_id target_url
-                           deploy job_id description].freeze
+    CLONE_ACCESSORS = %i[pipeline project ref tag options commands name
+                         allow_failure stage stage_idx trigger_request
+                         yaml_variables when environment coverage_regex
+                         description tag_list].freeze
 
     def execute(build)
       reprocess(build).tap do |new_build|
@@ -31,7 +22,7 @@ module Ci
         raise Gitlab::Access::AccessDeniedError
       end
 
-      attributes = CLONE_ATTRIBUTES.map do |attribute|
+      attributes = CLONE_ACCESSORS.map do |attribute|
         [attribute, build.send(attribute)]
       end
 
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 25e22f14e60..1297a792259 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -1,16 +1,16 @@
 module Commits
   class ChangeService < ::BaseService
-    class ValidationError < StandardError; end
-    class ChangeError < StandardError; end
+    ValidationError = Class.new(StandardError)
+    ChangeError = Class.new(StandardError)
 
     def execute
       @start_project = params[:start_project] || @project
       @start_branch = params[:start_branch]
       @target_branch = params[:target_branch]
       @commit = params[:commit]
-      @create_merge_request = params[:create_merge_request].present?
 
-      check_push_permissions unless @create_merge_request
+      check_push_permissions
+
       commit
     rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError,
            ValidationError, ChangeError => ex
@@ -26,34 +26,21 @@ module Commits
     def commit_change(action)
       raise NotImplementedError unless repository.respond_to?(action)
 
-      if @create_merge_request
-        into = @commit.public_send("#{action}_branch_name")
-        tree_branch = @start_branch
-      else
-        into = tree_branch = @target_branch
-      end
-
-      tree_id = repository.public_send(
-        "check_#{action}_content", @commit, tree_branch)
-
-      if tree_id
-        validate_target_branch(into) if @create_merge_request
+      validate_target_branch if different_branch?
 
-        repository.public_send(
-          action,
-          current_user,
-          @commit,
-          into,
-          tree_id,
-          start_project: @start_project,
-          start_branch_name: @start_branch)
+      repository.public_send(
+        action,
+        current_user,
+        @commit,
+        @target_branch,
+        start_project: @start_project,
+        start_branch_name: @start_branch)
 
-        success
-      else
-        error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically.
+      success
+    rescue Repository::CreateTreeError
+      error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically.
                      A #{action.to_s.dasherize} may have already been performed with this #{@commit.change_type_title(current_user)}, or a more recent commit may have updated some of its content."
-        raise ChangeError, error_msg
-      end
+      raise ChangeError, error_msg
     end
 
     def check_push_permissions
@@ -66,16 +53,17 @@ module Commits
       true
     end
 
-    def validate_target_branch(new_branch)
-      # Temporary branch exists and contains the change commit
-      return if repository.find_branch(new_branch)
-
+    def validate_target_branch
       result = ValidateNewBranchService.new(@project, current_user)
-        .execute(new_branch)
+        .execute(@target_branch)
 
       if result[:status] == :error
         raise ChangeError, "There was an error creating the source branch: #{result[:message]}"
       end
     end
+
+    def different_branch?
+      @start_branch != @target_branch || @start_project != @project
+    end
   end
 end
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 0a25f56d24c..c8a60422bf4 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -1,6 +1,6 @@
 module Files
   class BaseService < ::BaseService
-    class ValidationError < StandardError; end
+    ValidationError = Class.new(StandardError)
 
     def execute
       @start_project = params[:start_project] || @project
@@ -58,16 +58,12 @@ module Files
         raise_error("You are not allowed to push into this branch")
       end
 
-      unless project.empty_repo?
-        unless @start_project.repository.branch_exists?(@start_branch)
-          raise_error('You can only create or edit files when you are on a branch')
-        end
+      if !@start_project.empty_repo? && !@start_project.repository.branch_exists?(@start_branch)
+        raise ValidationError, 'You can only create or edit files when you are on a branch'
+      end
 
-        if different_branch?
-          if repository.branch_exists?(@target_branch)
-            raise_error('Branch with such name already exists. You need to switch to this branch in order to make changes')
-          end
-        end
+      if !project.empty_repo? && different_branch? && repository.branch_exists?(@branch_name)
+        raise ValidationError, "A branch called #{@branch_name} already exists. Switch to that branch in order to make changes"
       end
     end
 
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index 0609c6219e7..700f9f4f6f0 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -1,6 +1,6 @@
 module Files
   class MultiService < Files::BaseService
-    class FileChangedError < StandardError; end
+    FileChangedError = Class.new(StandardError)
 
     ACTIONS = %w[create update delete move].freeze
 
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index 54e1aaf3f67..fbbab97632e 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -1,6 +1,6 @@
 module Files
   class UpdateService < Files::BaseService
-    class FileChangedError < StandardError; end
+    FileChangedError = Class.new(StandardError)
 
     def commit
       repository.update_file(current_user, @file_path, @file_content,
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 27bcc047601..ed6ea638235 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -56,12 +56,16 @@ class GitOperationService
     start_project: repository.project,
     &block)
 
-    check_with_branch_arguments!(
-      branch_name, start_branch_name, start_project)
+    start_repository = start_project.repository
+    start_branch_name = nil if start_repository.empty_repo?
+
+    if start_branch_name && !start_repository.branch_exists?(start_branch_name)
+      raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.path_with_namespace}"
+    end
 
     update_branch_with_hooks(branch_name) do
       repository.with_repo_branch_commit(
-        start_project.repository,
+        start_repository,
         start_branch_name || branch_name,
         &block)
     end
@@ -149,31 +153,4 @@ class GitOperationService
       repository.raw_repository.autocrlf = :input
     end
   end
-
-  def check_with_branch_arguments!(
-    branch_name, start_branch_name, start_project)
-    return if repository.branch_exists?(branch_name)
-
-    if repository.project != start_project
-      unless start_branch_name
-        raise ArgumentError,
-          'Should also pass :start_branch_name if' +
-          ' :start_project is different from current project'
-      end
-
-      unless start_project.repository.branch_exists?(start_branch_name)
-        raise ArgumentError,
-          "Cannot find branch #{branch_name} nor" \
-          " #{start_branch_name} from" \
-          " #{start_project.path_with_namespace}"
-      end
-    elsif start_branch_name
-      unless repository.branch_exists?(start_branch_name)
-        raise ArgumentError,
-          "Cannot find branch #{branch_name} nor" \
-          " #{start_branch_name} from" \
-          " #{repository.project.path_with_namespace}"
-      end
-    end
-  end
 end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index dcb4a96d16a..b071a398481 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -203,6 +203,7 @@ class IssuableBaseService < BaseService
     change_state(issuable)
     change_subscription(issuable)
     change_todo(issuable)
+    toggle_award(issuable)
     filter_params(issuable)
     old_labels = issuable.labels.to_a
     old_mentioned_users = issuable.mentioned_users.to_a
@@ -263,6 +264,14 @@ class IssuableBaseService < BaseService
     end
   end
 
+  def toggle_award(issuable)
+    award = params.delete(:emoji_award)
+    if award
+      todo_service.new_award_emoji(issuable, current_user)
+      issuable.toggle_award_emoji(award, current_user)
+    end
+  end
+
   def has_changes?(issuable, old_labels: [])
     valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch]
 
diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb
index a2a5f57d069..711f4035c55 100644
--- a/app/services/issues/move_service.rb
+++ b/app/services/issues/move_service.rb
@@ -1,6 +1,6 @@
 module Issues
   class MoveService < Issues::BaseService
-    class MoveError < StandardError; end
+    MoveError = Class.new(StandardError)
 
     def execute(issue, new_project)
       @old_issue = issue
diff --git a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb
index 5081dd5a0c4..aed5287940e 100644
--- a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb
+++ b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb
@@ -1,18 +1,18 @@
 module MergeRequests
   class MergeWhenPipelineSucceedsService < MergeRequests::BaseService
-    # Marks the passed `merge_request` to be merged when the build succeeds or
+    # Marks the passed `merge_request` to be merged when the pipeline succeeds or
     # updates the params for the automatic merge
     def execute(merge_request)
       merge_request.merge_params.merge!(params)
 
       # The service is also called when the merge params are updated.
-      already_approved = merge_request.merge_when_build_succeeds?
+      already_approved = merge_request.merge_when_pipeline_succeeds?
 
       unless already_approved
-        merge_request.merge_when_build_succeeds = true
-        merge_request.merge_user                = @current_user
+        merge_request.merge_when_pipeline_succeeds = true
+        merge_request.merge_user = @current_user
 
-        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
+        SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
       end
 
       merge_request.save
@@ -23,7 +23,7 @@ module MergeRequests
       return unless pipeline.success?
 
       pipeline_merge_requests(pipeline) do |merge_request|
-        next unless merge_request.merge_when_build_succeeds?
+        next unless merge_request.merge_when_pipeline_succeeds?
 
         unless merge_request.mergeable?
           todo_service.merge_request_became_unmergeable(merge_request)
@@ -36,9 +36,9 @@ module MergeRequests
 
     # Cancels the automatic merge
     def cancel(merge_request)
-      if merge_request.merge_when_build_succeeds? && merge_request.open?
-        merge_request.reset_merge_when_build_succeeds
-        SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user)
+      if merge_request.merge_when_pipeline_succeeds? && merge_request.open?
+        merge_request.reset_merge_when_pipeline_succeeds
+        SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user)
 
         success
       else
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 581d18032e6..1131d6f4913 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -11,7 +11,7 @@ module MergeRequests
       # empty diff during a manual merge
       close_merge_requests
       reload_merge_requests
-      reset_merge_when_build_succeeds
+      reset_merge_when_pipeline_succeeds
       mark_pending_todos_done
       cache_merge_requests_closing_issues
 
@@ -78,8 +78,8 @@ module MergeRequests
       end
     end
 
-    def reset_merge_when_build_succeeds
-      merge_requests_for_source_branch.each(&:reset_merge_when_build_succeeds)
+    def reset_merge_when_pipeline_succeeds
+      merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
     end
 
     def mark_pending_todos_done
diff --git a/app/services/merge_requests/resolve_service.rb b/app/services/merge_requests/resolve_service.rb
index d22a1d3e0ad..82cd89d9a0b 100644
--- a/app/services/merge_requests/resolve_service.rb
+++ b/app/services/merge_requests/resolve_service.rb
@@ -1,7 +1,6 @@
 module MergeRequests
   class ResolveService < MergeRequests::BaseService
-    class MissingFiles < Gitlab::Conflict::ResolutionError
-    end
+    MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
 
     attr_accessor :conflicts, :rugged, :merge_index, :merge_request
 
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index b4f8b33d564..61d66a26932 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -8,14 +8,6 @@ module Notes
       note.author  = current_user
       note.system  = false
 
-      if note.award_emoji?
-        noteable = note.noteable
-        if noteable.user_can_award?(current_user, note.award_emoji_name)
-          todo_service.new_award_emoji(noteable, current_user)
-          return noteable.create_award_emoji(note.award_emoji_name, current_user)
-        end
-      end
-
       # We execute commands (extracted from `params[:note]`) on the noteable
       # **before** we save the note because if the note consists of commands
       # only, there is no need be create a note!
@@ -48,7 +40,7 @@ module Notes
           note.errors.add(:commands_only, 'Commands applied')
         end
 
-        note.commands_changes = command_params.keys
+        note.commands_changes = command_params
       end
 
       note
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 3734e3c4253..fbad85d310e 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -135,7 +135,7 @@ class NotificationService
       merge_request.target_project,
       current_user,
       :merged_merge_request_email,
-      skip_current_user: !merge_request.merge_when_build_succeeds?
+      skip_current_user: !merge_request.merge_when_pipeline_succeeds?
     )
   end
 
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index 6dc3d8c2416..fbdaa455651 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -12,7 +12,7 @@ module Projects
       @project = Project.new(params)
 
       # Make sure that the user is allowed to use the specified visibility level
-      unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
+      unless Gitlab::VisibilityLevel.allowed_for?(current_user, @project.visibility_level)
         deny_visibility_level(@project)
         return @project
       end
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 2e06826c311..a7142d5950e 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -2,7 +2,7 @@ module Projects
   class DestroyService < BaseService
     include Gitlab::ShellAdapter
 
-    class DestroyError < StandardError; end
+    DestroyError = Class.new(StandardError)
 
     DELETED_FLAG = '+deleted'.freeze
 
diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb
index cd230528743..1c5a549feb9 100644
--- a/app/services/projects/import_service.rb
+++ b/app/services/projects/import_service.rb
@@ -2,7 +2,7 @@ module Projects
   class ImportService < BaseService
     include Gitlab::ShellAdapter
 
-    class Error < StandardError; end
+    Error = Class.new(StandardError)
 
     def execute
       add_repository_to_project unless project.gitlab_project_import?
diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb
index 20dfbddc823..da6e6acd4a7 100644
--- a/app/services/projects/transfer_service.rb
+++ b/app/services/projects/transfer_service.rb
@@ -9,7 +9,7 @@
 module Projects
   class TransferService < BaseService
     include Gitlab::ShellAdapter
-    class TransferError < StandardError; end
+    TransferError = Class.new(StandardError)
 
     def execute(new_namespace)
       if allowed_transfer?(current_user, project, new_namespace)
diff --git a/app/services/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb
index 3e0a85cf059..595653ea58a 100644
--- a/app/services/slash_commands/interpret_service.rb
+++ b/app/services/slash_commands/interpret_service.rb
@@ -59,7 +59,7 @@ module SlashCommands
       @updates[:state_event] = 'reopen'
     end
 
-    desc 'Merge (when build succeeds)'
+    desc 'Merge (when the pipeline succeeds)'
     condition do
       last_diff_sha = params && params[:merge_request_diff_head_sha]
       issuable.is_a?(MergeRequest) &&
@@ -255,6 +255,18 @@ module SlashCommands
       @updates[:wip_event] = issuable.work_in_progress? ? 'unwip' : 'wip'
     end
 
+    desc 'Toggle emoji reward'
+    params ':emoji:'
+    condition do
+      issuable.persisted?
+    end
+    command :award do |emoji|
+      name = award_emoji_name(emoji)
+      if name && issuable.user_can_award?(current_user, name)
+        @updates[:emoji_award] = name
+      end
+    end
+
     desc 'Set time estimate'
     params '<1w 3d 2h 14m>'
     condition do
@@ -329,5 +341,10 @@ module SlashCommands
 
       ext.references(type)
     end
+
+    def award_emoji_name(emoji)
+      match = emoji.match(Banzai::Filter::EmojiFilter.emoji_pattern)
+      match[1] if match
+    end
   end
 end
diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb
index 9b6dd013e3a..868fa7b3f21 100644
--- a/app/services/system_hooks_service.rb
+++ b/app/services/system_hooks_service.rb
@@ -84,7 +84,7 @@ class SystemHooksService
       project_id: model.id,
       owner_name: owner.name,
       owner_email: owner.respond_to?(:email) ? owner.email : "",
-      project_visibility: Project.visibility_levels.key(model.visibility_level_field).downcase
+      project_visibility: Project.visibility_levels.key(model.visibility_level_value).downcase
     }
   end
 
@@ -101,7 +101,7 @@ class SystemHooksService
       user_email:                   model.user.email,
       user_id:                      model.user.id,
       access_level:                 model.human_access,
-      project_visibility:           Project.visibility_levels.key(project.visibility_level_field).downcase
+      project_visibility:           Project.visibility_levels.key(project.visibility_level_value).downcase
     }
   end
 
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 55b548a12f9..8e02fe3741a 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -187,14 +187,14 @@ module SystemNoteService
   end
 
   # Called when 'merge when pipeline succeeds' is executed
-  def merge_when_build_succeeds(noteable, project, author, last_commit)
+  def merge_when_pipeline_succeeds(noteable, project, author, last_commit)
     body = "enabled an automatic merge when the pipeline for #{last_commit.to_reference(project)} succeeds"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
   # Called when 'merge when pipeline succeeds' is canceled
-  def cancel_merge_when_build_succeeds(noteable, project, author)
+  def cancel_merge_when_pipeline_succeeds(noteable, project, author)
     body = 'canceled the automatic merge'
 
     create_note(noteable: noteable, project: project, author: author, note: body)
@@ -385,7 +385,6 @@ module SystemNoteService
   # Returns Boolean
   def cross_reference_disallowed?(noteable, mentioner)
     return true if noteable.is_a?(ExternalIssue) && !noteable.project.jira_tracker_active?
-    return true if noteable.is_a?(Issuable) && (noteable.try(:closed?) || noteable.try(:merged?))
     return false unless mentioner.is_a?(MergeRequest)
     return false unless noteable.is_a?(Commit)
 
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index ad86b4f9f42..8787a1c93a9 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -103,7 +103,7 @@ class TodoService
   #
   def merge_request_build_failed(merge_request)
     create_build_failed_todo(merge_request, merge_request.author)
-    create_build_failed_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds?
+    create_build_failed_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
   end
 
   # When a new commit is pushed to a merge request we should:
@@ -121,7 +121,7 @@ class TodoService
   #
   def merge_request_build_retried(merge_request)
     mark_pending_todos_as_done(merge_request, merge_request.author)
-    mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds?
+    mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
   end
   
   # When a merge request could not be automatically merged due to its unmergeable state we should:
@@ -129,7 +129,7 @@ class TodoService
   #  * create a todo for a merge_user
   #
   def merge_request_became_unmergeable(merge_request)
-    create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds?
+    create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
   end
   
   # When create a note we should: