Merge branch 'master' into refactor-clusters

6 files changed, 101 insertions, 97 deletions

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index 9c00ea789ec..46e19230328 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -39,11 +39,8 @@ class AccessTokenValidationService
       token_scopes = token.scopes.map(&:to_sym)
 
       required_scopes.any? do |scope|
-        if scope.respond_to?(:sufficient?)
-          scope.sufficient?(token_scopes, request)
-        else
-          API::Scope.new(scope).sufficient?(token_scopes, request)
-        end
+        scope = API::Scope.new(scope) unless scope.is_a?(API::Scope)
+        scope.sufficient?(token_scopes, request)
       end
     end
   end
diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb
new file mode 100644
index 00000000000..92eaa5d5115
--- /dev/null
+++ b/app/services/issuable/common_system_notes_service.rb
@@ -0,0 +1,81 @@
+module Issuable
+  class CommonSystemNotesService < ::BaseService
+    attr_reader :issuable
+
+    def execute(issuable, old_labels)
+      @issuable = issuable
+
+      if issuable.previous_changes.include?('title')
+        create_title_change_note(issuable.previous_changes['title'].first)
+      end
+
+      handle_description_change_note
+
+      handle_time_tracking_note if issuable.is_a?(TimeTrackable)
+      create_labels_note(old_labels) if issuable.labels != old_labels
+      create_discussion_lock_note if issuable.previous_changes.include?('discussion_locked')
+      create_milestone_note if issuable.previous_changes.include?('milestone_id')
+    end
+
+    private
+
+    def handle_time_tracking_note
+      if issuable.previous_changes.include?('time_estimate')
+        create_time_estimate_note
+      end
+
+      if issuable.time_spent?
+        create_time_spent_note
+      end
+    end
+
+    def handle_description_change_note
+      if issuable.previous_changes.include?('description')
+        if issuable.tasks? && issuable.updated_tasks.any?
+          create_task_status_note
+        else
+          # TODO: Show this note if non-task content was modified.
+          # https://gitlab.com/gitlab-org/gitlab-ce/issues/33577
+          create_description_change_note
+        end
+      end
+    end
+
+    def create_labels_note(old_labels)
+      added_labels = issuable.labels - old_labels
+      removed_labels = old_labels - issuable.labels
+
+      SystemNoteService.change_label(issuable, issuable.project, current_user, added_labels, removed_labels)
+    end
+
+    def create_title_change_note(old_title)
+      SystemNoteService.change_title(issuable, issuable.project, current_user, old_title)
+    end
+
+    def create_description_change_note
+      SystemNoteService.change_description(issuable, issuable.project, current_user)
+    end
+
+    def create_task_status_note
+      issuable.updated_tasks.each do |task|
+        SystemNoteService.change_task_status(issuable, issuable.project, current_user, task)
+      end
+    end
+
+    def create_time_estimate_note
+      SystemNoteService.change_time_estimate(issuable, issuable.project, current_user)
+    end
+
+    def create_time_spent_note
+      SystemNoteService.change_time_spent(issuable, issuable.project, issuable.time_spent_user)
+    end
+
+    def create_milestone_note
+      SystemNoteService.change_milestone(issuable, issuable.project, current_user, issuable.milestone)
+    end
+
+    def create_discussion_lock_note
+      SystemNoteService.discussion_lock(issuable, current_user)
+    end
+  end
+end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index d61a342ebad..68b49d880f7 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -1,56 +1,10 @@
 class IssuableBaseService < BaseService
   private
 
-  def create_milestone_note(issuable)
-    SystemNoteService.change_milestone(
-      issuable, issuable.project, current_user, issuable.milestone)
-  end
-
-  def create_labels_note(issuable, old_labels)
-    added_labels = issuable.labels - old_labels
-    removed_labels = old_labels - issuable.labels
-
-    SystemNoteService.change_label(
-      issuable, issuable.project, current_user, added_labels, removed_labels)
-  end
-
-  def create_title_change_note(issuable, old_title)
-    SystemNoteService.change_title(
-      issuable, issuable.project, current_user, old_title)
-  end
-
-  def create_description_change_note(issuable)
-    SystemNoteService.change_description(issuable, issuable.project, current_user)
-  end
-
-  def create_branch_change_note(issuable, branch_type, old_branch, new_branch)
-    SystemNoteService.change_branch(
-      issuable, issuable.project, current_user, branch_type,
-      old_branch, new_branch)
-  end
-
-  def create_task_status_note(issuable)
-    issuable.updated_tasks.each do |task|
-      SystemNoteService.change_task_status(issuable, issuable.project, current_user, task)
-    end
-  end
-
-  def create_time_estimate_note(issuable)
-    SystemNoteService.change_time_estimate(issuable, issuable.project, current_user)
-  end
-
-  def create_time_spent_note(issuable)
-    SystemNoteService.change_time_spent(issuable, issuable.project, current_user)
-  end
-
-  def create_discussion_lock_note(issuable)
-    SystemNoteService.discussion_lock(issuable, current_user)
-  end
-
   def filter_params(issuable)
     ability_name = :"admin_#{issuable.to_ability_name}"
 
-    unless can?(current_user, ability_name, project)
+    unless can?(current_user, ability_name, issuable)
       params.delete(:milestone_id)
       params.delete(:labels)
       params.delete(:add_label_ids)
@@ -233,15 +187,14 @@ class IssuableBaseService < BaseService
 
       # We have to perform this check before saving the issuable as Rails resets
       # the changed fields upon calling #save.
-      update_project_counters = issuable.update_project_counter_caches?
+      update_project_counters = issuable.project && issuable.update_project_counter_caches?
 
       if issuable.with_transaction_returning_status { issuable.save }
         # We do not touch as it will affect a update on updated_at field
         ActiveRecord::Base.no_touching do
-          handle_common_system_notes(issuable, old_labels: old_labels)
+          Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels)
         end
 
-        change_discussion_lock(issuable)
         handle_changes(
           issuable,
           old_labels: old_labels,
@@ -300,12 +253,6 @@ class IssuableBaseService < BaseService
     end
   end
 
-  def change_discussion_lock(issuable)
-    if issuable.previous_changes.include?('discussion_locked')
-      create_discussion_lock_note(issuable)
-    end
-  end
-
   def toggle_award(issuable)
     award = params.delete(:emoji_award)
     if award
@@ -328,35 +275,17 @@ class IssuableBaseService < BaseService
     attrs_changed || labels_changed || assignees_changed
   end
 
-  def handle_common_system_notes(issuable, old_labels: [])
-    if issuable.previous_changes.include?('title')
-      create_title_change_note(issuable, issuable.previous_changes['title'].first)
-    end
-
-    if issuable.previous_changes.include?('description')
-      if issuable.tasks? && issuable.updated_tasks.any?
-        create_task_status_note(issuable)
-      else
-        # TODO: Show this note if non-task content was modified.
-        # https://gitlab.com/gitlab-org/gitlab-ce/issues/33577
-        create_description_change_note(issuable)
-      end
-    end
-
-    if issuable.previous_changes.include?('time_estimate')
-      create_time_estimate_note(issuable)
-    end
-
-    if issuable.time_spent?
-      create_time_spent_note(issuable)
-    end
-
-    create_labels_note(issuable, old_labels) if issuable.labels != old_labels
-  end
-
   def invalidate_cache_counts(issuable, users: [])
     users.each do |user|
       user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts") # rubocop:disable GitlabSecurity/PublicSend
     end
   end
+
+  # override if needed
+  def handle_changes(issuable, options)
+  end
+
+  # override if needed
+  def execute_hooks(issuable, action = 'open', params = {})
+  end
 end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index e0339ddf9bb..1b7b5927c5a 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -27,10 +27,6 @@ module Issues
         todo_service.update_issue(issue, current_user, old_mentioned_users)
       end
 
-      if issue.previous_changes.include?('milestone_id')
-        create_milestone_note(issue)
-      end
-
       if issue.assignees != old_assignees
         create_assignee_note(issue, old_assignees)
         notification_service.reassigned_issue(issue, current_user, old_assignees)
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 2832d893e95..1f394cacc64 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -40,10 +40,6 @@ module MergeRequests
                                   merge_request.target_branch)
       end
 
-      if merge_request.previous_changes.include?('milestone_id')
-        create_milestone_note(merge_request)
-      end
-
       if merge_request.previous_changes.include?('assignee_id')
         create_assignee_note(merge_request)
         notification_service.reassigned_merge_request(merge_request, current_user)
@@ -111,5 +107,11 @@ module MergeRequests
                          end
       end
     end
+
+    def create_branch_change_note(issuable, branch_type, old_branch, new_branch)
+      SystemNoteService.change_branch(
+        issuable, issuable.project, current_user, branch_type,
+        old_branch, new_branch)
+    end
   end
 end
diff --git a/app/services/metrics_service.rb b/app/services/metrics_service.rb
index a02eee4961b..6b3939aeba5 100644
--- a/app/services/metrics_service.rb
+++ b/app/services/metrics_service.rb
@@ -6,8 +6,7 @@ class MetricsService
     Gitlab::HealthChecks::Redis::RedisCheck,
     Gitlab::HealthChecks::Redis::CacheCheck,
     Gitlab::HealthChecks::Redis::QueuesCheck,
-    Gitlab::HealthChecks::Redis::SharedStateCheck,
-    Gitlab::HealthChecks::FsShardsCheck
+    Gitlab::HealthChecks::Redis::SharedStateCheck
   ].freeze
 
   def prometheus_metrics_text